Repository navigation
fix(tempo): widen the sponsored validBefore window to 60s and enforce the 15s floor at issuance - #1099
fix(tempo): widen the sponsored validBefore window to 60s and enforce the 15s floor at issuance#1099brendanjryan wants to merge 2 commits into
Conversation
… the 15s floor at issuance
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![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: 06eeb5fef8
ℹ️ 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".
| if (methodDetails?.feePayer === true && challenge.expires) { | ||
| const remainingMs = new Date(challenge.expires).getTime() - Date.now() | ||
| if (!(remainingMs >= defaults.minRemainingValiditySeconds * 1000)) |
There was a problem hiding this comment.
When a sponsored challenge has only slightly more than 15 seconds remaining, this check can pass and then the awaited account resolution, swap/fee-token lookups, or prepareTransactionRequest can consume the remaining margin. The client subsequently signs a transaction capped at the challenge expiry even though the fee payer will reject it for having under 15 seconds left. Revalidate after asynchronous preparation, immediately before signing, so the promised refusal happens at the relevant time.
Useful? React with 👍 / 👎.
| if ( | ||
| !credential && | ||
| resolvedFeePayer && | ||
| expires !== undefined && | ||
| !(Date.parse(expires) - Date.now() >= defaults.minRemainingValiditySeconds * 1000) |
There was a problem hiding this comment.
When a request includes a credential, !credential bypasses the floor, but the recomputed route challenge is later emitted as the retry challenge for malformed, expired, mismatched, or rejected credentials. If a sponsored handler has a fixed expiry that has fallen below 15 seconds since the original issuance, this returns a sponsored retry challenge that the new client check always refuses to sign. Preserve verification of the echoed credential, but apply the floor before emitting a newly issued retry challenge.
Useful? React with 👍 / 👎.
| if ( | ||
| !credential && | ||
| resolvedFeePayer && | ||
| expires !== undefined && | ||
| !(Date.parse(expires) - Date.now() >= defaults.minRemainingValiditySeconds * 1000) |
There was a problem hiding this comment.
A fee-payer-configured route can issue zero-amount challenges, which require proof credentials and never create a transaction, and can also be configured with only push mode, whose hash credential is broadcast by the client without sponsorship. Because this condition looks only at resolvedFeePayer, both valid flows are rejected when their expiry is under 15 seconds even though no fee payer applies. Gate the floor on a request that can actually produce a sponsored pull transaction.
Useful? React with 👍 / 👎.
| if (methodDetails?.feePayer === true && challenge.expires) { | ||
| const remainingMs = new Date(challenge.expires).getTime() - Date.now() | ||
| if (!(remainingMs >= defaults.minRemainingValiditySeconds * 1000)) |
There was a problem hiding this comment.
If a fee-payer-advertising challenge has aged below 15 seconds but the selected mode is push, this check rejects it before mode resolution even though the push branch broadcasts an ordinary transaction and never asks the fee payer to co-sign. This especially affects JSON-RPC accounts, which prefer push mode, and prevents them from satisfying an otherwise unexpired challenge. Resolve the mode first and apply the remaining-validity floor only to sponsored pull transactions.
Useful? React with 👍 / 👎.
| capturedRequest: parameters.capturedRequest, | ||
| credential: parameters.credential, | ||
| expires: parameters.expires, |
There was a problem hiding this comment.
In the normal handler path, parameters.expires is the route expiry recalculated for the current invocation—by default, five minutes from now—not the expiry echoed in parameters.credential.challenge. Consequently, a request hook using the newly documented expires field receives a different value during credential verification than it received when issuing the challenge, which can change its canonical request or invalidate expiry-dependent checks. Select the credential's echoed expiry whenever a credential is present.
Useful? React with 👍 / 👎.
| 'mppx': patch | ||
| --- | ||
|
|
||
| Widened the Tempo charge client's expiring-nonce `validBefore` window from 25 to 60 seconds (still capped at the challenge expiry), leaving room for clock skew against the fee payer's 15-second remaining-validity minimum. Clients now refuse to sign a sponsored challenge that expires in under 15 seconds, and servers refuse to issue one. |
There was a problem hiding this comment.
The final sentence uses present-tense wording (Clients now refuse and servers refuse), so the generated changelog violates the repository requirement that changeset descriptions describe what the change did in past tense. Rewrite that sentence in past tense before release.
AGENTS.md reference: AGENTS.md:L180-L180
Useful? React with 👍 / 👎.
| if (methodDetails?.feePayer === true && challenge.expires) { | ||
| const remainingMs = new Date(challenge.expires).getTime() - Date.now() | ||
| if (!(remainingMs >= defaults.minRemainingValiditySeconds * 1000)) |
There was a problem hiding this comment.
The new remaining-validity rejection runs only in createCredential, while canHandleChallenge still reports the same challenge as acceptable. In a multi-challenge response, a short-lived sponsored pull offer can therefore remain the highest-ranked candidate, be selected over a usable alternative, and then fail during credential creation without candidate fallback. Apply the same branch-aware expiry policy in challenge selection so an unusable offer is skipped.
Useful? React with 👍 / 👎.
| * remaining validity when they co-sign, so a 60s window leaves about 45s for | ||
| * clock skew between client and server, signing and round trips. | ||
| */ | ||
| export const validityWindowSeconds = 60 |
There was a problem hiding this comment.
Needs review: 60s client validBefore outlives server's 30s recovery deadline, enabling double charge
The PR widens the default client validBefore to 60s but leaves the server's 30s receipt-recovery deadline unchanged. After an ambiguous broadcast, the server stops polling, skips its final lookup, and re-issues a fresh 402. The client then pays the new challenge automatically while the first signed transaction can still be mined.
Valid? React with 👍 / 👎.
| if ( | ||
| !credential && | ||
| resolvedFeePayer && | ||
| expires !== undefined && | ||
| !(Date.parse(expires) - Date.now() >= defaults.minRemainingValiditySeconds * 1000) | ||
| ) | ||
| throw new Error( | ||
| `Sponsored charge challenges must expire at least ${defaults.minRemainingValiditySeconds} seconds after issuance.`, |
There was a problem hiding this comment.
Medium (exposure unconfirmed): Sponsored expiry floor blocks short-lived zero-dollar authentication challenges
The new sponsored-challenge issuance floor fires whenever a fee payer is configured, including for zero-amount proof challenges. A zero-dollar authentication route with a fee payer configured and an expiry under 15 seconds now throws instead of issuing its challenge, even though proof credentials never need a sponsored transaction.
Valid? React with 👍 / 👎.
| /** | ||
| * Seconds from now the client sets as the expiring-nonce `validBefore`, | ||
| * capped at the challenge expiry. | ||
| * | ||
| * Fee payers require at least `defaults.minRemainingValiditySeconds` (15s) of | ||
| * remaining validity when they co-sign, so a 60s window leaves about 45s for | ||
| * clock skew between client and server, signing and round trips. | ||
| */ | ||
| export const validityWindowSeconds = 60 |
There was a problem hiding this comment.
Medium (exposure unconfirmed): Fixed 60s client validBefore exceeds sponsor windows below 60s
The Tempo charge client now always sets validBefore to now+60s unless the challenge expires sooner. The server still accepts feePayerPolicy.maxValidityWindowSeconds as low as 15 and rejects any sponsored transaction whose validBefore exceeds now plus that window. Servers configured with a window under about 60s therefore reject every default sponsored credential for normally expiring challenges.
Valid? React with 👍 / 👎.
| if (methodDetails?.feePayer === true && challenge.expires) { | ||
| const remainingMs = new Date(challenge.expires).getTime() - Date.now() | ||
| if (!(remainingMs >= defaults.minRemainingValiditySeconds * 1000)) | ||
| throw new Error( | ||
| `Sponsored challenge expires in less than ${defaults.minRemainingValiditySeconds} seconds; the fee payer would reject the transaction. Request a new challenge.`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Medium: Sponsor expiry guard blocks self-funded push payments
The Tempo charge client refuses any challenge that advertises a fee payer and has less than 15 seconds left. The check runs before the payment mode is chosen. Push payments never ask the merchant for a co-signature, so they are wrongly blocked even though the server would accept them.
Valid? React with 👍 / 👎.
| /** Expiry (ISO 8601) of the challenge being issued, or echoed by `credential`. */ | ||
| expires?: string | undefined |
There was a problem hiding this comment.
Informational (exposure unconfirmed): Request hook expires differs between handler and standalone verification paths
The new RequestContext.expires field is documented as the expiry of the challenge being issued, or the one echoed by the credential. When a credential is present, the HTTP handler gives the hook a newly generated expiry, while standalone verification gives it the echoed one. Custom request hooks therefore see different values for the same credential.
Valid? React with 👍 / 👎.
| // Issued sponsored challenges must leave room for the fee payer's | ||
| // minimum remaining validity, or every credential for them is rejected. | ||
| if ( | ||
| !credential && | ||
| resolvedFeePayer && | ||
| expires !== undefined && | ||
| !(Date.parse(expires) - Date.now() >= defaults.minRemainingValiditySeconds * 1000) | ||
| ) | ||
| throw new Error( | ||
| `Sponsored charge challenges must expire at least ${defaults.minRemainingValiditySeconds} seconds after issuance.`, | ||
| ) |
There was a problem hiding this comment.
Informational (exposure unconfirmed): Sponsored issuance floor skipped for retry challenges on credential-bearing requests
The tempo charge request hook only applies the new 15-second issuance floor when no credential is present. The route handler builds its retry 402 challenge from the credential-bearing hook call, using the route's fresh expires. So a sponsored route with a short or nearly reached expiry throws for bare requests but re-issues a short-expiry sponsored challenge when a failing credential…
Full finding
The tempo charge request hook only applies the new 15-second issuance floor when no credential is present. The route handler builds its retry 402 challenge from the credential-bearing hook call, using the route's fresh expires. So a sponsored route with a short or nearly reached expiry throws for bare requests but re-issues a short-expiry sponsored challenge when a failing credential is attached.
Valid? React with 👍 / 👎.
| // Issued sponsored challenges must leave room for the fee payer's | ||
| // minimum remaining validity, or every credential for them is rejected. | ||
| if ( | ||
| !credential && | ||
| resolvedFeePayer && | ||
| expires !== undefined && | ||
| !(Date.parse(expires) - Date.now() >= defaults.minRemainingValiditySeconds * 1000) | ||
| ) | ||
| throw new Error( | ||
| `Sponsored charge challenges must expire at least ${defaults.minRemainingValiditySeconds} seconds after issuance.`, | ||
| ) |
There was a problem hiding this comment.
Informational (exposure unconfirmed): Sponsored issuance floor refuses push-only charge challenges
The tempo charge request hook now refuses to issue a challenge whenever a fee payer is configured and the expiry is under 15 seconds. It does this even when supportedModes is ['push'], where fee-payer co-signing and its 15-second minimum can never apply. Such routes worked before this change and now throw on every unauthenticated request.
Valid? React with 👍 / 👎.
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review — 2 findings.
- **Needs review · unverified:** 60s expiring-nonce validBefore likely exceeds Tempo's expiring-nonce maximum expiry
- **Informational (exposure unconfirmed) · unverified:** Client sponsored-expiry guard uses the client clock without skew tolerance and fails…
| /** | ||
| * Seconds from now the client sets as the expiring-nonce `validBefore`, | ||
| * capped at the challenge expiry. | ||
| * | ||
| * Fee payers require at least `defaults.minRemainingValiditySeconds` (15s) of | ||
| * remaining validity when they co-sign, so a 60s window leaves about 45s for | ||
| * clock skew between client and server, signing and round trips. | ||
| */ | ||
| export const validityWindowSeconds = 60 |
There was a problem hiding this comment.
Needs review · unverified: 60s expiring-nonce validBefore likely exceeds Tempo's expiring-nonce maximum expiry
Mechanism: the PR raises the Tempo charge client's default expiring-nonce validBefore from now+25 to now+60 (src/tempo/client/Charge.ts:38 and :259; E1). The value is capped only by the challenge expiry. It is used for every pull transaction (prepareTransactionRequest with nonceKey 'expiring', sponsored or not) and every local-account push transaction (sendTransactionSync).
Full finding
Mechanism: the PR raises the Tempo charge client's default expiring-nonce validBefore from now+25 to now+60 (src/tempo/client/Charge.ts:38 and :259; E1). The value is capped only by the challenge expiry. It is used for every pull transaction (prepareTransactionRequest with nonceKey 'expiring', sponsored or not) and every local-account push transaction (sendTransactionSync). Server challenges expire in 5 minutes by default (src/server/Mppx.ts:1153-1156; E7), so default flows now sign validBefore = now+60.
Why this looks like a defect: the pinned viem Tempo chain config, which mppx relies on to prepare these transactions, defines const maxExpirySecs = 25 and uses it as the default validBefore offset for expiring nonces (viem tempo/chainConfig.ts:31, :235-241; E3, E4). The Tempo nonce precompile declares InvalidExpiringNonceExpiry (viem tempo/Abis.ts:585; E5), so the protocol bounds this field. mppx's own server-built expiring-nonce session close transactions also stay at 25s (ChannelStore.ts:656; E6). TIP-1009 expiring nonces are understood to require validBefore <= block.timestamp + 30s. The old 25s default was under that cap; 60s is double it.
Nothing in mppx catches the overshoot before the node. Unsponsored pull transactions are broadcast as the client signed them (server Charge.ts:752-759; E20). The sponsor policy allows validBefore up to 15 minutes by default (fee-payer.ts:692-706; E19).
Impact if the cap holds: every Tempo charge whose challenge expires more than ~30s out, which includes the default, is rejected at the txpool or precompile. That covers sponsored pull, unsponsored pull and local-account push, so payments fail across the board. Only challenges with expiry at or below the cap still work. The new unit tests mock viem/actions, so they cannot catch this.
Missing evidence: the Tempo node / nonce-precompile source defining the maximum expiry at the deployed hardfork is not among the pinned sources (CI uses ghcr.io/tempoxyz/tempo:latest). No localnet run of this head was available.
Remediation: keep the default window below the protocol maximum minus a margin (for example 25s, as viem does). Absorb clock skew against the 15s sponsor floor on the server side instead of raising validBefore. Alternatively define the protocol cap as a constant and min() the window against it. Add a localnet integration test that broadcasts a default-window charge transaction.
Valid? React with 👍 / 👎.
| if (methodDetails?.feePayer === true && challenge.expires) { | ||
| const remainingMs = new Date(challenge.expires).getTime() - Date.now() | ||
| if (!(remainingMs >= defaults.minRemainingValiditySeconds * 1000)) | ||
| throw new Error( | ||
| `Sponsored challenge expires in less than ${defaults.minRemainingValiditySeconds} seconds; the fee payer would reject the transaction. Request a new challenge.`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Informational (exposure unconfirmed) · unverified: Client sponsored-expiry guard uses the client clock without skew tolerance and fails…
Mechanism: the new guard in charge.createCredential (src/tempo/client/Charge.ts:147-153) refuses to sign a sponsored challenge when new Date(challenge.expires) - Date.now() < 15s, measured on the client's clock. The check it is meant to predict ('the fee payer would reject the transaction') runs on the server: fee-payer.ts:589-607 accepts while validBefore >= ceil(serverNow)+15. For challenges with under ~59s left, the client sets validBefore =…
Full finding
Client sponsored-expiry guard uses the client clock without skew tolerance and fails terminally
Mechanism: the new guard in charge.createCredential (src/tempo/client/Charge.ts:147-153) refuses to sign a sponsored challenge when new Date(challenge.expires) - Date.now() < 15s, measured on the client's clock. The check it is meant to predict ('the fee payer would reject the transaction') runs on the server: fee-payer.ts:589-607 accepts while validBefore >= ceil(serverNow)+15. For challenges with under ~59s left, the client sets validBefore = floor(expires)+1 (Charge.ts:258-265). The guard therefore judges a server-clock condition with the client clock, has zero skew allowance, and ignores the extra second in validBefore. It also throws a plain Error from createCredential instead of being reflected in canHandleChallenge (Charge.ts:86-95). The client Fetch loop picks the first candidate (Fetch.ts:296-327) and rethrows any error from credential creation without fetching a new challenge or trying another offer (Fetch.ts:534-553). The HTML page just shows the error (html/main.ts:104-111). Before this change the client signed; if the fee payer rejected, the server turned the FeePayerValidationError (a plain Error, fee-payer.ts:826-834) into InternalPaymentError and a 402 with a fresh challenge (Mppx.ts:1586-1605), which the 3-try retry loop paid. Concrete trigger: a server route with a sponsored tempo.charge and expires: Expires.seconds(20), which the new issuance floor explicitly allows (server Charge.ts:559-569; the test at Charge.expires.test.ts:290-300 even issues 15s challenges), and a client whose clock is 6s ahead, with 0.2s one-way latency. The client computes 20 - 0.2 - 6 = 13.8s left and throws 'Sponsored challenge expires in less than 15 seconds'. The client-clock pre-check (Expires.ts:9-24) passes. The fee payer would have accepted: validBefore = issue+21 >= ceil(issue+~0.5)+15. Before the change the same client got validBefore = min(clientNow+25, issue+21) = issue+21 and paid. Every fresh challenge from the route has the same 20s lifetime, so the failure happens on every request, not just once. In general, a client whose clock leads by d refuses every sponsored challenge with lifetime L <= d + 15 + latency, which is a band the server issues and its fee payer accepts. With no skew, a 15s challenge (permitted by the floor) is refused after any nonzero latency. The same terminal throw also replaces the old self-healing re-challenge when a user delays in an onChallenge/challenge.received confirmation step. Impact: availability only. Sponsored pull payments fail deterministically for clock-ahead clients on routes with short sponsored expiries, and the app must handle a new hard error; no funds move because the guard runs before signing. Preconditions: server issues sponsored challenges with a short lifetime (roughly 15-45s) and the client clock leads the server by more than about L-15s, or the delay before credential creation is long. Missing evidence: how common short sponsored expiries and client clock lead are in deployments, and runtime confirmation against a live fee payer. Remediation: make the guard match the fee payer's check, i.e. test the computed validBefore (floor(expires)+1) with an explicit skew allowance, or drop the client guard and rely on the server's rejection and re-challenge. Alternatively expose the condition through canHandleChallenge or as a retryable error that the Fetch loop answers by fetching a fresh challenge, and/or add a skew/latency margin above 15s to the server issuance floor.
Valid? React with 👍 / 👎.
…isories Bump next to 16.4.1, zod to ^4.6.6, vite-plus to 1.3.0 and @vitest/* to 5.0.5. Advance the body-parser, ip-address, undici and fast-uri overrides to their patched versions, and override zod <4.6.6 so the zod 3 copies pulled in by @x402/* resolve to a patched 4.x.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
No description provided.