Docs: the 2026-10-02 audit reports, the full sweep and the Android design audit, with their screenshots

REPORT.md is the full-app sweep and what became of each finding. ANDROID-DESIGN.md is a walk through
every phone screen on the emulator, from a cleared install to SFTP, with findings ranked by severity
and a screenshot for each. The home-folder listing in the sweep's transfers screenshot is blurred.

audit-2026-10-02/REPORT.md

@@ -0,0 +1,147 @@
+# DodoSSH full sweep — 2026-10-02
+
+## Fix status (same day)
+
+Every fix below has a regression test. Where marked ✔✔, the test was also checked to fail with the fix taken out.
+
+| Item | Fix |
+|---|---|
+| C1 ✔✔ | `LocalDirectory.ChildPath` cleans remote names and requires the resolved parent to be the target folder. It is used by every download path and by EDIT. `LocalEditor` opens a file that would run (PATHEXT plus a known list, and macOS launchers) with the type's own `edit` verb or with Notepad / `open -t`. On Linux, opening a `.desktop` file is refused. |
+| C2 ✔✔ | Pinned key types are offered first. A key under an algorithm with no pin of its own is accepted only if its fingerprint matches one of the endpoint's pins (the same RSA key under `rsa-sha2-256` vs `-512`); otherwise it is a hard mismatch. Tested against the real OpenSSH container. |
+| C3 ✔✔ | Complete, Revise and Park take the `PendingOperation` and act only while the row still holds that operation id. A later edit is rebased onto the version that was just accepted, so it pushes without a conflict. |
+| C4 | Pull leaves out change entries the item has moved past, so a revision is never paired with newer ciphertext. This also stops re-sending an item's payload once per historical edit. |
+| C5 | Deleting a team's last vault archives the team only when the owner does it. `TeamService.ArchiveAsync` now checks ownership itself. |
+| C6 | `DeviceKeyNames`: the default profile keeps `DodoSSH.DeviceKey.v1`, and every other profile gets its own name. Sign-out deletes only its own key. A profile still sealed under the shared key keeps reading it. The same applies to the macOS keychain tag. |
+| H1–H3 | The data plane reassembles messages up to 16 MiB. A frame that throws stays with its own session. Input writes are serialized per pump, so a slow write is never overtaken. |
+| H4 | The host and bucket the user chose are remembered across reloads. A typed password is only sent to the host it was typed for. |
+| H5 | Selecting a vault reads its people even while the screen is busy. A generation counter discards answers for an older selection. |
+| H6 / U9 | Locking or signing out leaves Settings, and the Members panel closes with it. Escape closes the Members panel first. |
+| H7 ✔✔ | The tracked user is reloaded after the out-of-band last-seen update. |
+| H8 | `sync_state.pulled_types` (with a client migration) restarts the pull once when the synced type list changes. |
+| H9 | `ActiveAccountRequirement` on both policies. A suspended account gets 403 `account-not-active`. |
+| H10 | `DevicePinWindow` palette keys fixed, and it now uses `InitializeComponent`. `Load` never assigned the named fields, so the window could never have opened. Covered by a new layout test. |
+| H11 ✔✔ | `IRemoteFileStore.CanWriteFromAnOffset`: false for buckets, so a retried upload starts over. |
+| H12 | `ImportOutcome` carries refused hosts and the reason a batch stopped early, including a busy keychain. Both import screens say so. |
+| U1–U4 | The host card's sync badge moved to the address line, so names show in full. Tag chips wrap and trim. Keychain names trim inside their column, with a tooltip. The Security caption wraps. |
+
+Second round, on branch `claude/audit-followups`:
+
+| Item | Fix |
+|---|---|
+| U5 ✔✔ | The closed vault's card takes the keyboard on launch and on each new card (server address, enrolment, passphrase). Enter signs in and enrols. |
+| U6 ✔✔ | The host-key, delete and sign-out questions give the keyboard to their harmless answer (CANCEL / CLOSE, never TRUST or DELETE). Escape says no to each, and closes the hosts drawer. Tab stays inside the host-key card. |
+| U7 ✔✔ | Arrow-key selection on the Hosts board is written back to the vault, from the list that has the keyboard only. An arrow on the board itself enters the cards. Enter on a card connects. |
+| U8 | `DeviceUnlockWording`: Windows Hello, Touch ID, a TPM-checked device PIN or a fingerprint, according to the platform. |
+| U10 | Session tab labels are capped at 200 px and trimmed, with the full label in a tooltip. |
+| U11 | The keychain's VAULT column is an Auto column in a shared size group, so it takes no width when no row names a vault. The Tags page's always-empty column is gone. The snippet buttons appear only with a snippet chosen. |
+| U12 ✔✔ | `TerminalTabViewModel.EndedAt`: the status-bar timer stops when the shell ends. |
+| P1 (part) ✔✔ | A sync pass that moved nothing no longer reloads (and decrypts) the whole keychain. It restrings the ago-texts instead. Failed vaults still reload. The Clear-plus-Add rebuild itself is unchanged. |
+| P3 | `ProbedDeviceKeyStore`: the TPM probe runs once per process, on the thread pool. |
+| P5 | A pulled page is mirrored in one write, and the outbox is read once per page. Changes with a local edit waiting still go one at a time, in order. |
+| P6 ✔✔ | `OpenShellAsync` makes its three round trips on the thread pool, honours cancellation, and closes a shell that arrives after the caller gave up. |
+| Transfers ✔✔ | Lost wake-up fixed with a `pumping` flag cleared under the same lock that finds the queue empty (a stress test failed 2 of 2 runs before the fix). Cancel as a transfer finishes no longer throws. A drop or download refused while busy says so. |
+| EF retry ✔✔ | `FreshAttempts.ExecuteFreshAsync`: a retried write transaction clears the tracker and reloads what it carries in. Before this, a lost commit made a team hand-over answer 204 with nothing changed. Tested with an interceptor that fails one commit. |
+| Rejected delete ✔✔ | A delete of a create that never landed is complete when the server says it has no such item. A create that did land still behaves as before: the delete gives way, and the user is told. |
+| Team archive race | Archive and team-vault creation take a per-team advisory lock. Archive re-counts the team's vaults, and creation re-checks the team is live. No race test. |
+| Relay revocation ✔✔ | `RelayAccessWatch` re-checks a user's open relay sessions when their access changes and ends lost ones with 4403 / `AccessRevoked`. |
+| Shell | `ToggleVaultAsync` catches a failure and redraws. `CrashLog` writes uncaught exceptions to `crash.log` in the profile (the process still exits). A lock pressed during an unlock now wins (✔✔). |
+| Window chrome | `CaptionPresses` decides a double-click on the press, before `BeginMoveDrag`. Only a headless test covers it; it is not verified on a real Windows window. |
+
+| P1 (rest) ✔✔ | `ReplaceableCollection.ReplaceAll`: the host, visible-host, section and bucket lists are swapped in whole and announced once, and each transfers picker refreshes only from its own list. |
+| P2 (most) | The transfer queue and import lists are virtualised (✔✔: 2,000 of 2,000 rows were built before). The find box hands its text to the vault after a 150 ms pause in typing, and the view model still filters synchronously. |
+
+| P2 (board) ✔✔ | `VirtualizingWrapPanel` builds only the host cards near the view. Measured headless, after warm-up: first layout with 300 hosts went from about 3 s to about 0.5 s, and with 1,000 from 8.3 s to 0.2 s. Clearing a filter went from 1–8 s to about 0.1–0.2 s. Arrow keys, End, the band and selection still work. |
+
+| P4 ✔✔ | **Server:** `TombstoneCollector` runs every 6 h. It deletes tombstones past the 90-day retention together with their change entries, and compacts each item's superseded entries down to its newest. It then raises the vault's `SyncHorizon`, and a cursor from before it is refused. A cursor now carries the horizon it was issued under. Without that, the read from the beginning that a refused client starts was refused on its own second page and looped for ever on any vault larger than a page (✔✔: found by a client test, then reproduced in an API test). **Client** ("resync and prune", as chosen): a read from the beginning marks every item it reads, keeping the mark in the sync state because the read can span passes. When it finishes, it removes unmarked items of the synced types. It keeps items with an unsent change, and items pushed during the read. Such an edit is then sent as a create, as before, but now records a `RecreatedAfterCollection` conflict and is named in the sync message. The removed items are named too. New client cache migration `AddSweepMarks`. |
+
+Not done, and why:
+- **Live-run observations** (production as the default server URL, Keycloak on host port 18080): product decisions, not bugs.
+
+Pre-existing flakiness: on untouched HEAD, 3 of 8 runs of `ShellFlowTests` each had one test fail: `TheCountsAPhoneShows_FollowEveryForwardsState`, `ASpelunkHost_IsDialledThroughTheTunnel_AndStillAsItself` and `ReconnectingAHostThatWantsAPassword_AsksForOneRatherThanDiallingWithout`. These races are in the tests and predate this work.
+
+How it was done:
+- The desktop client ran on Windows with an isolated profile against a local stack: Postgres, Keycloak and the API on :5333.
+- The real MainWindow was rendered headless through every screen and settings page, 39 captures at 1081×583 and 1440×900.
+- Four parallel code audits covered: the Shell view models; SSH/terminal/transfer; the server and sync; the desktop views and platform code.
+- The findings marked **✔ verified** were re-read in code by me, or seen in a screenshot. The rest come from the audits; each one was traced, but I did not check them again.
+
+The live session could not get past OIDC sign-in. The default browser is Edge, which I can't type into, so the terminal, SFTP and host-key flows were reviewed in code only.
+
+---
+
+## Critical — data loss, security
+
+| # | Finding | Where |
+|---|---|---|
+| C1 | **Path traversal on download / open-to-edit.** Remote names go straight into `Path.Combine(LocalPath, row.Name)`. SFTP only drops `.` and `..`, so on Windows a name with backslashes (`..\..\AppData\…\Startup\x.bat`) or a rooted name escapes the folder. S3 keys `../..` do the same on every OS. EDIT then opens the file with `UseShellExecute`, so a `.bat`/`.hta` *runs*. The Android head sanitises names; the desktop doesn't. ✔ verified | `TransfersViewModel.cs:2682,2698,2780,2784,3165`; `SshNetSftpSession.cs:105`; `ObjectKeys.cs:50`; `App.axaml.cs:380` |
+| C2 | **Host-key pin bypass by changing the key algorithm.** Pins are looked up per algorithm. A MITM that offers ecdsa or rsa to a host pinned for ed25519 gets the "never seen before" first-use prompt instead of the mismatch hard block. ✔ verified | `SshNetConnectionFactory.cs:261-272` |
+| C3 | **The outbox loses an edit made while a push is in flight.** `CompleteAsync` deletes the row by `Sequence` only, but `QueueAsync` folds a newer edit into that same row with a new `OperationId`. When the older push is acknowledged, the newer edit is deleted silently. ✔ verified | `OutboxStore.cs:239-250` vs `:71-97`; `SyncEngine.cs:554` |
+| C4 | **Pull pairs historical revision numbers with the current ciphertext.** `Describe` sends `Version: change.Revision`, but the payload comes from the item's current row. The AAD binds the version, so the payload fails to decrypt. A concurrent offline edit gets parked as "Undecryptable". ✔ verified | `SyncService.cs:565,592` |
+| C5 | **A team admin can archive the team (meant to be owner-only).** If an admin deletes the last vault, `ArchiveAsync` runs with no role check and revokes every membership, the owner's included. | `VaultGrantEndpoints.cs:182,196`; `TeamService.cs:242-289` |
+| C6 | **The shared TPM key name breaks other profiles.** `WindowsDeviceKeyStore` uses one CNG key name (`DodoSSH.DeviceKey.v1`) for the release and nightly profiles. Sign-out in one (`ForgetAsync`) deletes the key, and the other profile's device unlock stops working. This contradicts `ClientPaths.ForChannel`, which says the two installs are kept apart. ✔ verified | `WindowsDeviceKeyStore.cs:127,233-246` |
+
+## High — broken behaviour
+
+| # | Finding | Where |
+|---|---|---|
+| H1 | **Terminal input >64 KiB (a large paste or copy) is lost.** On a frame that doesn't fit the buffer, the loop `continue`s, and the tail is then dispatched as if it were a frame. ✔ verified | `TerminalDataPlane.cs:465-472` |
+| H2 | **Keystrokes can race.** Input frames are written to `ShellStream` without awaiting and without a lock, and the base `FlushAsync` runs on the pool. A snippet RUN (text, then `\r`) over about 4 KiB can execute truncated. | `TerminalDataPlane.cs:505`; `SshNetConnectionFactory.cs:554-558` |
+| H3 | **One resize on a closing shell tears down the renderer socket for every tab** (`ObjectDisposedException` escapes the receive loop). | `TerminalDataPlane.cs:516-520` |
+| H4 | **The SFTP host picker resets to the first host on every sync pass**, about once a minute. Selection is lost on `Hosts.Clear()`, so CONNECT can dial host[0] with the password typed for host X. The same Clear-then-Add pattern resets the Known Hosts and Snippets selection and closes the snippet delete card. | `TransfersViewModel.cs:3905,3923-3950`; `KnownHostsViewModel.cs:264`; `SnippetsViewModel.cs:689` |
+| H5 | **The Vaults Members panel can show vault A's members under vault B**, and SHARE / REVOKE then act on that mix. | `VaultsViewModel.cs:1646,1698-1765` |
+| H6 | **Locking or signing out from Settings leaves a window with no title bar** (no close, minimize or drag). Unlock then returns to Settings. ✔ verified by screenshot | `MainWindowViewModel.cs:3254,3400`; see `locked-from-settings-no-titlebar.png` |
+| H7 | **Enrollment returns 500 if last-seen is more than an hour old.** `ExecuteUpdate` changes `xmin` under a tracked entity, and the `SaveChanges` that follows throws a concurrency exception. | `CurrentUserContext.cs:92,120`; `EnrollmentService.cs:177` |
+| H8 | **A new synced type never back-fills.** A filtered pull moves the cursor past types the client didn't request, so after an upgrade adds a type (SpelunkKey, ObjectStore), older changes of that type never arrive. | `SyncService.cs:50,69`; `SyncEngine.cs:145` |
+| H9 | **Suspended users keep full access.** `UserStatus` is checked nowhere on the request path. | `CurrentUserContext.cs:126` |
+| H10 | **`DevicePinWindow` references `Surface` and `MutedText`, which don't exist**, so Linux TPM-PIN register/unlock should throw on open. ✔ verified | `DevicePinWindow.axaml:7,35` |
+| H11 | **A failed S3 upload commits a truncated part, and every retry then fails.** | `S3UploadStream.cs:126-146` |
+| H12 | **Import screens report success when the vault skipped or failed the import** (busy, or a mid-batch exception). | `ImportViewModel.cs:374`; `HetznerImportViewModel.cs:561` |
+
+## UI issues seen in the running app
+
+| # | Finding | Where |
+|---|---|---|
+| U1 | **Host card labels are cut to 3-4 characters even at 1440px.** "bastion" shows as "bast…", and k8s-worker-a / -b both show as "k8s…", as do web-prod-01 / -02. The label is the primary identifier, and the "not synced" badge takes most of its width. ✔ `hosts-truncated-labels.png` | `HostsScreen.axaml` card template |
+| U2 | **Tag chips overflow the host card edge.** The WrapPanel sits inside a horizontal StackPanel, so it never wraps. ✔ | `HostsScreen.axaml:737-774` |
+| U3 | **Keychain table: long names overprint the TYPE / USED BY columns.** `TextTrimming` has no effect inside a horizontal StackPanel. The same pattern appears in Logs, SessionSidebar, Snippets, NavRail and Transfers. ✔ `keychain-overlap.png` | `KeychainScreen.axaml:294` et al. |
+| U4 | **Security page: the "End-to-end encrypted" caption runs past the card edge.** ✔ `security-text-clipped.png` | `SettingsSecurityPage.axaml:49-60` |
+| U5 | **Enter does nothing in the server URL box** (verified live). The same applies to the enrollment passphrase boxes. The passphrase box is not focused on launch. | `MainWindow.axaml:385` |
+| U6 | **Overlay cards (host key, delete, sign-out, host editor) have no Enter/Escape and don't take focus**, and Tab walks the page behind them. | `HostKeyCard.axaml` etc. |
+| U7 | **Hosts board: arrow-key selection isn't written back to the view model**, so the drawer's Connect / Delete act on the previously clicked host. There is no Enter-to-connect. | `HostsScreen.axaml:573` |
+| U8 | **Windows-only wording on macOS/Linux:** "USE WINDOWS HELLO", "Waiting for Windows…". | `UnlockCard.axaml:51`; `MainWindowViewModel.GestureWait` |
+| U9 | **Escape over the Members panel leaves Settings and leaves the panel open** for the next visit. | `MainWindow.axaml.cs:242` |
+| U10 | **Session tab labels are unbounded.** One long label pushes "+" off the strip. | `SessionTabRow.axaml:89` |
+| U11 | **A VAULT column is drawn and empty** with a single vault (Keychain, Tags). The snippets list footer shows Edit / Move / Delete with nothing selected. | screenshots |
+| U12 | **The status-bar session timer keeps counting after a shell has ended.** | `MainWindowViewModel.cs:4979` |
+
+## Performance
+
+| # | Finding | Where |
+|---|---|---|
+| P1 | **Every sync pass, about once a minute, decrypts the entire keychain even when nothing changed**, then rebuilds each list with Clear + Add. That is O(n²) notification fan-out: with 300 hosts, roughly 45k collection notifications per pass. | `VaultViewModel.cs:6375,4709`; `MainWindowViewModel.cs:4549` |
+| P2 | **The Hosts board is not virtualised and is rebuilt on every filter keystroke.** The transfer queue (one row per file in a folder download) and the import lists are not virtualised either. | `HostsScreen.axaml:448-576`; `TransfersScreen.axaml:1193` |
+| P3 | **The TPM probe (create and delete an RSA-2048 key) runs synchronously on the UI thread** at startup and after every unlock. | `WindowsDeviceKeyStore.cs:152`; `MainWindowViewModel.cs:2594,2970` |
+| P4 | **Pull re-sends the full current payload once per historical change.** There is no change-log compaction and no tombstone GC, even though ADR 0003 promises 90-day GC. | `SyncService.cs:45-56,565` |
+| P5 | **The client applies each pulled change in 2 or more separate SQLite transactions.** | `SyncEngine.cs:157-169,303` |
+| P6 | **`OpenShellAsync` does blocking network round trips wrapped in `Task.FromResult`** and ignores cancellation. | `SshNetConnectionFactory.cs:401-418` |
+
+## Lower / plausible
+
+- **Transfers:**
+  - A wake-up can be lost, leaving a transfer queued forever (`FileTransferQueue.cs:452-505`).
+  - Cancel at the moment a transfer finishes can throw `ObjectDisposedException`.
+  - Drops and downloads are ignored silently while busy (`TransfersViewModel.cs:2272`).
+- **Sync and server:**
+  - The EF retry strategy re-runs a push with stale tracked state (`Persistence.cs:38`).
+  - Deleting a create that never landed leaves a permanent "Rejected" conflict.
+  - A team can be archived while a vault is being created in it.
+  - Relay sessions survive removal from the team, for up to 12 hours.
+- **Shell:**
+  - Lock can race an unlock that is still loading.
+  - `ToggleVaultAsync` has no try/catch, and there is no global `UnhandledException` handler.
+- **Window chrome:** double-click to maximise may be swallowed by `BeginMoveDrag`.
+
+## Observations from the live run
+
+- **The first-launch default is the production server, and sign-in completes without a credential prompt.** `DefaultServerUrl = "https://ssh.dodotech.cloud"` is pre-filled. With a live browser SSO session, one press of SIGN IN on a brand-new profile completed sign-in with no password typed, and wrote the account's wrapped key and vault list into the new profile. For a self-hosted product, an empty field with a placeholder is the safer default.
+- **The local dev compose hard-codes host port 18080 for Keycloak**, which collides with other projects on this machine.