Skip to content

fix(x402): bind signed payment to the approved payment option - #1404

Open
SashaMIT wants to merge 3 commits into
coinbase:mainfrom
SashaMIT:fix/x402-bind-signed-payment-to-approved-option
Open

fix(x402): bind signed payment to the approved payment option#1404
SashaMIT wants to merge 3 commits into
coinbase:mainfrom
SashaMIT:fix/x402-bind-signed-payment-to-approved-option

Conversation

@SashaMIT

@SashaMIT SashaMIT commented Aug 6, 2026

Copy link
Copy Markdown

Summary

retry_http_request_with_x402 asks the user (or agent policy) to approve a specific payment option, validates that option against maxPaymentUsdc — and then signs whatever the server returns on the retry's 402, because wrapFetchWithPayment receives a plain x402Client with the default accepts[0] selector. The approved option is never enforced on the payload that actually gets signed.

A registered service can therefore present a cheap option at confirmation time and demand a larger amount — or a different recipient — when the retry happens. The maxPaymentUsdc check gives false assurance because it runs against the approved option's amount, not the signed one.

Fix

Pass a payment-requirements selector into the x402Client constructor (the hook the x402 SDK provides for exactly this):

  • Retry path (retry_http_request_with_x402): createApprovedPaymentSelector accepts only requirements matching the approved option on network/asset/payTo at an amount not exceeding the approved amount; anything else throws before any signature is produced.
  • Direct path (http_request_with_x402, which by design skips user confirmation): createCappedPaymentSelector refuses requirements above the configured maxPaymentUsdc, so the provider's own spending limit also binds what actually gets signed there.

Test plan

  • New utils.selector.test.ts: matching requirement selected; inflated amount refused; swapped recipient refused; wrong network refused; v2 price field handled; cap selector accepts-at/refuses-above the limit
  • Full x402 suite: 28/28 pass; tsc --noEmit clean; eslint/prettier clean

Made with Cursor

retry_http_request_with_x402 validates the user-approved payment option
against the spending limit, but then calls wrapFetchWithPayment with a
plain x402Client, which signs whatever payment requirements the server
returns on the retry's 402. A registered service could present a cheap
option at confirmation time and demand a larger amount (or different
recipient) at retry time.

Pass a payment-requirements selector into the x402Client:

- retry path: selector accepts only requirements matching the approved
  option on network/asset/payTo at an amount not exceeding the approved
  amount; anything else throws before signing.
- direct (no-confirmation) path: selector caps the signed amount at the
  configured maxPaymentUsdc, matching the limit the confirmation path
  already enforces.

Made-with: Cursor
@SashaMIT
SashaMIT requested a review from murrlincoln as a code owner August 6, 2026 15:53
@cb-heimdall

Copy link
Copy Markdown

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@0rkz

0rkz commented Aug 15, 2026

Copy link
Copy Markdown

Nice fix on the retry path — pinning network/asset/payTo against the approved option (createApprovedPaymentSelector, utils.ts:775-794) closes the exact TOCTOU the PR describes, and gating it on the pre-signature isUsdcAsset + validatePaymentLimit + network checks in retryWithX402 (x402ActionProvider.ts:400, :418, :436) is solid defense in depth.

The direct path's binding looks narrower than the retry path's, though, and I think it's narrower than intended. createCappedPaymentSelector (utils.ts:803-815) only constrains the numeric amount:

export function createCappedPaymentSelector(maxPaymentUsdc: number) {
  const cap = parseUnits(maxPaymentUsdc.toString(), USDC_DECIMALS);
  return <T extends PaymentRequirementLike>(_x402Version: number, accepts: T[]): T => {
    const match = accepts.find(req => BigInt(req.maxAmountRequired ?? req.amount ?? "0") <= cap);
    ...

PaymentRequirementLike.asset is declared (utils.ts:744) but never read in this function. makeHttpRequestWithX402 (x402ActionProvider.ts:579-691) never calls isUsdcAsset before or around createCappedPaymentSelector(this.config.maxPaymentUsdc) (:621) the way retryWithX402 does at :400 — and there's no network-compatibility check either (retry path has one at :436). Registration in createX402Client (:857-887) registers no policies, only the EVM/SVM exact-scheme signer.

I checked whether anything upstream fills that gap and it doesn't. In @x402/core@2.7.0 (the version typescript/pnpm-lock.yaml pins here), x402Client.selectPaymentRequirements filters the server's accepts only by registered network+scheme, then applies registered policies, then hands the survivors to the caller-supplied selector as the last step — no asset check at any stage. The same holds on 2.22.0, which ^2.7.0 resolves to on a fresh install: it adds a paymentFlow filter, still nothing on asset. The network+scheme filter doesn't narrow it either, since registerExactEvmScheme is called here without a networks option (x402ActionProvider.ts:880) and so registers the wildcard eip155:*, which findSchemesByNetwork expands to ^eip155:.*$. And the EVM exact scheme client itself builds directly from paymentRequirements.asset with no allowlist of its own — signEIP3009Authorization uses it as the EIP-712 verifyingContract, createPermit2Payload as the permitted token. So isUsdcAsset is the only guard against a non-USDC asset anywhere in this call chain, and on the direct path it's never invoked.

Concretely: with maxPaymentUsdc = 1.0 (the default, x402ActionProvider.ts:70 — cap = 1_000_000n at USDC's 6 decimals), a registered service's live 402 for make_http_request_with_x402 can return accepts with any asset address and any maxAmountRequired numerically ≤ 1_000_000 — the selector doesn't care what token that number is denominated in. Since this is explicitly the "automatically handles payments without asking for confirmation" path (per the action's own description), there's no other point where a human or a USDC check would catch a requirement that's numerically small but denominated in a token worth far more than $1/atomic-unit-equivalent.

Suggested fix: give createCappedPaymentSelector the same asset guard retryWithX402 gets for free from its pre-check, e.g. resolve the expected USDC address once (via isUsdcAsset/walletProvider, same as filterUsdcPaymentOptions already does at utils.ts:663) and require isUsdcAsset(req.asset, walletProvider) inside the accepts.find(...) predicate before the amount comparison — mirroring the retry path's isUsdcAsset(args.selectedPaymentOption.asset, walletProvider) check at x402ActionProvider.ts:400, just moved inside the selector since the direct path only learns the live requirements at selector-call time. Worth a case in utils.selector.test.ts alongside the existing capped-selector tests: a requirement within the numeric cap but on a non-USDC asset should be refused the same way the inflated-amount and swapped-recipient cases are on the retry-path selector.

(Disclosure: I work on PayPerByte; we run a seller/facilitator stack on x402, so payer-side spending-cap enforcement is directly relevant to what we build. Drafted with AI assistance.)

The numeric cap alone accepted any asset whose atomic amount was
within maxPaymentUsdc. Require the wallet's USDC address and an
allowed network before signing on the no-confirmation path.
@github-actions github-actions Bot added action provider New action provider typescript labels Aug 15, 2026
@SashaMIT

Copy link
Copy Markdown
Author

That's a sharp catch, and you're right that the numeric cap alone was not enough.

The retry path already bound network/asset/payTo before signing. The direct path only compared maxAmountRequired against the USDC cap, so a live 402 could return a numerically small amount on a different token (or an unsupported network) and still get signed.

I pushed a follow-up that gives createCappedPaymentSelector the same asset guard, plus the network check you also called out. The selector now takes isAllowedAsset and optional allowedNetworks. makeHttpRequestWithX402 passes isUsdcAsset and getX402Networks. Added two tests: within-cap non-USDC asset, and within-cap USDC on an unsupported network, both refuse.

Happy to adjust the helper shape if you'd rather see the walletProvider passed through directly.

@0rkz

0rkz commented Aug 15, 2026

Copy link
Copy Markdown

Verified against the pinned head (3d3419974b225fd69fe82e3156cb43c5e1855295).

createCappedPaymentSelector (utils.ts:808-832) now takes isAllowedAsset and optional allowedNetworks, and both are checked inside the accepts.find(...) predicate before the amount comparison (utils.ts:818-822). The call site wires it correctly: isAllowedAsset: asset => isUsdcAsset(asset, walletProvider) and allowedNetworks: getX402Networks(walletProvider.getNetwork()) (x402ActionProvider.ts:621-623) — the same getX402Networks the retry path already relies on for its own network check.

Traced the refusal path end to end rather than trusting the throw in isolation: @x402/core@2.7.0's selectPaymentRequirements calls the injected selector outside createPaymentPayload's try/catch, so a thrown error there isn't caught internally; @x402/fetch@2.7.0's wrapFetchWithPayment wraps client.createPaymentPayload(...) in a try/catch that re-throws (not swallows); and makeHttpRequestWithX402's own try/catch turns that into an error: true JSON response (utils.ts:402 handleHttpError). The accepts[0] fallback in @x402/core only applies when no selector is registered at all — once a selector is present, its throw is honored all the way out. So the two new tests (refuses a within-cap requirement on a non-USDC asset, refuses a within-cap USDC requirement on an unsupported network, utils.selector.test.ts) are asserting a refusal that's real end-to-end, not just a local throw.

Extracted the exact predicate and ran it directly (not just read it) against a few adversarial fixtures: a first-bad/second-good accepts array where entry 0 fails on asset, network, or amount and entry 1 is valid — find() correctly skips to entry 1 in all three orderings. Also checked the asset comparison for the EIP-55/lowercase divergence we've flagged elsewhere (canonicalize/x402): isUsdcAsset does a plain .toLowerCase() string compare on the EVM path (utils.ts:522), not viem's getAddress, so there's no silent re-checksumming and no case-sensitivity gap here. isUsdcAsset is also network-aware in a way that matters: it keys off walletProvider.getNetwork() to pick the wallet's own per-network USDC address (this repo's own constants have different USDC addresses for base-mainnet vs base-sepolia), so the asset check alone already discriminates by chain in the common case.

Ran the actual suite at this head rather than eyeballing it: 9/9 in utils.selector.test.ts, 30/30 across action-providers/x402/, tsc --noEmit clean.

Two things worth naming precisely, neither of which is live in the shipped path:

  • allowedNetworks is optional in the signature, and if omitted the network check is skipped entirely (utils.ts:819) — asset alone doesn't bind to a chain, since isAllowedAsset only compares the address string. I confirmed this empirically: a requirement citing the wallet's own-network USDC address but a different network id still gets selected when allowedNetworks isn't passed. Not exploitable today — the one call site always supplies it — but it's an easy way to reintroduce a variant of this exact bug from a future call site. Might be worth making it required rather than optional.
  • The direct path still doesn't bind payTo the way createApprovedPaymentSelector does for the retry path. I don't think that's a gap, though: the retry path has something concrete to bind against (the ApprovedPaymentOption the user already saw and approved, payTo included). The direct path has no equivalent reference — registration is by URL only, with no pre-approved recipient — so there's nothing for it to check payTo against. Flagging it since it was explicitly worth checking, not because I think it should change.

On the helper shape: I'd lean toward keeping isAllowedAsset + allowedNetworks over threading walletProvider through, though it's a close call. isUsdcAsset already requires a walletProvider argument, so passing the provider into the selector wouldn't remove that dependency, just move it — the closure at the call site does the same binding with less coupling. It's also consistent with createApprovedPaymentSelector right above it, which takes a plain ApprovedPaymentOption rather than a provider. And it's the difference between the two new tests passing a bare function versus having to construct or mock EvmWalletProvider/SvmWalletProvider instances to satisfy the instanceof checks inside isUsdcAsset (utils.ts:517, :526).

The honest counterpoint, since it cuts against the residual above: threading walletProvider would let the selector derive the allowed networks itself, which makes that optional-parameter footgun structurally impossible rather than just documented. If you'd rather close it that way than by making allowedNetworks required, that seems like a defensible trade — the required-parameter version just looked like the smaller change.

(Disclosure: I work on PayPerByte; we run a seller/facilitator stack on x402, so payer-side spending-cap enforcement is directly relevant to what we build. Drafted with AI assistance.)

An omitted network list skipped the chain check entirely. The one
call site already passed getX402Networks. Make the field required
so a future caller cannot reintroduce that gap.
@SashaMIT

Copy link
Copy Markdown
Author

Thanks for tracing it end to end. Glad the two new tests line up with a real refusal through @x402/core and @x402/fetch, not just a local throw.

You're right that allowedNetworks being optional was a footgun. The one call site already passed getX402Networks, but a future caller could omit it and skip the chain check. I made the field required (f7095fa) so that path cannot come back. Left isAllowedAsset as the predicate rather than threading walletProvider, same as you leaned.

Agreed on payTo: the direct path has no approved recipient to bind against, so there is nothing to check there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants