Repository navigation
Ask before a host or identity write lets a vault key sign in somewhere new - #122
Sadykhzadeh wants to merge 7 commits into
Conversation
…as plain data operations The webview writes hosts and identities, and deciding whether such a write lets a vault key sign in somewhere new needs two things core did not offer: the credential resolution connect uses, callable without connecting, and the host and identity edits as operations on StoreData, so a write can be applied to a copy before it is committed.
key_reach compares what the saved hosts allow a vault key to do before and after a write: which account on which server it signs in to, and what command runs there. The reach is resolved the way a connection resolves it, so a link to an identity, a switch of auth method or a removed identity that sends hosts back to their own key are all caught without listing fields one by one.
…ey sign in somewhere new connect_ssh resolves everything from the store, but the page writes the store: add_host with a foreign hostname and a key id from list_keys, then connect_ssh, and the first-seen host key is pinned without a word, so the key signs in to that server and the startup command runs on it. add_host, update_host, add_identity, update_identity and delete_identity now preview the write and compare what every saved host would let a vault key do with what it allows today. Anything new is shown in an OS-drawn dialog, parented to the main window like the host key one: host label, user@hostname:port, the key by its vault label and fingerprint, and the startup command. Text written by the page is escaped and cut visibly so it cannot redraw the dialog. Cancel writes nothing and returns an error tagged [host-change-confirmation-declined]. Writes that reach nothing new are saved without a question, so renaming, tagging, password hosts and copies of an existing host stay as they were. The store lock is not held across the dialog; the write is previewed again afterwards and refused if it would now allow more than was shown. The dialog uses its own gate, so declining it does not silence host key prompts for 30 seconds, or the other way round.
A save that waits on the native dialog and is cancelled comes back as an error. HostForm let it escape as an unhandled rejection and IdentityManager put the raw tagged message in an alert. Both now keep the form open with what was typed and say the change was not saved, distinct from a real failure; the save button stays disabled while the dialog is open so a second click cannot queue a second write. A key generated or imported for an identity whose save was cancelled is selected for the retry instead of being created again, and a cancelled identity delete stays quiet.
computerbox124
left a comment
computerbox124
left a comment
There was a problem hiding this comment.
Code review: 9 findings, inline.
High: these undo the protection the PR adds, so fix them before merging.
- U+2028 and U+2029 aren't escaped, so text the page writes can still redraw the dialog (
host_change_prompt.rs:170). - Allow approves all new reaches, but the dialog shows only the first 5 (
host_change_prompt.rs:112).
Medium
3. Enter confirms, and the page can time the dialog to the user's terminal keystrokes (host_change_prompt.rs:103). Plausible; needs a check on each platform.
4. The 30 s decline cooldown turns the Cancel → fix → retry flow the forms invite into a red "Could not save" (host_change_prompt.rs:47).
5. The length limits count characters before escaping, so the dialog can still grow far past the screen (host_change_prompt.rs:185).
Design
6. The check lives only in commands.rs; Store's writers and the now-pub commit skip it (core/src/store.rs:113).
7. startup_command in KeyReach adds prompts but no protection, because connect_ssh + session_write already run commands (core/src/key_reach.rs:44). Optional.
Nits
8. The decline-tag matching duplicates KnownHostsView.vue (desktop/src/lib/hostChange.ts:4).
9. A /// followed by a blank line becomes delete_host's rustdoc; clippy warns (commands.rs:81).
The comment-hygiene check passes on this branch.
Generated by Claude Code
|
|
||
| /// Characters that can change how the text around them is laid out or read: | ||
| /// zero-width characters, and the marks that reorder bidirectional text. | ||
| fn is_layout_mark(c: char) -> bool { |
There was a problem hiding this comment.
High: U+2028 and U+2029 can still redraw the dialog.
shown() escapes char::is_control() (Unicode category Cc) plus the marks listed here. U+2028 LINE SEPARATOR and U+2029 PARAGRAPH SEPARATOR are neither. They are categories Zl/Zp, is_control() returns false for both, and they sit just below the '\u{202A}'..='\u{202E}' range. AppKit and Pango both lay them out as line breaks.
So the page can call add_host with a label such as
"prod\u{2028}\u{2028}This is a routine update to an existing host; no new access is granted.\u{2028}"
and the native dialog shows a fake reassuring paragraph. That is the redraw this function and stored_text_cannot_redraw_the_dialog are meant to prevent.
Suggested fix: add '\u{2028}' | '\u{2029}' here and to the test. Better still, escape by general category (Cc, Cf, Zl, Zp) instead of a hand-picked list, so the next missed code point doesn't reopen this.
Generated by Claude Code
There was a problem hiding this comment.
Confirmed and fixed in 35fc5cc. char::is_control() covers only Cc, so U+2028/U+2029 (Zl/Zp) went through unescaped.
I went with the category-based approach rather than adding two more code points. is_hidden now escapes anything whose Unicode general category is Cc, Cf, Zl or Zp. It also escapes Co and Cn: a format character added in a later Unicode version shows up as unassigned in this build, and private-use glyphs could be drawn as anything. The lookup comes from unicode-general-category (Unicode 16, no dependencies, pinned to =1.1.0), because std doesn't expose general category.
New test separators_and_unlisted_format_characters_are_escaped covers your example label and one format character from each part of the range that the old list missed (U+180E, U+FFF9, U+E0041 tag), plus U+E000 and an unassigned code point. It also checks that visible non-ASCII text (Prod — Zürich 東京 🚀) is left as is.
Generated by Claude Code
| "This change lets a key from your vault sign in where no saved host lets it sign in \ | ||
| now:\n", | ||
| ); | ||
| for new in reaches.iter().take(LISTED_HOSTS) { |
There was a problem hiding this comment.
High: Allow approves hosts the dialog never names.
Only the first LISTED_HOSTS (5) reaches are spelled out, and the rest collapse into …and N more hosts. But write_confirmed checks the post-dialog preview against the full shown list (shown.contains(now)). So Allow approves every entry, including ones whose hostname, account, key and command the user never saw.
Scenario: the page quietly adds a host for attacker.example that links a password-auth identity. It has no key reach, so no prompt, and it sits last in the list. Later the user switches that identity to a key. The dialog lists their five real servers plus …and 1 more host. The user recognises the list and clicks Allow, and the vault key now signs in to attacker.example.
Suggested fix: never approve an entry that wasn't shown. Either refuse a write that introduces more than LISTED_HOSTS new reaches (the user can make the change in smaller steps), or list every destination, at least by user@host:port and key.
Generated by Claude Code
There was a problem hiding this comment.
Confirmed and fixed in 35fc5cc. I took the first option: an entry that wasn't shown can never be approved.
write_confirmedrefuses a write that introduces more thanLISTED_HOSTSnew reaches before any dialog opens. The error isn't tagged, so the form shows it as a failure and tells the user to make the change in smaller steps, a few hosts at a time.message()no longer has the…and N morebranch. It lists every reach it is given, so the dialog names everything Allow approves.
I didn't list every destination instead, because that brings back the dialog-taller-than-the-screen problem LISTED_HOSTS exists to prevent. The cost is that an identity edit reaching more than 5 new hosts has to be done in steps. That seems right for a rare action that fans a key out that widely.
Tests: a_dialog_asks_about_every_host_it_approves (exactly 5, all named) and a_write_reaching_more_hosts_than_a_dialog_lists_is_refused_unasked (6 hosts: no dialog, nothing written).
Generated by Claude Code
| reaches: &[NewReach], | ||
| keys: Option<&[KeyMeta]>, | ||
| ) -> Result<(), String> { | ||
| host_key_prompt::confirm(app, gate, TITLE, &message(reaches, keys), "Allow").await |
There was a problem hiding this comment.
Medium (plausible, needs a check on each platform): one Enter keystroke can confirm.
The description already notes that Allow is the focused button. The added risk is timing. The page sees everything the user types into xterm, because it forwards keystrokes through session_write. So script can call add_host a few hundred ms before an Enter it expects, for example while the user is typing a shell command. The OS dialog takes focus and the user's Enter lands on Allow. No click is needed, which undercuts "the page cannot act without the user".
Possible mitigation: discard an Allow that arrives within about 1 s of the dialog opening, and ask again (browsers use a similar input delay on permission prompts). Don't swap the button labels instead: Escape maps to the cancel button, so that would make Escape approve.
Generated by Claude Code
There was a problem hiding this comment.
Agreed this is real. The page sees the terminal keystroke stream, and it's Space as well as Enter: on Windows and GTK, Space presses the focused button. Mitigated in 35fc5cc as you suggested.
host_key_prompt::confirm now goes through read_answer. An Allow that comes back within READ_DELAY (1 s, the same as Firefox's security.dialog_enable_delay) of the dialog opening doesn't count. The dialog is shown again with a first line saying the button was pressed before the dialog could be read. Cancel and Escape still take effect at once. The loop has no upper bound on purpose: each pass waits on a person, and a held key just keeps getting ignored until it is released. Button labels are unchanged, for the Escape reason you gave.
I put it in the shared confirm, so the host key dialogs ("Forget the key", "Trust the new key") get the same protection: the page can trigger those too. The clock is injected, so the tests are deterministic: a too-soon Allow is asked again, an Allow after reading is taken first time, and Cancel/Unavailable return immediately.
I couldn't check per-platform focus behaviour from here. That still needs the macOS/Linux dialog check that is already open in the test plan.
Generated by Claude Code
| /// The gate these dialogs share. It is kept apart from the host key gate so a | ||
| /// refusal of one kind of question does not silence the other. | ||
| pub fn gate() -> PromptGate { | ||
| PromptGate::for_subject("host change", DECLINED) |
There was a problem hiding this comment.
Medium: cancelling and then retrying is refused for 30 s.
This gate inherits DECLINE_COOLDOWN (30 s) from PromptGate. After Cancel, the forms keep what was typed so the user can fix it and try again. But a second save within 30 s gets a host change confirmation was declined; no further host change confirmations will be offered for 27s. That error doesn't carry [host-change-confirmation-declined], so saveFailure shows it in red as Could not save: ….
Scenario: add a key host, spot a typo in the dialog, click Cancel, fix the hostname, click "Add host" again. The result is a red error. The Vitest retry test mocks the backend, so it never hits the cooldown.
Suggested fix: keep some spam protection, but give this gate a much shorter cooldown, or tag the cooldown refusal so the form can say "try again in N s" instead of reporting a failure.
Generated by Claude Code
There was a problem hiding this comment.
Confirmed (the Vitest retry mocks the backend, so it never hit the gate). Fixed with both of your suggestions:
- 35fc5cc: the cooldown is now per gate (
PromptGate::for_subject(subject, declined, cooldown)). The host change gate uses 5 s, so Cancel → fix the typo → save is asked again. A script still can't reopen the dialog the moment it is dismissed. Host key dialogs keep 30 s. - Any gate's cooldown refusal now carries
[confirmation-cooling-down]. - 258fc22: the host and identity forms show that tag as "Not saved. A confirmation was cancelled a moment ago; try again in a few seconds to be asked." instead of
Could not save: …. Identity delete says "Not deleted. …".KnownHostsViewstrips the tag, so its existing message reads as before.
Tests: each_gate_holds_off_for_its_own_cooldown (Rust), plus a HostForm Vitest that saves during the cooldown and then retries successfully.
Generated by Claude Code
| /// reassuring paragraph, say — so they are printed as escapes. A long value is | ||
| /// cut with a count of what was left out, so padding cannot push the part that | ||
| /// matters out of view without the cut being visible. | ||
| fn shown(text: &str, limit: usize) -> String { |
There was a problem hiding this comment.
Medium: the length limits don't bound the dialog's size.
take(limit) counts input characters, and each escaped character then grows to as many as 8 (\u{202e}). A 64-character label or username made of U+202E renders as 512 characters, the hostname (255) as 2,040, and the command (300) as 2,400. Five such hosts make a message of about 27,000 characters, far taller than the screen, which is the case LISTED_HOSTS is there to prevent. The closing warning and Cancel end up off-screen while Allow keeps focus.
Suggested fix: apply the limit to the output (stop once out reaches the limit, then append the cut notice), or collapse runs of escaped characters into something like [12 hidden characters].
Generated by Claude Code
There was a problem hiding this comment.
Confirmed and fixed in 35fc5cc. I took the first option: shown() now applies the limit to the output. Each character is rendered (as itself or as its escape) and kept only if the whole piece still fits. So an escape is never cut in half, and the cut notice counts the input characters left out.
A 64 × U+202E label now prints 8 escapes plus … (56 more characters), instead of 512 characters. With the 5-host cap, the worst case is back to the size the author's limits were designed for, not ~27k. I didn't collapse runs into [N hidden characters]: alternating visible and hidden characters would make that longer than plain escapes.
Test: escapes_count_toward_the_limit, including an escape that would cross the limit and is dropped whole.
Generated by Claude Code
| /// for the rest of the session, so one bad mutation would take every | ||
| /// unrelated edit down with it. | ||
| fn commit(&mut self, f: impl FnOnce(&mut StoreData)) -> Result<()> { | ||
| pub fn commit(&mut self, f: impl FnOnce(&mut StoreData)) -> Result<()> { |
There was a problem hiding this comment.
Design: the Store API can skip the confirmation.
The "no new key reach without confirmation" invariant is enforced only in commands.rs::write_hosts. Store::add_host, update_host, add_identity, update_identity and remove_identity are still public writers with no check, and commit is now pub and accepts any mutation. A future command (an ~/.ssh/config import, duplicate host, bulk edit or workspace restore) that calls them directly would add key reach with no dialog, and nothing in the types or tests would catch it.
Suggestion: keep commit private and route every write that can add key reach through one entry point, for example a preview / apply-confirmed pair on Store. Then the check can't be skipped by accident.
Generated by Claude Code
There was a problem hiding this comment.
Agreed, and fixed in 35fc5cc by moving the invariant into Store.
commitis private again. It now just callscommit_confirmed(f, &[]).commit_confirmed(edit, confirmed)applies the edit, runskey_reach::introducedagainst the previous data, and rolls back with the newCoreError::UnconfirmedKeyReachif any new reach isn't inconfirmed. Every write goes through it, workspace and group writes included (for those the check is trivially empty).preview(edit)returns what a write would introduce, without writing.- The typed writers (
add_host,update_identity, …) confirm nothing. They still make every other change but now fail on one that would aim a key somewhere new. An~/.ssh/configimport or bulk edit added later can't skip the dialog by accident; it would have to pass in a confirmed list.
write_confirmed now does preview → dialog → commit_confirmed(&edit, &shown). The "store changed under the dialog" re-check is now this same core check, and it maps to the existing message.
Tests in core/src/store.rs: an unconfirmed key-host write is refused and never reaches disk; the same write passes with its preview; a confirmation covering only one of two new reaches is refused; renames, startup-command edits and removals need nothing.
Generated by Claude Code
| username: resolved.username.to_owned(), | ||
| hostname: host.hostname.clone(), | ||
| port: host.port, | ||
| startup_command: host |
There was a problem hiding this comment.
Design (optional): gating startup_command doesn't protect anything.
The page can already run any command on a saved destination by calling connect_ssh(host_id) and then session_write, with no dialog (the description puts sessions out of scope). So changing only the startup command on an already-allowed destination prompts, while typing the same command into a session does not. The Runs on connect … line and the "and the command runs on it" warning suggest the dialog guards remote command execution, which it doesn't.
Suggestion: still show the command in the dialog for new destinations, but don't count a command-only change as new reach. That means fewer prompts with the same protection. Or keep it and say plainly in the comment that it is informational.
Generated by Claude Code
There was a problem hiding this comment.
Agreed: once a key can sign in somewhere, connect_ssh + session_write can run anything there, remote persistence (~/.bashrc) included. So a command-only prompt added dialog fatigue without adding protection. Applied in 35fc5cc, as the first option you suggested:
KeyReachis now(key_id, username, hostname, port). A changed command, or a copy of an allowed host with a different command, introduces nothing.NewReachcarriesstartup_commandfor display. A new destination still showsRuns on connect: …and the "and the command runs on it" warning.- The post-dialog check compares whole
NewReaches. If the command changes while the dialog is open, the write is refused, so what is committed is always what was shown.
The module docs, the saved_host comment and docs/ARCHITECTURE.md now say plainly that the command is shown but not confirmed on its own. The "each field" test drops the command case, and there's a new a_startup_command_alone_introduces_nothing.
Generated by Claude Code
| // A host or identity write that would let a vault key sign in somewhere new | ||
| // is confirmed by the backend in a native dialog. Cancelling it fails the write | ||
| // with this tag: an answer rather than a fault, and nothing was saved. | ||
| const DECLINED = "[host-change-confirmation-declined]"; |
There was a problem hiding this comment.
Nit: the decline-tag matching is duplicated.
KnownHostsView.vue already has its own DECLINED = "[host-key-confirmation-declined]" and String(cause).includes(...). A shared helper in lib/ (for example isDeclined(cause, tag)) keeps the two in step if a tag or the error format changes. Otherwise a missed copy turns a user's Cancel into a red failure.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 258fc22. desktop/src/lib/confirmation.ts now holds the three tags (HOST_KEY_DECLINED, HOST_CHANGE_DECLINED, and the new COOLING_DOWN) along with hasTag(cause, tag) and untagged(cause). KnownHostsView.vue and lib/hostChange.ts both use it, so neither keeps a private copy. Covered by lib/confirmation.test.ts.
Generated by Claude Code
| Ok(host) | ||
| } | ||
|
|
||
| /// Removing hosts takes reach away and never adds it, so it asks nothing. |
There was a problem hiding this comment.
Nit: this /// comment becomes delete_host's rustdoc.
Followed by a blank line and then #[tauri::command], it reads like a section note but is attached to delete_host, and cargo clippy warns with empty_line_after_doc_comments. Either remove the blank line (if it is meant as the function's doc) or change it to //.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 35fc5cc. It is meant as delete_host's doc, so I removed the blank line ("Removing a host takes reach away and never adds it, so it asks nothing."). cargo clippy no longer reports empty_line_after_doc_comments there.
Generated by Claude Code
- Allow approved every new reach while the dialog named only the first five. A write that reaches more hosts than one dialog can list is now refused before asking, and the dialog lists everything it approves. - Stored text is escaped by Unicode general category (Cc, Cf, Zl, Zp, plus Co and Cn) instead of a hand-picked list, so U+2028/U+2029 and other unlisted marks can no longer lay out a fake paragraph. Length limits count printed escapes, so escaping cannot stretch the dialog. - An Allow that arrives within a second of a dialog opening is ignored and the dialog shown again, so a keystroke the page timed from the terminal cannot confirm it. This covers the host key dialogs too. - The host change gate cools down for 5 s rather than 30 s, so Cancel, fix, save again is asked instead of refused, and the cooldown refusal carries its own tag. - Store::commit is private again and checks every write: one that lets a vault key sign in somewhere new needs the reaches the user confirmed (preview + commit_confirmed), so no writer can skip the dialog. - A startup command is no longer part of a key reach. A session to an allowed destination can already run any command, so asking about a command-only change added prompts without protection; new destinations still show their command. - delete_host's doc comment no longer trails a blank line.
The confirmation tags now live in one helper shared by KnownHostsView and the host change forms, so a changed tag cannot turn a user's Cancel into a red failure in one view but not the other. While a recent Cancel holds the dialog shut, the host and identity forms say to try again in a few seconds instead of reporting that the save failed.
|
Code Review by QodoView medium (4)
1. A prompt can accept an unread approval
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| let opened = now(); | ||
| let answer = show(text).await; | ||
| if answer != Answer::Confirmed || now().duration_since(opened) >= READ_DELAY { | ||
| return answer; |
There was a problem hiding this comment.
1. A prompt can accept an unread approval 🐞 Bug ⛨ Security
read_answer starts its one-second timer before show queues the native dialog, rather than when the dialog becomes visible. If displaying it takes at least a second, an anticipated Enter or Space keystroke can land on the focused approval button as it appears and still pass the timing check.
Agent Prompt
## Issue description
The read delay starts before the native dialog is displayed, so queueing delays can make an immediate keystroke count as approval.
## Fix Focus Areas
- desktop/src-tauri/src/host_key_prompt.rs[224-264]
## Recommended Fix
Enforce the delay from a native dialog-visible event, or use a native approval control that cannot be activated by a keystroke arriving as the dialog opens.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// Serializes the native confirmation shown before a host or identity write | ||
| /// lets a vault key sign in somewhere new; see `host_change_prompt`. | ||
| pub host_change_prompt: PromptGate, |
There was a problem hiding this comment.
4. Two approval prompts can overlap 🐞 Bug ⛨ Security
AppState gives host-change and host-key confirmations separate gates, each of which only tracks its own open dialog. When both commands are invoked before either is answered, they can present concurrent native dialogs parented to the same window, leaving an approval ambiguous to the user.
Agent Prompt
## Issue description
Independent gates permit host-change and host-key approval dialogs to open at the same time.
## Fix Focus Areas
- desktop/src-tauri/src/state.rs[200-207]
- desktop/src-tauri/src/host_key_prompt.rs[197-207]
## Recommended Fix
Add a shared open-dialog guard across both confirmation types while retaining their separate cancellation cooldowns.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| confirm(shown.clone()).await?; | ||
| let mut store = store.lock().await; | ||
| store.commit_confirmed(&edit, &shown).map_err(|e| match e { |
There was a problem hiding this comment.
2. Concurrent edits to a host can be lost 🐞 Bug ≡ Correctness
write_confirmed releases the store lock for the dialog and later reapplies the original whole-record edit; commit_confirmed checks only for new key reaches, not intervening changes to that record. If another writer changes the same host's label, tags or startup command while approval is pending, the approved write silently replaces those newer values.
Agent Prompt
## Issue description
A pending approval can overwrite unrelated changes another writer made to the same host.
## Fix Focus Areas
- desktop/src-tauri/src/host_change_prompt.rs[86-109]
- core/src/store.rs[141-159]
## Recommended Fix
Record the edited host's version or original value at preview time and refuse a stale whole-record update after the dialog, asking the caller to reload and retry.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// it can be corrected and saved again, and that retry should be asked about | ||
| /// rather than refused. It still stops a script from reopening the dialog the | ||
| /// moment it is dismissed. | ||
| const DECLINE_COOLDOWN: Duration = Duration::from_secs(5); |
There was a problem hiding this comment.
3. A script can re-raise the allow dialog every five seconds 🐞 Bug ⛨ Security
DECLINE_COOLDOWN for the host change gate is 5 s, so after each Cancel a script can call add_host again with the same foreign key host as soon as the gate reopens. Each call puts a fresh native Allow dialog in front of the user, with no limit. This is the click-through fatigue that the 30 s host key cooldown in host_key_prompt.rs exists to prevent, and while it runs the user can stop it only by quitting the app or clicking Allow.
Agent Prompt
## Issue description
The host change confirmation gate holds off for only 5 seconds after a Cancel. A script in the webview can therefore reopen the native Allow dialog every 5 seconds without limit, wearing the user down until they click Allow.
## Fix Focus Areas
- desktop/src-tauri/src/host_change_prompt.rs[54-65]
- desktop/src-tauri/src/host_key_prompt.rs[141-150]
## Recommended Fix
Keep the short cooldown for the first Cancel so the form-retry flow still works, but increase it with each further decline inside a window (for example 5 s, then 30 s, then 5 minutes). Another option is to stop prompting after N declines until a native or user-initiated action resets the gate. Track a decline count in `GateState`, and add a test that repeated declines lengthen the refusal.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Closes #12.
#99 made
connect_sshandsftp_connecttake a saved host id, but the page also writes the store.add_hostwith a foreign hostname,auth: publickeyand a key id fromlist_keys, thenconnect_sshon that id, still signed in to a server of the caller's choosing and ran the startup command there: the first-seen host key is pinned without a prompt.What a write is checked against
For every saved host,
core::key_reachresolves what it lets a vault key do (account, hostname, port, key, startup command), using the same resolutionconnectuses (connection::credentials). A write is previewed on a copy of the store, and only an entry that no saved host already has needs confirming. Comparing resolved entries rather than fields means these are all caught without being listed:add_identityfor an id a saved host already names;delete_identity, where linked hosts fall back to their own key.Renames, tags, groups, password hosts and copies of an existing host save without a question.
The dialog
add_host,update_host,add_identity,update_identityanddelete_identityshow an OS-drawn dialog, the same onehost_key_promptuses and parented to the main window, listing for each affected host: its label,user@hostname:port, the key by its vault label and fingerprint (by id while the vault is locked) and the startup command. Text the page wrote is escaped and cut visibly, so a label cannot redraw the dialog. The store lock is not held across the dialog; the write is previewed again afterwards and refused if it would now allow more than was shown. The dialog has its own gate, so cancelling it does not silence host key prompts.Cancel writes nothing and returns
[host-change-confirmation-declined] …. The host and identity forms stay open with what was typed and say the change was not saved; the save button is disabled while the dialog is open.What this does and does not guarantee
A vault key reaches only destinations the user confirmed, or ones already in the store. It does not cover password hosts (the password is typed into the page anyway), a user who reads the dialog and clicks Allow, or what the page does in a session it has opened (#30).
saved_host's comment says exactly this.Two things worth knowing:
proxy_commandandjump_host_idare stored but never read by the connection code. If they are wired up, they need to feed intokey_reachtoo.Write paths checked
add_host,update_host,add_identity,update_identity,delete_identity.delete_host(removes only),add_group/delete_group, the workspace commands (layout only; connecting still goes by host id),delete_key, andgenerate_key/import_key(key ids are fresh UUIDs).Tests
key_reach: each field, auth switch, identity add/edit/remove, copies, duplicate ids.host_change_prompt: declined writes nothing, no prompt for nothing new, refusal when the store changes under the dialog, dialog text, escaping, truncation, list cap.host_key_prompt: separate gates keep separate cooldowns.Test plan
cargo test --workspacecd desktop && npx vue-tsc --noEmitcd desktop && npm test(389 passed)node scripts/verify/check-comments.mjstauri devon Windows: the issue'sadd_hostcall shows the dialog; Cancel rejects it and the store stays empty; a password host saves with no dialog; linking a host to a key identity prompts with the identity's account, and a key change on that identity names the new keycd desktop/e2e && UI_RECORD=0 npm test— not run: Playwright's Chromium is not installed on this machine, and none of the scenarios touch host or identity writesNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.