fix(desktop): allow remote gateway token storage on keyring-less Linux - #62319
Conversation
alt-glitch
added
type/bug
|
Thanks for the focused fix. The premise is confirmed on current main: Suggested changes
The explicit renderer confirmation and persisted warning are appropriate for the plaintext fallback. This is an automated hermes-sweeper review. |
teknium1
added
sweeper:risk-security-boundary
|
Small extra data point from testing the first launch connect flow of #62293 in a sandbox on my Hyprland machine. The failure here is not always a missing keyring. On my setup gnome-keyring is actually running with the secrets component and org.freedesktop.secrets is available on the session bus, but safeStorage still reported encryption as unavailable. The cause is Chromium's password store auto detection, which keys off XDG_CURRENT_DESKTOP, and under Hyprland it never picks the gnome-libsecret backend on its own. Launching with --password-store=gnome-libsecret made isEncryptionAvailable() return true right away and the token was saved encrypted through the real keyring, no plain text involved. So for Hyprland and Sway users who do run a keyring, that flag is a zero code workaround that avoids the plain text downgrade entirely. Might be worth mentioning gnome-libsecret in the error message or the docs next to the basic option, so people with a working keyring do not opt into plain text storage when they do not have to. |
f68f4e5 to
1546ca2
Compare
|
Pushed an update that adds the regression coverage from the review, plus a rebase onto current main (it came over clean). main.ts has no exports, so I moved the two pieces under test into small injected helpers in hardening.ts, right next to encryptDesktopSecret. The whenReady block is now enableBasicPasswordStoreEncryption, and the token persistence decision inside coerceDesktopConnectionConfig is now resolvePersistedRemoteToken, which also owns the strict opt-in coercion in one place: anything that is not exactly true never enables plain text storage. main.ts hands the raw allowPlainTextToken payload field through, so the strictness itself is what gets tested. The new hardening.test.ts cases cover both behaviorally. For the startup branch: linux with the exact basic switch flips the flag exactly once, other switch values and other platforms never touch it, and a missing or throwing setUsePlainTextEncryption is tolerated. For the propagation: the opt-in runs end to end through the real encryptDesktopSecret (true stores plain, undefined, false, 1 and 'yes' all still throw), an available keyring never downgrades even with the opt-in, the transient test connection path passes through without touching secure storage, and an edit without a retyped token keeps the existing one. The main.ts wiring (save and apply routing through coerceDesktopConnectionConfig, the raw field handoff, the startup call ordered before createWindow, and the secureTokenStorage and remoteTokenPlainText fields in the sanitized response) is pinned with the same source assertion pattern as windows-hermes-resolution.test.ts. I also verified both branches at runtime in a sandboxed dev launch, not just in the unit suite. With --password-store=basic the save succeeds and the token lands on disk safeStorage encoded with secureTokenStorage reporting true. Without the flag (no store detected on my Hyprland setup) the save rejects with the new error, allowPlainTextToken: 1 still rejects, and allowPlainTextToken: true persists the token as encoding plain with remoteTokenPlainText reporting true in the sanitized response. Full electron suite, typecheck and lint are clean, same single pre existing windows-child-process failure as a clean main checkout. |
1546ca2 to
961979a
Compare
|
Rebased again to clear the conflict with the new embeddable Gateway settings panel on main. The resolution keeps main's embedded restructure of gateway-settings.tsx as is (including the removal of the dev soft switch preview) and layers the plain text opt-in dialog and warning banner on top, ungated by the embedded flag so both the standalone page and the embedded panel get them. Typecheck, lint and the full electron suite are green at the same baseline as before, and the hardening regression tests all still pass. |
961979a to
da32b3b
Compare
|
@teknium1 rebased once more, this time over the desktop test suite move to vitest, and the PR is conflict free and mergeable again. The regression tests you asked for are ported to the new vitest idiom (the bodies are unchanged, only the runner import moved) and the whole electron project passes under the new runner, 429 tests green. Typecheck, lint and prettier are clean on everything this PR touches. Recap of where this PR stands: the plain text opt-in propagation and the basic password store startup are covered behaviorally through the extracted helpers plus wiring pins, both verified at runtime in a sandboxed launch earlier in the thread. From my side this is ready for another look whenever you get a chance. |
On Linux without a Secret Service keyring (e.g. Hyprland/Sway with no
GNOME Keyring or KWallet), safeStorage.isEncryptionAvailable() is false,
so saving a remote gateway session token from Settings -> Gateway failed
hard with no in-app way forward.
- encryptDesktopSecret gains an explicit allowPlainText opt-in: when
secure storage is unavailable and the user confirmed the prompt, the
token persists as { encoding: 'plain' } in connection.json (which
decryptDesktopSecret already round-trips).
- Settings -> Gateway now surfaces the opt-in: a destructive confirm
dialog before persisting a token in plain text, and a persistent
warning banner while the saved token is stored unencrypted. Localized
in en/ja/zh/zh-hant.
- The connection-config IPC response reports secureTokenStorage and
remoteTokenPlainText so the renderer can drive both affordances.
- Launching with --password-store=basic now works: on Linux the app
calls safeStorage.setUsePlainTextEncryption(true) at startup when the
switch is set, which Electron requires for the basic backend to count
as available.
- The no-opt-in error now spells out all three remedies (enable an OS
keyring, confirm plain-text storage, or use HERMES_DESKTOP_REMOTE_URL/
HERMES_DESKTOP_REMOTE_TOKEN).
Fixes NousResearch#62294
…ore startup Review feedback asked for regression coverage of the main process pieces: the connection-config save and apply IPC path that carries allowPlainTextToken down to encryptDesktopSecret, and the Linux --password-store=basic startup branch. main.ts has no exports, so both pieces now live as small injected helpers in hardening.ts next to encryptDesktopSecret. The whenReady block became enableBasicPasswordStoreEncryption, which only acts on linux with the exact basic switch value, tolerates a missing or throwing setUsePlainTextEncryption, and reports whether it actually flipped the flag. The token persistence ternary became resolvePersistedRemoteToken, which owns the strict opt-in coercion in one place: a truthy value that is not exactly true never enables plain text storage. main.ts passes the raw payload field through, so the strictness itself is what the tests pin. hardening.test.ts grows behavioral cases for both helpers, including the full path through the real encryptDesktopSecret for the opt-in, the never downgrade rule when the keyring is available, and the transient test connection passthrough. The wiring inside main.ts (save and apply routing through coerceDesktopConnectionConfig, the raw field handoff, the startup call ordered before createWindow, and the secureTokenStorage and remoteTokenPlainText fields in the sanitized response) is pinned with the repo's source assertion pattern.
da32b3b to
af400e1
Compare
# Conflicts: # apps/desktop/electron/main.ts # apps/desktop/src/app/settings/gateway-settings.tsx
The helpers were tested; nothing proved main.ts called them. Reverting both call sites and both imports in readDesktopConnectionConfig / writeDesktopConnectionConfig left the whole suite green (947 passed / 2 skipped, tsc 0, eslint clean, e2e 1 passed 1 skipped) while connection.json went back to 0644 — the user-visible fix this PR promises was untested. The e2e spec could not catch it by construction: it asserts the ENCRYPTION contract with a raw-bytes scan, and safeStorage keeps the token opaque regardless of the file's mode, so a 0644 file passes that scan every time. There was no mode assertion anywhere in e2e/. Adds the missing third contract — unreadable by other local accounts — on all three paths that can produce the file: - write: assert the mode of the artifact test 1 already proves the app wrote. - read, valid file: seed the app's own encrypted connection.json back to 0644 and assert launch tightens it. Scoped to the MODE only, so it is independent of the still-deferred plaintext migration — the fixture's token is already ciphertext, so nothing re-encrypts, no NousResearch#62319 opt-in marker is involved, and no rotation guidance is owed. - read, corrupt file: a truncated file still holds the token bytes and throws into the swallowing catch, so it would be the one file never tightened. This is the only test that distinguishes the chmod's placement relative to the parse. Also moves the tighten above JSON.parse for exactly that reason, and pins the cache invariant the placement depends on: the tighten must be a chmod, not a rewrite, because it sits inside the function whose cache keys on mtimeMs. Asserted as `mode & 0o077 === 0` rather than `=== 0o600` to avoid a change-detector, and skipped on win32, where chmod maps to the read-only bit and the fix deliberately no-ops (ACLs are PR NousResearch#77527). Every assertion was mutation-tested: reverting the full wiring fails all three; reverting only the write path fails only the write test; deleting only the tighten-on-read fails only the two read tests; moving the tighten below the parse fails only the corrupt test; making the tighten a rewrite instead of a chmod fails the mtime assertions. Bundle greps confirmed each mutation reached dist/electron-main.mjs before the run.
teknium1
closed this pull request by merging all changes
into
NousResearch:main
in
d48c5f2
|
Merged via #84878 — your commits were carried over onto current main with your authorship preserved in git history (e879d13, af400e1 now on main under your name). The only adjustments were two trivial both-sides-added conflict resolutions against newer main (an import line in main.ts and the sshRemoteProfile spread in gateway-settings.tsx). Thanks for the fix, the regression coverage, and the three rebases along the way — and for the Chromium password-store auto-detection data point, which is exactly the kind of detail that makes these reviews easier. |
`connection.json` under the desktop app's Electron `userData` was written with no
file mode, so it landed at the `0644` umask default — while its two
credential-bearing neighbours in the same directory, `desktop-installation.json`
and `native-oauth-tokens.json`, were already `0600`. That file holds the
safeStorage-encrypted gateway token plus the fields that are NOT encrypted: the
gateway URL and the SSH host, user, and key path.
- Route the single write choke point through a helper that creates the file
owner-only and atomically.
- Tighten an already-existing `0644` file once per launch on the read path, so
installs that already have one do not stay world-readable until the next save.
- Refuse to act on a path that is a symlink or not owned by the current user,
matching the guards `desktop-installation.ts` already applies to its sibling.
The symlink guard alone turned out to be insufficient, and that is worth
recording: `writeSecretFileAtomic` tightens its *temp* path, so a symlink planted
at `connection.json.tmp` meant `writeFileSync` followed it, the guard correctly
bailed, and `renameSync` then moved the link onto `connection.json` permanently.
Measured, guard-only vs. as-landed:
guards only token leaked: true config is a symlink: true 755
guards + temp unlink token leaked: false config is a symlink: false 600
So the temp path is unlinked before the write.
Issue #77486's headline claim — that a dashboard session token is persisted in
plaintext — does not hold against main. The token has been safeStorage-encrypted
since the desktop app reached mainline in 51c68d4, and `encryptDesktopSecret`
aborts with an actionable message rather than degrading to plaintext when
safeStorage is unavailable. The `{ encoding: 'plain', value }` literal does exist
at main.ts:7084, but only on the `persistToken: false` branch, whose sole caller
is the connection-test handler, which never writes. So no mainline path *writes*
a plaintext token. The commits that did contain a plaintext-writing fallback
(d3d1772, d208f2c) are not ancestors of main — they live only on
upstream/bb/gui-* and the desktop-pr20059-installers pre-release tag.
At-rest migration of legacy non-safeStorage payloads is deliberately NOT included.
An earlier revision of this branch implemented it and it was removed after review
reproduced two token-loss paths: it force-converts the opt-in plaintext choice
PR #62319 adds (silently reverting the user's decision, then destroying the token
on the next launch without the `--password-store=basic` flag), and it converts a
portable credential into a keychain-bound one with no consent — destroying the
only recoverable copy while not remediating the real exposure, since every
existing backup still holds the plaintext and the true remedy is rotation. It also
persisted raw `parsed`, bypassing `sanitizeConnectionProfiles`. A comment at the
read path records the three preconditions any future attempt needs.
`decryptDesktopSecret`'s non-safeStorage read fallback is untouched — it is what
lets a pre-release or hand-edited config work at all.
Windows still inherits the userData directory ACL rather than an explicit
owner-only one; mode bits are advisory there, so that half is deferred to
PR #77527 rather than growing a second ACL implementation here.
e2e: `at-rest-connection-token.spec.ts` asserts the at-rest contract
implementation-independently — the token's plaintext value (and its base64 form)
must not appear in a raw-bytes scan of any file under userData or HERMES_HOME,
AND the app must still put the exact original token on the wire after a restart,
so a fix that simply drops the token cannot pass. Proven non-vacuous by mutation:
writing `{ encoding: 'plain', value }` still fails the scan while the
file-exists and gateway-URL guards pass. The migration case is a documented
`test.fixme` naming its three blockers.
Electron project 928 -> 924 tests (-9 migration, +5 new guard and
mechanism-isolation). Two of those five exist because reverting either owner-only
mechanism alone initially scored zero failures — they were masking each other, so
either could have been deleted green.
(cherry picked from commit 6e01add)
The helpers were tested; nothing proved main.ts called them. Reverting both call sites and both imports in readDesktopConnectionConfig / writeDesktopConnectionConfig left the whole suite green (947 passed / 2 skipped, tsc 0, eslint clean, e2e 1 passed 1 skipped) while connection.json went back to 0644 — the user-visible fix this PR promises was untested. The e2e spec could not catch it by construction: it asserts the ENCRYPTION contract with a raw-bytes scan, and safeStorage keeps the token opaque regardless of the file's mode, so a 0644 file passes that scan every time. There was no mode assertion anywhere in e2e/. Adds the missing third contract — unreadable by other local accounts — on all three paths that can produce the file: - write: assert the mode of the artifact test 1 already proves the app wrote. - read, valid file: seed the app's own encrypted connection.json back to 0644 and assert launch tightens it. Scoped to the MODE only, so it is independent of the still-deferred plaintext migration — the fixture's token is already ciphertext, so nothing re-encrypts, no #62319 opt-in marker is involved, and no rotation guidance is owed. - read, corrupt file: a truncated file still holds the token bytes and throws into the swallowing catch, so it would be the one file never tightened. This is the only test that distinguishes the chmod's placement relative to the parse. Also moves the tighten above JSON.parse for exactly that reason, and pins the cache invariant the placement depends on: the tighten must be a chmod, not a rewrite, because it sits inside the function whose cache keys on mtimeMs. Asserted as `mode & 0o077 === 0` rather than `=== 0o600` to avoid a change-detector, and skipped on win32, where chmod maps to the read-only bit and the fix deliberately no-ops (ACLs are PR #77527). Every assertion was mutation-tested: reverting the full wiring fails all three; reverting only the write path fails only the write test; deleting only the tighten-on-read fails only the two read tests; moving the tighten below the parse fails only the corrupt test; making the tighten a rewrite instead of a chmod fails the mtime assertions. Bundle greps confirmed each mutation reached dist/electron-main.mjs before the run. (cherry picked from commit 99cfc16)
`connection.json` under the desktop app's Electron `userData` was written with no
file mode, so it landed at the `0644` umask default — while its two
credential-bearing neighbours in the same directory, `desktop-installation.json`
and `native-oauth-tokens.json`, were already `0600`. That file holds the
safeStorage-encrypted gateway token plus the fields that are NOT encrypted: the
gateway URL and the SSH host, user, and key path.
- Route the single write choke point through a helper that creates the file
owner-only and atomically.
- Tighten an already-existing `0644` file once per launch on the read path, so
installs that already have one do not stay world-readable until the next save.
- Refuse to act on a path that is a symlink or not owned by the current user,
matching the guards `desktop-installation.ts` already applies to its sibling.
The symlink guard alone turned out to be insufficient, and that is worth
recording: `writeSecretFileAtomic` tightens its *temp* path, so a symlink planted
at `connection.json.tmp` meant `writeFileSync` followed it, the guard correctly
bailed, and `renameSync` then moved the link onto `connection.json` permanently.
Measured, guard-only vs. as-landed:
guards only token leaked: true config is a symlink: true 755
guards + temp unlink token leaked: false config is a symlink: false 600
So the temp path is unlinked before the write.
Issue #77486's headline claim — that a dashboard session token is persisted in
plaintext — does not hold against main. The token has been safeStorage-encrypted
since the desktop app reached mainline in 51c68d4, and `encryptDesktopSecret`
aborts with an actionable message rather than degrading to plaintext when
safeStorage is unavailable. The `{ encoding: 'plain', value }` literal does exist
at main.ts:7084, but only on the `persistToken: false` branch, whose sole caller
is the connection-test handler, which never writes. So no mainline path *writes*
a plaintext token. The commits that did contain a plaintext-writing fallback
(d3d1772, d208f2c) are not ancestors of main — they live only on
upstream/bb/gui-* and the desktop-pr20059-installers pre-release tag.
At-rest migration of legacy non-safeStorage payloads is deliberately NOT included.
An earlier revision of this branch implemented it and it was removed after review
reproduced two token-loss paths: it force-converts the opt-in plaintext choice
PR #62319 adds (silently reverting the user's decision, then destroying the token
on the next launch without the `--password-store=basic` flag), and it converts a
portable credential into a keychain-bound one with no consent — destroying the
only recoverable copy while not remediating the real exposure, since every
existing backup still holds the plaintext and the true remedy is rotation. It also
persisted raw `parsed`, bypassing `sanitizeConnectionProfiles`. A comment at the
read path records the three preconditions any future attempt needs.
`decryptDesktopSecret`'s non-safeStorage read fallback is untouched — it is what
lets a pre-release or hand-edited config work at all.
Windows still inherits the userData directory ACL rather than an explicit
owner-only one; mode bits are advisory there, so that half is deferred to
PR #77527 rather than growing a second ACL implementation here.
e2e: `at-rest-connection-token.spec.ts` asserts the at-rest contract
implementation-independently — the token's plaintext value (and its base64 form)
must not appear in a raw-bytes scan of any file under userData or HERMES_HOME,
AND the app must still put the exact original token on the wire after a restart,
so a fix that simply drops the token cannot pass. Proven non-vacuous by mutation:
writing `{ encoding: 'plain', value }` still fails the scan while the
file-exists and gateway-URL guards pass. The migration case is a documented
`test.fixme` naming its three blockers.
Electron project 928 -> 924 tests (-9 migration, +5 new guard and
mechanism-isolation). Two of those five exist because reverting either owner-only
mechanism alone initially scored zero failures — they were masking each other, so
either could have been deleted green.
(cherry picked from commit 6e01add)
The helpers were tested; nothing proved main.ts called them. Reverting both call sites and both imports in readDesktopConnectionConfig / writeDesktopConnectionConfig left the whole suite green (947 passed / 2 skipped, tsc 0, eslint clean, e2e 1 passed 1 skipped) while connection.json went back to 0644 — the user-visible fix this PR promises was untested. The e2e spec could not catch it by construction: it asserts the ENCRYPTION contract with a raw-bytes scan, and safeStorage keeps the token opaque regardless of the file's mode, so a 0644 file passes that scan every time. There was no mode assertion anywhere in e2e/. Adds the missing third contract — unreadable by other local accounts — on all three paths that can produce the file: - write: assert the mode of the artifact test 1 already proves the app wrote. - read, valid file: seed the app's own encrypted connection.json back to 0644 and assert launch tightens it. Scoped to the MODE only, so it is independent of the still-deferred plaintext migration — the fixture's token is already ciphertext, so nothing re-encrypts, no #62319 opt-in marker is involved, and no rotation guidance is owed. - read, corrupt file: a truncated file still holds the token bytes and throws into the swallowing catch, so it would be the one file never tightened. This is the only test that distinguishes the chmod's placement relative to the parse. Also moves the tighten above JSON.parse for exactly that reason, and pins the cache invariant the placement depends on: the tighten must be a chmod, not a rewrite, because it sits inside the function whose cache keys on mtimeMs. Asserted as `mode & 0o077 === 0` rather than `=== 0o600` to avoid a change-detector, and skipped on win32, where chmod maps to the read-only bit and the fix deliberately no-ops (ACLs are PR #77527). Every assertion was mutation-tested: reverting the full wiring fails all three; reverting only the write path fails only the write test; deleting only the tighten-on-read fails only the two read tests; moving the tighten below the parse fails only the corrupt test; making the tighten a rewrite instead of a chmod fails the mtime assertions. Bundle greps confirmed each mutation reached dist/electron-main.mjs before the run. (cherry picked from commit 99cfc16)
3 participants
Fixes #62294
Problem
On Linux without a Secret Service keyring (e.g. Hyprland/Sway with no GNOME Keyring or KWallet),
safeStorage.isEncryptionAvailable()returns false, soencryptDesktopSecretthrew when saving a remote gateway session token — the Settings → Gateway form (and any flow built on the same save path) could not store a token at all. Launching with--password-store=basicdidn't help either, because the app never calledsafeStorage.setUsePlainTextEncryption(true), which Electron requires on Linux for the basic backend to count as available.Fix
Implements ideas 1 + 3 from the issue:
apps/desktop/electron/hardening.ts):encryptDesktopSecretgains anallowPlainTextoption. When secure storage is unavailable and the user opted in, the token persists as{ encoding: 'plain' }inconnection.json— an encodingdecryptDesktopSecretalready round-trips (it's what the transient test-connection path uses), so no decrypt-side changes. When encryption is available, the opt-in never downgrades: the safeStorage path is used unconditionally.gateway-settings.tsx): saving/applying a token-auth remote when secure storage is unavailable now opens a destructiveConfirmDialogspelling out that the token would be saved unencrypted and readable by any process running as the user; confirming re-issues the save withallowPlainTextToken: true. While the saved token is stored in plain text, a persistent warning banner (same idiom as the env-override banner) shows in the token section. Strings localized in en/ja/zh/zh-hant. A failed save inside the dialog rethrows a readable error so the dialog shows the inline failure instead of its success beat.hermes:connection-config:get/save/apply response now carriessecureTokenStorage(is safeStorage usable right now) andremoteTokenPlainText(is the saved token plain text) so the renderer can drive both affordances; the save payload acceptsallowPlainTextToken.--password-store=basicnow works: on Linux, startup callssafeStorage.setUsePlainTextEncryption(true)when the switch is present, before anything touches safeStorage.org.freedesktop.secrets, confirm the plain-text prompt in Settings → Gateway, or setHERMES_DESKTOP_REMOTE_URL/HERMES_DESKTOP_REMOTE_TOKEN).The first-launch connect screen from #62293 shares this save path, so it inherits the fix once both land.
Testing
New
hardening.test.tscases: unavailable + opt-in →{encoding:'plain'}; available + opt-in → still safeStorage-encoded; empty value → null; unavailable without opt-in → still throws.Full
node --testelectron suite: 323/324 (the one failure is a Windows-only tree-kill test that fails identically on the unmodified baseline in this environment).tsc(renderer + electron configs) and ESLint clean; renderer vitest for the touched areas green, full-suite results identical to baseline.Runtime validation on real Electron (headless Linux): a probe launched with
--password-store=basicconfirms the exact behavior this PR relies on —isEncryptionAvailable()isfalsebeforesetUsePlainTextEncryption(true)andtrueafter (backend: basic_text), with a successful encrypt/decrypt round-trip:🤖 Generated with Claude Code