diff --git a/cmd/crates/soroban-test/tests/it/config.rs b/cmd/crates/soroban-test/tests/it/config.rs index c27b11b344..a7328bb5cf 100644 --- a/cmd/crates/soroban-test/tests/it/config.rs +++ b/cmd/crates/soroban-test/tests/it/config.rs @@ -1241,6 +1241,9 @@ const SEED_PHRASE: &str = // A valid strkey charset with a bad checksum: fails to parse yet clears the // identity-name check, so it reaches the config lookup. const MISTYPED_SECRET: &str = "SBF5HLRREHMS36XZNTUSKZ6FTXDZGNXOHF4EXKUL5UCWZLPBX3NGJ4BX"; +// A valid secret key, used as a legitimate `--from` so a command reaches the +// resolution of the argument actually under test (`--to`/`--spender`). +const VALID_SECRET: &str = "SBF5HLRREHMS36XZNTUSKZ6FTXDZGNXOHF4EXKUL5UCWZLPBX3NGJ4BH"; #[test] fn keys_address_resolves_seed_phrase_without_leaking_it() { @@ -1312,6 +1315,157 @@ fn token_address_param_error_does_not_leak_secret_bearing_input() { } } +#[test] +fn contract_id_param_error_does_not_leak_secret_bearing_input() { + // `--id`/`--contract-id`/`STELLAR_CONTRACT_ID` share `UnresolvedContract`; a + // mistyped secret pasted there fails to resolve before any network call, so + // the "contract not found" error must not echo it. `contract fetch --id` + // stands in for the shared resolve path. + let sandbox = TestEnv::default(); + // A seed phrase additionally fails alias-name validation (spaces) before the + // lookup, so cover both the mistyped strkey and the seed-phrase paths. + for input in [MISTYPED_SECRET, SEED_PHRASE] { + sandbox + .new_assert_cmd("contract") + .args(["fetch", "--id", input, "--network", "testnet"]) + .assert() + .failure() + .stdout(predicate::str::contains(input).not()) + .stderr(predicate::str::contains(input).not()); + } +} + +#[test] +fn contract_id_param_error_names_plain_alias() { + // A genuine alias typo is not secret-bearing, so the error still names it — + // otherwise a multi-argument command gives no clue which value was wrong. + let sandbox = TestEnv::default(); + sandbox + .new_assert_cmd("contract") + .args(["fetch", "--id", "nosuchalias", "--network", "testnet"]) + .assert() + .failure() + .stderr(predicate::str::contains("nosuchalias")); +} + +#[test] +fn address_param_error_names_plain_alias() { + // Same contract for the address params: a mistyped alias (not secret-bearing) + // is still echoed so the user can see which value failed. + let sandbox = TestEnv::default(); + sandbox + .new_assert_cmd("token") + .args([ + "balance", + "--id", + "native", + "--account", + "nosuchalias", + "--rpc-url", + "http://localhost:1", + "--network-passphrase", + "Test SDF Network ; September 2015", + ]) + .assert() + .failure() + .stderr(predicate::str::contains("nosuchalias")); +} + +#[test] +fn token_from_param_error_does_not_leak_secret_bearing_input() { + // `--from` is the signing account (`UnresolvedMuxedAccount`); a mistyped + // secret pasted there must surface the concealing "invalid signing key" + // error, resolved before any network call. + let sandbox = TestEnv::default(); + for output in ["text", "json"] { + sandbox + .new_assert_cmd("token") + .args([ + "transfer", + "--id", + "native", + "--from", + MISTYPED_SECRET, + "--to", + "GAREAZZQWHOCBJS236KIE3AWYBVFLSBK7E5UW3ICI3TCRWQKT5LNLCEZ", + "--amount", + "1", + "--output", + output, + "--rpc-url", + "http://localhost:1", + "--network-passphrase", + "Test SDF Network ; September 2015", + ]) + .assert() + .failure() + .stdout(predicate::str::contains(MISTYPED_SECRET).not()) + .stderr(predicate::str::contains(MISTYPED_SECRET).not()); + } +} + +#[test] +fn token_to_param_error_does_not_leak_secret_bearing_input() { + // A valid `--from` lets resolution reach `--to` (`UnresolvedScAddress`); a + // mistyped secret there must not be echoed, in text or JSON. + let sandbox = TestEnv::default(); + for output in ["text", "json"] { + sandbox + .new_assert_cmd("token") + .args([ + "transfer", + "--id", + "native", + "--from", + VALID_SECRET, + "--to", + MISTYPED_SECRET, + "--amount", + "1", + "--output", + output, + "--rpc-url", + "http://localhost:1", + "--network-passphrase", + "Test SDF Network ; September 2015", + ]) + .assert() + .failure() + .stdout(predicate::str::contains(MISTYPED_SECRET).not()) + .stderr(predicate::str::contains(MISTYPED_SECRET).not()); + } +} + +#[test] +fn token_spender_param_error_does_not_leak_secret_bearing_input() { + // `--spender` also shares `UnresolvedScAddress`; a valid `--from` lets + // resolution reach it, and a mistyped secret there must not be echoed. + let sandbox = TestEnv::default(); + sandbox + .new_assert_cmd("token") + .args([ + "approve", + "--id", + "native", + "--from", + VALID_SECRET, + "--spender", + MISTYPED_SECRET, + "--amount", + "1", + "--expiration-ledger", + "1000", + "--rpc-url", + "http://localhost:1", + "--network-passphrase", + "Test SDF Network ; September 2015", + ]) + .assert() + .failure() + .stdout(predicate::str::contains(MISTYPED_SECRET).not()) + .stderr(predicate::str::contains(MISTYPED_SECRET).not()); +} + #[test] fn help_conceals_sensitive_env_var_values() { // Each concealed env var, paired with a subcommand whose `--help` renders it. diff --git a/cmd/soroban-cli/src/commands/contract/arg_parsing.rs b/cmd/soroban-cli/src/commands/contract/arg_parsing.rs index 2951c7c40c..12093740e7 100644 --- a/cmd/soroban-cli/src/commands/contract/arg_parsing.rs +++ b/cmd/soroban-cli/src/commands/contract/arg_parsing.rs @@ -104,8 +104,9 @@ pub fn build_host_function_parameters( slop: &[OsString], spec_entries: &[ScSpecEntry], config: &config::Args, + print: &Print, ) -> Result { - build_host_function_parameters_with_filter(contract_id, slop, spec_entries, config, true) + build_host_function_parameters_with_filter(contract_id, slop, spec_entries, config, true, print) } /// Build invocation parameters by mapping `ordered_values` to the contract @@ -123,6 +124,7 @@ pub fn build_host_function_parameters_by_position( ordered_values: &[String], spec_entries: &[ScSpecEntry], config: &config::Args, + print: &Print, ) -> Result { let spec = Spec(Some(spec_entries.to_vec())); let func = get_function_spec(&spec, function_name)?; @@ -142,7 +144,15 @@ pub fn build_host_function_parameters_by_position( let mut parsed_args = Vec::with_capacity(func.inputs.len()); let mut signers = Vec::::new(); for (input, value) in func.inputs.iter().zip(ordered_values.iter()) { - parse_positional_value(input, value, &spec, config, &mut signers, &mut parsed_args)?; + parse_positional_value( + input, + value, + &spec, + config, + &mut signers, + &mut parsed_args, + print, + )?; } let invoke_args = build_invoke_contract_args(contract_id, function_name, parsed_args)?; @@ -154,8 +164,16 @@ pub fn build_constructor_parameters( slop: &[OsString], spec_entries: &[ScSpecEntry], config: &config::Args, + print: &Print, ) -> Result { - build_host_function_parameters_with_filter(contract_id, slop, spec_entries, config, false) + build_host_function_parameters_with_filter( + contract_id, + slop, + spec_entries, + config, + false, + print, + ) } fn build_host_function_parameters_with_filter( @@ -164,12 +182,13 @@ fn build_host_function_parameters_with_filter( spec_entries: &[ScSpecEntry], config: &config::Args, filter_constructor: bool, + print: &Print, ) -> Result { let spec = Spec(Some(spec_entries.to_vec())); let cmd = build_clap_command(&spec, filter_constructor)?; let (function, matches_) = parse_command_matches(cmd, slop)?; let func = get_function_spec(&spec, &function)?; - let (parsed_args, signers) = parse_function_arguments(&func, &matches_, &spec, config)?; + let (parsed_args, signers) = parse_function_arguments(&func, &matches_, &spec, config, print)?; let invoke_args = build_invoke_contract_args(contract_id, &function, parsed_args)?; Ok((function, spec, invoke_args, signers)) @@ -239,12 +258,21 @@ fn parse_function_arguments( matches_: &clap::ArgMatches, spec: &Spec, config: &config::Args, + print: &Print, ) -> Result<(Vec, Vec), Error> { let mut parsed_args = Vec::with_capacity(func.inputs.len()); let mut signers = Vec::::new(); for i in &func.inputs { - parse_single_argument(i, matches_, spec, config, &mut signers, &mut parsed_args)?; + parse_single_argument( + i, + matches_, + spec, + config, + &mut signers, + &mut parsed_args, + print, + )?; } Ok((parsed_args, signers)) @@ -257,6 +285,7 @@ fn parse_single_argument( config: &config::Args, signers: &mut Vec, parsed_args: &mut Vec, + print: &Print, ) -> Result<(), Error> { let name = sanitize(&input.name.to_utf8_string_lossy()); let expected_type_name = get_type_name(&input.type_); //-0-- @@ -272,7 +301,7 @@ fn parse_single_argument( } }; - parse_positional_value(input, &s, spec, config, signers, parsed_args) + parse_positional_value(input, &s, spec, config, signers, parsed_args, print) } else if matches!(input.type_, ScSpecTypeDef::Option(_)) { parsed_args.push(ScVal::Void); Ok(()) @@ -284,6 +313,7 @@ fn parse_single_argument( expected_type_name, spec, config, + print, )?); Ok(()) } else { @@ -306,6 +336,7 @@ fn parse_positional_value( config: &config::Args, signers: &mut Vec, parsed_args: &mut Vec, + print: &Print, ) -> Result<(), Error> { let name = sanitize(&input.name.to_utf8_string_lossy()); @@ -329,6 +360,7 @@ fn parse_positional_value( &input.type_, spec, config, + print, )?); Ok(()) } @@ -340,6 +372,7 @@ fn parse_file_argument( expected_type_name: String, spec: &Spec, config: &config::Args, + print: &Print, ) -> Result { if matches!(type_def, ScSpecTypeDef::Bytes | ScSpecTypeDef::BytesN(_)) { let bytes = std::fs::read(arg_path).map_err(|e| Error::MissingFileArg { @@ -365,7 +398,7 @@ fn parse_file_argument( type_def, file_contents.len() ); - parse_argument_with_validation(name, &file_contents, type_def, spec, config) + parse_argument_with_validation(name, &file_contents, type_def, spec, config, print) } } @@ -503,7 +536,11 @@ pub fn output_to_string( Ok(TxnResult::Res(res_str)) } -fn resolve_address(addr_or_alias: &str, config: &config::Args) -> Result { +fn resolve_address( + addr_or_alias: &str, + config: &config::Args, + print: &Print, +) -> Result { let sc_address: UnresolvedScAddress = addr_or_alias.parse().unwrap(); let account = match sc_address { UnresolvedScAddress::Resolved(addr) => addr.to_string(), @@ -512,6 +549,7 @@ fn resolve_address(addr_or_alias: &str, config: &config::Args) -> Result account.to_string(), @@ -686,6 +724,7 @@ fn parse_argument_with_validation( expected_type: &ScSpecTypeDef, spec: &Spec, config: &config::Args, + print: &Print, ) -> Result { let expected_type_name = get_type_name(expected_type); @@ -704,7 +743,7 @@ fn parse_argument_with_validation( // Walk the input through resolve_aliases_in_json so identity aliases are // resolved at every Address/MuxedAddress position, top-level or nested. - let resolved = resolve_aliases(value, expected_type, spec, config)?; + let resolved = resolve_aliases(value, expected_type, spec, config, print)?; spec.from_string(&resolved, expected_type) .map_err(|error| Error::CannotParseArg { @@ -725,6 +764,7 @@ fn resolve_aliases( type_def: &ScSpecTypeDef, spec: &Spec, config: &config::Args, + print: &Print, ) -> Result { let is_address = matches!( type_def, @@ -737,7 +777,7 @@ fn resolve_aliases( Err(_) => return Ok(value.to_string()), }; - let mutated = resolve_aliases_in_json(&mut json, type_def, spec, config)?; + let mutated = resolve_aliases_in_json(&mut json, type_def, spec, config, print)?; // Nothing was rewritten — return the original input verbatim so we don't // disturb whitespace, key ordering, or number formatting just to reparse it. @@ -767,12 +807,13 @@ fn resolve_aliases_in_json( type_def: &ScSpecTypeDef, spec: &Spec, config: &config::Args, + print: &Print, ) -> Result { let mut mutated = false; match type_def { ScSpecTypeDef::Address | ScSpecTypeDef::MuxedAddress => { if let serde_json::Value::String(s) = value { - let resolved = resolve_address(s, config)?; + let resolved = resolve_address(s, config, print)?; if &resolved != s { *s = resolved; mutated = true; @@ -782,14 +823,15 @@ fn resolve_aliases_in_json( ScSpecTypeDef::Vec(inner) => { if let serde_json::Value::Array(arr) = value { for item in arr.iter_mut() { - mutated |= resolve_aliases_in_json(item, &inner.element_type, spec, config)?; + mutated |= + resolve_aliases_in_json(item, &inner.element_type, spec, config, print)?; } } } ScSpecTypeDef::Tuple(tuple) => { if let serde_json::Value::Array(arr) = value { for (item, ty) in arr.iter_mut().zip(tuple.value_types.iter()) { - mutated |= resolve_aliases_in_json(item, ty, spec, config)?; + mutated |= resolve_aliases_in_json(item, ty, spec, config, print)?; } } } @@ -802,8 +844,9 @@ fn resolve_aliases_in_json( if key_is_address { let entries = std::mem::take(obj); for (k, mut v) in entries { - mutated |= resolve_aliases_in_json(&mut v, &map.value_type, spec, config)?; - let resolved = resolve_address(&k, config)?; + mutated |= + resolve_aliases_in_json(&mut v, &map.value_type, spec, config, print)?; + let resolved = resolve_address(&k, config, print)?; if resolved != k { mutated = true; } @@ -814,13 +857,14 @@ fn resolve_aliases_in_json( } } else { for v in obj.values_mut() { - mutated |= resolve_aliases_in_json(v, &map.value_type, spec, config)?; + mutated |= + resolve_aliases_in_json(v, &map.value_type, spec, config, print)?; } } } } ScSpecTypeDef::Option(inner) if !matches!(value, serde_json::Value::Null) => { - mutated |= resolve_aliases_in_json(value, &inner.value_type, spec, config)?; + mutated |= resolve_aliases_in_json(value, &inner.value_type, spec, config, print)?; } ScSpecTypeDef::Result(result) => { // Result is rarely used as an input type. The walker descends into @@ -828,11 +872,11 @@ fn resolve_aliases_in_json( // shape doesn't fit the branch's type. Resolution is idempotent // (a strkey re-resolves to itself), so descending twice is safe // when both branches happen to share a shape. - mutated |= resolve_aliases_in_json(value, &result.ok_type, spec, config)?; - mutated |= resolve_aliases_in_json(value, &result.error_type, spec, config)?; + mutated |= resolve_aliases_in_json(value, &result.ok_type, spec, config, print)?; + mutated |= resolve_aliases_in_json(value, &result.error_type, spec, config, print)?; } ScSpecTypeDef::Udt(udt) => { - mutated |= resolve_aliases_in_udt(value, udt, spec, config)?; + mutated |= resolve_aliases_in_udt(value, udt, spec, config, print)?; } _ => {} } @@ -844,6 +888,7 @@ fn resolve_aliases_in_udt( udt: &stellar_xdr::ScSpecTypeUdt, spec: &Spec, config: &config::Args, + print: &Print, ) -> Result { let mut mutated = false; let name = udt.name.to_utf8_string_lossy(); @@ -862,15 +907,21 @@ fn resolve_aliases_in_udt( match value { serde_json::Value::Array(arr) if is_tuple_struct => { for (item, field) in arr.iter_mut().zip(strukt.fields.iter()) { - mutated |= resolve_aliases_in_json(item, &field.type_, spec, config)?; + mutated |= + resolve_aliases_in_json(item, &field.type_, spec, config, print)?; } } serde_json::Value::Object(obj) => { for field in &strukt.fields { let key = field.name.to_utf8_string_lossy(); if let Some(field_val) = obj.get_mut(key.as_str()) { - mutated |= - resolve_aliases_in_json(field_val, &field.type_, spec, config)?; + mutated |= resolve_aliases_in_json( + field_val, + &field.type_, + spec, + config, + print, + )?; } } } @@ -878,7 +929,7 @@ fn resolve_aliases_in_udt( } } ScSpecEntry::UdtUnionV0(union) => { - mutated |= resolve_aliases_in_union(value, union, spec, config)?; + mutated |= resolve_aliases_in_union(value, union, spec, config, print)?; } _ => {} } @@ -890,6 +941,7 @@ fn resolve_aliases_in_union( union: &stellar_xdr::ScSpecUdtUnionV0, spec: &Spec, config: &config::Args, + print: &Print, ) -> Result { use stellar_xdr::ScSpecUdtUnionCaseV0; @@ -910,12 +962,12 @@ fn resolve_aliases_in_union( // matching the form `soroban_spec_tools` accepts. Variants with two or more // elements take an array payload — `{"Variant": [a, b, ...]}`. if tuple.type_.len() == 1 { - return resolve_aliases_in_json(payload, &tuple.type_[0], spec, config); + return resolve_aliases_in_json(payload, &tuple.type_[0], spec, config, print); } let mut mutated = false; if let serde_json::Value::Array(arr) = payload { for (item, ty) in arr.iter_mut().zip(tuple.type_.iter()) { - mutated |= resolve_aliases_in_json(item, ty, spec, config)?; + mutated |= resolve_aliases_in_json(item, ty, spec, config, print)?; } } Ok(mutated) @@ -1157,24 +1209,49 @@ mod tests { let config = crate::config::Args::default(); // Bare string (no JSON quoting) should be accepted - let result = - parse_argument_with_validation("value", "Unit", &expected_type, &spec, &config); + let result = parse_argument_with_validation( + "value", + "Unit", + &expected_type, + &spec, + &config, + &Print::new(false), + ); assert!(result.is_ok(), "bare 'Unit' should be accepted: {result:?}"); // JSON-quoted string should also be accepted - let result = - parse_argument_with_validation("value", "\"Unit\"", &expected_type, &spec, &config); + let result = parse_argument_with_validation( + "value", + "\"Unit\"", + &expected_type, + &spec, + &config, + &Print::new(false), + ); assert!( result.is_ok(), "JSON-quoted '\"Unit\"' should be accepted: {result:?}" ); // Both forms should produce the same ScVal - let bare = parse_argument_with_validation("value", "Unit", &expected_type, &spec, &config) - .unwrap(); - let quoted = - parse_argument_with_validation("value", "\"Unit\"", &expected_type, &spec, &config) - .unwrap(); + let bare = parse_argument_with_validation( + "value", + "Unit", + &expected_type, + &spec, + &config, + &Print::new(false), + ) + .unwrap(); + let quoted = parse_argument_with_validation( + "value", + "\"Unit\"", + &expected_type, + &spec, + &config, + &Print::new(false), + ) + .unwrap(); assert_eq!( bare, quoted, "bare and quoted forms should produce identical ScVal" @@ -1218,6 +1295,7 @@ mod tests { &expected_type, &spec, &config, + &Print::new(false), ); assert!( result.is_ok(), @@ -1257,8 +1335,15 @@ mod tests { // Empty object (no case selected) and a wrong-arity tuple payload both // used to panic in the parser; both must now come back as CannotParseArg. for bad in [r"{}", r#"{"WithValue":{"0":1}}"#] { - let err = parse_argument_with_validation("value", bad, &expected_type, &spec, &config) - .unwrap_err(); + let err = parse_argument_with_validation( + "value", + bad, + &expected_type, + &spec, + &config, + &Print::new(false), + ) + .unwrap_err(); assert!( matches!(err, Error::CannotParseArg { .. }), "input {bad}: got {err:?}" @@ -1325,7 +1410,8 @@ mod tests { let config = crate::config::Args::default(); let mut value = serde_json::json!("native"); - let mutated = resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap(); + let mutated = + resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)).unwrap(); assert!( mutated, "native should resolve to the native asset contract" @@ -1353,12 +1439,13 @@ mod tests { let config = crate::config::Args::default(); let mut value = serde_json::json!([TEST_G_ADDRESS]); - resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::json!([TEST_G_ADDRESS])); // An unknown alias-shaped string at a nested Address position must surface as an error. let mut value = serde_json::json!(["definitely-not-a-known-alias"]); - let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap_err(); + let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)) + .unwrap_err(); assert!( matches!(err, Error::Config(_) | Error::ScAddress(_)), "expected alias-resolution error, got {err:?}" @@ -1378,11 +1465,12 @@ mod tests { let config = crate::config::Args::default(); let mut value = serde_json::json!([TEST_G_ADDRESS, 42]); - resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::json!([TEST_G_ADDRESS, 42])); let mut value = serde_json::json!(["bogus-alias", 42]); - let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap_err(); + let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)) + .unwrap_err(); assert!( matches!(err, Error::Config(_) | Error::ScAddress(_)), "expected alias-resolution error, got {err:?}" @@ -1408,7 +1496,7 @@ mod tests { let config = crate::config::Args::default(); let mut value = serde_json::json!({"count": 1, "addresses": [TEST_G_ADDRESS]}); - resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!( value, serde_json::json!({"count": 1, "addresses": [TEST_G_ADDRESS]}) @@ -1416,7 +1504,8 @@ mod tests { // Walker must reach the Address inside Vec inside the struct field. let mut value = serde_json::json!({"count": 1, "addresses": ["bogus-alias"]}); - let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap_err(); + let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)) + .unwrap_err(); assert!( matches!(err, Error::Config(_) | Error::ScAddress(_)), "expected alias-resolution error, got {err:?}" @@ -1450,11 +1539,12 @@ mod tests { let config = crate::config::Args::default(); let mut value = serde_json::json!({"Pick": [TEST_G_ADDRESS, 42]}); - resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::json!({"Pick": [TEST_G_ADDRESS, 42]})); let mut value = serde_json::json!({"Pick": ["bogus-alias", 42]}); - let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap_err(); + let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)) + .unwrap_err(); assert!( matches!(err, Error::Config(_) | Error::ScAddress(_)), "expected alias-resolution error, got {err:?}" @@ -1487,11 +1577,12 @@ mod tests { // Bare payload form: {"Only": addr} — not {"Only": [addr]}. let mut value = serde_json::json!({"Only": TEST_G_ADDRESS}); - resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::json!({"Only": TEST_G_ADDRESS})); let mut value = serde_json::json!({"Only": "bogus-alias"}); - let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap_err(); + let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)) + .unwrap_err(); assert!( matches!(err, Error::Config(_) | Error::ScAddress(_)), "expected alias-resolution error, got {err:?}" @@ -1509,11 +1600,11 @@ mod tests { let config = crate::config::Args::default(); let mut value = serde_json::Value::Null; - resolve_aliases_in_json(&mut value, &opt_ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &opt_ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::Value::Null); let mut value = serde_json::json!(TEST_G_ADDRESS); - resolve_aliases_in_json(&mut value, &opt_ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &opt_ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::json!(TEST_G_ADDRESS)); let map_ty = ScSpecTypeDef::Map(Box::new(ScSpecTypeMap { @@ -1521,11 +1612,12 @@ mod tests { value_type: Box::new(ScSpecTypeDef::Address), })); let mut value = serde_json::json!({"owner": TEST_G_ADDRESS}); - resolve_aliases_in_json(&mut value, &map_ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &map_ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::json!({"owner": TEST_G_ADDRESS})); let mut value = serde_json::json!({"owner": "bogus-alias"}); - let err = resolve_aliases_in_json(&mut value, &map_ty, &spec, &config).unwrap_err(); + let err = resolve_aliases_in_json(&mut value, &map_ty, &spec, &config, &Print::new(false)) + .unwrap_err(); assert!( matches!(err, Error::Config(_) | Error::ScAddress(_)), "expected alias-resolution error, got {err:?}" @@ -1544,11 +1636,12 @@ mod tests { let config = crate::config::Args::default(); let mut value = serde_json::json!(TEST_G_ADDRESS); - resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::json!(TEST_G_ADDRESS)); let mut value = serde_json::json!("bogus-alias"); - let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config).unwrap_err(); + let err = resolve_aliases_in_json(&mut value, &ty, &spec, &config, &Print::new(false)) + .unwrap_err(); assert!( matches!(err, Error::Config(_) | Error::ScAddress(_)), "expected alias-resolution error, got {err:?}" @@ -1568,7 +1661,7 @@ mod tests { let config = crate::config::Args::default(); let pretty = r#"{ "x": 1, "y": 2 }"#; assert_eq!( - resolve_aliases(pretty, &ty, &spec, &config).unwrap(), + resolve_aliases(pretty, &ty, &spec, &config, &Print::new(false)).unwrap(), pretty ); @@ -1579,7 +1672,7 @@ mod tests { let spec = Spec(Some(vec![])); let pretty = format!(r#"[ "{TEST_G_ADDRESS}" ]"#); assert_eq!( - resolve_aliases(&pretty, &ty, &spec, &config).unwrap(), + resolve_aliases(&pretty, &ty, &spec, &config, &Print::new(false)).unwrap(), pretty ); } @@ -1596,11 +1689,12 @@ mod tests { let config = crate::config::Args::default(); let mut value = serde_json::json!({ TEST_G_ADDRESS: 1 }); - resolve_aliases_in_json(&mut value, &map_ty, &spec, &config).unwrap(); + resolve_aliases_in_json(&mut value, &map_ty, &spec, &config, &Print::new(false)).unwrap(); assert_eq!(value, serde_json::json!({ TEST_G_ADDRESS: 1 })); let mut value = serde_json::json!({ "bogus-alias": 1 }); - let err = resolve_aliases_in_json(&mut value, &map_ty, &spec, &config).unwrap_err(); + let err = resolve_aliases_in_json(&mut value, &map_ty, &spec, &config, &Print::new(false)) + .unwrap_err(); assert!( matches!(err, Error::Config(_) | Error::ScAddress(_)), "expected alias-resolution error, got {err:?}" @@ -1656,6 +1750,7 @@ mod tests { &values, &spec_entries, &config, + &Print::new(false), ) .unwrap(); @@ -1704,6 +1799,7 @@ mod tests { &values, &spec_entries, &config, + &Print::new(false), ) .unwrap(); @@ -1736,6 +1832,7 @@ mod tests { &values, &spec_entries, &config, + &Print::new(false), ); match result { diff --git a/cmd/soroban-cli/src/commands/contract/deploy/wasm.rs b/cmd/soroban-cli/src/commands/contract/deploy/wasm.rs index 596fe68a6a..4655a2c123 100644 --- a/cmd/soroban-cli/src/commands/contract/deploy/wasm.rs +++ b/cmd/soroban-cli/src/commands/contract/deploy/wasm.rs @@ -415,6 +415,7 @@ impl Cmd { &slop, &entries, config, + &print, )?; (Some(invoke_args), signers) } diff --git a/cmd/soroban-cli/src/commands/contract/invoke.rs b/cmd/soroban-cli/src/commands/contract/invoke.rs index 90058a5501..85763ef277 100644 --- a/cmd/soroban-cli/src/commands/contract/invoke.rs +++ b/cmd/soroban-cli/src/commands/contract/invoke.rs @@ -333,9 +333,16 @@ impl Cmd { &inv.args, spec_entries, config, + &print, )?; } else { - build_host_function_parameters(&contract_id, &self.slop, spec_entries, config)?; + build_host_function_parameters( + &contract_id, + &self.slop, + spec_entries, + config, + &print, + )?; } } @@ -367,9 +374,10 @@ impl Cmd { &inv.args, &spec_entries, config, + &print, )? } else { - build_host_function_parameters(&contract_id, &self.slop, &spec_entries, config)? + build_host_function_parameters(&contract_id, &self.slop, &spec_entries, config, &print)? }; let (function, spec, host_function_params, signers) = params; diff --git a/cmd/soroban-cli/src/commands/token/allowance.rs b/cmd/soroban-cli/src/commands/token/allowance.rs index aa454c2d0e..b038064c37 100644 --- a/cmd/soroban-cli/src/commands/token/allowance.rs +++ b/cmd/soroban-cli/src/commands/token/allowance.rs @@ -122,12 +122,22 @@ impl Cmd { let from = self .from .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let spender = self .spender .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); // SEP-41 `allowance(from, spender) -> i128`. diff --git a/cmd/soroban-cli/src/commands/token/approve.rs b/cmd/soroban-cli/src/commands/token/approve.rs index 9d1c997186..94eaf536f1 100644 --- a/cmd/soroban-cli/src/commands/token/approve.rs +++ b/cmd/soroban-cli/src/commands/token/approve.rs @@ -176,7 +176,12 @@ impl Cmd { let spender = self .spender .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let amount = self.amount.to_string(); let expiration_ledger = self.expiration_ledger.to_string(); diff --git a/cmd/soroban-cli/src/commands/token/balance.rs b/cmd/soroban-cli/src/commands/token/balance.rs index 050bb91bc9..205cd78793 100644 --- a/cmd/soroban-cli/src/commands/token/balance.rs +++ b/cmd/soroban-cli/src/commands/token/balance.rs @@ -118,7 +118,12 @@ impl Cmd { let account = self .account .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let raw: i128 = self diff --git a/cmd/soroban-cli/src/commands/token/burn_from.rs b/cmd/soroban-cli/src/commands/token/burn_from.rs index d186faf7ee..a827ecba63 100644 --- a/cmd/soroban-cli/src/commands/token/burn_from.rs +++ b/cmd/soroban-cli/src/commands/token/burn_from.rs @@ -172,7 +172,12 @@ impl Cmd { let from = self .from .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let amount = self.amount.to_string(); diff --git a/cmd/soroban-cli/src/commands/token/clawback.rs b/cmd/soroban-cli/src/commands/token/clawback.rs index e6ae33718b..a4d0127192 100644 --- a/cmd/soroban-cli/src/commands/token/clawback.rs +++ b/cmd/soroban-cli/src/commands/token/clawback.rs @@ -130,7 +130,12 @@ impl Cmd { let from = self .from .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let amount = self.amount.to_string(); diff --git a/cmd/soroban-cli/src/commands/token/mint.rs b/cmd/soroban-cli/src/commands/token/mint.rs index 66dd935174..b0c0af1019 100644 --- a/cmd/soroban-cli/src/commands/token/mint.rs +++ b/cmd/soroban-cli/src/commands/token/mint.rs @@ -142,7 +142,12 @@ impl Cmd { let to = self .to .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let amount = self.amount.to_string(); diff --git a/cmd/soroban-cli/src/commands/token/set_admin.rs b/cmd/soroban-cli/src/commands/token/set_admin.rs index f31820cf4f..02bd151553 100644 --- a/cmd/soroban-cli/src/commands/token/set_admin.rs +++ b/cmd/soroban-cli/src/commands/token/set_admin.rs @@ -125,7 +125,12 @@ impl Cmd { let new_admin = self .new_admin .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); // SAC `set_admin(new_admin)` — supply the value and let the contract's diff --git a/cmd/soroban-cli/src/commands/token/set_authorized.rs b/cmd/soroban-cli/src/commands/token/set_authorized.rs index 1e06b5043c..b806545c2b 100644 --- a/cmd/soroban-cli/src/commands/token/set_authorized.rs +++ b/cmd/soroban-cli/src/commands/token/set_authorized.rs @@ -135,7 +135,12 @@ impl Cmd { let account = self .account .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let authorize = self.authorize.to_string(); diff --git a/cmd/soroban-cli/src/commands/token/transfer.rs b/cmd/soroban-cli/src/commands/token/transfer.rs index eef85e5324..5cbf587e83 100644 --- a/cmd/soroban-cli/src/commands/token/transfer.rs +++ b/cmd/soroban-cli/src/commands/token/transfer.rs @@ -161,7 +161,12 @@ impl Cmd { let to = self .to .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let amount = self.amount.to_string(); diff --git a/cmd/soroban-cli/src/commands/token/transfer_from.rs b/cmd/soroban-cli/src/commands/token/transfer_from.rs index 70bff4b5ad..bfd1e7e37d 100644 --- a/cmd/soroban-cli/src/commands/token/transfer_from.rs +++ b/cmd/soroban-cli/src/commands/token/transfer_from.rs @@ -180,12 +180,22 @@ impl Cmd { let from = self .from .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let to = self .to .clone() - .resolve(&config.locator, &network.network_passphrase, None)? + .resolve( + &config.locator, + &network.network_passphrase, + None, + &crate::print::Print::new(quiet), + )? .to_string(); let amount = self.amount.to_string(); diff --git a/cmd/soroban-cli/src/config/alias.rs b/cmd/soroban-cli/src/config/alias.rs index 5839e9fa24..f958c8ca0b 100644 --- a/cmd/soroban-cli/src/config/alias.rs +++ b/cmd/soroban-cli/src/config/alias.rs @@ -3,7 +3,7 @@ use std::{collections::HashMap, convert::Infallible, str::FromStr}; use serde::{Deserialize, Serialize}; use stellar_strkey::Contract; -use super::locator; +use super::{locator, secret}; use crate::config::token::UnresolvedToken; use crate::tx::builder; @@ -60,12 +60,29 @@ pub fn validate_reserved_aliases(alias: &str) -> Result<(), locator::Error> { } /// Address can be either a contract address, C.. or eventually an alias of a contract address. -#[derive(Clone, Debug)] +#[derive(Clone)] pub enum UnresolvedContract { Resolved(stellar_strkey::Contract), Alias(String), } +impl std::fmt::Debug for UnresolvedContract { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + UnresolvedContract::Resolved(contract) => { + f.debug_tuple("Resolved").field(contract).finish() + } + // A genuine alias is safe to show, but never echo a secret key or + // seed phrase pasted where a contract id was expected — e.g. under + // `--verbose`, which debug-prints parsed arguments. + UnresolvedContract::Alias(alias) if secret::looks_like_secret(alias) => { + f.debug_tuple("Alias").field(&"").finish() + } + UnresolvedContract::Alias(alias) => f.debug_tuple("Alias").field(alias).finish(), + } + } +} + impl Default for UnresolvedContract { fn default() -> Self { UnresolvedContract::Alias(String::default()) @@ -102,9 +119,25 @@ impl UnresolvedContract { locator: &locator::Args, network_passphrase: &str, ) -> Result { - locator - .get_contract_id(alias, network_passphrase)? - .ok_or_else(|| locator::Error::ContractNotFound(alias.to_owned())) + let result = locator.get_contract_id(alias, network_passphrase); + + // A secret key or seed phrase pasted where a contract id was expected is + // a misplaced secret, not a real alias: collapse every outcome — a miss, + // name validation (a seed phrase's spaces fail it), or a parse/I/O error + // keyed on the pasted value — to the payload-less concealed error. None of + // these is actionable for a pasted secret, and propagating one would echo + // it, since the locator error messages interpolate the raw alias (e.g. + // `CannotParseContractId`, `FileRead`). The concealed variant stores + // nothing, so the secret can't leak through a derived `Debug` either. + // Non-secret input keeps full error propagation below. + if secret::looks_like_secret(alias) { + return result + .ok() + .flatten() + .ok_or(locator::Error::ContractNotFoundConcealed); + } + + result?.ok_or_else(|| locator::Error::ContractNotFound(alias.to_owned())) } } @@ -123,4 +156,49 @@ mod tests { assert!(!is_reserved("my-token")); assert!(validate_reserved_aliases("my-token").is_ok()); } + + #[test] + fn debug_conceals_secret_bearing_alias() { + // `--verbose` debug-prints parsed arguments; a secret pasted where a + // contract id was expected must not surface through the `Alias` variant. + for input in [ + "SBF5HLRREHMS36XZNTUSKZ6FTXDZGNXOHF4EXKUL5UCWZLPBX3NGJ4BX", + "illness spike retreat truth genius clock brain pass fit cave bargain xyzzy", + ] { + let contract = UnresolvedContract::from_str(input).unwrap(); + assert!(!format!("{contract:?}").contains(input)); + } + } + + #[test] + fn debug_echoes_plain_alias() { + let contract = UnresolvedContract::from_str("my-token").unwrap(); + assert!(format!("{contract:?}").contains("my-token")); + } + + #[test] + fn resolve_conceals_secret_with_malformed_stored_id() { + // A secret-shaped alias with an existing but malformed stored contract-id + // file makes `get_contract_id` return `CannotParseContractId(alias, ..)`, + // whose message interpolates the raw alias. Resolution must collapse that + // to the concealing not-found error rather than propagate (and echo) it. + let secret = "SBF5HLRREHMS36XZNTUSKZ6FTXDZGNXOHF4EXKUL5UCWZLPBX3NGJ4BX"; + let dir = tempfile::tempdir().unwrap(); + let locator = locator::Args { + config_dir: Some(dir.path().to_path_buf()), + }; + let network_passphrase = "Test Network"; + + let contract_ids = dir.path().join("contract-ids"); + std::fs::create_dir_all(&contract_ids).unwrap(); + std::fs::write( + contract_ids.join(format!("{secret}.json")), + format!(r#"{{"ids":{{"{network_passphrase}":"not-a-valid-contract-id"}}}}"#), + ) + .unwrap(); + + let err = + UnresolvedContract::resolve_alias(secret, &locator, network_passphrase).unwrap_err(); + assert!(!err.to_string().contains(secret), "leaked secret: {err}"); + } } diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index 8216201b0c..481df7917b 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -80,8 +80,15 @@ pub enum Error { CannotAccessAliasConfigFile, #[error("cannot parse contract ID {0}: {1}")] CannotParseContractId(String, DecodeError), - #[error("contract not found: {0}{hint}", hint = wasm_hash_hint(.0))] + #[error("contract not found: {alias}{hint}", alias = soroban_spec_tools::sanitize(.0), hint = wasm_hash_hint(.0))] ContractNotFound(String), + // The secret-shaped counterpart of `ContractNotFound`: carries no payload so + // a secret key or seed phrase pasted where a contract id was expected is + // never stored (and so can't leak through a derived `Debug` or telemetry). + // `resolve_alias` routes secret-shaped input here; `ContractNotFound` above + // therefore only ever holds a genuine, safe-to-name alias. + #[error("contract not found")] + ContractNotFoundConcealed, #[error("Failed to read upgrade check file: {path}: {error}")] UpgradeCheckReadFailed { path: PathBuf, error: io::Error }, #[error("Failed to write upgrade check file: {path}: {error}")] @@ -1084,6 +1091,27 @@ mod error_message_tests { let err = Error::ContractNotFound(value.clone()); assert_eq!(err.to_string(), format!("contract not found: {value}")); } + + #[test] + fn contract_not_found_concealed_carries_no_input() { + // The secret-shaped counterpart carries no payload, so nothing to echo in + // either the message or a derived `Debug`. `resolve_alias` routes a + // mistyped secret here; see its `resolve_conceals_*` tests. + let err = Error::ContractNotFoundConcealed; + assert_eq!(err.to_string(), "contract not found"); + } + + #[test] + fn contract_not_found_escapes_control_characters() { + // The named alias reaches the terminal, so control characters in a + // mistyped id must be escaped rather than interpreted. + let err = Error::ContractNotFound("alias\u{1b}[31m\u{7}".to_string()); + let message = err.to_string(); + assert!( + !message.contains('\u{1b}') && !message.contains('\u{7}'), + "{message}" + ); + } } #[cfg(all(test, unix))] diff --git a/cmd/soroban-cli/src/config/sc_address.rs b/cmd/soroban-cli/src/config/sc_address.rs index 7174d8502e..dbf69c400d 100644 --- a/cmd/soroban-cli/src/config/sc_address.rs +++ b/cmd/soroban-cli/src/config/sc_address.rs @@ -1,8 +1,11 @@ use std::str::FromStr; +use soroban_spec_tools::sanitize; + +use crate::print::Print; use crate::xdr; -use super::{alias, key, locator, UnresolvedContract}; +use super::{alias, key, locator, secret, UnresolvedContract}; /// `ScAddress` can be either a resolved `xdr::ScAddress` or an alias of a `Contract` or `MuxedAccount`. #[allow(clippy::module_name_repetitions)] @@ -16,11 +19,12 @@ impl std::fmt::Debug for UnresolvedScAddress { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { UnresolvedScAddress::Resolved(addr) => f.debug_tuple("Resolved").field(addr).finish(), - // Never echo the raw input: it may be a secret key or seed phrase - // pasted where an alias was expected. - UnresolvedScAddress::Alias(_) => { + // A genuine alias is safe to show, but never echo a secret key or + // seed phrase pasted where an address was expected. + UnresolvedScAddress::Alias(alias) if secret::looks_like_secret(alias) => { f.debug_tuple("Alias").field(&"").finish() } + UnresolvedScAddress::Alias(alias) => f.debug_tuple("Alias").field(alias).finish(), } } } @@ -31,20 +35,45 @@ impl Default for UnresolvedScAddress { } } -#[derive(thiserror::Error, Debug)] +#[derive(thiserror::Error)] pub enum Error { #[error(transparent)] Locator(#[from] locator::Error), #[error(transparent)] Key(#[from] key::Error), - // The input is never echoed: a secret key or seed phrase mistyped where an - // address was expected must not reach the terminal, logs, or JSON output. - #[error("invalid address or alias")] - AccountAliasNotFound, + // A genuine alias typo is named so the user can see which value was wrong, + // but a secret key or seed phrase mistyped where an address was expected is + // concealed so it never reaches the terminal, logs, or JSON output. The + // hand-written `Debug` below redacts the same value, so a debug formatter or + // logger can't leak it either. + #[error("{}", address_not_found_message(.0))] + AccountAliasNotFound(String), #[error("alias '{0}' is reserved for the native asset contract but also matches a stored key; pass an explicit contract (C...) or account (G...) address instead")] ReservedAliasShadowsKey(String), } +impl std::fmt::Debug for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Error::Locator(e) => f.debug_tuple("Locator").field(e).finish(), + Error::Key(e) => f.debug_tuple("Key").field(e).finish(), + // Never echo a secret pasted where an address was expected, even + // through a debug formatter or logger. + Error::AccountAliasNotFound(value) if secret::looks_like_secret(value) => f + .debug_tuple("AccountAliasNotFound") + .field(&"") + .finish(), + Error::AccountAliasNotFound(value) => { + f.debug_tuple("AccountAliasNotFound").field(value).finish() + } + Error::ReservedAliasShadowsKey(value) => f + .debug_tuple("ReservedAliasShadowsKey") + .field(value) + .finish(), + } + } +} + impl FromStr for UnresolvedScAddress { type Err = Error; @@ -89,6 +118,7 @@ impl UnresolvedScAddress { locator: &locator::Args, network_passphrase: &str, hd_path: Option, + print: &Print, ) -> Result { let alias = match self { UnresolvedScAddress::Resolved(addr) => return Ok(addr), @@ -104,9 +134,7 @@ impl UnresolvedScAddress { if alias::is_reserved(&alias) { return Err(Error::ReservedAliasShadowsKey(alias)); } - eprintln!( - "Warning: ScAddress alias {alias} is ambiguous, assuming it is a contract" - ); + print.warnln(ambiguous_alias_warning(&alias)); Ok(xdr::ScAddress::Contract(stellar_xdr::ContractId( xdr::Hash(contract.0), ))) @@ -131,11 +159,36 @@ impl UnresolvedScAddress { xdr::ScAddress::MuxedAccount(xdr::MuxedEd25519Account { id, ed25519 }) } }), - _ => Err(Error::AccountAliasNotFound), + _ => Err(Error::AccountAliasNotFound(alias)), } } } +// Build the address "not found" message, concealing the input when it looks like +// a secret key or seed phrase mistyped where an address was expected. A genuine +// alias typo is named so the user can tell which argument and value were wrong. +fn address_not_found_message(value: &str) -> String { + if secret::looks_like_secret(value) { + "invalid address or alias".to_string() + } else { + format!("address alias '{}' not found", sanitize(value)) + } +} + +// The ambiguous-alias warning names the alias so the user can tell which one +// collided, but a secret key or seed phrase saved as an alias is concealed so it +// never reaches the terminal, logs, or JSON output. +fn ambiguous_alias_warning(value: &str) -> String { + if secret::looks_like_secret(value) { + "ScAddress alias is ambiguous, assuming it is a contract".to_string() + } else { + format!( + "ScAddress alias {} is ambiguous, assuming it is a contract", + sanitize(value) + ) + } +} + #[cfg(test)] mod tests { use super::*; @@ -159,7 +212,7 @@ mod tests { KeyType::Identity.write(native, &key, dir.path()).unwrap(); let err = UnresolvedScAddress::Alias(native.to_string()) - .resolve(&locator, network_passphrase, None) + .resolve(&locator, network_passphrase, None, &Print::new(false)) .unwrap_err(); assert!(matches!(err, Error::ReservedAliasShadowsKey(alias) if alias == native)); @@ -193,7 +246,7 @@ mod tests { .unwrap(); let err = UnresolvedScAddress::Alias(native.to_string()) - .resolve(&locator, network_passphrase, None) + .resolve(&locator, network_passphrase, None, &Print::new(false)) .unwrap_err(); assert!(matches!( @@ -219,7 +272,7 @@ mod tests { KeyType::Identity.write("bobmux", &key, dir.path()).unwrap(); let resolved = UnresolvedScAddress::Alias("bobmux".to_string()) - .resolve(&locator, network_passphrase, None) + .resolve(&locator, network_passphrase, None, &Print::new(false)) .unwrap(); assert_eq!(resolved, xdr::ScAddress::from_str(MUXED).unwrap()); @@ -344,7 +397,7 @@ mod tests { for input in [MISTYPED_SECRET, MALFORMED_SEED] { let err = UnresolvedScAddress::from_str(input) .unwrap() - .resolve(&locator, network_passphrase, None) + .resolve(&locator, network_passphrase, None, &Print::new(false)) .unwrap_err(); assert!( !err.to_string().contains(input), @@ -360,4 +413,78 @@ mod tests { assert!(!format!("{address:?}").contains(input)); } } + + #[test] + fn resolve_names_plain_alias() { + let dir = tempfile::tempdir().unwrap(); + let locator = locator::Args { + config_dir: Some(dir.path().to_path_buf()), + }; + let network_passphrase = "Test Network"; + + // A genuine alias typo is not secret-bearing, so it is still named in the + // error — otherwise a multi-address command gives no clue which value + // failed. + let err = UnresolvedScAddress::Alias("nosuchalias".to_string()) + .resolve(&locator, network_passphrase, None, &Print::new(false)) + .unwrap_err(); + assert!(err.to_string().contains("nosuchalias"), "got: {err}"); + } + + #[test] + fn debug_echoes_plain_alias() { + let address = UnresolvedScAddress::from_str("nosuchalias").unwrap(); + assert!(format!("{address:?}").contains("nosuchalias")); + } + + #[test] + fn debug_error_conceals_secret_bearing_input() { + // The error's own `Debug` must not leak the raw input either, so a debug + // formatter or logger can't print a secret pasted where an address was + // expected. + for input in [MISTYPED_SECRET, MALFORMED_SEED] { + let err = Error::AccountAliasNotFound(input.to_string()); + assert!(!format!("{err:?}").contains(input)); + } + } + + #[test] + fn debug_error_names_plain_alias() { + let err = Error::AccountAliasNotFound("nosuchalias".to_string()); + assert!(format!("{err:?}").contains("nosuchalias")); + } + + #[test] + fn ambiguous_warning_conceals_secret_bearing_input() { + for input in [MISTYPED_SECRET, MALFORMED_SEED] { + assert!(!ambiguous_alias_warning(input).contains(input)); + } + } + + #[test] + fn ambiguous_warning_names_plain_alias() { + assert!(ambiguous_alias_warning("nosuchalias").contains("nosuchalias")); + } + + // A named value reaches the terminal, so terminal control characters in a + // mistyped alias must be escaped rather than interpreted. + const CONTROL_CHARS: &str = "alias\u{1b}[31m\u{7}"; + + #[test] + fn not_found_message_escapes_control_characters() { + let message = address_not_found_message(CONTROL_CHARS); + assert!( + !message.contains('\u{1b}') && !message.contains('\u{7}'), + "{message}" + ); + } + + #[test] + fn ambiguous_warning_escapes_control_characters() { + let message = ambiguous_alias_warning(CONTROL_CHARS); + assert!( + !message.contains('\u{1b}') && !message.contains('\u{7}'), + "{message}" + ); + } } diff --git a/cmd/soroban-cli/src/config/secret.rs b/cmd/soroban-cli/src/config/secret.rs index 93630d545b..debeba4967 100644 --- a/cmd/soroban-cli/src/config/secret.rs +++ b/cmd/soroban-cli/src/config/secret.rs @@ -102,6 +102,45 @@ pub enum HardwareKind { Ledger, } +/// Whether `value` could be a secret key or seed phrase mistyped where an alias +/// or address was expected. Callers use it to decide whether a "not found" error +/// may echo the raw input: a genuine alias is safe to name so the user can see +/// which value was wrong, but secret-bearing input must be concealed. The check +/// is shape-based on purpose — a one-character typo of a real secret no longer +/// decodes yet still reveals nearly all of it, so matching a successful parse +/// would miss exactly the case that matters. +#[must_use] +pub fn looks_like_secret(value: &str) -> bool { + // Trim first: a secret pasted with surrounding whitespace (a quoted shell + // arg, a `STELLAR_CONTRACT_ID` env value) would otherwise fail the `S`-prefix + // check on a leading space and leak the near-complete key. No legitimate + // alias has surrounding whitespace anyway, so trimming only ever conceals + // more. The multi-word check below already ignores leading/trailing + // whitespace via `split_whitespace`, so it reads the same trimmed value. + let value = value.trim(); + // A secret seed strkey is `S…`, 56 uppercase base32 characters. Match on the + // `S` prefix and length (with slack for a dropped/added character) and + // tolerate a few stray characters, so a typo that lands outside the base32 + // alphabet — a lowercase letter, `0`/`1`/`8`/`9`, punctuation — still looks + // like a secret rather than printing the near-complete key. No real alias is + // a 50+ character, almost-entirely-base32 string beginning with `S`. + let non_base32 = value + .bytes() + .filter(|b| !(b.is_ascii_uppercase() || (b'2'..=b'7').contains(b))) + .count(); + let strkey_shaped = + (50..=60).contains(&value.len()) && value.starts_with('S') && non_base32 <= 3; + // A contract id, address, or alias never contains whitespace, so any + // multi-word input here is not a legitimate value — treat it as a possible + // seed phrase and conceal it. Matching on "two or more words" rather than a + // mnemonic's exact length (12–24) means every spacing drift is caught, + // including a joined word that drops a 12-word phrase to 11 tokens. The only + // cost is that a nonsensical space-containing input is not named in the + // error, which is harmless: it could never resolve anyway. + let mnemonic_shaped = value.split_whitespace().count() >= 2; + strkey_shaped || mnemonic_shaped +} + impl FromStr for Secret { type Err = Error; @@ -277,6 +316,50 @@ mod tests { const TEST_SEED_PHRASE: &str = "depth decade power loud smile spatial sign movie judge february rate broccoli"; + #[test] + fn looks_like_secret_matches_secret_shaped_input() { + assert!(looks_like_secret(TEST_SECRET_KEY)); + assert!(looks_like_secret(TEST_SEED_PHRASE)); + // A valid strkey charset with a bad checksum still looks like a secret. + assert!(looks_like_secret( + "SBF5HLRREHMS36XZNTUSKZ6FTXDZGNXOHF4EXKUL5UCWZLPBX3NGJ4BX" + )); + // A seed phrase with a bad final word keeps its 12-word shape. + assert!(looks_like_secret( + "illness spike retreat truth genius clock brain pass fit cave bargain xyzzy" + )); + // A joined word drops a 12-word phrase to 11 tokens; still concealed. + assert!(looks_like_secret( + "illness spike retreat truth genius clock brain pass fit cave bargaintoe" + )); + // Typos that land outside the base32 alphabet (lowercase, `0`/`1`) must + // still be detected, or the near-complete key would be printed. + assert!(looks_like_secret( + "SBF5HLRREHMS36XZNTUSKZ6FTXDZGNXOHF4EXKUL5UCWZLPBX3NGJ4Bx" + )); + assert!(looks_like_secret( + "SBF5HLRREHMS36XZNTUSKZ6FTXDZGNXOHF4EXKUL5UCWZLPBX3NGJ401" + )); + // Surrounding whitespace (a quoted arg or env value) must not defeat + // detection: a leading space would otherwise fail the `S`-prefix check. + assert!(looks_like_secret(&format!(" {TEST_SECRET_KEY}"))); + assert!(looks_like_secret(&format!("{TEST_SECRET_KEY} "))); + } + + #[test] + fn looks_like_secret_rejects_aliases_and_ids() { + assert!(!looks_like_secret("alice")); + assert!(!looks_like_secret("my-token")); + assert!(!looks_like_secret("native")); + // A contract id (C…) and a public key (G…) are not secrets. + assert!(!looks_like_secret( + "CA3D5KRYM6CB7OWQ6TWYRR3Z4T7GNZLKERYNZGGA5SOAOPIFY6YQGAXE" + )); + assert!(!looks_like_secret(TEST_PUBLIC_KEY)); + // A 64-char hex wasm hash begins with a hex digit, never `S`. + assert!(!looks_like_secret(&"a".repeat(64))); + } + #[test] fn test_from_str_for_secret_key() { let secret = Secret::from_str(TEST_SECRET_KEY).unwrap();