diff --git a/cmd/soroban-cli/src/commands/doctor.rs b/cmd/soroban-cli/src/commands/doctor.rs index 994bcabcd4..e5d5ffbd3f 100644 --- a/cmd/soroban-cli/src/commands/doctor.rs +++ b/cmd/soroban-cli/src/commands/doctor.rs @@ -9,11 +9,12 @@ use crate::{ config::{ self, data, locator::{self, KeyType}, - network::{redact_rpc_url, Network, DEFAULTS as DEFAULT_NETWORKS}, + network::{Network, DEFAULTS as DEFAULT_NETWORKS}, }, print::Print, rpc, upgrade_check::has_available_upgrade, + utils::url::redact_url, }; #[derive(Parser, Debug, Clone)] @@ -100,7 +101,7 @@ async fn print_network( print.globeln(format!( "{prefix} {name:?} ({})", - redact_rpc_url(&network.rpc_url) + redact_url(&network.rpc_url) )); print.blankln(format!("protocol {}", version_info.protocol_version)); print.blankln(format!("rpc {}", version_info.version)); @@ -123,7 +124,7 @@ async fn inspect_networks(print: &Print, config_locator: &locator::Args) -> Resu if print_network(true, print, &name, &network).await.is_err() { print.warnln(format!( "Default network {name:?} ({}) is unreachable", - redact_rpc_url(&network.rpc_url) + redact_url(&network.rpc_url) )); } } @@ -133,7 +134,7 @@ async fn inspect_networks(print: &Print, config_locator: &locator::Args) -> Resu if print_network(false, print, name, &network).await.is_err() { print.warnln(format!( "Network {name:?} ({}) is unreachable", - redact_rpc_url(&network.rpc_url) + redact_url(&network.rpc_url) )); } } diff --git a/cmd/soroban-cli/src/commands/network/info.rs b/cmd/soroban-cli/src/commands/network/info.rs index 3fea47a3fc..090f2cf707 100644 --- a/cmd/soroban-cli/src/commands/network/info.rs +++ b/cmd/soroban-cli/src/commands/network/info.rs @@ -2,6 +2,7 @@ use sha2::{Digest, Sha256}; use crate::commands::global; use crate::config::network; +use crate::utils::url::redact_url; use crate::{config, print, rpc}; #[derive(thiserror::Error, Debug)] @@ -93,7 +94,7 @@ impl Cmd { build_timestamp: version_result.build_timestamp, captive_core_version: version_result.captive_core_version, protocol_version: network_result.protocol_version, - friendbot_url: network_result.friendbot_url, + friendbot_url: network_result.friendbot_url.map(|u| redact_url(&u)), passphrase: network_result.passphrase, }; diff --git a/cmd/soroban-cli/src/commands/network/ls.rs b/cmd/soroban-cli/src/commands/network/ls.rs index 307ba0e98b..f3057b3024 100644 --- a/cmd/soroban-cli/src/commands/network/ls.rs +++ b/cmd/soroban-cli/src/commands/network/ls.rs @@ -47,7 +47,7 @@ impl Cmd { format!( "Name: {name}\nRPC url: {rpc_url}\nRPC headers:{headers}\nNetwork passphrase: {passphrase}", - rpc_url = crate::config::network::redact_rpc_url(&network.rpc_url), + rpc_url = crate::utils::url::redact_url(&network.rpc_url), passphrase = network.network_passphrase, ) }) diff --git a/cmd/soroban-cli/src/config/data.rs b/cmd/soroban-cli/src/config/data.rs index 0d227bb087..e310ebf131 100644 --- a/cmd/soroban-cli/src/config/data.rs +++ b/cmd/soroban-cli/src/config/data.rs @@ -4,7 +4,7 @@ use serde::{Deserialize, Serialize}; use std::str::FromStr; use url::Url; -use super::network::redact_rpc_url; +use crate::utils::url::redact_url; use crate::xdr::{self, WriteXdr}; #[derive(thiserror::Error, Debug)] @@ -61,7 +61,7 @@ pub fn bucket_dir() -> Result { pub fn write(action: Action, rpc_url: &Url) -> Result { let data = Data { action, - rpc_url: redact_rpc_url(rpc_url.as_str()), + rpc_url: redact_url(rpc_url.as_str()), }; let id = ulid::Ulid::new(); let file = actions_dir()?.join(id.to_string()).with_extension("json"); @@ -132,7 +132,12 @@ impl std::fmt::Display for DatedAction { .map_or_else(|| "SUCCESS".to_string(), |_| "ERROR".to_string()), Action::Send { response } => response.status.clone(), }; - write!(f, "{id} {} {status} {datetime} {uri} ", a.type_str()) + write!( + f, + "{id} {} {status} {datetime} {} ", + a.type_str(), + redact_url(uri.as_str()), + ) } } diff --git a/cmd/soroban-cli/src/config/network.rs b/cmd/soroban-cli/src/config/network.rs index 350b9ca46a..b0a0590328 100644 --- a/cmd/soroban-cli/src/config/network.rs +++ b/cmd/soroban-cli/src/config/network.rs @@ -10,7 +10,7 @@ use stellar_strkey::ed25519::PublicKey; use url::Url; use super::locator; -use crate::utils::http; +use crate::utils::{http, url::redact_url}; use crate::{ commands::HEADING_RPC, rpc::{self, Client}, @@ -165,23 +165,13 @@ impl std::fmt::Debug for Network { .map(|(k, _)| (k.as_str(), "")) .collect(); f.debug_struct("Network") - .field("rpc_url", &redact_rpc_url(&self.rpc_url)) + .field("rpc_url", &redact_url(&self.rpc_url)) .field("rpc_headers", &concealed) .field("network_passphrase", &self.network_passphrase) .finish() } } -pub fn redact_rpc_url(rpc_url: &str) -> String { - let Ok(mut url) = Url::parse(rpc_url) else { - return rpc_url.to_string(); - }; - if url.password().is_some() { - let _ = url.set_password(Some("redacted")); - } - url.to_string() -} - fn parse_http_header(header: &str) -> Result<(String, String), Error> { let header_components = header.splitn(2, ':'); @@ -221,8 +211,8 @@ impl Network { pub async fn helper_url(&self, addr: &str) -> Result { tracing::debug!("address {addr:?}"); - let rpc_url = - Url::from_str(&self.rpc_url).map_err(|_| Error::InvalidUrl(self.rpc_url.clone()))?; + let rpc_url = Url::from_str(&self.rpc_url) + .map_err(|_| Error::InvalidUrl(redact_url(&self.rpc_url)))?; if self.network_passphrase.as_str() == passphrase::LOCAL { let mut local_url = rpc_url; local_url.set_path("/friendbot"); @@ -231,12 +221,17 @@ impl Network { } else { let client = self.rpc_client()?; let network = client.get_network().await?; - tracing::debug!("network {network:?}"); + tracing::debug!( + "network passphrase={:?} protocol_version={} friendbot_url={:?}", + network.passphrase, + network.protocol_version, + network.friendbot_url.as_deref().map(redact_url), + ); let url = client.friendbot_url().await?; - tracing::debug!("URL {url:?}"); + tracing::debug!("URL {}", redact_url(&url)); let mut url = Url::from_str(&url).map_err(|e| { tracing::error!("{e}"); - Error::InvalidUrl(url.clone()) + Error::InvalidUrl(redact_url(&url)) })?; url.query_pairs_mut().append_pair("addr", addr); Ok(url) @@ -246,13 +241,13 @@ impl Network { #[allow(clippy::similar_names)] pub async fn fund_address(&self, addr: &PublicKey) -> Result<(), Error> { let uri = self.helper_url(&addr.to_string()).await?; - tracing::debug!("URL {uri:?}"); + tracing::debug!("URL {}", redact_url(uri.as_str())); let response = http::client().get(uri.as_str()).send().await?; let request_successful = response.status().is_success(); let body = response.bytes().await?; let res = serde_json::from_slice::(&body) - .map_err(|e| Error::FailedToParseJSON(uri.to_string(), e))?; + .map_err(|e| Error::FailedToParseJSON(redact_url(uri.as_str()), e))?; tracing::debug!("{res:#?}"); if !request_successful { if let Some(detail) = res.get("detail").and_then(Value::as_str) { @@ -273,7 +268,7 @@ impl Network { } pub fn rpc_uri(&self) -> Result { - Url::from_str(&self.rpc_url).map_err(|_| Error::InvalidUrl(self.rpc_url.clone())) + Url::from_str(&self.rpc_url).map_err(|_| Error::InvalidUrl(redact_url(&self.rpc_url))) } pub fn rpc_client(&self) -> Result { @@ -288,7 +283,7 @@ impl Network { rpc::Client::new_with_headers(&self.rpc_url, header_map).map_err(|e| match e { rpc::Error::InvalidRpcUrl(..) | rpc::Error::InvalidRpcUrlFromUriParts(..) => { - Error::InvalidUrl(self.rpc_url.clone()) + Error::InvalidUrl(redact_url(&self.rpc_url)) } other => Error::Rpc(other), }) @@ -693,36 +688,98 @@ mod tests { ); } - #[test] - fn redact_rpc_url_leaves_url_without_password_unchanged() { - let plain = "https://rpc.example.com/soroban"; - assert_eq!(redact_rpc_url(plain), plain); + #[tokio::test] + async fn fund_address_failed_to_parse_json_does_not_leak_credentialed_rpc_url() { + let mut server = Server::new_async().await; + // Friendbot returns a non-JSON body so serde_json::from_slice fails, + // triggering Error::FailedToParseJSON at the line we want to verify. + let _mock = server + .mock("GET", mockito::Matcher::Any) + .with_status(200) + .with_body("not valid json") + .create_async() + .await; - let user_only = "https://alice@rpc.example.com/soroban"; - assert_eq!(redact_rpc_url(user_only), user_only); - } + let host_port = server + .url() + .strip_prefix("http://") + .expect("mockito url starts with http://") + .to_string(); + let credentialed_rpc_url = format!("http://alice:supersecret@{host_port}"); - #[test] - fn redact_rpc_url_replaces_password_with_placeholder() { - let with_password = "https://alice:supersecret@rpc.example.com/soroban"; - let redacted = redact_rpc_url(with_password); - assert!( - !redacted.contains("supersecret"), - "password leaked: {redacted}" - ); + let network = Network { + rpc_url: credentialed_rpc_url, + network_passphrase: passphrase::LOCAL.to_string(), + rpc_headers: Vec::new(), + }; + + let addr = + PublicKey::from_string("GBZXN7PIRZGNMHGA7MUUUF4GWPY5AYPV6LY4UV2GL6VJGIQRXFDNMADI") + .unwrap(); + let err = network + .fund_address(&addr) + .await + .expect_err("fund_address must return Err when friendbot replies with non-JSON body"); + let rendered = err.to_string(); assert!( - redacted.contains("alice:redacted"), - "expected `alice:redacted`: {redacted}" + !rendered.contains("supersecret"), + "password leaked into error display: {rendered}" ); assert!( - redacted.contains("rpc.example.com/soroban"), - "expected host and path preserved: {redacted}" + rendered.contains("alice:redacted"), + "expected `alice:redacted` placeholder in error display: {rendered}" ); } - #[test] - fn redact_rpc_url_returns_input_when_unparseable() { - let bad = "not a url"; - assert_eq!(redact_rpc_url(bad), bad); + #[tokio::test] + async fn helper_url_returned_credentialed_url_is_redactable_at_display_sinks() { + // Non-LOCAL passphrase branch: helper_url asks the RPC for the friendbot URL. + // The mocked RPC returns a parseable URL carrying userinfo, so Url::from_str + // succeeds and helper_url returns Ok(url). The InvalidUrl branch is therefore + // not exercised here — driving it would require an unparseable URL, which by + // design leaks unchanged (see PR discussion). This test only documents that + // the parseable URL returned from helper_url can be safely run through + // redact_url at any subsequent display sink. + let mut server = Server::new_async().await; + let _mock = server + .mock("POST", "/") + .with_body_from_request(|req| { + let body: Value = serde_json::from_slice(req.body().unwrap()).unwrap(); + let id = body["id"].clone(); + // Returned friendbot URL has userinfo + is parseable by url::Url. + // Url::from_str inside helper_url accepts it, so the InvalidUrl + // path at line 239 isn't exercised. Instead the URL flows into + // the tracing line and (after fund_address) into FailedToParseJSON. + json!({ + "jsonrpc": "2.0", + "id": id, + "result": { + "friendbotUrl": "https://alice:supersecret@friendbot.example/", + "passphrase": passphrase::TESTNET.to_string(), + "protocolVersion": 21, + } + }) + .to_string() + .into() + }) + .create_async() + .await; + + let network = Network { + rpc_url: server.url(), + network_passphrase: passphrase::TESTNET.to_string(), + rpc_headers: Vec::new(), + }; + let returned = network + .helper_url("GBZXN7PIRZGNMHGA7MUUUF4GWPY5AYPV6LY4UV2GL6VJGIQRXFDNMADI") + .await + .expect("helper_url should accept a parseable credentialed friendbot URL"); + // The Url returned still carries the password — callers need it to authenticate. + assert_eq!(returned.password(), Some("supersecret")); + let redacted_for_display = redact_url(returned.as_str()); + assert!( + !redacted_for_display.contains("supersecret"), + "redact_url failed to redact a parseable friendbot URL: {redacted_for_display}" + ); } } diff --git a/cmd/soroban-cli/src/utils.rs b/cmd/soroban-cli/src/utils.rs index 99f1eae501..df1f54f455 100644 --- a/cmd/soroban-cli/src/utils.rs +++ b/cmd/soroban-cli/src/utils.rs @@ -267,6 +267,60 @@ pub mod http { } } +pub mod url { + use url::Url; + + /// Returns the given URL with any password component replaced by the literal + /// `redacted`. If the URL is not parseable, it is returned unchanged. + pub fn redact_url(url: &str) -> String { + let Ok(mut url) = Url::parse(url) else { + return url.to_string(); + }; + if url.password().is_some() { + let _ = url.set_password(Some("redacted")); + } + url.to_string() + } + + #[cfg(test)] + mod tests { + use super::*; + + #[test] + fn leaves_url_without_password_unchanged() { + let plain = "https://rpc.example.com/soroban"; + assert_eq!(redact_url(plain), plain); + + let user_only = "https://alice@rpc.example.com/soroban"; + assert_eq!(redact_url(user_only), user_only); + } + + #[test] + fn replaces_password_with_placeholder() { + let with_password = "https://alice:supersecret@rpc.example.com/soroban"; + let redacted = redact_url(with_password); + assert!( + !redacted.contains("supersecret"), + "password leaked: {redacted}" + ); + assert!( + redacted.contains("alice:redacted"), + "expected `alice:redacted`: {redacted}" + ); + assert!( + redacted.contains("rpc.example.com/soroban"), + "expected host and path preserved: {redacted}" + ); + } + + #[test] + fn returns_input_when_unparseable() { + let bad = "not a url"; + assert_eq!(redact_url(bad), bad); + } + } +} + pub mod args { #[derive(thiserror::Error, Debug)] pub enum DeprecatedError<'a> {