fix(desktop): create connection.json owner-only

[ZHJay]

What does this PR do?

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 were already 0600: desktop-installation.json (desktop-installation.ts:106) and native-oauth-tokens.json (main.ts:6304). That file holds the safeStorage-encrypted gateway token plus fields that are not encrypted: the gateway URL and the SSH host, user, and key path. This makes the three consistent.

Three mechanisms, all on the single write choke point (writeDesktopConnectionConfig, which every IPC save/apply and persistSshConnectionToken funnels through):

1. Owner-only atomic create — writeSecretFileAtomic creates the file 0600 from the moment it exists.

2. Tighten-on-read — an install that already has a 0644 file gets chmod'ed once per launch on the read path, so it does not stay group/other-readable until the user's next Settings save. Runs on a cache miss only; chmod moves ctime, not mtime, so it cannot invalidate the cache it sits inside.

   Sequenced before JSON.parse, not after. A truncated or hand-mangled connection.json still contains the token bytes, and the parse throws into the catch that falls back to local mode — a fallback that is never written back, so nothing would ever come back for that file. With the chmod after the parse, exactly the file that is both corrupt and world-readable would be the one file never tightened, permanently. The chmod needs only the path, so it has no reason to wait for valid JSON.

3. Refuse to act on a symlink or a path not owned by the current user — matching the guards desktop-installation.ts already applies to its sibling (desktop-installation.ts:19-30).

The symlink finding (why the guard alone was not enough)

writeSecretFileAtomic tightens its temp path, so a symlink planted at connection.json.tmp meant writeFileSync followed it, the lstat guard correctly bailed, and renameSync then moved the link onto connection.json permanently. Measured:

guards only            token leaked: true    config is a symlink: true   755
guards + temp unlink   token leaked: false   config is a symlink: false  600

Hence the temp path is unlinked before the write.

At-rest migration is deliberately NOT included

An earlier revision of this branch implemented migration of legacy non-safeStorage payloads to ciphertext. It was removed after review reproduced two token-loss paths:

• It force-converts the opt-in plaintext choice that open PR fix(desktop): allow remote gateway token storage on keyring-less Linux #62319 ("fix(desktop): allow remote gateway token storage on keyring-less Linux") deliberately adds — silently reverting the user's decision, then destroying the token on the next launch without the --password-store=basic flag that PR adds. It also makes that PR's "Token stored in plain text" banner lie.
• 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/sync copy still holds the plaintext. The true remedy is rotation.
• It persisted raw parsed, bypassing sanitizeConnectionProfiles (reproduced: profile names violating PROFILE_NAME_RE, bogus authMode, and arbitrary junk re-persisted by the app's own hand).

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

Mode bits are advisory there, so connection.json still inherits the userData directory ACL. tightenSecretFileMode no-ops on win32 rather than flipping Node's read-only bit and breaking the next write. Deferred to open PR #77527 ("fix(security): enforce owner-only ACLs on Windows in _secure_file") rather than growing a second ACL implementation here.

Related Issue

Context: #77486 — not Fixes, because that issue bundles three claims and this addresses one, with a corrected premise.

Correcting the premise. The issue's headline claim is that a dashboard session token is persisted in plaintext. That does not hold against main. The token has been safeStorage-encrypted since the desktop app reached mainline in 51c68d4ab, and encryptDesktopSecret aborts with an actionable message rather than degrading to plaintext when safeStorage is unavailable. Verified:

Check	Result
git merge-base --is-ancestor d3d177283 upstream/main	not an ancestor (exit 1)
git merge-base --is-ancestor d208f2c2c upstream/main	not an ancestor (exit 1)
git merge-base --is-ancestor 51c68d4ab upstream/main	is an ancestor (exit 0)
git log -S 'Fall through to plaintext' upstream/main -- apps/desktop	empty (0 commits)

To be precise, because the loose version of this claim is false: the { encoding: 'plain', value } literal does exist on main at main.ts:7084, but only on the persistToken: false branch. Its sole caller is testDesktopConnectionConfig (main.ts:7750, reached only via ipcMain.handle('hermes:connection-config:test') at main.ts:9839), and that function body contains no write call of any kind. So the accurate statement is no mainline path writes a plaintext token — not that the literal is absent.

The other two parts of #77486 are handled by sibling PRs, both open: #77579 ("fix(security): create browser-profile and media-cache artifacts owner-only") and #77584 ("fix(desktop): bound in-flight turn journal retention and purge it on delete"). That second one inverts the issue's proposed remedy: the issue asks for journal redaction, and #77584 instead bounds retention and purges on delete — redacting the turn tail would defeat the journal's purpose, since it exists to restore an interrupted turn and a redacted entry cannot do that.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• 🔒 Security fix

Changes Made

• apps/desktop/electron/hardening.ts — new tightenSecretFileMode and writeSecretFileAtomic helpers, SECRET_FILE_MODE = 0o600, and SAFE_STORAGE_ENCODING extracted so the writer here and the reader in main.ts cannot drift across the file boundary.
• apps/desktop/electron/main.ts — writeDesktopConnectionConfig routes through writeSecretFileAtomic; readDesktopConnectionConfig calls tightenSecretFileMode on a cache miss; decryptDesktopSecret uses the shared encoding constant and gains a comment stating the non-safeStorage fallback is a read path, not a write path.
• apps/desktop/electron/hardening.test.ts — 15 → 27 tests.
• apps/desktop/e2e/at-rest-connection-token.spec.ts — new, implementation-independent at-rest contract: encryption, usability after restart, and owner-only mode on all three paths that can produce the file (fresh write, pre-existing 0644, corrupt 0644).

native-oauth-tokens.json was deliberately not routed through the new helper: its introducing commit already wrote { mode: 0o600 }, so unlike connection.json there is no loose-mode population in the field, and changing it would mix concerns.

How to Test

1. From apps/desktop: npx vitest run --project electron electron/hardening.test.ts → 27 passed.
2. npx vitest run --project electron → 947 passed | 2 skipped. Measured baseline on the parent commit (b45d88690, current upstream/main) in a separate worktree: 935 passed | 2 skipped. So this is +12 tests, no coverage loss; hardening.test.ts alone goes 15 → 27.
3. npm run build && npx playwright test e2e/at-rest-connection-token.spec.ts --reporter=list → 3 passed, 1 skipped (the skip is a deliberate test.fixme for the migration case, naming its three blockers).
4. Manual: with an existing 0644 connection.json, launch the app and stat the file — it becomes 600 on first read, and the configured gateway still connects.

The vitest count is unchanged by the mode coverage below — the three new tests are Playwright, not vitest. main.ts cannot be unit-tested (it imports electron; grep -rln "from './main'" returns zero hits repo-wide), which is why the wiring is covered end to end instead.

The e2e spec asserts the 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.

Mutation testing (each mechanism reverted individually)

Two of the five new tests exist because reverting either owner-only mechanism alone initially scored zero failures — they were masking each other, so either could have been deleted green. That was a real anti-pattern, caught and fixed:

Reverted mechanism	Failures	Test that catches it
create-time mode	1	owner-only even where chmod does nothing
chmod-before-rename	1	owner-only even when a stale temp cannot be removed
stale-temp unlink	1	cannot be redirected through a symlink planted at the temp path
tighten-on-read chmod	5	the tightenSecretFileMode group
lstat guards → bare statSync	3	symlink / ownership / Windows guards

All five were restored byte-identically afterwards (hardening.ts sha256 603a7f4d…, main.ts 16f075ed…) and the suite is green again.

The wiring is now covered end to end

The mutation table above covers the helpers. It did not cover the wiring, and that turned out to matter: with both call sites and both imports in main.ts reverted — writeDesktopConnectionConfig back to plain writeFileAtomic, no tighten-on-read — the entire suite stayed green (947 passed / 2 skipped, tsc exit 0, eslint clean, e2e 1 passed 1 skipped) while connection.json went back to 0644. Bundle proof: writeSecretFileAtomic( and tightenSecretFileMode( both dropped to 0 occurrences in dist/electron-main.mjs. So the user-visible fix in this PR's title was, until now, untested.

The e2e spec could not have caught it by construction. It asserted 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/.

The spec now asserts a third contract — unreadable by other local accounts — on all three paths that can produce the file:

Test	Path covered	Fixture
a newly configured token … still works after restart	the write choke point	the app's own save
an install whose connection.json predates owner-only mode is tightened on read	tighten-on-read, valid file	the app's own encrypted file, chmod'ed back to 0644
a corrupt connection.json is tightened even though it never parses	tighten-on-read, parse fails	truncated JSON at 0644 still holding token bytes

Each is mutation-proven, individually:

Mutation	e2e failures	Which tests
revert all main.ts wiring (both call sites + both imports)	3	all three, each reporting mode 644
revert only writeDesktopConnectionConfig → writeFileAtomic	1	write test only; both read tests correctly stay green
delete only the tighten-on-read call	2	both read tests only; write test correctly stays green
move the tighten below JSON.parse	1	corrupt test only — the only test that can distinguish the ordering
make the tighten a rewrite instead of a chmod	2	both mtime assertions

main.ts was restored byte-identically after each (sha256 0431244d…), and the rebuilt bundle returned to sha256 96ed89fc… with writeSecretFileAtomic( back at 2 and tightenSecretFileMode( back at 3 occurrences. Every mutation was verified present in dist/electron-main.mjs before its run, since a stale dist/ would silently produce a false green.

Two design notes on the new assertions:

• Asserted as mode & 0o077 === 0, not === 0o600. The requirement is that nobody else can reach the file; pinning exact bits would be a change-detector against a future 0400 or a setgid-directory umask.
• The mode assertion is skipped on win32 (mode bits are advisory there and tightenSecretFileMode no-ops by design — see the Windows section), so it cannot go red on a Windows runner for behaviour this PR never claimed. The encryption half stays unconditional.

The two read tests also pin the invariant the tighten-on-read placement depends on: the tighten must be a chmod, not a rewrite, because it sits inside the function whose cache keys on mtimeMs. Measured directly on macOS: mode 644 → 600, mtimeMs unchanged, ctimeMs moved.

Follow-up, not done here: readDesktopConnectionConfig / writeDesktopConnectionConfig both hardcode the module-level DESKTOP_CONNECTION_CONFIG_PATH, which is why no unit test can reach them. Making the path injectable — as desktop-installation.ts already does, precisely so desktop-installation.test.ts:41 can assert the mode of the file its orchestrating function wrote — is the durable fix and would make this unit-testable without Electron. It is a refactor of a ~12k-line file's call sites, so it does not belong in a bug fix; the e2e assertions above cover the same behaviour today, and with better fidelity (they witness the real app.getPath('userData') file, not an injected temp path).

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(desktop):)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix (4 files, one logical change)
• I've run pytest tests/ -q and all tests pass — not run, and not applicable: this change is TypeScript-only under apps/desktop/ and touches no Python. I ran the JS/TS equivalents instead (vitest electron project, tsc on tsconfig.electron.json and tsconfig.e2e.json, eslint, Playwright) — all listed above. I did not run the full scripts/run_tests.sh Python suite.
• I've added tests for my changes
• I've tested on my platform: macOS 15 (arm64), Node 22

Lint/typecheck, run in this branch's worktree: npx eslint electron/ → clean (exit 0). npx eslint e2e/at-rest-connection-token.spec.ts → clean (exit 0). npx tsc -p tsconfig.electron.json --noEmit and npx tsc -p tsconfig.e2e.json --noEmit → both clean. To be clear, npx eslint e2e/ as a whole is not clean — it exits 1 with 244 pre-existing problems (103 errors, 141 warnings) unrelated to this change; I linted only my own file.

Documentation & Housekeeping

• I've updated relevant documentation — N/A (no user-facing surface, no new config keys; the reasoning lives in code comments at both the write and read paths)
• I've updated cli-config.yaml.example if I added/changed config keys — N/A
• I've updated CONTRIBUTING.md or AGENTS.md — N/A
• I've considered cross-platform impact — POSIX gets owner-only mode; Windows no-ops by design (see Windows section) and is deferred to fix(security): enforce owner-only ACLs on Windows in _secure_file #77527
• I've updated tool descriptions/schemas — N/A

Relationship to prior art

Searched gh search prs/issues for connection.json (19 PRs / 12 issues), safeStorage (11 PRs / 5 issues), and desktop userData mode (0 results). Two open PRs touch hardening.ts:

• fix(desktop): allow remote gateway token storage on keyring-less Linux #62319 — orthogonal now the migration is gone: it changes encryption policy (encryptDesktopSecret, a new resolvePersistedRemoteToken seam, an opt-in plaintext path), this changes file modes at the write/read choke point. It does not touch writeDesktopConnectionConfig, writeFileAtomic, or either new helper (verified by grep over its diff). Both PRs do append to the same export {} block, so I tested that specific hazard with git merge-file against the common base: exit 0, zero conflict markers, and all six symbols coexist in the merged output. No textual conflict.
• feat(desktop): auto-detect Linux keychain backend for secure token storage #41236 ("feat(desktop): auto-detect Linux keychain backend for secure token storage") — touches bootstrap-platform.ts and main.ts, not hardening.ts or this choke point.

On #74897 (the apparent counter-signal)

#74897 moved write_file new files from 0600 to umask-derived 0644, which superficially points the other way. It does not generalize here, and its own body says why: _atomic_write's chmod branch only ran when the target already existed (if [ -e "$t" ]), so new files silently kept mktemp's 0600 — "invisible in single-user setups, but breaks any cross-process/cross-user reader (Obsidian LiveSync, Docker volumes, NAS mounts)." That 0600 was an accident of a skipped branch, not a policy, and #74897 restored umask-derived permissions for a path the user names, under a documented interop contract. connection.json is an app-private credential file inside Electron's userData that the user never names and no second process is expected to read, and its two immediate neighbours are already 0600. Different contract, opposite default.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[teknium1]

Salvaged and merged via #84898 — your fix + E2E test landed on main with authorship preserved (rebase-merge, f84ecd3). The original branch couldn't rebase-merge due to a merge commit, so it was rebuilt as clean cherry-picks. Live-tested end-to-end before merge: fresh write 0600, rewrite tightens 0644→0600, stale 0666 temp not inherited, startup repair path. Thanks @ZHJay!