Code review, October 2026¶
Four reviewers (rendering/model, file writes, diagnostics, app layer) checked Keybund against the
OpenSSH and Git sources and reproduced every finding with the real ssh/git in a throw-away
HOME. This page records what was found, what was fixed (commits on main from 7074ed3 to
57e70a3), and what is still open – so the work can be picked up by anyone.
Fixed¶
| Area | Finding | Fix |
|---|---|---|
| render | A ' in a key path made ssh refuse the whole config for every host (argv_split opens a quote mid-token). |
Paths with whitespace or ' are double-quoted; \ in key paths is rejected. |
| store | apply rendered a hand-edited profiles.toml without validation (alias = "a b", id = "../evil"). |
store::validate_in runs in apply::plan; nested folders compared as real paths. |
| store | Host patterns are case-sensitive in ssh; MyNAS typed, ssh mynas missed the block. |
Alias and hostname lower-cased on save (app, CLI). |
| store | Same account twice in one key accepted; .. in folders never matched (git compares real paths); signing e-mail with spaces broke allowed_signers. |
New error kind duplicate_host; ./.. removed lexically and rejected; e-mail pattern chars rejected when signing. |
| apply | No cross-process lock: app + CLI lost each other's profiles and deleted gitconfigs as stale (10 parallel new → 6/20 profiles). |
~/.config/keybund/.lock (File::lock, Rust 1.89) around load → save → apply in CLI and app. |
| uninstall | Fresh install → uninstall left backups holding only Keybund's line; --restore-backups wrote the include back; empty 0-byte files stayed. |
No backup when a write removes Keybund; empty user file is deleted; restored backup is consumed; --purge also removes last-tests.json and known_hosts.d/*. |
| import | Header comment block (# managed by chezmoi / # DO NOT EDIT) was removed with the first block and used as its name. |
Only one comment line directly above Host belongs to a block. |
| apply | Include ~/.ssh/config.d/keybund.conf ~/.ssh/work.conf lost the second path. |
The line is rewritten without Keybund's path only. |
| fsutil | Dangling dotfiles symlink → apply failed. | Link resolved by hand, target created. |
| archive | .pub collision overwrote silently; agent kept the key; save failure left files moved. |
.pub checked; ssh-add -d best effort; rollback on failed save. |
| keyinstall | Dedupe by whole line (same key, other comment → appended again); no restorecon. |
Dedupe by type blob; restorecon when present. |
| keygen / hostkeys | Comment with a newline added lines to .pub; user's known_hosts chmod'ed to 600. |
Comment validated (field_comment); only new files get 0600. |
| probe | App could not ask a passphrase (no SSH_ASKPASS), and blocked on /dev/tty when started from a terminal; no timeout. |
probe_detached/install_detached: askpass forced, else setsid; 60 s timeout (process::output_timeout). |
| probe | Wrong passphrase and cancelled dialog reported as "could not ask – run ssh-add"; forced command (command=, git-shell) reported as failure. |
Counted from read_passphrase lines in the -v log; Authenticated to + non-zero exit = login (probe_loggedIn_noCommand). |
| doctor | Did not check what ssh refuses: ~/.ssh/config/keybund.conf writable by group/others or owned by someone else ("Bad owner or permissions"). Fix::Permissions rewrote a 0400 key to 0600. |
New check config-perms with targeted fix; fix only touches what is wrong. |
| doctor | User's Include ~/.ssh/config.d/*.conf reported as "Include missing" and a second line added. |
Glob includes recognised (includes_keybund_conf), never removed. |
| doctor | Agent from .bashrc → "no agent" although the terminal has one. |
Login shell's SSH_AUTH_SOCK checked: "ssh-agent only runs in the terminal". |
| which | git config --show-origin C-quoted non-ASCII homes (/home/j\303\266rg); .git/config shown without repo; no -- before ssh -G host; key paths with spaces cut in steps. |
git config -z, origin anchored at the repo, --, key_path() parser. |
| CLI | new/edit/import wrote the user's files without --yes off a terminal (docs said otherwise); import --all died on one missing key file; new left folders behind when the key was refused. |
confirm_and_apply refuses without tty and --yes; missing keys skipped with a note; folders created after the key. |
| app (Rust) | create_project left a generated key when save failed; trust_host_key held the write lock during ssh-keyscan; settings commands ran on the main thread; German text in English error. |
Rollback; lock dropped; async + blocking; error_stalePreview key. |
| frontend | Home page refetch loop on a load_profiles error (448 calls/1.5 s, spinner forever, no error shown). |
retryOnMount: false; error alerts on home, sidebar, settings cards. |
| frontend | Second concurrent host-key prompt dropped the first; language switch unmounted an open prompt. | Queue in HostKeyPrompt; prompt outside the key={lang} fragment. |
| frontend | Double click added a host twice; ensureHostKey swallowed every error. |
Submit disabled while saving; only tool_missing ignored. |
| mock | Mock accepted what the core refuses (validation, folder normalisation, latency, apply errors). | Mock validates like store::validate; test switches configError, applyError, hostKeyError. |
| tests | No end-to-end test through the binary with a real server (ssh reads ~/.ssh/config from the passwd entry, not $HOME). |
Sandbox::ssh_shim(); a_server_from_first_contact_to_git_over_ssh, parallel_runs_do_not_lose_profiles. |
Open – the work list¶
All eight items are done (commits b2755fd … on main, each with a test). Nothing here lost data
or broke a login; all of it was correctness at the edges. Each item
names the file and the test to write (a test that fails without the fix, next to the existing ones).
- ✅ Done. Known-hosts file as ssh sees it –
crates/core/src/hostkeys.rsfiles()/is_known: askssh -G <host>foruserknownhostsfileinstead of assuming~/.ssh/known_hosts, so a user's ownUserKnownHostsFileunderHost *andknown_hosts_per_profile = falseare honoured. Test incrates/core/tests/integration.rs: a config withUserKnownHostsFile ~/.ssh/kh2underHost *→trustwrites there andis_knownfinds it. - ✅ Done. Last test per host –
crates/core/src/history.rs,app/src-tauri/src/commands.rs:187,crates/cli/src/commands.rs:481: key the record by(profile, host), show it on the host row (app/src/routes/profile.tsx), keep the per-key summary for sidebar/home. ChangesLastTest→ runjust bindings, update the mock (app/src/lib/mock/backend.ts), Vitest and the CLI test. - ✅ Done.
git_include_presentby path, not text –crates/core/src/apply.rs: expand~on both sides before comparing, so an absolute hand-written path or a differingXDG_CONFIG_HOMEis recognised and no second[include]is appended. Unit test next togit_include_is_appended_once. - ✅ Done.
whichon a host with several listed keys –crates/core/src/which.rswhich_host: when more than one existing identity file is listed andidentitiesonlyis notyes, say so (which_triesKeys) even if the first one belongs to a profile. - ✅ Done. CRLF configs –
apply::ensure_ssh_include/uninstall::without_git_include: keep\r\nand leading blank lines of the user's file. Unit test with a CRLF~/.ssh/config. - ✅ Done.
clone.rsedge cases – acceptssh://[2001:db8::1]:22/path; rejectport: 0and relative key paths instore::check_values. Unit tests inclone.rsandstore.rs. - ✅ Done.
keybund edit–crates/cli/src/commands.rs~line 1010: create folders beforestore::save, or report the mkdir error without claiming the configs are stale. - ✅ Done. Docs –
docs/generated-files.md: say that a linked worktree inside a folder whose main repository is outside gets no key (git's rule); already noted, keep when editing.
Not fixable from here: the AppImage environment reaching ssh-askpass (test on a real desktop),
the current Bitbucket greeting text (needs network; both known variants are accepted).
Handover prompt¶
Read
docs/review-2026-10.mdin the keybund repository (also at https://firsttris.github.io/keybund/review-2026-10/). The "Fixed" table is done and pushed; work through "Open – the work list" top to bottom, one commit per item, each with a test that fails without the fix, onmain, in the order given. FollowCLAUDE.md(checks before every push:just lint,just test,mkdocs build --strictwhen docs change;just bindingsafter a Tauri type change; update the docs the change touches). Answer in German.
Still open, by choice¶
archive_profilein the app: a failingapply_configafterwards leaves configs referencing the old path until "Review & rewrite" (shown as error toast + pending footer, not silent).- A language switch remounts the router, so a row that started a test cannot show the in-flight result (the test completes; backend state is right).
- AppImage environment (GTK_PATH etc.) reaching
ssh-askpasswas not testable here. - Bitbucket greeting text could not be verified without network.
Verified correct¶
Generated Host blocks (alias + IP, user, port, IdentitiesOnly, UserKnownHostsFile with the
global file, UpdateHostKeys), no block for providers without alias, User git for aliased
providers; includeIf gitdir: with trailing slash, symlinks, worktrees, bare repos, folder names
with spaces; include order in ~/.gitconfig (last wins), ~/.config/git/config loses to
~/.gitconfig; core.sshCommand quoting through sh; signing end to end (verify-commit); atomic
writes through symlinks with modes kept; backups before the first write; StalePlan checks;
host-key fingerprints TOCTOU-safe (re-scan on trust); pinned provider keys match the published
fingerprints; ssh -v strings match current OpenSSH; GIT_SSH correctly not watched
(core.sshCommand beats it); Tauri capabilities minimal, CSP without scripts, open_url https only.