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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 👍 / 👎.
1 participant
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.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests