fix(ruby): MPP replay-store fail-closed + x402 accepted-amount binding (+ #216 hardening)#222
Conversation
The Ruby slice of the #216 pre-release security hardening, plus the residual fixes the bedrock audit found on top of it, re-delivered onto #219 (harness). No harness/CI files — those live in #219. Security behavior: - MPP replay-store fail-closed off-localnet (config/runtime/rack wiring); localnet keeps the dev MemoryStore with a warning. BREAKING: non-localnet Mpp.create now requires a durable replay_store. - x402 v2 accepted.amount binding: a credential that echoes a drifting amount / maxAmountRequired is rejected (aligned to the Rust verifier exact.rs:816); omission still tolerated for v2 compatibility. - plus the rest of #216's Ruby hardening. Verified: mise ruby@3.4.5 bundle exec rake test -> 463 runs, 0 failures, 0 errors.
Greptile SummaryThis PR delivers the Ruby slice of the #216 security hardening: MPP replay-store fail-closed enforcement off localnet, x402 v2
Confidence Score: 5/5The three blocking concerns from the previous review round are all correctly resolved, and no new correctness or security issues were introduced. The webrick CVE exposure is gone, the nil replay_store passthrough is fixed, accepted_requirement_matches? omission-tolerance now gates on the credential rather than the server requirement, and the lastValidBlockHeight release path is double-gated by a server-side isBlockhashValid call so client-echoed hints cannot independently free a replay reservation. The one new finding is a documentation gap around SettlementCache#delete that only affects custom cache implementations, not the default in-process path. ruby/lib/pay_kit/protocols/x402/server/exact.rb — custom settlement_cache implementors now need to provide delete in addition to put_if_absent and duplicate?; this requirement is not surfaced in the constructor docs or a base class. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant Rack as Rack Middleware
participant Mpp as Mpp.create
participant X402 as X402 Server::Exact
participant RPC as Solana RPC
Client->>Rack: POST /pay (credential)
Rack->>Mpp: "create(**options) [replay_store forwarded only if non-nil]"
alt non-localnet and no durable store
Mpp-->>Rack: ConfigurationError
end
Rack->>X402: settle_payment(decoded, requirements)
X402->>X402: accepted_requirement_matches?(accepted, candidate)
note over X402: left_amount.nil? pass v2 compat, left_amount != right_amount reject
X402->>RPC: broadcast signed tx
RPC-->>X402: signature
X402->>X402: settlement_cache.put_if_absent(consumed_key)
note over X402: RESERVE before confirmation poll
X402->>RPC: getSignatureStatuses poll loop
alt confirmed
RPC-->>X402: status present
X402-->>Client: 200 + X-PAYMENT-RESPONSE
else timeout TransactionNotFound
X402->>RPC: isBlockhashValid(tx.message.blockhash)
alt blockhash expired trusted RPC
RPC-->>X402: false
X402->>X402: settlement_cache.delete(consumed_key)
X402-->>Client: TransactionNotFound
else blockhash still valid or RPC error
X402-->>Client: timed out reservation KEPT
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 Rack as Rack Middleware
participant Mpp as Mpp.create
participant X402 as X402 Server::Exact
participant RPC as Solana RPC
Client->>Rack: POST /pay (credential)
Rack->>Mpp: "create(**options) [replay_store forwarded only if non-nil]"
alt non-localnet and no durable store
Mpp-->>Rack: ConfigurationError
end
Rack->>X402: settle_payment(decoded, requirements)
X402->>X402: accepted_requirement_matches?(accepted, candidate)
note over X402: left_amount.nil? pass v2 compat, left_amount != right_amount reject
X402->>RPC: broadcast signed tx
RPC-->>X402: signature
X402->>X402: settlement_cache.put_if_absent(consumed_key)
note over X402: RESERVE before confirmation poll
X402->>RPC: getSignatureStatuses poll loop
alt confirmed
RPC-->>X402: status present
X402-->>Client: 200 + X-PAYMENT-RESPONSE
else timeout TransactionNotFound
X402->>RPC: isBlockhashValid(tx.message.blockhash)
alt blockhash expired trusted RPC
RPC-->>X402: false
X402->>X402: settlement_cache.delete(consumed_key)
X402-->>Client: TransactionNotFound
else blockhash still valid or RPC error
X402-->>Client: timed out reservation KEPT
end
end
Reviews (14): Last reviewed commit: "Merge remote-tracking branch 'origin/spl..." | Re-trigger Greptile |
… CVE ignore Greptile 3/5: the amount tolerance predicate was keyed on `right` (the server's candidate requirement, which always carries the amount), so a v2 credential that legitimately OMITS the amount was rejected instead of tolerated. Fix: match scheme/network/asset/payTo strictly, and bind the amount by value (across `amount`/`maxAmountRequired`) only when the credential echoes one -- omission is tolerated (the facilitator fills it in), drift is rejected. Mirrors the Rust verifier. Adds a regression test for the omission-tolerated case. Also ignore CVE-2026-38969 (webrick trailer reparse) in bundler-audit: webrick is a transitive test/tooling dep (no require in the SDK runtime), no patched version released yet; documented in ruby/.bundler-audit.yml with a removal trigger. Verified: standardrb clean; COVERAGE runner 464 runs 0 failures (96.8% line / 90.21% branch); bundle-audit reports no vulnerabilities.
|
Fixed the 3/5 finding: the amount tolerance predicate was reversed (keyed on the server candidate @greptile-apps please review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile: webrick is a declared runtime dependency in solana-pay-kit.gemspec (a Rack server backend alongside puma), not a test/tooling-only dep as the previous comment claimed. Corrected the rationale: the SDK never requires webrick directly, puma is the recommended production server, the CVE has no patched release yet, and the removal trigger notes dropping webrick from runtime deps as the alternative.
|
Fixed the 4/5 note: you're right, webrick is a declared runtime dependency in @greptile-apps please review |
Two Greptile findings: - webrick was declared as a runtime dependency, exposing downstream consumers to CVE-2026-38969. Moved it to a dev-only dependency (the SDK never requires it at runtime; puma is the production server), so consumers no longer receive it. bundler-audit still sees it in the dev lockfile (also via yard), documented ignore retained. - invoke_signature_confirmer only detected :key/:keyreq params, so a confirmer that accepts **kwargs (:keyrest) was called without the definitive-expiry last_valid_block_height. Now the :keyrest splat is honored. Adds a regression test. Verified: gemspec validate, standardrb clean, 465 tests green, bundle-audit clean.
|
Both remaining findings fixed (7d... see commit):
gemspec validate + standardrb clean, 465 tests green, bundle-audit clean. @greptile-apps please review |
…o fix/ruby-security-hardening
|
@greptile-apps please review |
| options = { | ||
| method: method, | ||
| secret_key: secret, | ||
| realm: realm, | ||
| replay_store: replay_store, | ||
| expires_in: expires_in | ||
| ) | ||
| } | ||
| options[:replay_store] = replay_store unless replay_store.nil? | ||
| ::PayKit::Protocols::Mpp.create(**options) |
There was a problem hiding this comment.
replay_store: replay_store is unconditionally included in the options hash on line 278 even when replay_store is nil (the MppConfig default). The subsequent line 281 is dead code — it re-assigns the same value when not nil, and is a no-op when nil because line 278 already stored it. The result: Mpp.create receives replay_store: nil instead of letting the default parameter kick in, which hits the explicit nil guard in runtime.rb:69 and raises ConfigurationError: "nil is not a valid store" rather than falling through to the intended localnet DEV_ONLY_MEMORY_STORE warning + MemoryStore fallback.
Any localnet user of the Rack middleware who has not set config.mpp.replay_store will get an unconditional ConfigurationError instead of the documented dev-friendly behaviour. The fix is to remove the replay_store: entry from the initial hash so the conditional assignment on line 281 is the sole writer.
| end | ||
|
|
||
| # Invoke the injected confirmer while preserving its 2-arg public | ||
| # contract. The default confirmer (`await_confirmation`) accepts a | ||
| # `last_valid_block_height:` keyword for the definitive expiry check; | ||
| # injected confirmers that do not declare it are still called with | ||
| # exactly `(config, signature)`. | ||
| def invoke_signature_confirmer(config, signature, last_valid_block_height) |
There was a problem hiding this comment.
lastValidBlockHeight governs a security-critical release
credential_last_valid_block_height reads decoded["accepted"]["extra"]["lastValidBlockHeight"], which is attacker-controlled input. A malicious client can set this to 0 or 1, making block_height > last_valid_block_height trivially true and collapsing the "definitively never landed" proof to only the getSignatureStatuses-absent check. Under network congestion, if the confirmation poll (10 s default) exhausts before the transaction reaches the "processed" commitment level, the absent-status window is exploitable: the reservation is released while the transaction is still live, which can enable a second settlement attempt. The fix is to embed lastValidBlockHeight in the server-side challenge record and read it from there, rather than trusting the echoed credential value.
…through Resolve the two P1 security findings on this PR: - exact.rb: the replay-reservation release no longer trusts the client-echoed accepted.extra.lastValidBlockHeight. A maliciously-low echoed height could force the confirmer to raise TransactionNotFound and release the reservation while the transaction was still in-flight (premature release -> replay / double-serve). Release now requires the transaction's OWN signed recentBlockhash to be independently proven expired on-chain (isBlockhashValid == false) via a trusted gate (injectable blockhash_validator, real RPC by default); fail-closed on nil/ambiguous/error. The echoed height remains only a non-authoritative confirmer hint. - payment_required.rb: stop inserting an explicit replay_store: nil into the Mpp.create options, which defeated the localnet MemoryStore fallback (a dev with no store hit ConfigurationError). Only forward a configured store. Regressions: premature-release blocked while blockhash still valid, genuine release on proven expiry, validator-error fail-closed, localnet MemoryStore fallback vs non-localnet fail-closed. rake test 483/0; standardrb clean.
|
Both P1s addressed:
Regression tests cover premature-release-blocked, genuine-release-on-expiry, validator-error fail-closed, and the localnet fallback. rake test 483/0, standardrb clean, branch coverage 90.07%. @greptile-apps please review |
…o fix/ruby-security-hardening
…o fix/ruby-security-hardening
…o fix/ruby-security-hardening
2 participants
Ruby slice of the #216 pre-release security hardening, re-delivered onto #219 (base =
split/pr216-ci-harness-mega), plus the residual fixes the bedrock language audit found on top of it. No harness/CI files — those live in #219. First of the per-language / per-protocol PRs that flow into #219; when they all land, #219's SDK conformance/behavior gates go green.Security behavior
config.rb,runtime.rb,rack/payment_required.rb). Localnet keeps the devMemoryStorewith a warning. BREAKING: non-localnetMpp.createnow requires a durablereplay_store(raisesConfigurationError) — prevents same-signature replay across workers/restarts.accepted.amountbinding (schemes/exact/types.rb): a credential echoing a driftingamount/maxAmountRequiredis now rejected, aligned to the Rust verifier (rust/crates/kit/src/x402/server/exact.rs:816); omission is still tolerated for v2 client compatibility.Verification
mise ruby@3.4.5 bundle exec rake test→ 463 runs, 1377 assertions, 0 failures, 0 errors. New/updated regression tests cover the fail-closed store wiring and the accepted-amount tamper case.Follow-up
refactor(ruby): idiomatic (Shopify Ruby + rubystyle.guide)will follow as a separate PR (keeping the behavior fix and the style pass reviewable independently), also targeting #219.