diff --git a/harness/src/intents/charge.ts b/harness/src/intents/charge.ts index e080be16..02bea9e5 100644 --- a/harness/src/intents/charge.ts +++ b/harness/src/intents/charge.ts @@ -324,6 +324,12 @@ export const chargeScenarios: readonly HarnessScenario[] = [ // can drive a 9-split request through the env-only path, so this // is typescript-client only. Splits are intentionally tiny so the // sum stays well under amount. + // + // Rust audit #21 promoted this from a runtime-reject to a + // refuse-to-boot at server startup. The harness has no notion of + // "expected startup failure" yet, so rust is excluded from this + // scenario via serverIds — re-include it when the harness gains a + // way to assert on adapter-exit-before-readiness. id: "charge-splits-too-many", intent: "charge", network: "localnet", @@ -333,6 +339,7 @@ export const chargeScenarios: readonly HarnessScenario[] = [ resourcePath: "/protected/splits-too-many", settlementHeader: "x-fixture-settlement", clientIds: ["typescript"], + serverIds: ["typescript", "php", "ruby", "go", "python", "lua"], splits: [ { recipientKey: "platform", amount: "1" }, { recipientKey: "platform", amount: "1" }, diff --git a/harness/test/e2e.test.ts b/harness/test/e2e.test.ts index 3fc76864..b5a1dc7a 100644 --- a/harness/test/e2e.test.ts +++ b/harness/test/e2e.test.ts @@ -281,7 +281,10 @@ beforeAll(async () => { MPP_HARNESS_NETWORK: baseScenario.network, MPP_HARNESS_MINT: baseScenario.asset, MPP_HARNESS_PRICE: baseScenario.price, - MPP_HARNESS_SECRET_KEY: "mpp-harness-secret-key", + // Rust audit #24 requires ≥32-byte HMAC secrets (NIST SP 800-107 for + // HMAC-SHA256). Padded with `-pad` to clear the threshold without + // changing the test's intent. + MPP_HARNESS_SECRET_KEY: "mpp-harness-secret-key-with-32b-pad", MPP_HARNESS_PAY_TO: payTo.publicKey, MPP_HARNESS_CLIENT_SECRET_KEY: JSON.stringify(Array.from(client.secretKey)), MPP_HARNESS_FEE_PAYER_SECRET_KEY: JSON.stringify( @@ -593,7 +596,8 @@ describe("mpp harness", () => { const envA = environmentForScenario(harnessEnv, scenario); const envB = { ...environmentForScenario(harnessEnv, scenario), - MPP_HARNESS_SECRET_KEY: "mpp-harness-secret-key-server-b", + // Rust audit #24: 32-byte minimum. + MPP_HARNESS_SECRET_KEY: "mpp-harness-secret-key-server-b-pad", }; const a = await startServer(serverA, envA); runningServers.push(a); diff --git a/rust/AUDIT-ASSESSMENT.md b/rust/AUDIT-ASSESSMENT.md new file mode 100644 index 00000000..48c27909 --- /dev/null +++ b/rust/AUDIT-ASSESSMENT.md @@ -0,0 +1,763 @@ +# Audit Assessment — Solana MPP (Rust) + +Source: `Solana - MPP-findings-export-2026-05-26.json` (45 findings) +Code reviewed at branch: `fix/rust-audit` +Assessor: Ludo + Claude + +Legend for **Decision**: +- ✅ accept — fix as recommended +- 🟡 partial — fix differs from audit recommendation +- ❌ reject — won't fix (with rationale) +- ⏳ pending — not yet reviewed + +--- + +## Medium severity + +### #38 — Primary recipient of challenge can be in list of splits +**ID:** `b4dfcf0b` · **File:** `crates/mpp/src/{server,client}/charge.rs` + +**Audit claim:** Spec §9.5 forbids ATA-creation for the top-level recipient. Server/client never reject a split whose `recipient` equals the top-level `recipient`, so a fee-sponsored challenge could pay to (re)create the primary recipient's ATA — exploitable as a slow drain by closing/recreating. + +**Current code:** Still as described. +- `server/charge.rs:307` `validate_charge_options` — no check. +- `server/charge.rs:1185` `expected_ata_creation_policy` — primary recipient not excluded. +- `client/charge.rs:113` — client signs whatever it gets. + +**Decision:** 🟡 **partial — reject the strict ban, add a misconfig guard. Fixed.** + +**Rationale:** Having the primary recipient appear in `splits` is a legitimate use case we want to support (e.g., the merchant takes part of the funds as a split alongside other splits). Forbidding the recipient in splits would over-constrain the protocol. The actual drain shape is the *combination* primary-in-splits + `ataCreationRequired: true` in fee-sponsored mode, so we narrow the check to that combination. + +**Threat model framing:** this is a server misconfig (the only party harmed is the server's own fee-payer wallet — a malicious recipient can only trigger the loop if the server already authored a challenge with this shape). So the gate belongs server-side, before HMAC; no client guard, no verify-side defense-in-depth. + +**Action taken:** +- Added an early loop in `validate_charge_options` (`server/charge.rs:396`) that rejects with `Error::InvalidConfig` when any `split.recipient == self.recipient` AND `split.ata_creation_required == Some(true)`. Runs before the existing SPL-gating checks so the message points at the actual misconfig rather than a downstream consequence. +- The lower-level `charge_challenge_with_options` path (and its `validate_charge_request`) is intentionally *not* tightened — that path is the "trusted construction escape hatch" documented for callers who need to issue challenges for a different route. Audit #19's HMAC validation already pins network/currency/recipient/token program on that path. +- New tests: + - `charge_with_options_rejects_primary_recipient_with_ata_creation_required` — negative case. + - `charge_with_options_allows_primary_recipient_in_splits_without_ata_creation` — positive case (the legitimate use the strict ban would have over-blocked). + +--- + +### #32 — Missing checks in `find_sol_transfer` +**ID:** `923a9c9b` · **File:** `crates/mpp/src/server/charge.rs:1710` + +**Audit claim:** `find_sol_transfer` matched on `parsed.type == "transfer"` + `info.lamports` + `info.destination` only — no `programId` check and no `source` check. Defense-in-depth gap (parsed format is System-Program-specific in practice) plus a real risk that in fee-sponsored mode the server (fee payer) could end up bankrolling the value transfer. + +**Decision:** ✅ **accepted — fixed.** + +**Action taken:** +- Added `programId == System Program` check via the existing `parsed_program_id()` helper. +- Read `info.source` and reject when `source == fee_payer` (matches the policy of the lower-level `verify_sol_transfer_instructions:1485` — separation of duties: fee payer covers gas, not value). +- Threaded `fee_payer: Option<&str>` through `verify_sol_transfers` and the parsed-credential caller (passes `expected_ata_payer`, which is `Some(fee_payer_key)` only in fee-sponsored mode). +- Updated all 6 existing parsed-instruction tests to carry `program: "system"` + `info.source`. Added 2 new tests: + - `find_sol_transfer_rejects_non_system_program` + - `find_sol_transfer_rejects_source_equals_fee_payer` + +**Note on "source = expected payer":** the audit suggested checking source against the expected payer. The lower-level path's policy is asymmetric (forbid `source == fee_payer`, allow anything else). We mirrored that policy rather than tightening, so the two verification paths behave identically. + +--- + +### #29 — Push-mode SPL verification ignores the source ATA +**ID:** `45c6d39f` · **File:** `crates/mpp/src/server/charge.rs:1801` + +**Audit claim:** `find_spl_transfer` matched `transferChecked` instructions on `info.mint`, `info.tokenAmount.amount`, and derived destination ATA only. It never read `info.source` or `info.authority`, so in fee-sponsored mode the server's fee-payer signature could be re-used to fund the value transfer via its own ATA — same drain shape as #32, but on the SPL side. + +**Decision:** ✅ **accepted — fixed.** + +**Action taken:** mirror the lower-level path (`verify_spl_transfer_instructions:1586`): +- Read `info.authority` and reject when `authority == fee_payer`. +- Read `info.source` and reject when `source == fee_payer's ATA` (PDA derived from `[fee_payer, token_program, mint]` against the ATA program). Required even when the authority is a delegate. +- Threaded `fee_payer: Option<&str>` through `verify_spl_transfers` and the parsed-credential caller. +- Updated the 4 existing SPL parsed tests to take the new arg (`None`). Added 2 new tests: + - `find_spl_transfer_rejects_authority_equals_fee_payer` + - `find_spl_transfer_rejects_source_equals_fee_payer_ata` + +**Note on alternative recommendation:** the audit also suggested deriving the *expected source ATA* (from the signer/payer) and rejecting anything else, gating arbitrary sources behind a flag. We didn't take this route because the model already accepts delegate/multisig flows by design, and forcing source = signer's ATA would break that. The fee-payer exclusion is the narrower, sufficient fix. + +--- + +### #28 — Incorrect default fallback resolving mint to token program +**ID:** `048bfd43` · **Files:** `crates/mpp/src/protocol/solana.rs`, `crates/mpp/src/server/charge.rs` + +**Audit claim, two parts:** +1. `default_token_program_for_currency` only recognized `CASH` as Token-2022; PYUSD (also Token-2022) fell back to legacy Token. +2. Server falls back to the same guess instead of fetching the mint owner on-chain (spec §7.2), so a challenge generated for an arbitrary Token-2022 mint will go out with the wrong `tokenProgram`. + +**Status when reviewed:** +- Part 1: **already fixed in prior work.** `stablecoin_uses_token_2022` now covers PYUSD/USDG (mainnet+devnet) and CASH. +- Part 2: still valid for arbitrary mint addresses outside the known list. + +**Decision:** ✅ **accepted — fixed, resolved at boot rather than per-challenge.** + +**Action taken:** +- Added `is_known_stablecoin_mint()` helper in `protocol/solana.rs` to distinguish the static-table path from arbitrary mints. +- Added `resolve_server_token_program(rpc, currency, network)` in `server/charge.rs`: + - `SOL` → `None`. + - Known stablecoin symbol/mint → answer from the static table. + - Arbitrary mint address → parse as `Pubkey`, fetch the mint account, return its owner. Reject if the owner is not the Token Program or the Token-2022 Program. Reject if the currency parses as neither a known symbol nor a valid pubkey. +- Resolution runs once in `Mpp::new` and the result is cached on `Mpp.token_program: Option<&'static str>`. No per-challenge RPC fan-out; servers fail fast at boot if the mint is unreachable or has an unexpected owner. +- Challenge generation now emits `tokenProgram` straight from `self.token_program` (omits it for SOL). +- The parsed-credential verifier no longer falls back to `default_token_program_for_currency` when `methodDetails.tokenProgram` is missing — it prefers the embedded value, then `self.token_program`, then errors out. +- Updated the docstring on `default_token_program_for_currency` to warn that it is the static-table path only and callers handling arbitrary mints MUST go through the RPC-backed resolver. +- New tests: `new_resolves_token_program_for_sol_currency`, `_for_usdc`, `_for_pyusd_token_2022` (regression for part 1), `new_rejects_unparseable_currency_without_rpc`. RPC-backed arbitrary-mint path is exercised by integration tests in `tests/charge_integration.rs` against the localnet. + +--- + +### #26 — Client signs arbitrary mint-address currencies (Token-2022 hook risk) +**ID:** `5e1a1d39` · **Files:** `crates/mpp/src/client/charge.rs`, `crates/mpp/src/protocol/solana.rs` + +**Audit claim:** Spec §13.3 requires clients, if `currency` is a mint address, to verify it is a known token. Today the client passes any mint through and only checks the owner is `TOKEN_PROGRAM` or `TOKEN_2022_PROGRAM`. An arbitrary Token-2022 mint can ship **transfer hooks** that execute arbitrary code on every transfer; the server's pre-broadcast checks don't simulate inner instructions in pull mode. + +**Decision:** ✅ **accepted — two-tier gate, with opt-in.** + +**Rationale:** A pure allowlist breaks the "arbitrary mints first-class" story we just leaned into for #28 (server-side). But transfer hooks are the actual hostile surface, and they only exist on Token-2022. The vanilla Token Program has no hooks, so arbitrary mints there stay first-class. + +**Action taken:** +- Added `BuildChargeTransactionOptions::allow_unknown_token_2022` and `SelectChargeChallengeOptions::allow_unknown_token_2022` (both `bool`, default `false`). +- In `build_spl_instructions`: after `resolve_token_program`, if the token program is Token-2022 AND the mint is not in `is_known_stablecoin_mint`, refuse to sign unless the caller opted in. +- In `select_charge_challenge`: a new `challenge_is_unknown_token_2022` helper rejects candidates whose currency is an unknown mint when `methodDetails.tokenProgram` is either Token-2022 or missing (we cannot prove it isn't Token-2022). Vanilla `Token Program` hint allows the candidate through. +- Added `build_credential_header_with_options` so callers can opt in without dropping to the lower-level builder. +- New tests: + - `build_spl_refuses_unknown_token_2022_without_opt_in` + - `build_spl_allows_unknown_token_2022_with_opt_in` + - `build_spl_allows_unknown_vanilla_token_mint` + - `build_spl_does_not_gate_known_token_2022_stablecoin` + - `select_charge_challenge_skips_unknown_token_2022_by_default` + - `select_charge_challenge_skips_unknown_mint_with_no_token_program_hint` + - `select_charge_challenge_accepts_unknown_vanilla_token_mint` + - `select_charge_challenge_allows_unknown_token_2022_with_opt_in` + - `select_charge_challenge_does_not_gate_known_token_2022_stablecoin` + +**Note on departing from the audit recommendation:** the audit asked for a plain allowlist with opt-in. We split it on the actual threat axis (Token-2022 vs. Token) so unknown plain-Token mints don't need an opt-in dance. The opt-in still exists for the unsafe case. + +--- + +### #25 — Fee-sponsored pull mode lets clients inflate priority fees +**ID:** `b6791d00` · **Files:** `crates/mpp/src/server/charge.rs:42` (caps), `:1388` (caller), `:1448` (validator) + +**Audit claim:** Client builder emits `compute_unit_price=1, limit=200_000`. Server caps at `MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS=5_000_000`. In fee-sponsored pull mode the server signs *before* broadcast, so an attacker can pick a price up to the cap and the merchant pays the priority fee. Per spec math `priority_fee_lamports = ceil(price × limit / 1_000_000)`, that's up to `1_000_000` lamports (0.001 SOL) per "valid" charge — ≈200× the base fee, run in a loop = drain. + +**Decision:** ✅ **accepted — tight cap in fee-sponsored mode, general cap untouched.** + +**Action taken:** +- Added `MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS_FEE_SPONSORED = 10_000` (worst-case priority fee `ceil(10_000 × 200_000 / 1_000_000) = 2_000 lamports`, ≈20% of the per-signature base fee — enough room for honest clients to bump priority during congestion). +- `validate_compute_budget_instruction` now takes a `fee_sponsored: bool` and applies the tight cap when set. +- `validate_instruction_allowlist` passes `fee_payer.is_some()` — `fee_payer` is `Some` precisely when the server is acting as the fee payer. +- Client-paid mode (fee_payer not configured) keeps the 5_000_000 ceiling; the client is paying its own gas, no merchant risk. +- New tests: + - `compute_unit_price_fee_sponsored_under_tight_cap_passes` + - `compute_unit_price_fee_sponsored_above_tight_cap_rejected` + - `compute_unit_price_client_paid_above_tight_cap_passes` (regression: tight cap MUST NOT apply when the client is paying). + +**Note on alternative (b):** the audit also suggested locking to exact `price=1, limit=200_000` (the client builder's values). We chose the tight-cap shape so non-default-tooling clients can still tune priority during congestion without lockstep changes to the server. + +--- + +### #24 — Weak secret key accepted +**ID:** `b7c1edc5` · **File:** `crates/mpp/src/server/charge.rs` (`Mpp::new` / `detect_secret_key`) + +**Audit claim:** Both the `Config.secret_key` path and the `MPP_SECRET_KEY` env-var path accepted any string — empty, `"key"`, etc. That string is the HMAC-SHA256 key binding challenge IDs, so a weak key lets an attacker forge challenges. + +**Decision:** ✅ **accepted — fixed, strict 32-byte minimum.** + +**Action taken:** +- Added `MIN_SECRET_KEY_BYTES = 32`, matching NIST SP 800-107 guidance for HMAC-SHA256 (key ≥ hash output length). +- New `validate_secret_key()` runs in `Mpp::new` after the value is resolved from either `Config.secret_key` or the env var — both paths share the same gate. +- Updated the `Config.secret_key` docstring to require ≥ 32 bytes of cryptographically-random data and reference `openssl rand -base64 32`. +- Updated test secrets across `src/server/{charge,axum}.rs` and `tests/charge_integration.rs` to ≥ 32-byte strings; `"key"` literals in unit tests now use the existing `TEST_SECRET` constant. +- New tests: `new_rejects_empty_secret_key`, `new_rejects_short_secret_key`, `new_accepts_secret_key_at_minimum_length`, `new_rejects_short_env_secret_key` (regression: the env-var path must apply the same gate). + +**Note on threshold choice:** the audit asked for "a documented minimum size" without a number. 32 bytes is the cryptographically right answer for HMAC-SHA256; a permissive 16-byte minimum would have spared a few test churn but locks in an under-strength default for years. + +--- + +### #20 — Implicit client-funded split ATA creation +**ID:** `8d8dab0e` · **File:** `crates/mpp/src/client/charge.rs:498` + +**Audit claim:** In client-paid mode the client builder auto-created ATAs for every split, ignoring `ataCreationRequired`. A hostile server could attach N dust splits to a challenge and force the client to pay N × ~0.002 SOL of ATA rent. + +**Spec position (re-verified against draft-solana-charge-00):** +- §7.2 — "When `ataCreationRequired` is `true`, the client MUST include an idempotent ATA-create instruction…" +- §9.5 — "Clients MUST NOT include **fee-payer-funded** ATA creation instructions for the top-level `recipient`, unmarked split recipients, or arbitrary owners." + +So the spec mandates creation only when flagged; the §9.5 ban is narrower than the audit suggested (it only restricts fee-payer-funded creation), but the *threat* — silent rent drain on the client — is real regardless of mode. + +**Decision:** ✅ **accepted, client-only fix.** + +**Action taken:** changed the create-ATA gate at `client/charge.rs:498` from +``` +create_ata = fee_payer.is_none() || split.ata_creation_required == Some(true) +``` +to +``` +create_ata = split.ata_creation_required == Some(true) +``` +Same flag, both modes. Updated `build_spl_with_splits` and `build_spl_with_split_memo` to reflect the new behaviour (no auto-create, fewer ixs); added `build_spl_creates_split_ata_only_when_flagged` to lock in the positive case. + +**Why client-only:** Server-side `expected_ata_creation_policy` is permissive in client-paid mode (`allowed_owners = all split owners`), which is consistent with the spec (it only forbids *fee-payer-funded* ATA creation). Tightening the server would break integrators with legitimate auto-create flows on their own clients. The drain attack the audit identified is closed once *our* SDK client refuses to emit unflagged ATA-creates. + +**Honest-flow impact:** servers that need clients to fund split ATAs MUST now set `ataCreationRequired: true` per split. Servers that forget the flag will see the receiving transfer fail on-chain instead of silently charging the client — clearer failure mode. + +--- + +### #19 — Full `ChargeRequest` signed without validation +**ID:** `0fe8cced` · **File:** `crates/mpp/src/server/charge.rs:432` + +**Audit claim:** `charge_challenge_with_options` HMAC-signed a caller-supplied `ChargeRequest` directly. Nothing checked `amount`, `currency`, `recipient`, `network`, `decimals`, `feePayer`, `tokenProgram`, or `splits`, so a buggy or hostile caller could produce a *cryptographically-valid* challenge with malformed or off-route contents. + +**Decision:** ✅ **accepted — validate, both internally and against `self`.** + +**Action taken:** added `Mpp::validate_charge_request` and call it at the top of `charge_challenge_with_options`. The check enforces: +- `amount` parses as `u64` (reuses `ChargeRequest::parse_amount`). +- `currency` matches `self.currency` (case-insensitive). +- `recipient` is `Some(..)` and parses as a `Pubkey`. +- `methodDetails` (if present) deserializes as `MethodDetails` and each pinned field matches `self`: `network`, `decimals`, `tokenProgram` (against the boot-resolved `self.token_program`). +- Each `split` carries a parseable recipient `Pubkey` and a `u64` amount. + +`fee_payer`/`feePayerKey` are left untouched — the high-level path already accepts a per-request fee-payer override (`options.fee_payer || self.fee_payer`), and audit #11 (`#1335d2de`) handles the orthogonal `feePayer=true` with no signer case. + +Callers who legitimately need to issue challenges for a *different* route still have `PaymentChallenge::with_secret_key_full` as a public escape hatch — the trusted construction path the audit's recommendation refers to. + +**New tests:** `charge_challenge_rejects_mismatched_currency`, `_missing_recipient`, `_invalid_recipient`, `_unparseable_amount`, `_mismatched_network_in_method_details`, `_mismatched_token_program`, `_invalid_split_recipient`. + +--- + +### #10 — Client signs untrusted charge challenges +**ID:** `ad99fed8` · **File:** `crates/mpp/src/client/charge.rs` + +**Audit claim:** `build_credential_header` / `build_charge_transaction` decode the challenge and produce a signed transaction with no client-side policy enforcement (max amount, expected recipient/currency/network, expiry, split shape). Safe only when the caller has already validated the challenge; *unsafe* for auto-pay integrations where the server effectively controls what gets signed against the user's wallet. + +**Decision:** ✅ **accepted — narrow opt-in gates, plus always-on expiry.** + +**Rationale:** The protocol's working trust model assumes a human reviews the challenge before signing. Auto-pay agents break that, and that's the case the audit is calling out. We give the auto-pay caller a way to bind what we'll sign, without forcing the UI caller to plumb anything (all gates default to "no constraint"). Scope kept tight: amount cap, network pin, and an always-on expiry refusal. Recipient/currency match and split-shape policies are not in scope for this finding — auto-pay callers already control those values when they call our `select_charge_challenge` helper, so duplicating them in the builder didn't pull its weight. + +**Action taken:** +- Added two opt-in fields to `BuildChargeTransactionOptions`: + - `max_amount_base_units: Option` — reject when `request.amount > cap` (parsed as base units, matches how the server reasons about it). + - `expected_network: Option` — reject when `methodDetails.network` does not match. + - Both checks run at the top of `build_charge_transaction_with_options`, before any signing or instruction building. +- Always-on expiry refusal in `build_credential_header_with_options`: if `challenge.is_expired()` returns `true`, refuse to sign. Reuses the existing fail-closed RFC3339 parser. Challenges with `expires == None` are still accepted (the protocol allows omitting it; we have no client-side anchor to check against). +- Expiry lives on `PaymentChallenge` (not in the decoded `ChargeRequest`), so the gate is in the `build_credential_header` path. Lower-level callers who construct a transaction directly from `MethodDetails` without a challenge skip this check — there's no challenge to check. + +**Note on what we didn't add:** +- No `expected_recipient` / `expected_currency` options. The auto-pay caller selects the challenge via `select_charge_challenge` (or by hand); the builder doesn't need to re-check fields the caller just picked. +- No split policy options. Splits are bounded by the existing `Error::TooManySplits` gate (`splits.len() > 8`), and the spec already constrains amounts to be a subset of the total. Adding per-recipient allowlists felt like over-policy for what the auto-pay threat model actually needs. +- No client-side check that the recipient is *a* valid pubkey beyond what `build_charge_transaction_with_options` already does (it parses on its own). + +**New tests:** +- `build_charge_transaction_rejects_amount_above_max` +- `build_charge_transaction_accepts_amount_at_max` (equal-to-cap is allowed) +- `build_charge_transaction_rejects_unexpected_network` +- `build_charge_transaction_accepts_matching_network` +- `build_credential_header_rejects_expired_challenge` +- `build_credential_header_accepts_future_expiry` + +--- + +### #3 — Replay state recorded after broadcast +**ID:** `91c89aa6` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim:** `verify_pull` waits for confirmation before recording the signature in the replay store. On confirmation timeout, the verifier bails with a network error and the consumed signature is never inserted, leaving a confirmed payment without a successful receipt and without replay state. + +**Status when reviewed:** the *ordering* half of the claim is **already mitigated** (PR #85 / audit gap G05). `server/charge.rs:728-755` reserves the signature *between* broadcast and confirmation polling: +```rust +let signature = self.broadcast_pull(...).await?; +self.consume_signature(&signature).await?; +self.await_pull_confirmation(&signature)?; +``` +So the replay-state side of the bug is closed. + +**What was still live:** `await_pull_confirmation` exits with a network_error after 30 polls × 200 ms = 6 s. If the polling RPC is lagging or load-balanced and hasn't observed the signature yet, but the tx is actually on-chain, the verifier reports a timeout. The signature is reserved, so retrying the same credential fails with "already consumed" — user pays, never gets the resource, and can't recover. + +**Decision:** ✅ **accepted — narrow fix only.** + +**Rationale:** The pre-broadcast / two-phase / challenge-id-keyed reservation refactor the audit suggested as a Cadillac fix would touch the Store trait and the verify state machine for a marginal extra mitigation. The user-visible bug is the false-negative timeout, and a one-shot definitive status check after the poll loop closes it without churn. + +**Action taken:** +- After the 30-poll loop in `await_pull_confirmation`, call `rpc.get_signature_status(&signature)` once. Interpret the four possible outcomes: + - `Ok(Some(Ok(())))` — tx landed cleanly: return `Ok(())` and log `confirmed_via_status_recovery`. + - `Ok(Some(Err(e)))` — tx landed but failed on-chain: return `VerificationError::transaction_failed("Transaction landed on-chain but failed: …")`. Distinct from a polling timeout — we now know the payment didn't go through. + - `Ok(None)` — definitively not on-chain: keep the original `"Transaction not confirmed within timeout"` network_error. + - `Err(_)` — the recovery RPC itself failed: still network_error, but include the RPC failure detail for ops triage. +- Pulled the four-case interpretation into a free function `interpret_post_timeout_status` so the cases are unit-testable without a live RPC. The RPC call site does `.map(|opt| opt.map(|inner| inner.map_err(|e| e.to_string())))` so the helper stays free of `solana-rpc-client` types. +- No `Store` trait change, no key-shape change, no two-phase commit. The signature is still reserved before confirmation polling; this just rescues the recovery path. + +**Note on retry idempotency (Medium-shape we didn't take):** if a caller retries the same credential after a successful recovery, `consume_signature` still returns `signature_consumed`. We treat that as the correct outcome — the SDK doesn't store receipts indexed by signature, so we can't replay one. Adding that capability is a separate change, and a well-behaved caller does the recovery on the first attempt now that the status check rescues it. + +**New tests** (against the pure helper): +- `interpret_post_timeout_status_landed_returns_ok` +- `interpret_post_timeout_status_landed_with_onchain_err_returns_failed` +- `interpret_post_timeout_status_not_found_returns_timeout` +- `interpret_post_timeout_status_rpc_error_returns_timeout_with_detail` + +--- + +### #2 — Credential verification uses echoed request +**ID:** `2a3fd1f3` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim:** `verify_credential` decodes `ChargeRequest` from `credential.challenge.request` and verifies the payment against *that*. The HMAC tier confirms the challenge was issued by this server, and `verify_pinned_fields` pins currency/recipient against `self`, but **nothing pins the amount or other per-route economics**. A server that issues challenges for multiple priced routes (the common case for any non-trivial API) will see `verify_credential` accept a $1 credential against a $100 route. + +**Decision:** ✅ **accepted — delete the unsafe method outright. Breaking change accepted.** + +**Rationale:** The simple `verify_credential` API is safe only for servers that serve exactly one priced resource. The audit's recommendation was "reserve `verify_credential` for flows where the echoed challenge fully defines the payable resource," but pure documentation is a soft control — the footgun stays available. With breaking changes permitted at this stage, the strongest enforcement is removing the method so every caller is forced through `verify_credential_with_expected` with an explicit expected `ChargeRequest`. + +**Action taken:** +- Deleted `Mpp::verify_credential` from `server/charge.rs`. +- Updated `verify_credential_with_expected`'s rustdoc to be the canonical entry point and to call out audit #2 explicitly. +- Updated the rustdoc examples in `src/lib.rs` and the top of `src/server/charge.rs` to construct an `expected: ChargeRequest` from a route's static configuration. +- Migrated 6 unit tests (1 HMAC tier-1 + 5 tier-2 pinned-field tests) to call `verify(&cred, &request)` directly — that's the lowest-level public API and exercises the same layers the deleted method used. +- Migrated 9 integration test callsites in `tests/charge_integration.rs` to construct `expected` from the test's known configuration and call `verify_credential_with_expected`. Added a tiny `expected_charge(amount, currency, recipient)` helper that mirrors how SDK consumers should build the expected request from their own static config (not from the credential). +- No production callsite changed — `axum.rs` already used `verify_credential_with_expected`. + +**Note on `verify` still being public:** the lowest-level `verify(&self, &credential, &request)` remains public. A caller who really wants "trust the echoed request" can still write `let req = cred.challenge.request.decode()?; mpp.verify(&cred, &req).await`. We keep that escape hatch because `verify` takes an *explicit* request — the caller is now visibly choosing what to verify against, which is what the audit's spirit asks for. + +**Note on the future rename:** `verify_credential_with_expected` is wordy. After audit #1 tightens its internals (it still uses the credential-decoded request to populate most fields during settlement), I'd like to rename it back to `verify_credential`. Not done in this commit so the long name keeps signalling "expected required" while #1 is open. + +**Tests** (no new tests authored — the existing security boundary tests were migrated to the surviving APIs without loss of coverage): +- HMAC: `verify_rejects_tampered_id` (renamed from `verify_credential_rejects_tampered_id`) +- Tier-2 pinned-field: `tier2_rejects_tampered_realm`, `_currency`, `_recipient`, `_method`, `_non_charge_intent` (all migrated to `verify`) +- The pre-existing `verify_credential_with_expected_*` tests still cover the expected-comparison layer. + +--- + +### #1 — Partial expected charge validation +**ID:** `4e2a4d2d` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim, two parts:** +1. `verify_credential_with_expected` compared only `amount`, `currency`, `recipient` between the credential's decoded request and the expected request — leaving `externalId`, `description`, `methodDetails.splits`, `feePayer`, `feePayerKey`, `tokenProgram`, `network`, `decimals`, `recentBlockhash` unchecked. +2. After the partial comparison, the function called `verify` with the credential's decoded request rather than the expected request, so unchecked fields flowed into on-chain settlement. + +**Status when reviewed:** +- Part 2 was **already fixed**. The current code passes `expected` (not the credential's request) into `verify`, as proven by the existing `verify_credential_with_expected_routes_expected_into_verify` test. The marketplace-route attack the audit describes is already closed at on-chain verification. +- Part 1 was still live. + +**Decision:** ✅ **accepted — exhaustive up-front comparison, with one principled exception.** + +**Rationale:** Adding the up-front comparison gives earlier, clearer failure (`splits mismatch` beats `no matching SPL transferChecked instruction` for operators chasing a bug), and provides defense-in-depth: any field added to `ChargeRequest` or `MethodDetails` in the future is forced through this layer, so a divergence cannot silently slip past the settlement check. + +**Action taken:** +- Extracted a new helper `compare_expected_to_request(&request, &expected)` at module level. The helper compares every payment-constraining field exhaustively. Called from `verify_credential_with_expected` right after the credential's request is decoded. +- Fields compared: + - top level: `amount`, `currency`, `recipient`, `external_id`, `description` + - `method_details`: `network`, `decimals`, `token_program`, `fee_payer`, `fee_payer_key`, `splits` +- Splits compared element-wise (order-sensitive). A route that pins `[A, B]` will reject a credential carrying `[B, A]`. +- `method_details` parsing reuses `MethodDetails`; if either side has malformed `methodDetails` we return `credential_mismatch` with the source labeled (`"Invalid credential methodDetails: …"` vs `"Invalid expected methodDetails: …"`). +- The pre-existing `_routes_expected_into_verify` test had to update its assertion text — my new comparison catches the malformed `expected.method_details` *before* `verify` is called, so the failure surface moved from settlement to comparison. The test's intent (proving `expected` is the source of truth) is preserved. + +**Note on `recent_blockhash`:** deliberately *not* compared. It's per-challenge state (fresh from the RPC at challenge generation time), not per-route policy. Routes build `expected` from static config and have no blockhash to pin. Strict comparison would break the normal happy path. Added a regression test `verify_credential_with_expected_ignores_recent_blockhash` to lock this in. + +**Note on `description`:** the audit lists `description` as a payment-constraining field even though it has no on-chain effect. We compare it strictly for consistency with the audit's recommendation. This surfaced a latent bug in the `payment_link_server` example: it issued challenges with `description = Some("Open a fortune cookie")` but built `expected` with no description, so every honest credential would have been rejected after this change. Fixed the example to use a `ROUTE_DESCRIPTION` constant in both places. (Audit value: this finding's strict comparison catches integrator drift between "what challenges we issue" and "what we expect to verify against" — exactly the audit's defense-in-depth intent.) + +**Note on alternative ("soft default") we did not take:** an "if `expected..is_none()` accept anything" variant would have made the comparison friendlier for routes that don't pin every field, but it's exactly the soft-default that lets the audit's attack through — routes that *meant* to pin a field but forgot would silently accept any value. Strict comparison forces the route to fully describe its accepted charge shape. + +**New tests** (added to the existing `verify_credential_with_expected_*` suite): +- `_external_id_mismatch` +- `_description_mismatch` +- `_network_mismatch` +- `_decimals_mismatch` +- `_token_program_mismatch` +- `_fee_payer_mismatch` +- `_fee_payer_key_mismatch` +- `_splits_mismatch` +- `_ignores_recent_blockhash` (regression: blockhash divergence must NOT fail comparison) + +--- + +## Low severity + +### #39 — `parse_units` can overflow +**ID:** `4f8d51a3` · **File:** `crates/mpp/src/protocol/intents/mod.rs:18` + +**Audit claim:** the integer branch of `parse_units` computes `10u128.pow(decimals) * value` with neither input bound and neither operation checked. Depending on build mode that's a panic or a silent wrap. + +**Decision:** ✅ **accepted — cap + checked arithmetic.** + +**Action taken:** +- Added `MAX_DECIMALS: u8 = 18`. Solana SPL convention is 0–9 per the protocol spec; 18 gives ERC-20-style headroom while staying well below the 39-where-`10.pow`-overflows cliff. Single rejection site so any callsite that hasn't validated upstream gets a clear error. +- `parse_units` rejects `decimals > MAX_DECIMALS` up-front. +- `10u128.pow(decimals)` → `checked_pow(...)` with explicit overflow error. +- `value * factor` → `checked_mul(...)` with explicit overflow error. +- Decimal-branch (`"1.5"` etc.) is string concatenation — no arithmetic to overflow; the cap still applies for consistency. + +**Note on scope:** the `Mpp::Config.decimals: u32` → `as u8` truncation at the callsite is a latent boot-time issue but belongs with the audit #16 batch (boot-time footgun guards). Not bundled here to keep this fix surgical. + +**New tests:** +- `parse_units_rejects_decimals_above_max` +- `parse_units_at_max_decimals_succeeds` +- `parse_units_rejects_value_times_factor_overflow` +- `parse_units_huge_value_zero_decimals_no_overflow` (boundary at `u128::MAX`) + +--- + +### #30 — Summing split amounts exposed to overflows +**ID:** `7e2b1c5e` · **Files:** `crates/mpp/src/{protocol/solana.rs,server/charge.rs,client/charge.rs,error.rs}` + +**Audit claim:** three callsites (`build_charge_transaction_with_options`, `verify_on_chain`, `verify_versioned_transaction_pre_broadcast`) summed split amounts via `.sum::()` which panics on overflow in debug and wraps in release. Spec: `sum(splits) ≤ amount`. + +**Decision:** ✅ **accepted — extract helper, use checked arithmetic, centralize the count cap.** + +**Action taken:** +- Added `checked_sum_split_amounts(splits: &[Split]) -> Option` in `protocol/solana.rs` (next to `Split`). Uses `try_fold` + `checked_add`. Unparseable amounts treated as `0` for now — strict parseability is audit #21's concern. +- Migrated all 3 callsites to the helper, mapping `None` to the existing error type at each callsite (client `Error::SplitsExceedAmount`, server `VerificationError::invalid_amount`). +- **Bonus centralization (per Ludo): added `pub const MAX_SPLITS: usize = 8`** in `protocol/solana.rs` and replaced the two hardcoded `8`s (client `splits.len() > 8`, server `verify_versioned_transaction_pre_broadcast`). The `thiserror`-generated `Error::TooManySplits` message now interpolates `MAX_SPLITS` so the displayed count stays in sync if the cap ever changes. The `MethodDetails::splits` rustdoc now references the constant rather than the literal. + +**New tests:** +- `checked_sum_split_amounts_within_u64_sums_correctly` +- `checked_sum_split_amounts_overflows_returns_none` +- `checked_sum_split_amounts_unparseable_treated_as_zero` +- `checked_sum_split_amounts_empty_is_zero` + +--- + +### #8 — Balance diagnostics decimal overflow +**ID:** `6c8a7d18` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim:** `diagnose_balances` computed `10u64.pow(methodDetails.decimals)` to build a UI-amount divisor. `decimals` is `Option` bounded only by the type — values ≥ 20 panic (debug) or wrap (release). The function runs *after* settlement already failed and is best-effort. + +**Decision:** ✅ **accepted — extract a checked helper, silently omit the token-balance hint when the divisor doesn't fit.** + +**Action taken:** +- Extracted `to_ui_amount(amount_base_units: u64, decimals: u8) -> Option` next to `diagnose_balances`. Uses `checked_pow` and returns `None` when the divisor can't be represented. +- `diagnose_balances` now early-skips the token-balance diagnostic via `if let Some(needed) = ...`. The fee-payer SOL diagnostic below still runs. +- No new `MAX_DECIMALS` cap needed at this site — the checked_pow returning None is the cap. + +**New tests** (against the helper): +- `to_ui_amount_typical_decimals` (6-decimal USDC case) +- `to_ui_amount_zero_decimals` (divisor = 1) +- `to_ui_amount_returns_none_when_divisor_overflows_u64` (decimals = 20, 255) +- `to_ui_amount_safe_high_decimals_succeed` (boundary: 19 fits, 20 doesn't) + +--- + +### #13 — Hardcoded token program in `diagnose_balances` +**ID:** `b1f6e3a4` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim:** `diagnose_balances` derived the payer's ATA with a hardcoded `programs::TOKEN_PROGRAM`. For Token-2022 mints (PYUSD, USDG on Token-2022, CASH) this produced the wrong ATA, so the diagnostic could silently lie about the payer's balance. + +**Decision:** ✅ **accepted — use the value already resolved at boot.** + +**Rationale:** Audit #28 resolves the token program once in `Mpp::new` (static table for known stablecoins, on-chain mint-owner lookup for arbitrary mints) and embeds it on every SPL challenge as `methodDetails.tokenProgram`. The diagnostic just needs to use that value instead of guessing. No runtime RPC call needed — the resolution already happened at boot. + +**Action taken:** +- Read `method_details.token_program` and parse to `Pubkey`. If `Some` and parseable → use for ATA derivation. +- If `None` (or unparseable, which would be a separate validation failure upstream) → silently skip the token-balance diagnostic. The fee-payer SOL diagnostic below still runs. +- No `default_token_program_for_currency` fallback — that's exactly the wrong-for-Token-2022 path the audit flagged. + +**Note on no new tests:** the fix is a one-spot value-swap inside a private, RPC-bound, best-effort diagnostic. The arithmetic helpers in #8 cover the testable surface; the change here is "use the right input." Covered by the existing integration tests that exercise the token-2022 challenge paths. + +--- + +### #9 — Challenge parser missing max size cap +**ID:** `2f9c8d1e` · **File:** `crates/mpp/src/protocol/core/headers.rs` + +**Audit claim:** `parse_www_authenticate` decoded the `request` parameter (base64url) and parsed it as JSON without the `MAX_TOKEN_LEN = 16 * 1024` cap that `parse_authorization` and `parse_receipt` already enforced. A large `WWW-Authenticate` value drove proportionally larger decode + JSON parse work than the credential/receipt parsers allowed. + +**Decision:** ✅ **accepted — cap the `request` parameter at `MAX_TOKEN_LEN`.** + +**Rationale:** The audit asks for "consistent limits across challenge, credential, and receipt parsers." The credential parser caps the `token` (the data after the scheme); the receipt parser caps the `token` value. For the challenge parser, the only field that gets both base64-decoded and JSON-parsed is `request` — every other parameter (id/realm/method/intent/expires/digest) is a short pass-through string. Capping `request` matches the *kind* of work the other parsers cap. + +**Action taken:** added a `request_b64.len() > MAX_TOKEN_LEN` check immediately after `request` is read from the parameters and before `base64url_decode`/`serde_json::from_slice` run. Error message matches the parse_authorization/parse_receipt style for ops grep-ability. + +**What I didn't do:** +- Didn't cap the full header alongside the param cap — redundant once the param is capped, since the request param is the only field that drives O(n) decode/parse cost. +- Didn't cap `opaque` here — at parse time it's only stored raw via `Base64UrlJson::from_raw`. Any decode is lazy at the consumer site. + +**New tests:** +- `parse_www_authenticate_rejects_oversized_request_param` +- `parse_www_authenticate_accepts_at_max_request_size` (regression: at-cap shouldn't fire the size gate) + +--- + +### #42 — Decimal management contradicts the specs +**ID:** `7a1c2e4f` · **Files:** `crates/mpp/src/client/charge.rs`, `server/charge.rs` + +**Audit claim:** spec §7.2 marks `decimals` as conditionally required (MUST be present for SPL, MUST be absent for SOL). Two callsites used `method_details.decimals.unwrap_or(6)`, silently defaulting non-6-decimal SPL flows to a wrong divisor. + +**Decision:** ✅ **accepted — asymmetric fix.** + +**Rationale:** The two callsites have different audiences. The client builder is user-facing and produces signed transactions — silent wrong-decimals output is the worst possible failure mode, error out. The server's `diagnose_balances` is a post-failure best-effort hint — falsely confident output is worse than no output, silently skip (same shape as the audits #8 and #13 fixes for the same function). + +**Action taken:** +- **Client `build_spl_instructions`:** `unwrap_or(6)` → `ok_or(Error::Other("methodDetails.decimals is required for SPL charges (spec §7.2)"))?`. Path only runs when `currency` resolves to a mint (we're inside the SPL branch), so the spec's "MUST be present for mint" is the active rule. +- **Server `diagnose_balances`:** wrapped the token-balance diagnostic in `if let Some(needed) = method_details.decimals.and_then(|d| to_ui_amount(...))` — missing decimals now silently omits the line rather than guessing 6. Fee-payer SOL diagnostic still runs. + +**Note on `Mpp::charge` (server challenge issuer):** unchanged — it already populates `decimals` from `self.decimals` for every challenge this server issues. The `None` path in `diagnose_balances` is the lower-level-construction edge case. + +**New tests:** +- Client: `build_spl_rejects_missing_decimals`. +- Server: no new test — diagnose_balances is private + RPC-bound; the silent-skip branch is the same shape proven by the #8 tests. + +--- + +### #16 — `PaymentChallenge` instances can be created with `feePayer = true` and `fee_payer_signer = None` +**ID:** `9e3b1c47` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim:** spec §7.2 requires `feePayerKey` to be present when `feePayer = true`. `Mpp::new` accepted `Config { fee_payer: true, fee_payer_signer: None }` without complaint, and `charge_with_options` then emitted a spec-violating challenge (`"feePayer": true` with no `"feePayerKey"`). + +**Decision:** ✅ **accepted — two gates, both call paths covered.** + +**Action taken:** +- **`Mpp::new`:** reject when `config.fee_payer && config.fee_payer_signer.is_none()`. After this gate the invariant `self.fee_payer` implies `self.fee_payer_signer.is_some()` holds for the server's static config. +- **`validate_charge_options`:** reject when `options.fee_payer && self.fee_payer_signer.is_none()`. Catches the per-call override where `Config.fee_payer == false` but a route sets `ChargeOptions.fee_payer = true`. +- Two pre-existing tests (`charge_with_fee_payer_includes_method_details`, `charge_options_fee_payer_flag`) constructed misconfigured Mpps that fell into the audit's exact shape; updated them to provide `test_fee_payer_signer()` so the assertions now exercise the spec-compliant path. + +**Note on alternative:** the type-level refactor (fold `fee_payer` + `fee_payer_signer` into a single `Option` enum that makes the invariant unrepresentable) is the more durable fix but a bigger ergonomic change. Not bundled — the runtime gates close the audit shape today. + +**New tests:** +- `new_rejects_fee_payer_true_without_signer` +- `new_accepts_fee_payer_false_without_signer` (regression: default no-signer config keeps working) +- `charge_options_rejects_fee_payer_without_signer` +- `charge_options_fee_payer_succeeds_when_signer_configured` (happy path; asserts `feePayerKey` is populated) + +--- + +### #15 — Default `realm` shares credential namespace across servers +**ID:** `8d1c4a72` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim:** `DEFAULT_REALM = "MPP Payment"`. Realm is part of the HMAC ID input, so two services that share `MPP_SECRET_KEY` and both keep the default realm participate in one shared credential namespace — a credential paid against service A passes HMAC verification on service B. + +**Decision:** ✅ **accepted — derive default from recipient pubkey.** + +**Rationale:** The audit gives two options ("require non-empty realm" *or* "derive a unique default from an application identifier/origin"). Requiring an explicit realm would force 41 callsite updates (tests, examples, integration) for marginal gain over a derived default that already differs per-app. The `recipient` is a Solana pubkey, unique per merchant, and already mandatory in `Config` — perfect as the app identity. Two services with the same secret but different recipients now automatically get different realms; HMAC IDs differ; cross-service replay broken. + +**Action taken:** +- Removed `const DEFAULT_REALM: &str = "MPP Payment"`. +- Added `fn derive_default_realm(recipient: &str) -> String` that hashes the recipient with SHA-256, takes the first 4 bytes as `u32::from_be_bytes` mod 10^8, and formats as `"App Id - #"`. Human-friendly and deterministic. +- `Mpp::new` resolves the realm via a small `match` that: + - rejects explicit `Some("")` with `Error::InvalidConfig` (closes the bypass where an operator could re-introduce the audit threat with a typo), + - uses the supplied non-empty realm if provided, + - else derives from `config.recipient`. +- Updated two pre-existing tests that asserted `realm == DEFAULT_REALM` to use `derive_default_realm(TEST_RECIPIENT)`. + +**What I didn't do:** +- Didn't fold the realm into the type system (e.g., `enum Realm { Derived, Explicit(String) }`) — the runtime check + derivation is enough to close the audit's threat without an ergonomic refactor. +- Didn't change the realm shape for explicit overrides; operators who set `realm: Some("Acme API")` keep getting `"Acme API"`. + +**Note on wire-format impact:** the realm appears in `WWW-Authenticate` headers and binds HMAC IDs. Servers upgrading from the previous SDK release will see in-flight challenges (issued with the old `"MPP Payment"` realm) fail to verify under the new derived realm. Default TTL is 5 minutes; the rollout window closes quickly. + +**New tests:** +- `new_default_realm_format` — asserts the `"App Id - #"` shape with up to 8 digits. +- `new_default_realm_deterministic_for_same_recipient` — restart-safe (same recipient → same realm). +- `new_default_realm_differs_across_recipients` — closes the audit threat shape. +- `new_rejects_empty_realm` — explicit empty string rejected. + +--- + +### #37 — Unconditional default to mainnet, plus naming inconsistency +**ID:** `1d5ea7b2` · **Files:** `crates/mpp/src/{server,client}/charge.rs`, `protocol/solana.rs`, `server/html.rs` + +**Audit claim:** the codebase silently treated any network slug other than `"devnet"`/`"localnet"` as mainnet-beta (e.g. in `default_rpc_url`), contradicting the spec's "MUST be one of mainnet/devnet/localnet". Two copies of `default_rpc_url` (one private, one public) drifted independently. The audit also asked us to decide between `"mainnet"` and `"mainnet-beta"` as the canonical slug. + +**Decision:** ✅ **accepted — allowlist at server boot, canonicalize on `"mainnet"`, consolidate.** + +Ludo's call: `"mainnet"` is the canonical slug. `"mainnet-beta"` is the Solana RPC hostname convention only. + +**Action taken:** +- Added `NETWORK_MAINNET`/`NETWORK_DEVNET`/`NETWORK_LOCALNET` constants and `DEFAULT_NETWORK = NETWORK_MAINNET` in `protocol/solana.rs`. +- Added `validate_network(&str) -> Result<(), Error>` in `protocol/solana.rs`. Rejects everything outside the allowlist; explicit empty-string handling for a cleaner error. +- `Mpp::new` calls `validate_network(&config.network)?` next to the other boot-time guards (#16, #15, #24). Misconfig like `Config { network: "mainnet-beta" }` or `"testnet"` fails at boot, before any RPC client is built. +- Removed the private `default_rpc_url` in `server/charge.rs`; the single callsite (`Mpp::new`) now uses `crate::protocol::solana::default_rpc_url`. Tests for the helper live next to the public copy. +- Client `select_charge_challenge` → `matches_network` no longer falls back to `"mainnet-beta"` when `methodDetails.network` is `None`; uses `DEFAULT_NETWORK` (= `"mainnet"`) per spec §7.2. +- Docstrings on `Config.network`, `SelectChargeChallengeOptions::network`, and the `protocol/solana.rs` constants updated to reflect the canonical slug. +- Test fixtures across `client/charge.rs` and `server/html.rs` that used `"mainnet-beta"` as a network slug → `"mainnet"`. (RPC hostname strings like `https://api.mainnet-beta.solana.com` are unchanged — that's a Solana hostname, separate concern.) + +**What I didn't bundle in this finding:** +- `server/session.rs` / `protocol/intents/session.rs` still carry `"mainnet-beta"` as a session-flow network slug. Different intent (session, not charge), separate audit scope; consciously skipped to keep the diff tight. +- `x402` crate has its own network handling with `"mainnet-beta"` references — that's a sibling protocol, not in scope for MPP audit #37. +- Didn't refactor `Config.network` to an `enum Network { Mainnet, Devnet, Localnet }`. Cleaner but a larger ergonomic change; the runtime allowlist closes the audit threat as-is. + +**New tests:** +- `new_accepts_canonical_networks` — loop over `{mainnet, devnet, localnet}`, all succeed. +- `new_rejects_unknown_network` — `"testnet"` → error. +- `new_rejects_empty_network` — distinct error message for empty input. +- `new_rejects_mainnet_beta_slug` — explicitly locks in the canonicalization decision. + +--- + +### #21 — Incomplete split validation at challenge creation +**ID:** `3a8f7c91` · **Files:** `crates/mpp/src/{protocol/solana.rs,server/charge.rs}` + +**Audit claim:** `validate_charge_options` ran additional split checks only when at least one split had `ataCreationRequired = true`. For all other splits, `charge_with_options` embedded them into `methodDetails` with no parseability check, no positive-amount check, no dedup, and no count cap at challenge issuance. Invalid splits then surfaced only at on-chain settlement. + +**Decision:** ✅ **accepted — shared helper, both server entry points validate.** + +**Action taken:** +- Added `validate_splits(&[Split]) -> Result<(), Error>` in `protocol/solana.rs` next to the existing `checked_sum_split_amounts` and `MAX_SPLITS`. Single source of truth. +- Enforces: count ≤ `MAX_SPLITS`, recipient parses as `Pubkey`, amount parses as `u64` and is `> 0`, aggregate sum doesn't overflow `u64` (reuses `checked_sum_split_amounts`), no duplicate recipients. +- Called from both `validate_charge_options` (per-call path) and `validate_charge_request` (the lower-level `charge_challenge_with_options` path). +- Removed the duplicated per-split loop in `validate_charge_request`; the helper handles it. +- One pre-existing test (`charge_with_options_splits`) used placeholder strings as recipient pubkeys that never parsed as base58 — now uses `Pubkey::new_unique()`. The other pre-existing test (`charge_challenge_rejects_invalid_split_recipient`) had its assertion text updated to match the unified helper's error string. + +**What I didn't do:** +- **No application-level recipient allowlist.** The audit's `Consider` for this was a domain-specific policy that doesn't belong in the SDK — applications can wrap. +- **No client-side change.** Splits originate from the server; the client only consumes them via `methodDetails`. + +**New tests** (in `protocol::solana::tests`): +- `validate_splits_accepts_valid_set` +- `validate_splits_accepts_empty` +- `validate_splits_rejects_count_above_max` +- `validate_splits_rejects_invalid_recipient` +- `validate_splits_rejects_unparseable_amount` +- `validate_splits_rejects_zero_amount` +- `validate_splits_rejects_overflowing_aggregate` +- `validate_splits_rejects_duplicate_recipient` + +**New entry-point regression tests** (in `server::charge::tests`): +- `charge_with_options_rejects_invalid_split_recipient` +- `charge_with_options_rejects_zero_split_amount` +- `charge_with_options_rejects_duplicate_split_recipient` +- `charge_with_options_rejects_too_many_splits` + +--- + +### #33 — No check for minimum remaining SOL balance for signers +**ID:** `c4e9a3d1` · **File:** `crates/mpp/src/client/charge.rs` + +**Audit claim:** the client builder constructs SOL `system_instruction::transfer` instructions without verifying the signer retains lamports for fees + rent-exempt minimum. Three risks: drain the signer, leave it below rent (account swept at epoch boundary), or sign a tx that fails on-chain for insufficient funds. + +**Decision:** ❌ **rejected — threat does not apply to the product.** + +**Rationale:** The product is stablecoin-only. Signers transfer SPL tokens (USDC, USDT, PYUSD, USDG, CASH); the SOL `system_instruction::transfer` code path exists in the SDK but is not the user-facing flow. Walking through the cases: + +| Case | Outcome | Who catches it | +|---|---|---| +| Signer = fee_payer, insufficient SOL for fee | tx fails at broadcast/execution | Solana runtime — signer pays nothing | +| Signer ≠ fee_payer (server-cosigned) | signer needs zero SOL | n/a | +| Server fee-payer underfunded | server tx fails | spec §13.6 — server's responsibility, separate concern | +| Signer drained below rent via SOL transfer | account swept silently at epoch | only matters if SOL transfer path is reached, which the product doesn't use | + +The "drain below rent" silent-sweep is the only failure mode the chain doesn't catch cleanly, and it requires the SOL transfer path the product doesn't expose to end users. + +**Prototype shipped briefly during analysis** (added `MIN_RENT_EXEMPT_LAMPORTS`, `MIN_FEE_RESERVE_LAMPORTS`, `skip_balance_check` opt-out, and a pre-sign `rpc.get_balance` check) but **reverted before commit** once we clarified that the SOL leg isn't a product path. The 11 tests that broke under the prototype confirmed how invasive the change would be relative to a threat that doesn't apply. + +**What this leaves on the table:** +- Spec §13.6 (server fee-payer balance monitoring) is a real ask, but it targets the **server side** of fee-sponsored flows, not the client builder. Tracked separately if/when we address it. +- The `build_sol_instructions` function stays public but unprotected. If a future caller starts using the SOL path for end-user-facing flows, the audit's concern becomes live again — revisit at that point. + +--- + +### #22 — Lower-level `verify` request not bound to challenge +**ID:** `e5b8a47f` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim:** `verify(&credential, &request)` recomputes HMAC from `credential.challenge.request` (confirming the challenge was server-issued) but settles against the caller-supplied `&request`. They can diverge — a direct caller could authenticate "challenge Y was issued" while verifying settlement against "request X". The escape hatch kept public after audit #2. + +**Decision:** ✅ **accepted — bind request to credential inside `verify`.** + +**Rationale:** The audit gave two options: (1) require `request == credential.request` inside `verify`, or (2) make the low-level API private/rename it. (1) closes the threat for any caller without breaking the API or forcing every caller into `verify_credential_with_expected`. (2) was a half-measure that either (a) only hid the API from external callers via `pub(crate)` while the trust gap remained for internal callers, or (b) needed a full rename (`verify_request_unchecked`) that breaks tests for marginal extra clarity. + +**Action taken:** +- After the HMAC tier-1 check, `verify` decodes `credential.challenge.request` and calls `compare_expected_to_request(&decoded_from_credential, request)?` — the same audit #1 helper that `verify_credential_with_expected` uses. +- For `verify_credential_with_expected`: the comparison fires twice (once at the outer entry, once inside `verify`). Cheap, defense-in-depth. +- For direct callers of `verify`: any divergence between the supplied request and the credential's HMAC-authenticated request is rejected at the binding check, with the same per-field mismatch errors operators already see from audit #1. +- Tier-2 unit tests (added in audit #2's migration) construct `request` as-if-decoded-from-the-credential, so they pass the new check naturally — no regression. + +**Note on `verify` still being public:** the API is now self-protected. Direct callers are free to use it; they just can't pass a divergent request. Kept public because tests and well-behaved callers (notably `verify_credential_with_expected`) need it as a building block. + +**New test:** +- `verify_rejects_request_diverging_from_credential` — HMAC tier passes (credential not tampered), caller passes a different amount than the credential carries; expects the audit #1-style "Amount mismatch" error from the binding check. + +--- + +### #17 — Missing method and intent enforcement on Client and Server +**ID:** `5f3c1d68` · **Files:** `crates/mpp/src/server/charge.rs`, `crates/mpp/src/client/charge.rs` + +**Audit claim, two parts:** +1. **Server:** `verify` recomputes HMAC using whatever method/intent the credential echoes; never explicitly checks `method == "solana"` and `intent == "charge"` after HMAC. A challenge issued by the same server secret for another method/intent could reach the Solana charge verification path. +2. **Client:** `build_credential_header` doesn't reject non-`solana`/non-`charge` challenges before signing. + +**Status when reviewed:** +- **Server:** **already mitigated.** `verify_pinned_fields` (called unconditionally from `verify`) checks both `method` and `intent` — the `tier2_rejects_tampered_method` and `tier2_rejects_non_charge_intent` tests prove it. No code change needed; documenting in the assessment. +- **Client:** real gap. `select_charge_challenge` filters via `is_solana_charge_challenge_name`, but `build_credential_header_with_options` accepts whatever challenge it's handed. + +**Decision:** ✅ **accepted, client-only — close the entry-point gap.** + +**Action taken:** +- Added a method/intent gate at the top of `build_credential_header_with_options` (right after the audit #17 comment block, before the audit #10 expiry check). Reuses `is_solana_charge_challenge_name`. Error string surfaces both the unexpected method and intent for operator debugging. +- The lower-level `build_charge_transaction_with_options` doesn't change — it takes already-decoded fields and has no notion of method/intent; the trust boundary belongs at the `PaymentChallenge` entry point. +- Server-side: no code change. The pre-existing `verify_pinned_fields` already enforces the audit's exact recommendation. This entry calls it out so future readers know the check is intentional, not redundant. + +**New tests** (client): +- `build_credential_header_rejects_non_solana_method` — `method = "stripe"` → error with both `method=` and `intent=` in the message. +- `build_credential_header_rejects_non_charge_intent` — `intent = "session"` → same shape. + +--- + +### #5 — Push signature not bound to challenge +**ID:** `8b2f1e9c` · **File:** `crates/mpp/src/server/charge.rs` + +**Audit claim:** push-mode credentials (`CredentialPayload::Signature`) match on-chain transactions to challenges by shape (recipient, amount, currency, splits) only. Replay protection applies to the signature *after* verification. The on-chain tx carries no unique binding to a specific challenge, so two challenges with identical shape (or any unrelated payment with matching shape) can satisfy each other — "first accepted presentation wins." + +**Decision:** 🟡 **partial — spec-aware accept + opt-in gate.** + +**Rationale:** This is **acknowledged by the spec.** `draft-solana-charge-00.txt:1247-1268` (§13.5 "Front-running (Push Mode)") explicitly names the same attack model: +> "Push mode does not require the on-chain transaction to carry a challenge-specific marker. It proves that a payment matching the challenged terms was made, but not necessarily that the payment was created for one unique challenge instance. If multiple valid challenges have identical terms, the same confirmed transaction could satisfy any one of them, and the first accepted presentation wins." + +The spec also considers and rejects mandating the audit's recommended mitigation (a Memo carrying the challenge id): +> "Requiring an on-chain marker such as a Memo carrying the challenge id would provide stronger binding, but would also reveal extra correlation metadata on chain. This specification does not require such a marker in the base flow, but implementations MAY define a backward-compatible profile that does." + +So the base flow we ship is spec-compliant. Mandating the challenge-id memo would impose a privacy cost (each payment correlated to a specific request on-chain) the spec author explicitly declined to bake in. We follow suit. + +**What we add anyway:** +- **`Config::accept_push_mode: bool` (default `false`).** Opt-in flag for accepting push-mode credentials. Default-off means servers that don't actively need push mode reduce their attack surface — the §13.5 trade-off only applies to operators who explicitly choose it. Independent of the binding question. +- The new gate runs **before** B34 (the existing fee-payer-route reject). When push mode is off, the rejection message points at the spec section for ops triage; when on, B34 still narrows the fee-sponsored case. + +**Action taken:** +- `Config { ..., accept_push_mode: false }` plumbed through to `Mpp` and into the push-mode branch of `verify`. +- One pre-existing test (`b34_rejects_push_credential_on_fee_payer_route`) had to set `accept_push_mode: true` to exercise the B34-specific path in isolation now that the audit #5 gate runs first. +- `interop_server.rs` (the interop harness binary) sets `accept_push_mode: push_mode` so the interop suite still exercises push mode end-to-end when it's the mode under test. + +**What we didn't do** (and why): +- **Mandatory challenge-id memo profile** (audit's recommendation). Spec §13.5 explicitly leaves this as MAY, not MUST, citing on-chain correlation metadata as the trade-off. Adding it unilaterally would impose a privacy regression the spec author chose to avoid. If/when the spec evolves to mandate the profile, we adopt. +- **`request.external_id` enforcement for push mode.** Considered as a lower-cost alternative to the memo profile — but it conflates `external_id` (a business identifier integrators control) with a challenge-binding marker, and still imposes the on-chain correlation cost. Skip until the spec moves. +- **Server-side `verify_push` enrichment.** The existing memo verifier already enforces `external_id`-bound memos when integrators choose to use the field. No change there. + +**Note on the attacker model** that came up during analysis: +- The attacker doesn't need the victim's challenge id. They request their own challenge (the 402 endpoint is open) for the same resource, then submit the victim's on-chain signature against their own challenge. HMAC validates (their own challenge), shape matches (same recipient/amount/currency), signature points to a real tx — first-accepted-presentation wins, attacker gets service. +- This is exactly the model spec §13.5 names as the accepted base-flow trade-off. + +**New tests:** +- `verify_rejects_push_credential_when_accept_push_mode_off` — default Mpp, push credential, expect rejection with both "Push-mode credentials are disabled" and "§13.5" in the message. +- `verify_passes_audit_5_gate_when_accept_push_mode_on` — opt-in Mpp, confirm the audit #5 gate doesn't fire (downstream errors from the fake signature are fine; just not the gate's error). + +--- + +## Informational + +### #44, #45, #27, #14, #34 — Input strictness pass +**Commit:** `d015be1` + +Five informational findings batched together — input-strictness or docstring fixes. + +- **#44 / #45 — `parse_units` edge cases:** previously accepted dotted values missing the integer (`".5"`), the fraction (`"5."`), both (`"."`), or with more than one dot (`"1.2.3"` silently became `123` because `split_once('.')` stopped at the first dot). Now rejects all of those plus any non-ASCII-digit characters on either side. The pre-existing `parse_units_zero_decimals_with_dot` test (which expected `"1." == "1"`) became `parse_units_zero_decimals_with_trailing_dot_rejected`. Five new tests cover the new reject paths. +- **#27 — `resolve_mint` docstring:** previously said "Returns `None` for native SOL, or `Some(mint_address)` for SPL tokens." Now documents the third case: `Some(input passthrough)` for unknown symbols. +- **#14 — `SelectChargeChallengeOptions` precedence:** docstrings on `currency` and `currency_preferences` now state the precedence rule explicitly (`currency_preferences` takes priority when non-empty). +- **#34 — `ataCreationRequired` mint-address check:** both `verify_versioned_transaction_pre_broadcast` and `verify_on_chain` switched from the oblique `request.currency != expected_mint.to_string()` check to a direct `Pubkey::from_str(&request.currency).is_err()`. Same outcome, clearer intent. The same comment block in `verify_on_chain` references the matching block above. + +--- + +### #41, #11, #36 — API hygiene pass +**Commit:** `__` + +Three informational findings — small touch-ups that don't change behaviour for honest callers. + +- **#41 — Constant-time HMAC id comparison:** `server::charge::verify` used a plain `!=` to compare the credential's challenge id against the recomputed HMAC, even though the existing `constant_time_eq` helper backed `PaymentChallenge::verify`. A timing oracle could in principle leak how many leading bytes of an attacker-controlled id match an actually-issued one. The helper is now `pub(crate)` and called directly. No behaviour change for honest callers; the timing channel closes. +- **#11 — `VerificationError` title alignment:** `invalid_amount`, `invalid_recipient`, and `credential_mismatch` now have titles that match their function names (`"Invalid Amount"`, `"Invalid Recipient"`, `"Credential Mismatch"`) instead of repeating the bucket label. Codes (`verification-failed`, `malformed-credential`) unchanged so consumers grouping by code keep working. +- **#36 — Explicit commitment on client blockhash fetch:** `build_charge_transaction_with_options` previously called `rpc.get_latest_blockhash()` (no explicit commitment), relying on the RPC client's default. Solana's client guidance recommends `confirmed` — a `processed` hash can disappear under reorgs, leaving the client with a signed transaction that fails with `BlockhashNotFound` after broadcast. Now calls `get_latest_blockhash_with_commitment(CommitmentConfig::confirmed())`. + +--- + +### #40 — Push-mode + fee-sponsored already rejected (no code change) +**Existing mitigation:** the B34 reject at `server/charge.rs:837` (just below the audit #5 gate) refuses `CredentialPayload::Signature` when `methodDetails.feePayer == true`. Mirrors the spec §8.3 prohibition. The pre-existing `b34_rejects_push_credential_on_fee_payer_route` test (now in the same `accept_push_mode: true` branch added by audit #5) covers it. No code change needed; this entry documents the alignment. + +--- + +### #23 — Server fee-payer funds split ATA creation (already addressed) +**Existing mitigation:** audit #20 closed the implicit client-funded ATA creation in `build_spl_instructions` (the client now only emits an ATA-creation instruction when `ataCreationRequired: true` is explicitly set on the split). Audit #38 added the misconfig guard rejecting the primary recipient + `ataCreationRequired` combo at challenge issuance. Audit #21 enforces full split validation (positive amounts, dedup, parseable pubkeys) before the policy is computed. Together these cover the audit's "treat `ataCreationRequired` as server-trusted policy only" recommendation — the field is only honored when the server-side challenge-build path explicitly sets it. No additional code change. + +--- + +### #35 — Replay protection scope (already addressed by #3) +**Existing mitigation:** audit #3 reserved the consume_signature slot between broadcast and confirmation polling (PR #85 / G05), and the audit #3 fix also added the post-timeout `get_signature_status` recovery so a tx that landed during a polling timeout isn't lost. The audit #35 description was about the broader "what counts as consumed" model, which we already match: the signature is reserved before the confirmation poll completes and stays reserved on every outcome the server is responsible for. No additional code change. + +--- diff --git a/rust/crates/mpp/examples/payment_link_server.rs b/rust/crates/mpp/examples/payment_link_server.rs index b691d1d6..ab045cbc 100644 --- a/rust/crates/mpp/examples/payment_link_server.rs +++ b/rust/crates/mpp/examples/payment_link_server.rs @@ -17,15 +17,27 @@ use std::collections::HashMap; use std::sync::Arc; const ROUTE_PRICE: &str = "0.01"; +const ROUTE_DESCRIPTION: &str = "Open a fortune cookie"; /// Build the route's expected charge request. Threading this into /// `verify_credential_with_expected` is what protects against cross-route /// credential replay — without it, a credential issued for a cheaper route /// (or different recipient/currency) on the same server would be accepted. +/// +/// Important: the options here MUST match the options used when the route +/// issues its user-facing challenge. Audit #1 compares every +/// payment-constraining field (including description), so a mismatch here +/// would reject every honest credential. fn expected_request_for_route(mpp: &Mpp) -> Option { - mpp.charge(ROUTE_PRICE) - .ok() - .and_then(|challenge| challenge.request.decode().ok()) + mpp.charge_with_options( + ROUTE_PRICE, + solana_mpp::server::ChargeOptions { + description: Some(ROUTE_DESCRIPTION), + ..Default::default() + }, + ) + .ok() + .and_then(|challenge| challenge.request.decode().ok()) } const CSP: &str = "default-src 'self'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src *; worker-src 'self'"; @@ -85,12 +97,13 @@ async fn fortune( .into_response(); } - // Generate challenge. + // Generate challenge. Options here must match `expected_request_for_route` + // exactly — audit #1 compares every payment-constraining field. let challenge = mpp .charge_with_options( ROUTE_PRICE, solana_mpp::server::ChargeOptions { - description: Some("Open a fortune cookie"), + description: Some(ROUTE_DESCRIPTION), ..Default::default() }, ) diff --git a/rust/crates/mpp/src/bin/harness_server.rs b/rust/crates/mpp/src/bin/harness_server.rs index c7f3400a..7ef2e2c1 100644 --- a/rust/crates/mpp/src/bin/harness_server.rs +++ b/rust/crates/mpp/src/bin/harness_server.rs @@ -56,7 +56,9 @@ use solana_mpp::{ const DEFAULT_RESOURCE_PATH: &str = "/protected"; const HEALTH_PATH: &str = "/health"; const DEFAULT_PRICE: &str = "0.001"; -const DEFAULT_SECRET_KEY: &str = "mpp-harness-secret-key"; +// Audit #24: ≥32 bytes for HMAC-SHA256 keys. Pad to keep the harness +// default usable when no MPP_HARNESS_SECRET_KEY is set in the env. +const DEFAULT_SECRET_KEY: &str = "mpp-harness-secret-key-with-32b-pad"; const DEFAULT_SETTLEMENT_HEADER: &str = "x-fixture-settlement"; const DEFAULT_TOKEN_DECIMALS: u8 = 6; @@ -144,6 +146,11 @@ fn read_state() -> Result _ => DEFAULT_TOKEN_DECIMALS, }; let splits = read_splits()?; + // Refuse to boot with invalid splits (audit #21). The harness + // misconfig scenario depends on this — every server SDK should + // reject the misconfig consistently, and refusing to start is the + // earliest possible signal. + solana_mpp::protocol::solana::validate_splits(&splits)?; Ok(HarnessState { mpp: Mpp::new(Config { @@ -158,6 +165,9 @@ fn read_state() -> Result fee_payer_signer: if push_mode { None } else { Some(fee_payer) }, store: None, html: false, + // Interop tests exercise push mode end-to-end; the gate is + // opt-in (audit #5) so we set it explicitly here. + accept_push_mode: push_mode, })?, price, push_mode, diff --git a/rust/crates/mpp/src/client/authenticate.rs b/rust/crates/mpp/src/client/authenticate.rs index ce78f101..6ab920f4 100644 --- a/rust/crates/mpp/src/client/authenticate.rs +++ b/rust/crates/mpp/src/client/authenticate.rs @@ -193,13 +193,15 @@ mod tests { currency: "EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v".into(), decimals: 6, network: "mainnet".into(), - challenge_binding_secret: Some("test-secret".into()), + // ≥32 bytes to satisfy the audit #24 secret-length check at Mpp::new. + challenge_binding_secret: Some("test-secret-key-for-authenticate-32b-pad".into()), ..Default::default() }) .expect("mpp") .charge_challenge(&crate::ChargeRequest { amount: "1000".into(), currency: "EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v".into(), + recipient: Some(signer.pubkey().to_string()), ..Default::default() }) .expect("charge challenge"); diff --git a/rust/crates/mpp/src/client/charge.rs b/rust/crates/mpp/src/client/charge.rs index d042e079..be96a2f9 100644 --- a/rust/crates/mpp/src/client/charge.rs +++ b/rust/crates/mpp/src/client/charge.rs @@ -45,17 +45,53 @@ pub async fn build_charge_transaction( pub struct BuildChargeTransactionOptions { /// Optional root payment memo. Spec-aligned callers pass `ChargeRequest.externalId`. pub external_id: Option, + /// Opt-in: sign for an unknown Token-2022 mint. + /// + /// Token-2022 supports transfer hooks that run arbitrary program code on + /// every transfer. We refuse to sign for mints outside the known + /// stablecoin allowlist when they live on Token-2022 unless the caller + /// explicitly accepts that risk by setting this flag. The vanilla Token + /// Program has no hooks, so arbitrary mints there are always allowed. + pub allow_unknown_token_2022: bool, + /// Audit #10: client-side cap on what the wallet will sign. + /// + /// When set, the builder refuses to sign a challenge whose `amount` + /// (in base units) exceeds this value. Intended for auto-pay + /// integrations where the user can't review each challenge by hand + /// and the server is therefore implicitly untrusted. + pub max_amount_base_units: Option, + /// Audit #10: client-side pin on the network the wallet will sign for. + /// + /// When set, the builder refuses to sign a challenge whose + /// `methodDetails.network` does not match this value. Prevents an + /// auto-pay agent meant for one network from being lured into + /// signing a transaction for another. + pub expected_network: Option, } /// Options for selecting one Solana charge challenge from a challenge set. #[derive(Debug, Clone, Copy, Default)] pub struct SelectChargeChallengeOptions<'a> { /// Currency symbol or mint address the client wants to pay with. + /// + /// Audit #14: this is the *fallback* preference — if + /// `currency_preferences` is non-empty it takes priority and this + /// field is ignored. Set one or the other, not both. pub currency: Option<&'a str>, /// Currency symbols or mint addresses in client preference order. + /// + /// Audit #14: when non-empty, takes priority over `currency`. pub currency_preferences: &'a [&'a str], - /// Solana network identifier, e.g. "mainnet", "devnet", or "localnet". + /// Solana network identifier, one of "mainnet", "devnet", or "localnet" + /// (spec §7.2). The legacy "mainnet-beta" name is the RPC hostname, not + /// a canonical slug. pub network: Option<&'a str>, + /// Opt-in: select challenges whose currency is an unknown Token-2022 mint. + /// See `BuildChargeTransactionOptions::allow_unknown_token_2022` for the + /// underlying threat model. Default `false` — unknown Token-2022 + /// challenges (and challenges whose token program we can't determine + /// from `methodDetails`) are skipped. + pub allow_unknown_token_2022: bool, } /// Build a charge transaction from challenge parameters and additional client options. @@ -72,15 +108,31 @@ pub async fn build_charge_transaction_with_options( .parse() .map_err(|_| Error::Other(format!("Invalid amount: {amount}")))?; + // Audit #10: client-side policy gates. Run before any signing work so + // we never produce a signature for an out-of-policy challenge. + if let Some(cap) = options.max_amount_base_units { + if total_amount > cap { + return Err(Error::Other(format!( + "Challenge amount {total_amount} exceeds client max_amount_base_units {cap}" + ))); + } + } + if let Some(expected) = options.expected_network.as_deref() { + let actual = method_details.network.as_deref().unwrap_or(""); + if actual != expected { + return Err(Error::Other(format!( + "Challenge network `{actual}` does not match client expected_network `{expected}`" + ))); + } + } + let splits = method_details.splits.as_deref().unwrap_or(&[]); - if splits.len() > 8 { + if splits.len() > crate::protocol::solana::MAX_SPLITS { return Err(Error::TooManySplits); } - let splits_total: u64 = splits - .iter() - .filter_map(|s| s.amount.parse::().ok()) - .sum(); + let splits_total = crate::protocol::solana::checked_sum_split_amounts(splits) + .ok_or(Error::SplitsExceedAmount)?; let primary_amount = total_amount .checked_sub(splits_total) .ok_or(Error::SplitsExceedAmount)?; @@ -139,6 +191,7 @@ pub async fn build_charge_transaction_with_options( options.external_id.as_deref(), splits, fee_payer_pubkey.as_ref(), + options.allow_unknown_token_2022, )?; } else { build_sol_instructions( @@ -155,7 +208,14 @@ pub async fn build_charge_transaction_with_options( let blockhash = if let Some(bh) = &method_details.recent_blockhash { Hash::from_str(bh).map_err(|e| Error::Other(format!("Invalid blockhash: {e}")))? } else { - rpc.get_latest_blockhash() + // Audit #36: ask for `confirmed` explicitly instead of leaning on + // the RPC client's default commitment. Solana's client guidance + // recommends `confirmed` for blockhash fetches — a `processed` + // hash can disappear under reorgs and produce signed transactions + // that fail with BlockhashNotFound after broadcast. + use solana_commitment_config::CommitmentConfig; + rpc.get_latest_blockhash_with_commitment(CommitmentConfig::confirmed()) + .map(|(hash, _last_valid_block_height)| hash) .map_err(|e| Error::Rpc(e.to_string()))? }; @@ -194,6 +254,43 @@ pub async fn build_credential_header( rpc: &RpcClient, challenge: &PaymentChallenge, ) -> Result { + build_credential_header_with_options(signer, rpc, challenge, Default::default()).await +} + +/// Like `build_credential_header`, but lets the caller pass +/// `BuildChargeTransactionOptions` — in particular +/// `allow_unknown_token_2022` to opt into signing for unknown Token-2022 +/// mints (see that field's docs). +pub async fn build_credential_header_with_options( + signer: &dyn SolanaSigner, + rpc: &RpcClient, + challenge: &PaymentChallenge, + mut options: BuildChargeTransactionOptions, +) -> Result { + // Audit #17: refuse to sign anything but a `solana`/`charge` + // challenge. The lower-level `build_charge_transaction_with_options` + // takes already-decoded fields and has no notion of method/intent, + // so the trust boundary belongs at this `PaymentChallenge` entry + // point. `select_charge_challenge` already filters via the same + // helper; this gate catches callers who skip it. + if !is_solana_charge_challenge_name(challenge) { + return Err(Error::Other(format!( + "Refusing to sign: challenge is not a solana/charge challenge \ + (method=`{}`, intent=`{}`)", + challenge.method.as_str(), + challenge.intent.as_str(), + ))); + } + + // Audit #10: refuse to sign expired challenges. The protocol allows + // `expires` to be absent — when it is, we let the challenge through + // (no client-side anchor to check against). + if challenge.is_expired() { + return Err(Error::Other( + "Challenge has expired; refusing to sign".into(), + )); + } + // Decode the request to get Solana-specific fields. let request: crate::protocol::intents::ChargeRequest = challenge .request @@ -213,6 +310,12 @@ pub async fn build_credential_header( .as_deref() .ok_or_else(|| Error::Other("No recipient in challenge".into()))?; + // Default external_id to the challenge's value if the caller didn't + // override it (preserves prior build_credential_header behavior). + if options.external_id.is_none() { + options.external_id = request.external_id.clone(); + } + let payload = build_charge_transaction_with_options( signer, rpc, @@ -220,9 +323,7 @@ pub async fn build_credential_header( &request.currency, recipient, &method_details, - BuildChargeTransactionOptions { - external_id: request.external_id.clone(), - }, + options, ) .await?; @@ -260,6 +361,12 @@ pub fn select_charge_challenge<'a>( continue; } + if !options.allow_unknown_token_2022 + && challenge_is_unknown_token_2022(&request, &method_details) + { + continue; + } + candidates.push((challenge, request, method_details)); } @@ -282,6 +389,34 @@ pub fn select_charge_challenge<'a>( Ok(None) } +/// Returns true if the challenge's currency is an arbitrary mint address +/// (not a recognized stablecoin) AND we cannot confirm its token program +/// is the vanilla Token Program. In both the explicit Token-2022 case and +/// the "no `tokenProgram` hint" case we fail closed — see +/// `BuildChargeTransactionOptions::allow_unknown_token_2022`. +fn challenge_is_unknown_token_2022( + request: &ChargeRequest, + method_details: &MethodDetails, +) -> bool { + if request.currency.eq_ignore_ascii_case("SOL") { + return false; + } + let mint = match crate::protocol::solana::resolve_stablecoin_mint( + &request.currency, + method_details.network.as_deref(), + ) { + Some(m) => m, + None => return false, + }; + if crate::protocol::solana::is_known_stablecoin_mint(mint) { + return false; + } + // Arbitrary mint. Vanilla Token Program is hookless, so accept it; for + // anything else (Token-2022 or unspecified) we cannot tell that + // signing is safe. + !matches!(method_details.token_program.as_deref(), Some(p) if p == programs::TOKEN_PROGRAM) +} + /// Returns true when a challenge is a schema-valid Solana charge challenge. pub fn is_solana_charge_challenge(challenge: &PaymentChallenge) -> bool { is_solana_charge_challenge_name(challenge) && decode_charge_challenge(challenge).is_ok() @@ -358,10 +493,33 @@ fn build_spl_instructions( external_id: Option<&str>, splits: &[Split], fee_payer: Option<&Pubkey>, + allow_unknown_token_2022: bool, ) -> Result<(), Error> { let mint = Pubkey::from_str(spl).map_err(|e| Error::Other(format!("Invalid mint: {e}")))?; let token_program = resolve_token_program(rpc, &mint, method_details)?; - let decimals = method_details.decimals.unwrap_or(6); + + // Spec §13.3: refuse to sign for an arbitrary Token-2022 mint unless + // the caller opted in. Transfer hooks run on every transfer and can + // execute arbitrary program code; the server's pre-broadcast checks + // do not simulate inner instructions in pull mode. The vanilla Token + // Program has no hooks, so unknown mints there are allowed. + if token_program.to_string() == programs::TOKEN_2022_PROGRAM + && !crate::protocol::solana::is_known_stablecoin_mint(spl) + && !allow_unknown_token_2022 + { + return Err(Error::Other(format!( + "Refusing to sign for unknown Token-2022 mint {spl}: \ + set BuildChargeTransactionOptions::allow_unknown_token_2022 \ + to opt in (Token-2022 supports transfer hooks)" + ))); + } + + // Audit #42: spec §7.2 requires `decimals` to be present when `currency` + // is a mint address (i.e. always on this SPL path). Defaulting to 6 + // silently produced wrong-decimals transfers for non-6-decimal tokens. + let decimals = method_details.decimals.ok_or_else(|| { + Error::Other("methodDetails.decimals is required for SPL charges (spec §7.2)".into()) + })?; let source_ata = get_associated_token_address(signer_pubkey, &mint, &token_program); @@ -406,11 +564,17 @@ fn build_spl_instructions( .amount .parse() .map_err(|_| Error::Other(format!("Invalid split amount: {}", split.amount)))?; + // Audit #20: only create the split ATA when the challenge + // explicitly flags it. The old behaviour auto-created in + // client-paid mode, letting a hostile server drain the client + // with N dust splits × ATA rent. Spec §7.2 says the client MUST + // include the ATA-create ix when `ataCreationRequired` is true; + // it does not authorize creation otherwise. add_spl_transfer( instructions, &split_recipient, split_amount, - fee_payer.is_none() || split.ata_creation_required == Some(true), + split.ata_creation_required == Some(true), )?; push_memo_instruction(instructions, split.memo.as_deref())?; } @@ -524,7 +688,13 @@ fn transfer_checked_ix( /// Resolve a currency to an optional mint address. /// -/// Returns `None` for native SOL, or `Some(mint_address)` for SPL tokens. +/// Returns `None` for native SOL. +/// Returns `Some(mint_address)` for known stablecoin symbols (e.g. +/// `"USDC"` → the network's USDC mint). +/// Returns `Some(currency)` (passthrough) for anything else — symbol or +/// mint string we don't recognize. Callers handling arbitrary mints MUST +/// validate parseability separately; audit #27 calls out the docstring +/// drift from "Some(mint_address)" alone. fn resolve_mint<'a>(currency: &'a str, network: Option<&str>) -> Option<&'a str> { crate::protocol::solana::resolve_stablecoin_mint(currency, network) } @@ -554,9 +724,17 @@ fn decode_charge_challenge( } fn matches_network(method_details: &MethodDetails, network: Option<&str>) -> bool { + // Audit #37: when methodDetails omits `network`, spec §7.2 says it + // defaults to `mainnet` — not the legacy "mainnet-beta" RPC hostname. match network { None => true, - Some(expected) => method_details.network.as_deref().unwrap_or("mainnet") == expected, + Some(expected) => { + method_details + .network + .as_deref() + .unwrap_or(crate::protocol::solana::DEFAULT_NETWORK) + == expected + } } } @@ -719,6 +897,106 @@ mod tests { assert!(selected.is_none()); } + fn unknown_token_2022_selection_challenge(token_program: Option<&str>) -> PaymentChallenge { + // A made-up base58 mint address that is NOT in the known stablecoin + // allowlist. Used to exercise the Token-2022 gate. + // Valid base58 pubkey not in the stablecoin allowlist. + const UNKNOWN_MINT: &str = "9XHRopERTd4LfQ8b6e3p9bN2WhxgQzDxFRtbq1XwQ4mP"; + let details = MethodDetails { + decimals: Some(6), + network: Some("mainnet".to_string()), + token_program: token_program.map(|s| s.to_string()), + ..Default::default() + }; + let request = ChargeRequest { + amount: "1000".to_string(), + currency: UNKNOWN_MINT.to_string(), + method_details: Some(serde_json::to_value(details).unwrap()), + recipient: Some(RECIPIENT.to_string()), + ..Default::default() + }; + PaymentChallenge::new( + "unknown-2022", + "test", + "solana", + "charge", + Base64UrlJson::from_typed(&request).unwrap(), + ) + } + + #[test] + fn select_charge_challenge_skips_unknown_token_2022_by_default() { + let challenges = vec![unknown_token_2022_selection_challenge(Some( + programs::TOKEN_2022_PROGRAM, + ))]; + let selected = + select_charge_challenge(&challenges, SelectChargeChallengeOptions::default()).unwrap(); + assert!(selected.is_none(), "default must skip unknown Token-2022"); + } + + #[test] + fn select_charge_challenge_skips_unknown_mint_with_no_token_program_hint() { + // No tokenProgram in methodDetails — we cannot prove it isn't + // Token-2022, so default must fail closed. + let challenges = vec![unknown_token_2022_selection_challenge(None)]; + let selected = + select_charge_challenge(&challenges, SelectChargeChallengeOptions::default()).unwrap(); + assert!(selected.is_none()); + } + + #[test] + fn select_charge_challenge_accepts_unknown_vanilla_token_mint() { + // Same unknown mint but explicitly on the vanilla Token Program — + // no transfer hooks, so the gate does not apply. + let challenges = vec![unknown_token_2022_selection_challenge(Some( + programs::TOKEN_PROGRAM, + ))]; + let selected = + select_charge_challenge(&challenges, SelectChargeChallengeOptions::default()) + .unwrap() + .unwrap(); + assert_eq!(selected.id, "unknown-2022"); + } + + #[test] + fn select_charge_challenge_allows_unknown_token_2022_with_opt_in() { + let challenges = vec![unknown_token_2022_selection_challenge(Some( + programs::TOKEN_2022_PROGRAM, + ))]; + let selected = select_charge_challenge( + &challenges, + SelectChargeChallengeOptions { + allow_unknown_token_2022: true, + ..Default::default() + }, + ) + .unwrap() + .unwrap(); + assert_eq!(selected.id, "unknown-2022"); + } + + #[test] + fn select_charge_challenge_does_not_gate_known_token_2022_stablecoin() { + // PYUSD is Token-2022 but in the known allowlist; default selection + // must still pick it. + let challenges = vec![selection_challenge( + "pyusd-mainnet", + "solana", + mints::PYUSD_MAINNET, + "mainnet", + )]; + let selected = select_charge_challenge( + &challenges, + SelectChargeChallengeOptions { + network: Some("mainnet"), + ..Default::default() + }, + ) + .unwrap() + .unwrap(); + assert_eq!(selected.id, "pyusd-mainnet"); + } + #[test] fn is_solana_charge_challenge_rejects_invalid_request() { let challenge = PaymentChallenge::new( @@ -1499,11 +1777,46 @@ mod tests { None, &[], None, + false, ) .unwrap(); assert_eq!(ixs.len(), 1); } + #[test] + fn build_spl_rejects_missing_decimals() { + // Audit #42: spec §7.2 requires `decimals` for SPL challenges; + // we now error instead of silently defaulting to 6. + let signer_pk = Pubkey::new_unique(); + let recipient = Pubkey::from_str(RECIPIENT).unwrap(); + let rpc = dummy_rpc(); + let md = MethodDetails { + token_program: Some(programs::TOKEN_PROGRAM.to_string()), + decimals: None, // missing — must reject + ..Default::default() + }; + let mut ixs = vec![]; + let err = build_spl_instructions( + &mut ixs, + &signer_pk, + &recipient, + &rpc, + USDC_MINT, + &md, + 1_000_000, + None, + &[], + None, + false, + ) + .err() + .expect("missing decimals should be rejected"); + assert!( + format!("{err}").contains("decimals is required"), + "got: {err}" + ); + } + #[test] fn build_spl_with_external_id_memo() { let signer_pk = Pubkey::new_unique(); @@ -1526,6 +1839,7 @@ mod tests { Some("order-123"), &[], None, + false, ) .unwrap(); @@ -1561,6 +1875,7 @@ mod tests { None, &[], Some(&fee_payer), + false, ) .unwrap(); assert_eq!(ixs.len(), 1); @@ -1568,6 +1883,8 @@ mod tests { #[test] fn build_spl_with_splits() { + // Audit #20: with ata_creation_required=None the client must NOT + // emit the ATA-create instruction (even in client-paid mode). let signer_pk = Pubkey::new_unique(); let recipient = Pubkey::from_str(RECIPIENT).unwrap(); let split_recipient = Pubkey::new_unique(); @@ -1587,9 +1904,40 @@ mod tests { let mut ixs = vec![]; build_spl_instructions( &mut ixs, &signer_pk, &recipient, &rpc, USDC_MINT, &md, 1_000_000, None, &splits, None, + false, + ) + .unwrap(); + // 1 primary transfer + 1 split transfer. No split ATA create. + assert_eq!(ixs.len(), 2); + } + + #[test] + fn build_spl_creates_split_ata_only_when_flagged() { + // Audit #20: ata_creation_required = Some(true) means the client + // MUST include the ATA-create ix. + let signer_pk = Pubkey::new_unique(); + let recipient = Pubkey::from_str(RECIPIENT).unwrap(); + let split_recipient = Pubkey::new_unique(); + let rpc = dummy_rpc(); + let md = MethodDetails { + token_program: Some(programs::TOKEN_PROGRAM.to_string()), + decimals: Some(6), + ..Default::default() + }; + let splits = vec![Split { + recipient: split_recipient.to_string(), + amount: "1000".to_string(), + ata_creation_required: Some(true), + label: None, + memo: None, + }]; + let mut ixs = vec![]; + build_spl_instructions( + &mut ixs, &signer_pk, &recipient, &rpc, USDC_MINT, &md, 1_000_000, None, &splits, None, + false, ) .unwrap(); - // Primary recipient ATA creation is out of scope; split ATA creation is allowed. + // 1 primary transfer + 1 ATA create + 1 split transfer. assert_eq!(ixs.len(), 3); } @@ -1614,16 +1962,18 @@ mod tests { let mut ixs = vec![]; build_spl_instructions( &mut ixs, &signer_pk, &recipient, &rpc, USDC_MINT, &md, 1_000_000, None, &splits, None, + false, ) .unwrap(); - assert_eq!(ixs.len(), 4); + // 1 primary transfer + 1 split transfer + 1 split memo (no ATA create). + assert_eq!(ixs.len(), 3); assert_eq!( - ixs[3].program_id, + ixs[2].program_id, Pubkey::from_str(programs::MEMO_PROGRAM).unwrap() ); - assert!(ixs[3].accounts.is_empty()); - assert_eq!(ixs[3].data, b"platform fee"); + assert!(ixs[2].accounts.is_empty()); + assert_eq!(ixs[2].data, b"platform fee"); } #[test] @@ -1647,6 +1997,7 @@ mod tests { let mut ixs = vec![]; let err = build_spl_instructions( &mut ixs, &signer_pk, &recipient, &rpc, USDC_MINT, &md, 1_000_000, None, &splits, None, + false, ) .unwrap_err(); @@ -1686,6 +2037,7 @@ mod tests { None, &splits, Some(&fee_payer), + false, ) .unwrap(); @@ -1727,11 +2079,142 @@ mod tests { None, &splits, Some(&fee_payer), + false, ) .unwrap(); assert_eq!(ixs.len(), 2); } + #[test] + fn build_spl_refuses_unknown_token_2022_without_opt_in() { + // A made-up base58 mint NOT in the known stablecoin allowlist, + // explicitly placed on Token-2022. Default (allow_unknown_token_2022 + // = false) must refuse. + // Valid base58 pubkey not in the stablecoin allowlist. + const UNKNOWN_MINT: &str = "9XHRopERTd4LfQ8b6e3p9bN2WhxgQzDxFRtbq1XwQ4mP"; + let signer_pk = Pubkey::new_unique(); + let recipient = Pubkey::from_str(RECIPIENT).unwrap(); + let rpc = dummy_rpc(); + let md = MethodDetails { + token_program: Some(programs::TOKEN_2022_PROGRAM.to_string()), + decimals: Some(6), + ..Default::default() + }; + let mut ixs = vec![]; + let err = build_spl_instructions( + &mut ixs, + &signer_pk, + &recipient, + &rpc, + UNKNOWN_MINT, + &md, + 1_000_000, + None, + &[], + None, + false, + ); + let err = err.expect_err("should refuse unknown Token-2022 mint"); + assert!( + format!("{err}").contains("unknown Token-2022 mint"), + "got: {err}" + ); + } + + #[test] + fn build_spl_allows_unknown_token_2022_with_opt_in() { + // Same setup as above but with the opt-in flag set — gate passes + // and the function builds successfully. + // Valid base58 pubkey not in the stablecoin allowlist. + const UNKNOWN_MINT: &str = "9XHRopERTd4LfQ8b6e3p9bN2WhxgQzDxFRtbq1XwQ4mP"; + let signer_pk = Pubkey::new_unique(); + let recipient = Pubkey::from_str(RECIPIENT).unwrap(); + let rpc = dummy_rpc(); + let md = MethodDetails { + token_program: Some(programs::TOKEN_2022_PROGRAM.to_string()), + decimals: Some(6), + ..Default::default() + }; + let mut ixs = vec![]; + build_spl_instructions( + &mut ixs, + &signer_pk, + &recipient, + &rpc, + UNKNOWN_MINT, + &md, + 1_000_000, + None, + &[], + None, + true, + ) + .unwrap(); + assert!(!ixs.is_empty()); + } + + #[test] + fn build_spl_allows_unknown_vanilla_token_mint() { + // Arbitrary mint on the vanilla Token Program (no transfer hooks) + // — gate does not apply. + // Valid base58 pubkey not in the stablecoin allowlist. + const UNKNOWN_MINT: &str = "9XHRopERTd4LfQ8b6e3p9bN2WhxgQzDxFRtbq1XwQ4mP"; + let signer_pk = Pubkey::new_unique(); + let recipient = Pubkey::from_str(RECIPIENT).unwrap(); + let rpc = dummy_rpc(); + let md = MethodDetails { + token_program: Some(programs::TOKEN_PROGRAM.to_string()), + decimals: Some(6), + ..Default::default() + }; + let mut ixs = vec![]; + build_spl_instructions( + &mut ixs, + &signer_pk, + &recipient, + &rpc, + UNKNOWN_MINT, + &md, + 1_000_000, + None, + &[], + None, + false, + ) + .unwrap(); + assert!(!ixs.is_empty()); + } + + #[test] + fn build_spl_does_not_gate_known_token_2022_stablecoin() { + // PYUSD is Token-2022 but in the known allowlist — gate must not + // fire even with allow_unknown_token_2022 = false. + let signer_pk = Pubkey::new_unique(); + let recipient = Pubkey::from_str(RECIPIENT).unwrap(); + let rpc = dummy_rpc(); + let md = MethodDetails { + token_program: Some(programs::TOKEN_2022_PROGRAM.to_string()), + decimals: Some(6), + ..Default::default() + }; + let mut ixs = vec![]; + build_spl_instructions( + &mut ixs, + &signer_pk, + &recipient, + &rpc, + mints::PYUSD_MAINNET, + &md, + 1_000_000, + None, + &[], + None, + false, + ) + .unwrap(); + assert!(!ixs.is_empty()); + } + #[test] fn build_spl_invalid_mint() { let signer_pk = Pubkey::new_unique(); @@ -1753,6 +2236,7 @@ mod tests { None, &[], None, + false, ); assert!(err.is_err()); assert!(format!("{}", err.unwrap_err()).contains("Invalid mint")); @@ -1778,6 +2262,7 @@ mod tests { let mut ixs = vec![]; let err = build_spl_instructions( &mut ixs, &signer_pk, &recipient, &rpc, USDC_MINT, &md, 1_000_000, None, &splits, None, + false, ); assert!(err.is_err()); assert!(format!("{}", err.unwrap_err()).contains("Invalid split recipient")); @@ -1804,6 +2289,7 @@ mod tests { let mut ixs = vec![]; let err = build_spl_instructions( &mut ixs, &signer_pk, &recipient, &rpc, USDC_MINT, &md, 1_000_000, None, &splits, None, + false, ); assert!(err.is_err()); assert!(format!("{}", err.unwrap_err()).contains("Invalid split amount")); @@ -1870,6 +2356,258 @@ mod tests { assert!(format!("{}", err.unwrap_err()).contains("No recipient")); } + // ── Audit #10: client-side policy gates ── + + #[tokio::test] + async fn build_charge_transaction_rejects_amount_above_max() { + let signer = make_signer(); + let rpc = dummy_rpc(); + let md = MethodDetails { + recent_blockhash: Some(ZERO_HASH.to_string()), + ..Default::default() + }; + let err = build_charge_transaction_with_options( + signer.as_ref(), + &rpc, + "5000000", + "SOL", + RECIPIENT, + &md, + BuildChargeTransactionOptions { + max_amount_base_units: Some(1_000_000), + ..Default::default() + }, + ) + .await + .err() + .expect("amount above cap should be rejected"); + let msg = format!("{err}"); + assert!( + msg.contains("exceeds client max_amount_base_units"), + "unexpected error: {msg}" + ); + } + + #[tokio::test] + async fn build_charge_transaction_accepts_amount_at_max() { + let signer = make_signer(); + let rpc = dummy_rpc(); + let md = MethodDetails { + recent_blockhash: Some(ZERO_HASH.to_string()), + ..Default::default() + }; + build_charge_transaction_with_options( + signer.as_ref(), + &rpc, + "1000000", + "SOL", + RECIPIENT, + &md, + BuildChargeTransactionOptions { + max_amount_base_units: Some(1_000_000), + ..Default::default() + }, + ) + .await + .expect("amount equal to cap should be allowed"); + } + + #[tokio::test] + async fn build_charge_transaction_rejects_unexpected_network() { + let signer = make_signer(); + let rpc = dummy_rpc(); + let md = MethodDetails { + network: Some("mainnet".to_string()), + recent_blockhash: Some(ZERO_HASH.to_string()), + ..Default::default() + }; + let err = build_charge_transaction_with_options( + signer.as_ref(), + &rpc, + "1000000", + "SOL", + RECIPIENT, + &md, + BuildChargeTransactionOptions { + expected_network: Some("devnet".to_string()), + ..Default::default() + }, + ) + .await + .err() + .expect("network mismatch should be rejected"); + let msg = format!("{err}"); + assert!( + msg.contains("does not match client expected_network"), + "unexpected error: {msg}" + ); + } + + #[tokio::test] + async fn build_charge_transaction_accepts_matching_network() { + let signer = make_signer(); + let rpc = dummy_rpc(); + let md = MethodDetails { + network: Some("devnet".to_string()), + recent_blockhash: Some(ZERO_HASH.to_string()), + ..Default::default() + }; + build_charge_transaction_with_options( + signer.as_ref(), + &rpc, + "1000000", + "SOL", + RECIPIENT, + &md, + BuildChargeTransactionOptions { + expected_network: Some("devnet".to_string()), + ..Default::default() + }, + ) + .await + .expect("matching network should be allowed"); + } + + #[tokio::test] + async fn build_credential_header_rejects_expired_challenge() { + use crate::protocol::core::Base64UrlJson; + use crate::protocol::intents::ChargeRequest; + + let signer = make_signer(); + let rpc = dummy_rpc(); + let request = ChargeRequest { + amount: "1000000".to_string(), + currency: "SOL".to_string(), + recipient: Some(RECIPIENT.to_string()), + method_details: Some( + serde_json::to_value(MethodDetails { + recent_blockhash: Some(ZERO_HASH.to_string()), + ..Default::default() + }) + .unwrap(), + ), + ..Default::default() + }; + let request_b64 = Base64UrlJson::from_typed(&request).unwrap(); + let mut challenge = + PaymentChallenge::new("test-id", "test-realm", "solana", "charge", request_b64); + // RFC3339 timestamp in the distant past. + challenge.expires = Some("1970-01-01T00:00:00Z".to_string()); + + let err = build_credential_header(signer.as_ref(), &rpc, &challenge) + .await + .err() + .expect("expired challenge should be rejected"); + assert!( + format!("{err}").contains("expired"), + "unexpected error: {err}" + ); + } + + #[tokio::test] + async fn build_credential_header_accepts_future_expiry() { + use crate::protocol::core::Base64UrlJson; + use crate::protocol::intents::ChargeRequest; + + let signer = make_signer(); + let rpc = dummy_rpc(); + let request = ChargeRequest { + amount: "1000000".to_string(), + currency: "SOL".to_string(), + recipient: Some(RECIPIENT.to_string()), + method_details: Some( + serde_json::to_value(MethodDetails { + recent_blockhash: Some(ZERO_HASH.to_string()), + ..Default::default() + }) + .unwrap(), + ), + ..Default::default() + }; + let request_b64 = Base64UrlJson::from_typed(&request).unwrap(); + let mut challenge = + PaymentChallenge::new("test-id", "test-realm", "solana", "charge", request_b64); + challenge.expires = Some("2999-01-01T00:00:00Z".to_string()); + + build_credential_header(signer.as_ref(), &rpc, &challenge) + .await + .expect("future expiry should be accepted"); + } + + // ── Audit #17: method/intent gate on the credential builder ── + + #[tokio::test] + async fn build_credential_header_rejects_non_solana_method() { + use crate::protocol::core::Base64UrlJson; + use crate::protocol::intents::ChargeRequest; + + let signer = make_signer(); + let rpc = dummy_rpc(); + let request = ChargeRequest { + amount: "1000000".to_string(), + currency: "SOL".to_string(), + recipient: Some(RECIPIENT.to_string()), + method_details: Some( + serde_json::to_value(MethodDetails { + recent_blockhash: Some(ZERO_HASH.to_string()), + ..Default::default() + }) + .unwrap(), + ), + ..Default::default() + }; + let request_b64 = Base64UrlJson::from_typed(&request).unwrap(); + // Wrong method — would otherwise be accepted by the builder. + let challenge = + PaymentChallenge::new("test-id", "test-realm", "stripe", "charge", request_b64); + + let err = build_credential_header(signer.as_ref(), &rpc, &challenge) + .await + .err() + .expect("non-solana method should be rejected"); + let msg = format!("{err}"); + assert!( + msg.contains("not a solana/charge challenge") && msg.contains("method=`stripe`"), + "unexpected error: {msg}" + ); + } + + #[tokio::test] + async fn build_credential_header_rejects_non_charge_intent() { + use crate::protocol::core::Base64UrlJson; + use crate::protocol::intents::ChargeRequest; + + let signer = make_signer(); + let rpc = dummy_rpc(); + let request = ChargeRequest { + amount: "1000000".to_string(), + currency: "SOL".to_string(), + recipient: Some(RECIPIENT.to_string()), + method_details: Some( + serde_json::to_value(MethodDetails { + recent_blockhash: Some(ZERO_HASH.to_string()), + ..Default::default() + }) + .unwrap(), + ), + ..Default::default() + }; + let request_b64 = Base64UrlJson::from_typed(&request).unwrap(); + // Wrong intent. + let challenge = + PaymentChallenge::new("test-id", "test-realm", "solana", "session", request_b64); + + let err = build_credential_header(signer.as_ref(), &rpc, &challenge) + .await + .err() + .expect("non-charge intent should be rejected"); + let msg = format!("{err}"); + assert!( + msg.contains("not a solana/charge challenge") && msg.contains("intent=`session`"), + "unexpected error: {msg}" + ); + } + #[tokio::test] async fn build_credential_header_invalid_method_details() { use crate::protocol::core::Base64UrlJson; diff --git a/rust/crates/mpp/src/error.rs b/rust/crates/mpp/src/error.rs index 69881fe3..c1e061f4 100644 --- a/rust/crates/mpp/src/error.rs +++ b/rust/crates/mpp/src/error.rs @@ -43,7 +43,10 @@ pub enum Error { #[error("Splits consume the entire amount")] SplitsExceedAmount, - #[error("Splits exceed maximum of 8 entries")] + #[error( + "Splits exceed maximum of {} entries", + crate::protocol::solana::MAX_SPLITS + )] TooManySplits, #[error("Invalid configuration: {0}")] diff --git a/rust/crates/mpp/src/lib.rs b/rust/crates/mpp/src/lib.rs index 18638b7a..f50eaabd 100644 --- a/rust/crates/mpp/src/lib.rs +++ b/rust/crates/mpp/src/lib.rs @@ -25,9 +25,16 @@ //! let challenge = mpp.charge("0.10")?; //! let header = challenge.to_header()?; //! -//! // Verify a credential from Authorization header +//! // Verify a credential from Authorization header. The expected +//! // ChargeRequest pins this route's amount/currency/recipient (audit #2). //! let credential = PaymentCredential::from_header(&auth_header)?; -//! let receipt = mpp.verify_credential(&credential).await?; +//! let expected = ChargeRequest { +//! amount: "100000".to_string(), +//! currency: "USDC".to_string(), +//! recipient: Some("CXhrFZJLKqjzmP3sjYLcF4dTeXWKCy9e2SXXZ2Yo6MPY".to_string()), +//! ..Default::default() +//! }; +//! let receipt = mpp.verify_credential_with_expected(&credential, &expected).await?; //! ``` pub mod error; diff --git a/rust/crates/mpp/src/protocol/core/challenge.rs b/rust/crates/mpp/src/protocol/core/challenge.rs index 1378fb76..2896387f 100644 --- a/rust/crates/mpp/src/protocol/core/challenge.rs +++ b/rust/crates/mpp/src/protocol/core/challenge.rs @@ -224,7 +224,12 @@ pub fn compute_challenge_id( } /// Constant-time string comparison to prevent timing attacks. -fn constant_time_eq(a: &str, b: &str) -> bool { +/// +/// Audit #41 made this `pub(crate)` so the server's `verify` (in +/// `server/charge.rs`) can use the same constant-time path that +/// `PaymentChallenge::verify` uses, rather than a regular `!=` that +/// short-circuits at the first byte mismatch. +pub(crate) fn constant_time_eq(a: &str, b: &str) -> bool { if a.len() != b.len() { return false; } diff --git a/rust/crates/mpp/src/protocol/core/headers.rs b/rust/crates/mpp/src/protocol/core/headers.rs index 514af690..3bf99ffa 100644 --- a/rust/crates/mpp/src/protocol/core/headers.rs +++ b/rust/crates/mpp/src/protocol/core/headers.rs @@ -50,6 +50,17 @@ pub fn parse_www_authenticate(header: &str) -> Result { let intent = IntentName::new(require_param(¶ms, "intent")?); let request_b64 = require_param(¶ms, "request")?.clone(); + // Audit #9: cap the request parameter before any base64 / JSON work, + // matching the cap already enforced by parse_authorization and + // parse_receipt. Without it, a large `request=` value can drive + // proportionally larger decode + parse work than the other parsers + // allow. + if request_b64.len() > MAX_TOKEN_LEN { + return Err(Error::Other(format!( + "Challenge request parameter exceeds maximum length of {MAX_TOKEN_LEN} bytes" + ))); + } + let request_bytes = base64url_decode(&request_b64)?; let _ = serde_json::from_slice::(&request_bytes) .map_err(|e| Error::Other(format!("Invalid JSON in request field: {e}")))?; @@ -590,6 +601,42 @@ mod tests { assert!(format!("{}", err.unwrap_err()).contains("Missing 'realm'")); } + #[test] + fn parse_www_authenticate_rejects_oversized_request_param() { + // One byte past MAX_TOKEN_LEN. Contents don't need to be valid + // base64 — the size check fires first. + let oversized = "A".repeat(MAX_TOKEN_LEN + 1); + let header = format!( + r#"Payment id="x", realm="api", method="solana", intent="charge", request="{oversized}""# + ); + let err = parse_www_authenticate(&header) + .err() + .expect("oversized request should be rejected"); + assert!( + format!("{err}").contains("exceeds maximum length"), + "got: {err}" + ); + } + + #[test] + fn parse_www_authenticate_accepts_at_max_request_size() { + // Exactly MAX_TOKEN_LEN bytes. The size gate must pass; the + // payload itself isn't valid base64+JSON so we expect a later + // failure (decode or JSON), but NOT the size error. + let at_max = "A".repeat(MAX_TOKEN_LEN); + let header = format!( + r#"Payment id="x", realm="api", method="solana", intent="charge", request="{at_max}""# + ); + let err = parse_www_authenticate(&header) + .err() + .expect("invalid payload still errors"); + let msg = format!("{err}"); + assert!( + !msg.contains("exceeds maximum length"), + "size gate should not have fired at exactly MAX_TOKEN_LEN: {msg}" + ); + } + #[test] fn parse_rejects_invalid_json_in_request() { // base64url of "not json" diff --git a/rust/crates/mpp/src/protocol/intents/mod.rs b/rust/crates/mpp/src/protocol/intents/mod.rs index b8649841..bcbd87b4 100644 --- a/rust/crates/mpp/src/protocol/intents/mod.rs +++ b/rust/crates/mpp/src/protocol/intents/mod.rs @@ -22,14 +22,62 @@ pub use subscription::{ SubscriptionReceiptExtensions, SubscriptionRequest, }; +/// Audit #39: upper bound on the `decimals` argument to `parse_units`. +/// +/// Solana's SPL convention is 0–9 (the protocol spec says so). 18 gives +/// ERC-20-style headroom while staying well below the cliff at 39 where +/// `10u128.pow(decimals)` actually overflows. The point of the cap is to +/// give us a single rejection site so any callsite that hasn't validated +/// `decimals` upstream gets a clear error rather than a panic or wrap. +pub const MAX_DECIMALS: u8 = 18; + /// Convert a human-readable amount to base units. /// /// Matches the TypeScript SDK's `parseUnits(amount, decimals)`. /// e.g., `parse_units("1.5", 6)` → `"1500000"`. +/// +/// Audit #39: rejects `decimals > MAX_DECIMALS` and uses checked +/// arithmetic in the integer branch so a hostile or buggy caller cannot +/// trigger a panic (debug) or silent overflow (release). +/// +/// Audits #44 and #45: validate input shape and content. +/// - Reject empty amount and amounts with more than one `.` (e.g. +/// `"1.2.3"`) — `split_once('.')` only splits on the first dot, which +/// would otherwise let `"1.2.3"` parse as `"1" + "23"` and silently +/// produce the wrong value. +/// - Reject inputs that aren't strict ASCII digit strings on either side +/// of the dot — `"1a.2"`, `".5"`, `"5."`, `"."` all become errors. pub fn parse_units(amount: &str, decimals: u8) -> Result { + if decimals > MAX_DECIMALS { + return Err(crate::error::Error::Other(format!( + "Decimals {decimals} exceeds maximum {MAX_DECIMALS}" + ))); + } + if amount.is_empty() { + return Err(crate::error::Error::Other("Empty amount".into())); + } + if amount.matches('.').count() > 1 { + return Err(crate::error::Error::Other(format!( + "Invalid amount `{amount}`: more than one decimal point" + ))); + } let decimals = decimals as u32; if let Some((integer, fraction)) = amount.split_once('.') { + // Audit #44/#45: require non-empty digit strings on both sides + // of the dot. `".5"`, `"5."`, `"."`, `"1a.2"` all rejected. + if integer.is_empty() || fraction.is_empty() { + return Err(crate::error::Error::Other(format!( + "Invalid amount `{amount}`: integer and fractional parts must both be non-empty" + ))); + } + if !integer.bytes().all(|b| b.is_ascii_digit()) + || !fraction.bytes().all(|b| b.is_ascii_digit()) + { + return Err(crate::error::Error::Other(format!( + "Invalid amount `{amount}`: only ASCII digits and a single optional decimal point are allowed" + ))); + } let frac_len = fraction.len() as u32; if frac_len > decimals { return Err(crate::error::Error::Other(format!( @@ -50,8 +98,15 @@ pub fn parse_units(amount: &str, decimals: u8) -> Result Result<(), crate::error::Error> { + match network { + NETWORK_MAINNET | NETWORK_DEVNET | NETWORK_LOCALNET => Ok(()), + "" => Err(crate::error::Error::InvalidConfig( + "network must not be empty (one of `mainnet`, `devnet`, `localnet`)".into(), + )), + other => Err(crate::error::Error::InvalidConfig(format!( + "Unknown network `{other}` (allowed: `mainnet`, `devnet`, `localnet`)" + ))), + } +} + +/// Default RPC URLs per network. Inputs are expected to be canonical +/// slugs (see `validate_network`); unknown slugs fall through to the +/// mainnet RPC for backwards compatibility, but `validate_network` at +/// `Mpp::new` ensures servers can never reach the fallback path. pub fn default_rpc_url(network: &str) -> &'static str { match network { - "devnet" => "https://api.devnet.solana.com", - "testnet" => "https://api.testnet.solana.com", - "localnet" => "http://localhost:8899", + NETWORK_DEVNET => "https://api.devnet.solana.com", + NETWORK_LOCALNET => "http://localhost:8899", _ => "https://api.mainnet-beta.solana.com", } } @@ -78,7 +110,28 @@ fn stablecoin_uses_token_2022(mint: &str) -> bool { ) } +/// Whether `mint` is one of the well-known stablecoin mints whose token +/// program is hardcoded. Returning `false` for an arbitrary mint means +/// callers must do an on-chain mint-owner lookup to find the program. +pub fn is_known_stablecoin_mint(mint: &str) -> bool { + matches!( + mint, + mints::USDC_MAINNET + | mints::USDC_DEVNET + | mints::USDT_MAINNET + | mints::USDG_MAINNET + | mints::USDG_DEVNET + | mints::PYUSD_MAINNET + | mints::PYUSD_DEVNET + | mints::CASH_MAINNET + ) +} + /// Default token program for a currency or mint. +/// +/// Only valid for well-known stablecoins. Callers handling arbitrary mints +/// MUST resolve the token program via an on-chain mint-owner lookup +/// (spec §7.2) rather than relying on this fallback. pub fn default_token_program_for_currency(currency: &str, network: Option<&str>) -> &'static str { match resolve_stablecoin_mint(currency, network) { Some(mint) if stablecoin_uses_token_2022(mint) => programs::TOKEN_2022_PROGRAM, @@ -108,11 +161,6 @@ mod tests { ); } - #[test] - fn default_rpc_url_testnet() { - assert_eq!(default_rpc_url("testnet"), "https://api.testnet.solana.com"); - } - #[test] fn default_rpc_url_unknown_defaults_to_mainnet() { assert_eq!(default_rpc_url(""), "https://api.mainnet-beta.solana.com"); @@ -375,6 +423,150 @@ mod tests { let deserialized: Split = serde_json::from_str(&json).unwrap(); assert_eq!(deserialized.ata_creation_required, Some(true)); } + + // ── Audit #30: checked_sum_split_amounts ── + + fn split_with_amount(amt: &str) -> Split { + Split { + recipient: "R".to_string(), + amount: amt.to_string(), + ata_creation_required: None, + label: None, + memo: None, + } + } + + #[test] + fn checked_sum_split_amounts_within_u64_sums_correctly() { + let splits = [ + split_with_amount("100"), + split_with_amount("200"), + split_with_amount("3"), + ]; + assert_eq!(checked_sum_split_amounts(&splits), Some(303)); + } + + #[test] + fn checked_sum_split_amounts_overflows_returns_none() { + let near_max = (u64::MAX / 2) + 1; + let splits = [ + split_with_amount(&near_max.to_string()), + split_with_amount(&near_max.to_string()), + ]; + // Sum would be u64::MAX + 1 — must report overflow. + assert_eq!(checked_sum_split_amounts(&splits), None); + } + + #[test] + fn checked_sum_split_amounts_unparseable_treated_as_zero() { + // Strict parseability is audit #21's concern; here we just check + // that an unparseable amount doesn't break the arithmetic. + let splits = [ + split_with_amount("100"), + split_with_amount("not-a-number"), + split_with_amount("50"), + ]; + assert_eq!(checked_sum_split_amounts(&splits), Some(150)); + } + + #[test] + fn checked_sum_split_amounts_empty_is_zero() { + let splits: [Split; 0] = []; + assert_eq!(checked_sum_split_amounts(&splits), Some(0)); + } + + // ── Audit #21: validate_splits ── + + fn split(recipient: &str, amount: &str) -> Split { + Split { + recipient: recipient.to_string(), + amount: amount.to_string(), + ata_creation_required: None, + label: None, + memo: None, + } + } + + fn unique_pubkey() -> String { + solana_pubkey::Pubkey::new_unique().to_string() + } + + #[test] + fn validate_splits_accepts_valid_set() { + let splits = vec![ + split(&unique_pubkey(), "100"), + split(&unique_pubkey(), "200"), + split(&unique_pubkey(), "300"), + ]; + validate_splits(&splits).expect("valid splits should be accepted"); + } + + #[test] + fn validate_splits_accepts_empty() { + let splits: Vec = vec![]; + validate_splits(&splits).expect("empty list is allowed"); + } + + #[test] + fn validate_splits_rejects_count_above_max() { + let splits: Vec = (0..(MAX_SPLITS + 1)) + .map(|_| split(&unique_pubkey(), "1")) + .collect(); + let err = validate_splits(&splits).err().expect("too many splits"); + assert!(matches!(err, crate::error::Error::TooManySplits)); + } + + #[test] + fn validate_splits_rejects_invalid_recipient() { + let splits = vec![split("not-a-pubkey!!", "100")]; + let err = validate_splits(&splits).err().expect("bad recipient"); + assert!( + format!("{err}").contains("splits[0]: invalid recipient pubkey"), + "got: {err}" + ); + } + + #[test] + fn validate_splits_rejects_unparseable_amount() { + let splits = vec![split(&unique_pubkey(), "not-a-number")]; + let err = validate_splits(&splits).err().expect("bad amount"); + assert!( + format!("{err}").contains("is not a valid u64"), + "got: {err}" + ); + } + + #[test] + fn validate_splits_rejects_zero_amount() { + let splits = vec![split(&unique_pubkey(), "0")]; + let err = validate_splits(&splits).err().expect("zero amount"); + assert!( + format!("{err}").contains("amount must be greater than zero"), + "got: {err}" + ); + } + + #[test] + fn validate_splits_rejects_overflowing_aggregate() { + let near_max = (u64::MAX / 2) + 1; + let splits = vec![ + split(&unique_pubkey(), &near_max.to_string()), + split(&unique_pubkey(), &near_max.to_string()), + ]; + let err = validate_splits(&splits).err().expect("aggregate overflow"); + assert!(format!("{err}").contains("overflows u64"), "got: {err}"); + } + + #[test] + fn validate_splits_rejects_duplicate_recipient() { + let dup = unique_pubkey(); + let splits = vec![split(&dup, "100"), split(&dup, "200")]; + let err = validate_splits(&splits).err().expect("duplicate recipient"); + assert!( + format!("{err}").contains("duplicate recipient"), + "got: {err}" + ); + } } /// Solana-specific method details in the challenge request. @@ -398,7 +590,7 @@ pub struct MethodDetails { #[serde(skip_serializing_if = "Option::is_none")] pub fee_payer_key: Option, - /// Additional payment splits (max 8). + /// Additional payment splits (max `MAX_SPLITS`). #[serde(skip_serializing_if = "Option::is_none")] pub splits: Option>, @@ -428,6 +620,85 @@ pub struct Split { pub memo: Option, } +/// Maximum number of payment splits per challenge. +/// +/// Mirrors the upper bound enforced by the TS SDK and the wire-format +/// guidance from the MPP spec. Single source of truth for both client-side +/// (pre-build) and server-side (pre-broadcast) cap checks. +pub const MAX_SPLITS: usize = 8; + +/// Audit #21: validate a list of payment splits at challenge issuance. +/// +/// Single source of truth for both server entry points (`charge_with_options` +/// and `charge_challenge_with_options`). Without this gate, malformed +/// splits would otherwise only surface at the chain — too late for the +/// merchant to recover and bad UX for the payer. +/// +/// Checks (each callsite gets the same error shape): +/// 1. `splits.len() <= MAX_SPLITS`. +/// 2. Each `split.recipient` parses as a `Pubkey`. +/// 3. Each `split.amount` parses as `u64` AND is non-zero. +/// 4. The aggregate sum fits in `u64` (`checked_sum_split_amounts` is `Some`). +/// 5. No duplicate `recipient` across splits. +/// +/// Application-level recipient allowlists are out of scope — an SDK +/// shouldn't bake in domain-specific policy. +pub fn validate_splits(splits: &[Split]) -> Result<(), crate::error::Error> { + use crate::error::Error; + use std::collections::HashSet; + use std::str::FromStr; + + if splits.len() > MAX_SPLITS { + return Err(Error::TooManySplits); + } + + let mut seen_recipients: HashSet<&str> = HashSet::with_capacity(splits.len()); + for (idx, split) in splits.iter().enumerate() { + solana_pubkey::Pubkey::from_str(&split.recipient).map_err(|e| { + Error::InvalidConfig(format!("splits[{idx}]: invalid recipient pubkey: {e}")) + })?; + let amount = split.amount.parse::().map_err(|_| { + Error::InvalidConfig(format!( + "splits[{idx}]: amount `{}` is not a valid u64", + split.amount + )) + })?; + if amount == 0 { + return Err(Error::InvalidConfig(format!( + "splits[{idx}]: amount must be greater than zero" + ))); + } + if !seen_recipients.insert(split.recipient.as_str()) { + return Err(Error::InvalidConfig(format!( + "splits[{idx}]: duplicate recipient `{}`", + split.recipient + ))); + } + } + + if checked_sum_split_amounts(splits).is_none() { + return Err(Error::InvalidConfig( + "Sum of split amounts overflows u64".into(), + )); + } + + Ok(()) +} + +/// Audit #30: sum split amounts in base units with overflow detection. +/// +/// Returns `None` if the running total would overflow `u64`. Unparseable +/// `amount` strings are treated as 0 — strict parseability is audit #21's +/// concern; here we only address the *arithmetic* overflow shape so a +/// stuffed split list cannot panic (debug) or wrap (release) downstream +/// callers that derive the primary amount via `total - splits_total`. +pub fn checked_sum_split_amounts(splits: &[Split]) -> Option { + splits + .iter() + .map(|s| s.amount.parse::().unwrap_or(0)) + .try_fold(0u64, |acc, x| acc.checked_add(x)) +} + /// Credential payload — what the client sends in the Authorization header. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "camelCase")] diff --git a/rust/crates/mpp/src/server/axum.rs b/rust/crates/mpp/src/server/axum.rs index 400e05d0..d2012969 100644 --- a/rust/crates/mpp/src/server/axum.rs +++ b/rust/crates/mpp/src/server/axum.rs @@ -229,7 +229,7 @@ mod tests { use axum::http::Request; const TEST_RECIPIENT: &str = "CXhrFZJLKqjzmP3sjYLcF4dTeXWKCy9e2SXXZ2Yo6MPY"; - const TEST_SECRET: &str = "axum-extractor-test-secret-key"; + const TEST_SECRET: &str = "axum-extractor-test-secret-key-with-32b-padding"; fn test_mpp() -> Arc { Arc::new( diff --git a/rust/crates/mpp/src/server/charge.rs b/rust/crates/mpp/src/server/charge.rs index c385f2b9..2688aaed 100644 --- a/rust/crates/mpp/src/server/charge.rs +++ b/rust/crates/mpp/src/server/charge.rs @@ -13,9 +13,17 @@ //! // Generate a charge challenge (returns HTTP 402) //! let challenge = mpp.charge("0.10")?; //! -//! // Verify a credential from Authorization header +//! // Verify a credential from Authorization header. The expected ChargeRequest +//! // pins the route's amount/currency/recipient so a credential paid for one +//! // route can't be replayed against another (audit #2). //! let credential = solana_mpp::PaymentCredential::from_header(&auth_header)?; -//! let receipt = mpp.verify_credential(&credential).await?; +//! let expected = solana_mpp::ChargeRequest { +//! amount: "100000".to_string(), +//! currency: "USDC".to_string(), +//! recipient: Some("...".to_string()), +//! ..Default::default() +//! }; +//! let receipt = mpp.verify_credential_with_expected(&credential, &expected).await?; //! ``` use std::{collections::HashSet, sync::Arc}; @@ -43,10 +51,47 @@ const METHOD_NAME: &str = "solana"; const COMPUTE_BUDGET_PROGRAM: &str = "ComputeBudget111111111111111111111111111111"; const MAX_COMPUTE_UNIT_LIMIT: u32 = 200_000; const MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS: u64 = 5_000_000; +/// Tighter price cap applied when the *server* is the fee payer. +/// +/// In fee-sponsored pull mode the server signs the transaction before it is +/// broadcast, so the priority fee is paid out of the merchant's wallet. The +/// global cap above (5_000_000) is fine when the client pays its own gas, +/// but at that ceiling an attacker could burn `ceil(5_000_000 * 200_000 / +/// 1_000_000)` = 1_000_000 lamports of merchant SOL per "valid" charge. +/// 10_000 caps the worst case at ~2_000 lamports per request — about 20% of +/// the 5_000-lamport base fee per signature, which leaves enough headroom +/// for honest clients to bump priority during congestion without letting +/// the merchant be drained. +const MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS_FEE_SPONSORED: u64 = 10_000; const SIMULATION_MAX_ATTEMPTS: usize = 3; const SIMULATION_RETRY_DELAY_MS: u64 = 400; -const DEFAULT_REALM: &str = "MPP Payment"; +/// Audit #15: derive a per-app default realm from the recipient pubkey. +/// +/// `realm` is part of the HMAC ID input. With a fixed default of +/// `"MPP Payment"`, two services that shared `MPP_SECRET_KEY` and both +/// kept the default would participate in one shared credential namespace, +/// enabling cross-service credential replay. Deriving from `recipient` +/// (a Solana pubkey, unique per merchant) means two services with the +/// same secret but different recipients automatically get different +/// realms, so cross-service replay fails at HMAC verification. +/// +/// Format: `"App Id - #<8-digit decimal>"`. Decimal is `u32::from_be_bytes` +/// over the first 4 bytes of `SHA-256(recipient)`, modulo 10^8 for a +/// compact display. Deterministic for a given recipient. +fn derive_default_realm(recipient: &str) -> String { + use sha2::{Digest, Sha256}; + let hash = Sha256::digest(recipient.as_bytes()); + let first_four = u32::from_be_bytes([hash[0], hash[1], hash[2], hash[3]]); + let app_id = first_four % 100_000_000; + format!("App Id - #{app_id}") +} + +/// Minimum length, in bytes, for the HMAC-SHA256 key used to bind +/// challenge IDs. NIST SP 800-107 recommends a key at least as long as +/// the hash output (256 bits = 32 bytes); below that the key is the +/// weakest link, not the hash. +const MIN_SECRET_KEY_BYTES: usize = 32; fn detect_challenge_binding_secret() -> Result { std::env::var(SECRET_KEY_ENV_VAR).map_err(|_| { @@ -56,6 +101,68 @@ fn detect_challenge_binding_secret() -> Result { }) } +/// Reject empty / short challenge-binding secrets before they are used as +/// the HMAC key for challenge IDs. Audit #24: a weak key lets an attacker +/// forge challenges. We require at least `MIN_SECRET_KEY_BYTES` bytes of +/// input; callers SHOULD pass ≥32 bytes of cryptographically-random data +/// (e.g. `openssl rand -base64 32`). +fn validate_challenge_binding_secret(secret: &str) -> Result<(), Error> { + if secret.len() < MIN_SECRET_KEY_BYTES { + return Err(Error::InvalidConfig(format!( + "Secret key is too short ({} bytes): require at least {MIN_SECRET_KEY_BYTES} bytes \ + of cryptographically-random data (e.g. `openssl rand -base64 32`)", + secret.len() + ))); + } + Ok(()) +} + +// Audit #37: this used to be a private duplicate of the same function +// in `protocol/solana.rs`. Consolidated — callers in this file now use +// the public one via `crate::protocol::solana::default_rpc_url`. + +/// Resolve the SPL token program governing `currency`, once, at server +/// boot. Returns `None` for native SOL. For well-known stablecoins the +/// answer comes from the static table; for an arbitrary mint address the +/// owner is fetched on-chain and validated, per spec §7.2 (rather than +/// silently falling back to the legacy Token Program). +fn resolve_server_token_program( + rpc: &RpcClient, + currency: &str, + network: Option<&str>, +) -> Result, Error> { + if currency.eq_ignore_ascii_case("SOL") { + return Ok(None); + } + + if let Some(mint) = crate::protocol::solana::resolve_stablecoin_mint(currency, network) { + if crate::protocol::solana::is_known_stablecoin_mint(mint) { + return Ok(Some( + crate::protocol::solana::default_token_program_for_currency(currency, network), + )); + } + } + + let mint_pk = Pubkey::from_str(currency).map_err(|e| { + Error::InvalidConfig(format!( + "Currency {currency} is neither a known symbol nor a valid mint address: {e}" + )) + })?; + let account = rpc.get_account(&mint_pk).map_err(|e| { + Error::InvalidConfig(format!( + "Failed to fetch mint account for currency {currency}: {e}" + )) + })?; + let owner = account.owner.to_string(); + match owner.as_str() { + programs::TOKEN_PROGRAM => Ok(Some(programs::TOKEN_PROGRAM)), + programs::TOKEN_2022_PROGRAM => Ok(Some(programs::TOKEN_2022_PROGRAM)), + _ => Err(Error::InvalidConfig(format!( + "Mint {currency} is owned by unsupported program {owner}; expected the Token or Token-2022 program" + ))), + } +} + // ── Configuration ── /// Server configuration. @@ -67,11 +174,18 @@ pub struct Config { pub currency: String, /// Token decimals (default: 6 for USDC-like tokens). pub decimals: u8, - /// Solana network: mainnet, devnet, or localnet. + /// Solana network: one of "mainnet", "devnet", "localnet" (spec §7.2). + /// Validated at `Mpp::new` time. "mainnet-beta" is the RPC hostname, + /// not a canonical slug. pub network: String, /// RPC URL (overrides default for the network). pub rpc_url: Option, - /// Server secret key for HMAC challenge IDs. + /// Server challenge-binding secret for HMAC-SHA256 challenge IDs. + /// + /// MUST be at least 32 bytes of cryptographically-random data. Generate + /// with e.g. `openssl rand -base64 32`. Short or low-entropy keys are + /// rejected at `Mpp::new` time. If `None`, the value is read from the + /// `MPP_SECRET_KEY` environment variable. pub challenge_binding_secret: Option, /// Server realm. pub realm: Option, @@ -83,6 +197,20 @@ pub struct Config { pub store: Option>, /// Enable HTML payment link pages for browser requests. pub html: bool, + /// Audit #5: accept push-mode (`type=signature`) credentials. + /// + /// Push mode matches credentials to challenges by *shape* (recipient, + /// amount, currency, splits) — the on-chain tx is not bound to a + /// specific challenge id. Per spec §13.5 this is the accepted base + /// flow ("first accepted presentation wins"), but the lack of + /// cryptographic binding means any matching-shape transaction can + /// claim any matching-shape challenge. Routes that don't need push + /// mode should leave this off (default). + /// + /// Audit B34 already rejects push mode on fee-sponsored routes; this + /// gate runs first and covers the non-fee-sponsored case the audit + /// flagged. + pub accept_push_mode: bool, } impl Default for Config { @@ -99,6 +227,7 @@ impl Default for Config { fee_payer_signer: None, store: None, html: false, + accept_push_mode: false, } } } @@ -127,6 +256,12 @@ pub struct Mpp { realm: String, challenge_binding_secret: String, currency: String, + /// Token program governing `currency`. `None` for native SOL. Resolved + /// once at `Mpp::new` time — either from the hardcoded stablecoin table + /// or via an on-chain mint-owner lookup for arbitrary mint addresses + /// (spec §7.2). Reused at challenge generation and at verification so + /// the two sides stay in lockstep. + token_program: Option<&'static str>, recipient: String, decimals: u32, network: String, @@ -134,6 +269,8 @@ pub struct Mpp { fee_payer_signer: Option>, store: Arc, html: bool, + /// Audit #5: opt-in for push-mode credentials. + accept_push_mode: bool, } impl Mpp { @@ -145,21 +282,50 @@ impl Mpp { Pubkey::from_str(&config.recipient) .map_err(|e| Error::InvalidConfig(format!("Invalid recipient pubkey: {e}")))?; + // Audit #16: spec §7.2 requires `feePayerKey` when `feePayer` is + // true. Reject the boot-time misconfig at the source so + // `charge_with_options` can never emit a spec-violating challenge. + if config.fee_payer && config.fee_payer_signer.is_none() { + return Err(Error::InvalidConfig( + "Config.fee_payer is true but fee_payer_signer is None (spec §7.2 requires feePayerKey)".into(), + )); + } + + // Audit #37: spec §7.2 allows only `mainnet`, `devnet`, `localnet`. + // Rejecting `mainnet-beta`/`testnet`/typos at boot keeps the wire + // format canonical and stops the silent "everything unknown + // defaults to mainnet" behaviour that used to live in default_rpc_url. + crate::protocol::solana::validate_network(&config.network)?; + let rpc_url = config .rpc_url .unwrap_or_else(|| default_rpc_url(&config.network).to_string()); let challenge_binding_secret = config .challenge_binding_secret .map_or_else(detect_challenge_binding_secret, Ok)?; - let realm = config.realm.unwrap_or_else(|| DEFAULT_REALM.to_string()); + validate_challenge_binding_secret(&challenge_binding_secret)?; + let realm = match config.realm { + Some(r) if r.is_empty() => { + return Err(Error::InvalidConfig( + "Config.realm must be non-empty when provided".into(), + )); + } + Some(r) => r, + None => derive_default_realm(&config.recipient), + }; let store: Arc = config.store.unwrap_or_else(|| Arc::new(MemoryStore::new())); + let rpc = Arc::new(RpcClient::new(rpc_url.clone())); + let token_program = + resolve_server_token_program(&rpc, &config.currency, Some(&config.network))?; + Ok(Mpp { - rpc: Arc::new(RpcClient::new(rpc_url.clone())), + rpc, rpc_url, realm, challenge_binding_secret, currency: config.currency, + token_program, recipient: config.recipient, decimals: config.decimals as u32, network: config.network, @@ -167,6 +333,7 @@ impl Mpp { fee_payer_signer: config.fee_payer_signer, store, html: config.html, + accept_push_mode: config.accept_push_mode, }) } @@ -245,14 +412,10 @@ impl Mpp { } // Include token program so the client doesn't need to look up the mint account. - if self.currency.to_uppercase() != "SOL" { - details.insert( - "tokenProgram".into(), - serde_json::json!(crate::protocol::solana::default_token_program_for_currency( - &self.currency, - Some(&self.network), - )), - ); + // For arbitrary mints this was resolved on-chain at Mpp::new time and + // cached on the struct — never guessed from the currency string. + if let Some(token_program) = self.token_program { + details.insert("tokenProgram".into(), serde_json::json!(token_program)); } // Embed payment splits so the client can build multi-transfer transactions. @@ -301,6 +464,38 @@ impl Mpp { } fn validate_charge_options(&self, options: &ChargeOptions<'_>) -> Result<(), Error> { + // Audit #16: per-call fee-payer override is only honorable when a + // signer is configured on this server. Mpp::new already enforces + // the invariant for `self.fee_payer`; this catches the override + // case where Config.fee_payer is false but ChargeOptions.fee_payer + // is true. + if options.fee_payer && self.fee_payer_signer.is_none() { + return Err(Error::InvalidConfig( + "ChargeOptions.fee_payer is true but this server has no fee_payer_signer configured".into(), + )); + } + + // Audit #21: validate the splits up-front so malformed entries + // (bad pubkey, unparseable/zero amount, overflowing aggregate, + // duplicate recipients, too many splits) fail at challenge issuance + // instead of at on-chain settlement. + crate::protocol::solana::validate_splits(&options.splits)?; + + // Audit #38: spec §9.5 forbids fee-payer-funded ATA creation for the + // top-level recipient. A split that names the primary recipient AND + // sets `ataCreationRequired: true` is the misconfig shape that, in + // fee-sponsored mode, lets the recipient close/recreate its own ATA + // to keep draining server-funded rent. We still allow the primary + // recipient to appear in splits without the flag (legitimate when + // the merchant takes part of the funds as a regular split). + for (idx, split) in options.splits.iter().enumerate() { + if split.ata_creation_required == Some(true) && split.recipient == self.recipient { + return Err(Error::InvalidConfig(format!( + "splits[{idx}]: ataCreationRequired must not be true for the top-level recipient" + ))); + } + } + let has_ata_creation_splits = options .splits .iter() @@ -336,12 +531,23 @@ impl Mpp { } /// Generate a charge challenge from a full request with options. + /// + /// The override-point on the high-level `charge_with_options` path: + /// the caller supplies a fully-formed `ChargeRequest` and we issue a + /// challenge against *this* server's route. Audit #19: the request + /// is validated for internal consistency AND against the server's + /// own configuration before HMAC-signing, so a malformed or + /// off-route request cannot produce a cryptographically-valid + /// challenge. Callers who need to issue challenges for an unrelated + /// route should construct a `PaymentChallenge` directly via + /// `PaymentChallenge::with_secret_key_full`. pub fn charge_challenge_with_options( &self, request: &ChargeRequest, expires: Option<&str>, description: Option<&str>, ) -> Result { + self.validate_charge_request(request)?; let encoded = Base64UrlJson::from_typed(request)?; let default_expires = crate::expires::minutes(5); let expires = expires.unwrap_or(&default_expires); @@ -359,25 +565,89 @@ impl Mpp { )) } - // ── Verification ── + /// Audit #19: ensure a caller-built `ChargeRequest` parses and binds + /// to this server's route before we HMAC-sign it. Fields covered: + /// `amount`, `currency`, `recipient`, and the `methodDetails` + /// fragments that pin the server-side configuration + /// (`network`, `decimals`, `tokenProgram`, splits). + fn validate_charge_request(&self, request: &ChargeRequest) -> Result<(), Error> { + request.parse_amount()?; - /// Verify a payment credential (simple API). - /// - /// Decodes the charge request from the echoed challenge automatically. - pub async fn verify_credential( - &self, - credential: &PaymentCredential, - ) -> Result { - let request: ChargeRequest = credential - .challenge - .request - .decode() - .map_err(|e| VerificationError::new(format!("Failed to decode request: {e}")))?; - self.verify(credential, &request).await + if !request.currency.eq_ignore_ascii_case(&self.currency) { + return Err(Error::InvalidConfig(format!( + "ChargeRequest.currency `{}` does not match server-configured currency `{}`", + request.currency, self.currency + ))); + } + + let recipient = request + .recipient + .as_deref() + .ok_or_else(|| Error::InvalidConfig("ChargeRequest.recipient is required".into()))?; + Pubkey::from_str(recipient) + .map_err(|e| Error::InvalidConfig(format!("Invalid recipient pubkey: {e}")))?; + + if let Some(md_value) = &request.method_details { + let md: MethodDetails = serde_json::from_value(md_value.clone()) + .map_err(|e| Error::InvalidConfig(format!("Invalid methodDetails: {e}")))?; + + if let Some(network) = md.network.as_deref() { + if network != self.network { + return Err(Error::InvalidConfig(format!( + "methodDetails.network `{network}` does not match server-configured network `{}`", + self.network + ))); + } + } + + if let Some(decimals) = md.decimals { + if u32::from(decimals) != self.decimals { + return Err(Error::InvalidConfig(format!( + "methodDetails.decimals {decimals} does not match server-configured decimals {}", + self.decimals + ))); + } + } + + if let Some(tp) = md.token_program.as_deref() { + if Some(tp) != self.token_program { + return Err(Error::InvalidConfig(format!( + "methodDetails.tokenProgram `{tp}` does not match server-resolved token program {:?}", + self.token_program + ))); + } + } + + // Audit #21: shared split validation with the + // `charge_with_options` path. Failure modes (bad pubkey, + // unparseable/zero amount, overflowing aggregate, duplicate + // recipients, too many splits) all surface here. + if let Some(splits) = md.splits.as_deref() { + crate::protocol::solana::validate_splits(splits)?; + } + } + + Ok(()) } - /// Verify with cross-route protection — ensures the credential matches - /// the expected charge parameters for this endpoint. + // ── Verification ── + + /// Verify a payment credential against the expected charge for *this* + /// route. This is the canonical entry point for credential verification. + /// + /// **Audit #2 — why no simpler "trust the echoed challenge" variant.** + /// We deliberately do not offer a method that decodes the credential's + /// embedded request and verifies against *that*. A server that issues + /// multiple priced routes (the common case) would otherwise accept a + /// credential paid for the $1 route against the $100 route — same + /// currency, same recipient, same server-issued HMAC, but the wrong + /// resource. Callers must pass an `expected` `ChargeRequest` built + /// from this route's static configuration, so the amount and other + /// payment-constraining fields are pinned at the call site. + /// + /// Single-resource servers construct the same `expected` once and reuse + /// it; the boilerplate is small. The compile-time cost of the explicit + /// argument is the whole point. pub async fn verify_credential_with_expected( &self, credential: &PaymentCredential, @@ -389,21 +659,7 @@ impl Mpp { .decode() .map_err(|e| VerificationError::new(format!("Failed to decode request: {e}")))?; - if request.amount != expected.amount { - return Err(VerificationError::credential_mismatch(format!( - "Amount mismatch: credential has {} but endpoint expects {}", - request.amount, expected.amount - ))); - } - if request.currency != expected.currency { - return Err(VerificationError::credential_mismatch(format!( - "Currency mismatch: credential has {} but endpoint expects {}", - request.currency, expected.currency - ))); - } - if request.recipient != expected.recipient { - return Err(VerificationError::credential_mismatch("Recipient mismatch")); - } + compare_expected_to_request(&request, expected)?; // Pass the route's expected request — not the credential-decoded one — // through to `verify`. From this point on, on-chain settlement checks @@ -417,10 +673,10 @@ impl Mpp { /// /// After Tier 1 (HMAC) confirms the echoed challenge was issued by this /// server, this compares economically-significant fields against the - /// pinned `Mpp` configuration. It is the safety net for callers who use - /// the simple `verify_credential` API: even when the credential's - /// claimed request is trusted as-is, fields fixed at server construction - /// (method, intent, realm, currency, recipient) cannot silently diverge. + /// pinned `Mpp` configuration. Defense-in-depth against a route that + /// hands `verify` a request decoded from a tampered credential: fields + /// fixed at server construction (method, intent, realm, currency, + /// recipient) cannot silently diverge. fn verify_pinned_fields( &self, credential: &PaymentCredential, @@ -483,7 +739,14 @@ impl Mpp { credential.challenge.digest.as_deref(), credential.challenge.opaque.as_ref().map(|o| o.raw()), ); - if credential.challenge.id != expected_id { + // Audit #41: the HMAC id comparison must be constant-time — + // otherwise a timing oracle could leak how many leading bytes + // of an attacker-controlled `id` match an actually-issued one. + // The same helper backs `PaymentChallenge::verify`. + if !crate::protocol::core::challenge::constant_time_eq( + &credential.challenge.id, + &expected_id, + ) { return Err(VerificationError::credential_mismatch( "Challenge ID mismatch — not issued by this server", )); @@ -506,8 +769,22 @@ impl Mpp { } } - // Tier 2: Pinned-field backstop. Runs unconditionally so even simple - // `verify_credential` callers are protected against cross-route replay + // Audit #22: HMAC authenticates `credential.challenge.request` — the + // request the server originally issued. Settlement then runs against + // the caller-supplied `request`. Without binding the two, a direct + // caller could authenticate one request and verify settlement + // against a different one. Require equality on every + // payment-constraining field (reuses the audit #1 helper). + let credential_request: ChargeRequest = + credential.challenge.request.decode().map_err(|e| { + VerificationError::invalid_payload(format!( + "Failed to decode credential request: {e}" + )) + })?; + compare_expected_to_request(&credential_request, request)?; + + // Tier 2: Pinned-field backstop. Runs unconditionally so callers of + // the lower-level `verify` are protected against cross-route replay // for the fields that are pinned at `Mpp` construction time. self.verify_pinned_fields(credential, request)?; @@ -543,6 +820,16 @@ impl Mpp { signature } CredentialPayload::Signature { ref signature } => { + // Audit #5: push-mode acceptance is opt-in. Spec §13.5 names + // "first accepted presentation wins" as the model — any + // matching-shape on-chain tx can claim any matching-shape + // challenge. Servers that don't need push mode should leave + // `accept_push_mode = false` (default) to reduce surface. + if !self.accept_push_mode { + return Err(VerificationError::credential_mismatch( + "Push-mode credentials are disabled on this server (Config.accept_push_mode is false; spec §13.5)", + )); + } // B34: reject push-mode credentials (`type=signature`) on // routes that require a server-side fee payer. A signature- // only credential references an already-landed transaction @@ -774,9 +1061,27 @@ impl Mpp { _ => std::thread::sleep(std::time::Duration::from_millis(200)), } } - Err(VerificationError::network_error( - "Transaction not confirmed within timeout".to_string(), - )) + + // Audit #3: the polling RPC may be lagging or load-balanced behind an + // endpoint that hasn't observed the signature yet, while the tx is + // actually on-chain. Do one definitive status check before declaring + // a timeout — otherwise we'd return network_error for a payment the + // user has already made (the signature is reserved in the replay + // store, so a retry would also fail). + let final_status = self + .rpc + .get_signature_status(&signature) + .map(|opt| opt.map(|inner| inner.map_err(|e| e.to_string()))) + .map_err(|e| e.to_string()); + let result = interpret_post_timeout_status(final_status); + if result.is_ok() { + tracing::info!( + elapsed_ms = %t0.elapsed().as_millis(), + step = "confirmed_via_status_recovery", + "await_pull_confirmation" + ); + } + result } /// Push mode: fetch tx by signature, verify on-chain. @@ -826,10 +1131,8 @@ impl Mpp { })?; let splits = method_details.splits.as_deref().unwrap_or(&[]); - let splits_total: u64 = splits - .iter() - .filter_map(|s| s.amount.parse::().ok()) - .sum(); + let splits_total = crate::protocol::solana::checked_sum_split_amounts(splits) + .ok_or_else(|| VerificationError::invalid_amount("Split amounts overflow u64"))?; let primary_amount = total_amount.checked_sub(splits_total).ok_or_else(|| { VerificationError::invalid_amount("Split amounts exceed total amount") })?; @@ -880,7 +1183,13 @@ impl Mpp { "ataCreationRequired requires an SPL token charge", )); } - let matched = verify_sol_transfers(&instructions, recipient, primary_amount, splits)?; + let matched = verify_sol_transfers( + &instructions, + recipient, + primary_amount, + splits, + expected_ata_payer, + )?; let mut matched = matched; verify_parsed_memo_instructions( &instructions, @@ -900,18 +1209,30 @@ impl Mpp { } else { let expected_mint = resolve_expected_mint(&request.currency, method_details.network.as_deref())?; - if !required_ata_owners.is_empty() && request.currency != expected_mint.to_string() { + // Audit #34: check the property we care about directly — + // `request.currency` must parse as a Pubkey (i.e. be an actual + // mint address, not a symbol). The previous "currency != + // expected_mint" check was equivalent in outcome but expressed + // the intent obliquely. + if !required_ata_owners.is_empty() && Pubkey::from_str(&request.currency).is_err() { return Err(VerificationError::invalid_payload( "ataCreationRequired requires currency to be an SPL token mint address", )); } - let expected_token_program = - method_details.token_program.as_deref().unwrap_or_else(|| { - crate::protocol::solana::default_token_program_for_currency( - &request.currency, - method_details.network.as_deref(), + // Prefer the challenge's tokenProgram hint. If the credential + // came from a challenge we didn't sign (or one missing the + // hint), fall back to the boot-time resolution we did against + // our own currency — never to a guess based on the currency + // string (spec §7.2). + let expected_token_program = method_details + .token_program + .as_deref() + .or(self.token_program) + .ok_or_else(|| { + VerificationError::invalid_payload( + "Missing tokenProgram and server has no resolved token program for this currency", ) - }); + })?; let mut matched = verify_spl_transfers( &instructions, recipient, @@ -919,6 +1240,7 @@ impl Mpp { primary_amount, splits, Some(expected_token_program), + expected_ata_payer, )?; verify_parsed_memo_instructions( &instructions, @@ -1020,20 +1342,19 @@ fn verify_versioned_transaction_pre_broadcast( reject_address_lookup_tables(tx)?; let splits = method_details.splits.as_deref().unwrap_or(&[]); - if splits.len() > 8 { + if splits.len() > crate::protocol::solana::MAX_SPLITS { return Err(VerificationError::too_many_splits(format!( - "Too many splits: {} (maximum 8)", - splits.len() + "Too many splits: {} (maximum {})", + splits.len(), + crate::protocol::solana::MAX_SPLITS, ))); } let total_amount: u64 = request.amount.parse().map_err(|_| { VerificationError::invalid_amount(format!("Invalid amount: {}", request.amount)) })?; - let splits_total: u64 = splits - .iter() - .filter_map(|s| s.amount.parse::().ok()) - .sum(); + let splits_total = crate::protocol::solana::checked_sum_split_amounts(splits) + .ok_or_else(|| VerificationError::invalid_amount("Split amounts overflow u64"))?; let primary_amount = total_amount .checked_sub(splits_total) .ok_or_else(|| VerificationError::invalid_amount("Split amounts exceed total amount"))?; @@ -1113,7 +1434,9 @@ fn verify_versioned_transaction_pre_broadcast( } else { let expected_mint = resolve_expected_mint(&request.currency, method_details.network.as_deref())?; - if !ata_policy.required_owners.is_empty() && request.currency != expected_mint.to_string() { + // Audit #34: see the matching block above — check that + // `request.currency` parses as a Pubkey directly. + if !ata_policy.required_owners.is_empty() && Pubkey::from_str(&request.currency).is_err() { return Err(VerificationError::invalid_payload( "ataCreationRequired requires currency to be an SPL token mint address", )); @@ -1206,6 +1529,140 @@ fn expected_ata_creation_policy( }) } +/// Audit #1: exhaustively compare the credential's decoded request against +/// the route's expected request before any settlement work. +/// +/// Why up-front (when `verify_credential_with_expected` already passes +/// `expected` to `verify` and on-chain settlement checks against it): +/// 1. Earlier, clearer failure — `splits mismatch` beats `no matching SPL +/// transferChecked instruction` for an operator chasing a bug. +/// 2. Defense in depth — any field added to `ChargeRequest` or `MethodDetails` +/// in the future is forced into this comparison, so a divergence cannot +/// silently slip past the settlement layer. +/// +/// `recent_blockhash` is deliberately *not* compared: it's per-challenge +/// state, not per-route policy, and an `expected` built from a route's +/// static config carries no blockhash. +fn compare_expected_to_request( + request: &ChargeRequest, + expected: &ChargeRequest, +) -> Result<(), VerificationError> { + if request.amount != expected.amount { + return Err(VerificationError::credential_mismatch(format!( + "Amount mismatch: credential has {} but endpoint expects {}", + request.amount, expected.amount + ))); + } + if request.currency != expected.currency { + return Err(VerificationError::credential_mismatch(format!( + "Currency mismatch: credential has {} but endpoint expects {}", + request.currency, expected.currency + ))); + } + if request.recipient != expected.recipient { + return Err(VerificationError::credential_mismatch("Recipient mismatch")); + } + if request.external_id != expected.external_id { + return Err(VerificationError::credential_mismatch( + "externalId mismatch", + )); + } + if request.description != expected.description { + return Err(VerificationError::credential_mismatch( + "description mismatch", + )); + } + + let request_md = parse_method_details_for_compare(&request.method_details, "credential")?; + let expected_md = parse_method_details_for_compare(&expected.method_details, "expected")?; + + if request_md.network != expected_md.network { + return Err(VerificationError::credential_mismatch( + "methodDetails.network mismatch", + )); + } + if request_md.decimals != expected_md.decimals { + return Err(VerificationError::credential_mismatch( + "methodDetails.decimals mismatch", + )); + } + if request_md.token_program != expected_md.token_program { + return Err(VerificationError::credential_mismatch( + "methodDetails.tokenProgram mismatch", + )); + } + if request_md.fee_payer != expected_md.fee_payer { + return Err(VerificationError::credential_mismatch( + "methodDetails.feePayer mismatch", + )); + } + if request_md.fee_payer_key != expected_md.fee_payer_key { + return Err(VerificationError::credential_mismatch( + "methodDetails.feePayerKey mismatch", + )); + } + // Splits compared element-wise (order-sensitive). A route that pins + // `[A, B]` will reject a credential carrying `[B, A]`. + if !splits_eq(request_md.splits.as_deref(), expected_md.splits.as_deref()) { + return Err(VerificationError::credential_mismatch( + "methodDetails.splits mismatch", + )); + } + // recent_blockhash intentionally NOT compared — see helper docstring. + + Ok(()) +} + +fn parse_method_details_for_compare( + md: &Option, + label: &str, +) -> Result { + match md { + Some(v) => serde_json::from_value(v.clone()).map_err(|e| { + VerificationError::credential_mismatch(format!("Invalid {label} methodDetails: {e}")) + }), + None => Ok(MethodDetails::default()), + } +} + +fn splits_eq(a: Option<&[Split]>, b: Option<&[Split]>) -> bool { + let a = a.unwrap_or(&[]); + let b = b.unwrap_or(&[]); + if a.len() != b.len() { + return false; + } + a.iter().zip(b.iter()).all(|(x, y)| { + x.recipient == y.recipient + && x.amount == y.amount + && x.ata_creation_required == y.ata_creation_required + && x.label == y.label + && x.memo == y.memo + }) +} + +/// Audit #3: interpret a post-timeout `get_signature_status` result. +/// +/// Pulled out as a pure function so the four cases — landed, landed-but-failed, +/// not-found, RPC-error — can be unit-tested without needing a live RPC. +/// Errors are stringified at the call site so this helper stays free of +/// `solana-rpc-client` types. +fn interpret_post_timeout_status( + status: Result>, String>, +) -> Result<(), VerificationError> { + match status { + Ok(Some(Ok(()))) => Ok(()), + Ok(Some(Err(on_chain_err))) => Err(VerificationError::transaction_failed(format!( + "Transaction landed on-chain but failed: {on_chain_err}" + ))), + Ok(None) => Err(VerificationError::network_error( + "Transaction not confirmed within timeout".to_string(), + )), + Err(rpc_err) => Err(VerificationError::network_error(format!( + "Transaction not confirmed within timeout; final status check failed: {rpc_err}" + ))), + } +} + fn reject_address_lookup_tables(tx: &VersionedTransaction) -> Result<(), VerificationError> { if tx .message @@ -1306,7 +1763,7 @@ fn validate_instruction_allowlist( .ok_or_else(|| VerificationError::invalid_payload("Invalid program_id_index"))?; if program_id == &compute_budget_program { - validate_compute_budget_instruction(ix)?; + validate_compute_budget_instruction(ix, fee_payer.is_some())?; continue; } @@ -1366,7 +1823,10 @@ fn validate_instruction_allowlist( Ok(()) } -fn validate_compute_budget_instruction(ix: &CompiledInstruction) -> Result<(), VerificationError> { +fn validate_compute_budget_instruction( + ix: &CompiledInstruction, + fee_sponsored: bool, +) -> Result<(), VerificationError> { if !ix.accounts.is_empty() { return Err(VerificationError::invalid_payload( "Compute budget instruction must not have accounts", @@ -1385,9 +1845,14 @@ fn validate_compute_budget_instruction(ix: &CompiledInstruction) -> Result<(), V } Some(3) if ix.data.len() == 9 => { let price = u64::from_le_bytes(ix.data[1..9].try_into().unwrap()); - if price > MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS { + let max = if fee_sponsored { + MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS_FEE_SPONSORED + } else { + MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS + }; + if price > max { return Err(VerificationError::invalid_payload(format!( - "Compute unit price {price} exceeds maximum {MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS}" + "Compute unit price {price} exceeds maximum {max}" ))); } Ok(()) @@ -1674,12 +2139,14 @@ fn verify_sol_transfers( recipient: &str, primary_amount: u64, splits: &[Split], + fee_payer: Option<&str>, ) -> Result, VerificationError> { let mut matched_instruction_indexes = HashSet::new(); find_sol_transfer( instructions, recipient, primary_amount, + fee_payer, &mut matched_instruction_indexes, )?; for split in splits { @@ -1691,6 +2158,7 @@ fn verify_sol_transfers( instructions, &split.recipient, amt, + fee_payer, &mut matched_instruction_indexes, ) .map_err(|_| { @@ -1707,12 +2175,16 @@ fn find_sol_transfer( instructions: &[serde_json::Value], recipient: &str, amount: u64, + fee_payer: Option<&str>, matched_instruction_indexes: &mut HashSet, ) -> Result<(), VerificationError> { for (index, ix) in instructions.iter().enumerate() { if matched_instruction_indexes.contains(&index) { continue; } + if parsed_program_id(ix) != Some(programs::SYSTEM_PROGRAM) { + continue; + } if let Some(parsed) = ix.get("parsed").and_then(|p| p.as_object()) { if parsed.get("type").and_then(|t| t.as_str()) != Some("transfer") { continue; @@ -1722,8 +2194,14 @@ fn find_sol_transfer( .get("destination") .and_then(|d| d.as_str()) .unwrap_or(""); + let source = info.get("source").and_then(|s| s.as_str()).unwrap_or(""); let lamports = info.get("lamports").and_then(|l| l.as_u64()).unwrap_or(0); if dest == recipient && lamports == amount { + if fee_payer.is_some_and(|fp| source == fp) { + return Err(VerificationError::invalid_payload( + "Fee payer cannot fund the SOL payment transfer", + )); + } matched_instruction_indexes.insert(index); return Ok(()); } @@ -1742,6 +2220,7 @@ fn verify_spl_transfers( primary_amount: u64, splits: &[Split], expected_token_program: Option<&str>, + fee_payer: Option<&str>, ) -> Result, VerificationError> { let mut matched_instruction_indexes = HashSet::new(); find_spl_transfer( @@ -1750,6 +2229,7 @@ fn verify_spl_transfers( mint, primary_amount, expected_token_program, + fee_payer, &mut matched_instruction_indexes, )?; for split in splits { @@ -1763,6 +2243,7 @@ fn verify_spl_transfers( mint, amt, expected_token_program, + fee_payer, &mut matched_instruction_indexes, ) .map_err(|_| { @@ -1781,8 +2262,10 @@ fn find_spl_transfer( expected_mint: &str, amount: u64, expected_token_program: Option<&str>, + fee_payer: Option<&str>, matched_instruction_indexes: &mut HashSet, ) -> Result<(), VerificationError> { + let ata_program = Pubkey::from_str(programs::ASSOCIATED_TOKEN_PROGRAM).unwrap(); for (index, ix) in instructions.iter().enumerate() { if matched_instruction_indexes.contains(&index) { continue; @@ -1812,8 +2295,32 @@ fn find_spl_transfer( .get("destination") .and_then(|d| d.as_str()) .unwrap_or(""); + let source = info.get("source").and_then(|s| s.as_str()).unwrap_or(""); + let authority = info.get("authority").and_then(|a| a.as_str()).unwrap_or(""); let mint = info.get("mint").and_then(|m| m.as_str()).unwrap_or(""); if mint == expected_mint && verify_ata_owner(dest, recipient, mint, program) { + if let Some(fee_payer) = fee_payer { + if authority == fee_payer { + return Err(VerificationError::invalid_payload( + "Fee payer cannot authorize the SPL payment transfer", + )); + } + if let (Ok(fee_payer_pk), Ok(mint_pk), Ok(program_pk)) = ( + Pubkey::from_str(fee_payer), + Pubkey::from_str(mint), + Pubkey::from_str(program), + ) { + let (fee_payer_ata, _) = Pubkey::find_program_address( + &[fee_payer_pk.as_ref(), program_pk.as_ref(), mint_pk.as_ref()], + &ata_program, + ); + if source == fee_payer_ata.to_string() { + return Err(VerificationError::invalid_payload( + "Fee payer token account cannot fund the SPL payment transfer", + )); + } + } + } matched_instruction_indexes.insert(index); return Ok(()); } @@ -2110,6 +2617,16 @@ fn string_field<'a>( /// " | payer USDC balance: 0.00 (need 0.10), fee payer SOL: 0.005" /// /// Never fails — returns an empty string if any RPC call errors. +/// Audit #8: convert a base-unit amount to a UI amount for diagnostic +/// rendering. Returns `None` when `10u64.pow(decimals)` would overflow, +/// so the caller can omit that diagnostic line instead of panicking +/// (debug) or wrapping silently (release) — `diagnose_balances` only +/// runs after settlement already failed and is best-effort. +fn to_ui_amount(amount_base_units: u64, decimals: u8) -> Option { + let divisor = 10u64.checked_pow(decimals as u32)?; + Some(amount_base_units as f64 / divisor as f64) +} + fn diagnose_balances( rpc: &RpcClient, tx: &VersionedTransaction, @@ -2131,36 +2648,57 @@ fn diagnose_balances( .or(tx.message.static_account_keys().first()); // Check payer's token balance. - if let Some(payer) = payer_pk { + // Audit #13: derive the ATA against the actual token program for this + // currency, not a hardcoded TOKEN_PROGRAM. For Token-2022 mints (PYUSD, + // USDG on Token-2022, CASH, …) the legacy program produces the wrong + // ATA, so the diagnostic would silently lie about the payer's balance. + // The token program was already resolved at boot (audit #28) and + // embedded in `methodDetails.tokenProgram` for every SPL challenge this + // server issues; we just use it. If it's missing (lower-level + // ChargeRequest construction edge case), skip the token-balance hint — + // the fee-payer SOL diagnostic below still runs. + let token_program = method_details + .token_program + .as_deref() + .and_then(|s| Pubkey::from_str(s).ok()); + + if let (Some(payer), Some(token_program)) = (payer_pk, token_program.as_ref()) { if request.currency.to_uppercase() != "SOL" { if let Ok(mint) = resolve_expected_mint(&request.currency, method_details.network.as_deref()) { - let token_program = Pubkey::from_str(programs::TOKEN_PROGRAM).unwrap(); let ata_program = Pubkey::from_str(programs::ASSOCIATED_TOKEN_PROGRAM).unwrap(); let (ata, _) = Pubkey::find_program_address( &[payer.as_ref(), token_program.as_ref(), mint.as_ref()], &ata_program, ); - let decimals = method_details.decimals.unwrap_or(6) as u32; - let divisor = 10u64.pow(decimals) as f64; - let needed = request.amount.parse::().unwrap_or(0) as f64 / divisor; - match rpc.get_token_account_balance(&ata) { - Ok(bal) => { - let actual: f64 = bal.ui_amount.unwrap_or(0.0); - if actual < needed { + // Audit #42: spec mandates `decimals` on SPL challenges; + // pretending 6 would silently lie. Skip the diagnostic + // instead — fee-payer SOL hint below still runs. + // Audit #8: skip the token-balance hint when the divisor + // can't be represented — see `to_ui_amount` for the why. + let needed_base = request.amount.parse::().unwrap_or(0); + if let Some(needed) = method_details + .decimals + .and_then(|d| to_ui_amount(needed_base, d)) + { + match rpc.get_token_account_balance(&ata) { + Ok(bal) => { + let actual: f64 = bal.ui_amount.unwrap_or(0.0); + if actual < needed { + parts.push(format!( + "payer {} balance: {:.2} (need {:.2})", + request.currency, actual, needed, + )); + } + } + Err(_) => { parts.push(format!( - "payer {} balance: {:.2} (need {:.2})", - request.currency, actual, needed, + "payer {} token account not found (need {:.2})", + request.currency, needed, )); } } - Err(_) => { - parts.push(format!( - "payer {} token account not found (need {:.2})", - request.currency, needed, - )); - } } } } @@ -2293,19 +2831,22 @@ impl VerificationError { } pub fn invalid_amount(msg: impl Into) -> Self { + // Audit #11: title aligned to the function name. Code stays + // `verification-failed` so callers grouping by code keep working. Self::with_code( msg, "verification-failed", - "Verification Failed", + "Invalid Amount", "tag:paymentauth.org,2024:verification-failed", ) } pub fn invalid_recipient(msg: impl Into) -> Self { + // Audit #11: title aligned to the function name. Self::with_code( msg, "verification-failed", - "Verification Failed", + "Invalid Recipient", "tag:paymentauth.org,2024:verification-failed", ) } @@ -2339,10 +2880,12 @@ impl VerificationError { } pub fn credential_mismatch(msg: impl Into) -> Self { + // Audit #11: title aligned to the function name. Code stays + // `malformed-credential` (shared with `invalid_payload`). Self::with_code( msg, "malformed-credential", - "Malformed Credential", + "Credential Mismatch", "tag:paymentauth.org,2024:malformed-credential", ) } @@ -2447,6 +2990,102 @@ mod tests { assert_eq!(err.code, Some("wrong-network")); } + // ── Audit #8: to_ui_amount ── + + #[test] + fn to_ui_amount_typical_decimals() { + // 1 USDC = 1_000_000 base units with 6 decimals. + let v = to_ui_amount(1_000_000, 6).unwrap(); + assert!((v - 1.0).abs() < 1e-9); + } + + #[test] + fn to_ui_amount_zero_decimals() { + // No fractional rendering — divisor is 1. + let v = to_ui_amount(42, 0).unwrap(); + assert!((v - 42.0).abs() < 1e-9); + } + + #[test] + fn to_ui_amount_returns_none_when_divisor_overflows_u64() { + // 10^20 overflows u64. Helper must skip rather than panic. + assert!(to_ui_amount(1, 20).is_none()); + assert!(to_ui_amount(0, 255).is_none()); + } + + #[test] + fn to_ui_amount_safe_high_decimals_succeed() { + // 10^19 fits in u64 (< 1.84e19); 10^20 doesn't. + assert!(to_ui_amount(1, 19).is_some()); + assert!(to_ui_amount(1, 20).is_none()); + } + + // ── Audit #3: post-timeout status recovery ── + + #[test] + fn interpret_post_timeout_status_landed_returns_ok() { + // Polling timed out but the final status check shows the tx landed + // successfully — recover and report success. + assert!(interpret_post_timeout_status(Ok(Some(Ok(())))).is_ok()); + } + + #[test] + fn interpret_post_timeout_status_landed_with_onchain_err_returns_failed() { + // Tx landed on-chain but the runtime rejected it. This is a real + // transaction failure, not a timeout — surface the on-chain error. + let err = interpret_post_timeout_status(Ok(Some(Err("InsufficientFundsForFee".into())))) + .err() + .expect("on-chain failure should be reported"); + let msg = format!("{err}"); + assert!( + msg.contains("landed on-chain but failed"), + "unexpected error: {msg}" + ); + assert!( + msg.contains("InsufficientFundsForFee"), + "expected on-chain error to be propagated: {msg}" + ); + } + + #[test] + fn interpret_post_timeout_status_not_found_returns_timeout() { + // Final check confirms the tx is genuinely not on-chain — keep the + // timeout error. + let err = interpret_post_timeout_status(Ok(None)) + .err() + .expect("not-found should still error"); + let msg = format!("{err}"); + assert!( + msg.contains("not confirmed within timeout"), + "unexpected error: {msg}" + ); + // Must NOT claim landed-but-failed. + assert!(!msg.contains("landed on-chain"), "wrong shape: {msg}"); + } + + #[test] + fn interpret_post_timeout_status_rpc_error_returns_timeout_with_detail() { + // The final status call itself failed (e.g. RPC unreachable). We + // can't tell whether the tx landed, so we keep the timeout error + // but include the RPC failure in the message for ops. + let err = interpret_post_timeout_status(Err("connection refused".into())) + .err() + .expect("rpc failure should error"); + let msg = format!("{err}"); + assert!( + msg.contains("not confirmed within timeout"), + "unexpected error: {msg}" + ); + assert!( + msg.contains("final status check failed"), + "expected detail about the status RPC failure: {msg}" + ); + assert!( + msg.contains("connection refused"), + "expected underlying RPC error to be propagated: {msg}" + ); + } + #[test] fn network_check_localnet_with_surfpool_hash_ok() { assert!( @@ -2877,15 +3516,22 @@ mod tests { } #[test] - fn fee_payer_must_be_transaction_fee_payer() { - let sender = Pubkey::new_unique(); + fn compute_unit_price_fee_sponsored_under_tight_cap_passes() { + // Audit #25: in fee-sponsored mode the merchant pays the priority + // fee, so we apply MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS_FEE_SPONSORED + // instead of the general cap. A price right at the tight cap is + // allowed. let fee_payer = Pubkey::new_unique(); + let sender = Pubkey::new_unique(); let recipient = Pubkey::new_unique(); let amount = 500_000u64; let tx = dummy_tx( - vec![system_transfer_ix(&sender, &recipient, amount)], - &sender, + vec![ + compute_unit_price_ix(MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS_FEE_SPONSORED), + system_transfer_ix(&sender, &recipient, amount), + ], + &fee_payer, ); let request = charge_request(amount, "SOL", &recipient); let method_details = MethodDetails { @@ -2894,18 +3540,24 @@ mod tests { ..Default::default() }; - let err = verify_transaction_pre_broadcast(&tx, &request, &method_details).unwrap_err(); - assert!(err.message.contains("Transaction fee payer must be")); + assert!(verify_transaction_pre_broadcast(&tx, &request, &method_details).is_ok()); } #[test] - fn fee_payer_cannot_fund_sol_payment_transfer() { + fn compute_unit_price_fee_sponsored_above_tight_cap_rejected() { + // Audit #25: a price between the tight fee-sponsored cap and the + // general cap is what an attacker would use to drain the merchant. + // Must be rejected before the server co-signs. let fee_payer = Pubkey::new_unique(); + let sender = Pubkey::new_unique(); let recipient = Pubkey::new_unique(); let amount = 500_000u64; let tx = dummy_tx( - vec![system_transfer_ix(&fee_payer, &recipient, amount)], + vec![ + compute_unit_price_ix(MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS_FEE_SPONSORED + 1), + system_transfer_ix(&sender, &recipient, amount), + ], &fee_payer, ); let request = charge_request(amount, "SOL", &recipient); @@ -2916,26 +3568,92 @@ mod tests { }; let err = verify_transaction_pre_broadcast(&tx, &request, &method_details).unwrap_err(); - assert!(err.message.contains("Fee payer cannot fund")); + assert!(err.message.contains("Compute unit price")); } - // ── Pre-broadcast SPL verification tests ── - #[test] - fn spl_transfer_correct_amount_passes() { + fn compute_unit_price_client_paid_above_tight_cap_passes() { + // The tight cap only applies when the server is the fee payer. + // Without fee-sponsorship the client is paying their own gas, so + // the general (5_000_000) cap still applies and a price just above + // the tight cap must be accepted. let sender = Pubkey::new_unique(); let recipient = Pubkey::new_unique(); - let mint = Pubkey::from_str("EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v").unwrap(); - let amount = 1_000_000u64; // 1 USDC - - let tp = token_program_id(); - let source_ata = derive_ata(&sender, &mint, &tp); - let dest_ata = derive_ata(&recipient, &mint, &tp); + let amount = 500_000u64; let tx = dummy_tx( - vec![spl_transfer_checked_ix( - &source_ata, - &mint, + vec![ + compute_unit_price_ix(MAX_COMPUTE_UNIT_PRICE_MICROLAMPORTS_FEE_SPONSORED + 1), + system_transfer_ix(&sender, &recipient, amount), + ], + &sender, + ); + let request = charge_request(amount, "SOL", &recipient); + let method_details = MethodDetails::default(); + + assert!(verify_transaction_pre_broadcast(&tx, &request, &method_details).is_ok()); + } + + #[test] + fn fee_payer_must_be_transaction_fee_payer() { + let sender = Pubkey::new_unique(); + let fee_payer = Pubkey::new_unique(); + let recipient = Pubkey::new_unique(); + let amount = 500_000u64; + + let tx = dummy_tx( + vec![system_transfer_ix(&sender, &recipient, amount)], + &sender, + ); + let request = charge_request(amount, "SOL", &recipient); + let method_details = MethodDetails { + fee_payer: Some(true), + fee_payer_key: Some(fee_payer.to_string()), + ..Default::default() + }; + + let err = verify_transaction_pre_broadcast(&tx, &request, &method_details).unwrap_err(); + assert!(err.message.contains("Transaction fee payer must be")); + } + + #[test] + fn fee_payer_cannot_fund_sol_payment_transfer() { + let fee_payer = Pubkey::new_unique(); + let recipient = Pubkey::new_unique(); + let amount = 500_000u64; + + let tx = dummy_tx( + vec![system_transfer_ix(&fee_payer, &recipient, amount)], + &fee_payer, + ); + let request = charge_request(amount, "SOL", &recipient); + let method_details = MethodDetails { + fee_payer: Some(true), + fee_payer_key: Some(fee_payer.to_string()), + ..Default::default() + }; + + let err = verify_transaction_pre_broadcast(&tx, &request, &method_details).unwrap_err(); + assert!(err.message.contains("Fee payer cannot fund")); + } + + // ── Pre-broadcast SPL verification tests ── + + #[test] + fn spl_transfer_correct_amount_passes() { + let sender = Pubkey::new_unique(); + let recipient = Pubkey::new_unique(); + let mint = Pubkey::from_str("EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v").unwrap(); + let amount = 1_000_000u64; // 1 USDC + + let tp = token_program_id(); + let source_ata = derive_ata(&sender, &mint, &tp); + let dest_ata = derive_ata(&recipient, &mint, &tp); + + let tx = dummy_tx( + vec![spl_transfer_checked_ix( + &source_ata, + &mint, &dest_ata, &sender, amount, @@ -3303,7 +4021,7 @@ mod tests { // ── Helper: create an Mpp instance for testing ── - const TEST_SECRET: &str = "test-secret-key-for-unit-tests"; + const TEST_SECRET: &str = "test-secret-key-for-unit-tests-with-32b-padding"; const TEST_RECIPIENT: &str = "CXhrFZJLKqjzmP3sjYLcF4dTeXWKCy9e2SXXZ2Yo6MPY"; fn test_mpp() -> Mpp { @@ -3345,7 +4063,7 @@ mod tests { fn new_missing_recipient_errors() { let err = Mpp::new(Config { recipient: String::new(), - challenge_binding_secret: Some("key".to_string()), + challenge_binding_secret: Some(TEST_SECRET.to_string()), ..Default::default() }) .err() @@ -3360,7 +4078,7 @@ mod tests { fn new_invalid_recipient_pubkey_errors() { let err = Mpp::new(Config { recipient: "not-a-valid-pubkey!!!".to_string(), - challenge_binding_secret: Some("key".to_string()), + challenge_binding_secret: Some(TEST_SECRET.to_string()), ..Default::default() }) .err() @@ -3397,7 +4115,12 @@ mod tests { fn new_challenge_binding_secret_from_env() { let _guard = ENV_LOCK.lock().unwrap(); let prev = std::env::var(SECRET_KEY_ENV_VAR).ok(); - unsafe { std::env::set_var(SECRET_KEY_ENV_VAR, "env-secret") }; + unsafe { + std::env::set_var( + SECRET_KEY_ENV_VAR, + "env-secret-key-long-enough-for-hmac-binding-32b", + ) + }; let result = Mpp::new(Config { recipient: TEST_RECIPIENT.to_string(), @@ -3415,10 +4138,81 @@ mod tests { assert!(result.is_ok()); } + #[test] + fn new_rejects_empty_secret_key() { + // Audit #24: short keys weaken the HMAC binding. + let err = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(String::new()), + ..Default::default() + }) + .err() + .expect("should fail"); + assert!( + err.to_string().contains("Secret key is too short"), + "got: {err}" + ); + } + + #[test] + fn new_rejects_short_secret_key() { + // Just below the 32-byte minimum. + let short = "a".repeat(MIN_SECRET_KEY_BYTES - 1); + let err = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(short), + ..Default::default() + }) + .err() + .expect("should fail"); + assert!( + err.to_string().contains("Secret key is too short"), + "got: {err}" + ); + } + + #[test] + fn new_accepts_secret_key_at_minimum_length() { + let exact = "a".repeat(MIN_SECRET_KEY_BYTES); + let result = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(exact), + ..Default::default() + }); + assert!(result.is_ok()); + } + + #[test] + fn new_rejects_short_env_secret_key() { + // Env-var path must apply the same gate as the explicit-config path. + let _guard = ENV_LOCK.lock().unwrap(); + let prev = std::env::var(SECRET_KEY_ENV_VAR).ok(); + unsafe { std::env::set_var(SECRET_KEY_ENV_VAR, "too-short") }; + + let result = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: None, + ..Default::default() + }); + + if let Some(v) = prev { + unsafe { std::env::set_var(SECRET_KEY_ENV_VAR, v) }; + } else { + unsafe { std::env::remove_var(SECRET_KEY_ENV_VAR) }; + } + + let err = result.err().expect("should fail"); + assert!( + err.to_string().contains("Secret key is too short"), + "got: {err}" + ); + } + #[test] fn new_valid_config_succeeds() { let mpp = test_mpp(); - assert_eq!(mpp.realm(), DEFAULT_REALM); + // Audit #15: default realm now derives from recipient. + assert_eq!(mpp.realm(), derive_default_realm(TEST_RECIPIENT)); assert_eq!(mpp.currency(), "USDC"); assert_eq!(mpp.recipient(), TEST_RECIPIENT); assert_eq!(mpp.decimals(), 6); @@ -3428,7 +4222,7 @@ mod tests { fn new_custom_realm() { let mpp = Mpp::new(Config { recipient: TEST_RECIPIENT.to_string(), - challenge_binding_secret: Some("key".to_string()), + challenge_binding_secret: Some(TEST_SECRET.to_string()), realm: Some("Custom Realm".to_string()), ..Default::default() }) @@ -3436,12 +4230,127 @@ mod tests { assert_eq!(mpp.realm(), "Custom Realm"); } + // ── Audit #15: derived default realm ── + + #[test] + fn new_default_realm_format() { + // The derived default looks like "App Id - #" (max 8). + let realm = derive_default_realm(TEST_RECIPIENT); + assert!( + realm.starts_with("App Id - #"), + "unexpected realm format: {realm}" + ); + let digits = realm.trim_start_matches("App Id - #"); + assert!(digits.chars().all(|c| c.is_ascii_digit()), "got: {realm}"); + assert!(!digits.is_empty() && digits.len() <= 8, "got: {realm}"); + } + + #[test] + fn new_default_realm_deterministic_for_same_recipient() { + // Restart-safe: same recipient must always derive to the same realm, + // otherwise in-flight challenges would fail to verify after a deploy. + let a = derive_default_realm(TEST_RECIPIENT); + let b = derive_default_realm(TEST_RECIPIENT); + assert_eq!(a, b); + } + + #[test] + fn new_default_realm_differs_across_recipients() { + // Two servers with shared secret but different recipients must end + // up with different default realms — closes the audit threat shape + // (shared MPP_SECRET_KEY + shared default realm == shared + // credential namespace). + let other = "8tNDNRkk3JG1WK9NSRwUjytkGwY6Jq6gqQwNFmKt3pkP"; + assert_ne!( + derive_default_realm(TEST_RECIPIENT), + derive_default_realm(other), + ); + } + + #[test] + fn new_rejects_empty_realm() { + // Explicitly providing an empty realm bypasses the derivation — + // reject so an operator can't reintroduce the audit threat with a + // typo. + let err = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + realm: Some(String::new()), + ..Default::default() + }) + .err() + .expect("empty realm should be rejected"); + assert!( + err.to_string().contains("realm must be non-empty"), + "got: {err}" + ); + } + + // ── Audit #37: network allowlist + mainnet canonicalization ── + + #[test] + fn new_accepts_canonical_networks() { + for net in ["mainnet", "devnet", "localnet"] { + Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + network: net.to_string(), + ..Default::default() + }) + .unwrap_or_else(|e| panic!("{net} should be accepted: {e}")); + } + } + + #[test] + fn new_rejects_unknown_network() { + let err = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + network: "testnet".to_string(), + ..Default::default() + }) + .err() + .expect("testnet should be rejected"); + assert!(err.to_string().contains("Unknown network"), "got: {err}"); + } + + #[test] + fn new_rejects_empty_network() { + let err = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + network: String::new(), + ..Default::default() + }) + .err() + .expect("empty network should be rejected"); + assert!( + err.to_string().contains("network must not be empty"), + "got: {err}" + ); + } + + #[test] + fn new_rejects_mainnet_beta_slug() { + // Audit #37: canonicalize on "mainnet" — the legacy "mainnet-beta" + // is an RPC hostname, not a wire-format slug. + let err = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + network: "mainnet-beta".to_string(), + ..Default::default() + }) + .err() + .expect("mainnet-beta should be rejected as a slug"); + assert!(err.to_string().contains("Unknown network"), "got: {err}"); + } + #[test] fn new_custom_rpc_url() { // Should not fail — just verifying it accepts a custom RPC URL. let mpp = Mpp::new(Config { recipient: TEST_RECIPIENT.to_string(), - challenge_binding_secret: Some("key".to_string()), + challenge_binding_secret: Some(TEST_SECRET.to_string()), rpc_url: Some("http://custom:8899".to_string()), ..Default::default() }); @@ -3453,23 +4362,86 @@ mod tests { let store: Arc = Arc::new(MemoryStore::new()); let result = Mpp::new(Config { recipient: TEST_RECIPIENT.to_string(), - challenge_binding_secret: Some("key".to_string()), + challenge_binding_secret: Some(TEST_SECRET.to_string()), store: Some(store), ..Default::default() }); assert!(result.is_ok()); } - // ── default_rpc_url tests ── + // ── resolve_server_token_program tests (sync branches only) ── #[test] - fn default_rpc_url_devnet() { - assert_eq!(default_rpc_url("devnet"), "https://api.devnet.solana.com"); + fn new_resolves_token_program_for_sol_currency() { + let mpp = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + currency: "SOL".to_string(), + decimals: 9, + ..Default::default() + }) + .unwrap(); + assert_eq!(mpp.token_program, None); + } + + #[test] + fn new_resolves_token_program_for_usdc() { + // Default config is USDC. + let mpp = test_mpp(); + assert_eq!(mpp.token_program, Some(programs::TOKEN_PROGRAM)); + } + + #[test] + fn new_resolves_token_program_for_pyusd_token_2022() { + // PYUSD is Token-2022; if this returns the legacy Token Program + // (the old bug), the regression is back. + let mpp = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + currency: "PYUSD".to_string(), + network: "mainnet".to_string(), + ..Default::default() + }) + .unwrap(); + assert_eq!(mpp.token_program, Some(programs::TOKEN_2022_PROGRAM)); + } + + #[test] + fn new_rejects_unparseable_currency_without_rpc() { + // Not a known symbol and not a valid base58 pubkey — must reject + // up front, never silently fall back to the legacy Token Program. + let err = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + currency: "not-a-symbol-or-mint!!".to_string(), + ..Default::default() + }) + .err() + .expect("should fail"); + assert!( + err.to_string().contains("neither a known symbol nor"), + "got: {err}" + ); } + // ── Audit #16: fee_payer without signer ── + #[test] - fn default_rpc_url_localnet() { - assert_eq!(default_rpc_url("localnet"), "http://localhost:8899"); + fn new_rejects_fee_payer_true_without_signer() { + let err = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + fee_payer: true, + fee_payer_signer: None, + network: "devnet".to_string(), + ..Default::default() + }) + .err() + .expect("fee_payer=true without signer should be rejected"); + assert!( + err.to_string().contains("fee_payer_signer is None"), + "got: {err}" + ); } #[test] @@ -3488,21 +4460,91 @@ mod tests { } #[test] - fn default_rpc_url_unknown_defaults_to_mainnet() { - assert_eq!( - default_rpc_url("anything"), - "https://api.mainnet-beta.solana.com" - ); + fn new_accepts_fee_payer_false_without_signer() { + // Regression: the default config has no signer and fee_payer=false; + // it must keep working. + let mpp = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + fee_payer: false, + fee_payer_signer: None, + network: "devnet".to_string(), + ..Default::default() + }) + .expect("default fee_payer=false should be accepted"); + assert!(!mpp.fee_payer); } - // ── charge() and charge_with_options() tests ── - #[test] - fn charge_generates_valid_challenge() { - let mpp = test_mpp(); - let challenge = mpp.charge("0.10").unwrap(); + fn charge_options_rejects_fee_payer_without_signer() { + // Mpp is configured fee_payer=false, no signer. A per-call + // ChargeOptions.fee_payer = true override must be rejected. + let mpp = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + fee_payer: false, + fee_payer_signer: None, + network: "devnet".to_string(), + ..Default::default() + }) + .unwrap(); + let err = mpp + .charge_with_options( + "1.00", + ChargeOptions { + fee_payer: true, + ..Default::default() + }, + ) + .err() + .expect("per-call fee_payer without signer should be rejected"); + assert!( + err.to_string().contains("no fee_payer_signer"), + "got: {err}" + ); + } + + #[test] + fn charge_options_fee_payer_succeeds_when_signer_configured() { + // Happy path: server has a signer, per-call override works. + let mpp = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + fee_payer: false, + fee_payer_signer: Some(test_fee_payer_signer()), + network: "devnet".to_string(), + ..Default::default() + }) + .unwrap(); + let challenge = mpp + .charge_with_options( + "1.00", + ChargeOptions { + fee_payer: true, + ..Default::default() + }, + ) + .expect("per-call fee_payer with signer should succeed"); + let request: ChargeRequest = challenge.request.decode().unwrap(); + let details: MethodDetails = + serde_json::from_value(request.method_details.unwrap()).unwrap(); + assert_eq!(details.fee_payer, Some(true)); + assert!(details.fee_payer_key.is_some(), "feePayerKey must be set"); + } + + // ── default_rpc_url ── + // + // The previous private duplicate is gone; tests for the canonical + // implementation live next to it in `protocol/solana.rs`. + + // ── charge() and charge_with_options() tests ── + + #[test] + fn charge_generates_valid_challenge() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); - assert_eq!(challenge.realm, DEFAULT_REALM); + assert_eq!(challenge.realm, derive_default_realm(TEST_RECIPIENT)); assert_eq!(challenge.method.as_str(), "solana"); assert_eq!(challenge.intent.as_str(), "charge"); assert!(!challenge.id.is_empty()); @@ -3570,16 +4612,21 @@ mod tests { #[test] fn charge_with_options_splits() { let mpp = test_mpp(); + // Audit #21: split recipients must be parseable pubkeys; the old + // fixture strings were placeholders and now correctly fail + // validation. Use real base58 keypairs. + let vendor = Pubkey::new_unique().to_string(); + let processor = Pubkey::new_unique().to_string(); let splits = vec![ crate::protocol::solana::Split { - recipient: "VendorPayoutsWaLLetxxxxxxxxxxxxxxxxxxxxxx1111".to_string(), + recipient: vendor.clone(), amount: "500000".to_string(), ata_creation_required: None, label: None, memo: Some("Vendor payout".to_string()), }, crate::protocol::solana::Split { - recipient: "ProcessorFeeWaLLetxxxxxxxxxxxxxxxxxxxxxxx1111".to_string(), + recipient: processor.clone(), amount: "29000".to_string(), ata_creation_required: None, label: None, @@ -3608,6 +4655,152 @@ mod tests { assert_eq!(splits_arr[1]["amount"], "29000"); } + // ── Audit #21: split validation wired into both server entry points ── + + fn split_helper(recipient: &str, amount: &str) -> crate::protocol::solana::Split { + crate::protocol::solana::Split { + recipient: recipient.to_string(), + amount: amount.to_string(), + ata_creation_required: None, + label: None, + memo: None, + } + } + + #[test] + fn charge_with_options_rejects_invalid_split_recipient() { + let mpp = test_mpp(); + let err = mpp + .charge_with_options( + "1.00", + ChargeOptions { + splits: vec![split_helper("not-a-pubkey!!", "1000")], + ..Default::default() + }, + ) + .err() + .expect("invalid split recipient should be rejected"); + assert!( + format!("{err}").contains("invalid recipient pubkey"), + "got: {err}" + ); + } + + #[test] + fn charge_with_options_rejects_zero_split_amount() { + let mpp = test_mpp(); + let err = mpp + .charge_with_options( + "1.00", + ChargeOptions { + splits: vec![split_helper(&Pubkey::new_unique().to_string(), "0")], + ..Default::default() + }, + ) + .err() + .expect("zero split amount should be rejected"); + assert!(format!("{err}").contains("greater than zero"), "got: {err}"); + } + + #[test] + fn charge_with_options_rejects_duplicate_split_recipient() { + let mpp = test_mpp(); + let dup = Pubkey::new_unique().to_string(); + let err = mpp + .charge_with_options( + "1.00", + ChargeOptions { + splits: vec![split_helper(&dup, "100"), split_helper(&dup, "200")], + ..Default::default() + }, + ) + .err() + .expect("duplicate split recipient should be rejected"); + assert!( + format!("{err}").contains("duplicate recipient"), + "got: {err}" + ); + } + + #[test] + fn charge_with_options_rejects_too_many_splits() { + let mpp = test_mpp(); + let splits: Vec<_> = (0..(crate::protocol::solana::MAX_SPLITS + 1)) + .map(|_| split_helper(&Pubkey::new_unique().to_string(), "1")) + .collect(); + let err = mpp + .charge_with_options( + "1.00", + ChargeOptions { + splits, + ..Default::default() + }, + ) + .err() + .expect("too many splits should be rejected"); + assert!(matches!(err, Error::TooManySplits)); + } + + #[test] + fn charge_with_options_rejects_primary_recipient_with_ata_creation_required() { + // Audit #38: a split whose recipient duplicates the top-level + // recipient AND requests `ataCreationRequired: true` is the misconfig + // shape that, in fee-sponsored mode, lets the primary recipient drain + // server-funded ATA rent by closing/recreating its own ATA. + let mpp = test_mpp(); + let splits = vec![crate::protocol::solana::Split { + recipient: TEST_RECIPIENT.to_string(), + amount: "10000".to_string(), + ata_creation_required: Some(true), + label: None, + memo: None, + }]; + let err = mpp + .charge_with_options( + "1.00", + ChargeOptions { + splits, + ..Default::default() + }, + ) + .err() + .expect("should reject primary recipient with ataCreationRequired"); + let msg = err.to_string(); + assert!( + msg.contains("top-level recipient"), + "unexpected error: {msg}" + ); + } + + #[test] + fn charge_with_options_allows_primary_recipient_in_splits_without_ata_creation() { + // Legitimate use case the audit recommendation would have over-banned: + // the merchant takes part of the funds as a regular split alongside + // other payees. Allowed as long as the ATA-creation flag isn't set. + let mpp = test_mpp(); + let splits = vec![crate::protocol::solana::Split { + recipient: TEST_RECIPIENT.to_string(), + amount: "10000".to_string(), + ata_creation_required: None, + label: None, + memo: Some("merchant cut".to_string()), + }]; + let challenge = mpp + .charge_with_options( + "1.00", + ChargeOptions { + splits, + ..Default::default() + }, + ) + .expect("primary recipient as a regular split is allowed"); + let request: ChargeRequest = challenge.request.decode().unwrap(); + let details = request.method_details.unwrap(); + let splits_arr = details.get("splits").unwrap().as_array().unwrap(); + assert_eq!(splits_arr.len(), 1); + assert_eq!(splits_arr[0]["recipient"], TEST_RECIPIENT); + } + #[test] fn charge_with_options_no_splits_omitted() { let mpp = test_mpp(); @@ -3694,6 +4887,124 @@ mod tests { assert_eq!(challenge.description.as_deref(), Some("Premium access")); } + // ── charge_challenge validation (audit #19) ── + + #[test] + fn charge_challenge_rejects_mismatched_currency() { + let mpp = test_mpp(); // USDC + let request = ChargeRequest { + amount: "100".to_string(), + currency: "USDT".to_string(), + recipient: Some(TEST_RECIPIENT.to_string()), + ..Default::default() + }; + let err = mpp.charge_challenge(&request).unwrap_err(); + assert!(format!("{err}").contains("does not match server-configured currency")); + } + + #[test] + fn charge_challenge_rejects_missing_recipient() { + let mpp = test_mpp(); + let request = ChargeRequest { + amount: "100".to_string(), + currency: "USDC".to_string(), + recipient: None, + ..Default::default() + }; + let err = mpp.charge_challenge(&request).unwrap_err(); + assert!(format!("{err}").contains("recipient is required")); + } + + #[test] + fn charge_challenge_rejects_invalid_recipient() { + let mpp = test_mpp(); + let request = ChargeRequest { + amount: "100".to_string(), + currency: "USDC".to_string(), + recipient: Some("not-a-pubkey!!".to_string()), + ..Default::default() + }; + let err = mpp.charge_challenge(&request).unwrap_err(); + assert!(format!("{err}").contains("Invalid recipient pubkey")); + } + + #[test] + fn charge_challenge_rejects_unparseable_amount() { + let mpp = test_mpp(); + let request = ChargeRequest { + amount: "abc".to_string(), + currency: "USDC".to_string(), + recipient: Some(TEST_RECIPIENT.to_string()), + ..Default::default() + }; + let err = mpp.charge_challenge(&request).unwrap_err(); + assert!(format!("{err}").contains("Invalid amount")); + } + + #[test] + fn charge_challenge_rejects_mismatched_network_in_method_details() { + let mpp = test_mpp(); // network: devnet + let md = MethodDetails { + network: Some("mainnet".to_string()), + ..Default::default() + }; + let request = ChargeRequest { + amount: "100".to_string(), + currency: "USDC".to_string(), + recipient: Some(TEST_RECIPIENT.to_string()), + method_details: Some(serde_json::to_value(md).unwrap()), + ..Default::default() + }; + let err = mpp.charge_challenge(&request).unwrap_err(); + assert!(format!("{err}").contains("does not match server-configured network")); + } + + #[test] + fn charge_challenge_rejects_mismatched_token_program() { + let mpp = test_mpp(); // USDC -> TOKEN_PROGRAM + let md = MethodDetails { + token_program: Some(programs::TOKEN_2022_PROGRAM.to_string()), + ..Default::default() + }; + let request = ChargeRequest { + amount: "100".to_string(), + currency: "USDC".to_string(), + recipient: Some(TEST_RECIPIENT.to_string()), + method_details: Some(serde_json::to_value(md).unwrap()), + ..Default::default() + }; + let err = mpp.charge_challenge(&request).unwrap_err(); + assert!(format!("{err}").contains("does not match server-resolved token program")); + } + + #[test] + fn charge_challenge_rejects_invalid_split_recipient() { + let mpp = test_mpp(); + let md = MethodDetails { + splits: Some(vec![Split { + recipient: "not-a-pubkey!!".to_string(), + amount: "10".to_string(), + ata_creation_required: None, + label: None, + memo: None, + }]), + ..Default::default() + }; + let request = ChargeRequest { + amount: "100".to_string(), + currency: "USDC".to_string(), + recipient: Some(TEST_RECIPIENT.to_string()), + method_details: Some(serde_json::to_value(md).unwrap()), + ..Default::default() + }; + let err = mpp.charge_challenge(&request).unwrap_err(); + // Audit #21 unified the error string via the shared validate_splits helper. + assert!( + format!("{err}").contains("splits[0]: invalid recipient pubkey"), + "got: {err}" + ); + } + // ── Challenge HMAC verification tests ── #[test] @@ -3875,12 +5186,13 @@ mod tests { assert!(err.message.contains("Invalid credential payload")); } - // ── verify_credential() tests ── + // ── verify() tier-1 (HMAC) tests ── #[tokio::test(flavor = "multi_thread")] - async fn verify_credential_rejects_tampered_id() { + async fn verify_rejects_tampered_id() { let mpp = test_mpp(); let challenge = mpp.charge("0.10").unwrap(); + let request: ChargeRequest = challenge.request.decode().unwrap(); let mut cred = PaymentCredential { challenge: challenge.to_echo(), source: None, @@ -3888,10 +5200,44 @@ mod tests { }; cred.challenge.id = "bad".to_string(); - let err = mpp.verify_credential(&cred).await.unwrap_err(); + let err = mpp.verify(&cred, &request).await.unwrap_err(); assert_eq!(err.code, Some("malformed-credential")); } + /// Audit #22: `verify` must reject when the caller-supplied request + /// diverges from the request HMAC-authenticated by the credential — + /// otherwise direct callers could authenticate one request shape and + /// settle against a different one. + #[tokio::test(flavor = "multi_thread")] + async fn verify_rejects_request_diverging_from_credential() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); // credential carries amount=100000 + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + + // Caller passes a request with a different amount than what the + // credential's HMAC-authenticated request carries. HMAC tier-1 + // still passes (we didn't tamper with the credential), so the + // audit #22 binding check is the only thing that catches the + // divergence. + let divergent = ChargeRequest { + amount: "999999".to_string(), + currency: "USDC".to_string(), + recipient: Some(TEST_RECIPIENT.to_string()), + ..Default::default() + }; + + let err = mpp.verify(&cred, &divergent).await.unwrap_err(); + assert_eq!(err.code, Some("malformed-credential")); + assert!( + err.message.contains("Amount mismatch"), + "expected amount mismatch from the binding check, got: {err:?}" + ); + } + // ── verify_credential_with_expected() tests ── #[tokio::test(flavor = "multi_thread")] @@ -3915,12 +5261,259 @@ mod tests { .verify_credential_with_expected(&cred, &expected) .await .unwrap_err(); - assert_eq!(err.code, Some("malformed-credential")); - assert!(err.message.contains("Amount mismatch")); + assert_eq!(err.code, Some("malformed-credential")); + assert!(err.message.contains("Amount mismatch")); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_currency_mismatch() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + + let expected = ChargeRequest { + amount: "100000".to_string(), + currency: "SOL".to_string(), // mismatch: challenge has USDC + recipient: Some(TEST_RECIPIENT.to_string()), + ..Default::default() + }; + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert_eq!(err.code, Some("malformed-credential")); + assert!(err.message.contains("Currency mismatch")); + } + + // Audit #1: every payment-constraining field comparison. + + fn expected_from_challenge(challenge: &PaymentChallenge) -> ChargeRequest { + challenge.request.decode().unwrap() + } + + fn mutate_method_details( + req: &mut ChargeRequest, + f: impl FnOnce(&mut crate::protocol::solana::MethodDetails), + ) { + use crate::protocol::solana::MethodDetails; + let mut md: MethodDetails = req + .method_details + .as_ref() + .map(|v| serde_json::from_value(v.clone()).unwrap_or_default()) + .unwrap_or_default(); + f(&mut md); + req.method_details = Some(serde_json::to_value(&md).unwrap()); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_external_id_mismatch() { + let mpp = test_mpp(); + let challenge = mpp + .charge_with_options( + "0.10", + ChargeOptions { + external_id: Some("order-1"), + ..Default::default() + }, + ) + .unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + let mut expected = expected_from_challenge(&challenge); + expected.external_id = Some("order-2".to_string()); + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert!(err.message.contains("externalId mismatch"), "got: {err:?}"); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_description_mismatch() { + let mpp = test_mpp(); + let challenge = mpp + .charge_with_options( + "0.10", + ChargeOptions { + description: Some("A"), + ..Default::default() + }, + ) + .unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + let mut expected = expected_from_challenge(&challenge); + expected.description = Some("B".to_string()); + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert!(err.message.contains("description mismatch"), "got: {err:?}"); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_network_mismatch() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + let mut expected = expected_from_challenge(&challenge); + mutate_method_details(&mut expected, |md| md.network = Some("mainnet".into())); + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert!( + err.message.contains("methodDetails.network mismatch"), + "got: {err:?}" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_decimals_mismatch() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + let mut expected = expected_from_challenge(&challenge); + mutate_method_details(&mut expected, |md| md.decimals = Some(9)); + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert!( + err.message.contains("methodDetails.decimals mismatch"), + "got: {err:?}" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_token_program_mismatch() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + let mut expected = expected_from_challenge(&challenge); + mutate_method_details(&mut expected, |md| { + md.token_program = Some("TokenzQdBNbLqP5VEhdkAS6EPFLC1PHnBqCXEpPxuEb".into()) + }); + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert!( + err.message.contains("methodDetails.tokenProgram mismatch"), + "got: {err:?}" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_fee_payer_mismatch() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + let mut expected = expected_from_challenge(&challenge); + mutate_method_details(&mut expected, |md| md.fee_payer = Some(true)); + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert!( + err.message.contains("methodDetails.feePayer mismatch"), + "got: {err:?}" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_fee_payer_key_mismatch() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + let mut expected = expected_from_challenge(&challenge); + mutate_method_details(&mut expected, |md| { + md.fee_payer_key = Some(Pubkey::new_unique().to_string()) + }); + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert!( + err.message.contains("methodDetails.feePayerKey mismatch"), + "got: {err:?}" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_credential_with_expected_splits_mismatch() { + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({"type": "signature", "signature": "x"}), + }; + let mut expected = expected_from_challenge(&challenge); + mutate_method_details(&mut expected, |md| { + md.splits = Some(vec![crate::protocol::solana::Split { + recipient: Pubkey::new_unique().to_string(), + amount: "1".to_string(), + ata_creation_required: None, + label: None, + memo: None, + }]) + }); + + let err = mpp + .verify_credential_with_expected(&cred, &expected) + .await + .unwrap_err(); + assert!( + err.message.contains("methodDetails.splits mismatch"), + "got: {err:?}" + ); } + /// Audit #1: `recent_blockhash` is per-challenge state, not per-route + /// policy. A mismatch must NOT trigger a rejection, otherwise honest + /// flows (where the route's expected has no blockhash and the + /// credential's request has a fresh one) would break. #[tokio::test(flavor = "multi_thread")] - async fn verify_credential_with_expected_currency_mismatch() { + async fn verify_credential_with_expected_ignores_recent_blockhash() { let mpp = test_mpp(); let challenge = mpp.charge("0.10").unwrap(); let cred = PaymentCredential { @@ -3928,20 +5521,22 @@ mod tests { source: None, payload: serde_json::json!({"type": "signature", "signature": "x"}), }; - - let expected = ChargeRequest { - amount: "100000".to_string(), - currency: "SOL".to_string(), // mismatch: challenge has USDC - recipient: Some(TEST_RECIPIENT.to_string()), - ..Default::default() - }; + let mut expected = expected_from_challenge(&challenge); + // Strip the blockhash from `expected` even though the credential + // carries one. The comparison must pass; downstream `verify` will + // fail on the dummy signature payload, which is fine — we only care + // that we got *past* the comparison layer. + mutate_method_details(&mut expected, |md| md.recent_blockhash = None); let err = mpp .verify_credential_with_expected(&cred, &expected) .await .unwrap_err(); - assert_eq!(err.code, Some("malformed-credential")); - assert!(err.message.contains("Currency mismatch")); + let msg = format!("{}", err.message); + assert!( + !msg.contains("recentBlockhash mismatch") && !msg.contains("recent_blockhash mismatch"), + "comparison should not reject on blockhash, got: {err:?}" + ); } #[tokio::test(flavor = "multi_thread")] @@ -3994,8 +5589,12 @@ mod tests { .verify_credential_with_expected(&cred, &expected) .await .unwrap_err(); + // Audit #1 now catches the bad expected.method_details at the + // up-front comparison layer (before settlement); the error string + // changed accordingly. The point of the test still holds: `expected` + // (not the credential's request) is being parsed. assert!( - err.message.contains("Invalid method details"), + err.message.contains("Invalid expected methodDetails"), "expected `expected` request to be parsed, got: {err:?}" ); } @@ -4004,8 +5603,9 @@ mod tests { // // Each test forges a credential where one pinned field differs from what // the server has configured, then re-signs the HMAC so Tier-1 passes. The - // Tier-2 backstop must reject every case even via the simple - // `verify_credential` API. + // Tier-2 backstop must reject every case. Called via `verify` directly + // (the lowest-level public API) so the pinned-field layer is exercised + // in isolation regardless of the higher-level convenience entry points. fn resign_challenge( secret: &str, @@ -4028,6 +5628,7 @@ mod tests { async fn tier2_rejects_tampered_realm() { let mpp = test_mpp(); let challenge = mpp.charge("0.10").unwrap(); + let request: ChargeRequest = challenge.request.decode().unwrap(); let mut echo = challenge.to_echo(); echo.realm = "Attacker Realm".to_string(); // HMAC uses the *server's* realm, not the echoed one, so re-signing @@ -4039,7 +5640,7 @@ mod tests { source: None, payload: serde_json::json!({"type": "signature", "signature": "x"}), }; - let err = mpp.verify_credential(&cred).await.unwrap_err(); + let err = mpp.verify(&cred, &request).await.unwrap_err(); assert_eq!(err.code, Some("malformed-credential")); assert!(err.message.to_lowercase().contains("realm"), "got: {err:?}"); } @@ -4061,7 +5662,7 @@ mod tests { source: None, payload: serde_json::json!({"type": "signature", "signature": "x"}), }; - let err = mpp.verify_credential(&cred).await.unwrap_err(); + let err = mpp.verify(&cred, &request).await.unwrap_err(); assert_eq!(err.code, Some("malformed-credential")); assert!(err.message.contains("currency"), "got: {err:?}"); } @@ -4083,7 +5684,7 @@ mod tests { source: None, payload: serde_json::json!({"type": "signature", "signature": "x"}), }; - let err = mpp.verify_credential(&cred).await.unwrap_err(); + let err = mpp.verify(&cred, &request).await.unwrap_err(); assert_eq!(err.code, Some("malformed-credential")); assert!(err.message.contains("recipient"), "got: {err:?}"); } @@ -4092,6 +5693,7 @@ mod tests { async fn tier2_rejects_tampered_method() { let mpp = test_mpp(); let challenge = mpp.charge("0.10").unwrap(); + let request: ChargeRequest = challenge.request.decode().unwrap(); let mut echo = challenge.to_echo(); echo.method = "stripe".into(); resign_challenge(TEST_SECRET, &mpp.realm, &mut echo); @@ -4101,7 +5703,7 @@ mod tests { source: None, payload: serde_json::json!({"type": "signature", "signature": "x"}), }; - let err = mpp.verify_credential(&cred).await.unwrap_err(); + let err = mpp.verify(&cred, &request).await.unwrap_err(); assert_eq!(err.code, Some("malformed-credential")); assert!(err.message.contains("method"), "got: {err:?}"); } @@ -4110,6 +5712,7 @@ mod tests { async fn tier2_rejects_non_charge_intent() { let mpp = test_mpp(); let challenge = mpp.charge("0.10").unwrap(); + let request: ChargeRequest = challenge.request.decode().unwrap(); let mut echo = challenge.to_echo(); echo.intent = "session".into(); resign_challenge(TEST_SECRET, &mpp.realm, &mut echo); @@ -4119,11 +5722,78 @@ mod tests { source: None, payload: serde_json::json!({"type": "signature", "signature": "x"}), }; - let err = mpp.verify_credential(&cred).await.unwrap_err(); + let err = mpp.verify(&cred, &request).await.unwrap_err(); assert_eq!(err.code, Some("malformed-credential")); assert!(err.message.contains("intent"), "got: {err:?}"); } + // ── Audit #5: push-mode acceptance is opt-in ── + // + // Spec §13.5: push mode matches by shape; any matching-shape on-chain + // transaction can claim any matching-shape challenge. Gate runs before + // B34 (which catches the narrower fee-payer-route case). + + #[tokio::test(flavor = "multi_thread")] + async fn verify_rejects_push_credential_when_accept_push_mode_off() { + // Default config: accept_push_mode is false. No fee-sponsor either, + // so B34 wouldn't fire — only the audit #5 gate should reject. + let mpp = test_mpp(); + let challenge = mpp.charge("0.10").unwrap(); + let request: ChargeRequest = challenge.request.decode().unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({ + "type": "signature", + "signature": "3yZe7d2X1bxYjP6kJtNJzC8mFqLgK6vQ9zR3hT5wXdAfVjY8nW1qB4uHpM2sC3rTzJtNeWfDqRmKxYjP6kJtNJzC", + }), + }; + + let err = mpp.verify(&cred, &request).await.unwrap_err(); + assert_eq!(err.code, Some("malformed-credential")); + assert!( + err.message.contains("Push-mode credentials are disabled"), + "got: {err:?}" + ); + // The error message should also point at the spec for ops triage. + assert!( + err.message.contains("§13.5"), + "expected spec §13.5 callout, got: {err:?}" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn verify_passes_audit_5_gate_when_accept_push_mode_on() { + // Opt in. The audit #5 gate should NOT fire — any later error + // (e.g. on-chain verification against a fake signature) is fine, + // just not the "Push-mode credentials are disabled" one. + let mpp = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + currency: crate::protocol::solana::mints::USDC_DEVNET.to_string(), + network: "devnet".to_string(), + accept_push_mode: true, + ..Default::default() + }) + .unwrap(); + let challenge = mpp.charge("0.10").unwrap(); + let request: ChargeRequest = challenge.request.decode().unwrap(); + let cred = PaymentCredential { + challenge: challenge.to_echo(), + source: None, + payload: serde_json::json!({ + "type": "signature", + "signature": "3yZe7d2X1bxYjP6kJtNJzC8mFqLgK6vQ9zR3hT5wXdAfVjY8nW1qB4uHpM2sC3rTzJtNeWfDqRmKxYjP6kJtNJzC", + }), + }; + + let err = mpp.verify(&cred, &request).await.unwrap_err(); + assert!( + !err.message.contains("Push-mode credentials are disabled"), + "audit #5 gate should not fire when opted in: {err:?}" + ); + } + // ── B34: push-mode credentials rejected on fee-payer routes ── // // A signature-only credential references an already-landed transaction @@ -4137,6 +5807,9 @@ mod tests { #[tokio::test(flavor = "multi_thread")] async fn b34_rejects_push_credential_on_fee_payer_route() { + // Audit #5 added an earlier `accept_push_mode` gate. To exercise + // the B34 fee-payer-specific path in isolation, opt push mode in + // here so the audit #5 gate passes and B34 fires. let mpp = Mpp::new(Config { recipient: TEST_RECIPIENT.to_string(), challenge_binding_secret: Some(TEST_SECRET.to_string()), @@ -4144,6 +5817,7 @@ mod tests { fee_payer: true, fee_payer_signer: Some(test_fee_payer_signer()), network: "devnet".to_string(), + accept_push_mode: true, ..Default::default() }) .unwrap(); @@ -4348,74 +6022,154 @@ mod tests { fn find_sol_transfer_success() { let mut matched = HashSet::new(); let instructions = vec![serde_json::json!({ + "program": "system", "parsed": { "type": "transfer", "info": { + "source": "PayerPubkey", "destination": "RecipientPubkey", "lamports": 1000000 } } })]; - assert!( - find_sol_transfer(&instructions, "RecipientPubkey", 1_000_000, &mut matched).is_ok() - ); + assert!(find_sol_transfer( + &instructions, + "RecipientPubkey", + 1_000_000, + None, + &mut matched + ) + .is_ok()); } #[test] fn find_sol_transfer_wrong_amount() { let mut matched = HashSet::new(); let instructions = vec![serde_json::json!({ + "program": "system", "parsed": { "type": "transfer", "info": { + "source": "PayerPubkey", "destination": "RecipientPubkey", "lamports": 500000 } } })]; - assert!( - find_sol_transfer(&instructions, "RecipientPubkey", 1_000_000, &mut matched).is_err() - ); + assert!(find_sol_transfer( + &instructions, + "RecipientPubkey", + 1_000_000, + None, + &mut matched + ) + .is_err()); } #[test] fn find_sol_transfer_wrong_recipient() { let mut matched = HashSet::new(); let instructions = vec![serde_json::json!({ + "program": "system", "parsed": { "type": "transfer", "info": { + "source": "PayerPubkey", "destination": "WrongPubkey", "lamports": 1000000 } } })]; - assert!( - find_sol_transfer(&instructions, "RecipientPubkey", 1_000_000, &mut matched).is_err() - ); + assert!(find_sol_transfer( + &instructions, + "RecipientPubkey", + 1_000_000, + None, + &mut matched + ) + .is_err()); } #[test] fn find_sol_transfer_empty_instructions() { let mut matched = HashSet::new(); - assert!(find_sol_transfer(&[], "RecipientPubkey", 1_000_000, &mut matched).is_err()); + assert!(find_sol_transfer(&[], "RecipientPubkey", 1_000_000, None, &mut matched).is_err()); } #[test] fn find_sol_transfer_ignores_non_transfer_types() { let mut matched = HashSet::new(); let instructions = vec![serde_json::json!({ + "program": "system", "parsed": { "type": "createAccount", "info": { + "source": "PayerPubkey", "destination": "RecipientPubkey", "lamports": 1000000 } } })]; - assert!( - find_sol_transfer(&instructions, "RecipientPubkey", 1_000_000, &mut matched).is_err() - ); + assert!(find_sol_transfer( + &instructions, + "RecipientPubkey", + 1_000_000, + None, + &mut matched + ) + .is_err()); + } + + #[test] + fn find_sol_transfer_rejects_non_system_program() { + let mut matched = HashSet::new(); + // A "transfer" with a lamports field, but on the wrong program. The + // legacy implementation matched on parsed.type + info.lamports alone + // and would accept this; the hardened implementation must not. + let instructions = vec![serde_json::json!({ + "programId": programs::TOKEN_PROGRAM, + "parsed": { + "type": "transfer", + "info": { + "source": "PayerPubkey", + "destination": "RecipientPubkey", + "lamports": 1_000_000 + } + } + })]; + assert!(find_sol_transfer( + &instructions, + "RecipientPubkey", + 1_000_000, + None, + &mut matched + ) + .is_err()); + } + + #[test] + fn find_sol_transfer_rejects_source_equals_fee_payer() { + let mut matched = HashSet::new(); + let instructions = vec![serde_json::json!({ + "program": "system", + "parsed": { + "type": "transfer", + "info": { + "source": "FeePayerPubkey", + "destination": "RecipientPubkey", + "lamports": 1_000_000 + } + } + })]; + let err = find_sol_transfer( + &instructions, + "RecipientPubkey", + 1_000_000, + Some("FeePayerPubkey"), + &mut matched, + ) + .unwrap_err(); + assert!(err.message.contains("Fee payer cannot fund")); } #[test] @@ -4424,18 +6178,22 @@ mod tests { let split_recipient = "SplitRecipient"; let instructions = vec![ serde_json::json!({ + "program": "system", "parsed": { "type": "transfer", "info": { + "source": "PayerPubkey", "destination": primary_recipient, "lamports": 800000 } } }), serde_json::json!({ + "program": "system", "parsed": { "type": "transfer", "info": { + "source": "PayerPubkey", "destination": split_recipient, "lamports": 200000 } @@ -4451,15 +6209,19 @@ mod tests { memo: None, }]; - assert!(verify_sol_transfers(&instructions, primary_recipient, 800000, &splits).is_ok()); + assert!( + verify_sol_transfers(&instructions, primary_recipient, 800000, &splits, None).is_ok() + ); } #[test] fn verify_sol_transfers_missing_split() { let instructions = vec![serde_json::json!({ + "program": "system", "parsed": { "type": "transfer", "info": { + "source": "PayerPubkey", "destination": "PrimaryRecipient", "lamports": 800000 } @@ -4474,8 +6236,8 @@ mod tests { memo: None, }]; - let err = - verify_sol_transfers(&instructions, "PrimaryRecipient", 800000, &splits).unwrap_err(); + let err = verify_sol_transfers(&instructions, "PrimaryRecipient", 800000, &splits, None) + .unwrap_err(); assert!(err.message.contains("Missing split transfer")); } @@ -4483,18 +6245,22 @@ mod tests { fn verify_sol_transfers_rejects_reusing_single_instruction_for_duplicate_splits() { let instructions = vec![ serde_json::json!({ + "program": "system", "parsed": { "type": "transfer", "info": { + "source": "PayerPubkey", "destination": "PrimaryRecipient", "lamports": 800000 } } }), serde_json::json!({ + "program": "system", "parsed": { "type": "transfer", "info": { + "source": "PayerPubkey", "destination": "SplitRecipient", "lamports": 100000 } @@ -4519,8 +6285,8 @@ mod tests { }, ]; - let err = - verify_sol_transfers(&instructions, "PrimaryRecipient", 800000, &splits).unwrap_err(); + let err = verify_sol_transfers(&instructions, "PrimaryRecipient", 800000, &splits, None) + .unwrap_err(); assert!(err.message.contains("Missing split transfer")); } @@ -4732,9 +6498,16 @@ mod tests { } })]; - assert!( - find_spl_transfer(&instructions, owner, mint, 1_000_000, None, &mut matched).is_ok() - ); + assert!(find_spl_transfer( + &instructions, + owner, + mint, + 1_000_000, + None, + None, + &mut matched + ) + .is_ok()); } #[test] @@ -4759,6 +6532,7 @@ mod tests { "SomeMint", 1_000_000, None, + None, &mut matched ) .is_err()); @@ -4786,6 +6560,7 @@ mod tests { "SomeMint", 1_000_000, None, + None, &mut matched ) .is_err()); @@ -4828,6 +6603,7 @@ mod tests { wrong_mint, 1_000_000, None, + None, &mut matched ) .is_err()); @@ -4900,11 +6676,106 @@ mod tests { }, ]; - let err = - verify_spl_transfers(&instructions, owner, mint, 800000, &splits, None).unwrap_err(); + let err = verify_spl_transfers(&instructions, owner, mint, 800000, &splits, None, None) + .unwrap_err(); assert!(err.message.contains("Missing split SPL transfer")); } + #[test] + fn find_spl_transfer_rejects_authority_equals_fee_payer() { + let mut matched = HashSet::new(); + let owner = "CXhrFZJLKqjzmP3sjYLcF4dTeXWKCy9e2SXXZ2Yo6MPY"; + let mint = "EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v"; + let fee_payer = "9XHRopERTd4LfQ8b6e3p9bN2WhxgQzDxFRtbq1XwQ4mP"; + let tp = programs::TOKEN_PROGRAM; + + let owner_pk = Pubkey::from_str(owner).unwrap(); + let mint_pk = Pubkey::from_str(mint).unwrap(); + let tp_pk = Pubkey::from_str(tp).unwrap(); + let ata_program = Pubkey::from_str(programs::ASSOCIATED_TOKEN_PROGRAM).unwrap(); + let (dest_ata, _) = Pubkey::find_program_address( + &[owner_pk.as_ref(), tp_pk.as_ref(), mint_pk.as_ref()], + &ata_program, + ); + + let instructions = vec![serde_json::json!({ + "programId": tp, + "parsed": { + "type": "transferChecked", + "info": { + "source": "SomeSourceAta1111111111111111111111111111111", + "authority": fee_payer, + "destination": dest_ata.to_string(), + "mint": mint, + "tokenAmount": { "amount": "1000000" } + } + } + })]; + + let err = find_spl_transfer( + &instructions, + owner, + mint, + 1_000_000, + None, + Some(fee_payer), + &mut matched, + ) + .unwrap_err(); + assert!(err.message.contains("Fee payer cannot authorize")); + } + + #[test] + fn find_spl_transfer_rejects_source_equals_fee_payer_ata() { + let mut matched = HashSet::new(); + let owner = "CXhrFZJLKqjzmP3sjYLcF4dTeXWKCy9e2SXXZ2Yo6MPY"; + let mint = "EPjFWdd5AufqSSqeM2qN1xzybapC8G4wEGGkZwyTDt1v"; + let fee_payer = "9XHRopERTd4LfQ8b6e3p9bN2WhxgQzDxFRtbq1XwQ4mP"; + let tp = programs::TOKEN_PROGRAM; + + let owner_pk = Pubkey::from_str(owner).unwrap(); + let fee_payer_pk = Pubkey::from_str(fee_payer).unwrap(); + let mint_pk = Pubkey::from_str(mint).unwrap(); + let tp_pk = Pubkey::from_str(tp).unwrap(); + let ata_program = Pubkey::from_str(programs::ASSOCIATED_TOKEN_PROGRAM).unwrap(); + let (dest_ata, _) = Pubkey::find_program_address( + &[owner_pk.as_ref(), tp_pk.as_ref(), mint_pk.as_ref()], + &ata_program, + ); + let (fee_payer_ata, _) = Pubkey::find_program_address( + &[fee_payer_pk.as_ref(), tp_pk.as_ref(), mint_pk.as_ref()], + &ata_program, + ); + + let instructions = vec![serde_json::json!({ + "programId": tp, + "parsed": { + "type": "transferChecked", + "info": { + "source": fee_payer_ata.to_string(), + // Authority is a different account (e.g. a delegate) so the + // first check passes; the source-ATA check must still fire. + "authority": owner, + "destination": dest_ata.to_string(), + "mint": mint, + "tokenAmount": { "amount": "1000000" } + } + } + })]; + + let err = find_spl_transfer( + &instructions, + owner, + mint, + 1_000_000, + None, + Some(fee_payer), + &mut matched, + ) + .unwrap_err(); + assert!(err.message.contains("Fee payer token account cannot fund")); + } + #[test] fn parsed_allowlist_rejects_extra_spl_transfer_after_required_transfer() { let owner = "CXhrFZJLKqjzmP3sjYLcF4dTeXWKCy9e2SXXZ2Yo6MPY"; @@ -4943,7 +6814,8 @@ mod tests { }), ]; let matched = - verify_spl_transfers(&instructions, owner, mint, 1_000_000, &[], Some(tp)).unwrap(); + verify_spl_transfers(&instructions, owner, mint, 1_000_000, &[], Some(tp), None) + .unwrap(); let allowed_ata_owners = HashSet::from([owner.to_string()]); let required_ata_owners = HashSet::new(); @@ -5444,6 +7316,8 @@ mod tests { recipient: TEST_RECIPIENT.to_string(), challenge_binding_secret: Some(TEST_SECRET.to_string()), fee_payer: true, + // Audit #16: signer is now required alongside fee_payer = true. + fee_payer_signer: Some(test_fee_payer_signer()), network: "devnet".to_string(), ..Default::default() }) @@ -5458,7 +7332,16 @@ mod tests { #[test] fn charge_options_fee_payer_flag() { - let mpp = test_mpp(); + // Audit #16: per-call ChargeOptions.fee_payer requires the server + // to have a signer configured. + let mpp = Mpp::new(Config { + recipient: TEST_RECIPIENT.to_string(), + challenge_binding_secret: Some(TEST_SECRET.to_string()), + fee_payer_signer: Some(test_fee_payer_signer()), + network: "devnet".to_string(), + ..Default::default() + }) + .unwrap(); let challenge = mpp .charge_with_options( "1.00", diff --git a/rust/crates/mpp/src/server/subscription.rs b/rust/crates/mpp/src/server/subscription.rs index decaa9b1..d4985edc 100644 --- a/rust/crates/mpp/src/server/subscription.rs +++ b/rust/crates/mpp/src/server/subscription.rs @@ -1170,7 +1170,6 @@ mod tests { #[test] fn default_rpc_url_covers_each_network() { assert_eq!(default_rpc_url("devnet"), "https://api.devnet.solana.com"); - assert_eq!(default_rpc_url("testnet"), "https://api.testnet.solana.com"); assert_eq!(default_rpc_url("localnet"), "http://localhost:8899"); // `mainnet` is the canonical slug; the legacy `mainnet-beta` // alias still resolves so existing clients don't break. diff --git a/rust/crates/mpp/tests/charge_integration.rs b/rust/crates/mpp/tests/charge_integration.rs index a4b41f3d..4c655084 100644 --- a/rust/crates/mpp/tests/charge_integration.rs +++ b/rust/crates/mpp/tests/charge_integration.rs @@ -52,6 +52,34 @@ async fn wait_for_surfnet(surfnet: &Surfnet) { panic!("surfnet rpc did not become ready in time"); } +/// Build the `expected` ChargeRequest for an integration test from its +/// known static configuration. Mirrors how SDK consumers should construct +/// the expected request from their route configuration rather than from +/// the credential itself (audit #2). Includes the `methodDetails` fields +/// audit #1 now compares exhaustively (network, decimals, token program). +fn expected_charge( + amount_base_units: &str, + currency: &str, + recipient: &str, + network: &str, + decimals: u8, + token_program: Option<&str>, +) -> solana_mpp::ChargeRequest { + let md = solana_mpp::protocol::solana::MethodDetails { + network: Some(network.to_string()), + decimals: Some(decimals), + token_program: token_program.map(|s| s.to_string()), + ..Default::default() + }; + solana_mpp::ChargeRequest { + amount: amount_base_units.to_string(), + currency: currency.to_string(), + recipient: Some(recipient.to_string()), + method_details: Some(serde_json::to_value(md).unwrap()), + ..Default::default() + } +} + // ─── SOL charge flow ─────────────────────────────────────────────────── #[tokio::test(flavor = "multi_thread")] @@ -71,7 +99,9 @@ async fn sol_charge_full_flow() { decimals: 9, network: "localnet".to_string(), rpc_url: Some(surfnet.rpc_url().to_string()), - challenge_binding_secret: Some("test-secret".to_string()), + challenge_binding_secret: Some( + "test-secret-key-for-integration-tests-32b-padding".to_string(), + ), ..Default::default() }) .unwrap(); @@ -90,9 +120,20 @@ async fn sol_charge_full_flow() { assert!(auth_header.starts_with("Payment ")); - // Verify credential. + // Verify credential. `expected` mirrors the route's static config. + let expected = expected_charge( + "1000000", + "SOL", + &recipient.pubkey().to_string(), + "localnet", + 9, + None, + ); let receipt = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth_header).unwrap()) + .verify_credential_with_expected( + &solana_mpp::parse_authorization(&auth_header).unwrap(), + &expected, + ) .await .expect("verify credential"); assert_eq!(receipt.status.to_string(), "success"); @@ -116,7 +157,9 @@ async fn sol_charge_wrong_amount_rejected_before_broadcast() { decimals: 9, network: "localnet".to_string(), rpc_url: Some(surfnet.rpc_url().to_string()), - challenge_binding_secret: Some("test-secret".to_string()), + challenge_binding_secret: Some( + "test-secret-key-for-integration-tests-32b-padding".to_string(), + ), ..Default::default() }) .unwrap(); @@ -148,8 +191,19 @@ async fn sol_charge_wrong_amount_rejected_before_broadcast() { let auth = format_authorization(&credential).unwrap(); // Server should reject BEFORE broadcasting. + let expected = expected_charge( + "1000000", + "SOL", + &recipient.pubkey().to_string(), + "localnet", + 9, + None, + ); let err = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth).unwrap()) + .verify_credential_with_expected( + &solana_mpp::parse_authorization(&auth).unwrap(), + &expected, + ) .await .unwrap_err(); assert!( @@ -190,7 +244,9 @@ async fn sol_charge_wrong_recipient_rejected_before_broadcast() { decimals: 9, network: "localnet".to_string(), rpc_url: Some(surfnet.rpc_url().to_string()), - challenge_binding_secret: Some("test-secret".to_string()), + challenge_binding_secret: Some( + "test-secret-key-for-integration-tests-32b-padding".to_string(), + ), ..Default::default() }) .unwrap(); @@ -221,8 +277,19 @@ async fn sol_charge_wrong_recipient_rejected_before_broadcast() { let credential = PaymentCredential::new(challenge.to_echo(), payload); let auth = format_authorization(&credential).unwrap(); + let expected = expected_charge( + "1000000", + "SOL", + &real_recipient.pubkey().to_string(), + "localnet", + 9, + None, + ); let err = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth).unwrap()) + .verify_credential_with_expected( + &solana_mpp::parse_authorization(&auth).unwrap(), + &expected, + ) .await .unwrap_err(); assert!( @@ -249,7 +316,9 @@ async fn sol_charge_replay_rejected() { decimals: 9, network: "localnet".to_string(), rpc_url: Some(surfnet.rpc_url().to_string()), - challenge_binding_secret: Some("test-secret".to_string()), + challenge_binding_secret: Some( + "test-secret-key-for-integration-tests-32b-padding".to_string(), + ), ..Default::default() }) .unwrap(); @@ -262,8 +331,19 @@ async fn sol_charge_replay_rejected() { .unwrap(); // First: success. + let expected = expected_charge( + "1000000", + "SOL", + &recipient.pubkey().to_string(), + "localnet", + 9, + None, + ); let receipt = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth).unwrap()) + .verify_credential_with_expected( + &solana_mpp::parse_authorization(&auth).unwrap(), + &expected, + ) .await .unwrap(); assert_eq!(receipt.status.to_string(), "success"); @@ -271,7 +351,10 @@ async fn sol_charge_replay_rejected() { // Replay: rejected — either by the replay store (signature-consumed) // or by the network itself (duplicate transaction). let err = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth).unwrap()) + .verify_credential_with_expected( + &solana_mpp::parse_authorization(&auth).unwrap(), + &expected, + ) .await .unwrap_err(); assert!( @@ -300,7 +383,9 @@ async fn sol_charge_expired_challenge_rejected() { decimals: 9, network: "localnet".to_string(), rpc_url: Some(surfnet.rpc_url().to_string()), - challenge_binding_secret: Some("test-secret".to_string()), + challenge_binding_secret: Some( + "test-secret-key-for-integration-tests-32b-padding".to_string(), + ), ..Default::default() }) .unwrap(); @@ -318,19 +403,17 @@ async fn sol_charge_expired_challenge_rejected() { let signer = fund_signer(&surfnet); let rpc = RpcClient::new(surfnet.rpc_url().to_string()); - let auth = build_credential_header(&*signer, &rpc, &challenge) - .await - .unwrap(); - let err = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth).unwrap()) + // Audit #10: the client builder now refuses to sign expired challenges + // at build time. This is the canonical path for rejecting expired + // challenges; server-side rejection is the defense-in-depth backstop. + let err = build_credential_header(&*signer, &rpc, &challenge) .await - .unwrap_err(); + .err() + .expect("expired challenge should be rejected before signing"); assert!( - err.code == Some("payment-expired"), - "Expected expired rejection, got: {} ({:?})", - err.message, - err.code + err.to_string().contains("expired"), + "Expected expired rejection, got: {err}" ); } @@ -351,7 +434,9 @@ async fn sol_charge_www_authenticate_roundtrip() { decimals: 9, network: "localnet".to_string(), rpc_url: Some(surfnet.rpc_url().to_string()), - challenge_binding_secret: Some("test-secret".to_string()), + challenge_binding_secret: Some( + "test-secret-key-for-integration-tests-32b-padding".to_string(), + ), ..Default::default() }) .unwrap(); @@ -373,8 +458,19 @@ async fn sol_charge_www_authenticate_roundtrip() { .await .unwrap(); + let expected = expected_charge( + "1000000", + "SOL", + &recipient.pubkey().to_string(), + "localnet", + 9, + None, + ); let receipt = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth).unwrap()) + .verify_credential_with_expected( + &solana_mpp::parse_authorization(&auth).unwrap(), + &expected, + ) .await .unwrap(); assert_eq!(receipt.status.to_string(), "success"); @@ -413,7 +509,9 @@ async fn usdc_charge_full_flow() { decimals: 6, network: "localnet".to_string(), rpc_url: Some(surfnet.rpc_url().to_string()), - challenge_binding_secret: Some("test-secret".to_string()), + challenge_binding_secret: Some( + "test-secret-key-for-integration-tests-32b-padding".to_string(), + ), ..Default::default() }) .unwrap(); @@ -446,8 +544,19 @@ async fn usdc_charge_full_flow() { .await .expect("build USDC credential"); + let expected = expected_charge( + "1000000", + "USDC", + &recipient.pubkey().to_string(), + "localnet", + 6, + Some(solana_mpp::protocol::solana::programs::TOKEN_PROGRAM), + ); let receipt = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth).unwrap()) + .verify_credential_with_expected( + &solana_mpp::parse_authorization(&auth).unwrap(), + &expected, + ) .await .expect("verify USDC credential"); assert_eq!(receipt.status.to_string(), "success"); @@ -492,7 +601,9 @@ async fn usdc_charge_wrong_amount_no_broadcast() { decimals: 6, network: "localnet".to_string(), rpc_url: Some(surfnet.rpc_url().to_string()), - challenge_binding_secret: Some("test-secret".to_string()), + challenge_binding_secret: Some( + "test-secret-key-for-integration-tests-32b-padding".to_string(), + ), ..Default::default() }) .unwrap(); @@ -543,8 +654,19 @@ async fn usdc_charge_wrong_amount_no_broadcast() { let credential = PaymentCredential::new(challenge.to_echo(), payload); let auth = format_authorization(&credential).unwrap(); + let expected = expected_charge( + "1000000", + "USDC", + &recipient.pubkey().to_string(), + "localnet", + 6, + Some(solana_mpp::protocol::solana::programs::TOKEN_PROGRAM), + ); let err = mpp - .verify_credential(&solana_mpp::parse_authorization(&auth).unwrap()) + .verify_credential_with_expected( + &solana_mpp::parse_authorization(&auth).unwrap(), + &expected, + ) .await .unwrap_err(); assert!( diff --git a/rust/crates/x402/src/siwx.rs b/rust/crates/x402/src/siwx.rs index 368a1ef1..dd39b14d 100644 --- a/rust/crates/x402/src/siwx.rs +++ b/rust/crates/x402/src/siwx.rs @@ -132,24 +132,59 @@ impl SiwxExtension { } /// Return this extension as a payment-required `extensions` JSON object. + /// + /// The on-the-wire shape nests the challenge fields under `info` with a + /// sibling `supportedChains` array, per the x402 `sign-in-with-x` spec + /// (`specs/extensions/sign-in-with-x.md`). The in-memory `SiwxExtension` + /// stays flat for ergonomic field access; conversion happens here. pub fn as_extensions_value(&self) -> Result { + let wire = SiwxExtensionWire { + info: SiwxExtensionInfo { + domain: self.domain.clone(), + uri: self.uri.clone(), + statement: self.statement.clone(), + version: self.version.clone(), + nonce: self.nonce.clone(), + issued_at: self.issued_at.clone(), + expiration_time: self.expiration_time.clone(), + not_before: self.not_before.clone(), + request_id: self.request_id.clone(), + resources: self.resources.clone(), + }, + supported_chains: self.supported_chains.clone(), + }; Ok(serde_json::json!({ - SIGN_IN_WITH_X: serde_json::to_value(self) + SIGN_IN_WITH_X: serde_json::to_value(&wire) .map_err(|error| Error::Other(format!("Failed to encode SIWX extension: {error}")))?, })) } /// Parse an SIWX extension from a payment-required `extensions` JSON object. + /// + /// Expects the spec shape `{ "info": { … }, "supportedChains": [ … ] }` + /// under the `sign-in-with-x` key. Unknown sibling fields are ignored for + /// forward-compatibility. pub fn from_extensions_value(extensions: &serde_json::Value) -> Result, Error> { let Some(value) = extensions.get(SIGN_IN_WITH_X) else { return Ok(None); }; - serde_json::from_value(value.clone()) - .map(Some) - .map_err(|error| Error::Other(format!("Invalid SIWX extension: {error}"))) + let wire: SiwxExtensionWire = serde_json::from_value(value.clone()) + .map_err(|error| Error::Other(format!("Invalid SIWX extension: {error}")))?; + Ok(Some(SiwxExtension::new(wire.info, wire.supported_chains))) } } +/// On-the-wire representation of the `sign-in-with-x` extension: the challenge +/// fields nested under `info`, with `supportedChains` as a sibling array. Kept +/// private — callers use the flat [`SiwxExtension`] and the conversion helpers +/// [`SiwxExtension::as_extensions_value`] / [`SiwxExtension::from_extensions_value`]. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +struct SiwxExtensionWire { + info: SiwxExtensionInfo, + supported_chains: Vec, +} + /// Full SIWX message fields before the signature is attached. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -909,16 +944,72 @@ Issued At: 2026-04-27T00:00:00Z" .unwrap() .is_none()); + // Spec-shaped (nested under `info`) but missing the required `nonce`. let invalid = serde_json::json!({ + SIGN_IN_WITH_X: { + "info": { + "domain": "example.com", + "uri": "https://example.com", + "version": "1", + "issuedAt": "2026-04-27T00:00:00Z" + }, + "supportedChains": [] + } + }); + let error = SiwxExtension::from_extensions_value(&invalid).unwrap_err(); + assert!(error.to_string().contains("Invalid SIWX extension")); + + // The pre-fix flat shape (challenge fields at the top level, no `info`) + // is no longer accepted — it must be rejected, not silently parsed. + let legacy_flat = serde_json::json!({ SIGN_IN_WITH_X: { "domain": "example.com", "uri": "https://example.com", "version": "1", + "nonce": "n", "issuedAt": "2026-04-27T00:00:00Z", "supportedChains": [] } }); - let error = SiwxExtension::from_extensions_value(&invalid).unwrap_err(); - assert!(error.to_string().contains("Invalid SIWX extension")); + assert!(SiwxExtension::from_extensions_value(&legacy_flat).is_err()); + } + + #[test] + fn parses_spec_nested_info_challenge() { + // The real shape Venice and the x402 `sign-in-with-x` spec emit: + // challenge fields nested under `info`, with `supportedChains` as a + // sibling array. Regression guard for the flat-vs-nested parse bug. + let extensions = serde_json::json!({ + SIGN_IN_WITH_X: { + "info": { + "domain": "api.venice.ai", + "uri": "https://api.venice.ai/api/v1/x402/balance/96WoyH3J", + "version": "1", + "nonce": "kC-a1gRa4kUdg9DeAHsPS", + "issuedAt": "2026-06-15T16:06:26.551Z", + "expirationTime": "2026-06-15T16:11:26.551Z", + "statement": "Sign in to Venice AI" + }, + "supportedChains": [ + { "chainId": "eip155:8453", "type": "eip191" }, + { "chainId": "solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp", "type": "ed25519" } + ] + } + }); + + let parsed = SiwxExtension::from_extensions_value(&extensions) + .unwrap() + .unwrap(); + assert_eq!(parsed.domain, "api.venice.ai"); + assert_eq!(parsed.nonce, "kC-a1gRa4kUdg9DeAHsPS"); + assert_eq!(parsed.version, "1"); + assert_eq!( + parsed.expiration_time.as_deref(), + Some("2026-06-15T16:11:26.551Z") + ); + assert_eq!(parsed.supported_chains.len(), 2); + + // Round-trips back to the same nested wire shape. + assert_eq!(parsed.as_extensions_value().unwrap(), extensions); } } diff --git a/typescript/package.json b/typescript/package.json index 754c9a89..4144c4c9 100644 --- a/typescript/package.json +++ b/typescript/package.json @@ -24,7 +24,7 @@ "prettier": "@solana/prettier-config-solana", "pnpm": { "overrides": { - "ws": "^8.20.1" + "ws": "^8.21.0" } }, "devDependencies": { diff --git a/typescript/pnpm-lock.yaml b/typescript/pnpm-lock.yaml index fd5014b5..2b1789bd 100644 --- a/typescript/pnpm-lock.yaml +++ b/typescript/pnpm-lock.yaml @@ -5,7 +5,7 @@ settings: excludeLinksFromLockfile: false overrides: - ws: ^8.20.1 + ws: ^8.21.0 importers: @@ -2365,7 +2365,7 @@ packages: isows@1.0.7: resolution: {integrity: sha512-I1fSfDCZL5P0v33sVqeTDSpcstAg/N+wF5HS033mogOVIp4B+oHC7oOCsA3axAbBSGTJ8QubbNmnIRN/h8U7hg==} peerDependencies: - ws: ^8.20.1 + ws: ^8.21.0 istanbul-lib-coverage@3.2.2: resolution: {integrity: sha512-O8dpsF+r0WV/8MNRKfnmrtCWhuKjxrq2w+jpzBL5UZKTi2LeVWnWOmWRxFlesJONmc+wLAGvKQZEOanko0LFTg==} @@ -3440,8 +3440,8 @@ packages: resolution: {integrity: sha512-+QU2zd6OTD8XWIJCbffaiQeH9U73qIqafo1x6V1snCWYGJf6cVE0cDR4D8xRzcEnfI21IFrUPzPGtcPf8AC+Rw==} engines: {node: ^14.17.0 || ^16.13.0 || >=18.0.0} - ws@8.20.1: - resolution: {integrity: sha512-It4dO0K5v//JtTXuPkfEOaI3uUN87iYPnqo/ZzqCoG3g8uhA66QUMs/SrM0YK7/NAu+r4LMh/9dq2A7k+rHs+w==} + ws@8.21.0: + resolution: {integrity: sha512-Vsp28b7DRcimFQvrqu2Wek3z1iYxDCWqHYB8Qsnk/S4RfaCQzPGPyBNuVjJV3cd6UiKtUtp6sNM77gWvzcCH+g==} engines: {node: '>=10.0.0'} peerDependencies: bufferutil: ^4.0.1 @@ -4618,7 +4618,7 @@ snapshots: '@solana/functional': 6.5.0(typescript@5.9.3) '@solana/rpc-subscriptions-spec': 6.5.0(typescript@5.9.3) '@solana/subscribable': 6.5.0(typescript@5.9.3) - ws: 8.20.1(bufferutil@4.1.0)(utf-8-validate@6.0.6) + ws: 8.21.0(bufferutil@4.1.0)(utf-8-validate@6.0.6) optionalDependencies: typescript: 5.9.3 transitivePeerDependencies: @@ -5945,9 +5945,9 @@ snapshots: isexe@2.0.0: {} - isows@1.0.7(ws@8.20.1(bufferutil@4.1.0)(utf-8-validate@6.0.6)): + isows@1.0.7(ws@8.21.0(bufferutil@4.1.0)(utf-8-validate@6.0.6)): dependencies: - ws: 8.20.1(bufferutil@4.1.0)(utf-8-validate@6.0.6) + ws: 8.21.0(bufferutil@4.1.0)(utf-8-validate@6.0.6) istanbul-lib-coverage@3.2.2: {} @@ -7052,9 +7052,9 @@ snapshots: '@scure/bip32': 1.7.0 '@scure/bip39': 1.6.0 abitype: 1.2.3(typescript@5.9.3)(zod@4.3.6) - isows: 1.0.7(ws@8.20.1(bufferutil@4.1.0)(utf-8-validate@6.0.6)) + isows: 1.0.7(ws@8.21.0(bufferutil@4.1.0)(utf-8-validate@6.0.6)) ox: 0.14.5(typescript@5.9.3)(zod@4.3.6) - ws: 8.20.1(bufferutil@4.1.0)(utf-8-validate@6.0.6) + ws: 8.21.0(bufferutil@4.1.0)(utf-8-validate@6.0.6) optionalDependencies: typescript: 5.9.3 transitivePeerDependencies: @@ -7137,7 +7137,7 @@ snapshots: imurmurhash: 0.1.4 signal-exit: 4.1.0 - ws@8.20.1(bufferutil@4.1.0)(utf-8-validate@6.0.6): + ws@8.21.0(bufferutil@4.1.0)(utf-8-validate@6.0.6): optionalDependencies: bufferutil: 4.1.0 utf-8-validate: 6.0.6