fix(vc,ack-pay): bind credential verification to the verified proof (credential forgery)

[venables]

Summary

Fixes a credential-forgery vulnerability in @agentcommercekit/vc (and its consumer @agentcommercekit/ack-pay).

Fixes #105, #108. Supersedes #110 (same root-cause fix; this version reuses parseJwtCredential rather than re-casting in verify-proof.ts, and conforms to the assertive test-name convention).

verifyParsedCredential verified the JWT in proof.jwt but then made every trust decision — expiry, revocation, trusted-issuer, and claim verification — from the caller-supplied credential object, which is not bound to the proof. An attacker could take any legitimately signed credential, parse it to object form, mutate issuer / credentialSubject / expirationDate on the object while keeping the original valid proof.jwt, and pass verification.

This is remotely reachable: the verifier example accepts a parsed credential object (POST /verify), and verifyPaymentReceipt had the same gap on its object-input path — it returned the tampered paymentRequestToken and receipt.

Fix

The JWT proof is the only authoritative source.

• verifyProof now returns the credential decoded from the verified proof (via parseJwtCredential, which calls did-jwt-vc's verifyCredential — signature verification is unchanged).
• verifyParsedCredential runs all trust decisions against that verified credential and returns it.
• verifyPaymentReceipt uses the returned verified credential for the paymentRequestToken read and the returned receipt.

The JWT-string input paths were already safe (their object is derived from the verified JWT), so legitimate flows are unchanged. verifyProof / verifyParsedCredential return types widen from void to Verifiable<W3CCredential> — backward compatible for existing statement callers.

Verification

• Real-crypto PoCs (sign → parse → tamper → verify) confirm: legit credentials still verify; tampered objects' forged fields are ignored (both for verifyParsedCredential and verifyPaymentReceipt's object path).
• Added adversarial regression tests in both packages; vc 48/48 and ack-pay 34/34 pass; full pnpm run check is green.
• Reviewed via an independent multi-agent panel-review loop to convergence (final round: unanimous no-findings).

Notes

• Scoped to the published packages; demo/example callers that take JwtString inputs are already safe.
• Follow-up: add a runtime shape-guard on the did-jwt-vc result inside parseJwtCredential (centralizes the one remaining unchecked cast).
• A separate PR (chore(deps): update dependencies, expand catalogs, add knip + remove dead code #114) carries the unrelated dependency upgrades, knip setup, and the auth-bypass hardening.

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

verifyProof is changed to return the Verifiable<W3CCredential> decoded from the verified JWT instead of void. verifyParsedCredential stores that result and uses it exclusively for all subsequent trust decisions (expiry, revocation, trusted issuer, claim verifiers), returning the verified credential. verifyPaymentReceipt propagates the same binding, using the proof-verified receipt for all reads and the return value.

Changes

Proof-bound credential verification

Layer / File(s)	Summary
verifyProof: return decoded credential from JWT packages/vc/src/verification/verify-proof.ts, packages/vc/src/verification/verify-proof.test.ts	Replaces did-jwt-vc's verifyCredential import with the local parseJwtCredential. verifyJwtProof and verifyProof return types change from Promise<void> to Promise<Verifiable<W3CCredential>>, returning the decoded credential. Test updated to assert the resolved value.
verifyParsedCredential: bind all checks to verified credential packages/vc/src/verification/verify-parsed-credential.ts, packages/vc/src/verification/verify-parsed-credential.test.ts	Return type changes to Promise<Verifiable<W3CCredential>>. The verifyProof result is stored as verifiedCredential, which is used for expiry, revocation, issuer, and claim verifier checks. Returns verifiedCredential. Test default mock updated; new tampered-input test asserts verified fields override caller-supplied fields.
verifyPaymentReceipt: use proof-verified receipt packages/ack-pay/src/verify-payment-receipt.ts, packages/ack-pay/src/verify-payment-receipt.test.ts	Adds early isPaymentReceiptCredential fast-reject, re-checks the verifyParsedCredential result as verifiedReceipt, and reads paymentRequestToken and the returned receipt from verifiedReceipt instead of the caller-supplied object. New test validates tampered outer token is ignored.
Changeset .changeset/vc-ackpay-proof-binding.md	Patch release notes for @agentcommercekit/vc and @agentcommercekit/ack-pay documenting the security fix and its scope.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Tighten parsed credential binding to signed JWT payload #105: This PR implements the exact security fix described—verifyProof now returns the decoded credential, verifyParsedCredential uses it for all trust decisions, and regression tests for tampered outer fields are added.
• vc: verifyParsedCredential trusts caller-supplied fields on parsed-credential input #108: This PR directly closes the gap described in issue #108 by making verifyProof return the decoded credential and having verifyParsedCredential use only that verified payload for all subsequent checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately describes the main security fix: binding credential verification to the verified proof to prevent credential forgery attacks in both the vc and ack-pay packages.
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/vc-credential-proof-binding

---

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]

🧹 Nitpick comments (2)

packages/ack-pay/src/verify-payment-receipt.test.ts (1)

105-105: ⚡ Quick win

Rename this test to an approved assertive prefix.

Please rename to start with returns... (or throws.../requires.../creates...) to satisfy the repository test naming rule.

Suggested rename

-  it("ignores tampered outer fields on the object path and returns verified values", async () => {
+  it("returns verified values when outer object fields are tampered on the object path", async () => {

As per coding guidelines, "Use assertive test names with patterns: it(\"creates...\") , it(\"throws...\") , it(\"requires...\") , it(\"returns...\")".

🤖 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
function at the location uses "ignores" as the test name prefix, which does not
follow the approved assertive test naming convention. Rename the test case
starting with "ignores tampered outer fields on the object path and returns
verified values" to use an approved assertive prefix such as "returns..."
instead. Choose a prefix that accurately describes what the test verifies (in
this case, the test appears to verify that the function returns verified values,
so "returns..." is the most appropriate prefix).

Source: Coding guidelines

packages/vc/src/verification/verify-parsed-credential.test.ts (1)

224-224: ⚡ Quick win

Rename this test to match enforced *.test.ts naming prefixes.

Use an allowed prefix such as returns... for this case name.

Suggested rename

-  it("uses the credential from the verified proof, not the caller-supplied object", async () => {
+  it("returns the credential from the verified proof, not the caller-supplied object", async () => {

As per coding guidelines, "Use assertive test names with patterns: it(\"creates...\") , it(\"throws...\") , it(\"requires...\") , it(\"returns...\")".

🤖 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.test.ts` at line 224,
The test case named "uses the credential from the verified proof, not the
caller-supplied object" does not follow the enforced naming conventions for test
cases. Rename this test in the verify-parsed-credential.test.ts file to use an
allowed prefix pattern such as "returns..." to match the coding guidelines that
require test names to follow patterns like "creates...", "throws...",
"requires...", or "returns...". Choose a prefix that best describes what the
test verifies.

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.

Nitpick comments:
In `@packages/ack-pay/src/verify-payment-receipt.test.ts`:
- Line 105: The test function at the location uses "ignores" as the test name
prefix, which does not follow the approved assertive test naming convention.
Rename the test case starting with "ignores tampered outer fields on the object
path and returns verified values" to use an approved assertive prefix such as
"returns..." instead. Choose a prefix that accurately describes what the test
verifies (in this case, the test appears to verify that the function returns
verified values, so "returns..." is the most appropriate prefix).

In `@packages/vc/src/verification/verify-parsed-credential.test.ts`:
- Line 224: The test case named "uses the credential from the verified proof,
not the caller-supplied object" does not follow the enforced naming conventions
for test cases. Rename this test in the verify-parsed-credential.test.ts file to
use an allowed prefix pattern such as "returns..." to match the coding
guidelines that require test names to follow patterns like "creates...",
"throws...", "requires...", or "returns...". Choose a prefix that best describes
what the test verifies.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c3177be1-aba0-4726-9662-c5648bdc66fa

📥 Commits

Reviewing files that changed from the base of the PR and between 87d1de4 and 7f9e44e.

📒 Files selected for processing (7)

• .changeset/vc-ackpay-proof-binding.md
• packages/ack-pay/src/verify-payment-receipt.test.ts
• packages/ack-pay/src/verify-payment-receipt.ts
• 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