fix(vc): bind credential verification to the signed proof (#105, #108)#110
Conversation
verifyProof() now returns the credential decoded from proof.jwt, and verifyParsedCredential() runs expiry, revocation, trusted-issuer, and claim-verifier checks against that verified credential instead of the caller-supplied object. It also returns the verified credential so consumers can read signed fields from the return value rather than the object they passed in. On the parsed-credential input path a caller could previously attach a valid proof.jwt while mutating the outer credentialSubject / issuer / type; those fields were trusted directly. They are now ignored in favor of the signed payload. Adds regression tests proving a spoofed outer issuer, a mutated outer subject, and a mutated outer type cannot bypass verification, and that the returned credential is the signed one. Fixes agentcommercekit#105 Fixes agentcommercekit#108 Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughThis PR updates the ChangesProof Binding and Verification
Consumer Code Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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.
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/verify-proof.test.ts (1)
16-18:⚠️ Potential issue | 🟡 MinorRemove dead
./verify-credential-jwtmock inverify-proof.test.ts.
packages/vc/src/verification/verify-proof.tsdoesn’t import./verify-credential-jwt, andverifyCredentialJwtonly appears in the mock (lines 16-18) and in the test name—so the mock is unused. Delete it to keep the test focused.🤖 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/verify-proof.test.ts` around lines 16 - 18, Delete the unused mock declaration vi.mock("./verify-credential-jwt", () => ({ verifyCredentialJwt: vi.fn(), })) from the test file (it’s dead code because verify-proof.ts does not import that module); also update the test name if it still references verifyCredentialJwt so the test name accurately reflects the behavior being tested (remove or rename that phrase).
🧹 Nitpick comments (1)
packages/vc/src/verification/verify-parsed-credential.ts (1)
64-73: 🏗️ Heavy liftDownstream caller may still read spoofed fields from
parsedCredential.Context snippet shows
verifyPaymentReceiptcallsverifyParsedCredentialbut continues usingparsedCredential.credentialSubject.paymentRequestTokenafterward without capturing the returned verified credential. This undermines the binding guarantee for that call site.Consider updating downstream callers to use the returned verified credential, or document clearly in the JSDoc that callers must use the return value.
🤖 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/verify-parsed-credential.ts` around lines 64 - 73, The caller is still reading unverified fields from parsedCredential after calling verifyParsedCredential; update downstream callers (e.g., verifyPaymentReceipt) to use the verified credential returned by verifyParsedCredential (assign the function's return to a variable like verifiedCredential and read paymentRequestToken from verifiedCredential.credentialSubject) rather than parsedCredential, or alternatively update the JSDoc on verifyParsedCredential to explicitly state that callers must use the returned verified credential and then change each call site to follow that contract (ensure any place referencing parsedCredential.credentialSubject.* is changed to reference the returned verified credential or marked as requiring no change if covered by the new JSDoc policy).
🤖 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/verify-proof.test.ts`:
- Around line 16-18: Delete the unused mock declaration
vi.mock("./verify-credential-jwt", () => ({ verifyCredentialJwt: vi.fn(), }))
from the test file (it’s dead code because verify-proof.ts does not import that
module); also update the test name if it still references verifyCredentialJwt so
the test name accurately reflects the behavior being tested (remove or rename
that phrase).
---
Nitpick comments:
In `@packages/vc/src/verification/verify-parsed-credential.ts`:
- Around line 64-73: The caller is still reading unverified fields from
parsedCredential after calling verifyParsedCredential; update downstream callers
(e.g., verifyPaymentReceipt) to use the verified credential returned by
verifyParsedCredential (assign the function's return to a variable like
verifiedCredential and read paymentRequestToken from
verifiedCredential.credentialSubject) rather than parsedCredential, or
alternatively update the JSDoc on verifyParsedCredential to explicitly state
that callers must use the returned verified credential and then change each call
site to follow that contract (ensure any place referencing
parsedCredential.credentialSubject.* is changed to reference the returned
verified credential or marked as requiring no change if covered by the new JSDoc
policy).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72d9beac-de44-467f-bdfa-9ed19bf983af
📒 Files selected for processing (5)
.changeset/bind-parsed-credential-to-proof.mdpackages/vc/src/verification/verify-parsed-credential.test.tspackages/vc/src/verification/verify-parsed-credential.tspackages/vc/src/verification/verify-proof.test.tspackages/vc/src/verification/verify-proof.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 794e138e17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Because this now only returns the credential decoded from proof.jwt and leaves the caller-supplied object unchanged, internal call sites that ignore the return value can still consume unsigned outer fields. For example, verifyPaymentReceipt calls verifyParsedCredential(...) and then reads parsedCredential.credentialSubject.paymentRequestToken at packages/ack-pay/src/verify-payment-receipt.ts:82-90; with parsed-object input, a caller can keep a valid signed receipt proof while swapping the outer paymentRequestToken, so the signed subject is verified but the swapped token is the one returned/verified. Please update those consumers to use this returned credential, or otherwise replace/mutate the input before returning from verification.
Useful? React with 👍 / 👎.
| try { | ||
| await verifyCredential(proof.jwt, resolver) | ||
| const { verifiableCredential } = await verifyCredential(proof.jwt, resolver) | ||
| return verifiableCredential as Verifiable<W3CCredential> |
There was a problem hiding this comment.
Small / Optional polish: verifiableCredential as Verifiable<W3CCredential> is an unchecked type assertion. If did-jwt-vc ever returns a shape that diverges from Verifiable<W3CCredential>, the cast masks the mismatch.
Possible Solution: Validate the shape with a type guard or narrow the return type of verifyCredential.
|
Location: ack/demos/e2e/src/credential-verifier.ts Lines 82 to 89 in 794e138 After Possible Solution: Capture and return the value from |
|
Location: ack/examples/verifier/src/routes/verify.ts Lines 44 to 50 in 794e138 Small / Optional polish: Possible Solution: Capture and use the return value, or add a comment documenting why the return value is intentionally discarded here. |
venables
left a comment
venables
left a comment
There was a problem hiding this comment.
Thanks for putting this together — binding the downstream checks to the credential decoded from proof.jwt is the right fix and the new regression tests cover the tampering vectors well.
One thing to resolve before this lands: please check out the existing comment from the Codex reviewer on packages/vc/src/verification/verify-parsed-credential.ts. The core fix is correct, but the primary in-repo consumer — verifyPaymentReceipt in packages/ack-pay/src/verify-payment-receipt.ts:82-90 — still ignores the new return value and reads paymentRequestToken from the caller-supplied object, so the #105/#108 tampering vector remains exploitable through the payment-receipt path. Capturing the returned verified credential and reading the receipt fields from it closes that gap.
I've also left a couple of smaller notes on the demo/example call sites and the unchecked cast.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ack-pay/src/verify-payment-receipt.test.ts`:
- Line 105: The test title starting with "uses the signed paymentRequestToken
for parsed credentials" does not follow the approved test naming conventions.
Replace the "uses" prefix with one of the allowed assertive prefixes: "creates",
"throws", "requires", or "returns". Based on the test's intent of verifying that
the signed paymentRequestToken is used for parsed credentials, consider using
"returns" or "creates" as the new prefix while keeping the descriptive part of
the test name intact.
In `@packages/vc/src/verification/verify-proof.test.ts`:
- Line 72: The test "handles verification errors from verifyCredential" uses a
naming pattern that does not match the required assertive test naming
conventions. Rename this test to use one of the approved patterns: starts with
"creates...", "throws...", "requires...", or "returns..." depending on what the
test verifies. Choose the pattern that best describes what the test asserts
about the verification 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: 8a25eb07-4095-4304-b07e-caf1c37165a6
📒 Files selected for processing (7)
.changeset/bind-parsed-credential-to-proof.mddemos/e2e/src/credential-verifier.tsexamples/verifier/src/routes/verify.tspackages/ack-pay/src/verify-payment-receipt.test.tspackages/ack-pay/src/verify-payment-receipt.tspackages/vc/src/verification/verify-proof.test.tspackages/vc/src/verification/verify-proof.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/bind-parsed-credential-to-proof.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vc/src/verification/verify-proof.ts
| expect(result.paymentRequest).toBeDefined() | ||
| }) | ||
|
|
||
| it("uses the signed paymentRequestToken for parsed credentials", async () => { |
There was a problem hiding this comment.
Use an allowed assertive test title prefix here.
This test name starts with uses...; it should use one of the approved prefixes.
Suggested change
- it("uses the signed paymentRequestToken for parsed credentials", async () => {
+ it("returns the signed paymentRequestToken for parsed credentials", async () => {As per coding guidelines, "Use assertive test names with patterns: it(\"creates...\") , it(\"throws...\") , it(\"requires...\") , it(\"returns...\")".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("uses the signed paymentRequestToken for parsed credentials", async () => { | |
| it("returns the signed paymentRequestToken for parsed credentials", async () => { |
🤖 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/ack-pay/src/verify-payment-receipt.test.ts` at line 105, The test
title starting with "uses the signed paymentRequestToken for parsed credentials"
does not follow the approved test naming conventions. Replace the "uses" prefix
with one of the allowed assertive prefixes: "creates", "throws", "requires", or
"returns". Based on the test's intent of verifying that the signed
paymentRequestToken is used for parsed credentials, consider using "returns" or
"creates" as the new prefix while keeping the descriptive part of the test name
intact.
Source: Coding guidelines
| }) | ||
|
|
||
| it("handles verification errors from verifyCredentialJwt", async () => { | ||
| it("handles verification errors from verifyCredential", async () => { |
There was a problem hiding this comment.
Rename this test to the required assertive naming pattern.
The current title starts with handles..., which does not match the required patterns.
Suggested change
- it("handles verification errors from verifyCredential", async () => {
+ it("throws InvalidProofError when verifyCredential fails", async () => {As per coding guidelines, "Use assertive test names with patterns: it(\"creates...\") , it(\"throws...\") , it(\"requires...\") , it(\"returns...\")".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("handles verification errors from verifyCredential", async () => { | |
| it("throws InvalidProofError when verifyCredential fails", async () => { |
🤖 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/verify-proof.test.ts` at line 72, The test
"handles verification errors from verifyCredential" uses a naming pattern that
does not match the required assertive test naming conventions. Rename this test
to use one of the approved patterns: starts with "creates...", "throws...",
"requires...", or "returns..." depending on what the test verifies. Choose the
pattern that best describes what the test asserts about the verification
behavior.
Source: Coding guidelines
|
Thanks for this @EfeDurmaz16 — solid work pinning down the root cause and tying it to #105/#108. We're going to land this fix via #113, which takes the same approach (bind all trust decisions + returned values to the proof-decoded credential) and folds in the review feedback from here: it reuses Closing this in favor of #113 — appreciate you surfacing it. |
2 participants
TL;DR
On the parsed-credential input path,
verifyParsedCredential()verifiedproof.jwtbut then ran expiry, revocation,trustedIssuers, and claim-verifier checks against the caller-supplied outer object. A caller could present a validly-signedproof.jwtwhile mutating the outercredentialSubject/issuer/type, and those mutated fields were trusted. This binds all verification to the signed payload. Fixes #105 and #108.What changed (
@agentcommercekit/vc)verifyProof()now returns the credential decoded fromproof.jwt(previouslyvoid).verifyParsedCredential()runs every downstream check against that verified credential, and returns it, so consumers can read signed fields from the return value instead of the object they passed in.Where to look
packages/vc/src/verification/verify-proof.ts—verifyJwtProofreturns the decoded credential.packages/vc/src/verification/verify-parsed-credential.ts— expiry / revocation / trusted-issuer / claim-verifier checks and the return value now use the verified credential.Risk / compatibility
verifyProofandverifyParsedCredentialchange fromvoidto the verified credential. This is backward-compatible: callers that ignore the return value are unaffected. Patch changeset included.Tests
verify-parsed-credential.test.tsonto real signed credentials (no mocked proof).issuer(pointed at a trusted DID) is rejected; claim verifiers receive the signed subject rather than a mutated one; claim-verifier selection uses the signedtype; and the returned credential is the signed one.verify-proof.test.tsasserts the decoded credential is returned.@agentcommercekit/vc: 51 tests passing. Repo-wide:check:types,test,lint(0 errors), andoxfmt --checkall pass.Relationship to #88
#88 hardens the ACK-Pay receipt path specifically and adds the
paymentOptionId↔ Payment Request binding, which lives at the ACK-Pay layer and is not covered here. This PR is the general fix at the@agentcommercekit/vclayer and is complementary. WithverifyParsedCredentialnow returning the canonical credential, the receipt path's local re-parse in #88 can later be simplified to consume the return value.Reviewed with ChatGPT 5.5 (xhigh reasoning) in addition to the author. Areas that may still warrant human review: (1) consumer adoption —
verifyParsedCredentialnow returns the canonical credential, and call sites that read fields after verification should switch to the return value (the receipt path is separately hardened in #88); (2) expiry / revocation binding is covered structurally because those checks now receive the verified credential, but the unit tests mockisExpired/isRevoked, so a maintainer may want an end-to-end test with a genuinely expired or revoked signed credential.Summary by CodeRabbit
verifyProofandverifyParsedCredentialnow return the verified/decoded credential for safer downstream handling.paymentRequestTokenvalues.