From 7714fca0aecb1cf7aa6056e0c6bf8bd13fcc22f2 Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Wed, 16 Sep 2026 13:36:16 -0400 Subject: [PATCH 1/5] fix(sdk): read a manifest.json entry from TDF archives The OpenTDF spec puts the manifest at the archive root under `manifest.json`. `TDFReader` looked the entry up by exact name with no fallback, so a TDF produced by any implementation written against the published spec was rejected with `tdf doesn't contain a manifest` before any schema check ran. `SDK.isTDF` carried its own copy of the literal and screened such archives out one step earlier. The reader now accepts either name, preferring `manifest.json` when an archive carries both so a conformant entry is never passed over for a superseded one. `SDK.isTDF` accepts either name too. Read side only: the writer still emits `0.manifest.json`. Changing that is a breaking file-format change and is left to a separate change. Refs https://github.com/opentdf/platform/issues/3513 Co-Authored-By: Claude Opus 5 Signed-off-by: Paul Flynn --- .../java/io/opentdf/platform/sdk/SDK.java | 10 +- .../io/opentdf/platform/sdk/TDFReader.java | 10 +- .../io/opentdf/platform/sdk/TDFWriter.java | 17 +++ .../java/io/opentdf/platform/sdk/SDKTest.java | 39 ++++++ .../opentdf/platform/sdk/TDFReaderTest.java | 119 ++++++++++++++++++ 5 files changed, 190 insertions(+), 5 deletions(-) create mode 100644 sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java index 5e903498..c1d72335 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java @@ -157,7 +157,10 @@ public Optional getSrtSigner() { * Checks to see if this has the structure of a Z-TDF in that it is a zip file * containing * a `manifest.json` and a `0.payload` - * + *

+ * The off-spec `0.manifest.json` that this SDK writes is also accepted, matching + * {@link TDFReader}. + * * @param channel A channel containing the bytes of the potential Z-TDF * @return `true` if */ @@ -172,8 +175,9 @@ public static boolean isTDF(SeekableByteChannel channel) { if (entries.size() != 2) { return false; } - return entries.stream().anyMatch(e -> "0.manifest.json".equals(e.getName())) - && entries.stream().anyMatch(e -> "0.payload".equals(e.getName())); + return entries.stream().anyMatch(e -> TDFWriter.TDF_MANIFEST_FILE_NAME_SPEC.equals(e.getName()) + || TDFWriter.TDF_MANIFEST_FILE_NAME.equals(e.getName())) + && entries.stream().anyMatch(e -> TDFWriter.TDF_PAYLOAD_FILE_NAME.equals(e.getName())); } /** diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index 6e9f32d2..d81bb8b7 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -9,6 +9,7 @@ import java.util.stream.Collectors; import static io.opentdf.platform.sdk.TDFWriter.TDF_MANIFEST_FILE_NAME; +import static io.opentdf.platform.sdk.TDFWriter.TDF_MANIFEST_FILE_NAME_SPEC; import static io.opentdf.platform.sdk.TDFWriter.TDF_PAYLOAD_FILE_NAME; /** @@ -26,14 +27,19 @@ public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { .stream() .collect(Collectors.toMap(ZipReader.Entry::getName, e -> e)); - if (!entries.containsKey(TDF_MANIFEST_FILE_NAME)) { + // The spec name wins over the off-spec one when an archive carries both, so a + // conformant entry is never passed over for a superseded one. + var manifest = entries.containsKey(TDF_MANIFEST_FILE_NAME_SPEC) + ? entries.get(TDF_MANIFEST_FILE_NAME_SPEC) + : entries.get(TDF_MANIFEST_FILE_NAME); + if (manifest == null) { throw new IllegalArgumentException("tdf doesn't contain a manifest"); } if (!entries.containsKey(TDF_PAYLOAD_FILE_NAME)) { throw new IllegalArgumentException("tdf doesn't contain a payload"); } - manifestEntry = entries.get(TDF_MANIFEST_FILE_NAME); + manifestEntry = manifest; payload = entries.get(TDF_PAYLOAD_FILE_NAME).getData(); } diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFWriter.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFWriter.java index 7137c232..93046f83 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFWriter.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFWriter.java @@ -10,7 +10,24 @@ */ public class TDFWriter { public static final String TDF_PAYLOAD_FILE_NAME = "0.payload"; + + /** + * The manifest entry name this SDK writes. The {@code 0.} prefix is a holdover from an + * early design that anticipated several payload/manifest pairs per archive and never + * shipped; the spec names the entry {@link #TDF_MANIFEST_FILE_NAME_SPEC}. Changing what + * the writer emits is a breaking file-format change and is left to a separate change. + * See platform#3513. + */ public static final String TDF_MANIFEST_FILE_NAME = "0.manifest.json"; + + /** + * The manifest entry name given by the OpenTDF spec: + * opentdf.io/spec. Read-side only -- + * {@link TDFReader} accepts it so archives from spec-conformant implementations can be + * read, and the writer does not yet emit it. + */ + public static final String TDF_MANIFEST_FILE_NAME_SPEC = "manifest.json"; + private final ZipWriter archiveWriter; public TDFWriter(OutputStream destination) { diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java index 289d2f42..1ec2afa3 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java @@ -10,9 +10,11 @@ import org.junit.jupiter.api.Test; import org.mockito.Mockito; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.nio.ByteBuffer; import java.nio.channels.SeekableByteChannel; +import java.nio.charset.StandardCharsets; import java.util.Random; import static org.assertj.core.api.Assertions.assertThat; @@ -29,6 +31,43 @@ void testExaminingValidZTDF() throws IOException { } } + /** + * The spec names the manifest entry {@code manifest.json}; the fixture above is an + * archive this SDK wrote, which names it {@code 0.manifest.json}. Both are recognized. + * See platform#3513. + */ + @Test + void testExaminingTDFWithSpecManifestName() throws IOException { + try (var chan = zipOf("0.payload", "manifest.json")) { + assertThat(SDK.isTDF(chan)).isTrue(); + } + } + + @Test + void testExaminingZipWithNoManifest() throws IOException { + try (var chan = zipOf("0.payload", "something-else")) { + assertThat(SDK.isTDF(chan)).isFalse(); + } + } + + @Test + void testExaminingZipWithNoPayload() throws IOException { + try (var chan = zipOf("manifest.json", "something-else")) { + assertThat(SDK.isTDF(chan)).isFalse(); + } + } + + /** Builds a zip holding the named entries; contents are irrelevant to {@link SDK#isTDF}. */ + private static SeekableInMemoryByteChannel zipOf(String... names) throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + for (var name : names) { + writer.data(name, name.getBytes(StandardCharsets.UTF_8)); + } + writer.finish(); + return new SeekableInMemoryByteChannel(out.toByteArray()); + } + @Test void testReadingProtocolClient() { var platformServicesClient = mock(ProtocolClient.class); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java new file mode 100644 index 00000000..f1dcef11 --- /dev/null +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -0,0 +1,119 @@ +package io.opentdf.platform.sdk; + +import org.apache.commons.compress.utils.SeekableInMemoryByteChannel; +import org.junit.jupiter.api.Test; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.LinkedHashMap; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * The OpenTDF spec names the manifest entry {@code manifest.json}; this SDK writes + * {@code 0.manifest.json}, so the reader accepts either. + * See platform#3513. + */ +public class TDFReaderTest { + + /** + * Carries the fields the TDF manifest schema requires, so the fixtures below are + * manifests rather than arbitrary JSON. {@link TDFReader#manifest()} hands back the + * bytes without parsing them, so the literal is spelled out here. + */ + private static String manifestJson(String mimeType) { + return "{\"payload\":{\"type\":\"reference\",\"url\":\"" + TDFWriter.TDF_PAYLOAD_FILE_NAME + + "\",\"protocol\":\"zip\",\"isEncrypted\":true,\"mimeType\":\"" + mimeType + + "\"},\"encryptionInformation\":{\"type\":\"split\"}}"; + } + + /** + * What nearly every fixture here stores. These tests exercise the entry name, not + * manifest contents, so the same manifest serves whichever name it is filed under. + */ + private static final String MANIFEST = manifestJson("application/octet-stream"); + + /** + * Exists only for the test that must tell the two entries apart -- with identical + * content, it could not say which one the reader returned. + */ + private static final String OTHER_MANIFEST = manifestJson("text/plain"); + + private static final String PAYLOAD = "payload bytes"; + + /** Builds a zip holding exactly the given entries, in iteration order. */ + private static SeekableInMemoryByteChannel archiveOf(Map entries) throws IOException { + var out = new ByteArrayOutputStream(); + var writer = new ZipWriter(out); + for (var entry : entries.entrySet()) { + writer.data(entry.getKey(), entry.getValue().getBytes(StandardCharsets.UTF_8)); + } + writer.finish(); + return new SeekableInMemoryByteChannel(out.toByteArray()); + } + + private static Map entries(String... namesAndContents) { + var entries = new LinkedHashMap(); + for (int i = 0; i < namesAndContents.length; i += 2) { + entries.put(namesAndContents[i], namesAndContents[i + 1]); + } + return entries; + } + + @Test + void readsManifestUnderTheSpecName() throws IOException { + try (var tdf = archiveOf(entries( + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "manifest.json", MANIFEST))) { + assertThat(new TDFReader(tdf).manifest()).isEqualTo(MANIFEST); + } + } + + @Test + void readsManifestUnderTheOffspecName() throws IOException { + try (var tdf = archiveOf(entries( + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "0.manifest.json", MANIFEST))) { + assertThat(new TDFReader(tdf).manifest()).isEqualTo(MANIFEST); + } + } + + /** + * The two entries hold different manifests, so this cannot pass by reading whichever + * one the reader happened to pick. + */ + @Test + void prefersTheSpecNameWhenAnArchiveCarriesBoth() throws IOException { + try (var tdf = archiveOf(entries( + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "0.manifest.json", OTHER_MANIFEST, + "manifest.json", MANIFEST))) { + assertThat(new TDFReader(tdf).manifest()).isEqualTo(MANIFEST); + } + } + + @Test + void rejectsAnArchiveWithNoManifestUnderEitherName() throws IOException { + try (var tdf = archiveOf(entries(TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD))) { + assertThatThrownBy(() -> new TDFReader(tdf)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("tdf doesn't contain a manifest"); + } + } + + @Test + void readsThePayloadAlongsideASpecNamedManifest() throws IOException { + try (var tdf = archiveOf(entries( + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "manifest.json", MANIFEST))) { + var reader = new TDFReader(tdf); + var buf = new byte[PAYLOAD.length()]; + + assertThat(reader.readPayloadBytes(buf)).isEqualTo(PAYLOAD.length()); + assertThat(new String(buf, StandardCharsets.UTF_8)).isEqualTo(PAYLOAD); + } + } +} From 842b7dad78a50f5d6eb3907abe0489a32600c204 Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Wed, 16 Sep 2026 13:54:33 -0400 Subject: [PATCH 2/5] refactor(sdk): pick the manifest entry with getOrDefault Sonar java:S9358 on the ternary around the two `entries.get` calls: the conditional belongs inside the operation. `getOrDefault` says the same thing in one lookup-shaped expression -- the spec name if present, the off-spec name otherwise -- and drops the separate `containsKey` probe. Entry values are never null, so the absent case is unambiguous. Co-Authored-By: Claude Opus 5 Signed-off-by: Paul Flynn --- sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index d81bb8b7..ad6071b8 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -29,9 +29,7 @@ public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { // The spec name wins over the off-spec one when an archive carries both, so a // conformant entry is never passed over for a superseded one. - var manifest = entries.containsKey(TDF_MANIFEST_FILE_NAME_SPEC) - ? entries.get(TDF_MANIFEST_FILE_NAME_SPEC) - : entries.get(TDF_MANIFEST_FILE_NAME); + var manifest = entries.getOrDefault(TDF_MANIFEST_FILE_NAME_SPEC, entries.get(TDF_MANIFEST_FILE_NAME)); if (manifest == null) { throw new IllegalArgumentException("tdf doesn't contain a manifest"); } From 90a88dacba2a07158cef36de9f9f87da49ab4e8d Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Wed, 16 Sep 2026 14:51:28 -0400 Subject: [PATCH 3/5] fix(sdk): stop isTDF rejecting archives by entry count isTDF required the archive to hold exactly two entries. An archive carrying both manifest names holds three, and TDFReader now reads it by preferring the spec name -- so the sniffer rejected what the reader it screens for accepts. The count also made isTDF stricter than the reader generally: the spec fixes where the manifest lives, not what else the archive may hold. Entry presence is what isTDF was checking for; the count was never part of the structure it describes. Co-Authored-By: Claude Opus 5 Signed-off-by: Paul Flynn --- .../main/java/io/opentdf/platform/sdk/SDK.java | 7 +++---- .../java/io/opentdf/platform/sdk/SDKTest.java | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java index c1d72335..2833d735 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java @@ -159,7 +159,9 @@ public Optional getSrtSigner() { * a `manifest.json` and a `0.payload` *

* The off-spec `0.manifest.json` that this SDK writes is also accepted, matching - * {@link TDFReader}. + * {@link TDFReader}. Entries beyond the manifest and payload are ignored rather than + * disqualifying: an archive carrying both manifest names holds three, and the reader + * accepts it, so a count check here would reject what the reader it screens for reads. * * @param channel A channel containing the bytes of the potential Z-TDF * @return `true` if @@ -172,9 +174,6 @@ public static boolean isTDF(SeekableByteChannel channel) { return false; } var entries = zipReader.getEntries(); - if (entries.size() != 2) { - return false; - } return entries.stream().anyMatch(e -> TDFWriter.TDF_MANIFEST_FILE_NAME_SPEC.equals(e.getName()) || TDFWriter.TDF_MANIFEST_FILE_NAME.equals(e.getName())) && entries.stream().anyMatch(e -> TDFWriter.TDF_PAYLOAD_FILE_NAME.equals(e.getName())); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java index 1ec2afa3..1ea86c57 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java @@ -43,6 +43,24 @@ void testExaminingTDFWithSpecManifestName() throws IOException { } } + /** + * The reader accepts an archive carrying both manifest names, so the sniffer that + * screens for it must not turn that third entry into a rejection. + */ + @Test + void testExaminingTDFWithBothManifestNames() throws IOException { + try (var chan = zipOf("0.payload", "manifest.json", "0.manifest.json")) { + assertThat(SDK.isTDF(chan)).isTrue(); + } + } + + @Test + void testExaminingTDFWithAnExtraEntry() throws IOException { + try (var chan = zipOf("0.payload", "manifest.json", "something-else")) { + assertThat(SDK.isTDF(chan)).isTrue(); + } + } + @Test void testExaminingZipWithNoManifest() throws IOException { try (var chan = zipOf("0.payload", "something-else")) { From 8cd97dc4c289d006ab6959bd2bd7e8f3c02107ab Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Thu, 17 Sep 2026 11:06:32 -0400 Subject: [PATCH 4/5] fix(sdk): reject a tdf that lists one entry name twice A zip may legally list the same name twice, and readers disagree about which copy wins. Collectors.toMap without a merge function turned that into IllegalStateException from TDFReader's constructor -- outside the `throws SDKException, IOException` it declares, outside what a caller screening untrusted input catches, and outside the fuzz targets' catch list, so it surfaced as an uncaught-exception finding rather than a rejected file. Dropping isTDF's entry-count check made the input newly reachable: a three-entry archive listing `0.payload` twice now passes the sniffer and lands in the constructor. Reject it, as IllegalArgumentException, alongside the constructor's other malformed-input paths. An archive carrying both manifest names stays readable -- those are distinct entries and the spec settles which wins; a name listed twice offers no principled choice. Review fixes to the tests added by this branch, in the same commit because the first one is what makes the case above expressible: - Collapse TDFReaderTest's entries()/archiveOf() pair into one varargs builder. The Map layer silently de-duplicated names, so a duplicate fixture could not be written, and its i += 2 loop threw on odd arity. - Pin the exact, rooted manifest match: `Manifest.json`, `sub/manifest.json` and `evil-manifest.json` are rejected. A basename or case-insensitive lookup passed the suite before. - Cover isTDF's widened rule with a 3-entry negative; both negatives held two entries, so "any zip over two entries is a TDF" passed. - Size the payload buffer from the UTF-8 encoding, not String.length(), and make it one byte long to catch a short read. - Drop the fixture javadoc's claim that the literal carries every field the manifest schema requires -- readManifest rejects it for null integrityInformation -- and the "fixture above" positional reference. Co-Authored-By: Claude Opus 5 Signed-off-by: Paul Flynn --- .../io/opentdf/platform/sdk/TDFReader.java | 14 ++- .../java/io/opentdf/platform/sdk/SDKTest.java | 17 ++- .../opentdf/platform/sdk/TDFReaderTest.java | 106 +++++++++++++----- 3 files changed, 103 insertions(+), 34 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java index ad6071b8..93548d67 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/TDFReader.java @@ -23,12 +23,20 @@ public class TDFReader { private final InputStream payload; public TDFReader(SeekableByteChannel tdf) throws SDKException, IOException { + // A zip may legally list the same name twice, and readers disagree about which copy + // wins, so reject rather than pick one. Without a merge function this collector throws + // IllegalStateException -- outside the constructor's declared error model, and outside + // what callers screening untrusted input catch. Map entries = new ZipReader(tdf).getEntries() .stream() - .collect(Collectors.toMap(ZipReader.Entry::getName, e -> e)); + .collect(Collectors.toMap(ZipReader.Entry::getName, e -> e, (first, second) -> { + throw new IllegalArgumentException("tdf contains more than one entry named " + first.getName()); + })); - // The spec name wins over the off-spec one when an archive carries both, so a - // conformant entry is never passed over for a superseded one. + // An archive carrying both names is read, not rejected, and the spec name wins, so a + // conformant entry is never passed over for a superseded one. Two entries under the + // two names are distinct entries -- unlike the duplicate above, where one name is + // listed twice and there is no principled way to choose. var manifest = entries.getOrDefault(TDF_MANIFEST_FILE_NAME_SPEC, entries.get(TDF_MANIFEST_FILE_NAME)); if (manifest == null) { throw new IllegalArgumentException("tdf doesn't contain a manifest"); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java index 1ea86c57..65a0bff2 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java @@ -32,8 +32,9 @@ void testExaminingValidZTDF() throws IOException { } /** - * The spec names the manifest entry {@code manifest.json}; the fixture above is an - * archive this SDK wrote, which names it {@code 0.manifest.json}. Both are recognized. + * The spec names the manifest entry {@code manifest.json}; the {@code sample.txt.tdf} + * fixture that {@link #testExaminingValidZTDF} reads is an archive this SDK wrote, which + * names it {@code 0.manifest.json}. Both are recognized. * See platform#3513. */ @Test @@ -75,6 +76,18 @@ void testExaminingZipWithNoPayload() throws IOException { } } + /** + * Dropping the entry-count check widened what counts as a Z-TDF, so the negative cases + * have to cover archives larger than two entries too -- otherwise "any zip with more + * than two entries" would pass this suite. + */ + @Test + void testExaminingLargerZipWithNoManifest() throws IOException { + try (var chan = zipOf("0.payload", "something-else", "and-another")) { + assertThat(SDK.isTDF(chan)).isFalse(); + } + } + /** Builds a zip holding the named entries; contents are irrelevant to {@link SDK#isTDF}. */ private static SeekableInMemoryByteChannel zipOf(String... names) throws IOException { var out = new ByteArrayOutputStream(); diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java index f1dcef11..119dc1a3 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/TDFReaderTest.java @@ -6,8 +6,6 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.util.LinkedHashMap; -import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -20,9 +18,11 @@ public class TDFReaderTest { /** - * Carries the fields the TDF manifest schema requires, so the fixtures below are - * manifests rather than arbitrary JSON. {@link TDFReader#manifest()} hands back the - * bytes without parsing them, so the literal is spelled out here. + * Manifest-shaped, but deliberately not schema-complete: {@link TDFReader#manifest()} + * hands back the entry's bytes without parsing them, and these tests are about which + * entry it picks. {@link Manifest#readManifest} would reject this literal -- it has no + * integrityInformation -- so anything that parses belongs in a test that builds a real + * manifest instead. */ private static String manifestJson(String mimeType) { return "{\"payload\":{\"type\":\"reference\",\"url\":\"" + TDFWriter.TDF_PAYLOAD_FILE_NAME @@ -44,39 +44,38 @@ private static String manifestJson(String mimeType) { private static final String PAYLOAD = "payload bytes"; - /** Builds a zip holding exactly the given entries, in iteration order. */ - private static SeekableInMemoryByteChannel archiveOf(Map entries) throws IOException { + /** + * Builds a zip holding exactly the given name/content pairs, in the order given. Writes + * each pair straight through, so a name may repeat -- that is a legal zip and one of the + * inputs under test. + */ + private static SeekableInMemoryByteChannel archiveOf(String... namesAndContents) throws IOException { + if (namesAndContents.length % 2 != 0) { + throw new IllegalArgumentException("expected name/content pairs"); + } var out = new ByteArrayOutputStream(); var writer = new ZipWriter(out); - for (var entry : entries.entrySet()) { - writer.data(entry.getKey(), entry.getValue().getBytes(StandardCharsets.UTF_8)); + for (int i = 0; i < namesAndContents.length; i += 2) { + writer.data(namesAndContents[i], namesAndContents[i + 1].getBytes(StandardCharsets.UTF_8)); } writer.finish(); return new SeekableInMemoryByteChannel(out.toByteArray()); } - private static Map entries(String... namesAndContents) { - var entries = new LinkedHashMap(); - for (int i = 0; i < namesAndContents.length; i += 2) { - entries.put(namesAndContents[i], namesAndContents[i + 1]); - } - return entries; - } - @Test void readsManifestUnderTheSpecName() throws IOException { - try (var tdf = archiveOf(entries( + try (var tdf = archiveOf( TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, - "manifest.json", MANIFEST))) { + "manifest.json", MANIFEST)) { assertThat(new TDFReader(tdf).manifest()).isEqualTo(MANIFEST); } } @Test void readsManifestUnderTheOffspecName() throws IOException { - try (var tdf = archiveOf(entries( + try (var tdf = archiveOf( TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, - "0.manifest.json", MANIFEST))) { + "0.manifest.json", MANIFEST)) { assertThat(new TDFReader(tdf).manifest()).isEqualTo(MANIFEST); } } @@ -87,33 +86,82 @@ void readsManifestUnderTheOffspecName() throws IOException { */ @Test void prefersTheSpecNameWhenAnArchiveCarriesBoth() throws IOException { - try (var tdf = archiveOf(entries( + try (var tdf = archiveOf( TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, "0.manifest.json", OTHER_MANIFEST, - "manifest.json", MANIFEST))) { + "manifest.json", MANIFEST)) { assertThat(new TDFReader(tdf).manifest()).isEqualTo(MANIFEST); } } @Test void rejectsAnArchiveWithNoManifestUnderEitherName() throws IOException { - try (var tdf = archiveOf(entries(TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD))) { + try (var tdf = archiveOf(TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD)) { + assertThatThrownBy(() -> new TDFReader(tdf)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("tdf doesn't contain a manifest"); + } + } + + /** + * The match is exact and rooted. The spec requires the manifest to "reside within the + * root of the OpenTDF Zip archive", so a nested or differently-cased entry is not it -- + * without this, a basename or case-insensitive match would look equally correct. + */ + @Test + void rejectsNearMissManifestEntryNames() throws IOException { + try (var tdf = archiveOf( + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "Manifest.json", MANIFEST, + "sub/manifest.json", MANIFEST, + "evil-manifest.json", MANIFEST)) { assertThatThrownBy(() -> new TDFReader(tdf)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("tdf doesn't contain a manifest"); } } + /** + * A zip may list one name twice; readers disagree about which copy wins, so this one + * refuses to choose. Distinct from an archive carrying both manifest names, which is + * read: there, the spec settles the choice. + */ + @Test + void rejectsAnArchiveThatListsOneNameTwice() throws IOException { + try (var tdf = archiveOf( + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "manifest.json", MANIFEST)) { + assertThatThrownBy(() -> new TDFReader(tdf)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("more than one entry named " + TDFWriter.TDF_PAYLOAD_FILE_NAME); + } + } + + @Test + void rejectsAnArchiveThatListsTheManifestNameTwice() throws IOException { + try (var tdf = archiveOf( + TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, + "manifest.json", MANIFEST, + "manifest.json", OTHER_MANIFEST)) { + assertThatThrownBy(() -> new TDFReader(tdf)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("more than one entry named manifest.json"); + } + } + + /** The buffer is one byte longer than the payload, so a short read would show up. */ @Test void readsThePayloadAlongsideASpecNamedManifest() throws IOException { - try (var tdf = archiveOf(entries( + var expected = PAYLOAD.getBytes(StandardCharsets.UTF_8); + try (var tdf = archiveOf( TDFWriter.TDF_PAYLOAD_FILE_NAME, PAYLOAD, - "manifest.json", MANIFEST))) { + "manifest.json", MANIFEST)) { var reader = new TDFReader(tdf); - var buf = new byte[PAYLOAD.length()]; + var buf = new byte[expected.length + 1]; - assertThat(reader.readPayloadBytes(buf)).isEqualTo(PAYLOAD.length()); - assertThat(new String(buf, StandardCharsets.UTF_8)).isEqualTo(PAYLOAD); + assertThat(reader.readPayloadBytes(buf)).isEqualTo(expected.length); + assertThat(buf).startsWith(expected); } } } From 2d52b614f981bc63a7f209b74a64265eabf27b99 Mon Sep 17 00:00:00 2001 From: Paul Flynn Date: Fri, 18 Sep 2026 08:47:00 -0400 Subject: [PATCH 5/5] fix(sdk): reject duplicate entry names from isTDF TDFReader refuses an archive that lists one name twice rather than guess which copy wins -- a zip's central directory carries no uniqueness constraint, and readers disagree about whether the first or the last record wins, so the same bytes can present two different manifests to two implementations. isTDF checked only that the required names were present. An archive with a valid manifest and payload plus any repeated name cleared the sniffer and then took an IllegalArgumentException from the read the sniffer exists to gate. Reject repeated names before the presence checks. An archive carrying both manifest names still passes: three distinct names, and the spec settles which one wins. Both new tests cover it -- a repeated payload name, and a repeated unrelated name. The second pins that any repeated name disqualifies, not just a repeated manifest or payload, matching what the reader rejects. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Paul Flynn --- .../java/io/opentdf/platform/sdk/SDK.java | 14 ++++++++++--- .../java/io/opentdf/platform/sdk/SDKTest.java | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java index 2833d735..3d03d58c 100644 --- a/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java +++ b/sdk/src/main/java/io/opentdf/platform/sdk/SDK.java @@ -162,6 +162,10 @@ public Optional getSrtSigner() { * {@link TDFReader}. Entries beyond the manifest and payload are ignored rather than * disqualifying: an archive carrying both manifest names holds three, and the reader * accepts it, so a count check here would reject what the reader it screens for reads. + *

+ * An archive that lists one name twice is rejected, again matching {@link TDFReader}: + * passing it here and then failing the read would hand callers an exception from the + * very check this method exists to spare them. * * @param channel A channel containing the bytes of the potential Z-TDF * @return `true` if @@ -174,9 +178,13 @@ public static boolean isTDF(SeekableByteChannel channel) { return false; } var entries = zipReader.getEntries(); - return entries.stream().anyMatch(e -> TDFWriter.TDF_MANIFEST_FILE_NAME_SPEC.equals(e.getName()) - || TDFWriter.TDF_MANIFEST_FILE_NAME.equals(e.getName())) - && entries.stream().anyMatch(e -> TDFWriter.TDF_PAYLOAD_FILE_NAME.equals(e.getName())); + var names = entries.stream().map(ZipReader.Entry::getName).collect(Collectors.toSet()); + if (names.size() != entries.size()) { + return false; + } + return (names.contains(TDFWriter.TDF_MANIFEST_FILE_NAME_SPEC) + || names.contains(TDFWriter.TDF_MANIFEST_FILE_NAME)) + && names.contains(TDFWriter.TDF_PAYLOAD_FILE_NAME); } /** diff --git a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java index 65a0bff2..bd099a00 100644 --- a/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java +++ b/sdk/src/test/java/io/opentdf/platform/sdk/SDKTest.java @@ -88,6 +88,26 @@ void testExaminingLargerZipWithNoManifest() throws IOException { } } + /** + * {@link TDFReader} refuses an archive that lists one name twice rather than guess which + * copy wins, so the sniffer that screens for it has to refuse the same archive -- otherwise + * a caller clears {@link SDK#isTDF} and then takes an exception on the read it gated. + */ + @Test + void testExaminingZipThatListsThePayloadNameTwice() throws IOException { + try (var chan = zipOf("0.payload", "0.payload", "manifest.json")) { + assertThat(SDK.isTDF(chan)).isFalse(); + } + } + + /** The reader rejects any repeated name, not just a repeated manifest or payload. */ + @Test + void testExaminingZipThatListsAnUnrelatedNameTwice() throws IOException { + try (var chan = zipOf("0.payload", "manifest.json", "something-else", "something-else")) { + assertThat(SDK.isTDF(chan)).isFalse(); + } + } + /** Builds a zip holding the named entries; contents are irrelevant to {@link SDK#isTDF}. */ private static SeekableInMemoryByteChannel zipOf(String... names) throws IOException { var out = new ByteArrayOutputStream();