feat(rust): add support for x402:upto#174
Conversation
Greptile SummaryThis PR adds the
Confidence Score: 5/5Safe to merge. All previously identified security and correctness issues have been resolved; the remaining findings are documentation and defensive-coding suggestions only. All blocking issues from the previous review cycle are fully addressed with tests. The two remaining notes are a misleading doc comment about No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "fix(x402): reject unsupported upto split..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| settlementHeaders: { [PAYMENT_RESPONSE_HEADER]: encodePaymentResponseHeader(settlement) }, | ||
| transaction: settlement.transaction, | ||
| }; | ||
| } | ||
|
|
||
| #requirements(maxPrice: Price): PaymentRequirements { | ||
| const coin = maxPrice.primaryCoin() ?? this.#stablecoins[0] ?? 'USDC'; | ||
| const mint = resolveStablecoinMint(coin, this.#network); |
There was a problem hiding this comment.
extra.facilitatorAddress diverges from spec and Rust wire format
The spec (scheme_upto_svm.md §4.1) defines the required field as facilitator, and the Rust UptoExtra struct serializes it as "facilitator". This adapter emits facilitatorAddress instead. When a TypeScript server sends an upto 402 challenge to a Rust client, the Rust client tries to deserialize UptoExtra.facilitator (no #[serde(default)]), finds no matching key, and fails deserialization — breaking cross-SDK interoperability entirely for the upto scheme. The test confirms the mismatch: it explicitly casts to { facilitatorAddress?: string } rather than checking facilitator.
There was a problem hiding this comment.
| payload.valid_after | ||
| ))); | ||
| } | ||
| if now > payload.expires_at { | ||
| return Err(Error::Other(format!( | ||
| "authorization expired (expiresAt {} < now {now})", | ||
| payload.expires_at | ||
| ))); |
There was a problem hiding this comment.
requirements here is always built server-side by upto_requirements(), which sets extra.facilitator = self.operator(). So requirements.extra.facilitator != operator is equivalent to self.operator() != self.operator() and can never be true. The check gives a false sense of security and will never fire in production or tests. The meaningful binding is the next check (payload.authorized_signer != operator), which verifies the client actually authorized the correct operator.
There was a problem hiding this comment.
Fixed in c48e7fa: removed the redundant requirements.extra.facilitator != operator check. The payload.authorized_signer == operator check is the real binding and remains.
| reference: open.channel_id.to_string(), | ||
| }); | ||
| req.extensions_mut().insert(charge); | ||
| let mut resp = next.run(req).await; |
There was a problem hiding this comment.
Both the settlement read and Charge::charge() use lock().ok() / if let Ok(...), silently treating a poisoned mutex as a missing charge and defaulting to 0. If a handler panics after calling charge(), the middleware will settle for zero base units — a full refund — even though the resource was (partially) consumed. Using unwrap_or_else(|e| e.into_inner()) (the standard Rust "recover from poisoning" idiom) would preserve the charge. Alternatively, tokio::sync::Mutex avoids poisoning entirely.
There was a problem hiding this comment.
Fixed in c48e7fa: both the post-handler read and Charge::charge() now use unwrap_or_else(|e| e.into_inner()) to recover the charge from a poisoned lock instead of silently settling zero.
| .map_err(|e| Error::Other(format!("settlement serialization failed: {e}")))?; | ||
| Ok(( | ||
| PAYMENT_RESPONSE_HEADER.to_string(), | ||
| base64::Engine::encode(&base64::engine::general_purpose::STANDARD, json.as_bytes()), |
There was a problem hiding this comment.
verify_open re-fetches a fresh blockhash for requirements but the client used a different blockhash
Inside verify_open, upto_requirements() is called, which does a best-effort get_latest_blockhash() and embeds the result in the requirements. The resulting requirements object is then passed to verify_upto_payload. Calling upto_requirements a second time here can produce a requirements object with a different blockhash than the one the client acted on. This is harmless today because verify_upto_payload doesn't compare blockhashes, but it's a subtle inconsistency that could confuse future audit work.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in c48e7fa: upto_requirements is now pure (no RPC). The recent blockhash is fetched only in upto() when building the challenge, so verify_open no longer fetches (or diverges on) a blockhash.
The Rust CI 'format check' step (cargo fmt --check) failed on the upto scheme + core/subscriptions changes. No logic changes — formatting only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| /// Co-sign the fee-payer (operator) slot of a partially-signed transaction. | ||
| async fn cosign_fee_payer(&self, tx: &mut VersionedTransaction) -> Result<(), Error> { | ||
| let account_keys = tx.message.static_account_keys(); | ||
| let idx = account_keys | ||
| .iter() | ||
| .position(|k| k == &self.operator) | ||
| .ok_or_else(|| Error::Other("operator (fee payer) not in open transaction".into()))?; | ||
| if idx >= tx.signatures.len() { | ||
| return Err(Error::Other( | ||
| "operator is not a required signer in the open transaction".into(), | ||
| )); | ||
| } | ||
| let msg_data = tx.message.serialize(); | ||
| let sig_bytes: [u8; 64] = self | ||
| .config | ||
| .operator_signer | ||
| .sign_message(&msg_data) | ||
| .await | ||
| .map_err(|e| Error::Other(format!("fee payer signing failed: {e}")))? | ||
| .into(); | ||
| tx.signatures[idx] = Signature::from(sig_bytes); | ||
| Ok(()) |
There was a problem hiding this comment.
cosign_fee_payer signs whatever message the client sends as long as the operator's pubkey is somewhere in static_account_keys. A malicious client can include SystemProgram::transfer { from: operator, to: attacker, lamports: all } (or any other operator-authorized instruction) alongside — or instead of — the channel-open instruction. The operator key's public key is already advertised in every 402 challenge (extra.facilitator), so the attacker has all the information needed. After the server signs and broadcasts, fetch_channel fails (no valid channel), the server returns a 402, but the operator's SOL has already been transferred.
Before signing, the server must verify the transaction contains only the expected program's open instruction: check that every instruction's program_id equals self.program_id() (the payment-channels program), and that the instruction accounts match the expected payee, mint, operator, and payer derived from the payload. Alternatively, the server should build the open transaction itself rather than accepting an unverified one from the client.
There was a problem hiding this comment.
Fixed in b6b58ee: added validate_open_instruction, called before co-signing. It requires exactly one instruction, on the payment-channels program, with the open discriminator and accounts bound to the expected payer/payee/mint/operator/channel — rejecting any other operator-authorized instruction (e.g. a SystemProgram transfer). Added unit tests including a SOL-drain rejection.
| let recent_blockhash = self | ||
| .rpc | ||
| .get_latest_blockhash() | ||
| .ok() | ||
| .map(|hash| hash.to_string()); |
There was a problem hiding this comment.
When get_latest_blockhash() returns an error, recent_blockhash is None and the 402 challenge is returned normally — HTTP 402 is issued with no indication of the missing field. Any Rust client using build_upto_payload (in client/upto/payment.rs) hard-requires extra.recentBlockhash and will immediately fail with "requirement missing extra.recentBlockhash". The client sees a payment failure, not a transient RPC error, and has no way to retry with a self-fetched blockhash.
If the blockhash is missing the server should either return an error (so the caller can surface a retryable failure) or let the client supply its own blockhash by documenting and handling the absent field gracefully in the client. Silently advertising a challenge that the in-SDK client cannot act on breaks the Rust-to-Rust flow whenever the operator's RPC endpoint is momentarily unreachable.
There was a problem hiding this comment.
Fixed in b6b58ee: upto() now returns a retryable error when the blockhash RPC call fails, instead of issuing a challenge with no blockhash. The gate surfaces this as a 503 rather than a header-less 402.
- TS (P1): emit `extra.facilitator` not `facilitatorAddress` in the upto adapter so it matches the spec (§4.1) and the Rust `UptoExtra` wire field — fixes cross-SDK deserialization (Rust client would reject the challenge). - rust verify.rs (P2): drop the redundant `requirements.extra.facilitator != operator` check (always built server-side; can never fire). The authorized_signer == operator check is the real binding. - rust gate.rs (P2): recover from mutex poisoning in `Charge` so a handler that panics after `charge()` still settles the consumed amount instead of a silent zero refund. - rust server/upto.rs (P2): make `upto_requirements` pure (no RPC); fetch the recent blockhash only in `upto()` when building the challenge, so `verify_open` no longer fetches a divergent blockhash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| #requirements(maxPrice: Price): PaymentRequirements { | ||
| const coin = maxPrice.primaryCoin() ?? this.#stablecoins[0] ?? 'USDC'; | ||
| const mint = resolveStablecoinMint(coin, this.#network); | ||
| if (!mint) throw new ConfigurationError(`No ${coin} mint known for ${this.#network}.`); | ||
| return { | ||
| amount: maxPrice.baseUnits().toString(), | ||
| asset: mint, | ||
| extra: { facilitator: this.#operator, feePayer: this.#operator }, | ||
| maxTimeoutSeconds: MAX_TIMEOUT_SECONDS, | ||
| network: this.#network, | ||
| payTo: this.#recipient, | ||
| scheme: 'upto', | ||
| }; |
There was a problem hiding this comment.
profiles in extra breaks Rust client interoperability
The Rust client's build_upto_payload requires requirements.extra.profiles to contain "payment-channel" before it builds the open transaction. UptoExtra has #[serde(default)] on profiles, so when the field is absent it deserializes to an empty Vec. The Rust check requirements.extra.profiles.iter().any(|p| p == PROFILE_PAYMENT_CHANNEL) then fails unconditionally, returning "requirement does not advertise the payment-channel profile". Any Rust client receiving a challenge from this TypeScript server cannot proceed with the upto flow at all. The spec (§4.1) also marks profiles as required (✓). The fix is to add profiles: ['payment-channel'] to the returned extra.
There was a problem hiding this comment.
The TS adapter was untracked from this Rust-only PR (9d17022), so it no longer ships here. profiles will be included in extra when the TS integration lands in its own PR.
| let channel_id = Pubkey::from_str(&payload.channel_id) | ||
| .map_err(|e| Error::Other(format!("invalid channelId: {e}")))?; | ||
| let payer = Pubkey::from_str(&payload.from) | ||
| .map_err(|e| Error::Other(format!("invalid payer: {e}")))?; | ||
| let max = payload.max_amount()?; | ||
|
|
||
| // Broadcast the client-signed open (pull). Push (already broadcast) is | ||
| // not yet supported; require the transaction. | ||
| let open_tx_b64 = payload.open_transaction.as_deref().ok_or_else(|| { | ||
| Error::Other("payment-channel profile requires openTransaction (pull)".to_string()) | ||
| })?; | ||
| let mut tx = decode_transaction(open_tx_b64)?; | ||
| self.cosign_fee_payer(&mut tx).await?; | ||
| self.rpc | ||
| .send_and_confirm_transaction(&tx) | ||
| .map_err(|e| Error::Rpc(format!("open broadcast failed: {e}")))?; | ||
|
|
||
| // Read the confirmed channel state and bind it. | ||
| let channel = self.fetch_channel(&channel_id)?; | ||
| if channel.status != CHANNEL_STATUS_OPEN { | ||
| return Err(Error::Other( | ||
| "channel is not open after broadcast".to_string(), | ||
| )); | ||
| } | ||
| if pc::from_address(&channel.mint) != expected_mint { | ||
| return Err(Error::MintMismatch { | ||
| expected: pc::pubkey_string(&expected_mint), | ||
| actual: pc::pubkey_string(&pc::from_address(&channel.mint)), | ||
| }); | ||
| } | ||
| if pc::from_address(&channel.payee) != expected_payee { | ||
| return Err(Error::RecipientMismatch { | ||
| expected: pc::pubkey_string(&expected_payee), | ||
| actual: pc::pubkey_string(&pc::from_address(&channel.payee)), | ||
| }); | ||
| } | ||
| if pc::from_address(&channel.authorized_signer) != self.operator { | ||
| return Err(Error::Other( | ||
| "channel authorized_signer is not the operator".to_string(), | ||
| )); | ||
| } | ||
| if channel.deposit < max { | ||
| return Err(Error::Other(format!( | ||
| "on-chain deposit {} below authorized maximum {max}", | ||
| channel.deposit | ||
| ))); | ||
| } | ||
|
|
||
| Ok(VerifiedUptoOpen { | ||
| channel_id, | ||
| payer, | ||
| mint: expected_mint, | ||
| token_program: self.token_program()?, | ||
| program_id, | ||
| deposit: channel.deposit, | ||
| max_amount: max, | ||
| expires_at: payload.expires_at, | ||
| network: requirements.network, | ||
| }) |
There was a problem hiding this comment.
channel.payer binding check causes settlement to fail on-chain
verify_open fetches the confirmed channel state and checks mint, payee, authorized_signer, and deposit — but never asserts pc::from_address(&channel.payer) == payer (where payer is parsed from payload.from). VerifiedUptoOpen.payer is set from payload.from, not from the on-chain channel.payer. When settle_actual calls build_distribute_instruction(&open.channel_id, &open.payer, ...), the instruction accounts include the wrong payer pubkey. The payment-channels program enforces that the payer account matches channel.payer for the refund transfer, so the settlement transaction will fail on-chain even though the handler has already run and served the resource. The channel stays open until the grace period expires and the real payer reclaims it.
There was a problem hiding this comment.
Fixed in b6b58ee: verify_open now asserts on-chain channel.payer == payload.from after fetching the channel, so settlement distribute cannot fail on-chain due to a payer mismatch.
The pay-kit TS x402/upto adapters belong to a separate, in-progress TS integration (they import the unpublished @x402 packages). Remove them from this Rust-only PR; the files stay on disk for that follow-up work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| pub async fn verify_open( | ||
| &self, | ||
| header: &str, | ||
| max_amount: &str, | ||
| ) -> Result<VerifiedUptoOpen, Error> { | ||
| let envelope = self.parse_payment_signature(header)?; | ||
| let requirements = self.upto_requirements(max_amount)?; | ||
| let payload = &envelope.payload; | ||
|
|
||
| verify_upto_payload(payload, &requirements, &self.operator(), now_unix())?; | ||
|
|
||
| let program_id = self.program_id()?; | ||
| let expected_mint = self.mint()?; | ||
| let expected_payee = Pubkey::from_str(&self.config.recipient) | ||
| .map_err(|e| Error::Other(format!("invalid recipient: {e}")))?; | ||
| let channel_id = Pubkey::from_str(&payload.channel_id) | ||
| .map_err(|e| Error::Other(format!("invalid channelId: {e}")))?; | ||
| let payer = Pubkey::from_str(&payload.from) | ||
| .map_err(|e| Error::Other(format!("invalid payer: {e}")))?; | ||
| let max = payload.max_amount()?; | ||
|
|
||
| // Broadcast the client-signed open (pull). Push (already broadcast) is | ||
| // not yet supported; require the transaction. | ||
| let open_tx_b64 = payload.open_transaction.as_deref().ok_or_else(|| { | ||
| Error::Other("payment-channel profile requires openTransaction (pull)".to_string()) | ||
| })?; | ||
| let mut tx = decode_transaction(open_tx_b64)?; | ||
| self.cosign_fee_payer(&mut tx).await?; | ||
| self.rpc | ||
| .send_and_confirm_transaction(&tx) | ||
| .map_err(|e| Error::Rpc(format!("open broadcast failed: {e}")))?; | ||
|
|
||
| // Read the confirmed channel state and bind it. | ||
| let channel = self.fetch_channel(&channel_id)?; | ||
| if channel.status != CHANNEL_STATUS_OPEN { | ||
| return Err(Error::Other( | ||
| "channel is not open after broadcast".to_string(), | ||
| )); | ||
| } | ||
| if pc::from_address(&channel.mint) != expected_mint { | ||
| return Err(Error::MintMismatch { | ||
| expected: pc::pubkey_string(&expected_mint), | ||
| actual: pc::pubkey_string(&pc::from_address(&channel.mint)), | ||
| }); | ||
| } | ||
| if pc::from_address(&channel.payee) != expected_payee { | ||
| return Err(Error::RecipientMismatch { | ||
| expected: pc::pubkey_string(&expected_payee), | ||
| actual: pc::pubkey_string(&pc::from_address(&channel.payee)), | ||
| }); | ||
| } | ||
| if pc::from_address(&channel.authorized_signer) != self.operator { | ||
| return Err(Error::Other( | ||
| "channel authorized_signer is not the operator".to_string(), | ||
| )); | ||
| } | ||
| if channel.deposit < max { | ||
| return Err(Error::Other(format!( | ||
| "on-chain deposit {} below authorized maximum {max}", | ||
| channel.deposit | ||
| ))); | ||
| } | ||
|
|
||
| Ok(VerifiedUptoOpen { | ||
| channel_id, | ||
| payer, | ||
| mint: expected_mint, | ||
| token_program: self.token_program()?, | ||
| program_id, | ||
| deposit: channel.deposit, | ||
| max_amount: max, | ||
| expires_at: payload.expires_at, | ||
| network: requirements.network, | ||
| }) |
There was a problem hiding this comment.
verify_open has no memory of channel IDs that are currently being processed. If two concurrent requests arrive with the same PAYMENT-SIGNATURE header before the first request's settle_actual finalizes the channel, both calls to send_and_confirm_transaction may succeed (Solana RPC is typically idempotent for already-confirmed transactions), both calls to fetch_channel see CHANNEL_STATUS_OPEN, and both handlers run and serve the resource for a single payment.
An attacker can deliberately replay the same header in parallel to get two (or more) LLM-token generations for one channel deposit. The window of exploitation equals the handler execution time — exactly the slow, expensive case upto is designed for. An Arc<Mutex<HashSet<Pubkey>>> (or DashMap) tracking in-flight channel IDs in X402Upto, acquired before broadcasting and released after settlement, would close this window.
There was a problem hiding this comment.
Fixed in b6b58ee: added an in-flight channel set on X402Upto plus an RAII guard held by VerifiedUptoOpen. A concurrent replay of the same channel is rejected, and the slot is released on drop (after settlement, on early-return errors, or on a handler panic).
- server/upto.rs (P0, security): validate the client open transaction before the operator co-signs as fee payer — exactly one instruction, on the payment-channels program, with the open discriminator and accounts bound to the expected payer/payee/mint/operator/channel. Closes the SOL-drain vector where a malicious client could get the operator to sign an arbitrary (e.g. SystemProgram transfer) instruction. Added unit tests. - server/upto.rs (P1, security): in-flight channel dedup via an RAII guard, so the same authorization replayed concurrently can't be served twice for one deposit. - server/upto.rs (P1): bind on-chain channel.payer == payload.from in verify_open, so settlement's distribute can't fail on-chain after the resource was served. - server/upto.rs (P1): upto() now fails (retryable) if the blockhash RPC errors instead of issuing a 402 with no blockhash the in-SDK client can't act on. - kit/gate.rs: surface that as a retryable 503 (not a header-less 402); test updated accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ardened x402 Mirrors the cross-implementation review on the Rust upto PR (#174) on the TS side: - interop: the x402-upto adapter now emits the spec `extra.facilitator` (was the non-spec `facilitatorAddress`) and `profiles: ['payment-channel']`, so a Rust/ other-SDK upto client can act on a pay-kit-issued 402. Offer-building reads `facilitator` too. - concurrent double-serve: `requireUsage` now dedups in-flight upto channel IDs (acquired after verify, released when settle completes), so a replayed PAYMENT-SIGNATURE can't serve the metered resource twice for one deposit. - re-pin the external/x402 submodule + vendored tarballs to the hardened fork commit (channel-open co-sign allowlist + ALT reject + channel.payer binding + the same facilitator/profiles rename).
| .map_err(|e| Error::Other(format!("invalid recipient: {e}")))?, | ||
| &pc::treasury_owner(), | ||
| &open.mint, | ||
| &[], |
There was a problem hiding this comment.
Is this intentionally limited to empty-recipient channels? The draft SVM upto spec says verification should bind channel.distribution_hash to payTo/splits, but verify_open does not check it and settle_actual always distributes with an empty recipient list. If splits are out of scope, should we enforce the empty distribution hash before serving?
There was a problem hiding this comment.
Addressed by enforcing the empty-recipient distribution hash before serving, since this first slice does not advertise split recipients.
2 participants
No description provided.