Close open sessions on vault reset, and stop retaining imported key material after Cancel, a failed import or a vault lock

[Sadykhzadeh]

Closes #56. Both findings originally came from #44 (items 1 and 3), which can have them checked off when this lands.

Same defect in two places: a security control that does less than the UI says it does.

1. A reset now drops the access the destroyed keys bought

secure_reset_vault destroyed the vault, the biometric credential and the auth generation, and left state.sessions, state.sftp and state.local_terminals alone. Every SSH session, SFTP channel and local PTY opened before the reset stayed live and writable through session_write and the sftp_* commands, while the dialog told the user the reset "permanently deletes all stored SSH keys and credentials". The keys were gone; the connections they authenticated were not.

I went with closing the sessions rather than softening the copy. The reset is the only "make it stop" gesture in the app, it is already built like a containment control everywhere else, and a reset that leaves a writable shell open on the machine the user was worried about is worse than no reset — they now believe they are done. Re-establishing the sessions is not a real cost, since the keys that opened them no longer exist.

The care is all in not letting that break the reset itself:

• SessionManager::close_all / SftpManager::close_all drain their map under the lock and release the guard before any network work. A session stops being addressable the instant the call starts, so a remote that never acknowledges the disconnect can only delay the polite goodbye — it can never keep a session reachable. The existing close holds the map lock across handle.disconnect().await, which is exactly the shape that would have deadlocked here.
• The teardown runs only past the destructive boundary, after reset_crossed_destructive_boundary and after auth_generation.invalidate() / the passphrase clear. normalize_reset_result and reset_crossed_destructive_boundary are untouched and keep their semantics, including the [vault-reset-durability] marker the frontend reconciles against.
• Its outcome is discarded and it runs under a budget. contain_open_sessions returns whether it finished; secure_reset_vault ignores that and returns the reset_result it already computed. A connection that will not close cannot turn a completed reset into a reported failure, and cannot leave the command hanging after the vault file is already unlinked — which would strand the renderer showing an unlocked vault that no longer exists.

The reset dialog now also says open sessions and terminals are closed, so the warning covers the consequence the user is consenting to.

session_write, session_resize and close_session are unchanged; the panes reconcile through the existing session-closed event, which the reader thread and the SSH channel loop already emit when their transport goes away.

2. KeyManager stops retaining imported key material

resetForm() ran on success only. A private key pasted — or pulled off disk by Browse, which loads the whole file — survived Cancel, the dialog's own dismissal, a failed import and a vault lock, and was rendered straight back into the plaintext <Textarea> the next time the dialog opened.

Correction to an earlier revision of this description and of the first commit message. Both said the auto-lock path was covered by the showAdd watcher and onUnmounted. It was not, and the enumeration should not have implied it. Neither of those fires on an auto-lock:

• useAutoLock calls vault.lock() and nothing else. It never opens VaultUnlockModal — that is driven by ui.showVaultUnlockModal, which only the SSH flows set — so showAdd does not change and the visibility watcher does not run.
• VaultUnlockModal is mounted in App.vue as a sibling of </main>, not as a replacement for the view inside it, so KeyManager stays mounted and onUnmounted does not run either.

Against the previous head of this branch, after vault.unlocked flipped false the key was still in addForm, still rendered in the plaintext textarea, and the dialog was still open. The one component that holds raw private key material was the only one missing the guard VaultView.vue and VaultUnlockModal.vue already have. watch(() => vault.unlocked, resetForm, { flush: "sync" }) now closes that gap, with the same unguarded flush: "sync" shape as those two call sites — it clears in both directions, so a lock and an unlock in the same tick cannot carry key material across an authentication boundary.

So the exits that clear it are: the Cancel button, the dialog's @close (which the overlay click emits), the flush: "sync" watcher on showAdd, the flush: "sync" watcher on vault.unlocked, and onUnmounted. The redundancy is deliberate: a sixth exit path added later cannot quietly reopen the window.

There is no Escape path, and I did not add one. An earlier revision of this description listed "overlay/Escape dismissal" as one exit. That was wrong: neither ui/Dialog.vue nor ui/Overlay.vue has a key handler, and App.vue's global keydown only handles Escape for fullscreen panes and the command palette — so Escape does nothing to this dialog at all. Every dialog that closes on Escape today (SftpBrowser.vue, VaultUnlockModal.vue) binds @keydown.escape on its own focused input. Adding Escape to ui/Dialog.vue would change dismissal behaviour for every dialog in the app, which is a UX change that does not belong in a key-retention fix; binding it to this dialog's inputs only would work solely while focus is in a field, which is not worth claiming as a security exit. Corrected the enumeration instead — the retention window is closed by the watchers regardless of how the dialog goes away.

A failed import no longer tears down the whole dialog. The failure branch used to call resetForm(), and clearAddForm also resets import, so a rejected key closed the dialog, dropped the label, and would have reopened on the Generate tab. The common rejection is a wrong passphrase on an encrypted key — the label and the tab selection were never the problem, and making the user rebuild them is punishment for a typo. The secret fields are split out as clearKeyMaterial() and the failure branch calls only that: privateKey and passphrase go, the dialog stays open on the Import tab with the label intact. Clearing the secret is still unconditional.

I did not mask the textarea. A <textarea> has no type="password", so masking means -webkit-text-security or a fake overlay, which breaks the one thing users need that field for — checking that they pasted the right key. And it fixes the wrong half: the material being visible while the user is deliberately looking at it is fine. The defect is that it is still there afterwards.

Tests

New:

• core + desktop/src-tauri: teardown drops every session, SFTP channel and local terminal; a teardown step that never completes still returns to the reset; an unfinished teardown leaves the reset outcome (including the durability marker) untouched.
• desktop/src/components/KeyManager.test.ts: key material is gone after Cancel, after an overlay dismissal, after a rejected import (which also asserts the dialog, the Import tab and the label survive), after a vault lock (which also asserts the dialog closes and the key leaves the DOM), and after unmount.

Non-vacuity, checked by running the suite against the component with the fix reverted:

before (unfixed KeyManager.vue):  3 passed, 2 failed
  ✗ ... when the vault locks              addForm.privateKey still held the key
  ✗ ... when the import is rejected       #key-private gone — the dialog had closed
after:                            5 passed, 0 failed

cargo test --workspace
  clavyn-core unit:        23 passed; 0 failed
  core integration:        28 passed; 0 failed
  clavyn-desktop unit:     21 passed; 0 failed   (18 before)

cd desktop && npm test
  Test Files  37 passed (37)
       Tests  313 passed (313)            (308 before)

cd desktop && npx vue-tsc --noEmit        clean
node scripts/verify/check-comments.mjs     {"ok":true,"count":0,"files":126}
cd desktop/e2e && UI_RECORD=0 npm test     all 9 suites pass, 68 assertions

Also driven through the live renderer: opened the import dialog with a key pasted in, called vault.lock() — which is all an auto-lock does — and the dialog closes with the key absent from the DOM. Cancel/reopen leaves both fields empty. (Dev server on 127.0.0.1:1437; ports near 1420 were held by other checkouts, and Vite binds localhost to ::1 only on this machine, so the served KeyManager.vue was fetched back from the dev server to confirm it was this branch being exercised.)

Expected conflicts

• Keep the vault available and off the KDF during SSH connects #52 — the biggest one. It replaces the passphrase slot with VaultSessionSlot, so the let mut pw = state.passphrase.lock().await; *pw = None; that my teardown call sits directly after becomes state.vault_session.clear().await. The conflict is that one line; the teardown call moves below whichever version wins. It also converts several vault_commands.rs tests to #[tokio::test] and reworks the use block I added imports to. Happy to rebase onto it — say the word and I'll do it in whichever order suits.
• Allow renaming session tabs (custom tab names) #53, if it is still stacked on Keep the vault available and off the KDF during SSH connects #52, inherits the same.
• Coalesce terminal output and move it off the JSON event path #49 touches core/src/session.rs, but only the imports at the top and the body of create_ssh_session's spawned loop. close_all goes in after list() at the end of the impl, so this should merge cleanly.
• Nothing here touches App.vue, stores/**, TerminalPane.vue or any Cargo.toml. KeyManager.vue now imports stores/vault, but does not modify it.

[greptile-apps]

Greptile Summary

This update strengthens vault-reset containment and clears retained imported key material. It drains SSH and SFTP session reachability before remote teardown, clears local terminals during reset, bounds best-effort cleanup, and resets imported private-key state when the dialog closes, an import fails, the vault locks, or the component unmounts.

Confidence Score: 5/5

Safe to merge with no blocking issues identified.

The previous non-blocking test-coverage concern remains outstanding: the containment test creates empty SSH and SFTP managers, so it does not exercise draining populated remote-session entries. This does not block merging.

_{Reviews (4): Last reviewed commit: "Reset the Add Key form on vault lock onl..." | Re-trigger Greptile}

[greptile-apps]

Exercise populated managers

This containment test creates new SSH and SFTP managers immediately before reset, so both are already empty. Its assertions therefore pass without executing the new non-empty drain and disconnect paths in close_all. Add focused manager tests with controllably populated entries, or a test seam that lets this test populate them, so a teardown regression is caught before release.

This is non-blocking, but the current test adds coverage confidence without covering the behavior it is intended to protect.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Artifacts

Focused empty-manager reproduction script

• Authored script creates a temporary integration test, runs the real public close_all APIs with fresh managers, prints their entry counts and return values, and removes the temporary test; it directly checks that fresh managers exercise zero-item paths.

Pre-PR containment test absence capture

• Executed command against the parent commit finds no reset containment test, establishing the before side of the PR test addition.

Fresh SSH and SFTP manager close_all execution

• Executed authored integration test passes while printing zero SSH and SFTP entries and zero close_all return values; the added test setup cannot execute non-empty manager teardown.

Targeted desktop containment test build attempt

• Attempted execution of the exact vault containment test stops during Tauri dependencies because gdk-3.0 is unavailable; the focused core reproduction still executed the changed manager APIs.

_{Ran code and verified through T-Rex}

[computerbox124]

@Sadykhzadeh address to this, please.

[Sadykhzadeh]

Review: APPROVE WITH NITS

Focused on the auto-lock watcher and the split-out clearKeyMaterial, since those are what changed. Both hold up. Nits are about trade-offs made rather than defects.

What I verified

resetForm is the right callback for the lock direction. KeyManager.vue:62. The two precedents it cites clear fields only — VaultView.vue:53 calls clearSensitiveFormState, VaultUnlockModal.vue:26 calls clearPassphrase, and neither hides anything. resetForm additionally sets showAdd = false, and that extra step earns its keep here: the other two components hold secrets in <input>s that stay in the DOM either way, while this one holds a private key in a plaintext <textarea> that only v-if removes. Clearing the ref without closing the dialog would leave an empty textarea sitting in a locked session; closing it takes the element out of the tree, which is what expect(wrapper.find("#key-private").exists()).toBe(false) in the new test actually pins. Given the vault is locked and no import can proceed anyway, closing costs nothing.

No loop, and the double-clear is by construction. resetForm sets showAdd = false, which under flush: "sync" re-enters the showAdd watcher at :52 inside resetForm, so clearAddForm() runs there and again on the next line. Both are idempotent, and neither clearAddForm nor clearKeyMaterial touches showAdd or vault.unlocked, so it terminates after that one extra call — it cannot re-enter the vault.unlocked watcher at all. If showAdd is already false the inner watcher does not fire. Nothing to fix; noting it because a future edit that made either clear function touch showAdd would turn this into a real cycle.

Clearing in both directions does not break anything reachable. importKey/generateKey in stores/keys.ts never request an unlock, so the common flows cannot flip unlocked false→true mid-dialog on their own; checkStatus is not polled, and Vue's watch does not fire on a same-value write. The one path that reaches it is an SSH connect in another pane opening the shared unlock modal — which is a sibling of the view, so KeyManager stays mounted — while the Add Key dialog happens to be open. See nit 1.

The failed-import behaviour is what the body says. :88-99 calls clearKeyMaterial() only, and alert() runs after the fields are already empty. The test asserts #key-label is still laptop and the Import Key button is still present, so the dialog, the label and the tab all survive. Correct call — a wrong passphrase on an encrypted key was never a reason to make the user rebuild the form.

The corrected body claims check out. No key handler in ui/Dialog.vue or ui/Overlay.vue, and App.vue:46-62 handles Escape only for ui.fullscreenPaneId and the command palette — so Escape genuinely does nothing to this dialog, and the earlier revision's "overlay/Escape dismissal" really was wrong. Leaving Escape out rather than changing ui/Dialog.vue for every dialog in the app is the right scope call for a key-retention fix.

Numbers reproduce, on a checkout I confirmed was at 11f6c56. npm test 313 passed / 37 files, KeyManager.test.ts 5 passed, cargo test --workspace 23/28/21, e2e 68 PASS 0 FAIL against a dev server I fetched KeyManager.vue back from to confirm it was serving this commit, check-comments {"ok":true,"count":0,"files":126}. Non-vacuity: with KeyManager.vue reverted to d8e277f, exactly 3 passed / 2 failed — the vault-lock case and the rejected-import case — with expected '-----BEGIN OPENSSH PRIVATE KEY-----\n…' to be ''. Matches the claim precisely.

Nits

1. The unlock direction closes the dialog too, which the precedents do not. "Add Key" is not gated on vault.unlocked (:164, :235), so the dialog can be open while the vault is locked; if an SSH connect in another pane then unlocks the vault, resetForm fires on false→true and the user loses the dialog and the label they were typing. The security argument for the unguarded shape is sound and I would not add a direction guard for its own sake — but the cost is asymmetric here in a way it is not for VaultView/VaultUnlockModal, because only this callback hides UI. resetForm on lock and clearKeyMaterial on unlock would keep the guarantee with no papercut, at the cost of one if. Either way is defensible; the trade is worth recording rather than reading as "same shape as the other two", which it is only at the watcher and not at the callback.

2. Minor: resetForm is passed straight to watch, so it receives (newValue, oldValue, onCleanup) and ignores them. Harmless and consistent with the precedents, just worth knowing it is a coincidence of the signature rather than a deliberate use of the value.

3. Out of scope, for the record: none of this can wipe the JavaScript string itself — addForm.privateKey = "" drops the reference and leaves the old immutable string for the GC. The body does not claim otherwise, and there is no platform answer; noting it so nobody later reads the five exit paths as a memory guarantee rather than a DOM-and-state one.

No AI attribution anywhere in the diff, comments or commit messages.

[Sadykhzadeh]

APPROVE WITH NITS — reviewed at f23c14b.

Verified locally in a clean detached checkout (git rev-parse HEAD == f23c14bdeeb4653a1574eb573727f5a6a3f23255):

• npm test (Vitest) — 314 passed / 37 files
• desktop/e2e with UI_RECORD=0 — 68 PASS, 0 fail
• node scripts/verify/check-comments.mjs — {"ok":true,"count":0,"files":126}

The guard is correct

I went looking for a false → true edge that could carry key material typed by someone other than the person who authenticated, and I could not construct one. Enumerating every write to unlocked in stores/vault.ts:

• unlock() :127 — passphrase typed in this window, so the authenticator is at the keyboard.
• unlockWithBiometric() :139 — a biometric presented on this device, same property. This is the one I expected to be the weak spot and it is not: it still requires a physically present authenticator, and it does not run behind the user's back (VaultView.vue:61-67 gates the auto-prompt on needsUnlock, and it is a mount-time path).
• checkStatus() :88 — the only genuine "re-sync without authenticating" write. It has exactly two callers: App.vue:65, which runs once at boot when no KeyManager exists, and VaultView.vue:57, which runs on the mount of a component that is a v-else-if sibling of KeyManager (App.vue:92-98). Reaching it therefore unmounts KeyManager, and onUnmounted(clearAddForm) at KeyManager.vue:81 has already emptied the form. There is no polling and no third caller.
• initialize() :115 — the reset-then-initialize path. publishDestroyedVaultState() writes false at :27, but I do not think that write is what saves this case, because if the vault was already locked the value does not change and the watcher does not fire. What actually contains it is the same structural fact: resetVault/resetMode exist only in VaultView.vue, again a v-else-if sibling, so KeyManager is unmounted and cleared before reset is reachable at all.
• Multi-window — not reachable. tauri.conf.json declares a single window and there is no WebviewWindow construction anywhere in desktop/src or desktop/src-tauri/src.

I also checked the flush: "sync" claim rather than taking it on trust, since the comment leans on it and nothing in the suite pins it. Throwaway probe in a scratch checkout, setting vault.unlocked = false and vault.unlocked = true back to back with no await between them: the form is cleared, including the label. The sync watcher does observe the transition through false and clear on it even though the value ends the tick at true. That half of the reasoning is solid.

Non-vacuity

Both directions check out.

The new test fails at the parent 11f6c56, where the both-direction watcher closes the dialog: Unable to get #key-private within: … <teleport-stub to="body"><!--v-if--></teleport-stub>.

The lock-direction containment test is still load-bearing. I neutered the watcher body to void unlocked; at f23c14b and re-ran it: AssertionError: expected '-----BEGIN OPENSSH PRIVATE KEY-----\n…' to be ''. The guard narrowed the watcher without hollowing out the test that justifies it.

Nit — the stated justification is stronger than what holds

KeyManager.vue:65-66 and the test comment at KeyManager.test.ts:107-111 both assert that reaching the unlock edge with key material in the form means "the person who typed it is the person who just authenticated" / "the same person typed it and then authenticated". That is not quite provable. Counter-case: A opens the Keys view while the vault is locked, types key material, and walks away; B walks up and unlocks. Both halves of the claim fail — A typed it, B authenticated, and no lock ever fell between them because the vault was already locked the whole time.

The conclusion is still right, for a different reason. A locked vault does not hide this view. The unlock modal is only ever opened by ui.requestVaultUnlock(), whose only callers are TerminalPane.vue:239 and :277 during a connect — nothing opens it on lock. So throughout that entire locked interval the private key is sitting in an unobstructed plaintext textarea that B could read without unlocking anything. The false → true edge discloses nothing B did not already have, which is what actually makes resetForm there pure loss.

I would reword to the weaker claim, roughly: material present at a false → true edge was necessarily typed during the current locked interval, throughout which this view renders it in plaintext with no vault gate in front of it — so the unlock adds no exposure, while resetForm would cost the user a half-typed import. Same decision, and it does not rest on an identity assumption the UI cannot enforce.

Worth fixing because it is the whole security argument for the change, and the next person to touch this watcher will read that comment as the spec.

Minor

The same-tick lock-then-unlock behaviour is load-bearing enough to deserve a test of its own — it is the sentence in the comment doing the most work, and it currently rests on a Vue scheduling guarantee with nothing pinning it. My probe above is three lines on top of the existing openImportTabWithKeyMaterial helper if you want to fold it in.

Not blocking. The narrowing itself is right and the containment it preserves is properly tested.

[computerbox124]

@greptile-apps, can you review this PR and report the found issues?

[Sadykhzadeh]

@computerbox124 Ready.

Greptile 5/5. Two distinct gaps closed, and the second one is the reason this PR went through three rounds.

Vault reset left every session alive. VaultResetForm.vue tells the user the reset "permanently deletes all stored SSH keys and credentials", but secure_reset_vault touched only the vault, passphrase and auth generation — every SSH/SFTP session and local PTY kept running and stayed writable via session_write. A user resetting because they suspect compromise did not get the containment the copy implies. Now closed after the destructive boundary, with the result discarded under a budget so a stuck connection can neither fail nor stall a completed reset.

One trap avoided: the existing SessionManager::close holds the map lock across handle.disconnect().await, so calling it from the reset would have deadlocked. close_all drains under the lock and releases before any network work.

KeyManager retained an imported private key — and the first fix claimed to cover auto-lock but did not. The watcher was on showAdd, and auto-lock never touches showAdd: useAutoLock.ts:36 calls vault.lock() only, and App.vue:105 mounts VaultUnlockModal as a sibling of the view, so KeyManager stays mounted and onUnmounted never fires. Reproduced: after vault.unlocked flips false, the plaintext private key was still rendered in the DOM textarea.

Fixed with watch(() => vault.unlocked, resetForm, { flush: "sync" }), matching VaultView.vue:53 and VaultUnlockModal.vue:26, then guarded to the lock direction only — on unlock, the only way key material exists is if it was typed during the locked session, so the typist is the person who just authenticated and clearing costs them their work for no security gain.

Verified live through the real auto-lock path: before-lock key-in-dom: true → after-lock key-in-dom: false.

Also declined, deliberately: masking the textarea. A <textarea> has no type="password", masking breaks checking a pasted key, and it fixes the wrong half — visibility while looking at it is fine, retention afterwards is the bug.

314 vitest / 68 e2e pass; every new test verified to fail against the unfixed component.

[Sadykhzadeh]

For the merge queue: contain_open_sessions here clears state.passphrase, which #59 replaces with state.vault_session.clear().await, and it is generic over &Mutex<HashMap<String, T>>, which #31 replaces with LocalTerminals. The first is a textual conflict in vault_commands.rs (finding F); the second is silent — git merges it and it does not compile (finding L). LocalTerminals needs a clear() that drops live terminals and reservations, since a reservation left behind holds a slot for a terminal nobody wants any more. Both written up in #71.

Nothing to change here. @computerbox124 всё готово, можно смотреть — 5/5, merges cleanly into main.