Repository navigation
feat(sdk): add opt-in unprefixed manifest entry name - #405
Open
pflynn-virtru wants to merge 1 commit into
Open
pflynn-virtru wants to merge 1 commit into
pflynn-virtru wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 27.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 12 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: adding an opt-in unprefixed manifest entry name in the SDK. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the archive's name,
The spec path joins the file-name game.
The old path stays when options rest,
A payload reads and passes the test.
Then hops away, content in tow.
Comment @coderabbitai help to get the list of available commands.
pflynn-virtru
marked this pull request as ready for review
September 15, 2026 19:04
pflynn-virtru
force-pushed
the
fix/manifest-json-entry-name
branch
from
September 15, 2026 19:06
446d8ec to
9503fa7
Compare
pflynn-virtru
changed the title
Contributor
X-Test Failure Report |
Contributor
Contributor
pflynn-virtru
added a commit
that referenced
this pull request
Oct 1, 2026
Refs opentdf/platform#3513. The read half of #405, split out so it can land on its own. ## Problem The [OpenTDF spec](https://opentdf.io/spec#tdf-structure) names the manifest entry in a `.tdf` archive `manifest.json`: > The `manifest.json` file MUST be in JSON format and reside within the root of the OpenTDF Zip archive. This SDK writes and reads `0.manifest.json`. On the read side `TDFReader` does an exact `containsKey` with no fallback, so a TDF produced by any implementation written against the published spec is rejected with `tdf doesn't contain a manifest` before any schema check runs. `SDK.isTDF` carries its own copy of the literal, so such an archive is screened out one step earlier still. The `0.` prefix is a holdover from an early design that anticipated several payload/manifest pairs per archive. That design never shipped. ## Change **Read** — `TDFReader` prefers `manifest.json` and falls back to `0.manifest.json`. Preference matters rather than first-match: an archive carrying both must not have its conformant entry passed over for the superseded one. **Sniffing** — `SDK.isTDF` accepts either name, so it does not reject an archive the reader can now read. It also no longer requires the archive to hold *exactly* two entries. That check was scoped out at first as unrelated to the entry name, but review pointed out it is not: an archive carrying both manifest names holds three entries, and `prefersTheSpecNameWhenAnArchiveCarriesBoth` is precisely the case the reader now handles — so the sniffer would reject what the reader it screens for accepts. The count was never part of the structure `isTDF` describes; the spec fixes where the manifest lives, not what else the archive may hold. **Duplicate entry names** — a zip may legally list one name twice, and `Collectors.toMap` turned that into an `IllegalStateException` out of `TDFReader`'s constructor: outside its declared `throws SDKException, IOException`, outside what a caller screening untrusted input catches, and outside the fuzz targets' catch list. Dropping the count check above made it newly reachable through `isTDF`, so it is rejected here as an `IllegalArgumentException` alongside the constructor's other malformed-input paths. An archive carrying *both* manifest names is still read, not rejected. Those are two distinct entries and the spec settles which one wins; a single name listed twice offers no principled choice. Worth being explicit about the tradeoff, since the root signature covers only `integrityInformation.segments` — not `policy`, not `keyAccess[].url` — and no hash covers the entry name: a DEK holder can file two internally-valid manifests over one ciphertext and have this SDK enforce one while a `0.`-keyed peer enforces the other. That ambiguity predates this PR; preferring the spec name relocates which side Java lands on rather than creating it. Failing closed would be a cross-SDK decision, not a Java-only one. **Write is untouched.** `TDFWriter` still emits `0.manifest.json`, so this release produces byte-identical archives and every existing reader keeps working. The new `TDF_MANIFEST_FILE_NAME_SPEC` constant is read-side only. Flipping the writer is the breaking half and stays in #405. `0.payload` is untouched. It is recorded in the manifest's `payload.url`, so renaming it would alter manifest contents rather than just archive layout, and neither the spec nor opentdf/platform#4049 fixes it. ## Testing Ported from #405, minus the write-side test: | Test | Failure without the change | |---|---| | `TDFReaderTest.readsManifestUnderTheSpecName` | `IllegalArgumentException: tdf doesn't contain a manifest` | | `TDFReaderTest.readsThePayloadAlongsideASpecNamedManifest` | same | | `TDFReaderTest.prefersTheSpecNameWhenAnArchiveCarriesBoth` | returned the non-aligned manifest | | `SDKTest.testExaminingTDFWithSpecManifestName` | `false` | | `SDKTest.testExaminingTDFWithBothManifestNames` | `false` -- three entries | | `SDKTest.testExaminingTDFWithAnExtraEntry` | `false` -- three entries | | `TDFReaderTest.rejectsAnArchiveThatListsOneNameTwice` | `IllegalStateException: Duplicate key 0.payload` | | `TDFReaderTest.rejectsAnArchiveThatListsTheManifestNameTwice` | same, for `manifest.json` | `prefersTheSpecNameWhenAnArchiveCarriesBoth` files a *different* manifest under each name, so it cannot pass by reading whichever entry the reader happened to pick. Guarding behavior that must not change: `TDFReaderTest.readsManifestUnderTheOffspecName`, `TDFReaderTest.rejectsAnArchiveWithNoManifestUnderEitherName`, `SDKTest.testExaminingZipWithNoManifest`, `SDKTest.testExaminingZipWithNoPayload`, and the pre-existing `SDKTest.testExaminingValidZTDF` / `testExaminingManifest`, which run against the checked-in non-aligned-named `sample.txt.tdf` fixture. The two negative `isTDF` cases hold two entries each, so they failed for the name they were missing rather than for their entry count even before the count check came out. Review of this branch added four tests that pin rules nothing else held down: `rejectsNearMissManifestEntryNames` (a basename or case-insensitive lookup passed the suite before, so `sub/manifest.json` would have been read as the manifest despite the spec's root-only rule), `SDKTest.testExaminingLargerZipWithNoManifest` (both negatives held two entries, so "any zip over two entries is a TDF" passed), and the two duplicate-name cases above. **Verification:** Local Java execution was unavailable because this machine has no JDK or Maven. GitHub CI passed `mavenverify`, Platform Integration (including FIPS), and the full cross-SDK xtest matrix on the PR head; the previously failing matrix job passed on rerun. The new tests were also watched failing against unmodified production code on #405's branch, where the shared code is identical. ## Downstream impact None. Readers gain a name they accept and `isTDF` gains archive shapes it recognizes; nothing either previously accepted is taken away, and no archive this SDK writes changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Compatibility** * Improved support for TDF archives using the standard `manifest.json` entry name. * Continued support for archives using the SDK’s existing `0.manifest.json` entry name. * When both manifest names are present, the standard `manifest.json` entry is used. * Archives may include additional entries beyond the manifest and payload. * **Bug Fixes** * TDF detection now correctly recognizes valid archives and rejects those missing a manifest or payload. * Archives containing duplicate entry names are now rejected with a clear error. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com> Co-authored-by: Paul Flynn <pflynn-virtru@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
pflynn-virtru
force-pushed
the
fix/manifest-json-entry-name
branch
from
October 19, 2026 22:59
ce5f6f7 to
487dedc
Compare
pflynn-virtru
changed the title
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an opt-in that writes the TDF manifest under the zip entry name
manifest.json, the name the TDF spec defines, instead of the legacy0.manifest.json. Default output is unchanged.Config.withUnprefixedManifestName()and the cmdline flag--unprefixed-manifest-name.Config.withTargetModeoverrides the opt-in and keeps0.manifest.json. The override is resolved where the writer is built, so option order doesn't matter.TDFWriterconstructor that takes an entry name is package-private.This replaces the earlier version of this PR, which changed the default and was a breaking change. It mirrors opentdf/platform#4049 (Go) and opentdf/web-sdk#1040 (JS).
Testing
mvn -pl sdk,cmdline -am test: sdk 314, sdk-pqc-bc 28, cmdline 14, all passing.TDFWriterTest.writesTheManifestUnderTheGivenEntryName,TDFTest.unprefixedManifestNameRoundTrips,TDFTest.legacyTargetModeOverridesUnprefixedManifestName(both option orders),TDFTest.defaultManifestNameIsUnchanged,ConfigTest.withUnprefixedManifestName_shouldSetUnprefixedManifestName,CommandTest.encrypt_unprefixedManifestNameFlag_acceptedByPicocli.🤖 Generated with Claude Code
Summary by CodeRabbit
manifest.jsoninstead of the default0.manifest.json.manifest.jsonis an opt-in manifest name.