Fix a real credential-leak vector a review found, plus four smaller issues
Secret Scan / scan (push) Successful in 12s
Build App (Preview) / compute-version (pull_request) Successful in 3s
Secret Scan / scan (pull_request) Successful in 3s
Build App (Preview) / create-release (pull_request) Successful in 1s
Build App (Preview) / build-macos (pull_request) Successful in 2m41s
Build App (Preview) / build-windows (pull_request) Successful in 4m51s
Build App (Preview) / build-linux (pull_request) Successful in 5m12s
Build App (Preview) / prune-previews (pull_request) Successful in 1s

The headline finding: WebTerminalSettings::access_token is a live bearer
credential for a server that binds every interface, stored as a plain
field on AppSettings — which this feature was exporting and importing
wholesale as if it were as inert as a port number. A crafted export file
could set web_terminal.enabled and access_token together, and importing
it (with no more warning than any other setting change) would silently
stand up a LAN-listening terminal server with an attacker-known token on
the victim's next launch.

Fixed by carving the token out into ExportedSecrets, same as the other
three global secrets, with the same "only overwrite what the import
actually has" treatment — except that has to be done by hand here, since
this one lives inside the AppSettings blob that gets replaced wholesale
rather than in the keychain. Added SettingsImportPreview::
enables_web_terminal so "this turns on a listening service" gets its own
visible warning in the confirmation modal rather than hiding inside a
generic "settings replaced" bullet list.

Also fixed:

- read_and_decrypt checked format_version only after attempting to parse
  the full payload, so a future version bump that isn't
  deserialize-compatible would fail on the shape mismatch before the
  version check ever ran — and serde's type-mismatch errors quote the
  offending value inline, which is a real leak path since the plaintext
  here can hold a live credential. Now probes just the version field
  first, and neither error path interpolates the underlying serde message
  into what the user sees.
- apply_settings_import cleared the pending-import path before it could
  fail, so a rejected import (an invalid host path, anything
  update_settings validates) dead-ended the modal with no way back except
  cancelling and reopening the file picker. The path is now only cleared
  on success.
- Secrets are restored before the settings replace runs, not after —
  replacing settings is what triggers reconcile_gateway, and restoring
  secrets afterward left a real window where a gateway recreation
  happened against the destination's stale keys.
- The 8-character password minimum was frontend-only; export_settings now
  enforces it too, since that's the actual boundary a weak password has
  to cross. The derived key and decrypted plaintext are wrapped in
  zeroize::Zeroizing (already in the tree via aes-gcm).

Added test coverage the review named as missing: format-version
ordering, the generic-error-message guarantee, non_blank's blank-vs-
absent handling, and the new web-terminal preview/warning behavior on
both sides of the IPC boundary.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ
This commit is contained in:
2026-08-27 12:16:43 -07:00
co-authored by Claude Sonnet 5
parent 722d9aeff1
commit 925e51e435
11 changed files with 407 additions and 63 deletions
+35 -6
View File
@@ -555,12 +555,26 @@ nobody had reason to open. Fixtures are never live values; there is no case wher
## Settings export/import
`commands::settings_export_commands`, `storage::settings_crypto`, `models::settings_export`
(triple-c#35). Exports the *host* environment — global `AppSettings` (already the non-secret
shape persisted to `settings.json`) plus the global secrets that live in the OS keychain instead:
the shared Claude Code OAuth login and the model gateway's two keys. Per-project settings,
per-project secrets, and anything in a project's Docker volumes are deliberately out of scope —
this is not a project backup.
(triple-c#35). Exports the *host* environment — global `AppSettings` plus the global secrets that
live in the OS keychain instead: the shared Claude Code OAuth login and the model gateway's two
keys. Per-project settings, per-project secrets, and anything in a project's Docker volumes are
deliberately out of scope — this is not a project backup.
- **`AppSettings` is not entirely the non-secret shape it looks like, and a review of this feature
caught the one place that isn't.** `WebTerminalSettings::access_token` is a live bearer
credential for a server that binds every interface — exporting `AppSettings` wholesale would
have carried it along as if it were as inert as a port number, and importing it would have
applied `web_terminal.enabled` and the token together with no more warning than any other
setting, letting a crafted export silently stand up a LAN-listening terminal on the next launch.
`export_settings`/`apply_settings_import` carve this one field out into `ExportedSecrets`
instead, with the same "only overwrite what the import actually has" treatment as the other
three secrets — except "leave it alone" has to be done by hand in `apply_settings_import`, since
unlike the keychain secrets this one lives inside the `AppSettings` blob that gets replaced
wholesale. `SettingsImportPreview::enables_web_terminal` also exists because of this: `enabled`
and the token are independent fields, and "this turns on a listening service" must not hide
inside a generic "settings replaced" summary. Read this as the standing example of the class of
thing to keep checking for in this feature, not a one-off fixed bug — any other field that looks
like config but is actually a live credential would have the same problem.
- **Encrypted because it can carry live credentials, not for appearance's sake.** Argon2id derives
a 256-bit key from the user's password (memory-hard — meaningfully resistant to GPU/ASIC
brute-forcing, unlike PBKDF2 at any reasonable iteration count), AES-256-GCM does the actual
@@ -584,7 +598,22 @@ this is not a project backup.
field-by-field merge. Secrets are different on purpose: an absent secret in the export means
"the source machine never had this configured," not "delete this on import" — a user who wants
to clear a secret already has dedicated UI for that (signing out of shared auth, clearing the
gateway key).
gateway key). Secrets are restored *before* the settings replace runs, not after — replacing
settings is what triggers `reconcile_gateway`, and restoring the other way round leaves a real
window where a gateway recreation happens against the destination's old keys.
- **`read_and_decrypt` checks `format_version` before attempting to parse the full payload, not
after.** A version bump that isn't deserialize-compatible is exactly the case that check exists
for, and parsing the full struct first would fail on the shape mismatch before the version check
ever ran. Neither error path interpolates what `serde_json` actually says into the message
shown to the user — its type-mismatch errors quote the offending value inline, and the plaintext
here can hold a live credential.
- **The 8-character password minimum is enforced in `export_settings` itself, not only in the
export modal.** The frontend minimum is a UX nudge; the Rust command is the actual boundary a
weak password has to cross, and Argon2id's memory-hardness buys little against an attacker who
can just try a short password directly. The derived key and the decrypted plaintext are both
wrapped in `zeroize::Zeroizing` for the same reason every other secret in this codebase gets
handled carefully — cheap insurance (`zeroize` is already pulled in transitively via `aes-gcm`)
for material that exists only to hold or produce live credentials.
## Testing