diff --git a/audit-2026-10-02/ANDROID-DESIGN.md b/audit-2026-10-02/ANDROID-DESIGN.md new file mode 100644 index 0000000..541fb86 --- /dev/null +++ b/audit-2026-10-02/ANDROID-DESIGN.md @@ -0,0 +1,328 @@ +# Android design audit — 2026-10-02 + +Run on the emulator (`dodossh_api36`, Pixel 5 profile, 1080×2340 @ 440 dpi, Android 16). It used a +standalone **Debug** APK from `main` at `858e362`, against a local API, Keycloak and a throwaway `sshd`. +The walk went from a cleared install through sign-in, enrolment, recovery code, unlock, adding a host, +connecting, host-key trust, terminal, and every Settings sub-page. + +Screenshots are in [`android/`](android/), downscaled. The recovery-code screen can't be captured, +because `FLAG_SECURE` blocks it, so it was reviewed from its markup and the accessibility tree. + +**How to read the evidence.** "a11y tree" means `uiautomator dump`, which reads what TalkBack and any +other accessibility service see. Times and hangs come from a Debug build on an emulator (JIT, no AOT), +so treat any performance note as "confirm on Release". + +--- + +## What works well + +- **Connection progress card** ([11](android/11-connecting.png)): five named steps, the current one lit, + with *close* and *logs* available while it's in progress. +- **Host-key sheet** ([13](android/13-host-key.png)): "UNKNOWN HOST KEY", first contact, the algorithm, + the full SHA256 fingerprint, and an instruction to compare. **TRUST AND CONNECT** and **CANCEL** are + both full width. +- **Recovery-code screen**: + - no back and no skip; + - **CONTINUE** stays disabled until the tick; + - a full-width **COPY CODE**; + - capture is blocked. +- **Terminal** ([14](android/14-terminal.png)): + - resizes above the keyboard; + - the two-row accessory bar (esc, tab, ctrl, `| - ~ /`, arrows, A−/A+) stays reachable; + - the open-sessions strip and the tab strip are clear. +- **Busy states**: enrolment disables its fields and button and explains the wait ("deliberately takes a + moment"). +- **Touch targets** measure 44 dp or more almost everywhere: checkboxes, buttons, rows. +- **Back** closes sheets. The soft keyboard's ✓ submits single-field forms (unlock, connect). +- **Copy is honest and specific**, e.g. "works with no network — the keychain decrypts on this phone", + and the Vaults explanation. + +--- + +## Critical / high + +### H1 · Password fields expose their plaintext to accessibility services + +- **What:** the vault passphrase field ([05](android/05-unlock-prefilled.png)) and the SSH password field + ([10](android/10-connect-sheet.png)) both report `password="false"` in the a11y tree. Their `text` + attribute is the secret itself, in clear. +- **Evidence:** the a11y tree on both screens; the values matched exactly what was typed (redacted here). +- **Why it matters:** any installed app holding an accessibility service can read every passphrase and + host password typed into DodoSSH. That's a common Android malware technique. TalkBack would also read + the passphrase aloud character by character. +- **Where:** `TextBox.secret` sets `PasswordChar` and `ContentType=Password` + (`Theme/Phone.axaml:583`). That's correct for the input method, but Avalonia's Android automation peer + ignores it. +- **Fix:** give secret boxes an automation peer that reports `IsPassword` and returns no value. At + minimum, override the exposed text on `TextBox.secret`. File it upstream with Avalonia as well. Check + the desktop's UIA peer for the same leak. + +### H2 · The passphrase survives enrolment and pre-fills the unlock box + +- **What:** right after the recovery code, the unlock screen already holds the passphrase (16 dots; + [05](android/05-unlock-prefilled.png)) beside "Unlock with the passphrase you just chose." +- **Why:** `ConfirmRecoveryCode` clears `RecoveryCode` from memory on purpose ("keeping it in a view + model … would undo that"), but `Passphrase` is never cleared after enrolment. Combined with H1, it's + readable from the a11y tree on that screen. It also skips the one moment that proves the user + remembers the passphrase. +- **Fix:** clear `Passphrase` and its confirmation when enrolment completes, as the recovery code is + cleared. + +### H3 · A hardware Enter that submits the connect sheet leaves the window "not responding" + +- **What:** I typed the password and pressed Enter (an injected `KEYCODE_ENTER`, which is what a + Bluetooth or DeX keyboard sends). The connection started, but Android reported the window + unresponsive. The key-up was never acknowledged (`InputDispatcher: Waited 5009ms for + KeyEvent … ENTER … action=UP`). The ANR dialog then kept coming back + ([12](android/12-anr.png)) until the process was killed. Meanwhile the main thread was idle in + `Looper.pollOnce`: it wasn't busy, the event was simply lost. +- **Not affected:** tapping **CONNECT**, and the soft keyboard's ✓ (which sends an editor action, not a + key event). Both connected cleanly. +- **Likely cause:** focus moves to the terminal's native renderer (`Platform/RendererFocus.cs`) while + the Enter key-up is still in flight. +- **Repro:** host row → type password → hardware Enter. +- **Confidence:** reproduced once, and an accessibility dump was running at the time. Confirm on a real + device with a keyboard. + +### H4 · Screen-reader names are type names across the app + +The a11y tree announces these: + +| Control | Announced as | +|---|---| +| Bottom nav (Hosts / Connections / Settings) | `Avalonia.Controls.Panel` ×3 | +| Add-sheet rows, Settings rows, session chip | `Avalonia.Controls.StackPanel` / `Avalonia.Controls.Grid` | +| Keychain filter chips | `Avalonia.Controls.StackPanel` ×5 | +| Host list rows | `DodoSSH.Client.Shell.ViewModels.HostRowViewModel` | +| Host editor dropdowns | `AuthenticationChoice { Kind = Typed, EntityId = , … }`, `GroupChoice { … }` | +| Vault row | `VaultRowViewModel { VaultId = 01a0fe77-… }` (leaks an internal id) | +| FAB / header add | `+` (no name) | +| Decorative glyphs | `⌕ ▤ ⌗ ⚙ ⚿ ❯ ⇅ ◳ ≡ ◎ ›` exposed as text | + +There are two further problems: +- **Hidden content:** the hosts list stays in the tree under the full-screen host editor, so a + screen-reader user can wander into a screen they can't see. +- **Leftover desktop controls:** the desktop scrollbar's "Line up / Page down" buttons are exposed too. + +**Fix:** set `AutomationProperties.Name` on every button whose content isn't a string. Give row +templates a name, or override `ToString` on the row view models. Mark glyph `TextBlock`s +`AutomationProperties.AccessibilityView="Raw"`. Hide covered content from the tree while an editor +covers it. + +### H5 · Copying the recovery code isn't marked sensitive + +`CopyRecoveryCode` goes through Avalonia's plain `SetTextAsync` (`App.axaml.cs:187`). On Android 13+, +the system shows a clipboard preview of the copied text. That overlay is system UI, so this screen's +`FLAG_SECURE` doesn't cover it, and Gboard keeps the text in its clipboard history. Use a native +`ClipData` with `ClipDescription.EXTRA_IS_SENSITIVE` for any secret the app copies. (Read from code; +not observed, because the screen can't be captured.) + +--- + +## Medium — visual system + +### M1 · The accent glow is clipped to a square on every primary button and the FAB + +Each gradient button sits on a slightly lighter rectangle, and its corners show past the rounded shape: +- [z-glow-button](android/z-glow-button.png): SIGN IN, CREATE THE VAULT, UNLOCK, CONNECT, SAVE, ADD + FILES; +- [z-glow-fab](android/z-glow-fab.png): the round FAB sits inside a visible square. + +`AccentGlow` (`0 0 22 0`) is set on the template's `ContentPresenter` +(`Phone.axaml` `Button.primary` / `Button.fab /template/ ContentPresenter`) and gets clipped to the +button's own bounds. The desktop's `Button.accent` uses the same pattern, so check it there too. Fix: +draw the glow from an unclipped wrapper (a `Border` with margin), or drop it on the phone. + +### M2 · Body copy is monospace, against the shared-typeface decision + +`docs/design-import-gaps.md` ("The phone catches up") records that the phone shares the desktop's +faces. On the desktop, prose (`hint`, `heading`, the settings titles and captions) is Montserrat, and +mono is kept for identifiers (`.mono`) and labels (`.label`). On the phone, `TextBlock.body`, `.title` +and `.detail` all still set `MonoFont` (`Phone.axaml:661–680`). So every paragraph, title and caption is +JetBrains Mono, and only screen headers (Personal, Settings, New host) are Montserrat. The result is +long, wide-set paragraphs that are tiring to read ([03](android/03-enrol.png), +[16](android/16-settings.png), [19](android/19-sftp.png)). + +Keep mono for labels, buttons (the recorded ALL-CAPS-mono convention), addresses and code. Move +`.body`, `.title` and `.detail` to the default sans. + +### M3 · Fluent's selection fill leaks into the host list + +After a connect, the tapped row stays lit in `#3d486b` ([15](android/15-selected-row.png)). That +slate-blue isn't in `Palette.axaml`, which says selection is `Active` `#1E1E2C`. The comment at +`HostsScreen.axaml:360` says the selection fill was deliberately removed ("a lit row would be a mark +with no action behind it"). But only the custom `Border.hostrow.chosen` style was changed. Fluent's +`ListBoxItem:selected /template/ ContentPresenter` background was never overridden. + +### M4 · Disabled secondary buttons are heavier than enabled ones + +`Button.secondary` has no `:disabled` style, so Fluent's `#3c3c43` slab shows through. SFTP's disabled +**SAVE FILE** is the most prominent button in its row ([z-sftp-actions](android/z-sftp-actions.png)). +Mirror `Button.primary:disabled`: a `Raised` fill and `TextFaint` text. + +### M5 · Unstyled Fluent controls among styled ones + +- **Dropdowns:** `ComboBox` keeps Fluent's ~4 px radius, bright white border and black fill beside + 10 px, softly bordered fields (host editor, SFTP host picker; [08](android/08-host-editor.png)). +- **Checkboxes:** the default `CheckBox`, with its label sitting below the box's centre (relay, remember + password, press-Enter; [10](android/10-connect-sheet.png)). +- **Focused fields:** the fill switches from `Field` to pure `#000`. +- **Scrollbars:** desktop scrollbars are always visible. On Keychain, the horizontal bar is drawn over + the bottom of the filter chips ([17](android/17-keychain.png)). Android expects auto-hiding overlay + bars. +- **Stuck hover:** after a tap, Vaults' ↻ keeps a grey hover square. Touch has no hover, so + `:pointerover` styles need a touch-safe variant. + +### M6 · The terminal's colour and font have drifted from the palette + +`WebAssets/terminal.css` says its `#171a26` background "is `TerminalSurface` in Palette.axaml". The +palette now says `#18161F`. The terminal therefore reads navy, against the app's purple-black +([14](android/14-terminal.png)). Its font stack starts at `ui-monospace` and never names the embedded +JetBrains Mono, so on Android the WebView falls back to the system mono. That makes two different +monospace faces on one screen. + +### M7 · Iconography + +- **Glyphs, not icons:** the phone uses Unicode glyphs (`⚿ ❯ ⇅ ◳ ≡ ◎ ⚙ ▤ ⌗ ⌕`), even though Material + Icons is embedded and the desktop uses it. Weights and optical sizes vary (the heavy purple `❯` beside + hairline `◳`; [16](android/16-settings.png)). +- **Reused meanings:** ⚙ is both the Settings tab and the Preferences row, and ⚿ is both Keychain and + "Lock the keychain". +- **Splash:** the splash shows a flat purple circle, while the app's own icon is a rounded square with + the gradient. + +--- + +## Medium — information architecture and flows + +### M8 · "Settings" holds the product + +Keychain, Snippets, SFTP, S3, Logs and Vaults all sit under the ⚙ **Settings** tab, beside +Preferences and Lock ([16](android/16-settings.png)). File transfer and snippets are working tools, not +settings. Options: +- rename the tab ("More" or "Tools") and keep Preferences as one row in it; +- or promote SFTP to the bottom nav. Its icon is already ⇅. + +### M9 · Keychain can't add anything + +The empty state says "Add an SSH key or a password to stop typing one" ([17](android/17-keychain.png)). +The screen has no add control: no FAB, no header +, nothing in the a11y tree. Passwords can only be +created from the host editor's "+ NEW CREDENTIAL", and no phone path adds an SSH key. If keys are +desktop-only, the empty state should say so. + +### M10 · Three ways to add, two ways to edit + +- **Add:** Hosts has a FAB, Snippets a header "+", Keychain nothing, and the terminal's header "+" + means "new session". +- **Edit:** the host editor is a full-screen page with ← and ✓ ([08](android/08-host-editor.png)). The + snippet editor is an inline card with **SAVE/CANCEL**, and the header "+" stays live while it's open + ([18](android/18-snippet-editor.png)). + +Pick one pattern for each. + +### M11 · SFTP + +- **Action block:** it covers more than half the screen (seven buttons, a permanent help paragraph and a + status line), so only about four file rows are visible ([19](android/19-sftp.png)). +- **No selection:** RENAME, MOVE, PERMS and DELETE are enabled with nothing selected. Tapping one only + writes "Choose something on the host to rename." into the status line at the bottom. +- **DELETE:** neutral-styled here, while delete is danger-red on hosts and the keychain. +- **Directory rows:** marked only by a tiny purple ▸, with no folder icon. +- **Password again:** it's asked for again although a shell to the same host is already open. +- **S3:** blocked while an SFTP session is open. Tapping S3 bounces to SFTP, with "An SFTP session is + open. Close it before opening a bucket." at the bottom ([20](android/20-s3-bounced.png)). + +Suggestion: a contextual action bar that appears with a selection, help moved behind an ⓘ, and the +listing given the screen. + +### M12 · Bottom sheets + +- **Partial scrim:** the scrim covers only the content area. The header (with **LOCK**) and the bottom + nav stay bright and tappable under every sheet ([07](android/07-add-sheet.png), + [10](android/10-connect-sheet.png)). +- **Decorative handle:** it has a drag handle, but swipe-down doesn't dismiss. +- **Button order:** the primary action is on the left (CONNECT | CANCEL, SAVE | CANCEL). Android puts + the confirming action on the right. +- **No autofocus:** the connect sheet's password field isn't focused on open, so it takes an extra tap. +- **Nav above keyboard:** the bottom nav rides up above the keyboard and eats the sheet's space + ([13 before trust](android/13-host-key.png)). + +### M13 · Sign-in and enrolment + +- **Full browser:** sign-in opens a full Chrome tab (`ChromeTabbedActivity`), not a Custom Tab, so the + user leaves the app. Chrome's own first-run prompts then land on top. +- **Unannounced second sign-in:** step 2 asks for a passphrase, and **CREATE THE VAULT** then sends the + user to the identity provider's login form a second time. That's the deliberate `prompt=login` key + binding (`OidcClient.AuthorizeKeyBindingAsync`), but no copy warns that it's coming. Say "you'll + confirm your identity once more" on the passphrase screen. +- **Blank resume:** after the browser returns, the app showed a blank `#05050a` canvas for 5–10 s with + no progress indicator. Every shell state has a screen, so this looks like surface re-creation on + resume (Debug + emulator). Confirm on Release, and keep a visible state on screen while it happens. +- **Step labels:** start at "STEP 2 OF 3"; step 1 is never labelled. + +### M14 · The global status line leaks across screens + +A single `StatusMessage` follows the user around: +- "Unlocked 'Personal'." in the middle of Preferences ([21](android/21-preferences.png)); +- "Synchronised: 2 in, 2 out." after adding one host. `Describe` counts `Pulled`/`Pushed`, which include + the app's own log entries, while `PulledItems`/`PushedItems` already exist for this; +- "audit-box asks for a password." repeated under the sheet that asks for it. + +Scope messages to the screen that raised them, or use transient snackbars. + +--- + +## Low — input, copy, polish + +- **Keyboards:** + - the server URL field gets a text keyboard with contact suggestions, not the URL keyboard + ([02](android/02-server-keyboard.png)); + - the port field gets the full letter keyboard ([z-port-keyboard](android/z-port-keyboard.png)); + - hostname and username get autocorrect suggestions. + + Set `TextInputOptions.ContentType` (`Url`, `Digits`) and turn autocorrect off for those. +- **Placeholder-as-label:** name, hostname, username and notes in the host editor, the unlock field, + and the snippet fields have no labels, so each field's meaning disappears once typed in. +- **Terminology:** + - "CREATE THE VAULT", then "Creating your keychain"; + - "vault passphrase" (placeholder) next to "Enter your keychain passphrase" on the same screen; + - "ENROLLMENT" (US spelling) in a British-English app ("Synchronised", "unopenable"). +- **Fingerprint wrapping:** a single "I" wraps onto its own line. Chunk it, or size it to fit + ([13](android/13-host-key.png)). +- **Address format:** "10.0.2.2 · 2222" on the host-key sheet, "10.0.2.2:2222" everywhere else. +- **Dates:** "10/2/2026 11:29 PM" in Logs ([23](android/23-logs.png)), "2026-10-02 21:04" in SFTP. +- **Notification permission:** Android's prompt appears without explanation at the first connect. + Explain the keep-alive notification first. +- **Terminal width:** the default size gives about 37 columns, so `ls -la` wraps every line. +- **Empty and loading states:** + - Hosts' empty state is one grey line at the top left ([06](android/06-hosts-empty.png)); + - Vaults first rendered an empty list with no loading indicator; + - the keychain shows both "0 items" and an empty-state sentence. +- **Connecting card:** **CLOSE THIS TAB** and **SHOW LOGS** are indented past the card's left edge + ([11](android/11-connecting.png)). +- **Host-key context:** the sheet opened over the Quick Connect form, not over the connection progress + it belongs to. +- **Disabled fields:** on the enrolment busy state they turn a flat mid-grey + ([04](android/04-enrol-busy.png)), and the keyboard stays up. +- **Fingerprint setting:** "Unlock with a fingerprint" in Preferences is drawn as a bare circle; it reads + as a radio button, not a switch. + +## Not verified here + +- **System font size:** whether the app honours it. No `FontScale` handling exists in the phone code, + and Avalonia scales by density, so text likely ignores the setting. I didn't change device settings + to confirm. +- **Landscape and tablet:** an `IsWide` layout exists but wasn't exercised. +- **TalkBack:** not run end to end. The H4 findings come from the tree TalkBack reads. + +## Environment notes for a re-run + +- **Keycloak port:** 18080 was taken by another project's container, so Keycloak ran on **18180** via a + compose override, with `Oidc__Authority=http://127.0.0.1:18180/realms/dodossh` and + `adb reverse tcp:18180`. +- **`adb root`** restarts adbd and drops `adb reverse`. Re-add both forwards afterwards, or the app + shows OFFLINE. +- **The sshd** was a throwaway `linuxserver/openssh-server` container (`dodossh-audit-sshd`, port 2222, + user `dodo`). +- **Build:** `-p:RuntimeIdentifier=android-x64` adds an `android-x64` section to every + `packages.lock.json`. Restore those files afterwards. diff --git a/audit-2026-10-02/REPORT.md b/audit-2026-10-02/REPORT.md new file mode 100644 index 0000000..5114911 --- /dev/null +++ b/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. diff --git a/audit-2026-10-02/android/01-sign-in.png b/audit-2026-10-02/android/01-sign-in.png new file mode 100644 index 0000000..0a2444c Binary files /dev/null and b/audit-2026-10-02/android/01-sign-in.png differ diff --git a/audit-2026-10-02/android/02-server-keyboard.png b/audit-2026-10-02/android/02-server-keyboard.png new file mode 100644 index 0000000..f6a2eb6 Binary files /dev/null and b/audit-2026-10-02/android/02-server-keyboard.png differ diff --git a/audit-2026-10-02/android/03-enrol.png b/audit-2026-10-02/android/03-enrol.png new file mode 100644 index 0000000..8340163 Binary files /dev/null and b/audit-2026-10-02/android/03-enrol.png differ diff --git a/audit-2026-10-02/android/04-enrol-busy.png b/audit-2026-10-02/android/04-enrol-busy.png new file mode 100644 index 0000000..ecb9595 Binary files /dev/null and b/audit-2026-10-02/android/04-enrol-busy.png differ diff --git a/audit-2026-10-02/android/05-unlock-prefilled.png b/audit-2026-10-02/android/05-unlock-prefilled.png new file mode 100644 index 0000000..ababef4 Binary files /dev/null and b/audit-2026-10-02/android/05-unlock-prefilled.png differ diff --git a/audit-2026-10-02/android/06-hosts-empty.png b/audit-2026-10-02/android/06-hosts-empty.png new file mode 100644 index 0000000..dfd998b Binary files /dev/null and b/audit-2026-10-02/android/06-hosts-empty.png differ diff --git a/audit-2026-10-02/android/07-add-sheet.png b/audit-2026-10-02/android/07-add-sheet.png new file mode 100644 index 0000000..b9bff25 Binary files /dev/null and b/audit-2026-10-02/android/07-add-sheet.png differ diff --git a/audit-2026-10-02/android/08-host-editor.png b/audit-2026-10-02/android/08-host-editor.png new file mode 100644 index 0000000..eae4f1c Binary files /dev/null and b/audit-2026-10-02/android/08-host-editor.png differ diff --git a/audit-2026-10-02/android/09-host-list.png b/audit-2026-10-02/android/09-host-list.png new file mode 100644 index 0000000..36d15f5 Binary files /dev/null and b/audit-2026-10-02/android/09-host-list.png differ diff --git a/audit-2026-10-02/android/10-connect-sheet.png b/audit-2026-10-02/android/10-connect-sheet.png new file mode 100644 index 0000000..9c6efab Binary files /dev/null and b/audit-2026-10-02/android/10-connect-sheet.png differ diff --git a/audit-2026-10-02/android/11-connecting.png b/audit-2026-10-02/android/11-connecting.png new file mode 100644 index 0000000..e855892 Binary files /dev/null and b/audit-2026-10-02/android/11-connecting.png differ diff --git a/audit-2026-10-02/android/12-anr.png b/audit-2026-10-02/android/12-anr.png new file mode 100644 index 0000000..f3e39d4 Binary files /dev/null and b/audit-2026-10-02/android/12-anr.png differ diff --git a/audit-2026-10-02/android/13-host-key.png b/audit-2026-10-02/android/13-host-key.png new file mode 100644 index 0000000..1ea0268 Binary files /dev/null and b/audit-2026-10-02/android/13-host-key.png differ diff --git a/audit-2026-10-02/android/14-terminal.png b/audit-2026-10-02/android/14-terminal.png new file mode 100644 index 0000000..18aa693 Binary files /dev/null and b/audit-2026-10-02/android/14-terminal.png differ diff --git a/audit-2026-10-02/android/15-selected-row.png b/audit-2026-10-02/android/15-selected-row.png new file mode 100644 index 0000000..749935d Binary files /dev/null and b/audit-2026-10-02/android/15-selected-row.png differ diff --git a/audit-2026-10-02/android/16-settings.png b/audit-2026-10-02/android/16-settings.png new file mode 100644 index 0000000..c35f9bd Binary files /dev/null and b/audit-2026-10-02/android/16-settings.png differ diff --git a/audit-2026-10-02/android/17-keychain.png b/audit-2026-10-02/android/17-keychain.png new file mode 100644 index 0000000..d7f06a5 Binary files /dev/null and b/audit-2026-10-02/android/17-keychain.png differ diff --git a/audit-2026-10-02/android/18-snippet-editor.png b/audit-2026-10-02/android/18-snippet-editor.png new file mode 100644 index 0000000..1a9f5fd Binary files /dev/null and b/audit-2026-10-02/android/18-snippet-editor.png differ diff --git a/audit-2026-10-02/android/19-sftp.png b/audit-2026-10-02/android/19-sftp.png new file mode 100644 index 0000000..73fe339 Binary files /dev/null and b/audit-2026-10-02/android/19-sftp.png differ diff --git a/audit-2026-10-02/android/20-s3-bounced.png b/audit-2026-10-02/android/20-s3-bounced.png new file mode 100644 index 0000000..03ae2a8 Binary files /dev/null and b/audit-2026-10-02/android/20-s3-bounced.png differ diff --git a/audit-2026-10-02/android/21-preferences.png b/audit-2026-10-02/android/21-preferences.png new file mode 100644 index 0000000..fc6da04 Binary files /dev/null and b/audit-2026-10-02/android/21-preferences.png differ diff --git a/audit-2026-10-02/android/22-vaults.png b/audit-2026-10-02/android/22-vaults.png new file mode 100644 index 0000000..0de58ef Binary files /dev/null and b/audit-2026-10-02/android/22-vaults.png differ diff --git a/audit-2026-10-02/android/23-logs.png b/audit-2026-10-02/android/23-logs.png new file mode 100644 index 0000000..c281717 Binary files /dev/null and b/audit-2026-10-02/android/23-logs.png differ diff --git a/audit-2026-10-02/android/z-glow-button.png b/audit-2026-10-02/android/z-glow-button.png new file mode 100644 index 0000000..9889184 Binary files /dev/null and b/audit-2026-10-02/android/z-glow-button.png differ diff --git a/audit-2026-10-02/android/z-glow-fab.png b/audit-2026-10-02/android/z-glow-fab.png new file mode 100644 index 0000000..3904135 Binary files /dev/null and b/audit-2026-10-02/android/z-glow-fab.png differ diff --git a/audit-2026-10-02/android/z-port-keyboard.png b/audit-2026-10-02/android/z-port-keyboard.png new file mode 100644 index 0000000..ac7aaaf Binary files /dev/null and b/audit-2026-10-02/android/z-port-keyboard.png differ diff --git a/audit-2026-10-02/android/z-sftp-actions.png b/audit-2026-10-02/android/z-sftp-actions.png new file mode 100644 index 0000000..d6a1d08 Binary files /dev/null and b/audit-2026-10-02/android/z-sftp-actions.png differ diff --git a/audit-2026-10-02/hosts-truncated-labels.png b/audit-2026-10-02/hosts-truncated-labels.png new file mode 100644 index 0000000..fd7418d Binary files /dev/null and b/audit-2026-10-02/hosts-truncated-labels.png differ diff --git a/audit-2026-10-02/keychain-overlap.png b/audit-2026-10-02/keychain-overlap.png new file mode 100644 index 0000000..67a90b7 Binary files /dev/null and b/audit-2026-10-02/keychain-overlap.png differ diff --git a/audit-2026-10-02/locked-from-settings-no-titlebar.png b/audit-2026-10-02/locked-from-settings-no-titlebar.png new file mode 100644 index 0000000..ee98ef9 Binary files /dev/null and b/audit-2026-10-02/locked-from-settings-no-titlebar.png differ diff --git a/audit-2026-10-02/security-text-clipped.png b/audit-2026-10-02/security-text-clipped.png new file mode 100644 index 0000000..6355af0 Binary files /dev/null and b/audit-2026-10-02/security-text-clipped.png differ diff --git a/audit-2026-10-02/transfers.png b/audit-2026-10-02/transfers.png new file mode 100644 index 0000000..82b85db Binary files /dev/null and b/audit-2026-10-02/transfers.png differ