demo(payments): enforce policy before signing#97
Conversation
WalkthroughThis PR demonstrates policy-before-signing enforcement in ACK-Pay by adding a minimal local payment policy module that evaluates spend limits and recipient allowlists before the payment service signs or executes transactions. Changes include the policy contract, evaluation logic, full test coverage, service integration, and usage documentation. ChangesPayment policy-before-signing enforcement
🎯 3 (Moderate) | ⏱️ ~25 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 |
![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: 264bbd2bf0
ℹ️ 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 decision = evaluatePaymentPolicy(paymentOption, { | ||
| allowedRecipients: [], | ||
| maxAutonomousAmount: 1_000_000, | ||
| trustedRequestIssuer, |
There was a problem hiding this comment.
When this payment service is reachable by any client, a third party can submit a valid PaymentRequest JWT it signed itself with a Stripe option whose recipient is that same DID; because trustedRequestIssuer is set from the token's own parsed.issuer, the empty allowlist is bypassed and the service will return a payment URL and later sign the receipt-service payload for an unapproved merchant. The policy should compare against a configured trusted server/merchant DID or explicit allowlist, not the self-asserted issuer from the request being evaluated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f0f5df8 by removing approval based on the Payment Request token’s own issuer. The demo now derives the autonomous recipient allowlist from the configured server identity and keeps self-asserted recipients in approval_required unless explicitly allowlisted.\n\nVerification:\n- pnpm --filter ./demos/payments test -- payment-policy.test.ts\n- pnpm exec oxfmt --check demos/payments/README.md demos/payments/src/payment-policy.ts demos/payments/src/payment-policy.test.ts demos/payments/src/payment-service.ts\n- git diff --check\n\nI also tried pnpm --filter ./demos/payments check:types, but this checkout currently fails to resolve the workspace agentcommercekit package and reports existing demo type errors outside this change.
| paymentOption: PaymentOption, | ||
| policy: PaymentPolicy = demoPaymentPolicy, | ||
| ): PaymentPolicyDecision { | ||
| const amount = Number(paymentOption.amount) |
There was a problem hiding this comment.
Number(paymentOption.amount) breaks the repo-wide BigInt money convention (receipt-service.ts:350, index.ts:530 both use BigInt(paymentOption.amount)) and accepts non-positive / fractional / empty string amounts the ACK-Pay schema permits (the amount string branch is unconstrained), which then pass policy and reach signing for an allowlisted recipient.
Possible Solution: use BigInt(paymentOption.amount), type maxAutonomousAmount as bigint, and add an explicit amount > 0n guard (catching the throw BigInt("1.5") raises on malformed input).
| } | ||
| } | ||
|
|
||
| if (amount > policy.maxAutonomousAmount) { |
There was a problem hiding this comment.
amount > policy.maxAutonomousAmount compares raw subunits with no currency/decimals dimension, so one flat limit is incoherent across the USD (2dp) / USDC (6dp) / SOL options this service can receive; the documented "autonomous spend limit" does not actually bound real value.
Possible Solution: make the limit currency-aware (a per-currency map keyed on currency), or normalize to a human value per currency before comparing; reject options whose currency has no configured limit.
| ) { | ||
| const decision = evaluatePaymentPolicy(paymentOption, { | ||
| allowedRecipients, | ||
| maxAutonomousAmount: 1_000_000, |
There was a problem hiding this comment.
enforcePaymentPolicy hardcodes maxAutonomousAmount: 1_000_000, bypassing the exported demoPaymentPolicy.maxAutonomousAmount default entirely — so the exported config is dead/misleading and the real value has no single clear home.
Possible Solution: make demoPaymentPolicy the single source of truth — evaluatePaymentPolicy(paymentOption, { ...demoPaymentPolicy, allowedRecipients }) — and drop the literal.
|
|
||
| - known low-value recipient: continue automatically | ||
| - unknown recipient: return `approval_required` | ||
| - amount above the autonomous spend limit: deny before payment execution |
There was a problem hiding this comment.
A per-transaction cap with no cumulative or rate budget is trivially defeated by splitting (e.g. 99.99 × N), yet the README presents it as "the autonomous spend limit" safety boundary.
Possible Solution: either implement a running/windowed budget, or (for a demo) reword to state plainly that this is an illustrative per-transaction check, not a real spend control, and that production needs cumulative + rate limits.
|
|
||
| log(colors.dim(`${name} Verifying payment request token...`)) | ||
| const { paymentRequest } = await verifyPaymentRequestToken( | ||
| const { paymentRequest, parsed } = await verifyPaymentRequestToken( |
There was a problem hiding this comment.
Small / Optional polish: parsed is destructured from verifyPaymentRequestToken and added to validatePaymentOption's return, but no caller consumes it — dead code from the earlier issuer-comparison iteration.
Possible Solution: drop parsed from the destructure and return object.
| } | ||
| } | ||
|
|
||
| export function assertPaymentPolicyApproved( |
There was a problem hiding this comment.
Small / Optional polish: assertPaymentPolicyApproved is exported but never called in the demo or tests (0 usages).
Possible Solution: remove it, or add a caller/test if it is meant to be part of the example pattern.
venables
left a comment
venables
left a comment
There was a problem hiding this comment.
Nice direction — enforcing policy at the verify→sign seam is the right shape. Requesting changes on a few items before merge (left as inline comments):
- Money handling should follow the repo BigInt convention (
Number()on amounts loses precision and accepts malformed/negative/fractional strings the schema permits). - The autonomous spend limit has no currency/decimals dimension, so a single flat threshold is incoherent across the USD/USDC/SOL options.
- The effective limit is hardcoded in
enforcePaymentPolicy, bypassing the exporteddemoPaymentPolicydefault — make that the single source of truth. - A per-transaction cap is trivially split-gameable; either add a cumulative/rate budget or reword the README so it does not present this as a real spend control.
Plus two optional cleanups (dead parsed, unused assertPaymentPolicyApproved).
Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
Follow the repo-wide BigInt money convention instead of Number() so the policy no longer loses precision or accepts fractional/non-positive/empty amounts the ACK-Pay schema's string branch permits. Replace the single flat maxAutonomousAmount with a per-currency map keyed on currency code, expressed in each currency's subunits, so the threshold is coherent across USD (2dp) and USDC (6dp); currencies with no configured limit are denied. Remove the unused assertPaymentPolicyApproved export. Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
enforcePaymentPolicy hardcoded maxAutonomousAmount, bypassing the exported demoPaymentPolicy default. Spread demoPaymentPolicy and override only allowedRecipients so the exported config is the single source of truth. Also drop the now-dead parsed value from validatePaymentOption (no caller consumes it after the issuer-comparison iteration was removed). Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
Reword the policy example so the amount check reads as an illustrative per-transaction cap rather than a true spend control, and call out that it is trivially split-gameable (cap x N) without a cumulative/rate-limited budget. Document the per-currency, subunit-denominated limit and that unconfigured currencies are denied. Signed-off-by: EfeDurmaz16 <efebarandurmaz05@gmail.com>
EfeDurmaz16
force-pushed
the
demo/ack-pay-policy-before-signing
branch
from
f0f5df8 to
4cfc2e7
Compare
|
Thanks for the thorough review — all requested changes addressed, and rebased onto latest
Tests extended to cover the per-currency limit, unconfigured currencies, non-positive amounts, and malformed string amounts. Verification:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@demos/payments/src/payment-policy.test.ts`:
- Around line 14-133: Rename each it(...) test title to an assertive form using
the required verbs (returns/creates/requires/throws) while keeping the rest of
the description and the same test bodies; update titles referencing
evaluatePaymentPolicy/basePaymentOption accordingly (e.g., change "approves
below-threshold payments to allowed recipients" to "returns approved decision
for below-threshold payments to allowed recipients", "approves string subunit
amounts within the limit" to "returns approved decision for string subunit
amounts within the limit", "does not approve self-asserted recipients without an
allowlist" to "requires approval for self-asserted recipients without an
allowlist", "requires approval before execution for unknown recipients" to
"requires approval for unknown recipients", "denies payments above the
autonomous spend limit" to "returns denied decision for payments above the
autonomous spend limit", "applies the per-currency limit in the currency's own
subunits" to "returns approved decision when per-currency limits are respected",
"denies currencies with no configured limit" to "returns denied decision for
currencies with no configured limit", "denies non-positive amounts" to "returns
denied decision for non-positive amounts", etc.; ensure all remaining it(...)
titles follow the same assertive verb pattern.
In `@demos/payments/src/payment-policy.ts`:
- Around line 61-69: The code currently reads limit =
policy.maxAutonomousAmount[paymentOption.currency] and then compares amount >
limit, but that can pick up inherited prototype keys (e.g., "constructor") and
cause a crash; change the lookup to only accept own properties and numeric
values — e.g., use
Object.prototype.hasOwnProperty.call(policy.maxAutonomousAmount,
paymentOption.currency) (or ensure maxAutonomousAmount is Object.create(null))
before assigning/using limit, and also verify Number.isFinite(limit); if the
property is not an own property or not a finite number, treat it as undefined
and return the deterministic denial branch used for missing limits. Ensure
references: policy.maxAutonomousAmount, paymentOption.currency, amount, and
limit are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ec54b36-83ae-4acf-817e-220dc938c55b
📒 Files selected for processing (4)
demos/payments/README.mddemos/payments/src/payment-policy.test.tsdemos/payments/src/payment-policy.tsdemos/payments/src/payment-service.ts
| it("approves below-threshold payments to allowed recipients", () => { | ||
| const decision = evaluatePaymentPolicy(basePaymentOption, { | ||
| allowedRecipients: [basePaymentOption.recipient], | ||
| maxAutonomousAmount: { USDC: 1_000n }, | ||
| }) | ||
|
|
||
| expect(decision).toEqual({ | ||
| status: "approved", | ||
| }) | ||
| }) | ||
|
|
||
| it("approves string subunit amounts within the limit", () => { | ||
| const decision = evaluatePaymentPolicy( | ||
| { ...basePaymentOption, amount: "50000" }, | ||
| { | ||
| allowedRecipients: [basePaymentOption.recipient], | ||
| maxAutonomousAmount: { USDC: 5_000_000n }, | ||
| }, | ||
| ) | ||
|
|
||
| expect(decision).toEqual({ | ||
| status: "approved", | ||
| }) | ||
| }) | ||
|
|
||
| it("does not approve self-asserted recipients without an allowlist", () => { | ||
| const decision = evaluatePaymentPolicy(basePaymentOption, { | ||
| allowedRecipients: [], | ||
| maxAutonomousAmount: { USDC: 1_000n }, | ||
| }) | ||
|
|
||
| expect(decision).toEqual({ | ||
| status: "approval_required", | ||
| reason: "Recipient is not on the autonomous payment allowlist", | ||
| }) | ||
| }) | ||
|
|
||
| it("requires approval before execution for unknown recipients", () => { | ||
| const decision = evaluatePaymentPolicy(basePaymentOption, { | ||
| allowedRecipients: ["did:example:trusted-merchant"], | ||
| maxAutonomousAmount: { USDC: 1_000n }, | ||
| }) | ||
|
|
||
| expect(decision).toEqual({ | ||
| status: "approval_required", | ||
| reason: "Recipient is not on the autonomous payment allowlist", | ||
| }) | ||
| }) | ||
|
|
||
| it("denies payments above the autonomous spend limit", () => { | ||
| const decision = evaluatePaymentPolicy( | ||
| { | ||
| ...basePaymentOption, | ||
| amount: 10_000, | ||
| }, | ||
| { | ||
| allowedRecipients: [basePaymentOption.recipient], | ||
| maxAutonomousAmount: { USDC: 1_000n }, | ||
| }, | ||
| ) | ||
|
|
||
| expect(decision).toEqual({ | ||
| status: "denied", | ||
| reason: "Payment amount exceeds the autonomous spend limit", | ||
| }) | ||
| }) | ||
|
|
||
| it("applies the per-currency limit in the currency's own subunits", () => { | ||
| const policy = { | ||
| allowedRecipients: [basePaymentOption.recipient], | ||
| // 5.00 USD (2dp) and 5.000000 USDC (6dp) — same value, different subunits | ||
| maxAutonomousAmount: { USD: 500n, USDC: 5_000_000n }, | ||
| } | ||
|
|
||
| // 4.00 USD is below the USD limit | ||
| expect( | ||
| evaluatePaymentPolicy( | ||
| { ...basePaymentOption, amount: 400, decimals: 2, currency: "USD" }, | ||
| policy, | ||
| ), | ||
| ).toEqual({ status: "approved" }) | ||
|
|
||
| // The same 400 subunits in USDC (0.0004) is also below the USDC limit, | ||
| // confirming each currency is bounded by its own threshold | ||
| expect( | ||
| evaluatePaymentPolicy({ ...basePaymentOption, amount: 400 }, policy), | ||
| ).toEqual({ status: "approved" }) | ||
| }) | ||
|
|
||
| it("denies currencies with no configured limit", () => { | ||
| const decision = evaluatePaymentPolicy( | ||
| { ...basePaymentOption, currency: "SOL", decimals: 9 }, | ||
| { | ||
| allowedRecipients: [basePaymentOption.recipient], | ||
| maxAutonomousAmount: { USDC: 1_000n }, | ||
| }, | ||
| ) | ||
|
|
||
| expect(decision).toEqual({ | ||
| status: "denied", | ||
| reason: "No autonomous spend limit configured for currency SOL", | ||
| }) | ||
| }) | ||
|
|
||
| it("denies non-positive amounts", () => { | ||
| const decision = evaluatePaymentPolicy( | ||
| { ...basePaymentOption, amount: 0 }, | ||
| { | ||
| allowedRecipients: [basePaymentOption.recipient], | ||
| maxAutonomousAmount: { USDC: 1_000n }, | ||
| }, | ||
| ) | ||
|
|
||
| expect(decision).toEqual({ | ||
| status: "denied", | ||
| reason: "Payment amount must be greater than zero", | ||
| }) | ||
| }) | ||
|
|
||
| it("denies fractional or malformed amounts the schema permits as strings", () => { |
There was a problem hiding this comment.
Rename test cases to the required assertive pattern.
Current it(...) titles don’t follow the enforced creates/throws/requires/returns naming convention for *.test.ts files. Please rename these test names accordingly (for example, it("returns approved decision..."), it("requires approval..."), it("returns denied decision...")).
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 `@demos/payments/src/payment-policy.test.ts` around lines 14 - 133, Rename each
it(...) test title to an assertive form using the required verbs
(returns/creates/requires/throws) while keeping the rest of the description and
the same test bodies; update titles referencing
evaluatePaymentPolicy/basePaymentOption accordingly (e.g., change "approves
below-threshold payments to allowed recipients" to "returns approved decision
for below-threshold payments to allowed recipients", "approves string subunit
amounts within the limit" to "returns approved decision for string subunit
amounts within the limit", "does not approve self-asserted recipients without an
allowlist" to "requires approval for self-asserted recipients without an
allowlist", "requires approval before execution for unknown recipients" to
"requires approval for unknown recipients", "denies payments above the
autonomous spend limit" to "returns denied decision for payments above the
autonomous spend limit", "applies the per-currency limit in the currency's own
subunits" to "returns approved decision when per-currency limits are respected",
"denies currencies with no configured limit" to "returns denied decision for
currencies with no configured limit", "denies non-positive amounts" to "returns
denied decision for non-positive amounts", etc.; ensure all remaining it(...)
titles follow the same assertive verb pattern.
| const limit = policy.maxAutonomousAmount[paymentOption.currency] | ||
| if (limit === undefined) { | ||
| return { | ||
| status: "denied", | ||
| reason: `No autonomous spend limit configured for currency ${paymentOption.currency}`, | ||
| } | ||
| } | ||
|
|
||
| if (amount > limit) { |
There was a problem hiding this comment.
Prevent inherited-key crashes when resolving per-currency limits.
policy.maxAutonomousAmount[paymentOption.currency] can resolve inherited keys (for example "constructor"), which makes amount > limit throw and turns an invalid input into a 500 path instead of a deterministic policy denial.
Suggested fix
- const limit = policy.maxAutonomousAmount[paymentOption.currency]
- if (limit === undefined) {
+ if (
+ !Object.prototype.hasOwnProperty.call(
+ policy.maxAutonomousAmount,
+ paymentOption.currency,
+ )
+ ) {
return {
status: "denied",
reason: `No autonomous spend limit configured for currency ${paymentOption.currency}`,
}
}
+ const limit = policy.maxAutonomousAmount[paymentOption.currency]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const limit = policy.maxAutonomousAmount[paymentOption.currency] | |
| if (limit === undefined) { | |
| return { | |
| status: "denied", | |
| reason: `No autonomous spend limit configured for currency ${paymentOption.currency}`, | |
| } | |
| } | |
| if (amount > limit) { | |
| if ( | |
| !Object.prototype.hasOwnProperty.call( | |
| policy.maxAutonomousAmount, | |
| paymentOption.currency, | |
| ) | |
| ) { | |
| return { | |
| status: "denied", | |
| reason: `No autonomous spend limit configured for currency ${paymentOption.currency}`, | |
| } | |
| } | |
| const limit = policy.maxAutonomousAmount[paymentOption.currency] | |
| if (amount > limit) { |
🤖 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 `@demos/payments/src/payment-policy.ts` around lines 61 - 69, The code
currently reads limit = policy.maxAutonomousAmount[paymentOption.currency] and
then compares amount > limit, but that can pick up inherited prototype keys
(e.g., "constructor") and cause a crash; change the lookup to only accept own
properties and numeric values — e.g., use
Object.prototype.hasOwnProperty.call(policy.maxAutonomousAmount,
paymentOption.currency) (or ensure maxAutonomousAmount is Object.create(null))
before assigning/using limit, and also verify Number.isFinite(limit); if the
property is not an own property or not a finite number, treat it as undefined
and return the deterministic denial branch used for missing limits. Ensure
references: policy.maxAutonomousAmount, paymentOption.currency, amount, and
limit are updated accordingly.
2 participants
Summary
Fixes #91
Review follow-ups (addressed)
BigInt(paymentOption.amount)), with an explicit positive-integer guard so fractional/non-positive/empty amounts the schema permits are denied instead of passing policy.demoPaymentPolicyis now the single source of truth —enforcePaymentPolicyspreads it and overrides onlyallowedRecipients, dropping the hardcoded literal.parsedvalue and the unusedassertPaymentPolicyApprovedexport.main.Verification
AI Usage Disclosure
This contribution was AI-assisted. Initial work used Codex CLI and the Codex app; the review follow-ups above were 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
New Features
Documentation
Tests