fix: surface outcome-unknown RPC failures and sanitize daemon error text - #3
Merged
erubboli merged 1 commit intoSep 19, 2026
Conversation
Implements the remediations for the two confirmed findings of the security audit of this repository (ref 9f9dfc5). F-1 (medium, wallet.fund-moving-rpc.response-loss-retry-double-spend): Transport::call converted every failure observed after the HTTP request was delivered (id mismatch, oversized or undecodable body, post-delivery transport errors) into the same public error variants used for pre-dispatch failures, so a consumer retrying on Err would re-issue a fund-moving mutation as a fresh second transaction. Every failure at or after delivery is now wrapped in RequestError::AfterDispatch and surfaces publicly as node/wallet::Error::OutcomeUnknown, including send-phase timeouts (the request was likely fully written); only failures that never established a connection stay plain. Response-loss semantics and the recovery contract are documented in docs/wallet.md. F-2 (low, jsonrpc-rpc-error-message-unsanitized-into-public-display): daemon-controlled JSON-RPC error messages flowed verbatim into the public error Display, enabling forged multi-line log entries and ANSI terminal escape injection, uncapped below the 64 MiB body limit. The message is now sanitized once at construction (control characters stripped, 8 KiB cap, trimmed) via a shared limits helper that the indexer client also uses, so the two transports cannot drift. Breaking: node::Error / wallet::Error gain OutcomeUnknown(Box<Self>) and IdMismatch / Json / ResponseTooLarge now arrive wrapped whenever the request was delivered; update matches on those variants to unwrap.
erubboli
self-requested a review
September 19, 2026 14:28
erubboli
approved these changes
Sep 19, 2026
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
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
Implements the remediations for the two confirmed findings of a security audit of this repository (six-phase audit, source ref
9f9dfc5, full report retained separately).F-1 (medium) — post-delivery failures are now outcome-unknown
Transport::callpreviously converted every failure observed after the HTTP request was delivered (id mismatch, oversized or undecodable body, post-delivery transport errors) into the same public error variants used for pre-dispatch failures. A consumer following the common retry-on-Errpattern would re-issue a fund-moving call (send,send_token,sweep_spendable,spend_utxo,deposit_data, and the staking/token/order mutators) as a fresh second transaction — the daemon builds and broadcasts a new transaction per call, and no layer made the retry safe. Locally reproduced during the audit: ambiguousErr(IdMismatch)after full delivery, then an identical retry completing as a second accepted payment with a distincttx_id.Fix: every failure at or after delivery is wrapped in
RequestError::AfterDispatchand surfaces publicly asnode::Error::OutcomeUnknown/wallet::Error::OutcomeUnknown— including send-phase timeouts (the request was likely fully written, so the outcome is inherently ambiguous); only failures that never established a connection (connection refused, DNS failure) stay plain. A daemon error envelope with a matching id remains a plainError::Rpc(a definitive refusal, not an unknown outcome). Response-loss semantics and the recovery contract (list_pending_transactions/transaction_get, or the retry-safebroadcast_to_mempool: Some(false)+submit_transactionflow) are documented indocs/wallet.md.F-2 (low) — daemon error text is sanitized before it reaches
DisplayDaemon-controlled JSON-RPC error messages flowed verbatim into the public error
Display(pinned as the canonical rendering byrpc_error_is_surfaced): valid JSON\n/\u001bescapes decode into real control characters, enabling forged multi-line audit/log lines and ANSI terminal escape injection, uncapped below the 64 MiB body limit. The indexer client already sanitized the identical data class; the JSON-RPC transports did not.Fix: the message is sanitized once at construction — control characters stripped, 8 KiB cap, trimmed — via
limits::sanitize_daemon_text, withMAX_ERROR_BODY_CHARSmoved to the sharedlimitsmodule so the indexer and JSON-RPC transports cannot drift. Reproduced fixed: single-line display, no ESC bytes, 1 MiB message truncated.Breaking change
node::Error/wallet::ErrorgainOutcomeUnknown(Box<Self>), andIdMismatch/Json/ResponseTooLargenow arrive wrapped whenever the request was delivered. Updatematches!arms on those variants to unwrap. Crate is pre-1.0; documented inCHANGELOG.mdunder[Unreleased].Tests
missing_response_id_is_rejected,oversized_responses_are_rejected(now expect the wrapped variants)rpc_error_message_is_sanitized(single-line, no ESC, 8 KiB cap, payload preserved),send_response_loss_is_outcome_unknown(the audit repro shape on the realsendpath),send_phase_timeout_is_outcome_unknown(raw-listener timeout →OutcomeUnknown(Transport)withis_timeout()),undecodable_result_is_outcome_unknown(pins theJsonarm)rpc_error_is_surfaced(node + wallet) unchanged and passing — proves benign messages and theRpcvariant are untouched by sanitization/wrappingVerification
cargo fmt --all --check— cleancargo clippy --no-default-features {,--features node,indexer,wallet,crypto,node,indexer,wallet,crypto} --all-targets --locked --offline -- -D warnings— zero warnings across the matrixcargo test --all-features --locked --offline— 92 passed, 0 failed; per-feature suites (node19,indexer18,wallet22,crypto28) all greencargo doc --all-features --locked --offline --no-deps— cleanThe changes also passed a focused independent security review of the diff (no code defects; the one low doc-precision finding it raised — send-phase timeout ambiguity — is addressed in this PR by both the
is_connect()classification and the docs).