Repository navigation
fix(sdk): report a malformed manifest as an invalid file - #1044
Open
pflynn-virtru wants to merge 11 commits into
Open
pflynn-virtru wants to merge 11 commits into
pflynn-virtru wants to merge 11 commits into
Conversation
The OpenTDF spec names the manifest entry `manifest.json` at the archive root. This SDK looks it up as `0.manifest.json` by exact central-directory match with no fallback, so a TDF produced by any implementation written against the published spec is rejected outright with `Unable to retrieve CD manifest`. Resolve the entry through `manifestEntryName()`, which prefers the spec name and falls back to the off-spec `0.manifest.json` this SDK writes. Read side only: `writeStream` still emits `0.manifest.json`, so the file format this SDK produces is unchanged and nothing downstream breaks. Renaming what the writer emits is a breaking change, tracked separately. Resolution runs against the central directory, not against a failed read. That keeps other failures honest: an oversized manifest under the spec name stays a size error rather than silently yielding the off-spec entry. Refs opentdf/platform#3513. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Review of the first cut found the test named for this change's central
design decision did not pin it, and that none of the coverage survived
the follow-up writer flip.
The oversized-manifest test composed `manifestEntryName` and
`getManifest` itself instead of going through a call site, so it only
re-asserted ZipReader's size guard — already covered in zip.spec.ts.
Rewriting `loadTDFStream` as `try { spec } catch { off-spec }`, the exact
design the implementation comment rejects, left all ten tests green.
Replace it with an archive that carries a valid off-spec manifest plus an
oversized spec-named central-directory record shadowing it, and assert
`decrypt` and `getPolicyId` both fail on size. That reader now fails two
tests.
Reading `0.manifest.json` was covered only incidentally, because the
writer still emits it; flipping the writer left the off-spec name
untested. Generate both fixtures from one table so each name is exercised
explicitly under either writer.
`renameManifestEntry` matched only the off-spec name, so after that flip
it would have become an identity rebuild and the guard test asserting the
fixture was really renamed would have passed vacuously. It now throws
unless exactly one manifest entry matched.
Also: compare the policy id to the original archive's rather than
asserting it is a string; slice stored bytes with `compressedSize`; pin
the missing-manifest error; drop the file comment's claim to cover
spec-conforming producers generally, which the fixtures cannot support.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Code review found the two roles collapsed into one constant. Reading `0.manifest.json` and writing it are different decisions, but `offspecManifestFileName` served both, and its docstring invited exactly the edit — repoint it at the spec name — that would have left `manifestEntryName` returning the spec name on both branches and made every archive this SDK has ever written unreadable. Give the writer its own binding, `manifestEntryNameToWrite`. Flipping it is now the whole of the writer change and cannot disturb the read side. The guard against that mistake was the writer tripwire, and it was blind: it asserted against `offspecManifestFileName` rather than the literal, as did every other assertion in the suite, so a suite built out of those constants could not notice them changing. Collapsing the constant used to leave the tripwire green while the writer emitted the wrong name; the spec now spells both names out and that mutation fails five tests, including the tripwire and the whole off-spec read group. Also: `Client.getPolicyId` was a statement-for-statement copy of `loadTDFStream`, which it can just call — the duplication is why this branch had to make the same change in two files; drop `entry()`'s unused `overrides` parameter; correct `renameManifestEntry`'s docstring, which promised a rename its guard does not check for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughTDF reading now supports both ChangesTDF manifest reading
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Archive
participant loadTDFStream
participant manifestEntryName
participant Client
Archive->>loadTDFStream: supplies TDF archive
loadTDFStream->>manifestEntryName: passes central directory
manifestEntryName-->>loadTDFStream: returns selected manifest entry name
loadTDFStream-->>Client: returns loaded manifest
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change supports both manifest names while preserving legacy writes and reports malformed manifests as invalid files. No merge-blocking issue is established; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Invalid archive entries are rejected more consistently, and no new authentication bypass was demonstrated. Policy identifiers remain unverified metadata, however, and whether callers rely on them for authorization is not established. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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’s name, Comment |
Refs opentdf/platform#3513. Follow-up to #1042, stacked on it. `getManifest` resolves the manifest entry by name and returns `JSON.parse` of whatever it finds, so a `manifest.json` holding something else surfaces as a raw `TypeError` or `SyntaxError` — outside the `TdfError` hierarchy, and missed by callers catching it. Wrap the parse and gate on `isManifest`. The guard is deliberately weak: it rejects only what every caller would crash on. `getPolicyId` needs just `encryptionInformation.policy`, so demanding `keyAccess` or `integrityInformation` would narrow a reader #1042 just widened. An earlier revision did demand them; the `carrying only what its caller reads` test caught it. 22 tests added to the #1042 suite. Mutation-tested: isManifest always true (behavior before this commit) 9 failing guard widened to demand keyAccess + integrityInfo 1 failing JSON.parse error rethrown raw 4 failing $ cd lib && npm run test:mocha # 447 passing, 0 failing $ cd lib && npm run test:wtr # 253 passed, 15 files $ cd lib && npm run lint # clean $ cd lib && npm run license-check # clean Not run: cli/web-app suites (untouched), Playwright and xtest (need a live platform). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
pflynn-virtru
force-pushed
the
fix/validate-manifest-shape
branch
from
September 18, 2026 14:30
8d5f797 to
d9eaa4d
Compare
X-Test Failure Reportopentdf-sdk-lib |
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
# Conflicts: # lib/tdf3/src/utils/zip-reader.ts
pflynn-virtru
marked this pull request as ready for review
September 23, 2026 15:01
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>
|
This branch has not been deployed
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.
Refs opentdf/platform#3513. Follow-up to #1042, stacked on it — retarget to
mainonce that lands.Problem
ZipReader.getManifestresolves the manifest entry by name and returnsJSON.parseof whatever it finds. Nothing establishes the entry holds a manifest, so the first dereference downstream reports the problem — from outside theTdfErrorhierarchy, and missed by callers catching it.Against an SDK-written archive with an intact
0.manifest.jsonplus amanifest.jsonthat isn't a manifest:manifest.jsondecrypt/getPolicyId{"name":"my-web-app"}TypeError: Cannot read properties of undefinedhelloSyntaxError: Unexpected token 'h'SyntaxError: Unexpected end of JSON inputinstanceof InvalidFileError === falsein every case. All three decrypt on a non-aligned-only reader, so this is strictly the cost of widening the lookup. Reachable before #1042 by planting0.manifest.json; #1042 makes it reachable under a second name.Change
getManifestwraps the parse and gates on a newisManifest.The guard is deliberately weak. #1042 widened the reader; a guard demanding every declared field would reject what that change set out to accept.
getPolicyIdreadsencryptionInformation.policyand nothing else, so requiringkeyAccessorintegrityInformationwould fail manifests it serves fine. It rejects only what every caller would crash on — a non-object, or one with noencryptionInformation.An earlier revision did demand those fields; the
carrying only what its caller readstest caught it.Passing says nothing about authenticity — the root signature runs later.
Testing
22 tests added to #1042's
manifest-entry-name.spec.ts:a spec-named entry that is not a TDF manifest— seven fixtures (web app manifest, missingencryptionInformation, array,null, string, non-JSON, zero-length), each asserted to leave the non-aligned manifest intact, then to faildecryptandgetPolicyIdasInvalidFileError.a spec-named manifest carrying only what its caller reads— a manifest holding onlyencryptionInformation.policystill yields its policy id, so the guard can't become a narrowing.assertInvalidFilecheckserror.constructor === InvalidFileError, notinstanceof—DecryptError,IntegrityErrorandUnsafeUrlErrorall extend it.isManifestreturnstrue(behavior before this PR)keyAccess+integrityInformationJSON.parseerror rethrown rawNot run:
cli/web-appsuites (untouched), Playwright and xtest (need a live platform).Out of scope
Three sibling gaps in reading foreign archives, all pre-existing:
0.payloadis hardcoded (manifest.payload.urlhas zero read sites), the central directory is parsed from a 1000-byte tail window, and DEFLATE is unsupported.🤖 Generated with Claude Code
Summary by CodeRabbit