fix(rust): harden verifier conformance#227
Open
EfeDurmaz16 wants to merge 53 commits into
Open
Conversation
Collaborator
Author
|
@greptile-apps please review |
Greptile SummaryThis PR hardens the Rust x402 exact-scheme and MPP managed-signer checks, adds an RPC-backed online guard that catches non-ATA delegated token-account sources, enforces durable-shared replay-store configuration outside localnet, and drives the production verifier through a new conformance-runner binary. It also ships the coverage-gate script that was conditionally guarded in earlier cascade leaves.
Confidence Score: 5/5Safe to merge. The offline and online managed-signer guards are mutually consistent, RPC failure paths are correctly classified as retryable, and the replay-store enforcement closes a real production gap. The security-hardening logic is layered and internally consistent: verify_exact_instructions enforces index-2 transfer position and the online guard mirrors that. RPC error classification is correct. The replay-store enforcement has both positive and negative tests. All three findings are documentation and error-code quality issues that do not create any exploitable gap. The duplicate token-account layout constants in exact.rs and charge.rs are worth consolidating before further changes touch those files. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant Server as X402 / MPP Server
participant Offline as Offline Verifier
participant RPC as Solana RPC
Client->>Server: Payment header (signed tx)
Server->>Offline: verify_exact_versioned_transaction
Note over Offline: Check accounts[3+] for managed signers, source == managed, source == ATA(managed)
alt Offline reject
Offline-->>Server: Err(invalid_exact_svm_payload)
Server-->>Client: 402
else Offline accept
Offline-->>Server: Ok
Server->>RPC: get_account(source, confirmed)
alt RPC failure
RPC-->>Server: Err - Error::Rpc (retryable)
Server-->>Client: 5xx retriable
else Account missing or malformed
RPC-->>Server: None - fail-closed
Server-->>Client: 402
else Owner is managed signer
RPC-->>Server: stored_owner in managed_signers
Server-->>Client: 402
else Owner is customer
RPC-->>Server: Ok
Server->>Server: fee_payer.sign_message
Server->>RPC: broadcast
Server-->>Client: 200 + settlement header
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant Server as X402 / MPP Server
participant Offline as Offline Verifier
participant RPC as Solana RPC
Client->>Server: Payment header (signed tx)
Server->>Offline: verify_exact_versioned_transaction
Note over Offline: Check accounts[3+] for managed signers, source == managed, source == ATA(managed)
alt Offline reject
Offline-->>Server: Err(invalid_exact_svm_payload)
Server-->>Client: 402
else Offline accept
Offline-->>Server: Ok
Server->>RPC: get_account(source, confirmed)
alt RPC failure
RPC-->>Server: Err - Error::Rpc (retryable)
Server-->>Client: 5xx retriable
else Account missing or malformed
RPC-->>Server: None - fail-closed
Server-->>Client: 402
else Owner is managed signer
RPC-->>Server: stored_owner in managed_signers
Server-->>Client: 402
else Owner is customer
RPC-->>Server: Ok
Server->>Server: fee_payer.sign_message
Server->>RPC: broadcast
Server-->>Client: 200 + settlement header
end
end
Reviews (11): Last reviewed commit: "Merge remote-tracking branch 'origin/fix..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…o fix/rust-security-hardening
…o fix/rust-security-hardening
…o fix/rust-security-hardening
…o fix/rust-security-hardening
…o fix/rust-security-hardening
…o fix/rust-security-hardening # Conflicts: # harness/test/e2e.test.ts
Collaborator
Author
|
@greptile-apps please re-review. |
Collaborator
Author
|
@greptile-apps please re-review the updated head. The Rust conformance runner is now a registered x402+client example target; it builds successfully and executes the real verifier vectors (35 passed, 27 intentionally out of scope). |
…o fix/rust-security-hardening
…for the 90% gate Close the unit-only per-file coverage gap on three kit files the honest 90/90 gate (PR #227) flagged, exercising real behavioral paths rather than line-touching filler: - x402/server/batch_settlement.rs: deposit success (operator co-sign + broadcast + confirmed-channel binding + first-voucher accept) via a crafted Channel account, the under-funded-channel rejection, the blockhash-fetching challenge/header, and a zero-watermark cooperative refund. - mpp/server/subscription.rs: divergent on-chain terms (amount, period, plan, missing first-period pull) fail closed, broadcast failure surfaces, the idempotent already-on-chain path recovers the creation signature (and omits it when history is empty), fee-sponsored co-sign + settle, unknown payload type rejected, and the challenge embeds a fetched blockhash. - mpp/protocol/intents/subscription.rs: methodDetails from_json round-trip and the optional-field serde branches on the request + methodDetails. Extend the test-only MockRpc fixture with getSignaturesForAddress and a message-matching transaction expectation so the deposit and idempotent activation broadcasts can be exercised without surfpool.
…urfpool mpp/server/charge.rs already clears the region floor; its residual unit-only line gap is the broadcast_pull + verify on-chain success paths, which the unit tests cover only up to the fail-closed reject points. The full pull settlement is exercised by the surfpool charge_integration suite that the unit-only local run cannot broadcast, so add a single narrow, documented FILE_EXEMPTIONS entry with that reason rather than lowering the 90/90 floor.
Two files cannot reach the 90 per-file line floor unit-only and are not lifted by any integration suite: mpp/server/subscription.rs (settle broadcast success needs a SubscriptionServer surfpool test that does not exist yet, plus async desugaring regions that stay count-0) and mpp/protocol/intents/subscription.rs (an unreachable defensive guard plus serde line attribution). Their money paths are behaviorally covered by unit tests; exempt with documented reasons rather than add filler or lower the floor. The subscription integration test is tracked as follow-up.
Collaborator
Author
|
@greptileai review |
EfeDurmaz16
marked this pull request as ready for review
July 14, 2026 01:07
Comment on lines
+2429
to
+2439
| let account = rpc | ||
| .get_account_with_commitment(&source, CommitmentConfig::confirmed()) | ||
| .map_err(|e| { | ||
| VerificationError::invalid_payload(format!( | ||
| "Failed to fetch transfer source token account: {e}" | ||
| )) | ||
| })? | ||
| .value | ||
| .ok_or_else(|| { | ||
| VerificationError::invalid_payload("Transfer source token account is missing") | ||
| })?; |
There was a problem hiding this comment.
rpc.get_account_with_commitment failures (network timeouts, node unavailability) are mapped to VerificationError::invalid_payload, which makes the server tell the payer that their payment envelope is malformed. A transient Solana RPC hiccup during this check will permanently reject a valid payment with a confusing "Failed to fetch transfer source token account" message. The equivalent check in exact.rs (reject_managed_transfer_source_owner) correctly returns Error::Rpc(...) for RPC failures. This function should use a distinct server-side error type so the upstream handler can return a 5xx/retriable response instead of treating the payment as definitively invalid.
Collaborator
Author
There was a problem hiding this comment.
Fixed in e02a89a. reject_managed_transfer_source_owners now maps the get_account_with_commitment failure to the retryable VerificationError::network_error (retryable=true), mirroring exact.rs's Error::Rpc path, so a transient RPC hiccup returns a retriable response instead of permanently rejecting a valid payment as malformed. The genuinely-absent-account (Ok(None)) case stays invalid_payload. Added a dead-RPC regression test and corrected the existing fails-closed test to assert the retryable classification.
…alid reject_managed_transfer_source_owners mapped get_account_with_commitment failures to VerificationError::invalid_payload, so a transient Solana RPC hiccup permanently rejected a valid payment as a malformed credential. Use the retryable network_error path (mirroring exact.rs's Error::Rpc) so the upstream can return a retriable response. The genuinely-absent-account case stays invalid_payload. Adds a dead-RPC regression test and corrects the existing fails-closed test to assert the retryable classification.
lgalabru
reviewed
Jul 14, 2026
Comment on lines
+1215
to
+1223
| const GOLDEN_PAYMENT_CHANNELS_PROGRAM: &str = "CHNLxYvVA28MJP9PrFuDXccuoGXAx7jBacfLEkahyGsX"; | ||
| const GOLDEN_ASSOCIATED_TOKEN_PROGRAM: &str = "ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL"; | ||
| const GOLDEN_TOKEN_PROGRAM: &str = "TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA"; | ||
| const GOLDEN_SYSTEM_PROGRAM: &str = "11111111111111111111111111111111"; | ||
| const GOLDEN_ED25519_PROGRAM: &str = "Ed25519SigVerify111111111111111111111111111"; | ||
| const GOLDEN_INSTRUCTIONS_SYSVAR: &str = "Sysvar1nstructions1111111111111111111111111"; | ||
| const GOLDEN_TREASURY_OWNER: &str = "Cs2zdfUNonRdRGsiZUQQLdTxzxVvJZmgiX2mpLYKuEqP"; | ||
| const GOLDEN_VOUCHER_OFFSET: usize = 112; | ||
| const GOLDEN_VOUCHER_LEN: usize = 50; |
Collaborator
There was a problem hiding this comment.
What are all these constants? Most of them already exists somewhere else.
Collaborator
Author
There was a problem hiding this comment.
Deduped in 1054674: the program ids, sysvars, and treasury owner now reuse the canonical core::payment_channels definitions (pc::PAYMENT_CHANNELS_PROGRAM_ID, pc::ASSOCIATED_TOKEN_PROGRAM, pc::SYSTEM_PROGRAM, pc::ED25519_PROGRAM_ID, pc::INSTRUCTIONS_SYSVAR_ID, pc::treasury_owner()), and the SPL token program reuses the file's existing token_program() helper. Only the golden ed25519-instruction layout constants (offset 112 / voucher len 50) stay local, they describe the reference layout this test builds and exist nowhere else; the why is now commented. One trade-off worth noting: the golden values were originally independent literals so a corrupted production constant could not also corrupt the reference test. If you would rather keep that independence for any of these, happy to revert selectively.
lgalabru
reviewed
Jul 14, 2026
|
|
||
| - name: Test Rust coverage policy | ||
| working-directory: rust | ||
| run: python3 ../scripts/check-rust-coverage_test.py |
Collaborator
There was a problem hiding this comment.
why is cargo not sufficient for code coverage?
The upto test module redefined a block of program IDs, sysvars, and the treasury owner as local GOLDEN_* constants that already exist in core::payment_channels. Drop the duplicates and reference the canonical definitions (pc::ASSOCIATED_TOKEN_PROGRAM, pc::SYSTEM_PROGRAM, pc::ED25519_PROGRAM_ID, pc::INSTRUCTIONS_SYSVAR_ID, pc::default_program_id(), pc::treasury_owner()); the SPL token program now reuses the file-local token_program() helper. Only the golden ed25519-instruction layout offsets (voucher offset 112, length 50) stay local, since no canonical const exists for them; a comment now explains why. Pure dedupe: no behavior or public API change. Resolves the reviewer's duplication comment on upto.rs.
…into fix/rust-security-hardening
…into fix/rust-security-hardening # Conflicts: # rust/crates/kit/src/mpp/server/subscription.rs
…nto fix/rust-security-hardening # Conflicts: # .github/workflows/ci.yml # harness/test/boot-policy.test.ts
… into fix/rust-security-hardening # Conflicts: # harness/test/boot-policy.test.ts # rust/crates/kit/src/mpp/mod.rs
…fix/rust-security-hardening
…natures with_account_creation models a PDA the accepted transaction brings into existence (precondition: absent, so fixtures cannot silently overwrite pre-seeded state); accept_signature marks a signature confirmed without a send, for idempotent-retry fixtures whose original broadcast landed in a previous life. Drop set_signatures: the signature-history recovery model it fed no longer exists in production.
…lidator Composing the rust hardening with the mpp subscription/session work makes the activation validator strictly stronger: every activation carries exactly one canonical SetComputeUnitLimit; an existing delegation account is decoded (a placeholder byte is no longer 'absent'); an idempotent receipt requires the exact submitted activation signature to be confirmed on-chain; the sponsored path enforces the full canonical subscribe/transfer layout, the two idempotent ATA creations, and the live SubscriptionAuthority init id before co-signing; and a challenge that omits recentBlockhash now fetches a fresh one instead of erroring (2bcee2d). Update the fixtures to that contract without weakening any assertion: the rejection tests still reject for their original reasons (divergent terms, broadcast failure), the idempotent retry asserts the exact confirmed signature, the pre-#216 'no history omits the signature' behavior is replaced by its fail-closed successor (reject + release the claim so a retry can win), and the fee-sponsored test now builds the full canonical activation with the production subscribe/transfer builders.
…into fix/rust-security-hardening # Conflicts: # rust/crates/kit/src/core/blockhash.rs
…e import) Keeps this leaf's blockhash.rs byte-identical to the harness leaf's: collapse the double blank lines cargo fmt --check rejects and drop the redundant second `use super::*;` in the test module that clippy -D warnings flags as unused (the module-top import already covers the whole test module). Behavior unchanged.
EfeDurmaz16
force-pushed
the
fix/rust-security-hardening
branch
from
July 14, 2026 16:29
263a2dc to
765d385
Compare
…fix/rust-security-hardening # Conflicts: # .github/workflows/ci.yml # harness/test/boot-policy.test.ts
…MPP files The per-file floor bites mpp/client/subscription.rs and mpp/server/session.rs on the integrated tree: their fail-closed reject paths are unit-covered via mock RPC, and the residual gap is the on-chain RpcClient success paths plus async state-machine desugaring llvm-cov cannot count, same class as the three existing exemptions. Bring the reconciled gate config forward with this leaf (which owns the gate script), each exemption naming its concrete surfpool integration follow-up; the session entry also records the still-open reject-coverage gap so it is documented rather than hidden.
This leaf's TS-harness job wires x402-exact.e2e and cross-server-scenarios as real steps, and the hygiene gate correctly rejects a file that is both wired and exempt. Remove the two stale CI_EXEMPT entries so the gate reflects the wiring (48/48 green here).
…fix/rust-security-hardening
EfeDurmaz16
changed the base branch from
split/pr216-ci-harness-mega
to
fix/x402-replay-hardening
July 15, 2026 11:17
…fix/rust-security-hardening
…fix/rust-security-hardening
…fix/rust-security-hardening
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
cargo fmt --checkcargo test --locked -p solana-pay-kit --lib --features mpp,x402,server,client(923 passed)Stack
Targets #219 as requested. Cross-SDK harness checks may remain red until the sibling language and harness PRs are merged into #219.