fix(mpp): require atomic shared replay stores#237
Conversation
|
@greptile-apps please review the current head. |
Greptile SummaryThis PR enforces atomic shared replay stores as a hard security invariant across TypeScript, Go, PHP, Python, and Rust SDKs for MPP, replacing the previous implicit process-local-memory default with an explicit unsafe development opt-in (
Confidence Score: 5/5Safe to merge; the replay-store security invariant is correctly enforced across all five SDKs with no regressions in the core charge/session verification paths. The core security change is implemented correctly and covered by targeted tests. The two findings are a minor env-var acceptance asymmetry between the replay-store and session-store escape hatches in TypeScript, and a less strict identity check for allowUnsafeMemoryStore in PHP. Neither affects correctness in the common deployment model. typescript/packages/pay-kit/src/config.ts (env-var acceptance inconsistency) and php/src/Protocols/Mpp/Adapter.php + php/src/Protocols/Mpp/Server/SolanaChargeHandler.php (MemoryStore identity check less strict than TypeScript). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant ChargeHandler
participant Solana RPC
participant ReplayStore
Client->>ChargeHandler: credential (signed tx bytes)
ChargeHandler->>ChargeHandler: resolveReplayStore (fails closed without shared store)
ChargeHandler->>ChargeHandler: co-sign transaction
ChargeHandler->>Solana RPC: broadcastTransaction(signedTx)
Solana RPC-->>ChargeHandler: signature
ChargeHandler->>Solana RPC: waitForConfirmation(signature)
Solana RPC-->>ChargeHandler: confirmed
ChargeHandler->>Solana RPC: verifyOnChain(signature, challenge, recipient)
Solana RPC-->>ChargeHandler: instructions verified
ChargeHandler->>ReplayStore: putIfAbsent("solana-charge:consumed:sig", true)
alt first winner
ReplayStore-->>ChargeHandler: true
ChargeHandler-->>Client: Receipt (200)
else replay / concurrent race
ReplayStore-->>ChargeHandler: false
ChargeHandler-->>Client: VerificationFailedError → 402
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 ChargeHandler
participant Solana RPC
participant ReplayStore
Client->>ChargeHandler: credential (signed tx bytes)
ChargeHandler->>ChargeHandler: resolveReplayStore (fails closed without shared store)
ChargeHandler->>ChargeHandler: co-sign transaction
ChargeHandler->>Solana RPC: broadcastTransaction(signedTx)
Solana RPC-->>ChargeHandler: signature
ChargeHandler->>Solana RPC: waitForConfirmation(signature)
Solana RPC-->>ChargeHandler: confirmed
ChargeHandler->>Solana RPC: verifyOnChain(signature, challenge, recipient)
Solana RPC-->>ChargeHandler: instructions verified
ChargeHandler->>ReplayStore: putIfAbsent("solana-charge:consumed:sig", true)
alt first winner
ReplayStore-->>ChargeHandler: true
ChargeHandler-->>Client: Receipt (200)
else replay / concurrent race
ReplayStore-->>ChargeHandler: false
ChargeHandler-->>Client: VerificationFailedError → 402
end
Reviews (13): Last reviewed commit: "Merge remote-tracking branch 'origin/ci/..." | Re-trigger Greptile |
|
@greptile-apps please re-review — the exact-head Rust build break, Windows import portability, and typed replay rejection findings are fixed with focused regressions. |
|
@greptile-apps please re-review the current head; the optional Rust MPP accessor, replay-store contracts, and mainnet alias guards now have focused regression coverage. |
|
@greptile-apps please re-review the exact current head; CI-derived replay capability and harness boot fixes are now covered, while the remaining conformance dependencies are owned by #214 and #236. |
|
@greptile-apps please re-review the updated replay-store hardening. |
| // pays but the signature is never recorded, so a retry re-broadcasts (double | ||
| // charge) or replays. Reserving here closes the replay window; the | ||
| // post-timeout status recovery below rescues the false-negative case. | ||
| await store.put(`solana-charge:consumed:${signature}`, true); | ||
| if (!(await store.putIfAbsent(`solana-charge:consumed:${signature}`, true))) { | ||
| throw new Error('Transaction signature already consumed'); |
There was a problem hiding this comment.
Plain
Error on replay collision may reach mppx as a 500
verifyTransaction now calls store.putIfAbsent() directly and throws new Error('Transaction signature already consumed') when a replay is detected. The atomicReplayStoreView.put() interception (in store.ts) throws Errors.VerificationFailedError, which mppx converts to a 402. But that interception path is only exercised if mppx internally calls store.put() for a consumed key — Charge.ts's direct putIfAbsent() call bypasses it entirely. If verifyTransaction's caller (within Charge.ts) does not explicitly catch a plain Error and convert it to a VerificationFailedError before returning to mppx, a concurrent duplicate-spend attempt will surface as an unhandled error rather than the expected 402 challenge. The same pattern appears in verifySignature at the other call site in this file. Consider replacing both throw new Error(...) instances with throw new Errors.VerificationFailedError({ reason: '...' }) to stay consistent with the view's interception path and ensure mppx handles the rejection correctly regardless of which code path is active.
There was a problem hiding this comment.
Resolved on the current head. All four replay-collision throws in Charge.ts use Errors.VerificationFailedError({ reason: 'Transaction signature already consumed' }) (verifyTransaction commit, verifySignature pre-lock read, in-lock recheck, in-lock commit), so mppx returns a 402, not a 500.
server.New now enforces the core.SharedStore/IsShared() contract on any injected replay Store: an injected store must affirmatively implement SharedStore and report IsShared() == true on every network, closing the gap where a non-shared custom store passed the prior concrete-type check. AllowUnsafeMemoryStore is an explicit per-server development opt-in. It only ever authorizes the internally created process-local MemoryStore when Config.Store is nil; it never blesses an injected unshared store, and it is rejected outright on mainnet. A nil store without the opt-in fails closed on all networks, including localnet. The high-level paykit MPP adapter threads ReplayStore and the opt-in through and validates lazily at first serverFor(), so the guard surfaces as a challenge/settle error rather than at client construction.
Adds a ReplayStoreValidator that fails closed unless an injected store affirmatively declares a durable/shared replay capability via DurableStore or ReplayStoreCapability (and every declared capability method returns true). The MPP Adapter now enforces it: an injected store must pass the validator, allowUnsafeMemoryStore only permits an internally created process-local MemoryStore, that opt-in is rejected on mainnet, and a missing store fails closed on every network including localnet. Threads allowUnsafeMemoryStore through MppConfig, the middleware, and the Laravel/Symfony wiring, and updates the cross-SDK PHP harness to opt into the process-local store explicitly.
Introduces an atomic charge replay store distinct from the session store. mpp/server/store.ts adds ReplayStore plus resolveReplayStore, which fails closed unless an injected store reports isShared and isDurable or the explicit allowUnsafeMemoryStore flag authorizes an SDK-created process-local store; mainnet plus that flag is rejected. solana.charge and the push-mode path now reserve the consumed-signature marker with putIfAbsent after every fallible settlement check and raise VerificationFailedError (mapped to 402) on collision, so a broadcast ambiguity or timeout never burns a legitimate retry and only one concurrent caller obtains a receipt. pay-kit/replay-store.ts bridges the store into pay-kit with an identity-based production brand (declareProductionReplayStore) that copying capability booleans onto an unknown store cannot forge. configure() resolves and validates the store, and assertReplayStorePolicy revalidates it at every adapter boundary. The MPP adapter forwards the atomic view to both charge and subscription construction so subscription consumed-markers also upgrade to atomic putIfAbsent. Session-store durability policy (Session.ts, session/store.ts) and the subscription store threading stay with the subscription delivery.
The low-level session() method silently fell back to a process-local in-memory SessionStore on any network, including mainnet. A direct @solana/mpp consumer that bypasses the pay-kit adapter therefore lost every open channel on restart and never shared channel state across workers, letting cumulative vouchers double-spend. resolveSessionStore now mirrors the replay-store policy: a durable-shared store is honored everywhere, localnet may use a process-local store freely, devnet only behind the PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE (or allowUnsafeMemoryStore) dev opt-in, and mainnet forbids it outright. A mainnet session() with no store or a memory store throws a ConfigurationError. Adds the isMemorySessionStore marker and SessionStoreDurability type consumed by the adapter layer, plus regression tests covering the mainnet-reject, devnet-opt-in, localnet, and durable-store paths.
209e390 to
631d36e
Compare
|
@greptileai review |
Reconcile #238 (MPP subscription) onto #237's nominal replay-store policy, which the maintainer chose as the single config-layer model. #238's flag-based policy is removed; all its unique behaviour is preserved on top of #237's model. config.ts: - Delete #238's validateReplayStore + requiresSettlementReplayStore. - Keep #237's assertReplayStorePolicy as the single replay policy (isAtomicReplayStore / isProductionReplayStore / declareProductionReplayStore / isAuthorizedUnsafeMemoryReplayStore). - x402 config coverage stays adapter-only: the x402 store cannot satisfy the putIfAbsent production marker and #237's own tests require x402-only configs to accept a legacy Store.Store, so the mpp-only gate is left in place and x402 replay is fenced by the x402 adapter (#236). Never silently accepts a non-durable x402 mainnet store. - Preserve #238's validators, inMemoryStoresAllowed, freezePayKitConfig / freezePayKitSigner snapshot, 32-byte challenge-secret check, expiresIn date check, and demo-signer-by-address detection (now also enforced at the assertReplayStorePolicy boundary). validatePayKitConfig runs the replay/signer policy before the challenge-secret length so mainnet/store misconfig reports first. subscription reconciliation: - Subscription adapter requires a durable shared store via #237's nominal isProductionReplayStore rather than isShared/isDurable flags; the devnet in-memory opt-in (createUnsafeMemoryReplayStore, isAuthorizedUnsafe) is rejected for subscription activation and never accepted on mainnet. - The atomic view (createAtomicReplayStoreView) now exposes reserve() aliasing putIfAbsent, so #238's reserve-based subscription server runs unchanged against the operator's putIfAbsent production store. subscription-replay-store.ts is removed. session store: keep #237's low-level durability guard (sessionStoreDurability) and adopt #238's more robust global-registry enumerable isMemorySessionStore brand (survives package-copy and shallow-spread). tests: rewrite #238 tests that asserted the deleted validateReplayStore messages to the nominal assertReplayStorePolicy model; every replay/session/subscription money-path test still asserts fail-closed. Broaden the harness fail-closed signature to accept #237's Go "forbidden on mainnet" wording.
The charge-server store guard required a shared replay store (or the AllowUnsafeMemoryStore opt-in) on every network including localnet, diverging from the base, the TypeScript, and the Python SDKs and breaking the Go playground E2E: the server refused to boot on localnet with no injected store, so payment-link pages hit ERR_CONNECTION_REFUSED. Localnet is single-process development with no multi-replica or restart-persistence replay risk, so it now permissively defaults to a process-local MemoryStore with no opt-in and no IsShared requirement, matching the other SDKs. Off localnet the shared-store requirement and the AllowUnsafeMemoryStore opt-in are unchanged. Tests that asserted the over-strict localnet rejection now assert the permissive default and the off-localnet fail-closed path.
|
@greptileai review |
# Conflicts: # .github/workflows/ci.yml # harness/test/boot-policy.test.ts
Add #239's unique fail-closed binding: a session open/top-up is rejected unless the on-chain Channel account (fetched via getAccountInfo at the confirmed slot) confirms the claimed deposit, mint, payee, authorized signer, grace period and distribution hash, and a signature-only open is bound to its canonical broadcast transaction. - server/session/on-chain.ts: verifyChannelAccountState, verifySignatureOnlyOpenTransaction, GetAccountInfoRpc, TopUpTransactionRpc. - server/session/wire-tx.ts + store.ts: openSignature/salt/settlement fields carried on ChannelState for the binding. - server/Session.ts: wire the verify calls into open/top-up/routes; export SettlementRpc. - tests: session-state-binding, session-signature-only-open, plus the #239 additions to session-on-chain / session-server-on-chain / session-store. Dedup against stacked siblings #237/#238 (already in the base): keep their session-store durability guard (resolveSessionStore + allowUnsafeMemoryStore) and replay-store policy; drop #239's independent re-implementation of the same concern (requireProductionSessionSafety + allowUnsafeEphemeralStoreOffLocalnet + the adapter wiring), which conflicted with those siblings' regression tests. Superseded session-store-durability assertions were removed from the #239 test files; the mainnet fail-closed control is unchanged (stronger via #237).
…e combined tree Composing #236 (x402 reserve lifecycle) with the mpp nominal replay-store model (#237) surfaced three TS fallouts: - adapters/x402-shared: when a config injects an mpp-style atomic putIfAbsent store (the shared process-local store a dual x402+mpp config provisions from the unsafe opt-in), x402 now upgrades it to the reserve surface via the atomic view instead of rejecting it, so both protocols fence the same markers. Fixes the openapi x402+mpp dual-protocol config. - x402-adapter/x402-shared tests: pin accept: ['x402'] so the pure-x402 adapter tests use the reserve store rather than defaulting to the mpp putIfAbsent policy (which correctly rejects a reserve-only store). - mpp server index/store: prettier formatting the auto-merge left unformatted.
…ix/mpp-replay-store-hardening
…n replay store This leaf's replay-store policy requires an atomic putIfAbsent, and the mainnet-fork settlement fixture injected a spread of the legacy get/put store (which drops the prototype method), so createPayKit fails closed in beforeAll and every gated settlement assertion errors. Bring forward the reconciled fixture: a real Map-backed store (get/put/delete + atomic putIfAbsent, isShared+isDurable) declared production, matching the SDK's own createSharedTestReplayStore pattern, with no unsafe-memory escape hatch. The gate travels with the policy that created its need.
… store policy This leaf tightens the opt-in semantics (an in-memory replay store is forbidden on mainnet even with PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE=1; the opt-in remains honored off-mainnet), but the boot-policy contract shipped here still asserted the old opt-in-boots-on-mainnet behavior — an internally inconsistent pair that went unnoticed because the TS-harness leg died earlier on the Rust runner build on every PR. Bring forward the reconciled contract (and the createPayKit boot fixture it drives) from the subscription leaf, which asserts exactly this policy: fail closed on mainnet regardless of opt-in, boot with the opt-in on devnet, native fail-closed guards probed by subject. Green here: 10 passed, 8 pending-skips with owned reasons.
…ly accept This leaf's replay-store policy validates the MPP store contract whenever mpp is accepted; the over-ceiling fixture built its upto server with the default accept list, so the policy rejected its reserve-only store before the ceiling assertions ran. Bring forward the reconciled fixture, which pins accept:['x402'] — the surface actually under test — so the ceiling gate runs against the real X402Upto.settle verifier.
…ix/mpp-replay-store-hardening # Conflicts: # harness/test/boot-policy.test.ts
…ix/mpp-replay-store-hardening
The reconciled boot-policy contract this leaf carries drives the high-level createPayKit fixture, which needs three support pieces that lived one leaf later: the setup-harness build-pay-kit input (ordered @solana/mpp -> @solana/pay-kit build) wired into the TS-harness job, the harness package declaring the pay-kit file dependency, and the tsconfig exclusion that keeps the shared cross-language typecheck on its @solana/mpp-only baseline (the fixture is typechecked by the dedicated paths that actually build pay-kit). Without them the shared Build-harness-deps job typechecks a module that is never built there and every downstream leg dies.
…ix/mpp-replay-store-hardening
Summary
Security invariants
Verification
Base is #219. This is a protocol PR because the same replay-store capability contract spans multiple SDKs.