fix(ack-pay): validate receipt payment option#88
Conversation
WalkthroughThis PR adds payment option ID validation to the receipt verification flow. A new ChangesPayment Option Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 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 👍 / 👎.
Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
|
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. |
verifyParsedCredential() validates proof.jwt but does not reconcile the outer credential object against the decoded payload, so on the parsed-credential input path the paymentOptionId / paymentRequestToken reads were caller-supplied and the new payment-option binding could be bypassed by mutating the outer object while attaching any validly-signed proof. When the input is a parsed credential with a JWT proof, re-derive the credential from proof.jwt so those reads come from the signed payload. Add a regression test that a mutated parsed credential cannot smuggle a receipt past the binding. Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
|
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 |
3 participants
Summary
Why
A receipt currently proves a paymentRequestToken and selected paymentOptionId, but verification did not bind that selected option back to the verified request. This keeps receipt proof semantics aligned with the payment options actually offered by the request.
Verification
Notes
Initial targeted tests/typecheck failed before building workspace dependencies because local package exports resolve to dist. Building the ack-pay dependency graph fixed that, and the rerun passed. pnpm install reported an ignored better-sqlite3 build script, but this ACK-Pay path does not use it.
AI Usage Disclosure
This contribution was AI-assisted. Initial work used Codex CLI and the Codex app; the parsed-input hardening follow-up was completed with Claude Opus. AI assistance was used for repository/docs navigation, understanding ACK/Catena context, identifying relevant issues or contribution areas, and assisting with small edits/validation. I reviewed the final diff and take responsibility for the submitted changes.
Summary by CodeRabbit
Bug Fixes
Tests
Chores