Repository navigation
Stop echoing a mistyped secret key given where an address is expected - #2785
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Prevents mistyped secrets in address parameters from appearing in errors, debug output, or JSON responses.
Changes:
- Replaces echoed alias errors with
invalid address or alias. - Redacts unresolved aliases in
Debugoutput. - Adds unit and integration regression tests.
| File | Description |
|---|---|
cmd/soroban-cli/src/config/sc_address.rs |
Redacts aliases and sanitizes address-resolution errors. |
cmd/crates/soroban-test/tests/it/config.rs |
Tests token address error redaction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
leighmcculloch
left a comment
leighmcculloch
left a comment
There was a problem hiding this comment.
One concern as this degrades the UX more than it necessarily needs to, but regardless resolves the issue and defer to you on whether the UX issue mentioned inline is worth the additional complexity.
| AccountAliasNotFound(String), | ||
| // 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")] |
There was a problem hiding this comment.
This also drops the alias name for the ordinary case of a mistyped alias. --account alise used to say Account alias "alise" not Found and now only says invalid address or alias, so the user can't tell which argument or value was wrong. This matters most for commands that take several address args (token transfer --from --to, --spender).
There was a problem hiding this comment.
For a mistyped alias the value is still right there in the command the user just ran, so the only thing lost is which arg failed on a multi-address command — and that matches how --from already reports (invalid signing key or identity name). I'd keep it consistent and treat per-arg naming as a possible follow-up.
There was a problem hiding this comment.
the only thing lost is which arg failed on a multi-address command
That's the issue I'm concerned with.
that matches how
--fromalready reports (invalid signing key or identity name). I'd keep it consistent and treat per-arg naming as a possible follow-up.
I think this is an issue with the change on --from too and in other places that behave this way, there's no way to tell what the error which field the error is referring to. Maybe we can attach the field name the error is arising from?
There was a problem hiding this comment.
Followed up in #2795: concealment is now conditional — a genuine alias typo is named again (address alias 'nosuchalias' not found), so you can see which value was wrong, while only secret-shaped input is hidden. Same treatment extends to the contract-id path, which had the identical leak.
What
tokencommands that take an address (transfer --to,allowance --spender,balance --account, and the others sharingUnresolvedScAddress) echoed the raw input back in the "alias not found" error, in both text and JSON. A mistyped secret key or seed phrase pasted there printed a near-copy of the secret to the terminal, logs, and JSON consumers. The error is now a generic non-echoinginvalid address or alias, matching the behaviour--fromalready has. A customDebugimpl also conceals the rawAliasvalue so it can't leak through an error chain.Why
Fixes #2771.
--fromalready hid the value (invalid signing key or identity name), but the address params echoed it, so a one-character typo of a real secret leaked it. This is the address-resolution counterpart to #2763, which fixed the signing path.Known limitations
A genuine alias typo (e.g.
--to myaccount) no longer names the alias in the error — the value is never echoed, by design, the same trade-off--frommakes. A valid secret key or seed phrase in an address param still resolves to its derived account (existing behaviour); only inputs that fail to resolve reach this error.