From 1d29de196ede14d453d3a757324f49ed57eaecc3 Mon Sep 17 00:00:00 2001 From: Ranjeet2063 Date: Thu, 15 Oct 2026 11:56:55 +0545 Subject: [PATCH 1/3] fix(keys): avoid persisting one-off --hd-path overrides for secure-store identities When reading an unmigrated SecureStore identity that does not yet have its public key cached, a caller might specify an ad-hoc --hd-path override (e.g. `stellar keys address alice --hd-path 3`). Previously, this override was written back to the identity file on disk, permanently mutating the identity's default account for future invocations. Only persist the migrated cache to disk if no override was provided or if the override explicitly matches the identity's configured derivation path. Closes #2777 --- cmd/soroban-cli/src/config/locator.rs | 35 +++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index 481df7917b..ceafd35306 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -369,8 +369,12 @@ impl Args { hd_path: effective, }); // Best-effort write-back: if persistence fails we still return the - // freshly-derived value so the current call succeeds. - let _ = self.write_key(key_or_name, &migrated); + // freshly-derived value so the current call succeeds. Only persist the + // cache when deriving for the identity's own default hd_path. A one-off + // override with `--hd-path N` must not permanently mutate the identity on disk. + if hd_path.is_none() || hd_path == *persisted_hd_path { + let _ = self.write_key(key_or_name, &migrated); + } return Ok(migrated); } Ok(key) @@ -1649,6 +1653,33 @@ mod tests { .unwrap(); assert!(matches!(key, Key::PublicKey(_))); } + + #[test] + fn does_not_persist_hd_path_override_on_one_off_lookup() { + let (_dir, locator) = locator_with_tempdir(); + + let unmigrated = Secret::SecureStore { + entry_name: "secure_store:org.stellar.cli-alice".to_string(), + public_key: None, + hd_path: None, + }; + locator.write_identity("alice", &unmigrated).unwrap(); + + // Calling with an explicit one-off hd_path override (e.g. Some(3)) + // must not rewrite the identity on disk. + let _ = locator.read_key_with_secure_store_cache("alice", Some(3)); + + // Verify identity on disk still has its original unmutated hd_path: None + let disk_identity = locator.read_identity("alice").unwrap(); + assert!(matches!( + disk_identity, + Key::Secret(Secret::SecureStore { + public_key: None, + hd_path: None, + .. + }) + )); + } } mod secret_by_public_key { From 69cb6916f9570a01cb4acc708a5b74bf14e42496 Mon Sep 17 00:00:00 2001 From: Ranjeet2063 Date: Thu, 15 Oct 2026 13:22:59 +0545 Subject: [PATCH 2/3] docs(locator): clarify conditional cache persistence in method docs Update doc comments on `read_key_with_secure_store_cache` and `get_secret_key_with_hd_path` to state that only the identity's configured derivation path is cached to disk, leaving one-off overrides in-memory. Address review comments on PR #2805. --- cmd/soroban-cli/src/config/locator.rs | 43 +++++++-------------------- 1 file changed, 11 insertions(+), 32 deletions(-) diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index ceafd35306..fc5d8e71c2 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -340,8 +340,11 @@ impl Args { } /// Like [`Args::read_key`], but for a `SecureStore` identity loaded from disk - /// that lacks a cached public key, derive one via the keychain (one prompt) - /// and persist it back so subsequent reads avoid the keychain. + /// that lacks a cached public key, derive one via the keychain (one prompt). + /// If the derived public key corresponds to the identity's configured derivation + /// path (or no explicit override was passed), it is persisted back to disk so + /// subsequent reads avoid the keychain. A differing one-off `hd_path` override + /// is derived in-memory and returned without mutating the identity on disk. pub fn read_key_with_secure_store_cache( &self, key_or_name: &str, @@ -392,8 +395,10 @@ impl Args { /// Like [`Args::get_secret_key`], but if the secret is a `SecureStore` /// identity loaded from disk without a cached public key, derive it for the - /// given `hd_path` and persist the cache. Use from signing paths so the - /// returned `Secret` already carries the data signing needs for the hint. + /// given `hd_path`. If `hd_path` matches the identity's configured derivation + /// path (or none was configured), persist the cache to disk. Use from signing + /// paths so the returned `Secret` already carries the data signing needs for + /// the hint. pub fn get_secret_key_with_hd_path( &self, key_or_name: &str, @@ -1594,7 +1599,8 @@ mod tests { (dir, args) } - // The legacy-file -> derive-via-keychain -> write-back path is + // The legacy-file -> derive-via-keychain -> write-back path (including + // preserving unmigrated state on one-off --hd-path overrides) is // exercised end-to-end by the soroban-test integration test // `secure_store_key_management`. The keyring crate's mock builder // assigns each `Entry` instance its own in-memory credential @@ -1653,33 +1659,6 @@ mod tests { .unwrap(); assert!(matches!(key, Key::PublicKey(_))); } - - #[test] - fn does_not_persist_hd_path_override_on_one_off_lookup() { - let (_dir, locator) = locator_with_tempdir(); - - let unmigrated = Secret::SecureStore { - entry_name: "secure_store:org.stellar.cli-alice".to_string(), - public_key: None, - hd_path: None, - }; - locator.write_identity("alice", &unmigrated).unwrap(); - - // Calling with an explicit one-off hd_path override (e.g. Some(3)) - // must not rewrite the identity on disk. - let _ = locator.read_key_with_secure_store_cache("alice", Some(3)); - - // Verify identity on disk still has its original unmutated hd_path: None - let disk_identity = locator.read_identity("alice").unwrap(); - assert!(matches!( - disk_identity, - Key::Secret(Secret::SecureStore { - public_key: None, - hd_path: None, - .. - }) - )); - } } mod secret_by_public_key { From d423f60a266dc6df4ee1d3e5c5447fe28a0fb4c0 Mon Sep 17 00:00:00 2001 From: Ranjeet2063 Date: Thu, 15 Oct 2026 21:34:58 +0545 Subject: [PATCH 3/3] fix(locator): normalize default derivation path when checking hd-path override Compare effective and persisted paths using unwrap_or_default() so None and Some(0) are recognized as the same default derivation path, matching the normalization across the rest of the codebase (e.g. secret.rs). Also ensure the original persisted_hd_path representation is preserved on disk. --- cmd/soroban-cli/src/config/locator.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs index fc5d8e71c2..1287c6d382 100644 --- a/cmd/soroban-cli/src/config/locator.rs +++ b/cmd/soroban-cli/src/config/locator.rs @@ -373,10 +373,16 @@ impl Args { }); // Best-effort write-back: if persistence fails we still return the // freshly-derived value so the current call succeeds. Only persist the - // cache when deriving for the identity's own default hd_path. A one-off - // override with `--hd-path N` must not permanently mutate the identity on disk. - if hd_path.is_none() || hd_path == *persisted_hd_path { - let _ = self.write_key(key_or_name, &migrated); + // cache when deriving for the identity's own default hd_path (normalizing + // None and Some(0) as the same path, consistent with the rest of the codebase). + // A one-off override with `--hd-path N` must not permanently mutate the identity on disk. + if effective.unwrap_or_default() == persisted_hd_path.unwrap_or_default() { + let to_persist = Key::Secret(Secret::SecureStore { + entry_name: entry_name.clone(), + public_key: Some(format!("{pk}")), + hd_path: *persisted_hd_path, + }); + let _ = self.write_key(key_or_name, &to_persist); } return Ok(migrated); }