Repository navigation
fix(sdk): read the TDF manifest under the spec name manifest.json - #4063
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe TDF3 reader now checks ChangesManifest name compatibility
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Caller
participant TDFReader
participant ZIPArchive
Caller->>TDFReader: Request Manifest()
TDFReader->>ZIPArchive: Read manifest.json
ZIPArchive-->>TDFReader: Manifest or missing-entry error
TDFReader->>ZIPArchive: Read 0.manifest.json when manifest.json is missing
ZIPArchive-->>TDFReader: Manifest or error
TDFReader-->>Caller: Return manifest or error
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The reader now accepts specification-named manifests while retaining compatibility with existing files. No concrete merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 with care, Comment |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
Concrete downstream impact, in case it helps prioritise review: OpenTDFKit (the Swift SDK) moved its writer to the spec name Go-encrypt → Swift-decrypt passes, because the Swift reader already accepts both names. So the read-side fallback in this PR is the only thing between a spec-compliant writer and every released Go consumer. It is non-breaking and independent of the writer rename in #4049. @opentdf/go-sdk — could someone take a look? Happy to add a test vector (a |
The OpenTDF spec names the manifest entry in a .tdf archive manifest.json. This SDK looks it up as 0.manifest.json with an exact map lookup and no fallback, so a TDF produced by any implementation written against the published spec is rejected outright. TDFReader.Manifest() is the single choke point for both LoadTDF and IsValidTdf, so validation fails before any schema check runs. Accept either name on read, preferring manifest.json when an archive carries both. The fallback fires only on a missing entry, so an oversized manifest under the spec name stays a size failure rather than silently returning the superseded off-spec entry. The writer is unchanged and still emits 0.manifest.json. Renaming what we write is a file-format change that needs the other SDKs to accept the spec name first; this is the read half, which is backwards compatible on its own. The dual-name handling lives in tdf3_reader.go, not reader.go: Reader is a generic ZIP accessor whose other two lookups are payload paths that must stay exact. Refs #3513 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Two gaps from review of the manifest read fallback. Manifest() may now issue two ReadAllFileData calls against the shared io.ReadSeeker, which makes readBytes' absolute seek load-bearing in a way it was not when there was a single lookup. The spec-name validation test was copied from TestNew_ShouldValidateStandardTdf but dropped its repeat call, and the original no longer covers the two-lookup path: its fixture has no manifest.json, so it takes the miss path once. Restore the repeat call on the spec-name test. TestManifest_MissingUnderEitherName asserted only ErrorIs, which the unwrapped inner error satisfies on its own, so deleting the wrapping branch left every test green. Assert both names appear in the message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
067648c to
61e0aa4
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Reword comments and test descriptions that call the legacy 0.manifest.json entry "off-spec". Wording only; no behavior change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
Released otdfctl (v0.38.1) reads only 0.manifest.json and cannot open OpenTDFKit's spec-named manifest.json, so the upstream swift -> go leg has failed since #53. Build the go CLI from the #4063 merge commit (a36b5dfd), the first upstream SDK that reads manifest.json, until an otdfctl/* tag includes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BW3jJb6WzgesNADnPgGaef
Refs #3513. Read half only — carved out of #4049, which also renames what the writer emits.
Problem
The OpenTDF spec names the manifest entry in a
.tdfarchivemanifest.json. This SDK writes and reads0.manifest.json. On the read side it is an exact map lookup with no fallback, so a TDF produced by any implementation written against the published spec is rejected outright.TDFReader.Manifest()is the single choke point for bothLoadTDFandIsValidTdf, so validation fails before any schema check runs.Change
TDFReader.Manifest()accepts either name, preferringmanifest.jsonwhen an archive carries both. The fallback fires only on a missing entry — an oversized manifest under the spec name stays a size failure, rather than silently returning the superseded non-aligned entry.The dual-name handling lives in
tdf3_reader.go, notreader.go:Readeris a generic ZIP accessor whose other two lookups are payload paths that must stay exact.The writer is unchanged and still emits
0.manifest.json. That rename is a breaking file-format change that needs java-sdk, web-sdk, client-cpp and xtest to accept the spec name first; #4049 carries it. This PR is the read half, which is backwards compatible on its own and is the prerequisite for any of that work.Testing
Each behavioral test was verified to fail without the change by reverting
tdf3_reader.goand re-running:TestManifest_ReadsSpecNamezip: file not foundTestManifest_PrefersSpecNameOverOffspectext/plainwhereapplication/octet-streamexpectedTestManifest_OversizedSpecNameDoesNotFallBackAn error is expected but got nilTestIsValidTdf_AcceptsSpecManifestNametdfReader.Manifest failed: zip: file not foundTestManifest_PrefersSpecNameOverOffspecgives the two entries different content so it cannot pass vacuously.TestManifest_ReadsOffspecNameandTestManifest_MissingUnderEitherNameguard behavior that must not change, and pass either way.The
specNamedManifestTdffixture insdk_test.gowas generated from the existing non-aligned blob by re-zipping it under the spec entry name, so the two hold the same manifest and payload and differ only in how the manifest entry is named. The existing blob is untouched, which keeps a spec-named and a non-aligned-named archive both under test. Thezipstreamfixtures use deliberately incomplete manifest JSON because that package treats entry contents as opaque bytes; they verify ZIP name resolution, not schema validation.golangci-lint on the
sdkmodule: 3 issues, all pre-existing in files this PR does not touch (segment_writer.go,kas_client_test.go) — zero new.xtest is unaffected: it reads
0.manifest.json, which this SDK still writes.🤖 Generated with Claude Code
Known limitations
Surfaced by review of this branch and deliberately left out of scope. None is a regression from
mainexcept where noted.payload.urlfield is ignored (sdk: payload entry name is hardcoded to 0.payload; the spec's payload.url is never read #4065).ReadPayload/PayloadSizehardcode0.payload, andpayload.urlhas no read sites in the SDK — it is written atsdk/tdf.go:325andexperimental/tdf/writer.go:468and never read back. The spec is explicit that the payload filename is whateverurlsays, and that0.payloadis only a convention:schema/OpenTDF/README.mdsays "the filename within the archive is referenced by themanifest.json(commonly0.payload)",payload.mdcalls it "typically", and the normative JSON schema typesurlas an unconstrained string with nopattern,enumorconst. A spec-valid archive may therefore name its payload anything, and this SDK fails on the firstRead— after a KAS rewrap has already been spent. This is exercised in the wild: the community Rust SDKarkavo-org/opentdf-rswrites{index}.payloadper entry and setspayload.urlto match, so its multi-entry archives carry payloads named1.payload,2.payloadand so on. See sdk: payload entry name is hardcoded to 0.payload; the spec's payload.url is never read #4065 for the full survey.manifest.jsonshadows a valid0.manifest.json. The fallback keys onerrZipFileNotFound, and a zero-length entry is found, soManifest()returns an empty string with a nil error and the failure surfaces later as a JSON parse error. New with this PR, but it requires a malformed archive; preferring the spec entry unconditionally is the same policy as fix(sdk)!: name the TDF manifest entry manifest.json per spec #4049 and keeps the two PRs aligned.reader.gogaps that the new name makes easier to reach.ReadAllFileDatadoes not checkCompressionMethod, so a DEFLATE-compressed manifest returns raw compressed bytes with a nil error — reachable via exactly the "re-zip my TDF under the spec name" workflow this PR enables, since mainstream zip tools deflate by default. Separately, local file header names are never compared against the central directory, and duplicate entry names collapse last-wins. All three predate this PR and affect0.manifest.jsonidentically.One review finding did not survive checking:
IsValidTdfwas reported to accept any zip containing amanifest.json. It passesLax, notSkip(sdk.go:494), and a browser-extension zip is rejected withcould not validate manifest.json.Summary by CodeRabbit
manifest.jsonentry are now recognized and validated.0.manifest.jsonremain supported, preserving compatibility with previously created files.manifest.jsonentry takes precedence.