From 4a183f034c9cb11e6e47451497abcbdcfe161509 Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Tue, 15 Sep 2026 13:03:24 -0400 Subject: [PATCH 1/4] test(xtest)!: read the manifest entry as manifest.json, per spec The OpenTDF spec puts `manifest.json` at the root of the archive ("The manifest.json file MUST be in JSON format and reside within the root of the OpenTDF Zip archive" -- spec/schema/OpenTDF/manifest.md). xtest hardcoded `0.manifest.json` in four places, so the suite was asserting a container the spec does not describe. Every SDK agreed on the same wrong name, so nothing ever caught it. platform#4049 aligns the go writer. Its xct cells came back 93 failed, 5 passed, and 78 of those failures were xtest's own Python rather than anything wrong with the SDK: 46 `KeyError: There is no item named '0.manifest.json'` from `manifest()`/`validate_manifest_schema()`, and 32 `FileNotFoundError` from `update_manifest()` reaching into the extracted tree by name. Reads the spec name and no other. No fallback to the legacy name: a tolerant reader is what let the divergence survive unreported, and accepting both here would hide exactly the defect the suite exists to find. `manifest_entry_name()` centralizes the lookup and lists the archive's actual members on failure -- zipfile's own KeyError names only the entry it wanted, which made a renamed manifest read as an empty archive. `0.payload` is unchanged. It is not fixed by the spec: it is whatever the manifest's `payload.url` says, so renaming it would change manifest contents rather than archive layout. Adds `test_manifest_entry_name_is_spec_compliant`, ungated. A writer still emitting `0.manifest.json` fails it, and that red cell is the finding. Co-Authored-By: Claude Opus 5 (1M context) --- xtest/sizes.py | 15 +++--- xtest/tdfs.py | 53 ++++++++++++++++++--- xtest/test_tdfs.py | 36 +++++++++++++++ xtest/test_tdfs_units.py | 97 +++++++++++++++++++++++++++++++++++++-- xtest/test_zip64_units.py | 40 ++++++++-------- 5 files changed, 203 insertions(+), 38 deletions(-) diff --git a/xtest/sizes.py b/xtest/sizes.py index 32184049..7acf00f4 100644 --- a/xtest/sizes.py +++ b/xtest/sizes.py @@ -32,13 +32,14 @@ #: 2.1 GiB. Sits ~102 MiB inside the low edge of the broken window. #: -#: The margin is the point. A TDF writes ``0.payload`` first and -#: ``0.manifest.json`` after it, so the manifest's local-header offset is -#: roughly the payload size -- and that offset is the value under test. The -#: gap to 2**31 has to be wider than anything that could shift it: segment -#: padding, manifest length, per-entry header overhead. 102 MiB is not a -#: round number because it does not need to be; it needs to be unarguably -#: larger than those. +#: The margin is the point. A TDF writes ``0.payload`` first and the manifest +#: after it, so the manifest's local-header offset is roughly the payload size +#: -- and that offset is the value under test. The gap to 2**31 has to be wider +#: than anything that could shift it: segment padding, manifest length, +#: per-entry header overhead, and the two bytes by which the entry name itself +#: differs between ``manifest.json`` and the legacy ``0.manifest.json``. +#: 102 MiB is not a round number because it does not need to be; it needs to be +#: unarguably larger than those. #: #: Shrinking this below 2**31 does not make the test cheaper, it makes it #: vacuous -- every SDK takes the safe path and the test passes without diff --git a/xtest/tdfs.py b/xtest/tdfs.py index 8f3bf8e0..63abee1e 100644 --- a/xtest/tdfs.py +++ b/xtest/tdfs.py @@ -8,7 +8,7 @@ import urllib.parse import urllib.request import zipfile -from collections.abc import Callable +from collections.abc import Callable, Iterable from pathlib import Path from typing import Any, Literal, TypeIs, get_args @@ -476,9 +476,47 @@ class Manifest(BaseModel): ) +#: The manifest entry name given by the OpenTDF spec, which puts +#: ``manifest.json`` at the root of the archive. See ``spec/schema/OpenTDF``: +#: "The ``manifest.json`` file MUST be in JSON format and reside within the +#: root of the OpenTDF Zip archive." +#: +#: This suite reads and asserts this name and no other. The ``0.manifest.json`` +#: every SDK wrote until platform#4049 is not a second spelling to tolerate -- +#: it is a container the spec does not describe, and a reader that accepts it +#: is the reason the divergence survived four years unreported. An SDK still +#: writing the legacy name fails here, loudly, which is the intended report. +MANIFEST_ENTRY = "manifest.json" + +#: The payload entry name. Unlike the manifest's, this one is not fixed by the +#: spec -- it is whatever the manifest's ``payload.url`` says, and ``0.payload`` +#: is only the conventional value every SDK uses. Kept as a constant for the +#: fixtures and tamper helpers that have to name it. +PAYLOAD_ENTRY = "0.payload" + + +def manifest_entry_name(names: Iterable[str]) -> str: + """Check that an archive carries the spec's manifest entry, and name it. + + Returns :data:`MANIFEST_ENTRY` or raises. The indirection buys the error + message: ``zipfile`` reports only the name it wanted, so a container filed + under the legacy name reads as an empty archive -- every cell that hit it + said "There is no item named ..." without once saying what was in there. + Listing the actual members turns that into a one-line diagnosis. + """ + present = set(names) + if MANIFEST_ENTRY in present: + return MANIFEST_ENTRY + raise KeyError( + f"archive has no {MANIFEST_ENTRY!r} entry, which the OpenTDF spec " + f"requires at the archive root; it holds {sorted(present)}" + ) + + def manifest(tdf_file: Path) -> Manifest: with zipfile.ZipFile(tdf_file, "r") as tdfz: - with tdfz.open("0.manifest.json") as manifestEntry: + entry = manifest_entry_name(tdfz.namelist()) + with tdfz.open(entry) as manifestEntry: return Manifest.model_validate_json(manifestEntry.read()) @@ -491,11 +529,12 @@ def update_manifest( fname = tdf_file.stem unzipped_dir = tmp_dir / f"{fname}-{scenario_name}-unzipped" with zipfile.ZipFile(tdf_file, "r") as zipped: + entry = manifest_entry_name(zipped.namelist()) zipped.extractall(unzipped_dir) - with (unzipped_dir / "0.manifest.json").open("r") as manifest_file: + with (unzipped_dir / entry).open("r") as manifest_file: manifest_data = Manifest.model_validate_json(manifest_file.read()) new_manifest_data = manifest_change(manifest_data) - with (unzipped_dir / "0.manifest.json").open("w") as manifest_file: + with (unzipped_dir / entry).open("w") as manifest_file: # exclude_unset so the rewrite carries only what the original manifest # said plus whatever manifest_change touched. Without it every optional # field is re-emitted as an explicit null, and a reader that distinguishes @@ -528,10 +567,10 @@ def update_payload( unzipped_dir = tmp_dir / f"{fname}-{scenario_name}-unzipped" with zipfile.ZipFile(tdf_file, "r") as zipped: zipped.extractall(unzipped_dir) - with (unzipped_dir / "0.payload").open("rb") as payload_file: + with (unzipped_dir / PAYLOAD_ENTRY).open("rb") as payload_file: payload_data = payload_file.read() new_payload_data = payload_change(payload_data) - with (unzipped_dir / "0.payload").open("wb") as payload_file: + with (unzipped_dir / PAYLOAD_ENTRY).open("wb") as payload_file: payload_file.write(new_payload_data) outfile = tmp_dir / f"{fname}-{scenario_name}.tdf" with zipfile.ZipFile(outfile, "w") as zipped: @@ -641,7 +680,7 @@ def validate_manifest_schema(tdf_file: Path): ## Load the manifest file directly from the zipfile with zipfile.ZipFile(tdf_file, "r") as zipped: - with zipped.open("0.manifest.json") as manifest_file: + with zipped.open(manifest_entry_name(zipped.namelist())) as manifest_file: manifest = json.load(manifest_file) ## Validate diff --git a/xtest/test_tdfs.py b/xtest/test_tdfs.py index eb8b7fb2..749cde6e 100644 --- a/xtest/test_tdfs.py +++ b/xtest/test_tdfs.py @@ -4,6 +4,7 @@ import re import string import subprocess +import zipfile from pathlib import Path import pytest @@ -339,6 +340,41 @@ def test_manifest_validity_with_assertions( tdfs.validate_manifest_schema(ct_file) +def test_manifest_entry_name_is_spec_compliant( + encrypt_sdk: tdfs.SDK, + in_focus: set[tdfs.SDK], + attribute_default_rsa: Attribute, + encrypted_tdf: EncryptFactory, +): + """The manifest member is named ``manifest.json``, as the spec requires. + + Schema validity is not enough on its own: a manifest that satisfies + manifest.schema.json byte for byte is still unreadable by a spec-conformant + implementation if the archive files it under a name the spec never + mentions. That is the gap platform#3513 reports, and nothing in this suite + noticed it for four years, because every SDK agreed on the same wrong name + and xtest hardcoded it too. + + Deliberately ungated. A writer that still emits ``0.manifest.json`` fails + here, and that red cell is the finding, not a configuration problem to + route around -- java-sdk and web-sdk are expected to fail this until they + land their side of platform#4049. + """ + if not in_focus & {encrypt_sdk}: + pytest.skip("Not in focus") + ct_file = encrypted_tdf( + encrypt_sdk, + attr_values=attribute_default_rsa.value_fqns, + ) + + with zipfile.ZipFile(ct_file, "r") as z: + names = z.namelist() + assert tdfs.MANIFEST_ENTRY in names, ( + f"{encrypt_sdk} wrote {names}; the spec names the manifest entry " + f"{tdfs.MANIFEST_ENTRY}" + ) + + #### ASSERTION TESTS diff --git a/xtest/test_tdfs_units.py b/xtest/test_tdfs_units.py index e6d86ed7..4138a5cf 100644 --- a/xtest/test_tdfs_units.py +++ b/xtest/test_tdfs_units.py @@ -9,6 +9,12 @@ The GMAC root forgery (DSPX-4703) is here for a related reason: an exploit helper that quietly forges the wrong bytes would make the security tests in ``test_root_signature.py`` pass for the wrong reason. + +``manifest_entry_name`` (platform#3513) is here because it decides which +archive member the whole suite reads as the manifest, and the suite holds that +to the spec's ``manifest.json`` alone. The tests below pin the rejection of the +legacy ``0.manifest.json`` as much as the acceptance of the spec name: a +tolerant reader is what let the two diverge unreported for four years. """ import base64 @@ -90,7 +96,13 @@ def test_a_branch_build_is_not(self): # --- tdfs.elides_segment_sizes / tdfs.skip_chunky_skew ------------------------ -def _manifest_zip(tmp_path: Path, name: str, *, elides: bool) -> Path: +def _manifest_zip( + tmp_path: Path, + name: str, + *, + elides: bool, + manifest_entry: str = tdfs.MANIFEST_ENTRY, +) -> Path: """A minimal container with just enough manifest to exercise elides_segment_sizes.""" segments = [ { @@ -122,15 +134,15 @@ def _manifest_zip(tmp_path: Path, name: str, *, elides: bool) -> Path: }, "payload": { "type": "reference", - "url": "0.payload", + "url": tdfs.PAYLOAD_ENTRY, "protocol": "zip", "isEncrypted": True, }, } p = tmp_path / name with zipfile.ZipFile(p, "w") as z: - z.writestr("0.manifest.json", json.dumps(manifest)) - z.writestr("0.payload", b"") + z.writestr(tdfs.PAYLOAD_ENTRY, b"") + z.writestr(manifest_entry, json.dumps(manifest)) return p @@ -176,6 +188,83 @@ def test_does_not_skip_when_container_does_not_elide(self, tmp_path: Path): tdfs.skip_chunky_skew(ct_file, decrypt_sdk) +# --- tdfs.manifest_entry_name (platform#3513) --------------------------------- + + +class TestManifestEntryName: + """Which archive member holds the manifest. + + One answer only: the spec's ``manifest.json``. The ``0.manifest.json`` + every SDK wrote until platform#4049 is not a second spelling to tolerate, + and these tests exist to keep a fallback from creeping back in -- a reader + that quietly accepts both is what let the divergence go unreported for + four years. + """ + + def test_finds_the_spec_name(self): + assert ( + tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "manifest.json"]) + == "manifest.json" + ) + + def test_rejects_the_legacy_name(self): + """The whole point: ``0.manifest.json`` is a non-conformant container + and reading it anyway would hide exactly the defect under test.""" + with pytest.raises(KeyError, match="manifest.json"): + tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "0.manifest.json"]) + + def test_rejects_a_suffix_match(self): + """``0.manifest.json`` ends with the spec name, so the lookup must be + equality; an ``endswith`` test would readmit the legacy container.""" + with pytest.raises(KeyError): + tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "not-a-manifest.json"]) + + def test_the_error_reports_what_the_archive_held(self): + """``zipfile``'s own KeyError names only the entry it wanted, so the 46 + cells that hit this read as an empty archive. Naming the members turns + "there is no manifest.json" into "this writer used the legacy name".""" + with pytest.raises(KeyError) as exc: + tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "0.manifest.json"]) + assert "0.manifest.json" in str(exc.value) + assert tdfs.PAYLOAD_ENTRY in str(exc.value) + + +class TestManifestRequiresTheSpecEntry: + def test_manifest_parses_a_spec_named_container(self, tmp_path: Path): + ct_file = _manifest_zip(tmp_path, "m.tdf", elides=False) + assert tdfs.manifest(ct_file).payload.url == tdfs.PAYLOAD_ENTRY + + def test_manifest_rejects_a_legacy_named_container(self, tmp_path: Path): + ct_file = _manifest_zip( + tmp_path, "legacy.tdf", elides=False, manifest_entry="0.manifest.json" + ) + with pytest.raises(KeyError, match="manifest.json"): + tdfs.manifest(ct_file) + + def test_update_manifest_round_trips_under_the_spec_name(self, tmp_path: Path): + """A tamper fixture must differ from its source only in the bytes the + change touched, entry name included.""" + ct_file = _manifest_zip(tmp_path, "u.tdf", elides=False) + + def bump(m: tdfs.Manifest) -> tdfs.Manifest: + m.encryptionInformation.integrityInformation.rootSignature.sig = b"dGFtcGVy" + return m + + out = tdfs.update_manifest("tampered", ct_file, bump) + + with zipfile.ZipFile(out) as z: + assert tdfs.MANIFEST_ENTRY in z.namelist() + root = tdfs.manifest(out).encryptionInformation.integrityInformation + assert root.rootSignature.sig == b"dGFtcGVy" + + def test_update_manifest_rejects_a_legacy_named_container(self, tmp_path: Path): + ct_file = _manifest_zip( + tmp_path, "legacy.tdf", elides=False, manifest_entry="0.manifest.json" + ) + with pytest.raises(KeyError, match="manifest.json"): + tdfs.update_manifest("tampered", ct_file, lambda m: m) + + # --- tdfs.forge_gmac_root_signature (DSPX-4703) ------------------------------- diff --git a/xtest/test_zip64_units.py b/xtest/test_zip64_units.py index a935e294..05e03ad0 100644 --- a/xtest/test_zip64_units.py +++ b/xtest/test_zip64_units.py @@ -283,10 +283,10 @@ def test_reads_a_real_zip(self, tmp_path: Path): p = tmp_path / "ordinary.zip" with zipfile.ZipFile(p, "w") as z: z.writestr("0.payload", b"a" * 4096) - z.writestr("0.manifest.json", b"{}") + z.writestr("manifest.json", b"{}") cd = zipinspect.central_directory(p) - assert [e.name for e in cd.entries] == ["0.payload", "0.manifest.json"] + assert [e.name for e in cd.entries] == ["0.payload", "manifest.json"] with zipfile.ZipFile(p) as z: expected = {i.filename: i.header_offset for i in z.infolist()} assert {e.name: e.local_header_offset for e in cd.entries} == expected @@ -326,11 +326,11 @@ def test_reads_a_zip64_end_of_central_directory(self, tmp_path: Path): """ records = [ cen_record("0.payload", raw_offset=0), - cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES), + cen_record("manifest.json", raw_offset=MEDIUM_BYTES), ] p = synth_zip64_eocd(tmp_path / "z64-eocd.zip", records) cd = zipinspect.central_directory(p) - assert [e.name for e in cd.entries] == ["0.payload", "0.manifest.json"] + assert [e.name for e in cd.entries] == ["0.payload", "manifest.json"] assert cd.entries[1].raw_local_header_offset == MEDIUM_BYTES def test_rejects_a_locator_pointing_at_nothing(self, tmp_path: Path): @@ -354,7 +354,7 @@ def test_raw_value_in_the_window_is_preserved(self, tmp_path: Path): tmp_path / "window.zip", [ cen_record("0.payload", raw_offset=0, raw_usize=MEDIUM_BYTES), - cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES + 64), + cen_record("manifest.json", raw_offset=MEDIUM_BYTES + 64), ], ) manifest = zipinspect.central_directory(p).entries[1] @@ -372,7 +372,7 @@ def test_signed_read_of_a_windowed_offset_goes_negative(self, tmp_path: Path): """ p = synth_zip( tmp_path / "signed.zip", - [cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES)], + [cen_record("manifest.json", raw_offset=MEDIUM_BYTES)], ) (entry,) = zipinspect.central_directory(p).entries assert entry.signed_read_of_offset() < 0 @@ -382,7 +382,7 @@ def test_signed_read_is_harmless_below_the_window(self, tmp_path: Path): """Below 2**31 the two reads agree, which is why smaller payloads miss this.""" offset = ZIP64_WINDOW_LOW - 1 p = synth_zip( - tmp_path / "safe.zip", [cen_record("0.manifest.json", raw_offset=offset)] + tmp_path / "safe.zip", [cen_record("manifest.json", raw_offset=offset)] ) (entry,) = zipinspect.central_directory(p).entries assert entry.signed_read_of_offset() == offset @@ -394,7 +394,7 @@ def test_sentinel_resolves_through_the_extra_field(self, tmp_path: Path): tmp_path / "sentinel.zip", [ cen_record( - "0.manifest.json", + "manifest.json", raw_offset=ZIP64_SENTINEL_32, zip64_offset=true_offset, ) @@ -445,7 +445,7 @@ def test_skips_a_foreign_extra_field_record(self, tmp_path: Path): tmp_path / "foreign-extra.zip", [ cen_record( - "0.manifest.json", + "manifest.json", raw_offset=ZIP64_SENTINEL_32, zip64_offset=7 * 2**30, extra_prefix=timestamp, @@ -471,7 +471,7 @@ def test_rejects_a_sentinel_with_no_zip64_extra(self, tmp_path: Path): """ p = synth_zip( tmp_path / "sentinel-no-extra.zip", - [cen_record("0.manifest.json", raw_offset=ZIP64_SENTINEL_32)], + [cen_record("manifest.json", raw_offset=ZIP64_SENTINEL_32)], ) with pytest.raises(MalformedZipError, match="local header offset"): zipinspect.central_directory(p) @@ -492,7 +492,7 @@ def test_rejects_more_values_than_the_sentinels_call_for(self, tmp_path: Path): tmp_path / "over-long-extra.zip", [ cen_record( - "0.manifest.json", + "manifest.json", raw_offset=ZIP64_SENTINEL_32, zip64_usize=MEDIUM_BYTES, zip64_csize=MEDIUM_BYTES, @@ -539,7 +539,7 @@ def test_rejects_a_record_claiming_more_than_remains(self, tmp_path: Path): tmp_path / "overrun-extra.zip", [ cen_record( - "0.manifest.json", + "manifest.json", raw_offset=ZIP64_SENTINEL_32, zip64_offset=5 * 2**30, zip64_extra_len_override=64, @@ -559,7 +559,7 @@ def test_tolerates_the_trailing_disk_start_field(self, tmp_path: Path): tmp_path / "disk-start.zip", [ cen_record( - "0.manifest.json", + "manifest.json", raw_offset=ZIP64_SENTINEL_32, zip64_offset=5 * 2**30, zip64_disk_start=0, @@ -586,7 +586,7 @@ def test_truncated_eocd(self, tmp_path: Path): zipinspect.central_directory(p) def test_truncated_central_directory_record(self, tmp_path: Path): - record = cen_record("0.manifest.json", raw_offset=MEDIUM_BYTES) + record = cen_record("manifest.json", raw_offset=MEDIUM_BYTES) p = tmp_path / "short-cen.zip" cd = record[:20] eocd = ( @@ -601,7 +601,7 @@ def test_truncated_central_directory_record(self, tmp_path: Path): def test_central_directory_record_with_a_name_past_the_end(self, tmp_path: Path): """A plausible header whose variable-length fields overrun the buffer.""" - record = cen_record("0.manifest.json", raw_offset=0) + record = cen_record("manifest.json", raw_offset=0) # Claim a 4096-byte name where 15 bytes were written. record = record[:28] + struct.pack(" Date: Tue, 15 Sep 2026 13:09:23 -0400 Subject: [PATCH 2/4] docs(xtest): drop incident history from the manifest-entry comments Keep the normative rule and the reason a fallback is refused; the platform#4049 timeline and failure counts belong in the PR description, not in comments that outlive it. Co-Authored-By: Claude Opus 5 (1M context) --- xtest/sizes.py | 13 ++++++------ xtest/tdfs.py | 26 ++++++++++-------------- xtest/test_tdfs.py | 17 +++++++--------- xtest/test_tdfs_units.py | 43 ++++++++++++++++++---------------------- 4 files changed, 43 insertions(+), 56 deletions(-) diff --git a/xtest/sizes.py b/xtest/sizes.py index 7acf00f4..0c52906e 100644 --- a/xtest/sizes.py +++ b/xtest/sizes.py @@ -32,13 +32,12 @@ #: 2.1 GiB. Sits ~102 MiB inside the low edge of the broken window. #: -#: The margin is the point. A TDF writes ``0.payload`` first and the manifest -#: after it, so the manifest's local-header offset is roughly the payload size -#: -- and that offset is the value under test. The gap to 2**31 has to be wider -#: than anything that could shift it: segment padding, manifest length, -#: per-entry header overhead, and the two bytes by which the entry name itself -#: differs between ``manifest.json`` and the legacy ``0.manifest.json``. -#: 102 MiB is not a round number because it does not need to be; it needs to be +#: The margin is the point. A TDF writes ``0.payload`` first and +#: ``manifest.json`` after it, so the manifest's local-header offset is +#: roughly the payload size -- and that offset is the value under test. The +#: gap to 2**31 has to be wider than anything that could shift it: segment +#: padding, manifest length, per-entry header and name overhead. 102 MiB is +#: not a round number because it does not need to be; it needs to be #: unarguably larger than those. #: #: Shrinking this below 2**31 does not make the test cheaper, it makes it diff --git a/xtest/tdfs.py b/xtest/tdfs.py index 63abee1e..2250e83b 100644 --- a/xtest/tdfs.py +++ b/xtest/tdfs.py @@ -476,22 +476,19 @@ class Manifest(BaseModel): ) -#: The manifest entry name given by the OpenTDF spec, which puts -#: ``manifest.json`` at the root of the archive. See ``spec/schema/OpenTDF``: -#: "The ``manifest.json`` file MUST be in JSON format and reside within the -#: root of the OpenTDF Zip archive." +#: The manifest entry name required by the OpenTDF spec. From +#: ``spec/schema/OpenTDF/manifest.md``: "The ``manifest.json`` file MUST be in +#: JSON format and reside within the root of the OpenTDF Zip archive." #: -#: This suite reads and asserts this name and no other. The ``0.manifest.json`` -#: every SDK wrote until platform#4049 is not a second spelling to tolerate -- -#: it is a container the spec does not describe, and a reader that accepts it -#: is the reason the divergence survived four years unreported. An SDK still -#: writing the legacy name fails here, loudly, which is the intended report. +#: This suite reads and asserts this name and no other. Any other spelling is +#: a container the spec does not describe, and a reader that accepts one +#: cannot report that the writer produced it. MANIFEST_ENTRY = "manifest.json" #: The payload entry name. Unlike the manifest's, this one is not fixed by the #: spec -- it is whatever the manifest's ``payload.url`` says, and ``0.payload`` -#: is only the conventional value every SDK uses. Kept as a constant for the -#: fixtures and tamper helpers that have to name it. +#: is only the conventional value. Kept as a constant for the fixtures and +#: tamper helpers that have to name it. PAYLOAD_ENTRY = "0.payload" @@ -499,10 +496,9 @@ def manifest_entry_name(names: Iterable[str]) -> str: """Check that an archive carries the spec's manifest entry, and name it. Returns :data:`MANIFEST_ENTRY` or raises. The indirection buys the error - message: ``zipfile`` reports only the name it wanted, so a container filed - under the legacy name reads as an empty archive -- every cell that hit it - said "There is no item named ..." without once saying what was in there. - Listing the actual members turns that into a one-line diagnosis. + message: ``zipfile``'s own ``KeyError`` reports only the name it wanted, so + an archive that files the manifest elsewhere is indistinguishable from one + with no manifest at all. Listing the actual members says which it was. """ present = set(names) if MANIFEST_ENTRY in present: diff --git a/xtest/test_tdfs.py b/xtest/test_tdfs.py index 749cde6e..0acc4a04 100644 --- a/xtest/test_tdfs.py +++ b/xtest/test_tdfs.py @@ -348,17 +348,14 @@ def test_manifest_entry_name_is_spec_compliant( ): """The manifest member is named ``manifest.json``, as the spec requires. - Schema validity is not enough on its own: a manifest that satisfies - manifest.schema.json byte for byte is still unreadable by a spec-conformant - implementation if the archive files it under a name the spec never - mentions. That is the gap platform#3513 reports, and nothing in this suite - noticed it for four years, because every SDK agreed on the same wrong name - and xtest hardcoded it too. - - Deliberately ungated. A writer that still emits ``0.manifest.json`` fails + Distinct from ``test_manifest_validity``, which checks the manifest's + *contents* against manifest.schema.json: a manifest that satisfies the + schema byte for byte is still unreadable by a spec-conformant + implementation if the archive files it under some other name. + + Deliberately ungated. A writer that names the entry anything else fails here, and that red cell is the finding, not a configuration problem to - route around -- java-sdk and web-sdk are expected to fail this until they - land their side of platform#4049. + route around. """ if not in_focus & {encrypt_sdk}: pytest.skip("Not in focus") diff --git a/xtest/test_tdfs_units.py b/xtest/test_tdfs_units.py index 4138a5cf..4047673f 100644 --- a/xtest/test_tdfs_units.py +++ b/xtest/test_tdfs_units.py @@ -10,11 +10,10 @@ helper that quietly forges the wrong bytes would make the security tests in ``test_root_signature.py`` pass for the wrong reason. -``manifest_entry_name`` (platform#3513) is here because it decides which -archive member the whole suite reads as the manifest, and the suite holds that -to the spec's ``manifest.json`` alone. The tests below pin the rejection of the -legacy ``0.manifest.json`` as much as the acceptance of the spec name: a -tolerant reader is what let the two diverge unreported for four years. +``manifest_entry_name`` is here because it decides which archive member the +whole suite reads as the manifest, and holds that to the spec's +``manifest.json`` alone. Its tests pin the rejection of other spellings as +much as the acceptance of that one. """ import base64 @@ -188,17 +187,16 @@ def test_does_not_skip_when_container_does_not_elide(self, tmp_path: Path): tdfs.skip_chunky_skew(ct_file, decrypt_sdk) -# --- tdfs.manifest_entry_name (platform#3513) --------------------------------- +# --- tdfs.manifest_entry_name ------------------------------------------------- class TestManifestEntryName: - """Which archive member holds the manifest. + """Which archive member holds the manifest: ``manifest.json``, and no other. - One answer only: the spec's ``manifest.json``. The ``0.manifest.json`` - every SDK wrote until platform#4049 is not a second spelling to tolerate, - and these tests exist to keep a fallback from creeping back in -- a reader - that quietly accepts both is what let the divergence go unreported for - four years. + The rejection cases carry as much weight as the acceptance one. A reader + that also takes ``0.manifest.json`` reports a conformant archive and a + non-conformant one identically, which is the one thing this lookup must + not do. """ def test_finds_the_spec_name(self): @@ -207,22 +205,19 @@ def test_finds_the_spec_name(self): == "manifest.json" ) - def test_rejects_the_legacy_name(self): - """The whole point: ``0.manifest.json`` is a non-conformant container - and reading it anyway would hide exactly the defect under test.""" + def test_rejects_the_zero_prefixed_name(self): with pytest.raises(KeyError, match="manifest.json"): tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "0.manifest.json"]) def test_rejects_a_suffix_match(self): - """``0.manifest.json`` ends with the spec name, so the lookup must be - equality; an ``endswith`` test would readmit the legacy container.""" + """``0.manifest.json`` ends with the spec name, so the lookup has to be + equality; an ``endswith`` test would readmit it.""" with pytest.raises(KeyError): tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "not-a-manifest.json"]) def test_the_error_reports_what_the_archive_held(self): - """``zipfile``'s own KeyError names only the entry it wanted, so the 46 - cells that hit this read as an empty archive. Naming the members turns - "there is no manifest.json" into "this writer used the legacy name".""" + """Without the member list, a misnamed manifest and a missing one give + the same message.""" with pytest.raises(KeyError) as exc: tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "0.manifest.json"]) assert "0.manifest.json" in str(exc.value) @@ -234,9 +229,9 @@ def test_manifest_parses_a_spec_named_container(self, tmp_path: Path): ct_file = _manifest_zip(tmp_path, "m.tdf", elides=False) assert tdfs.manifest(ct_file).payload.url == tdfs.PAYLOAD_ENTRY - def test_manifest_rejects_a_legacy_named_container(self, tmp_path: Path): + def test_manifest_rejects_an_off_spec_named_container(self, tmp_path: Path): ct_file = _manifest_zip( - tmp_path, "legacy.tdf", elides=False, manifest_entry="0.manifest.json" + tmp_path, "off-spec.tdf", elides=False, manifest_entry="0.manifest.json" ) with pytest.raises(KeyError, match="manifest.json"): tdfs.manifest(ct_file) @@ -257,9 +252,9 @@ def bump(m: tdfs.Manifest) -> tdfs.Manifest: root = tdfs.manifest(out).encryptionInformation.integrityInformation assert root.rootSignature.sig == b"dGFtcGVy" - def test_update_manifest_rejects_a_legacy_named_container(self, tmp_path: Path): + def test_update_manifest_rejects_an_off_spec_named_container(self, tmp_path: Path): ct_file = _manifest_zip( - tmp_path, "legacy.tdf", elides=False, manifest_entry="0.manifest.json" + tmp_path, "off-spec.tdf", elides=False, manifest_entry="0.manifest.json" ) with pytest.raises(KeyError, match="manifest.json"): tdfs.update_manifest("tampered", ct_file, lambda m: m) From 3bf4546ed9fc4a195dd4688a0d5029d578f16273 Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Tue, 15 Sep 2026 13:14:16 -0400 Subject: [PATCH 3/4] revert(xtest): drop the PAYLOAD_ENTRY constant Out of scope. The payload entry name is not what platform#4049 changes, so update_payload and the fixtures keep their 0.payload literals. Co-Authored-By: Claude Opus 5 (1M context) --- xtest/sizes.py | 6 +++--- xtest/tdfs.py | 10 ++-------- xtest/test_tdfs_units.py | 17 ++++++++--------- 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/xtest/sizes.py b/xtest/sizes.py index 0c52906e..3f9ddaa1 100644 --- a/xtest/sizes.py +++ b/xtest/sizes.py @@ -36,9 +36,9 @@ #: ``manifest.json`` after it, so the manifest's local-header offset is #: roughly the payload size -- and that offset is the value under test. The #: gap to 2**31 has to be wider than anything that could shift it: segment -#: padding, manifest length, per-entry header and name overhead. 102 MiB is -#: not a round number because it does not need to be; it needs to be -#: unarguably larger than those. +#: padding, manifest length, per-entry header overhead. 102 MiB is not a +#: round number because it does not need to be; it needs to be unarguably +#: larger than those. #: #: Shrinking this below 2**31 does not make the test cheaper, it makes it #: vacuous -- every SDK takes the safe path and the test passes without diff --git a/xtest/tdfs.py b/xtest/tdfs.py index 2250e83b..c2f2f38d 100644 --- a/xtest/tdfs.py +++ b/xtest/tdfs.py @@ -485,12 +485,6 @@ class Manifest(BaseModel): #: cannot report that the writer produced it. MANIFEST_ENTRY = "manifest.json" -#: The payload entry name. Unlike the manifest's, this one is not fixed by the -#: spec -- it is whatever the manifest's ``payload.url`` says, and ``0.payload`` -#: is only the conventional value. Kept as a constant for the fixtures and -#: tamper helpers that have to name it. -PAYLOAD_ENTRY = "0.payload" - def manifest_entry_name(names: Iterable[str]) -> str: """Check that an archive carries the spec's manifest entry, and name it. @@ -563,10 +557,10 @@ def update_payload( unzipped_dir = tmp_dir / f"{fname}-{scenario_name}-unzipped" with zipfile.ZipFile(tdf_file, "r") as zipped: zipped.extractall(unzipped_dir) - with (unzipped_dir / PAYLOAD_ENTRY).open("rb") as payload_file: + with (unzipped_dir / "0.payload").open("rb") as payload_file: payload_data = payload_file.read() new_payload_data = payload_change(payload_data) - with (unzipped_dir / PAYLOAD_ENTRY).open("wb") as payload_file: + with (unzipped_dir / "0.payload").open("wb") as payload_file: payload_file.write(new_payload_data) outfile = tmp_dir / f"{fname}-{scenario_name}.tdf" with zipfile.ZipFile(outfile, "w") as zipped: diff --git a/xtest/test_tdfs_units.py b/xtest/test_tdfs_units.py index 4047673f..992b8048 100644 --- a/xtest/test_tdfs_units.py +++ b/xtest/test_tdfs_units.py @@ -133,15 +133,15 @@ def _manifest_zip( }, "payload": { "type": "reference", - "url": tdfs.PAYLOAD_ENTRY, + "url": "0.payload", "protocol": "zip", "isEncrypted": True, }, } p = tmp_path / name with zipfile.ZipFile(p, "w") as z: - z.writestr(tdfs.PAYLOAD_ENTRY, b"") z.writestr(manifest_entry, json.dumps(manifest)) + z.writestr("0.payload", b"") return p @@ -201,33 +201,32 @@ class TestManifestEntryName: def test_finds_the_spec_name(self): assert ( - tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "manifest.json"]) - == "manifest.json" + tdfs.manifest_entry_name(["0.payload", "manifest.json"]) == "manifest.json" ) def test_rejects_the_zero_prefixed_name(self): with pytest.raises(KeyError, match="manifest.json"): - tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "0.manifest.json"]) + tdfs.manifest_entry_name(["0.payload", "0.manifest.json"]) def test_rejects_a_suffix_match(self): """``0.manifest.json`` ends with the spec name, so the lookup has to be equality; an ``endswith`` test would readmit it.""" with pytest.raises(KeyError): - tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "not-a-manifest.json"]) + tdfs.manifest_entry_name(["0.payload", "not-a-manifest.json"]) def test_the_error_reports_what_the_archive_held(self): """Without the member list, a misnamed manifest and a missing one give the same message.""" with pytest.raises(KeyError) as exc: - tdfs.manifest_entry_name([tdfs.PAYLOAD_ENTRY, "0.manifest.json"]) + tdfs.manifest_entry_name(["0.payload", "0.manifest.json"]) assert "0.manifest.json" in str(exc.value) - assert tdfs.PAYLOAD_ENTRY in str(exc.value) + assert "0.payload" in str(exc.value) class TestManifestRequiresTheSpecEntry: def test_manifest_parses_a_spec_named_container(self, tmp_path: Path): ct_file = _manifest_zip(tmp_path, "m.tdf", elides=False) - assert tdfs.manifest(ct_file).payload.url == tdfs.PAYLOAD_ENTRY + assert tdfs.manifest(ct_file).payload.url == "0.payload" def test_manifest_rejects_an_off_spec_named_container(self, tmp_path: Path): ct_file = _manifest_zip( From 337ca078a19a42bb3b9b6161c6f23be13a71bd2a Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Tue, 15 Sep 2026 13:24:33 -0400 Subject: [PATCH 4/4] test(xtest): name the manifest entry from tdfs.MANIFEST_ENTRY in zip64 fixtures These fixtures model TDF layout deliberately -- payload first, manifest after it at the ZIP64 boundary -- so the entry should come from the same constant the readers use rather than a repeated literal. Also drops a byte count from the overrun test's comment. It said the name was 15 bytes, which stopped being true when the entry was renamed, and zipinspect never reads the name anyway. Co-Authored-By: Claude Opus 5 (1M context) --- xtest/test_zip64_units.py | 44 ++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/xtest/test_zip64_units.py b/xtest/test_zip64_units.py index 05e03ad0..02c1de01 100644 --- a/xtest/test_zip64_units.py +++ b/xtest/test_zip64_units.py @@ -20,6 +20,7 @@ import pytest import conftest +import tdfs import zipinspect from sizes import MEDIUM_BYTES, ZIP64_WINDOW_HIGH, ZIP64_WINDOW_LOW from zipinspect import ZIP64_SENTINEL_32, MalformedZipError @@ -283,10 +284,10 @@ def test_reads_a_real_zip(self, tmp_path: Path): p = tmp_path / "ordinary.zip" with zipfile.ZipFile(p, "w") as z: z.writestr("0.payload", b"a" * 4096) - z.writestr("manifest.json", b"{}") + z.writestr(tdfs.MANIFEST_ENTRY, b"{}") cd = zipinspect.central_directory(p) - assert [e.name for e in cd.entries] == ["0.payload", "manifest.json"] + assert [e.name for e in cd.entries] == ["0.payload", tdfs.MANIFEST_ENTRY] with zipfile.ZipFile(p) as z: expected = {i.filename: i.header_offset for i in z.infolist()} assert {e.name: e.local_header_offset for e in cd.entries} == expected @@ -326,11 +327,11 @@ def test_reads_a_zip64_end_of_central_directory(self, tmp_path: Path): """ records = [ cen_record("0.payload", raw_offset=0), - cen_record("manifest.json", raw_offset=MEDIUM_BYTES), + cen_record(tdfs.MANIFEST_ENTRY, raw_offset=MEDIUM_BYTES), ] p = synth_zip64_eocd(tmp_path / "z64-eocd.zip", records) cd = zipinspect.central_directory(p) - assert [e.name for e in cd.entries] == ["0.payload", "manifest.json"] + assert [e.name for e in cd.entries] == ["0.payload", tdfs.MANIFEST_ENTRY] assert cd.entries[1].raw_local_header_offset == MEDIUM_BYTES def test_rejects_a_locator_pointing_at_nothing(self, tmp_path: Path): @@ -354,7 +355,7 @@ def test_raw_value_in_the_window_is_preserved(self, tmp_path: Path): tmp_path / "window.zip", [ cen_record("0.payload", raw_offset=0, raw_usize=MEDIUM_BYTES), - cen_record("manifest.json", raw_offset=MEDIUM_BYTES + 64), + cen_record(tdfs.MANIFEST_ENTRY, raw_offset=MEDIUM_BYTES + 64), ], ) manifest = zipinspect.central_directory(p).entries[1] @@ -372,7 +373,7 @@ def test_signed_read_of_a_windowed_offset_goes_negative(self, tmp_path: Path): """ p = synth_zip( tmp_path / "signed.zip", - [cen_record("manifest.json", raw_offset=MEDIUM_BYTES)], + [cen_record(tdfs.MANIFEST_ENTRY, raw_offset=MEDIUM_BYTES)], ) (entry,) = zipinspect.central_directory(p).entries assert entry.signed_read_of_offset() < 0 @@ -382,7 +383,7 @@ def test_signed_read_is_harmless_below_the_window(self, tmp_path: Path): """Below 2**31 the two reads agree, which is why smaller payloads miss this.""" offset = ZIP64_WINDOW_LOW - 1 p = synth_zip( - tmp_path / "safe.zip", [cen_record("manifest.json", raw_offset=offset)] + tmp_path / "safe.zip", [cen_record(tdfs.MANIFEST_ENTRY, raw_offset=offset)] ) (entry,) = zipinspect.central_directory(p).entries assert entry.signed_read_of_offset() == offset @@ -394,7 +395,7 @@ def test_sentinel_resolves_through_the_extra_field(self, tmp_path: Path): tmp_path / "sentinel.zip", [ cen_record( - "manifest.json", + tdfs.MANIFEST_ENTRY, raw_offset=ZIP64_SENTINEL_32, zip64_offset=true_offset, ) @@ -445,7 +446,7 @@ def test_skips_a_foreign_extra_field_record(self, tmp_path: Path): tmp_path / "foreign-extra.zip", [ cen_record( - "manifest.json", + tdfs.MANIFEST_ENTRY, raw_offset=ZIP64_SENTINEL_32, zip64_offset=7 * 2**30, extra_prefix=timestamp, @@ -471,7 +472,7 @@ def test_rejects_a_sentinel_with_no_zip64_extra(self, tmp_path: Path): """ p = synth_zip( tmp_path / "sentinel-no-extra.zip", - [cen_record("manifest.json", raw_offset=ZIP64_SENTINEL_32)], + [cen_record(tdfs.MANIFEST_ENTRY, raw_offset=ZIP64_SENTINEL_32)], ) with pytest.raises(MalformedZipError, match="local header offset"): zipinspect.central_directory(p) @@ -492,7 +493,7 @@ def test_rejects_more_values_than_the_sentinels_call_for(self, tmp_path: Path): tmp_path / "over-long-extra.zip", [ cen_record( - "manifest.json", + tdfs.MANIFEST_ENTRY, raw_offset=ZIP64_SENTINEL_32, zip64_usize=MEDIUM_BYTES, zip64_csize=MEDIUM_BYTES, @@ -539,7 +540,7 @@ def test_rejects_a_record_claiming_more_than_remains(self, tmp_path: Path): tmp_path / "overrun-extra.zip", [ cen_record( - "manifest.json", + tdfs.MANIFEST_ENTRY, raw_offset=ZIP64_SENTINEL_32, zip64_offset=5 * 2**30, zip64_extra_len_override=64, @@ -559,7 +560,7 @@ def test_tolerates_the_trailing_disk_start_field(self, tmp_path: Path): tmp_path / "disk-start.zip", [ cen_record( - "manifest.json", + tdfs.MANIFEST_ENTRY, raw_offset=ZIP64_SENTINEL_32, zip64_offset=5 * 2**30, zip64_disk_start=0, @@ -586,7 +587,7 @@ def test_truncated_eocd(self, tmp_path: Path): zipinspect.central_directory(p) def test_truncated_central_directory_record(self, tmp_path: Path): - record = cen_record("manifest.json", raw_offset=MEDIUM_BYTES) + record = cen_record(tdfs.MANIFEST_ENTRY, raw_offset=MEDIUM_BYTES) p = tmp_path / "short-cen.zip" cd = record[:20] eocd = ( @@ -601,8 +602,8 @@ def test_truncated_central_directory_record(self, tmp_path: Path): def test_central_directory_record_with_a_name_past_the_end(self, tmp_path: Path): """A plausible header whose variable-length fields overrun the buffer.""" - record = cen_record("manifest.json", raw_offset=0) - # Claim a 4096-byte name where 15 bytes were written. + record = cen_record(tdfs.MANIFEST_ENTRY, raw_offset=0) + # Claim a 4096-byte name where only the entry name's bytes were written. record = record[:28] + struct.pack("