Skip to content

Prevent stale keyring entry when overwriting a key.#2505

Merged
fnando merged 4 commits intomainfrom
fix-2492
Apr 21, 2026
Merged

Prevent stale keyring entry when overwriting a key.#2505
fnando merged 4 commits intomainfrom
fix-2492

Conversation

@fnando
Copy link
Copy Markdown
Member

@fnando fnando commented Apr 20, 2026

What

Prevent stale keyring entry when overwriting a key

Why

Close #2492

Known limitations

N/A

Copilot AI review requested due to automatic review settings April 20, 2026 23:39
@fnando fnando requested review from mootz12 and removed request for Copilot April 20, 2026 23:39
@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX Apr 20, 2026
@fnando fnando self-assigned this Apr 20, 2026
@fnando fnando moved this from Backlog (Not Ready) to Needs Review in DevX Apr 20, 2026
@fnando fnando requested review from Copilot April 20, 2026 23:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses issue #2492 by ensuring Secure Store (keyring) entries are actually updated when a key is overwritten, preventing the CLI from continuing to reference stale key material after a successful-looking overwrite.

Changes:

  • Thread an overwrite: bool flag through secure_store::save_secret into StellarEntry::write.
  • Change keyring write behavior to error on existing entries unless overwrite is explicitly allowed, and to always persist the new seed phrase when overwriting.
  • Add unit tests covering overwrite and non-overwrite behavior for keyring writes.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
cmd/soroban-cli/src/signer/secure_store.rs Adds overwrite parameter and forwards it to keyring entry writes.
cmd/soroban-cli/src/signer/keyring.rs Implements overwrite-aware keyring writes and adds tests for the new semantics.
cmd/soroban-cli/src/commands/keys/generate.rs Passes CLI --overwrite through to Secure Store save path.
cmd/soroban-cli/src/commands/keys/add.rs Passes CLI --overwrite through to Secure Store save path.

Comment thread cmd/soroban-cli/src/signer/keyring.rs Outdated
Comment thread cmd/soroban-cli/src/signer/keyring.rs Outdated
Comment thread cmd/soroban-cli/src/commands/keys/generate.rs
Comment thread cmd/soroban-cli/src/commands/keys/add.rs
Copy link
Copy Markdown
Contributor

@mootz12 mootz12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG2M! One minor suggestion:

[picky] could we add a regression test case in soroban-test/tests/it/integration/secure_store.rs for the failure without --overwrite and working successfully with --overwrite?

@fnando fnando enabled auto-merge (squash) April 21, 2026 20:58
@fnando fnando merged commit bb4da7b into main Apr 21, 2026
213 checks passed
@fnando fnando deleted the fix-2492 branch April 21, 2026 21:14
@github-project-automation github-project-automation Bot moved this from Needs Review to Done in DevX Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Secure-store overwrite keeps a stale keyring entry bound to the alias

3 participants