fix(ack-pay): validate receipt payment option#88
Conversation
WalkthroughThis PR adds ChangesPayment Option Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
.changeset/validate-receipt-payment-option.md (1)
5-5: ⚡ Quick winConsider mentioning the new error type.
The changeset describes the validation behavior but doesn't mention that a new
InvalidPaymentReceiptErroris exported. Users reading the changelog may want to know about new error types they can catch.📝 Suggested enhancement
-Validate that a PaymentReceipt paymentOptionId matches an option from the verified Payment Request token. +Validate that a PaymentReceipt paymentOptionId matches an option from the verified Payment Request token. Introduces `InvalidPaymentReceiptError` for receipt validation failures.🤖 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 @.changeset/validate-receipt-payment-option.md at line 5, Update the changeset text to explicitly mention the newly exported error type so consumers know to catch it: add a line stating that the validation will throw/export InvalidPaymentReceiptError when a PaymentReceipt paymentOptionId does not match the verified Payment Request token; reference the exported symbol InvalidPaymentReceiptError and briefly note that it can be imported and used for error handling in client code.
🤖 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 @.changeset/validate-receipt-payment-option.md:
- Line 5: Update the changeset text to explicitly mention the newly exported
error type so consumers know to catch it: add a line stating that the validation
will throw/export InvalidPaymentReceiptError when a PaymentReceipt
paymentOptionId does not match the verified Payment Request token; reference the
exported symbol InvalidPaymentReceiptError and briefly note that it can be
imported and used for error handling in client code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 099e3e98-0b72-400c-bc66-1a1df2aa2922
📒 Files selected for processing (4)
.changeset/validate-receipt-payment-option.mdpackages/ack-pay/src/errors.tspackages/ack-pay/src/verify-payment-receipt.test.tspackages/ack-pay/src/verify-payment-receipt.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: c28c1e307d
ℹ️ 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".
| const receiptPaymentOptionId = | ||
| parsedCredential.credentialSubject.paymentOptionId | ||
| const paymentOptionExists = paymentRequest.paymentOptions.some( | ||
| (paymentOption) => paymentOption.id === receiptPaymentOptionId, | ||
| ) |
There was a problem hiding this comment.
For parsed-credential inputs, this new binding check reads credentialSubject.paymentOptionId from the caller-supplied object, but verifyParsedCredential() only validates proof.jwt and then runs claim checks against the current object. If a service accepts a parsed VC, an attacker can supply a valid receipt proof whose signed payload names one option, mutate this field to an offered option before verification, and this check passes even though the signed receipt did not bind that option. Reparse or derive the receipt fields from proof.jwt (or otherwise reject mutated parsed credentials) before enforcing the payment-option match.
Useful? React with 👍 / 👎.
|
Addressed the changeset nit in 5a4cb46 by mentioning the exported |
|
Hey @EfeDurmaz16, thanks for the burst of contributions here. Before we dig into individual PRs, one thing: our AI Usage Policy requires disclosure of AI tooling used in contributions, including the tool and the extent of assistance. Your PRs and issues don't include any disclosure, but we'd like to confirm: were any of these written or developed with AI assistance? If so, please update the PR descriptions accordingly. We hold all contributors to this standard, ourselves included. Appreciate the initiative and will be reviewing each PR individually. |
|
Hey @ak68a ! Yes, I used AI assistance, mainly Codex CLI and the Codex app. The assistance was mostly for understanding ACK/Catena context, navigating docs/issues, identifying possible contribution areas, and helping draft or validate small changes. I reviewed the final diffs myself and can explain the changes and why they were made. I should have disclosed this in the PR descriptions from the start. I’ll update the open PRs with the tooling and extent of AI assistance so they comply with the policy. Appreciate the clarification, and thanks for taking the time to review them individually. |
| }, | ||
| ) | ||
|
|
||
| const receiptPaymentOptionId = |
There was a problem hiding this comment.
Looks good. One note for future reference: this check runs against parsedCredential.credentialSubject, which is trustworthy when the input is a JWT string (decoded from the signed payload) but is the caller-supplied object when the input is a pre-parsed credential.
The proof verification validates the JWT inside proof, but doesn't currently enforce that the outer object matches the signed payload. Not a blocker for this PR tho.
|
Good fix, @EfeDurmaz16. A receipt could claim any One thing I want to flag. I dug into the security question re: parsed-credential path: For JWT inputs (the primary path), For pre-parsed credential inputs, In theory, a caller could supply a valid receipt proof whose signed payload names one option but mutate the outer object to name another. That said, this is a pre-existing trust assumption in the VC verification pipeline, not something introduced by your PR. Your change is strictly better than having no check at all. We can open a separate issue to track tightening the parsed-credential binding in |
|
Thanks for digging into that path. Agreed, this sounds like a separate pre-existing trust boundary in the parsed credential verification flow rather than something this PR should expand into. I will keep this PR scoped to binding the receipt I opened a focused follow-up issue for tightening |
|
@EfeDurmaz16 — there are two open inline review threads on What are your thoughts on these two comments? Do you see it as in-scope to address here (e.g. deriving the option from the signed proof), or better tracked as separate follow-up? The comments have no replies, so I'm curious what your take is. |
|
Thanks @venables — I went through the full path and I agree with both threads. Mechanism: on the JWT-string input path, Scope: this is pre-existing rather than introduced here — the What I changed here: rather than leave the parsed path bypassable, I added a localized hardening — when the input is a parsed credential with a JWT proof, the receipt is now re-derived from Still recommend a follow-up: the general fix belongs in Verification:
|
|
General |
| // paymentOptionId / paymentRequestToken reads below come from the signed | ||
| // payload rather than a (potentially mutated) caller-supplied object. | ||
| parsedCredential = isJwtProof(receipt.proof) | ||
| ? await parseJwtCredential(receipt.proof.jwt, resolver) |
There was a problem hiding this comment.
Previously, when a Verifiable<W3CCredential> was passed in, parsedCredential = receipt and the proof was only validated later by verifyParsedCredential(), which throws InvalidProofError on a bad proof. Now parseJwtCredential(receipt.proof.jwt, resolver) runs first, so an invalid/malformed proof.jwt surfaces a raw did-jwt-vc error instead of InvalidProofError — a silent contract change for callers catching InvalidProofError.
Non-blocking. If you want to preserve the prior contract, re-throw as InvalidProofError around this call; otherwise it may be fine to leave as-is given the general vc trust-boundary work is already deferred to #108. Flagging so the choice is deliberate.
|
Thanks @EfeDurmaz16 , this looks good to merge once the check/tests are passing |
Bind a PaymentReceipt's selected paymentOptionId back to an option offered by the verified Payment Request token. The check reads from the proof-decoded credential returned by verifyParsedCredential, so it cannot be bypassed by mutating the outer object on the parsed-credential input path. Adds InvalidPaymentReceiptError for receipt-level validation failures and a regression test for the mismatched-option case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EfeDurmaz16
force-pushed
the
codex/validate-receipt-payment-option
branch
from
7d3eb3f to
4147b31
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ack-pay/src/verify-payment-receipt.ts (1)
77-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip the pre-verification receipt-schema reject on object inputs.
Lines 77-83 still let the caller-supplied outer object fail verification before
verifyParsedCredential()can recover the proof-decoded receipt. A validVerifiable<W3CCredential>with a tampered outercredentialSubjectwill now false-negative here, even though the signed JWT insideproofis still valid. If you want to keep this fast-reject, gate it to the JWT-string path only; otherwise rely on the verified check at Lines 95-99.Suggested fix
- if (!isPaymentReceiptCredential(parsedCredential)) { + if (isJwtString(receipt) && !isPaymentReceiptCredential(parsedCredential)) { throw new InvalidCredentialError( "Credential is not a PaymentReceiptCredential", ) }🤖 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.ts` around lines 77 - 89, The early isPaymentReceiptCredential fast-reject in verify-payment-receipt.ts is incorrectly blocking valid object-based inputs before the proof-verified credential is recovered. Update the precheck around parsedCredential so it only runs for the JWT-string path, or remove it for object inputs and rely on verifyParsedCredential() plus the verifiedReceipt validation instead. Keep the fix scoped to the verifyPaymentReceipt flow and the isPaymentReceiptCredential / verifyParsedCredential call sites.
🤖 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.
Duplicate comments:
In `@packages/ack-pay/src/verify-payment-receipt.ts`:
- Around line 77-89: The early isPaymentReceiptCredential fast-reject in
verify-payment-receipt.ts is incorrectly blocking valid object-based inputs
before the proof-verified credential is recovered. Update the precheck around
parsedCredential so it only runs for the JWT-string path, or remove it for
object inputs and rely on verifyParsedCredential() plus the verifiedReceipt
validation instead. Keep the fix scoped to the verifyPaymentReceipt flow and the
isPaymentReceiptCredential / verifyParsedCredential call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40788aaa-cc39-4fc4-909b-35667bcf234b
📒 Files selected for processing (4)
.changeset/validate-receipt-payment-option.mdpackages/ack-pay/src/errors.tspackages/ack-pay/src/verify-payment-receipt.test.tspackages/ack-pay/src/verify-payment-receipt.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/validate-receipt-payment-option.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ack-pay/src/errors.ts
|
@coderabbitai Thanks for the review. On the
This PR is scoped to binding the receipt's |
|
Tip For best results, initiate chat on the files or code changes.
The key point is that ✏️ Learnings added
|
3 participants
Summary
paymentOptionIdmatches an option offered by the verified Payment Request tokenInvalidPaymentReceiptErrorfor receipt-level semantic failures@agentcommercekit/ack-payWhy
A receipt proves a
paymentRequestTokenand a selectedpaymentOptionId, but verification did not bind that selection back to an option actually offered by the request. This keeps receipt proof semantics aligned with the payment options the request offered.The binding check reads from the proof-decoded credential (
verifiedReceipt, returned byverifyParsedCredential), so it cannot be bypassed by mutating the outer object on the parsed-credential input path. This builds on the trust-boundary work in #108 / the proof-binding changeset already onmain.Verification
pnpm install --frozen-lockfilepnpm run buildpnpm run check(build + format + types + lint + test) — all green, lint 0 warnings/0 errorspnpm --filter @agentcommercekit/ack-pay test— 35 passingAI Usage Disclosure
This contribution was AI-assisted (Codex CLI/app and Claude Opus). AI assistance was used for repository/docs navigation, understanding ACK context, and small edits/validation. The branch was rebased onto current
mainand the feature was rebuilt on top of main's proof-verified-receipt mechanism. I reviewed the final diff and take responsibility for the submitted changes.Summary by CodeRabbit
paymentOptionIddoesn’t match an option in the verified payment request.verifyPaymentReceipt(...)throws when the payment option is missing from the payment request.InvalidPaymentReceiptErrorfor callers to catch receipt validation failures.