Pre-release security hardening and CI/harness push (9 SDKs)#216
Open
EfeDurmaz16 wants to merge 113 commits into
Open
Pre-release security hardening and CI/harness push (9 SDKs)#216EfeDurmaz16 wants to merge 113 commits into
EfeDurmaz16 wants to merge 113 commits into
Conversation
verifyTransaction (type=transaction) reserved the consumed-signature marker after broadcast but never checked it and took no per-signature lock, so the same signed transaction could be verified concurrently or resubmitted, settling one on-chain payment as many receipts. This is the pull-path sibling of the push-mode TOCTOU fixed in #211. Derive the fee-payer signature from the signed wire transaction before broadcast (it equals the value sendTransaction returns) and run the consumed-check, broadcast, reserve, and on-chain verify inside withKeyLock, sharing the same solana-charge:consumed namespace as the push path. Release the reservation only when the transaction definitively never landed, so an honest retry of a still-valid transaction is not bricked. Adds sequential and concurrent pull-mode replay regression tests.
verifySolTransfer compared String(info.lamports) === expectedAmount, where the jsonParsed lamports is a JS number. Above 2^53 that number is lossy, so a correct large SOL transfer could mismatch (the SPL path is safe because the RPC returns amounts as strings). Parse lamports to an exact bigint with a safe-integer guard and fail closed on a numeric value beyond the safe range. Also format the interactive payment-page amount from bigint instead of Number(amount) / 10 ** dec, which loses precision on large amounts (display only; settlement already uses the exact string). Adds a regression test that a system transfer with lamports at 2^53 is rejected.
process_open (push mode) trusted the client-supplied channelId, deposit, payer, payee, and authorizedSigner, and only confirmed that *some* transaction signature had succeeded. A client could therefore claim a fabricated deposit up to max_cap against any confirmed signature and be served metered vouchers up to that phantom balance, with settlement later failing. When an rpc_url is configured, validate the payload against the challenge and the derived channel PDA (reusing payment_channel_open_params), confirm the open transaction succeeded, then fetch the on-chain Channel account and bind status, mint, payee, authorized_signer, and payer to it, persisting the on-chain deposit rather than the client's claim. Mirrors the x402 batch-settlement fetch-and-bind path. Off localnet with no rpc_url the open now fails closed instead of trusting the client. Adds a fail-closed regression test.
replayStore defaulted to undefined, so the mpp adapter passed no store and each cached charge handler built its own in-memory store. Replay markers were therefore process-local: a second replica or a restart would accept a replayed payment, and even within one process distinct handlers did not share the consumed set. Resolve the store once into a single shared instance (mirroring resolveChallengeBindingSecret) and fail closed outside localnet when no store is provided, unless PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE=1 opts into single-process scope. The error points operators at a shared, atomic-reserve backend. Adds a fail-closed regression test and updates off-localnet config tests.
…model Commit rust/Cargo.lock so CI builds, the coverage gate, and hashFiles cache keys are reproducible; without it every build re-resolved floating transitive deps and a fresh transitive release could break any branch. The workspace ships binaries, so a committed lockfile is appropriate. Add a Deployment threat model section to SECURITY.md covering the shared, atomic replay-store requirement and the on-chain session-open binding, which the keyLock and charge-verify comments reference.
The native-SOL on-chain verify compared info.lamports (a Lua number = double on the jsonParsed RPC path under LuaJIT/Lua 5.1) via uint.compare. A u64 value at or above 2^53 is lossy and serializes as scientific notation, raising "invalid unsigned integer" on legitimate large payments and risking a truncated-amount match. Add uint.exact to coerce only exact integers (rejecting NaN/inf/negative/non-integral/out-of-range doubles) and guard both the SOL and SPL amount comparisons; a rejected value is a non-match, falling through to the canonical rejection. Adds regression tests that pass lamports as a number.
The MPP adapter fell back to an in-memory replay store with only a warning, so off-localnet deployments got process-local replay markers that a restart or a second replica would not see. Mirror the TS resolveReplayStore guard: fail closed off localnet unless a store is injected or PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE=1 opts into single-process scope.
Go, Ruby, and Swift were already safe (Go/Ruby stores expose an atomic put_if_absent checked on the broadcast path; Swift is client-only with lamports only ever an exact UInt64). Add regression tests so the pull-mode replay guard and the exact-integer lamports handling cannot silently regress into the TypeScript failure mode.
…rt, npm env gate) - Pin the rolling/branch-tracked actions to commit SHAs: rust-toolchain@stable, gh-action-pypi-publish@release/v1, install-action@cargo-llvm-cov. These refs literally move, so an upstream compromise would run in CI and the publish jobs. - Add a Dependabot github-actions config to keep the pinned SHAs fresh. - report.yml: drop contents:write to contents:read. It runs in the privileged workflow_run context over fork-PR artifacts and only downloads and comments; it never pushes. - npm-publish: add a protected 'npm' environment gate to both publish jobs (mirroring pypi) and document npm OIDC trusted publishing. Note: the version-tagged actions (actions/*@vn, etc.) remain on tags and are Dependabot-tracked; a full SHA sweep is a mechanical follow-up. The surfpool feat/sdk action ref in report.yml could not be resolved (branch renamed) and still needs a SHA pin.
Mirror the Rust process_open fix: a push-mode session open must not trust the client-supplied deposit, channelId, payer, or authorizedSigner. When an RPC is configured, confirm the open signature, read the authoritative on-chain Channel account, bind mint/payee/authorized_signer, and persist the on-chain deposit rather than the client's claim. Off localnet without an RPC the open fails closed. Adds a fetch_and_bind_channel_account helper and regression tests.
Complete the supply-chain hardening: every third-party action across all workflows and composite actions is now pinned to a full commit SHA with a version comment, so a retagged or force-moved upstream ref cannot inject code into CI or the publish jobs. Dependabot (added earlier) keeps the SHAs fresh. Local composite actions (./.github/actions/*) are unaffected. The surfpool feat/sdk external action ref in report.yml still needs a SHA once the upstream branch/ref is confirmed.
Both publish jobs now check npm for the target version before publishing and fail if it already exists, so a re-dispatch or a forgotten version bump cannot silently no-op or error mid-publish. Combined with the earlier protected environment gate and OIDC provenance, this closes the L3 release-workflow gaps.
…sible M4: add an off-chain golden test asserting the account order, signer/writable flags, and discriminator for settle, settle_and_finalize, request_close, and finalize. A codama regen or hand edit that reorders accounts or flips a flag (the #202 class) now fails without a live program. settle_and_finalize stays merchant-first by design (merchant must equal the channel payee). M2: the on-chain settlement test skipped silently when the surfnet was unreachable, so a broken or absent on-chain gate passed green. It still skips locally, but fails hard when PAYKIT_ONCHAIN_REQUIRED=1 (which the CI on-chain job sets), so the gate cannot pass without actually running.
new_session silently defaulted to an in-memory channel store, which is process-local: a second replica or a restart drops the voucher watermark and would accept a replayed voucher. Off localnet, fail closed unless a store is provided or PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE=1 opts into single-process scope, mirroring the charge/pay-kit replay-store guard. Adds a regression test.
…adapter (H3) The x402 adapter fell back to an in-memory replay store with only a warning, so off-localnet deployments got process-local settlement-replay markers (a restart or a second replica accepts a replayed settlement). Mirror the MPP adapter guard (defaultReplayStore): fail closed off localnet unless a store is injected or PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE=1 opts into single-process scope. Also fixes a regression the earlier MPP replay-store guard introduced but could not catch without phpunit: Middleware/RequirePaymentTest builds a devnet client whose adapters are auto-constructed with no store, so it now opts into single-process scope. x402 adapter tests inject a store; adds x402 H3 tests. Verified: full php suite 435 tests OK.
verifyChargeTransaction (pre-broadcast, pull) and verifyInstructions (on-chain, push) are two copies of the same charge policy over different transaction representations, so they can drift. Add a parity test asserting the same tampered payment (underpayment, wrong recipient) is rejected by both paths. A deeper structural dedup of the two verifiers is deferred: they operate on different data shapes (compiled message vs jsonParsed RPC) and merging them is a large, risky rewrite of the security-critical verifier better done outside a pre-release change. The parity test is the anti-drift guard in the meantime.
…loor (M5) The Rust coverage floor excluded both src/mpp/program/ and src/x402/, so the instruction builders (the #202-relevant code) and the entire x402 implementation were invisible to the gate. Now: - src/mpp/program/ is inside the 90% mpp floor (verified 90.57%), its account layout additionally pinned by settle_family_account_layout_golden. - src/x402/ gets its own ratchet floor at 75% (verified 77.50%) instead of being excluded, so x402 regressions are caught. Raising it to 90% needs new tests for server/batch_settlement (~37%) and server/upto (~63%), tracked as follow-up. One instrumented run enforces both floors from per-file data.
Swift shipped tests with code coverage but enforced no floor, so it could silently regress to near-zero. Add scripts/check_coverage.sh, which parses the llvm-cov JSON SwiftPM exports (located via swift test --show-codecov-path, robust to the arch-specific build layout) and gates Sources/ line coverage. Wire it into swift.yml at an 80% ratchet (current Sources coverage is ~89%). Verified locally: 89.40% >= 80%.
…sions (M5) phpunit.xml excluded the security-critical x402 Verifier and Adapter from the 91% coverage gate, so their verification and settlement logic was untested by the floor. Remove both exclusions and add branch-focused tests: - VerifierBranchTest (31 cases): mutate one field per test to hit every rejection branch (compute-budget guards, transfer program/data/account, fee-payer authority, mint/recipient/amount, optional-slot allowlist, memo, missing fields) plus Token + Token-2022 happy paths. - AdapterSettleTest (14 cases): verifyAndSettle end-to-end via FakeRpcGateway (v2 + v1 happy paths, payment-id extension gate, replay guard, broadcast failure, confirmation timeout, malformed payloads). Verifier 69->97%, Adapter 67->95%, overall 92.83% (floor 91). Full suite 482 OK.
Cover the genuinely under-tested modules so x402 can be gated at 90 rather than excluded. Aggregate line coverage: x402 77.5 -> 94.3%, mpp 90.6 -> 93.2%. - x402 settlement servers (batch_settlement 37->94%, upto 63->93%, exact 89->92%) are now testable via a test-only in-process mock Solana JSON-RPC server (mock_rpc.rs, served over a std TcpListener, no new dependency, no production change): the concrete RpcClient reads config.rpc_url, so pointing it at the mock exercises the broadcast/confirm/channel-bind paths and every bind-mismatch branch. - x402 client builders + protocol schemes + error (44/55/81/85/80 -> 94-99%) via pure error/edge-path unit tests. - mpp subscription 63->91%, authenticate 84->98% (subscription has one behavior-preserving extraction of the terms-validation block so its mismatch branches are unit-testable). Full lib suite 1095 passing, fmt clean.
…to >=90 mpp/error.rs, mpp/client/subscription.rs, and mpp/protocol/intents/subscription.rs were 78-88% line; add error/edge-path unit tests to bring each over 90 line and region. Full lib suite 1119 passing.
…+region Cover the last core modules via the test-only mock JSON-RPC server: - mpp/server/session.rs 88->98% line: the H1 on-chain open-bind path and every bind-mismatch branch (mint/payee/signer/payer/status/deposit), asserting the persisted deposit equals the on-chain value not the client's claim. - mpp/server/charge.rs 88->94% line: pull-mode broadcast/confirm and push-mode on-chain verify (transfer match, allowlist, memo, ATA owner, mint resolution). - core/settlement/worker.rs 80->95% line: RpcBroadcaster + actor drain/regroup and the confirm-loop Failed/transient/pending arms via scripted broadcasters. - mock_rpc.rs (test-only) gains getTransaction + set_transaction and a corrected failed-status fixture. mpp 96.8% line / 96.7% region, x402 94.4% / 93.8%. Full lib suite 1210 passing.
…covery) blockhash.rs had no tests. Add the empty-get, set+fresh-get, stale-entry (seeded with an old Instant so the MAX_AGE false arm runs without a 45s wait), and poisoned-lock recovery cases, bringing it >=90 line and region.
Cover the SIWX Default impl, chain selection (supported + unsupported), header create/encode, and every verify error branch (nonce mismatch, issued-at too old, not-before/expiration bounds, malformed/pre-epoch RFC3339). 91.4->97.8% line, 89.3->95.7% region.
… (M5) Replace the mpp>=90 / x402>=75-ratchet split with a single strong gate: both mpp and x402 must hold >=90% on LINE and REGION (llvm-cov's branch-like metric), and the floor is enforced PER-FILE as well as in aggregate, so a weak file cannot hide behind inflated easy ones and no branch class can silently regress. x402 is now fully inside the floor (was excluded). The test-only mock JSON-RPC server is excluded. Verified against a real run: mpp 96.8/96.7, x402 94.9/94.2, every file >=90 on both metrics, 1224 lib tests passing.
Add coverage tests for the least-covered Sources files (X402 Exact Types 61->99%, RpcClient 52->96%, Transaction 75->90%, SolanaPayKit 84->98%, plus SolanaSigner/Ed25519/x402 client guards) via dedicated URLProtocol stubs, each suite isolated so Swift Testing's parallel suites don't race on shared stub state. Sources aggregate 88->95%; raise the CI floor from 80 to 90. 231 tests passing.
The TypeScript reference SDK collected coverage but enforced no floor, and sat at 79% line / 69% branch. Exclude the Codama-generated client and barrel files from coverage (matching the Rust src/generated exclusion), then add real tests across the under-covered surface: - pay-kit adapters (mpp/x402/x402-upto/mpp-session 44-59 -> ~100%), paykit.ts (70 -> 98%), errors/express-routes/protocol. - mpp server Charge.ts (81 -> 98% line / 91% branch), Session.ts (87 -> 94/91), session/on-chain.ts (91/71 -> 99/93), session/lifecycle.ts (0 -> 100), Subscription.ts (93/75 -> 98/91), challenge-guard. - mpp client PaymentChannels/SessionFetch/MultiDelegate/Charge/Subscription and shared/voucher branch coverage to >=90. Aggregate: line 97.8, branch 91.9, functions 97.6, statements 96.4. Add a vitest thresholds block (90 on all four metrics) so the reference SDK is now gated like the others. 955 tests passing.
…payer to index 0 (C1) The subscription activation path co-signs the client-supplied transaction with the server's sponsored fee-payer key. Two gaps let an attacker abuse that signature: 1. validate_activation_scope scanned for the subscribe/transfer pair and silently skipped every other instruction, so a client could append an arbitrary instruction (e.g. a System or SPL transfer draining the fee payer) and still pass scope validation. 2. co_sign_as_fee_payer located the fee-payer key with position() and signed wherever it appeared, so the key could be placed at a non-zero index as the authority/source of an attacker-inserted instruction and still collect the server signature. Enforce a strict program allowlist (subscriptions program subscribe/ transfer only, plus compute-budget, idempotent-ATA, and memo aux programs) and reject anything else instead of skipping. Require the fee payer to be account index 0 (Solana's fee-payer slot) before signing. Add regression tests: reject an instruction on an unknown program, an out-of-range program index, a disallowed subscriptions-program discriminator, and a fee-payer key at a non-zero index; accept the full legitimate activation shape.
…enticate verifiers (M3) The subscription and SIWX-authenticate verifiers compared the client- supplied challenge id against the recomputed HMAC with String equality, which short-circuits on the first differing byte and leaks a timing side channel usable to recover a valid id byte-by-byte. Use the existing constant_time_eq helper, matching the charge verifier.
…ture mode (H3) Signature (push) mode presented an already-confirmed on-chain transaction and accepted it after only checking the transfer amount/mint/recipient. That let a single confirmed transfer be replayed to satisfy unlimited requests, any same-priced route, and arbitrarily old transactions, since there was no consumed-signature store and no freshness check. Add three guards to the Signature arm of verify_envelope_payload: - require a route-bound memo (extra.memo) so a push payment is tied to a specific challenge, not just an amount+recipient (verify_transaction_details then enforces the tx actually carries it); - enforce freshness against max_age using the tx block_time; - atomically consume the settlement signature via a pluggable Store (default in-process MemoryStore; with_store injects a shared one for multi-replica deployments), mirroring the MPP charge consume_signature. Add Error::StaleTransaction and Error::MissingSignatureBinding. Regression tests via the mock JSON-RPC cover: missing-memo rejection, fresh+bound success then replay rejection, stale-transaction rejection, and shared-store single-use across two instances.
…blockhash, and release reservations on pre-broadcast failure
…leanly outside git
Strip four stray dashless audit IDs (H1/C1/H3) from Rust inline test-module comments that the widened repo-hygiene guard now flags, and build the x402 library test step with the server and client features so its feature-gated test imports compile.
… pnpm setup resolves
Collaborator
Author
|
@greptile-apps review |
Collaborator
Author
|
@greptile-apps A few focused areas for this review, please:
|
Greptile SummaryThis PR lands a large pre-release security hardening pass across all nine SDKs, fixing issues surfaced by an external audit. The core patterns are: atomic subscription-activation replay guards (upfront
Confidence Score: 5/5This PR is safe to merge. All previously-identified double-receipt and replay-guard gaps in the subscription activation and x402 exact settlement paths are now closed with atomic upfront reservations, release-on-error across every failure path, and shared injectable stores. The batch-settlement watermark race and the co-signed fee-payer fund-drain vector are also addressed with proper per-channel serialization gates and a strict instruction allowlist. Every critical path reviewed — TypeScript subscription activation (both transaction and signature modes), Rust subscription, Rust/Go x402 exact pull and push, batch settlement watermark gate — has correct atomic guard semantics with documented failure-mode behavior. CI gates close the false-green paths identified in the audit. No new correctness or security defects were found across the reviewed paths. No files require special attention. The key security paths (Subscription.ts, exact.rs, exact.go, batch_settlement.rs) all have regression tests that fail without the fix, and the CI harness now validates cross-SDK parity. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant C as Client
participant S as Server (x402 exact)
participant BC as Blockchain
participant RS as Replay Store
Note over S: Pull (Transaction) path
C->>S: VerifyAndSettle(credential)
S->>RS: PutIfAbsent(consumed_key)
RS-->>S: "inserted=true"
S->>BC: send_transaction(tx)
BC-->>S: broadcast_sig
Note over S: Pin marker (cleanupConsumed=false)
S->>BC: awaitConfirmation(sig)
alt Confirmed
BC-->>S: confirmed
S-->>C: PaymentOK
else Timeout - Not Landed
S->>RS: Delete(consumed_key)
S-->>C: Error (client can retry)
else Timeout - Landed or Indeterminate
Note over RS: Marker KEPT (fail-closed)
S-->>C: Error
end
Note over S: Subscription Activation (TS)
C->>S: settleActivation(credential)
S->>S: withKeyLock(consumedKey)
S->>RS: claimConsumed(consumedKey)
RS-->>S: "claimed=true"
S->>BC: broadcast + waitForConfirmation
alt Confirmed
BC-->>S: confirmed
S-->>C: consumedKey and subscriber
else Error
S->>RS: delete(consumedKey)
S-->>C: Error (retry safe)
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 C as Client
participant S as Server (x402 exact)
participant BC as Blockchain
participant RS as Replay Store
Note over S: Pull (Transaction) path
C->>S: VerifyAndSettle(credential)
S->>RS: PutIfAbsent(consumed_key)
RS-->>S: "inserted=true"
S->>BC: send_transaction(tx)
BC-->>S: broadcast_sig
Note over S: Pin marker (cleanupConsumed=false)
S->>BC: awaitConfirmation(sig)
alt Confirmed
BC-->>S: confirmed
S-->>C: PaymentOK
else Timeout - Not Landed
S->>RS: Delete(consumed_key)
S-->>C: Error (client can retry)
else Timeout - Landed or Indeterminate
Note over RS: Marker KEPT (fail-closed)
S-->>C: Error
end
Note over S: Subscription Activation (TS)
C->>S: settleActivation(credential)
S->>S: withKeyLock(consumedKey)
S->>RS: claimConsumed(consumedKey)
RS-->>S: "claimed=true"
S->>BC: broadcast + waitForConfirmation
alt Confirmed
BC-->>S: confirmed
S-->>C: consumedKey and subscriber
else Error
S->>RS: delete(consumedKey)
S-->>C: Error (retry safe)
end
Reviews (8): Last reviewed commit: "test(mpp): keep subscription co-sign bra..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…m loop confirm_pull_settlement is async and runs on a Tokio worker; std::thread::sleep held the worker for up to 6s per confirmation timeout, which under concurrent settlement can starve the executor. Use tokio::time::sleep().await so the wait yields.
…fork replace harness/go-server built against the fork's go.sum; after the main module moved to upstream gagliardetto/solana-go and a local ATA helper, its go.mod/go.sum were stale and 'go build' failed. go mod tidy resolves them.
…the go and python on-chain binders
…nd IO-cap plugin autoload - PHP x402 harness server: inject a process-scoped FileStore into the X402 adapter so it no longer fails closed on the non-localnet harness network, mirroring the MPP charge path. - python-x402 / python-x402-upto clients: run through the SDK uv env (uv run --project ../python) like python-session so runtime deps such as httpx resolve instead of failing under a bare python3. - IO-cap CI step: disable the anchorpy pytest plugin on the command line (-p no:pytest_anchorpy) so it does not autoload and crash importing the uninstalled pytest-xprocess; addopts alone did not apply for a target outside python/.
Collaborator
Author
|
Thanks — all three review findings are resolved:
@greptile-apps please run a full general re-review of the whole branch (not limited to these areas). |
The go-x402 harness client (go-client) still pinned the removed lgalabru solana-go fork in its go.mod/go.sum, so 'go run .' failed to resolve deps. go mod tidy moves it to upstream, matching go/ and harness/go-server.
go mod tidy alone kept the explicit replace => lgalabru/solana-go directive, so the go-x402 client still tried to fetch the unreliable fork in clean CI. Remove the replace so it resolves upstream gagliardetto/solana-go v1.22.0, matching go/ and harness/go-server.
Collaborator
Author
|
@greptile-apps review |
Rust used solana-x402-exact:consumed:, Go and TypeScript used x402-exact:consumed:, while Python, Ruby, and PHP used x402-svm-exact:consumed:. Two differently-languaged replicas sharing one replay store wrote to different namespaces and would not dedupe each other's replays. Standardize every SDK on x402-svm-exact:consumed: so a shared, cross-process store protects across all language servers.
Collaborator
Author
|
Resolved the cross-SDK replay-namespace divergence: unified the x402 exact consumed-signature key prefix to x402-svm-exact:consumed: across every SDK (Rust used solana-x402-exact:, Go and TypeScript used x402-exact:; Python/Ruby/PHP already used x402-svm-exact:). A shared cross-process store now dedupes replays across differently-languaged replicas. Verified: Rust 54, Go, and the TypeScript x402-adapter (31) tests pass. (commit 201e8fd) @greptile-apps please re-review. |
The RELEASE_SAFE_SETTLE_REASONS set hard-codes @x402/svm's errorReason strings; note the vendored 2.16.0 version so an upgrade prompts re-deriving the set rather than silently reclassifying a payload between release and keep.
Collaborator
Author
|
@greptile-apps review — all inline findings are resolved: the async sleep (tokio::time::sleep, 7447faa), the subscription replay TOCTOU (atomic put_if_absent + release-on-failure + regression tests, 30c0085), the cross-SDK consumed-key prefix (unified to x402-svm-exact:consumed:, 201e8fd), and the @x402/svm reason-set version pin (ef6e977). Please run a fresh full review of the latest commit. |
…omic (claimConsumed)
Collaborator
Author
|
Resolved the remaining finding — the TypeScript subscription activation TOCTOU. settleActivation now claims the consumed key atomically via claimConsumed under withKeyLock in BOTH transaction and signature modes (before broadcast), releases the reservation on every failure path so a transient error does not brick a retry, and keeps it only when a receipt is produced. This mirrors the Rust fix (30c0085) and the charge path's usage. Added two regression tests (concurrent-activation admits exactly one; release-on-failure allows retry), both verified to fail on the old get/put and pass on the fix. Full @solana/mpp suite: 1057 tests pass. (commit ca8396e) @greptile-apps please run a fresh full review of the latest commit. |
Collaborator
Author
Collaborator
Author
|
@greptile-apps please run a fresh full review of the latest commit. better if you can do TREX review! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Pre-release security hardening and a CI/harness push across all nine SDKs (typescript, rust, go, python, ruby, php, lua, kotlin, swift).
Security fixes, each ported to every SDK that implements the behavior:
CI and harness hardening:
Reproducibility:
Why
These SDKs are going live. The audit surfaced one critical and several high-severity issues, nearly all of them the same shape: a guard hardened in one SDK but never ported to its siblings, with no shared conformance vector to catch the drift. This lands the fixes with cross-SDK parity and closes the CI gaps that let that class of divergence ship unnoticed.
Tests
The full local CI matrix was run and is green:
Every fix ships with a regression test that fails before the fix and passes after it. The x402 exact verifier gains shared cross-SDK reject vectors that every port must satisfy.
What to review especially
Note on size: this is the full audit branch, so it is large (99 commits, ~228 files). rust/Cargo.lock accounts for roughly 13k of the added lines. It is best reviewed by area using the list above.
Fable 5 reviewer note
This branch was put through a multi-model adversarial review before it opened: Claude (Fable 5) across four domain slices, plus independent passes from codex (GPT) and opencode (GLM). That review found one blocker and several high and medium issues, all since fixed and re-reviewed as closed. The blocker was that the strict subscription allowlist rejected the TypeScript client's own first-time-subscriber transaction; the client now pre-broadcasts the subscription-authority init as a separate transaction, matching the Rust client. The highs are also closed: the challenge-serialization guard now runs on the real 402 emission path, the false-green CI fix covers every release-gating leg, the repo-hygiene guard checks every package.json and catches both dashed and dashless audit IDs, and the new cross-SDK reject vectors are wired into CI. The batch-settlement per-channel gate is bounded and now has a regression test that actually fails if the gate is removed.
The full local CI matrix was run and is green: TypeScript, Rust (clippy with -D warnings), Go (91 percent coverage gate), Python (pyright clean), Ruby, PHP, and Swift. Kotlin and Lua were verified structurally since their runners are CI-only locally. Two items are worth a reviewer's attention: the harness assert-run-count values were not re-derived by hand and will fail loudly rather than pass silently if wrong, and Cargo.lock is committed and pinned.