fix(vc): validate decoded credential shape in parseJwtCredential#115
Conversation
did-jwt-vc's `verifyCredential` returns its own credential shape, which `parseJwtCredential` cast to `Verifiable<W3CCredential>` unchecked. If that shape ever diverged, the cast would silently mask the mismatch. Validate the decoded credential against `isCredential` (our `credentialSchema`) before returning, throwing `InvalidCredentialError` on a non-conforming shape. Adds a regression test (delegating did-jwt-vc mock so the real-signing tests still run); legitimate did-jwt-vc output is unaffected. Follow-up to #113 (addresses the unchecked-cast review comment from #110). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 15 minutes and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesparseJwtCredential shape guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vc/src/verification/parse-jwt-credential.test.ts`:
- Around line 72-83: Replace the `test` function call with `it` at the test case
"throws when the verified JWT does not decode to a valid credential" to follow
the project's test naming convention. The test should use the `it()` pattern for
assertive test names that clearly describe the expected behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7a70d4c-de9b-4e81-9a50-1e678f3b22bb
📒 Files selected for processing (4)
.changeset/parse-jwt-credential-shape-guard.mdpackages/vc/src/verification/parse-jwt-credential.test.tspackages/vc/src/verification/parse-jwt-credential.tspackages/vc/src/verification/verify-proof.test.ts
Panel review of #115 found the `isCredential` guard was the wrong validator: - it validates ACK's authoring schema's pre-transform shape, so it would PASS a top-level string `issuer` that downstream then reads as `issuer.id === undefined` (codex); - and it is simultaneously too strict — rejecting valid W3C VCs with an object `@context` entry or a VC 2.0 shape that the previous cast accepted (claude). Replace it with a focused `isDecodedCredential` guard that validates exactly what the verification chain consumes: `credentialSubject` (object), `issuer.id` (string), `type` (array), and `proof` — and nothing else, so valid third-party VCs are not false-rejected. Add tests for the string-issuer rejection and the object-`@context` acceptance. Also preserve `InvalidCredentialError` through `verifyJwtProof` instead of re-wrapping it as `InvalidProofError`, so a malformed decoded credential is distinguishable from a bad signature (opencode). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vc/src/verification/parse-jwt-credential.test.ts (1)
16-21:⚠️ Potential issue | 🟡 MinorUse
vi.importActual()instead of theimportOriginalparameter for this partial mock.The codebase standardizes on
vi.importActual()to preserve real implementations in partial mocks. Replace theimportOriginalparameter with a direct call tovi.importActual("did-jwt-vc")to match the pattern used elsewhere in the verification module (e.g.,verify-proof.test.ts).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vc/src/verification/parse-jwt-credential.test.ts` around lines 16 - 21, Replace the `importOriginal` parameter usage in the vi.mock call with a direct call to vi.importActual("did-jwt-vc") to match the codebase's standardization pattern for partial mocks. Instead of using the importOriginal callback parameter, call vi.importActual("did-jwt-vc") to obtain the actual implementation, then spread it in the return object and override only the verifyCredential function with vi.fn(actual.verifyCredential) to maintain the same behavior.Source: Coding guidelines
♻️ Duplicate comments (1)
packages/vc/src/verification/parse-jwt-credential.test.ts (1)
72-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename new test cases to assertive
it("throws...")/it("returns...")forms.The added regression tests still use
test(...); this violates the required naming convention for*.test.tsfiles.As per coding guidelines, "Use assertive test names with patterns:
it(\"creates...\"),it(\"throws...\"),it(\"requires...\"),it(\"returns...\")."Also applies to: 85-103, 105-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vc/src/verification/parse-jwt-credential.test.ts` around lines 72 - 83, The test cases in this file use the `test(...)` function instead of `it(...)`, which violates the required assertive naming convention. Replace all instances of `test(` with `it(` in the new test cases, including the one starting with "throws when the verified JWT does not decode to a valid credential" and the other test cases referenced in the comment. The test names themselves are already properly assertive with patterns like "throws..." and "returns...", so only the function call needs to change from `test` to `it`.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/vc/src/verification/parse-jwt-credential.test.ts`:
- Around line 16-21: Replace the `importOriginal` parameter usage in the vi.mock
call with a direct call to vi.importActual("did-jwt-vc") to match the codebase's
standardization pattern for partial mocks. Instead of using the importOriginal
callback parameter, call vi.importActual("did-jwt-vc") to obtain the actual
implementation, then spread it in the return object and override only the
verifyCredential function with vi.fn(actual.verifyCredential) to maintain the
same behavior.
---
Duplicate comments:
In `@packages/vc/src/verification/parse-jwt-credential.test.ts`:
- Around line 72-83: The test cases in this file use the `test(...)` function
instead of `it(...)`, which violates the required assertive naming convention.
Replace all instances of `test(` with `it(` in the new test cases, including the
one starting with "throws when the verified JWT does not decode to a valid
credential" and the other test cases referenced in the comment. The test names
themselves are already properly assertive with patterns like "throws..." and
"returns...", so only the function call needs to change from `test` to `it`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 108b0c0c-ce58-4176-aa1a-b30e4458009d
📒 Files selected for processing (3)
packages/vc/src/verification/parse-jwt-credential.test.tspackages/vc/src/verification/parse-jwt-credential.tspackages/vc/src/verification/verify-proof.ts
Resolves the CodeRabbit comment on #115: the file used test() while the rest of the suite (and AGENTS.md) uses assertive it("...") names. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Follow-up to #113. Addresses the unchecked-cast review comment left on #110:
parseJwtCredentialcastverifyCredential's result toVerifiable<W3CCredential>without validating it.verifyCredential(did-jwt-vc) returns its own credential shape.parseJwtCredentialnow validates that shape conforms to ourW3CCredential(via the existingisCredential/credentialSchema) before returning it, throwingInvalidCredentialErroron a divergent shape rather than silently casting. SinceparseJwtCredentialis the single decode point used byverifyProof/verifyParsedCredential/verifyPaymentReceipt, this hardens the whole verification chain.Changes
packages/vc/src/verification/parse-jwt-credential.ts— validateresult.verifiableCredentialwithisCredentialbefore returning.packages/vc/src/verification/parse-jwt-credential.test.ts— regression test for a non-conforming decoder result (delegatingdid-jwt-vcmock so the existing real-signing test still exercises the real implementation).Verification
@agentcommercekit/vc49/49 and@agentcommercekit/ack-pay34/34 pass; fullpnpm run checkis green.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
InvalidCredentialErrorwhen credential shape validation fails, and treat other failures asInvalidProofError.parseJwtCredentialandverifyProoftest coverage for edge cases and additional credential payload fields.parseJwtCredentialnow validates credential shape instead of relying on unchecked casting.