fix(vc): bind credential verification to the signed proof (#105, #108)

[EfeDurmaz16]

TL;DR

On the parsed-credential input path, verifyParsedCredential() verified proof.jwt but then ran expiry, revocation, trustedIssuers, and claim-verifier checks against the caller-supplied outer object. A caller could present a validly-signed proof.jwt while mutating the outer credentialSubject / 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 from proof.jwt (previously void).
• 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 — verifyJwtProof returns 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

• The return types of verifyProof and verifyParsedCredential change from void to the verified credential. This is backward-compatible: callers that ignore the return value are unaffected. Patch changeset included.
• The behavior change is intentional and scoped to the parsed-credential path: mutated outer fields no longer affect verification.

Tests

• Rewrote verify-parsed-credential.test.ts onto real signed credentials (no mocked proof).
• Regression tests: a spoofed outer issuer (pointed at a trusted DID) is rejected; claim verifiers receive the signed subject rather than a mutated one; claim-verifier selection uses the signed type; and the returned credential is the signed one.
• verify-proof.test.ts asserts the decoded credential is returned.
• @agentcommercekit/vc: 51 tests passing. Repo-wide: check:types, test, lint (0 errors), and oxfmt --check all 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/vc layer and is complementary. With verifyParsedCredential now 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 — verifyParsedCredential now 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 mock isExpired / isRevoked, so a maintainer may want an end-to-end test with a genuinely expired or revoked signed credential.

Summary by CodeRabbit

• Bug Fixes

  • Credential verification now validates against the signed proof payload rather than potentially modified input fields, preventing acceptance of tampered credentials.
  • Verification functions now return the verified credential to callers for reliable downstream use.

• Tests

  • Enhanced test coverage to validate signed credential integrity and prevent spoofed issuer and mutated field acceptance.

[coderabbitai]

Walkthrough

This PR updates the @agentcommercekit/vc package to bind credential verification to the signed proof payload. The proof verification functions now return the decoded credential from the JWT, and parsed-credential verification performs all downstream checks against that verified credential instead of the caller-supplied input.

Changes

Proof Binding and Verification

Layer / File(s)	Summary
Proof verification return value packages/vc/src/verification/verify-proof.ts, packages/vc/src/verification/verify-proof.test.ts	verifyJwtProof and verifyProof return type changes from Promise<void> to Promise<Verifiable<W3CCredential>>. The decoded credential from verifyCredential(proof.jwt) is now returned instead of discarded. Test is updated to assert the returned decoded credential.
Parsed credential verification binding and test refactor packages/vc/src/verification/verify-parsed-credential.ts, packages/vc/src/verification/verify-parsed-credential.test.ts	verifyParsedCredential captures the verified credential from verifyProof and runs expiry, revocation, trusted-issuer, and claim-verifier checks against it instead of the caller-supplied credential, then returns it. Test infrastructure is refactored to build real signed JWT credentials. New regression tests verify spoofed outer fields are ignored in favor of the signed proof payload.
Release notes .changeset/bind-parsed-credential-to-proof.md	Documents the behavior change: verification is driven by the credential decoded from proof.jwt, all downstream checks use the verified credential, caller-supplied outer fields are ignored on the parsed-credential path, and the verified credential is returned to the caller.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• #108: Implements the proposed fix by changing verifyProof to return the decoded credential and updating verifyParsedCredential to run checks against the verified credential, directly binding parsed-credential validation to the signed proof.
• #105: Makes the same code-level changes to bind parsed-credential verification to the signed JWT payload through return values and validation rewiring, with corresponding test updates.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly and specifically describes the main change: binding credential verification to the signed proof, which is the primary security fix across the entire changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 | 🟡 Minor

Remove dead ./verify-credential-jwt mock in verify-proof.test.ts.

packages/vc/src/verification/verify-proof.ts doesn’t import ./verify-credential-jwt, and verifyCredentialJwt only 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 lift

Downstream caller may still read spoofed fields from parsedCredential.

Context snippet shows verifyPaymentReceipt calls verifyParsedCredential but continues using parsedCredential.credentialSubject.paymentRequestToken afterward 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

📥 Commits

Reviewing files that changed from the base of the PR and between 60eec23 and 794e138.

📒 Files selected for processing (5)

• .changeset/bind-parsed-credential-to-proof.md
• packages/vc/src/verification/verify-parsed-credential.test.ts
• packages/vc/src/verification/verify-parsed-credential.ts
• packages/vc/src/verification/verify-proof.test.ts
• packages/vc/src/verification/verify-proof.ts

[chatgpt-codex-connector]

💡 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".

[chatgpt-codex-connector]

Consume the verified credential before reading receipt fields

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 👍 / 👎.