From a3ec5d08696d18f6b4506ed2588698979a5b38f3 Mon Sep 17 00:00:00 2001 From: Lukas Friedrich Date: Tue, 1 Sep 2026 22:54:34 +0200 Subject: [PATCH] Audit round three: the traceback my last fix missed, and six smaller holes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The README's own demo still ended in a stack trace on a base install.** #22 moved evaluation inside the guarded block and listed the exceptions it knew about. `SynthIdUnavailableError` was not one of them — and the demo config declares `text_marking: synthid`, so the single command the README sends a newcomer to was the case the fix did not reach. Exit 1, which in this tool means "a rule failed". I found it by building a wheel and installing it into a fresh virtualenv rather than by reading, which is the only way that class of defect shows up. The three optional-dependency errors now share a base the CLI catches by type, so the next optional dependency cannot reintroduce it. Verified end to end on a clean install: exit 2, no traceback, and the install command survives intact — which it would not have before the Rich escaping in #34. **The manifest that decides a document verdict was recorded nowhere.** A reader could re-fetch the document, find its digest matching, and still not know which manifest produced the answer. The finding now carries the sidecar digest and how it was found, header or element. **MPF-M-001 passed without saying who signed.** The rulepack allows self-signed chains, so "validly signed" without "by whom" is half an answer. The signer's common name is in the finding now, and a reader decides what it is worth. **A `` in a comment or a script string was taken as the manifest pointer.** Anything that can put text on a page could choose the bytes that decide the verdict. Inert markup is stripped before the scan. **NOTICE and DISCLAIMER both claimed no component is named after a third-party mark.** False: `checks/synthid.py`, `checks/c2pa_verify.py`, the `[synthid]` extra and the `c2pa-verify` check type all are. The narrower parenthetical was true and the sentence around it was not. Both now say what is actually the case — the marks appear where they describe what the code does, which is nominative use, and nowhere as a product name or branding. Renaming them would make the code harder to read and the claim harder to check. **Nothing acknowledged that evidence can contain personal data.** Probes record responses verbatim and reports record the URLs probed. Now stated where the other scope limits are. **The determinism gate asserted nothing about which rules its goldens cover.** It checked that all four verdict values appear, which eleven cases exercising the same two rules would satisfy. Every shipped rule must now appear in some golden. Co-Authored-By: Claude Opus 5 --- NOTICE | 8 ++- docs/DISCLAIMER.md | 20 +++++-- src/markproof/checks/synthid.py | 3 +- src/markproof/cli.py | 8 ++- src/markproof/optional.py | 31 ++++++++++ src/markproof/probes/document.py | 10 +++- src/markproof/report/pdf_reportlab.py | 4 +- src/markproof/report/pdf_weasy.py | 3 +- src/markproof/rules/engine.py | 31 +++++++++- .../document-marked/expected_report.json | 9 ++- .../document-rewritten/expected_report.json | 9 ++- .../golden/media-marked/expected_report.json | 5 +- .../media-tampered/expected_report.json | 5 +- .../expected_report.json | 5 +- tests/golden/multi-probe/expected_report.json | 5 +- tests/test_cli.py | 59 +++++++++++++++++++ tests/test_determinism.py | 17 ++++++ tests/test_document_probe.py | 24 ++++++++ 18 files changed, 238 insertions(+), 18 deletions(-) create mode 100644 src/markproof/optional.py diff --git a/NOTICE b/NOTICE index 6b87c30..9f1f7e8 100644 --- a/NOTICE +++ b/NOTICE @@ -86,8 +86,12 @@ SynthID is a trademark of Google LLC / Google DeepMind. C2PA and Content Credentials are marks associated with the Coalition for Content Provenance and Authenticity and the Content Authenticity Initiative. These names appear here descriptively, to state what this tool verifies. No third-party logo is used in -this repository, its documentation or its marketing assets, and no component of -this project is named after a third-party mark. +this repository, its documentation or its marketing assets, and no third-party +mark is used as a product name, a package name or branding. The marks do appear +inside the code where they describe what it does -- the modules +markproof.checks.synthid and markproof.checks.c2pa_verify, the optional extra +[synthid], the check type c2pa-verify -- which is nominative use and not a claim +to the marks. See docs/DISCLAIMER.md. markproof performs technical conformance testing. It is not legal advice. See docs/DISCLAIMER.md. diff --git a/docs/DISCLAIMER.md b/docs/DISCLAIMER.md index c145603..6925e35 100644 --- a/docs/DISCLAIMER.md +++ b/docs/DISCLAIMER.md @@ -27,10 +27,16 @@ Consequently: - no third-party logo appears in this repository, the documentation site, the demo GIF or any social asset; -- no component, package or command of this project is named after a third-party - mark (no `synthid-*`, no `c2pa-*`); -- the project, the distribution and the command are all simply `markproof`, - maintained by Tippel. +- the project, the distribution, the command and the rulepack namespace are all + simply `markproof` / `MPF-*`, maintained by Tippel — no third-party mark is used + as a product name, a package name or branding; +- the marks do appear inside the code where they describe what it does: the module + `markproof.checks.synthid`, the module `markproof.checks.c2pa_verify`, the + optional extra `[synthid]` and the rulepack check type `c2pa-verify`. That is + nominative use — naming the technology being verified — and it is deliberate: + calling the SynthID check something else would make the code harder to read and + the claim harder to check. It is not an endorsement, an affiliation, or a claim + to the marks. ## Not legal advice @@ -73,6 +79,12 @@ than none: not part of v1; v1 validates manifest presence, hash bindings and the required assertions. Deepfake and emotion labels are checked for presence, not for prominence. +- **Evidence can contain personal data.** Probes record what an endpoint returned, + verbatim, and a report records the URLs probed. If a target's response includes + personal data — a name in a chat reply, an identifier in an error — it lands in + the evidence and, where a report is signed and shared, travels with it. markproof + does not inspect, redact or classify what it records. Point it at a test target, + or read what you are about to hand over. - **Applicability is recorded, not decided.** The `applicability` block in `markproof.yaml` is the operator's own statement about which Article 50 obligations bind the target. markproof writes it into the report and skips the diff --git a/src/markproof/checks/synthid.py b/src/markproof/checks/synthid.py index 7be45bd..e320346 100644 --- a/src/markproof/checks/synthid.py +++ b/src/markproof/checks/synthid.py @@ -35,6 +35,7 @@ from pydantic import BaseModel, ConfigDict, Field +from markproof.optional import OptionalDependencyError from markproof.rules.schema import SynthIdDetectCheck if TYPE_CHECKING: # pragma: no cover - import cost only matters at runtime @@ -53,7 +54,7 @@ ] -class SynthIdUnavailableError(RuntimeError): +class SynthIdUnavailableError(OptionalDependencyError): """The optional detection stack is not installed. ``transformers`` and ``torch`` are a heavy extra, so a rulepack asking for diff --git a/src/markproof/cli.py b/src/markproof/cli.py index fd4af1d..c553c79 100644 --- a/src/markproof/cli.py +++ b/src/markproof/cli.py @@ -49,6 +49,7 @@ UiProbeConfig, load_config, ) +from markproof.optional import OptionalDependencyError from markproof.probes.base import Evidence, ProbeError from markproof.probes.document import DocumentProbe from markproof.probes.http_chat import HttpChatProbe @@ -487,7 +488,12 @@ def run( # deserves a sentence, not a traceback. err_console.print(f"[bold red]error:[/] {_plain(exc)}") raise typer.Exit(code=2) from exc - except (ConfigurationRequiredError, UnsupportedCheckError, KeyError) as exc: + except ( + ConfigurationRequiredError, + OptionalDependencyError, + UnsupportedCheckError, + KeyError, + ) as exc: # Evaluation used to sit outside this block, so a rulepack asking for a # check this build cannot perform — the SynthID extra missing, most # commonly — reached the user as a traceback and exit 1. Exit 1 is this diff --git a/src/markproof/optional.py b/src/markproof/optional.py new file mode 100644 index 0000000..f609c14 --- /dev/null +++ b/src/markproof/optional.py @@ -0,0 +1,31 @@ +# SPDX-FileCopyrightText: 2026 Lukas Friedrich / Tippel +# SPDX-License-Identifier: Apache-2.0 +"""One base class for "this build cannot do that, and here is how to install it". + +markproof keeps its heavy parts optional on purpose: the default path has to work +on any runner with no system dependencies, so SynthID, the browser and both PDF +renderers arrive through extras. Each of them already raised a well-worded error +naming the extra to install. + +What was missing was a way for the CLI to *catch* them as a group. It listed the +exceptions it knew about, `SynthIdUnavailableError` was not among them, and so the +README's own demo command on a base install ended in a stack trace and exit 1 — +which in this tool means "a rule failed". A stranger's first contact was a +traceback answering the compliance question with a stack frame. + +Naming the shared case is what stops that recurring the next time an optional +dependency is added. +""" + +from __future__ import annotations + +__all__ = ["OptionalDependencyError"] + + +class OptionalDependencyError(RuntimeError): + """An optional extra this build does not have is needed to answer. + + Not a rule failure and not a bug: the run could not be performed. The CLI maps + it to exit 2 and prints the message, which is expected to name the install + command. + """ diff --git a/src/markproof/probes/document.py b/src/markproof/probes/document.py index ce6d1dc..47a9715 100644 --- a/src/markproof/probes/document.py +++ b/src/markproof/probes/document.py @@ -109,9 +109,17 @@ def manifest_link_from_header(value: str) -> str | None: return None +#: HTML comments and script bodies, removed before the element scan. A `` +#: written inside either is not a declaration the browser acts on, and taking one +#: as the document's manifest pointer lets anything that can put text on a page — +#: a comment, a code sample, a user-supplied string — choose the bytes that decide +#: the verdict. +_INERT = re.compile(r"|]*>.*?", re.IGNORECASE | re.DOTALL) + + def manifest_link_from_html(body: str) -> str | None: """The manifest URL advertised by a ```` element, if there is one.""" - for element in _LINK_ELEMENT.finditer(body): + for element in _LINK_ELEMENT.finditer(_INERT.sub("", body)): attrs = element.group("attrs") rel = _attr_value(_REL_ATTR.search(attrs)) if rel is None or _REL not in _rel_names(rel): diff --git a/src/markproof/report/pdf_reportlab.py b/src/markproof/report/pdf_reportlab.py index 56d3721..9141b02 100644 --- a/src/markproof/report/pdf_reportlab.py +++ b/src/markproof/report/pdf_reportlab.py @@ -84,6 +84,8 @@ from types import SimpleNamespace from typing import Any, Final +from markproof.optional import OptionalDependencyError + __all__ = [ "DISCLAIMER", "RESULT_ORDER", @@ -97,7 +99,7 @@ ] -class ReportlabUnavailableError(RuntimeError): +class ReportlabUnavailableError(OptionalDependencyError): """The ``[pdf]`` extra is not installed. Raised instead of an ``ImportError`` from three frames deep, so the CLI can diff --git a/src/markproof/report/pdf_weasy.py b/src/markproof/report/pdf_weasy.py index 85e417b..8b6390b 100644 --- a/src/markproof/report/pdf_weasy.py +++ b/src/markproof/report/pdf_weasy.py @@ -39,6 +39,7 @@ from pathlib import Path from typing import Any, Final +from markproof.optional import OptionalDependencyError from markproof.report.pdf_reportlab import ( DISCLAIMER, RESULT_ORDER, @@ -56,7 +57,7 @@ ] -class WeasyPrintUnavailableError(RuntimeError): +class WeasyPrintUnavailableError(OptionalDependencyError): """weasyprint, or one of its system libraries, could not be loaded. Deliberately distinct from the reportlab error so a caller can tell the two diff --git a/src/markproof/rules/engine.py b/src/markproof/rules/engine.py index 435bd87..a45c039 100644 --- a/src/markproof/rules/engine.py +++ b/src/markproof/rules/engine.py @@ -37,7 +37,7 @@ WatermarkConfig, detect_watermark, ) -from markproof.probes.base import Evidence +from markproof.probes.base import Evidence, sha256_hex from markproof.rules.schema import ( Applicability, C2paVerifyCheck, @@ -475,6 +475,12 @@ def _finding_from_c2pa(rule: Rule, evidence: Evidence, check: C2paVerifyCheck) - "outcome": "verified" if not failed else failed[0].outcome.value, "assets": sorted(r.artifact_id for r in results), } + signers = sorted({r.signer for r in results if r.signer}) + if signers: + # "Validly signed" is only half an answer without "by whom": the rulepack + # allows self-signed certificates, so a passing manifest may carry a + # signer nobody has ever heard of. Naming it lets a reader decide. + detail["signers"] = signers if failed: detail["failed_assets"] = sorted(r.artifact_id for r in failed) types = sorted({r.source_type for r in failed if r.source_type}) @@ -482,6 +488,29 @@ def _finding_from_c2pa(rule: Rule, evidence: Evidence, check: C2paVerifyCheck) - detail["declared_source_types"] = types hashes = tuple(a.sha256 for turn in evidence.turns for a in turn.artifacts) + # A sidecar manifest decides the verdict as much as the asset does, and it was + # recorded nowhere: a reader could re-fetch the document, find the digest + # matching, and still not know which manifest produced the answer. + sidecars = sorted( + { + sha256_hex(a.sidecar_manifest) + for turn in evidence.turns + for a in turn.artifacts + if a.sidecar_manifest is not None + } + ) + if sidecars: + detail["sidecar_manifests_sha256"] = sidecars + sources = sorted( + { + a.sidecar_source + for turn in evidence.turns + for a in turn.artifacts + if a.sidecar_source + } + ) + if sources: + detail["sidecar_source"] = ", ".join(sources) if not failed: noun = "asset" if len(results) == 1 else "assets" diff --git a/tests/golden/document-marked/expected_report.json b/tests/golden/document-marked/expected_report.json index 3201c57..fc28f1d 100644 --- a/tests/golden/document-marked/expected_report.json +++ b/tests/golden/document-marked/expected_report.json @@ -7,7 +7,14 @@ "document-fetch-document" ], "checked": 1, - "outcome": "verified" + "outcome": "verified", + "sidecar_manifests_sha256": [ + "61228703b5edc2c4e1831b116d27d189514a8fe6505016f764a8a2b99b51774c" + ], + "sidecar_source": "link-header", + "signers": [ + "markproof test fixtures" + ] }, "evidence_sha256": [ "8cc23ef8d1e33877c433382a4793455df5609c8615951526079a4f8e5fbea960" diff --git a/tests/golden/document-rewritten/expected_report.json b/tests/golden/document-rewritten/expected_report.json index 190f887..90c6486 100644 --- a/tests/golden/document-rewritten/expected_report.json +++ b/tests/golden/document-rewritten/expected_report.json @@ -10,7 +10,14 @@ "failed_assets": [ "document-fetch-document" ], - "outcome": "invalid" + "outcome": "invalid", + "sidecar_manifests_sha256": [ + "61228703b5edc2c4e1831b116d27d189514a8fe6505016f764a8a2b99b51774c" + ], + "sidecar_source": "link-header", + "signers": [ + "markproof test fixtures" + ] }, "evidence_sha256": [ "166c7e32179b19ee0a4f41168fa297a6f3673c72e9b1d12173bba98d6e1a11c3" diff --git a/tests/golden/media-marked/expected_report.json b/tests/golden/media-marked/expected_report.json index 7bf335e..379b53a 100644 --- a/tests/golden/media-marked/expected_report.json +++ b/tests/golden/media-marked/expected_report.json @@ -23,7 +23,10 @@ "media-generation-0" ], "checked": 1, - "outcome": "verified" + "outcome": "verified", + "signers": [ + "markproof test fixtures" + ] }, "evidence_sha256": [ "ac893cb331d2242a3ee3b920e91aa70e9c3d276053d19bd9eb93fdcbd61222b4" diff --git a/tests/golden/media-tampered/expected_report.json b/tests/golden/media-tampered/expected_report.json index 81b84bd..5b8a708 100644 --- a/tests/golden/media-tampered/expected_report.json +++ b/tests/golden/media-tampered/expected_report.json @@ -26,7 +26,10 @@ "failed_assets": [ "media-generation-0" ], - "outcome": "invalid" + "outcome": "invalid", + "signers": [ + "markproof test fixtures" + ] }, "evidence_sha256": [ "80b654dc96d4e96407ef826f0ce7bc247224a9fe9b7ecd823f9a46ff45ba5be0" diff --git a/tests/golden/media-wrong-source-type/expected_report.json b/tests/golden/media-wrong-source-type/expected_report.json index 77fd798..cf3a60a 100644 --- a/tests/golden/media-wrong-source-type/expected_report.json +++ b/tests/golden/media-wrong-source-type/expected_report.json @@ -29,7 +29,10 @@ "failed_assets": [ "media-generation-0" ], - "outcome": "wrong_source_type" + "outcome": "wrong_source_type", + "signers": [ + "markproof test fixtures" + ] }, "evidence_sha256": [ "8b57d660c94434380203d4448c47daa8eb3096e45db32339e8178c7fa9f48b65" diff --git a/tests/golden/multi-probe/expected_report.json b/tests/golden/multi-probe/expected_report.json index dbb5afe..57b9e64 100644 --- a/tests/golden/multi-probe/expected_report.json +++ b/tests/golden/multi-probe/expected_report.json @@ -114,7 +114,10 @@ "media-generation-0" ], "checked": 1, - "outcome": "verified" + "outcome": "verified", + "signers": [ + "markproof test fixtures" + ] }, "evidence_sha256": [ "ac893cb331d2242a3ee3b920e91aa70e9c3d276053d19bd9eb93fdcbd61222b4" diff --git a/tests/test_cli.py b/tests/test_cli.py index 18d61e5..291b825 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -596,3 +596,62 @@ def test_rules_list_does_not_eat_a_bracketed_title(self) -> None: # The shipped attribution contains "(EU) 2024/1689" and bracketed clause # references; none of it may vanish. assert "2024/1689" in result.output + + +class TestNoOptionalExtraEndsInATraceback: + """The README's own demo, on a base install, ended in a stack trace. + + #22 moved evaluation inside the guarded block and listed the exceptions it + knew about. `SynthIdUnavailableError` was not among them, so the fix did not + reach the one case a stranger actually hits: the demo config declares + `text_marking: synthid`, and a base install has no detector. + + Exit 1 is this tool's word for "a rule failed". Answering "is my endpoint + compliant?" with a stack frame and that exit code is the worst possible first + contact, and it survived the previous round because nothing tested it. + """ + + @respx.mock + def test_a_missing_detector_is_a_sentence_and_exit_two( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from markproof.checks import synthid + + def _absent(*_: object, **__: object) -> None: + raise synthid.SynthIdUnavailableError( + "text marking needs the optional extra: pip install 'markproof[synthid]'" + ) + + monkeypatch.setattr(synthid, "detect_watermark", _absent) + monkeypatch.setattr("markproof.rules.engine.detect_watermark", _absent) + + watermark = tmp_path / "wm.json" + watermark.write_text( + '{"ngram_len": 5, "keys": [1, 2, 3], "tokenizer": "t"}', encoding="utf-8" + ) + respx.post(_ENDPOINT).mock(return_value=_reply(_DISCLOSED * 12)) + config = _config( + tmp_path, + extra=f"text_marking:\n method: synthid\n watermark_config: {watermark}\n", + ) + result = RUNNER.invoke(app, ["run", "-c", str(config)]) + + assert "Traceback" not in result.output, result.output + assert result.exit_code == 2, ( + f"a run that could not happen must not exit 1: {result.output}" + ) + assert "markproof[synthid]" in result.output, "the install command was mangled" + + def test_every_optional_error_shares_a_base_the_cli_catches(self) -> None: + """Named so the next optional dependency cannot reintroduce this.""" + from markproof.checks.synthid import SynthIdUnavailableError + from markproof.optional import OptionalDependencyError + from markproof.report.pdf_reportlab import ReportlabUnavailableError + from markproof.report.pdf_weasy import WeasyPrintUnavailableError + + for error in ( + SynthIdUnavailableError, + ReportlabUnavailableError, + WeasyPrintUnavailableError, + ): + assert issubclass(error, OptionalDependencyError), error.__name__ diff --git a/tests/test_determinism.py b/tests/test_determinism.py index e125ce8..beb8f98 100644 --- a/tests/test_determinism.py +++ b/tests/test_determinism.py @@ -193,6 +193,23 @@ def test_a_failing_verdict_is_among_them(self) -> None: assert any("SKIP" in v for v in verdicts.values()), verdicts assert any("WARN" in v for v in verdicts.values()), verdicts + def test_every_shipped_rule_appears_in_some_golden(self) -> None: + """A rule no golden exercises is a rule the gate cannot protect. + + The gate asserted that all four verdict values appear somewhere, which + says nothing about coverage: eleven cases could all exercise the same two + rules. MPF-M-002 shipped and was in the goldens by luck of the case that + was written for it, not by a check. + """ + from markproof.rules.schema import load_rulepack + + packaged = Path(__file__).resolve().parent.parent / "src" / "markproof" / "rulepacks" + shipped = {r.id for r in load_rulepack(packaged / "art50-eu-2026.07.yaml").rules} + exercised = { + f["rule_id"] for case in _cases() for f in json.loads(_report_bytes(case))["findings"] + } + assert shipped <= exercised, f"no golden exercises: {sorted(shipped - exercised)}" + def test_no_model_in_the_report_graph_is_required_but_nullable(self) -> None: """The shape that made reports unreadable, guarded structurally. diff --git a/tests/test_document_probe.py b/tests/test_document_probe.py index 4972b59..0f8dc60 100644 --- a/tests/test_document_probe.py +++ b/tests/test_document_probe.py @@ -286,3 +286,27 @@ def test_an_empty_manifest_is_not_a_manifest(self) -> None: respx.get(_MANIFEST_URL).mock(return_value=httpx.Response(204)) with pytest.raises(ProbeError, match="HTTP 204"): _probe().collect() + + +class TestInertMarkupIsNotADeclaration: + """A `` a browser never acts on must not choose the verdict. + + Taking one from a comment or a script string lets anything that can put text + on a page — a code sample, a user-supplied string, a stale commented-out + header — point markproof at bytes of its choosing. + """ + + def test_a_link_in_a_comment_is_ignored(self) -> None: + assert ( + manifest_link_from_html('') is None + ) + + def test_a_link_in_a_script_string_is_ignored(self) -> None: + body = '' + assert manifest_link_from_html(body) is None + + def test_a_real_link_after_a_comment_is_still_found(self) -> None: + assert ( + manifest_link_from_html('') + == "m.c2pa" + )