fix(mpp): harden subscription activation#238
Conversation
|
@greptile-apps please review the current head. |
Greptile SummaryThis PR hardens MPP subscription activation across TypeScript and Rust by validating the complete canonical instruction sequence, plan-snapshot field binding, account identities and privileges, ATAs, and compute-budget limits before co-signing. It also reserves activation signatures atomically in the replay store before broadcast and releases the claim only on definitively-retryable failures, eliminating replay and double-settlement races.
Confidence Score: 5/5Safe to merge; all identified findings are non-blocking style or minor behavioral nuances rather than defects in the hardened activation path. The activation hardening logic is coherent and well-tested across 482 TypeScript and 376 Rust tests. All three findings are minor: an unnecessary blockhash RPC fetch when authority already existed, the feePayer consistency check being deferred from configure() to gate construction, and subscription challenges now carrying an expiry that they previously did not. The Rust server subscription.rs (+3,800 lines) warrants close secondary review given its volume. The decodeSubscriptionDelegation mapping (header.delegatee to planPda) relies on the Codama IDL being in sync with the deployed on-chain program. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant Server
participant Store
participant RPC
Note over Client,RPC: Subscription activation (fee-sponsored path)
Client->>Server: GET /subscription-endpoint (challenge request)
Server->>Client: 402 challenge (HMAC-bound, includes planSnapshot + recentBlockhash)
Client->>Client: initializeSubscriptionAuthority (if needed)
Client->>Client: buildSubscriptionActivationTransaction
Client->>Client: partial-sign as subscriber
Client->>Server: "credential { transaction: base64 }"
Server->>Server: assertSubscriptionCredentialBinding
Server->>Server: validateActivationInstructions
Server->>Server: coSignBase64Transaction
Server->>Server: signatureFromWireTransaction to consumedKey
Server->>Store: claimConsumed(consumedKey) [atomic putIfAbsent]
alt key already claimed
Store-->>Server: false
Server-->>Client: 400 already consumed
else key free
Store-->>Server: true
Server->>RPC: fetchSubscriptionDelegation
alt delegation already on-chain
Server->>RPC: isSignatureConfirmed
RPC-->>Server: confirmed
else delegation absent
Server->>RPC: simulateTransaction
Server->>RPC: sendTransaction
Server->>RPC: waitForConfirmation
end
Server->>RPC: fetchSubscriptionDelegation (verify terms)
Server-->>Client: Receipt
end
Note over Server,Store: On post-settlement error: releaseConsumedBestEffort
%%{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
participant Store
participant RPC
Note over Client,RPC: Subscription activation (fee-sponsored path)
Client->>Server: GET /subscription-endpoint (challenge request)
Server->>Client: 402 challenge (HMAC-bound, includes planSnapshot + recentBlockhash)
Client->>Client: initializeSubscriptionAuthority (if needed)
Client->>Client: buildSubscriptionActivationTransaction
Client->>Client: partial-sign as subscriber
Client->>Server: "credential { transaction: base64 }"
Server->>Server: assertSubscriptionCredentialBinding
Server->>Server: validateActivationInstructions
Server->>Server: coSignBase64Transaction
Server->>Server: signatureFromWireTransaction to consumedKey
Server->>Store: claimConsumed(consumedKey) [atomic putIfAbsent]
alt key already claimed
Store-->>Server: false
Server-->>Client: 400 already consumed
else key free
Store-->>Server: true
Server->>RPC: fetchSubscriptionDelegation
alt delegation already on-chain
Server->>RPC: isSignatureConfirmed
RPC-->>Server: confirmed
else delegation absent
Server->>RPC: simulateTransaction
Server->>RPC: sendTransaction
Server->>RPC: waitForConfirmation
end
Server->>RPC: fetchSubscriptionDelegation (verify terms)
Server-->>Client: Receipt
end
Note over Server,Store: On post-settlement error: releaseConsumedBestEffort
Reviews (14): Last reviewed commit: "Merge remote-tracking branch 'origin/fix..." | Re-trigger Greptile |
|
@greptile-apps please re-review the current head after the Rust blockhash parity fix. |
…o fix/mpp-subscription-hardening
|
@greptile-apps please re-review exact head 2bcee2d. This refreshes activation blockhashes after authority initialization, binds fee-payer identities, preserves fail-closed replay claims, and unifies the TypeScript reserve contract. |
|
@greptile-apps please review |
|
@greptile-apps please review |
…o fix/mpp-subscription-final
Session store: mark createMemorySessionStore() with an enumerable
global-symbol brand so a shallow spread copy ({ ...store }) still reports
as process-local via isMemorySessionStore and is rejected off-localnet. A
symbol key stays invisible to JSON.stringify and Object.keys regardless of
the enumerable flag, so nothing leaks into serialized output; the prior
non-enumerable brand was dropped by spread and could be smuggled onto
mainnet as durable. Export isMemorySessionStore from the server entrypoint
so pay-kit adapters can detect it across the package boundary.
Subscription store: require isShared and isDurable outside localnet and
route consumed-reservation cleanup through releaseConsumedBestEffort so a
failed delete keeps the reservation instead of throwing.
Adds session-store brand tests (factory detection, JSON/Object.keys
invisibility, spread fail-closed, non-memory-store rejection) and extends
the subscription-server tests.
…lnet Generalize the config store gate to requiresSettlementReplayStore, covering both mpp and x402 settlement paths, and materialize a single shared in-memory replay store only on the devnet opt-in (PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE=1). The session adapter rejects an explicit createMemorySessionStore() off-localnet via isMemorySessionStore so process-local session state cannot reach mainnet, and the x402 path is held to the same policy. Error copy generalized from "MPP replayStore" to "replayStore". Extends config, mpp-adapter, mpp-session, and paykit tests, including a new x402 config case.
Drive the TypeScript fail-closed store contract through a real createPayKit + requirePayment fixture (paykit-boot.ts) and require a covered probe only once its SDK carries the PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE guard marker in-tree (detected by git-grepping the SDK source). Go and TypeScript ship the guard here so they run; Python asserts-SKIP with a loud note until its remediation lands, after which the same grep auto-promotes it to a required probe. This keeps the leaf green without hardcoding "python is red". Wires the ordered @solana/mpp -> @solana/pay-kit build and a dedicated fixture typecheck into CI behind a build-pay-kit input, excludes the fixture from the mpp-only baseline typecheck, and bumps @solana/kit to ^6.10.0 (lockfile fan-out only: @solana/* to 6.10, undici-types transitive, commander dropped).
Repo text uses no em-dash; two remediation comment lines and two earlier subscription comment lines carried them.
|
@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.
|
@greptileai review |
The native source-contract probes asserted Rust markers PR #227 never ships (is_shared legacy capability, config.allow_unsafe_memory_store, the "atomic durable shared replay store is required" message) and PHP error text the php leaf never emits ("MPP requires an injected atomic durable/shared replay store", "does not affirm durable/shared capability"). Those probes were permanently red against the real SDK sources. Replace them with the same mechanism the #238 finalize agent uses: an asserted-skip roster for SDKs that do not implement the shared PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE fail-closed contract, plus a git-grep honest-roster test that REDs the moment any rostered SDK gains an opt-in reference, forcing promotion to a live probe instead of a silent skip. No probe now asserts a contract no leaf delivers.
…o boot-policy roster cc93323 dropped the php/ruby/lua source-contract probes and mislabeled all three in unimplementedProbes as having "no off-localnet fail-closed boot guard". That is false: each enforces the same money-path policy natively. - php src/Protocols/Mpp/Adapter.php + Server/SolanaChargeHandler.php reject an omitted or non-durable/non-shared replay store off localnet - ruby lib/pay_kit/protocols/mpp/runtime.rb raises unless localnet or a durable replay_store is supplied - lua protocols/mpp/init.lua + server/init.lua error unless localnet or a shared replay store is configured Remove php/ruby/lua from unimplementedProbes and cover them with native source-contract probes, gated by the same git-grep detection #238 uses (sdkFilesReferencingOptIn kept byte-identical; a parallel marker helper carries each SDK's native rejection string). A probe auto-requires when its guard is in-tree and asserts-skip with an honest reason otherwise. Patterns are verified against current source, unlike the stale strings cc93323 removed. rust/kotlin/swift stay asserted-skip (verified: no such guard in-tree).
The go/ts/python fail-closed probes required their SDK guard, but those remediations land in sibling leaves (#227/#238/#228), so in this harness leaf they hard-failed. Resolve each covered probe through the same sdkImplementsGuard git-grep mechanism the native php/ruby/lua probes already use: a probe is required only when its SDK source references the opt-in marker in-tree, else it asserts-skip with a loud PENDING note and auto-promotes to required at integration. Moves the REPO_ROOT const above the probe resolution to avoid a module-eval temporal-dead-zone error.
…olana-foundation/pay-kit into fix/mpp-subscription-final
|
@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).
| default: | ||
| throw new Error(`Unsupported MPP_HARNESS_NETWORK: ${network}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't we already have a function doing this in pay-kit?
There was a problem hiding this comment.
Right — replaced in ee9dd33: the fixture's hand-rolled network-slug switch is gone; it now passes the env value through pay-kit's exported toNetwork (the same normalization configure runs), so the harness pulls from the lib instead of re-implementing it. Fail-closed on an unknown network is preserved by configure's validateNetwork.
| let token_program = parse_pubkey(&method_details.token_program, "tokenProgram")?; | ||
| validate_subscription_token_program(&mint, &token_program, allow_unknown_token_2022)?; | ||
| let associated_token_program = parse_pubkey( | ||
| "ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL", |
There was a problem hiding this comment.
We should have a constant for this
There was a problem hiding this comment.
Done in e648022: it now reuses programs::ASSOCIATED_TOKEN_PROGRAM (the canonical id in mpp/protocol/solana.rs, already used for the token programs in this file) — the same literal at line 150 got the same treatment.
|
|
||
| let blockhash = rpc | ||
| .get_latest_blockhash() | ||
| .map_err(|e| Error::Other(format!("Failed to fetch SA init blockhash: {e}")))?; |
There was a problem hiding this comment.
this lib should have access to a block cache.
There was a problem hiding this comment.
Agreed — wired in 89dd1fe. The kit already had core::blockhash::BlockhashCache (45s TTL, used by the server challenge builders); the subscription client now routes its fetches through a process-wide per-endpoint instance (shared_blockhash_cache), so activation bursts collapse into one getLatestBlockhash within the TTL. Fail-closed: a failed fetch propagates and nothing past the TTL is ever served. The one deliberately direct fetch left is post-SA-init, which must NOT reuse the just-broadcast tx's blockhash.
The boot fixture hand-rolled a `networkSlug` switch that re-implemented the slug normalization + validation `createPayKit`/`configure` already runs via `toNetwork` and `validateNetwork`. Drop the local copy and pass the env value straight to `createPayKit` through the exported `toNetwork`, so the harness pulls the behavior from the lib instead of duplicating it. Fail-closed on an unknown network is preserved (configure's `validateNetwork` throws), and the mainnet-must-reject boot probe is unaffected (it fails on the replay-store policy, not the network name). Resolves lgalabru's review comment on paykit-boot.ts:15.
…on client Both ATA derivations in the subscription client passed the raw `ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL` literal to `parse_pubkey`. The crate already exports this address as `programs::ASSOCIATED_TOKEN_PROGRAM` (the constant this file already uses for TOKEN_PROGRAM / TOKEN_2022_PROGRAM), so reuse it at both sites instead of repeating the literal. Resolves lgalabru's review comment on subscription.rs:394.
…ed cache The subscription activation client fetched a fresh blockhash over RPC on every call (SA init tx, and the activation-tx fallbacks), so repeated or concurrent activations each paid a getLatestBlockhash round-trip. Reuse the crate's existing core::blockhash::BlockhashCache: add a fail-closed `get_or_fetch` fetch-through and a process-wide `shared_blockhash_cache(rpc_url)` keyed by endpoint, then route the client's recent-blockhash fetches through it. Safety: the cache never serves an entry past its TTL (MAX_AGE) and a failed fetch propagates instead of returning stale data. The post-SA-init activation fetch stays deliberately uncached and direct, because it must be a blockhash distinct from the init tx we just broadcast. Keyed by URL so a process talking to two networks never serves one network's blockhash to the other. Resolves lgalabru's review comment on subscription.rs:415.
…into fix/mpp-subscription-hardening # Conflicts: # .github/workflows/ci.yml
…dening The subscription leaf's CI leg used to stop at the coverage gate, so its clippy step never ran and 31 mechanical lints accumulated (28 err_expect, 1 useless_format, 1 useless_conversion, 1 too_many_arguments). Fix them shape-only: every assertion keeps asserting the same property and no behavior changes. The one too_many_arguments hit takes the allow the file already uses for its sibling validate_open_instruction; the twelve err().expect() sites whose Ok type is not Debug stay as-is because clippy intentionally skips them.
…into fix/mpp-subscription-hardening # Conflicts: # rust/crates/kit/src/mpp/server/subscription.rs
…into fix/mpp-subscription-hardening
…into fix/mpp-subscription-hardening
…into fix/mpp-subscription-hardening
Summary
Security invariants
Verification
Repository-wide TypeScript typecheck still reports the existing TS2742 declaration-portability errors in the MPP client/server factories; this branch does not introduce a new error class. Repository-wide Rust Clippy likewise reaches only pre-existing
err().expect()warnings in unchanged protocol tests.Base is #219. This is a protocol PR because the activation transaction contract and replay invariant are shared by the TypeScript and Rust SDKs.