From f18563d2c23f854ef941489a14648f0b08467571 Mon Sep 17 00:00:00 2001 From: nullPointerEnjoyer Date: Sat, 19 Sep 2026 18:14:33 +0400 Subject: [PATCH] fix: surface outcome-unknown RPC failures and sanitize daemon error text 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) and IdMismatch / Json / ResponseTooLarge now arrive wrapped whenever the request was delivered; update matches on those variants to unwrap. --- CHANGELOG.md | 29 ++++++++++++++++ docs/node.md | 8 +++++ docs/wallet.md | 32 ++++++++++++++++-- src/indexer/mod.rs | 11 ++----- src/jsonrpc.rs | 70 ++++++++++++++++++++++++++++++++------- src/limits.rs | 18 ++++++++++ tests/node.rs | 82 +++++++++++++++++++++++++++++++++++++++++----- tests/wallet.rs | 79 ++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 296 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06dc33f..ea70186 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,35 @@ All notable changes to the Mintlayer Rust SDK are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## [Unreleased] + +### Added + +- `node::Error::OutcomeUnknown` / `wallet::Error::OutcomeUnknown`: the + request was delivered but its outcome could not be confirmed (lost, + oversized, undecodable or mismatched response, or a timeout while + waiting for the response), so a mutating call may already have taken + effect. `IdMismatch`, `Json`, `ResponseTooLarge` and ambiguous + transport errors now arrive wrapped in this variant; only failures that + never established a connection (e.g. connection refused, DNS resolution + failure) surface as the plain transport variant. + +### Changed + +- **Breaking:** post-delivery transport failures surface as + `OutcomeUnknown(...)` instead of the bare variant, so a retry on `Err` + can no longer silently double a fund-moving mutation; update matches on + `IdMismatch` / `Json` / `ResponseTooLarge` to unwrap the new variant. + +### Fixed + +- Daemon-controlled JSON-RPC error messages are sanitized before they + reach the public error `Display`: control characters are stripped and + the message is capped at 8 KiB (matching the indexer client), defusing + forged multi-line log entries and terminal escape injection. The limit + and sanitizer live in the shared `limits` module so the indexer and + JSON-RPC transports cannot drift. + ## [0.1.0] - 2026-09-17 ### Added diff --git a/docs/node.md b/docs/node.md index 3ea57bc..867f96a 100644 --- a/docs/node.md +++ b/docs/node.md @@ -28,6 +28,14 @@ Errors are returned as `node::Error`: - `Error::Json(serde_json::Error)` — the response could not be decoded. - `Error::IdMismatch { expected, actual }` — response id mismatch. - `Error::ResponseTooLarge { limit }` — the 64 MiB cap was exceeded. +- `Error::OutcomeUnknown(Error)` — the request was delivered but its outcome + could not be confirmed (lost, oversized, undecodable or mismatched + response; also a timeout while waiting for the response); for mutating + calls the daemon may already have acted, so check for side effects before + retrying. `IdMismatch`, `Json`, `ResponseTooLarge` and ambiguous + transport errors arrive wrapped in this variant; only failures that + never established a connection (e.g. connection refused, DNS resolution + failure) surface as the plain variant. --- diff --git a/docs/wallet.md b/docs/wallet.md index aeed62e..cdc55ca 100644 --- a/docs/wallet.md +++ b/docs/wallet.md @@ -20,9 +20,10 @@ let c = wallet::Client::builder("http://127.0.0.1:3034") Errors are returned as `wallet::Error` — `Error::Rpc { code, message }` for daemon errors, plus `Http`, `Json`, -`IdMismatch` and `ResponseTooLarge` transport variants. Basic-auth -credentials are redacted from `Debug` output; response bodies are capped -at 64 MiB. +`IdMismatch`, `ResponseTooLarge` and `OutcomeUnknown` transport variants. +Basic-auth credentials are redacted from `Debug` output; response bodies +are capped at 64 MiB, and daemon error messages are sanitized (control +characters stripped, 8 KiB cap) before they reach the error value. --- @@ -94,6 +95,31 @@ Most send methods take `TxOptions { in_top_x_mb, broadcast_to_mempool }`: `broadcast_to_mempool: Some(false)` builds and signs without broadcasting while still returning the transaction hex. +## Response loss + +**Never retry a fund-moving call (`send`, `send_token`, +`sweep_spendable`, `spend_utxo`, `deposit_data`, and the staking, token +and order mutators) on `Err` without checking the outcome first.** The +daemon builds, signs and broadcasts the transaction as soon as the +request arrives — before the response is read. If the response is then +lost, oversized, undecodable, or carries a mismatched id, the call +returns `Error::OutcomeUnknown` (wrapping the underlying transport +error): the daemon may already have broadcast the transaction, while a +pre-dispatch failure such as a connection refusal surfaces as the plain +transport variant. + +On `OutcomeUnknown`, check `list_pending_transactions` / +`transaction_get` before re-issuing the request, or use the retry-safe +flow: `TxOptions { broadcast_to_mempool: Some(false) }` followed by +`submit_transaction` — resubmitting the same transaction hex converges +on one transaction, so retries are safe. + +A timeout while waiting for the response is inherently ambiguous: the +request was likely fully written, so the client also classifies it as +`OutcomeUnknown`. Only failures that never established a connection +(e.g. connection refused, DNS resolution failure) are known to be +pre-dispatch and surface as the plain transport variant. + ## Staking See [staking.md](staking.md) for the full guide. diff --git a/src/indexer/mod.rs b/src/indexer/mod.rs index 975b62b..27e5cd4 100644 --- a/src/indexer/mod.rs +++ b/src/indexer/mod.rs @@ -33,9 +33,6 @@ pub use types::{ use crate::limits::DEFAULT_TIMEOUT; -/// Upper bound for the characters kept from a daemon error body. -const MAX_ERROR_BODY_CHARS: usize = 8 * 1024; - /// Client for the Mintlayer indexer REST API (api-web-server). #[derive(Debug, Clone)] pub struct Client { @@ -106,14 +103,10 @@ impl Client { let status = response.status(); if status.is_client_error() || status.is_server_error() { let bytes = Self::read_capped(response).await.unwrap_or_default(); - let body: String = String::from_utf8_lossy(&bytes) - .chars() - .filter(|c| !c.is_control()) - .take(MAX_ERROR_BODY_CHARS) - .collect(); + let body = crate::limits::sanitize_daemon_text(&String::from_utf8_lossy(&bytes)); return Err(Error::Http { status_code: status.as_u16(), - body: body.trim().to_owned(), + body, }); } let bytes = Self::read_capped(response).await?; diff --git a/src/jsonrpc.rs b/src/jsonrpc.rs index cd3be94..243935f 100644 --- a/src/jsonrpc.rs +++ b/src/jsonrpc.rs @@ -69,6 +69,15 @@ macro_rules! define_error { /// The limit that was exceeded. limit: usize, }, + /// The request was delivered, but its outcome could not be + /// confirmed: the response was lost, oversized, undecodable, or + /// did not carry the matching id. For mutating calls the daemon + /// may already have acted; check for side effects before + /// retrying. + #[error( + "request was delivered but its outcome is unknown (the daemon may have acted): {0}" + )] + OutcomeUnknown(Box), } impl From for $name { @@ -83,6 +92,9 @@ macro_rules! define_error { crate::jsonrpc::RequestError::ResponseTooLarge { limit } => { Self::ResponseTooLarge { limit } } + crate::jsonrpc::RequestError::AfterDispatch(inner) => { + Self::OutcomeUnknown(Box::new(Self::from(*inner))) + } } } } @@ -114,6 +126,13 @@ pub(crate) enum RequestError { }, #[error("daemon response exceeds the maximum accepted size of {limit} bytes")] ResponseTooLarge { limit: usize }, + /// Marks every failure observed after the HTTP request was delivered: + /// the daemon may have acted before the response became unreadable, so + /// a caller cannot distinguish "the call never happened" from "the + /// call may have succeeded". For mutating calls this must be checked + /// before a retry. + #[error("request was delivered but its outcome is unknown (the daemon may have acted): {0}")] + AfterDispatch(Box), } #[derive(Serialize)] @@ -177,30 +196,57 @@ impl Transport { // The HTTP status code is intentionally not inspected: the daemon may // answer with a valid JSON-RPC envelope on a non-2xx status (parity // with the go-sdk client). - let http_response = builder.send().await?; - let response: Response = read_json_body(http_response).await?; + let http_response = builder.send().await.map_err(|err| { + // A failure while establishing the connection cannot have + // reached the daemon, so it is a genuine pre-dispatch failure. + // Every other send-phase failure (a timeout while waiting for + // the response headers, a connection reset mid-write) is + // inherently ambiguous: the request may have been fully written + // and the daemon may have acted, so it is classified as + // outcome-unknown. + if err.is_connect() { + RequestError::Http(err) + } else { + RequestError::AfterDispatch(Box::new(RequestError::Http(err))) + } + })?; + // From here on the request has been delivered: any failure means the + // daemon may have acted before the response became unreadable, which + // callers must be able to distinguish from a pre-dispatch failure. + let response: Response = read_json_body(http_response) + .await + .map_err(|err| RequestError::AfterDispatch(Box::new(err)))?; match response.id { Some(actual) if actual.as_u64() == Some(id) => {} Some(actual) => { - return Err(RequestError::IdMismatch { - expected: id, - actual, - }); + return Err(RequestError::AfterDispatch(Box::new( + RequestError::IdMismatch { + expected: id, + actual, + }, + ))); } None => { - return Err(RequestError::IdMismatch { - expected: id, - actual: serde_json::Value::Null, - }); + return Err(RequestError::AfterDispatch(Box::new( + RequestError::IdMismatch { + expected: id, + actual: serde_json::Value::Null, + }, + ))); } } + // An error envelope carrying the matching id is a definitive answer + // from the daemon (it refused the request), not an unknown outcome. + // The daemon-controlled message is sanitized before it reaches the + // public error value. if let Some(err) = response.error { return Err(RequestError::Rpc { code: err.code, - message: err.message, + message: crate::limits::sanitize_daemon_text(&err.message), }); } - Ok(serde_json::from_value(response.result)?) + serde_json::from_value(response.result) + .map_err(|err| RequestError::AfterDispatch(Box::new(RequestError::Json(err)))) } } diff --git a/src/limits.rs b/src/limits.rs index 73e821c..54202ed 100644 --- a/src/limits.rs +++ b/src/limits.rs @@ -12,6 +12,24 @@ use std::time::Duration; /// exhaustion from a misconfigured or hostile endpoint. pub(crate) const MAX_RESPONSE_BYTES: usize = 64 * 1024 * 1024; +/// Upper bound for the characters kept from daemon-controlled error text +/// before it is embedded in a public error value. +pub(crate) const MAX_ERROR_BODY_CHARS: usize = 8 * 1024; + +/// Neutralizes daemon-controlled error text before it is embedded in a +/// public error value: control characters are stripped (defusing forged +/// multi-line log entries and terminal escape sequences), the length is +/// capped at [`MAX_ERROR_BODY_CHARS`], and surrounding whitespace is +/// trimmed. +pub(crate) fn sanitize_daemon_text(raw: &str) -> String { + raw.chars() + .filter(|c| !c.is_control()) + .take(MAX_ERROR_BODY_CHARS) + .collect::() + .trim() + .to_owned() +} + /// Default request timeout for the daemon HTTP clients. pub(crate) const DEFAULT_TIMEOUT: Duration = Duration::from_secs(30); diff --git a/tests/node.rs b/tests/node.rs index 19488be..d649720 100644 --- a/tests/node.rs +++ b/tests/node.rs @@ -118,11 +118,14 @@ async fn missing_response_id_is_rejected() { let client = Client::new(server.url("/")); match client.best_block_height().await { - Err(Error::IdMismatch { expected, actual }) => { - assert_eq!(expected, 1); - assert_eq!(actual, serde_json::Value::Null); - } - other => panic!("expected IdMismatch, got: {other:?}"), + Err(Error::OutcomeUnknown(inner)) => match *inner { + Error::IdMismatch { expected, actual } => { + assert_eq!(expected, 1); + assert_eq!(actual, serde_json::Value::Null); + } + other => panic!("expected IdMismatch, got: {other:?}"), + }, + other => panic!("expected OutcomeUnknown(IdMismatch), got: {other:?}"), } } @@ -148,6 +151,64 @@ async fn rpc_error_is_surfaced() { assert_eq!(display, "RPC error -32601: Method not found"); } +#[tokio::test] +async fn rpc_error_message_is_sanitized() { + let server = MockServer::start(); + // A hostile daemon forges a multi-line audit-log entry containing an + // ANSI escape sequence and pads it past the 8 KiB cap; the sanitized + // message must be a single line, escape-free, and length-capped. + let message = "rejected\n2026-09-19T10:00:00Z ERROR audit: wallet drained for \ + operator=admin\u{1b}[31mINJECTED\u{1b}[0m" + .to_string() + + &"x".repeat(9 * 1024); + mock_rpc( + &server, + "\"jsonrpc\"".to_string(), + rpc_error(1, -32600, &message), + ); + + let client = Client::new(server.url("/")); + let err = client.best_block_height().await.unwrap_err(); + let display = err.to_string(); + match err { + Error::Rpc { code, message } => { + assert_eq!(code, -32600); + // The newline was stripped and the payload preserved, not emptied. + assert!(message.starts_with("rejected2026-")); + } + other => panic!("expected Error::Rpc, got {other:?}"), + } + // No forged multi-line output. + assert_eq!(display.lines().count(), 1); + // No terminal escape sequence survived. + assert!(!display.contains('\u{1b}')); + // 8 KiB message cap plus slack for the "RPC error -32600: " prefix. + assert!(display.chars().count() <= 8 * 1024 + 64); +} + +// The result-decode failure happens after the request was delivered, so it must surface as outcome-unknown. +#[tokio::test] +async fn undecodable_result_is_outcome_unknown() { + let server = MockServer::start(); + let mock = mock_rpc( + &server, + "\"jsonrpc\"".to_string(), + rpc_ok(1, json!("not-a-number")), + ); + + let client = Client::new(server.url("/")); + match client.best_block_height().await { + Err(Error::OutcomeUnknown(inner)) => match *inner { + Error::Json(_) => {} + other => panic!("expected Json, got: {other:?}"), + }, + other => panic!("expected OutcomeUnknown(Json), got: {other:?}"), + } + // The mock was hit: the request really was delivered before the decode + // failed, which is what makes the outcome unknown. + assert_eq!(mock.hits(), 1); +} + #[tokio::test] async fn basic_auth_header_is_sent() { let server = MockServer::start(); @@ -435,10 +496,13 @@ async fn oversized_responses_are_rejected() { }); let client = Client::new(server.url("/")); match client.best_block_height().await { - Err(Error::ResponseTooLarge { limit }) => { - assert_eq!(limit, 64 * 1024 * 1024); - } - other => panic!("expected ResponseTooLarge, got: {other:?}"), + Err(Error::OutcomeUnknown(inner)) => match *inner { + Error::ResponseTooLarge { limit } => { + assert_eq!(limit, 64 * 1024 * 1024); + } + other => panic!("expected ResponseTooLarge, got: {other:?}"), + }, + other => panic!("expected OutcomeUnknown(ResponseTooLarge), got: {other:?}"), } } diff --git a/tests/wallet.rs b/tests/wallet.rs index 8c6eec7..cc234e4 100644 --- a/tests/wallet.rs +++ b/tests/wallet.rs @@ -10,6 +10,7 @@ mod common; use std::sync::Arc; +use std::time::Duration; use common::{mock_rpc, respond, rpc_error, rpc_ok}; use httpmock::prelude::*; @@ -174,6 +175,84 @@ async fn send_params_wire_shape() { assert_eq!(mock.hits(), 1); } +/// A `send` response that fails verification after the request was delivered +/// (here: an envelope with the wrong id) must surface as `OutcomeUnknown`, +/// distinct from a pre-dispatch failure, so a retry-on-Err cannot silently +/// double a fund-moving mutation. +#[tokio::test] +async fn send_response_loss_is_outcome_unknown() { + let server = MockServer::start(); + // Valid SendResult payload, but the envelope answers id 999 instead of + // the request's id 1: the daemon may already have built and broadcast + // the transaction before its response became unverifiable. + mock_rpc( + &server, + "\"jsonrpc\"".to_string(), + rpc_ok(999, send_result()), + ); + + let client = Client::new(server.url("/")); + let params = SendParams { + account: 0, + address: "mtc1qsending".to_string(), + amount: Amount::from_atoms(1000), + selected_utxos: Vec::new(), + options: TxOptions::default(), + }; + match client.send(params).await { + Err(Error::OutcomeUnknown(inner)) => match *inner { + Error::IdMismatch { expected, actual } => { + assert_eq!(expected, 1); + assert_eq!(actual, serde_json::Value::from(999)); + } + other => panic!("expected IdMismatch, got: {other:?}"), + }, + other => panic!("expected OutcomeUnknown(IdMismatch), got: {other:?}"), + } +} + +/// A timeout while waiting for response headers is outcome-ambiguous: the +/// request was likely fully written, so the daemon may already have built +/// and broadcast the transaction. It must therefore surface as +/// `OutcomeUnknown`, not the plain `Transport` variant. +#[tokio::test] +async fn send_phase_timeout_is_outcome_unknown() { + // Raw TCP endpoint (no httpmock) that accepts the connection but never + // writes a response: the client sends its request, then times out + // waiting for response headers. + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let port = listener.local_addr().unwrap().port(); + std::thread::spawn(move || { + // Hold the accepted connection open (never answering) past the + // client's 200 ms timeout so the send fails as a read timeout, + // not a premature connection close; a lingering thread after the + // test ends is harmless. + if let Ok((stream, _)) = listener.accept() { + std::thread::sleep(Duration::from_millis(1000)); + drop(stream); + } + }); + + let client = Client::builder(format!("http://127.0.0.1:{port}/")) + .timeout(Duration::from_millis(200)) + .build() + .unwrap(); + let params = SendParams { + account: 0, + address: "mtc1qsending".to_string(), + amount: Amount::from_atoms(1000), + selected_utxos: Vec::new(), + options: TxOptions::default(), + }; + match client.send(params).await { + Err(Error::OutcomeUnknown(inner)) => match *inner { + Error::Transport(err) => assert!(err.is_timeout(), "expected a timeout, got {err:?}"), + other => panic!("expected Transport, got: {other:?}"), + }, + other => panic!("expected OutcomeUnknown(Transport), got: {other:?}"), + } +} + #[tokio::test] async fn submit_transaction_hardcodes_trusted() { let server = MockServer::start();