fix(ack-pay): bind payment receipt issuer to the selected payment option's receiptService - #199
Open
ygd58 wants to merge 1 commit into
Open
Conversation
…ion's receiptService Fixes agentcommercekit#192 verifyPaymentReceipt only checked that the receipt's issuer was somewhere in the caller's global trustedReceiptIssuers list, and that its paymentOptionId existed in the verified Payment Request. It did not check that the receipt issuer matched the specific receiptService the selected payment option named. In multi-rail deployments where a verifier trusts multiple receipt issuers globally (one per payment rail: card, USDC, Solana), this meant a receipt legitimately issued by one trusted service could be accepted for a payment option that named a different trusted receiptService, weakening per-option trust separation. Fix: changed the existence check (paymentOptions.some) into a lookup (paymentOptions.find) for the selected option, then - when its receiptService is a DID - require it to equal verifiedReceipt.issuer.id. URL-form receiptService values are left unenforced, since binding a DID-based issuer to a URL isn't well-defined without an application-level policy; matches the option laid out in the issue. Added three regression tests: rejects a receipt from a different trusted issuer than the option's receiptService (the exact reproduction from the issue), accepts a receipt from the option's own receiptService, and confirms the check is a no-op when the option doesn't specify a receiptService. Verified the rejection test fails when the binding check is disabled and passes with it enabled. pnpm --filter @agentcommercekit/ack-pay exec vitest run - 15/15 passing (full test file), 53/53 passing (full package). oxlint and oxfmt clean. AI usage disclosure: this fix was developed with Claude (Anthropic) assistance - identifying the exact code path, writing the fix and tests, and verifying locally including a manual before/after check that the tests catch the regression. I reviewed and understand the change: it's a targeted authorization check that binds a receipt's verified issuer to the specific receiptService the paid-for option named, closing a gap where a globally-trusted-but-wrong issuer could be accepted.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. Walkthrough
ChangesReceipt issuer binding
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to Receipt verification now binds DID receipt issuers to the selected payment option while preserving existing behavior for omitted and URL-form services. The intended behavior is covered without an identified current-head merge risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #192
verifyPaymentReceiptonly checked that the receipt's issuer was somewhere in the caller's globaltrustedReceiptIssuerslist, and that itspaymentOptionIdexisted in the verified Payment Request. It did not check that the receipt issuer matched the specificreceiptServicethe selected payment option named.In multi-rail deployments where a verifier trusts multiple receipt issuers globally (one per payment rail: card, USDC, Solana), this meant a receipt legitimately issued by one trusted service could be accepted for a payment option that named a different trusted
receiptService, weakening per-option trust separation.Fix: changed the existence check (
paymentOptions.some) into a lookup (paymentOptions.find) for the selected option, then - when itsreceiptServiceis a DID - require it to equalverifiedReceipt.issuer.id. URL-formreceiptServicevalues are left unenforced, since binding a DID-based issuer to a URL isn't well-defined without an application-level policy; matches the option laid out in the issue.Tests: added three regression tests: rejects a receipt from a different trusted issuer than the option's
receiptService(the exact reproduction from the issue), accepts a receipt from the option's ownreceiptService, and confirms the check is a no-op when the option doesn't specify areceiptService. Verified the rejection test fails when the binding check is disabled and passes with it enabled.Changeset: added (
@agentcommercekit/ack-pay, patch).Verified locally:
pnpm --filter @agentcommercekit/ack-pay exec vitest run- 15/15 passing (full test file), 53/53 passing (full package).oxlintandoxfmtclean.AI usage disclosure (per AI_POLICY.md): this fix was developed with Claude (Anthropic) assistance - identifying the exact code path, writing the fix and tests, and verifying locally including a manual before/after check that the tests catch the regression. I reviewed and understand the change: it's a targeted authorization check that binds a receipt's verified issuer to the specific
receiptServicethe paid-for option named, closing a gap where a globally-trusted-but-wrong issuer could be accepted.Summary by CodeRabbit