fix(x402): validate server payment requirements before signing in pay and eip3009AuthenticatedFetch#11
Open
Nexory wants to merge 1 commit into
Conversation
The amount, recipient, and asset in an x402 402 response are server-controlled. `paidFetch` validates them before signing (validatePaymentRequirements: payTo is not a burn address, asset defaults to the network USDC, optional maxAmount/allowedRecipients), but two other client paths did not: - `pay` (runPaymentOnly) passed the server's requirements straight to signX402Payment with no checks, so a hostile endpoint could have the caller sign a transfer of an arbitrary asset/amount to an arbitrary address. It now runs validatePaymentRequirements with a default 1 USDC cap, overridable via --max-amount (mirroring `smoke`). - eip3009AuthenticatedFetch is documented as signing a zero-value proof of wallet control, but it forwarded the server's maxAmountRequired to signX402Payment, so a value-bearing requirement was signed. It now signs only when the requirement is zero-value; a value-bearing 402 is returned as-is. validatePaymentRequirements is exported so `pay` can reuse it. Adds regression tests for both paths.
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.
What
Two client-side x402 paths sign an EIP-3009 transfer authorization using the amount, recipient, and asset from the server's
402response without validating them.paidFetchalready guards this; these two did not. This wires the same guard into both.Why
The values in an x402
accepts[]entry are server-controlled.paidFetchrunsvalidatePaymentRequirementsbefore signing (rejects a burn-addresspayTo, defaultsassetto the network USDC, optionalmaxAmount/allowedRecipients), and its JSDoc notes that "a compromised server can request payment to an attacker-controlled address or for an inflated amount." Two other paths skip that:pay(runPaymentOnly) passes the server's requirements straight tosignX402Payment(src/cli/commands/pay.ts). A hostile endpoint thattool-sdk pay <url>is pointed at can have the caller sign a transfer of an arbitrary asset and amount to an arbitrary address.eip3009AuthenticatedFetchis documented as signing a zero-value proof of wallet control (src/lib/client/eip3009-auth.ts), but it forwards the server'smaxAmountRequiredtosignX402Payment, so a value-bearing402got signed. The siblingsmokecommand validates asset and amount before callingsignX402Payment; this path did not.Change
validatePaymentRequirementsfromx402-payment.tsand call it inrunPaymentOnly, with a default1 USDCcap overridable via a new--max-amountflag (mirrorssmoke).extractPaymentRequirements, only accept a requirement when it is zero-value (maxAmountRequiredis"0"or absent); a value-bearing402is returned as-is instead of being signed.Existing behavior is unchanged for honest servers: a USDC requirement within the cap still signs and replays, and a zero-value
402still produces the auth proof.Tests
Adds one regression test per path. Against the current code both fail:
eip3009AuthenticatedFetchsigns and replays the value-bearing challenge:paysigns without validation:With the fix both pass and the full suite stays green: