From 7e151bd9d3cb1daa8f4c7897acb39b9ade7be42a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E8=B1=AA=E6=9D=B0?= Date: Mon, 3 Aug 2026 18:53:26 +0800 Subject: [PATCH] fix(desktop): create connection.json owner-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 51c68d4ab, 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 (d3d177283, d208f2c2c) 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 6e01add6578f08f015a567d3a7a7378f2ec3e768) --- .../e2e/at-rest-connection-token.spec.ts | 720 ++++++++++++++++++ apps/desktop/electron/hardening.test.ts | 350 ++++++++- apps/desktop/electron/hardening.ts | 121 ++- apps/desktop/electron/main.ts | 36 +- 4 files changed, 1218 insertions(+), 9 deletions(-) create mode 100644 apps/desktop/e2e/at-rest-connection-token.spec.ts diff --git a/apps/desktop/e2e/at-rest-connection-token.spec.ts b/apps/desktop/e2e/at-rest-connection-token.spec.ts new file mode 100644 index 0000000000000..63ebab3bda153 --- /dev/null +++ b/apps/desktop/e2e/at-rest-connection-token.spec.ts @@ -0,0 +1,720 @@ +/** + * E2E at-rest contract for the remote-gateway session token (issue #77486). + * + * The reported bug: configuring a remote gateway persisted the dashboard + * session token as PLAINTEXT into `connection.json` under the app's userData + * dir (macOS `~/Library/Application Support/Hermes/connection.json`, Windows + * `AppData\Roaming\Hermes\connection.json`). Anything that can read the file + * — a backup, a sync client, another local process, a support bundle — got a + * live gateway credential. + * + * The contract these tests encode is deliberately stated WITHOUT naming a + * storage strategy: + * + * 1. ABSENT FROM DISK. After the app has been configured with a remote + * gateway token, the token's plaintext value must not appear anywhere in + * `connection.json`, in any sibling file the app writes under userData, + * or in HERMES_HOME (logs included). + * 2. STILL FUNCTIONAL. After a restart, the app must still be able to USE + * that credential — it decrypts the stored blob and puts the exact + * original token on the wire. + * + * Both halves matter and neither is sufficient alone. (1) alone is trivially + * satisfied by a "fix" that drops the token on the floor; (2) alone is + * satisfied by the bug itself. So (2) is verified through the app's own + * connection test against a fake gateway that records the + * `X-Hermes-Session-Token` header it receives — a dropped or mangled token + * cannot produce that header. + * + * We deliberately do NOT assert `encoding === 'safeStorage'` or any other + * shape of the stored blob. That would be a change-detector: a fix that moved + * to the OS keychain proper, to an async safeStorage provider, or to a + * separate credential file would break the test while being *more* correct. + * The load-bearing assertion is the raw-bytes absence of the secret. + * + * Two at-rest paths, hence two tests — one enforced, one a documented gap: + * + * 1. A NEWLY configured token (ACTIVE). The app's own write path routes + * through the strict `encryptDesktopSecret`; this test holds it there + * against regression. + * 2. An EXISTING plaintext `connection.json` (`test.fixme`). Legacy payloads + * are deliberately NOT migrated yet. The test is kept, disabled, with a + * precise reason — see the block comment above it. + * + * ── Correcting the record on (2) ──────────────────────────────────────── + * + * An earlier revision of this file asserted that migration and justified it by + * claiming the first implementation (`d3d177283`) fell back to + * `{ encoding: 'plain', value }` when `isEncryptionAvailable()` was false. + * That citation is FALSE for this codebase. What is actually true: + * + * git merge-base --is-ancestor d3d1772837a7b0552940b55455ae734c72e0a8f1 HEAD -> 1 (NOT an ancestor) + * git merge-base --is-ancestor 51c68d4ab1a9e3c62fb1048fccb84144c409f0e7 HEAD -> 0 (IS an ancestor) + * git log -S 'Fall through to plaintext' upstream/main -- apps/desktop -> (no commits) + * + * `d3d177283` exists only on `upstream/bb/gui-mainmerge-tmp`, + * `brooklyn/gui-installer-prereqs`, and the `desktop-pr20059-installers` + * pre-release tag. Mainline NEVER shipped a code path that wrote a plaintext + * gateway token: `51c68d4ab` ("Add Hermes desktop app (#20059)"), the commit + * that brought the desktop app to mainline, already contained the strict + * throw ("Secure token storage is unavailable, …") in `hardening.cjs`. + * + * One `{ encoding: 'plain', value }` literal does remain on mainline + * (`electron/main.ts`, in `coerceDesktopConnectionConfig`), but it is + * unreachable as an at-rest write: it is gated on `persistToken === false`, + * whose only caller is the connection-TEST handler, which never calls + * `writeDesktopConnectionConfig`. That token stays in memory for the duration + * of one probe. + * + * So the affected population is not "anyone who configured a gateway before + * the fix". It is narrow and non-mainline: pre-release `bb/gui` installs + * (including the `desktop-pr20059-installers` build) plus hand-edited or + * hand-migrated `connection.json` files. Those files DO still work, because + * `decryptDesktopSecret` returns any non-safeStorage `value` verbatim on read + * — the read path is intentionally unchanged, so nobody is signed out. That + * read-path acceptance, not a mainline writer, is what makes the fixme'd + * fixture realistic. + * + * Migration is DEFERRED, not forgotten. An adversarial review of the + * migration that briefly lived here returned DO NOT SHIP, having reproduced + * two token-loss scenarios: it silently reverts and then destroys the opt-in + * plaintext choice that open upstream PR #62319 deliberately adds; and it + * converts a portable credential into a keychain-bound one with no consent, + * destroying the only recoverable copy while not actually remediating the + * exposure (the plaintext is already in backups, so the real remedy is + * ROTATION). The prerequisites are enumerated above test 2. + * + * Environment limits are encoded rather than papered over. Electron's + * safeStorage is unavailable on Linux with no keyring, which is the shape of + * this suite's CI runner (ubuntu-latest, see .github/workflows/e2e-desktop.yml). + * The absence assertion is unconditional there — it is the security + * requirement, and it must hold in every environment. Only the *other* half is + * conditional: with secure storage the save must succeed, and without it the + * save must fail loudly (which is what the current strict `encryptDesktopSecret` + * does) instead of quietly writing plaintext. See the branch comments in each + * test for the reasoning, including the one case this spec refuses to invent a + * policy for. + * + * Prerequisite: `npm run build` must have been run so dist/ exists. + */ + +import * as fs from 'node:fs' +import * as http from 'node:http' +import type { AddressInfo } from 'node:net' +import * as path from 'node:path' + +import { buildAppEnv, createSandbox, launchDesktop, type Sandbox } from './fixtures' +import { allowErrorBanners, type ElectronApplication, expect, type Page, test } from './test' + +/** + * The secret under test. Long, random-looking, and unique to this spec so a + * raw-bytes scan cannot produce a false negative by colliding with ordinary + * config content. Kept to `[A-Za-z0-9-]` on purpose: encodeURIComponent() is + * the identity function over this alphabet, so the raw-bytes needle also + * covers the URL-encoded form the WS dialer builds (`?token=…`). + */ +const SENTINEL_TOKEN = 'hermes-e2e-at-rest-sentinel-Zq7Z4hV9nX2pL8sK3tB6wR1yM5jD0fG' + +/** Skip absurdly large files during the leak scan (Chromium caches). */ +const MAX_SCAN_BYTES = 16 * 1024 * 1024 + +/** + * One fixed Electron app name for this spec, instead of the timestamped one + * `buildAppEnv` generates. On macOS the safeStorage keychain item is derived + * from the app name, so a per-launch name would (a) make the post-restart + * decrypt fail for the wrong reason and (b) leave a fresh keychain entry on + * the developer's login keychain on every run. Safe because the suite runs + * one worker at a time and both launches here are sequential; the + * single-instance lock keys off userData, which is per-sandbox. + */ +const STABLE_APP_NAME = 'HermesE2EAtRestStorage' + +// ─── Fake gateway ─────────────────────────────────────────────────────── + +interface FakeGateway { + url: string + /** Every `X-Hermes-Session-Token` value the app has sent us. */ + sessionTokens: string[] + close: () => Promise +} + +/** + * A minimal stand-in for a remote Hermes gateway. It serves the public + * `/api/status` probe (which the desktop connection test hits first, with the + * session token in a header) and refuses the WebSocket upgrade immediately so + * the second leg of the connection test fails fast instead of burning the + * probe's 10s connect timeout. We only care about the header it captured. + * + * The e2e mock-server is an OpenAI-compatible *inference* mock, not a gateway, + * so it cannot answer /api/status — hence this small local server. + */ +async function startFakeGateway(): Promise { + const sessionTokens: string[] = [] + + const server = http.createServer((req, res) => { + const token = req.headers['x-hermes-session-token'] + + if (typeof token === 'string' && token) { + sessionTokens.push(token) + } + + if (req.url?.startsWith('/api/status')) { + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify({ auth_required: false, ok: true, version: '0.0.0-e2e-fake' })) + + return + } + + res.writeHead(404, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify({ detail: 'not found' })) + }) + + // Refuse the WS leg at once: the connection test's WS probe should return a + // fast failure rather than hang. The status header is already captured. + server.on('upgrade', (req, socket) => { + const token = new URL(req.url ?? '/', 'http://127.0.0.1').searchParams.get('token') + + if (token) { + sessionTokens.push(token) + } + + socket.destroy() + }) + + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)) + + const { port } = server.address() as AddressInfo + + return { + close: () => + new Promise(resolve => { + server.closeAllConnections?.() + server.close(() => resolve()) + }), + sessionTokens, + url: `http://127.0.0.1:${port}`, + } +} + +// ─── On-disk leak scanning ────────────────────────────────────────────── + +interface Needle { + bytes: Buffer + label: string +} + +/** + * The forms a leak could take. Raw bytes, not JSON.parse + field inspection: + * the point is that the secret is nowhere in the file — including inside a + * nested field, a cached WS URL, or a field name nobody thought to check. + * + * The base64 needle catches the cheapest wrong "fix": base64 is an encoding, + * not encryption, so a token that is merely base64'd is still plaintext at + * rest. A real ciphertext will contain neither needle. + */ +function secretNeedles(secret: string): Needle[] { + return [ + { bytes: Buffer.from(secret, 'utf8'), label: 'plaintext' }, + { bytes: Buffer.from(Buffer.from(secret, 'utf8').toString('base64'), 'utf8'), label: 'base64' }, + ] +} + +/** Relative paths of every file under `root` whose bytes contain a needle. */ +function scanTreeForSecret(root: string, needles: Needle[]): string[] { + const hits: string[] = [] + + const walk = (dir: string): void => { + let entries: fs.Dirent[] + + try { + entries = fs.readdirSync(dir, { withFileTypes: true }) + } catch { + return + } + + for (const entry of entries) { + const full = path.join(dir, entry.name) + + if (entry.isDirectory()) { + walk(full) + + continue + } + + if (!entry.isFile()) { + continue + } + + try { + if (fs.statSync(full).size > MAX_SCAN_BYTES) { + continue + } + } catch { + continue + } + + let buf: Buffer + + try { + buf = fs.readFileSync(full) + } catch { + continue + } + + for (const needle of needles) { + if (buf.includes(needle.bytes)) { + hits.push(`${path.relative(root, full)} [${needle.label}]`) + } + } + } + } + + walk(root) + + return hits +} + +/** + * Read a file's bytes, or an empty buffer when it does not exist. A correct + * fix is allowed to delete/replace `connection.json` rather than rewrite it, + * and the refusal path may never create it at all — neither should crash the + * scan before its assertion runs. + */ +function readIfExists(filePath: string): Buffer { + try { + return fs.readFileSync(filePath) + } catch { + return Buffer.alloc(0) + } +} + +/** + * The stored token's `encoding` tag, for diagnostics only — never its value. + * Reported on failure so a red run says *why* (e.g. still `plain`) instead of + * only that a scan matched. Deliberately NOT an assertion: which encoding a + * correct fix chooses is its own business. + */ +function storedTokenEncoding(connectionFile: string): string { + try { + const parsed = JSON.parse(readIfExists(connectionFile).toString('utf8')) + + return String(parsed?.remote?.token?.encoding ?? '') + } catch { + return '' + } +} + +// ─── App helpers ──────────────────────────────────────────────────────── + +/** + * Launch the desktop app against `sandbox` with a fake boot failure injected. + * + * The credential path we are testing is entirely main-process (IPC handler → + * coerce → safeStorage → userData write) and does not need a live agent + * backend, so we skip spawning `hermes serve` (no Python needed, ~3s launch, + * hermetic). This is also a real user situation rather than an artificial one: + * the boot-failure overlay's own recovery affordance is "Connection settings", + * i.e. pointing the app at a remote gateway is exactly what a user does from + * this state. BOOT_FAKE_ERROR short-circuits startHermes() *before* remote + * resolution, so no launch ever dials the fake gateway on its own. + */ +async function launchAgainst(sandbox: Sandbox): Promise<{ app: ElectronApplication; page: Page }> { + const env = buildAppEnv(sandbox, { + HERMES_DESKTOP_APP_NAME: STABLE_APP_NAME, + HERMES_DESKTOP_BOOT_FAKE_ERROR: 'E2E at-rest storage spec: local backend intentionally not started', + }) + + const { app, page } = await launchDesktop(env) + + // The capability bridge is what we drive; it lands with the preload, well + // before the app would be "ready" in the boot sense. + await page.waitForFunction( + () => Boolean((window as unknown as { hermesDesktop?: Record }).hermesDesktop?.saveConnectionConfig), + undefined, + { timeout: 60_000 }, + ) + + return { app, page } +} + +/** + * Ask the running app where userData actually is, the same way the app does + * (`app.getPath('userData')`). The fixtures point userData at a temp sandbox, + * so a home-relative hardcoded path would test the wrong file — or no file. + */ +async function resolveUserDataDir(app: ElectronApplication): Promise { + return app.evaluate(({ app: electronApp }) => electronApp.getPath('userData')) +} + +interface SafeStorageCapability { + available: boolean + backend: string +} + +/** + * What secure storage is actually capable of on THIS host, asked after ready + * (on Linux the answer is meaningless before then). + * + * `backend` matters for the honest reading of a green run: on Linux with no + * keyring, Electron can still report encryption as available while selecting + * the `basic_text` backend, which encrypts with a hardcoded password — the + * bytes on disk are not the plaintext, but they are not meaningfully + * protected either. We record it rather than assert on it, because which + * posture Hermes should take there (refuse to save vs. accept basic_text) is + * a product decision, not something this test should silently ratify. + */ +async function readSafeStorageCapability(app: ElectronApplication): Promise { + return app.evaluate(async ({ app: electronApp, safeStorage }) => { + await electronApp.whenReady() + + let available = false + let backend = 'unavailable' + + try { + available = safeStorage.isEncryptionAvailable() + } catch { + available = false + } + + try { + // Linux-oriented API; other platforms may not implement it. + backend = safeStorage.getSelectedStorageBackend?.() ?? 'n/a' + } catch { + backend = 'n/a' + } + + return { available, backend } + }) +} + +interface SaveOutcome { + config: { remoteTokenPreview?: null | string; remoteTokenSet?: boolean; remoteUrl?: string } | null + error: null | string +} + +/** + * Drive the app's REAL save surface: the same `saveConnectionConfig` payload + * Settings → Gateway sends (see src/app/settings/gateway-settings.tsx). We use + * save rather than apply so the app persists the credential without trying to + * re-home onto the fake gateway. + */ +async function saveRemoteToken(page: Page, remoteUrl: string, remoteToken?: string): Promise { + return page.evaluate( + async ([url, token]) => { + const desktop = (window as unknown as { hermesDesktop: any }).hermesDesktop + + try { + const config = await desktop.saveConnectionConfig({ + mode: 'remote', + remoteAuthMode: 'token', + ...(token ? { remoteToken: token } : {}), + remoteUrl: url, + }) + + return { config, error: null } + } catch (error) { + return { config: null, error: error instanceof Error ? error.message : String(error) } + } + }, + [remoteUrl, remoteToken ?? ''] as const, + ) +} + +/** + * Make the app USE the stored credential. No token in the payload, so the main + * process must read `connection.json`, decrypt what it stored, and put the + * plaintext on the wire itself. `buildRemoteBlock` throws "Remote gateway + * session token is required." when the stored blob no longer decrypts, so a + * fix that dropped the token fails here instead of quietly passing the + * absence assertion. + */ +async function exerciseStoredToken(page: Page, remoteUrl: string): Promise<{ error: null | string }> { + return page.evaluate(async url => { + const desktop = (window as unknown as { hermesDesktop: any }).hermesDesktop + + try { + await desktop.testConnectionConfig({ mode: 'remote', remoteUrl: url }) + + return { error: null } + } catch (error) { + // A failing WS leg is expected (the fake gateway refuses the upgrade). + // The assertion is on what the gateway received, not on this result. + return { error: error instanceof Error ? error.message : String(error) } + } + }, remoteUrl) +} + +// ─── Tests ────────────────────────────────────────────────────────────── + +let gateway: FakeGateway | null = null +let sandbox: Sandbox | null = null +let app: ElectronApplication | null = null + +test.beforeAll(async () => { + gateway = await startFakeGateway() +}) + +test.afterAll(async () => { + await gateway?.close() + gateway = null +}) + +test.beforeEach(() => { + // Boot is intentionally failed in this spec (see launchAgainst), so the + // boot-failure overlay's error banner is expected, not a failure. + allowErrorBanners() +}) + +test.afterEach(async () => { + await app?.close().catch(() => undefined) + app = null + sandbox?.cleanup() + sandbox = null +}) + +test.describe('remote gateway session token at rest', () => { + test('a newly configured token is never written to userData in plaintext, and still works after restart', async () => { + const fake = gateway! + sandbox = createSandbox('at-rest-fresh') + + const first = await launchAgainst(sandbox) + app = first.app + + const capability = await readSafeStorageCapability(app) + const userDataDir = await resolveUserDataDir(app) + const connectionFile = path.join(userDataDir, 'connection.json') + + test.info().annotations.push({ + description: `isEncryptionAvailable=${capability.available} backend=${capability.backend}`, + type: 'safeStorage', + }) + + const saved = await saveRemoteToken(first.page, fake.url, SENTINEL_TOKEN) + + // Defined degradation, not a silent plaintext write. Where secure storage + // works, the save must succeed. Where it genuinely does not (headless + // Linux with no keyring, per Electron's safeStorage docs), refusing the + // save with a loud error is an acceptable outcome — what is NEVER + // acceptable is reporting success while leaving the secret readable on + // disk. The absence assertion below runs in both branches. + if (capability.available) { + expect( + saved.error, + 'secure storage is available on this host, so saving a remote gateway token must succeed', + ).toBeNull() + expect(saved.config?.remoteTokenSet).toBe(true) + } else { + expect( + saved.error, + 'secure storage is unavailable, so the save must fail loudly rather than persist a plaintext token', + ).not.toBeNull() + } + + // Guard against a vacuous pass: when the save succeeded, the artifact must + // exist and must be the file the app really wrote for THIS connection. + // Without this, "no plaintext on disk" would also be true if nothing had + // been saved at all. Only asserted on the success branch — a refused save + // legitimately leaves no file behind. + const rawConnection = readIfExists(connectionFile) + + if (capability.available) { + expect(fs.existsSync(connectionFile), `expected the app to write ${connectionFile}`).toBe(true) + expect( + rawConnection.includes(Buffer.from(fake.url, 'utf8')), + 'connection.json should record the configured gateway URL (proves this is the real artifact)', + ).toBe(true) + } + + // ── The load-bearing assertion ───────────────────────────────────── + const needles = secretNeedles(SENTINEL_TOKEN) + + const connectionHits = needles.filter(needle => rawConnection.includes(needle.bytes)).map(needle => needle.label) + expect( + connectionHits, + `the gateway session token must not be recoverable from ${connectionFile} ` + + `(stored token encoding is "${storedTokenEncoding(connectionFile)}")`, + ).toEqual([]) + + // …and not in any sibling file the app writes alongside it, nor in + // HERMES_HOME (desktop.log lives there). + expect( + scanTreeForSecret(userDataDir, needles), + 'the gateway session token leaked into a userData file', + ).toEqual([]) + expect( + scanTreeForSecret(sandbox.hermesHome, needles), + 'the gateway session token leaked into a HERMES_HOME file (logs included)', + ).toEqual([]) + + if (!capability.available) { + // Nothing was stored, so there is no round trip to verify. The refusal + // itself was already asserted above. + return + } + + // ── Secondary: the credential must still be USABLE ───────────────── + // Restart against the same userData so the token comes off disk, not out + // of a live process's memory. + await app.close().catch(() => undefined) + app = null + + const second = await launchAgainst(sandbox) + app = second.app + + expect( + await resolveUserDataDir(app), + 'the restarted app must resolve the same userData dir, or this is not a round trip', + ).toBe(userDataDir) + + const reread = await second.page.evaluate(async () => { + const desktop = (window as unknown as { hermesDesktop: any }).hermesDesktop + + return desktop.getConnectionConfig() + }) + + expect(reread.remoteTokenSet, 'the stored token must survive a restart').toBe(true) + expect(reread.remoteUrl).toBe(fake.url) + + const before = fake.sessionTokens.length + await exerciseStoredToken(second.page, fake.url) + + // The gateway is the witness: the app decrypted its stored blob and put + // the original secret on the wire. A dropped, truncated, or re-encoded + // token cannot produce this. + expect( + fake.sessionTokens.slice(before), + 'the app must send the exact stored token to the gateway after a restart', + ).toContain(SENTINEL_TOKEN) + }) + + /** + * DEFERRED GAP — legacy plaintext payloads are not migrated. + * + * Held as `fixme` rather than deleted: the fixture below is the correct + * fixture for the population that a migration must eventually cover, and + * the harness (seed → boot-poll → authoritative re-save → raw-bytes scan → + * wire check) is the harness such a migration needs. Keeping it typechecked + * and listed makes the gap visible in `--list` and in every report; deleting + * it would make the gap invisible and cost the next implementer this setup. + * + * It is NOT enabled because the migration it asserted was reviewed + * DO NOT SHIP. Before this can be un-fixme'd, three prerequisites (see the + * header, and the matching note in electron/main.ts readDesktopConnectionConfig): + * + * 1. Sequence with #62319's opt-in plaintext marker, so a user who + * deliberately chose plaintext is not silently overridden. This + * fixture has NO marker, so it stays in scope for migration — but the + * implementation must be able to tell the two apart. + * 2. Write through the config sanitizer, not around it. + * 3. Surface ROTATION guidance. Re-encrypting cannot un-expose a secret + * that is already in a backup; it only prevents future exposure. + * + * Un-fixme'ing this without (1) risks destroying a deliberate user choice, + * and without (3) it reports a remediation it did not actually perform. + */ + test('an existing plaintext connection.json is migrated off plaintext and keeps working', async () => { + test.fixme( + true, + 'Deferred: legacy plaintext connection.json is intentionally NOT migrated. ' + + 'Affected population is pre-release bb/gui installs (incl. the desktop-pr20059-installers build) ' + + 'plus hand-edited configs — mainline never wrote a plaintext gateway token. ' + + 'Blocked on: (1) #62319 opt-in-marker coordination, (2) writing through the config sanitizer, ' + + '(3) surfacing token-rotation guidance. Re-encrypting alone does not remediate an already-backed-up secret.', + ) + + const fake = gateway! + sandbox = createSandbox('at-rest-migrate') + + // Seed the file an affected user has on disk. This is live, usable + // plaintext rather than a strawman, because `decryptDesktopSecret` returns + // `value` verbatim for any non-safeStorage encoding — the READ path + // accepts it. Note what does NOT justify this fixture: mainline never + // WROTE this shape to disk. `coerceDesktopConnectionConfig` does build it, + // but only under `persistToken: false`, whose sole caller is the + // connection-test handler, which never persists. The writers were + // non-mainline pre-release builds and hand edits. There is deliberately no + // opt-in marker here, so this payload is in scope for a future migration. + fs.writeFileSync( + path.join(sandbox.userDataDir, 'connection.json'), + JSON.stringify( + { + mode: 'remote', + profiles: {}, + remote: { + authMode: 'token', + token: { encoding: 'plain', value: SENTINEL_TOKEN }, + url: fake.url, + }, + }, + null, + 2, + ), + 'utf8', + ) + + const launched = await launchAgainst(sandbox) + app = launched.app + + const capability = await readSafeStorageCapability(app) + + test.info().annotations.push({ + description: `isEncryptionAvailable=${capability.available} backend=${capability.backend}`, + type: 'safeStorage', + }) + + if (!capability.available) { + // With no secure storage there is nowhere to migrate the secret TO, and + // scrubbing it would silently sign the user out of a working gateway. + // Asserting either outcome here would be inventing policy. + test.skip( + true, + 'secure storage unavailable on this host — the correct migration policy for an existing plaintext file is undecided', + ) + + return + } + + const userDataDir = await resolveUserDataDir(app) + const connectionFile = path.join(userDataDir, 'connection.json') + const needles = secretNeedles(SENTINEL_TOKEN) + + // Two chances, so the test does not depend on WHERE the fix hooks the + // migration: (a) on read at boot, (b) on the next authoritative write. + // Poll for (a) first. + const deadline = Date.now() + 15_000 + let stillPlaintext = true + + while (Date.now() < deadline) { + stillPlaintext = readIfExists(connectionFile).includes(needles[0].bytes) + + if (!stillPlaintext) { + break + } + + await launched.page.waitForTimeout(500) + } + + if (stillPlaintext) { + // (b) A real save through the app's own surface, carrying no new token — + // the stored blob is inherited. Re-persisting an inherited secret is the + // other place plaintext must not survive. + const resaved = await saveRemoteToken(launched.page, fake.url) + expect(resaved.error, 'a re-save that inherits the stored token must not fail').toBeNull() + } + + expect( + scanTreeForSecret(userDataDir, needles), + 'an existing plaintext gateway token must not remain readable under userData after the app has run ' + + `(stored token encoding is still "${storedTokenEncoding(connectionFile)}")`, + ).toEqual([]) + + // And the migration must not have cost the user their credential. + const before = fake.sessionTokens.length + await exerciseStoredToken(launched.page, fake.url) + + expect( + fake.sessionTokens.slice(before), + 'the migrated token must still reach the gateway unchanged', + ).toContain(SENTINEL_TOKEN) + }) +}) diff --git a/apps/desktop/electron/hardening.test.ts b/apps/desktop/electron/hardening.test.ts index 0549f8cef28fa..8f24af37c10d8 100644 --- a/apps/desktop/electron/hardening.test.ts +++ b/apps/desktop/electron/hardening.test.ts @@ -20,9 +20,54 @@ import { resolveReadableFileForIpc, resolveRequestedPathForIpc, resolveTimeoutMs, - sensitiveFileBlockReason + SAFE_STORAGE_ENCODING, + SECRET_FILE_MODE, + sensitiveFileBlockReason, + tightenSecretFileMode, + writeSecretFileAtomic } from './hardening' +/** + * Real temp dir per test: the property under test IS the on-disk mode after a + * temp-file-then-rename, which a mocked fs would assert into existence rather + * than verify. `platform` is still injected so the Windows branch is coverable + * from a POSIX run. + */ +function withTempDir(run: (dir: string) => void) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'hermes-secret-file-')) + + try { + run(dir) + } finally { + fs.rmSync(dir, { force: true, recursive: true }) + } +} + +function modeOf(filePath: string) { + return fs.statSync(filePath).mode & 0o777 +} + +/** + * No file other than the target may survive a write, and nothing left in the + * directory may contain the payload. Asserts the CONTRACT (no readable debris) + * instead of a literal directory listing, so adding a lock file or renaming the + * staging file does not break the test. + */ +function assertNoSecretDebris(dir: string, targetName: string, secret: string) { + for (const name of fs.readdirSync(dir)) { + if (name === targetName) { + continue + } + + // Substring, not RegExp: a real token can contain regex metacharacters. + assert.equal( + fs.readFileSync(path.join(dir, name), 'utf8').includes(secret), + false, + `leftover file ${name} still contains the secret` + ) + } +} + async function rejectsWithCode(promise, code: string) { await assert.rejects(promise, (error: any) => { assert.equal(error?.code, code) @@ -99,9 +144,27 @@ test('encryptDesktopSecret stores safeStorage base64 payload', () => { encryptString: value => Buffer.from(`enc:${value}`, 'utf8') }) - assert.deepEqual(secret, { - encoding: 'safeStorage', - value: Buffer.from('enc:token-123', 'utf8').toString('base64') + // Contract: the payload is tagged with the SAME constant main's + // decryptDesktopSecret dispatches on, and `value` is the keychain ciphertext + // base64'd — not the token itself. + assert.equal(secret?.encoding, SAFE_STORAGE_ENCODING) + assert.equal(Buffer.from(String(secret?.value), 'base64').toString('utf8'), 'enc:token-123') + assert.doesNotMatch(String(secret?.value), /token-123/, 'the plaintext is not recoverable from the payload') +}) + +// ─── Owner-only credential files (connection.json) ───────────────────────── + +test('writeSecretFileAtomic creates the file owner-only, not at the 0644 umask default', () => { + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + const payload = JSON.stringify({ remote: { token: { encoding: SAFE_STORAGE_ENCODING, value: 'BLOB' } } }) + + writeSecretFileAtomic(target, payload) + + assert.equal(modeOf(target), SECRET_FILE_MODE) + assert.equal(modeOf(target) & 0o077, 0, 'no group/other bits') + assert.equal(fs.readFileSync(target, 'utf8'), payload, 'content round-trips') + assertNoSecretDebris(dir, 'connection.json', 'BLOB') }) }) @@ -318,6 +381,285 @@ test('resolvePersistedRemoteToken keeps the existing token when no new token is assert.equal(called, false, 'an empty incoming token must not re-encrypt anything') }) +test('writeSecretFileAtomic does not inherit loose bits from a stale temp file', () => { + // renameSync keeps the TEMP file's permissions, and writeFileSync's `mode` + // is ignored when the path already exists — so a temp left by a crashed + // earlier write would otherwise hand 0644 straight to the target. + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + fs.writeFileSync(`${target}.tmp`, 'stale', { mode: 0o666 }) + assert.notEqual(modeOf(`${target}.tmp`), SECRET_FILE_MODE) + + writeSecretFileAtomic(target, 'fresh') + + assert.equal(modeOf(target), SECRET_FILE_MODE) + assert.equal(fs.readFileSync(target, 'utf8'), 'fresh') + }) +}) + +/** + * Owner-only is carried by two independent mechanisms — the create-time `mode` + * and the chmod before the rename — because each covers a case the other + * cannot. The next two tests knock out one mechanism at a time (with the REAL + * fs doing the actual write, so the assertion is still the on-disk mode) and + * require the survivor to hold the line on its own. Without them, either + * mechanism could be deleted with every test still green. + */ +function fsWith(overrides: Record) { + return { ...fs, ...overrides } as any +} + +test('the written file is owner-only even where chmod does nothing', () => { + // Windows, and any mount that refuses chmod. The create-time `mode` is what + // covers this — there is no second chance to tighten. + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + // Pin a permissive umask so a mode-less create WOULD land + // group/other-readable — otherwise a restrictive-umask host could pass this + // for free. Synchronous and restored in `finally`, and the electron project + // runs one process per file, so no other test observes it. + const previousUmask = process.umask(0o022) + + try { + const witness = path.join(dir, 'witness.json') + fs.writeFileSync(witness, 'x') + assert.notEqual(modeOf(witness), SECRET_FILE_MODE, 'the ambient default is NOT already owner-only') + + writeSecretFileAtomic(target, 'tok', { fs: fsWith({ chmodSync: () => void 0 }) }) + } finally { + process.umask(previousUmask) + } + + assert.equal(modeOf(target), SECRET_FILE_MODE, 'created owner-only, not tightened after the fact') + }) +}) + +test('the written file is owner-only even when a stale temp cannot be removed', () => { + // The unlink is best-effort; if the stale temp survives, writeFileSync's + // `mode` is ignored on an existing path and only the chmod before the rename + // can still fix the bits. + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + fs.writeFileSync(`${target}.tmp`, 'stale', { mode: 0o666 }) + + writeSecretFileAtomic(target, 'tok', { fs: fsWith({ rmSync: () => void 0 }) }) + + assert.equal(modeOf(target), SECRET_FILE_MODE, 'tightened before the rename handed the bits over') + assert.equal(fs.readFileSync(target, 'utf8'), 'tok') + }) +}) + +test('writeSecretFileAtomic cannot be redirected through a symlink planted at the temp path', () => { + // A stale temp path is attacker-controllable in a shared temp/userData dir. + // Following it would write the token into the victim file AND then rename the + // link over connection.json, so every later write leaks too. + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + const victim = path.join(dir, 'victim.txt') + fs.writeFileSync(victim, 'original', { mode: 0o644 }) + + try { + fs.symlinkSync(victim, `${target}.tmp`, 'file') + } catch (error: any) { + if (error?.code === 'EPERM' || error?.code === 'EACCES') { + return + } + + throw error + } + + writeSecretFileAtomic(target, 'tok-live-42') + + assert.equal(fs.readFileSync(victim, 'utf8'), 'original', 'the symlink target was not written through') + assert.equal(modeOf(victim), 0o644, 'the victim file was not chmodded either') + assert.equal(fs.readFileSync(target, 'utf8'), 'tok-live-42') + assert.equal(fs.lstatSync(target).isSymbolicLink(), false, 'the target is a real file, not the planted link') + assert.equal(modeOf(target), SECRET_FILE_MODE) + }) +}) + +test('tightenSecretFileMode tightens a pre-existing world-readable config in place', () => { + // The upgrade path: a connection.json written by an older build sits at 0644 + // with a real (encrypted) token in it. Tightening must change the mode and + // nothing else — the token has to stay readable or the user loses their + // configured gateway. + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + + const legacy = JSON.stringify({ + mode: 'remote', + remote: { + url: 'https://gw.example.com', + authMode: 'token', + token: { encoding: SAFE_STORAGE_ENCODING, value: 'BLOB' } + } + }) + + fs.writeFileSync(target, legacy, { mode: 0o644 }) + assert.equal(modeOf(target), 0o644) + + assert.equal(tightenSecretFileMode(target), true) + + assert.equal(modeOf(target), SECRET_FILE_MODE) + assert.deepEqual(JSON.parse(fs.readFileSync(target, 'utf8')), JSON.parse(legacy), 'contents untouched') + }) +}) + +test('tightenSecretFileMode leaves a non-safeStorage token payload readable', () => { + // A hand-edited config (or one from a pre-release build) can hold a + // non-safeStorage token payload, which decryptDesktopSecret still reads + // verbatim on purpose. Tightening the mode must not disturb that fallback — + // it only narrows who can open the file. + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + + const legacyPlain = JSON.stringify({ + mode: 'remote', + remote: { url: 'https://gw.example.com', authMode: 'token', token: { encoding: 'plain', value: 'tok-live-42' } } + }) + + fs.writeFileSync(target, legacyPlain, { mode: 0o644 }) + + tightenSecretFileMode(target) + + assert.equal(modeOf(target), SECRET_FILE_MODE) + assert.equal(JSON.parse(fs.readFileSync(target, 'utf8')).remote.token.value, 'tok-live-42') + }) +}) + +test('tightenSecretFileMode is idempotent and never throws on an unusable path', () => { + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + writeSecretFileAtomic(target, '{}') + + assert.equal(tightenSecretFileMode(target), true) + assert.equal(tightenSecretFileMode(target), true) + assert.equal(modeOf(target), SECRET_FILE_MODE) + + // Missing file (fresh install, nothing saved yet) reports failure quietly + // instead of breaking the read path it is called from. + assert.equal(tightenSecretFileMode(path.join(dir, 'absent.json')), false) + }) +}) + +test('tightenSecretFileMode refuses to chmod a symlink instead of following it to its target', () => { + // Matches readInstallationId in desktop-installation.ts. Without the lstat + // guard a link planted at the config path sends the chmod to whatever it + // resolves to — someone else's file gets its mode rewritten. + withTempDir(dir => { + const target = path.join(dir, 'connection.json') + const victim = path.join(dir, 'victim.txt') + fs.writeFileSync(victim, 'not mine', { mode: 0o644 }) + + try { + fs.symlinkSync(victim, target, 'file') + } catch (error: any) { + if (error?.code === 'EPERM' || error?.code === 'EACCES') { + return + } + + throw error + } + + assert.equal(tightenSecretFileMode(target), false, 'reports "not tightened" rather than acting on the link') + assert.equal(modeOf(victim), 0o644, 'the symlink target keeps its own mode') + }) +}) + +test('tightenSecretFileMode only touches a regular file the current user owns', () => { + // Directories, sockets, fifos and files owned by another account are all + // "not ours to chmod". Injected lstat so the foreign-owner branch is + // reachable without a second OS account. + const chmodded: string[] = [] + + const fakeFs = (stat: Record) => + ({ + chmodSync: (filePath: string) => void chmodded.push(filePath), + lstatSync: () => ({ isFile: () => true, isSymbolicLink: () => false, mode: 0o644, uid: 0, ...stat }), + renameSync: () => void 0, + rmSync: () => void 0, + writeFileSync: () => void 0 + }) as any + + const uid = typeof process.getuid === 'function' ? process.getuid() : 0 + + assert.equal( + tightenSecretFileMode('/x/connection.json', { fs: fakeFs({ isFile: () => false }), platform: 'linux' }), + false + ) + assert.equal( + tightenSecretFileMode('/x/connection.json', { fs: fakeFs({ uid: uid + 1 }), platform: 'linux' }), + false, + 'a file owned by another user is left alone' + ) + assert.deepEqual(chmodded, [], 'nothing was chmodded on the rejected paths') + + // The same fs shape, but ours and loose: now it tightens. + assert.equal(tightenSecretFileMode('/x/connection.json', { fs: fakeFs({ uid }), platform: 'linux' }), true) + assert.deepEqual(chmodded, ['/x/connection.json']) +}) + +test('tightenSecretFileMode leaves Windows alone rather than flipping the read-only bit', () => { + const chmods: string[] = [] + + const fakeFs = { + chmodSync: (filePath: string) => void chmods.push(filePath), + lstatSync: () => ({ + isFile: () => true, + isSymbolicLink: () => false, + mode: 0o644, + uid: typeof process.getuid === 'function' ? process.getuid() : 0 + }), + renameSync: () => void 0, + rmSync: () => void 0, + writeFileSync: () => void 0 + } as any + + assert.equal(tightenSecretFileMode('C:\\Users\\me\\connection.json', { fs: fakeFs, platform: 'win32' }), true) + assert.deepEqual(chmods, [], 'no chmod on win32') + + // Same fs, POSIX: the chmod does happen, proving the platform gate is what + // suppressed it above. + assert.equal(tightenSecretFileMode('/home/me/connection.json', { fs: fakeFs, platform: 'linux' }), true) + assert.ok(chmods.includes('/home/me/connection.json'), 'the POSIX path was tightened') +}) + +test('a token is never persisted in plaintext when safeStorage is unavailable', () => { + // The defined degradation for `isEncryptionAvailable() === false` (Linux with + // no keyring): encryptDesktopSecret throws with an actionable message, so the + // save aborts before any write. It must never fall back to a plaintext + // payload — the file mode is defense in depth, not a substitute for the + // keychain. + const unavailable = { + isEncryptionAvailable: () => false, + encryptString: () => Buffer.from('unused', 'utf8') + } + + assert.throws( + () => encryptDesktopSecret('tok-live-42', unavailable), + (error: unknown) => { + assert.ok(error instanceof Error, 'aborts instead of returning a payload') + assert.match(String((error as Error).message), /Secure token storage is unavailable/) + assert.doesNotMatch(String((error as Error).message), /tok-live-42/, 'the secret is not echoed in the error') + + return true + } + ) + + // And a throwing keychain (available, but encryptString fails) is the same + // contract — no silent plaintext. + assert.throws( + () => + encryptDesktopSecret('tok-live-42', { + isEncryptionAvailable: () => true, + encryptString: () => { + throw new Error('keyring locked') + } + }), + /Failed to encrypt the remote gateway token/ + ) +}) + test('sensitiveFileBlockReason blocks obvious secret file patterns', () => { assert.match(String(sensitiveFileBlockReason('/tmp/.env')), /\.env/) assert.equal(sensitiveFileBlockReason('/tmp/.env.example'), null) diff --git a/apps/desktop/electron/hardening.ts b/apps/desktop/electron/hardening.ts index 9df4fd3db4ec5..b885dda1afffa 100644 --- a/apps/desktop/electron/hardening.ts +++ b/apps/desktop/electron/hardening.ts @@ -35,6 +35,119 @@ function dataUrlReadMaxBytesFromMb(maxMb) { const SAFE_ENV_SUFFIXES = new Set(['dist', 'example', 'sample', 'template']) const SENSITIVE_EXTENSIONS = new Set(['.kdbx', '.p12', '.pem', '.pfx']) +// Owner-only mode for userData files that carry credentials (the encrypted +// gateway token in connection.json, and the URL/SSH fields alongside it). +// connection.json was the odd one out: its two credential-bearing neighbours +// under userData are already 0600 — desktop-installation.json +// (desktop-installation.ts) and native-oauth-tokens.json (main.ts +// `_nativeTokenStoreIo`) — while connection.json was written with no mode at +// all and landed at the 0644 umask default. This makes the three consistent. +const SECRET_FILE_MODE = 0o600 + +// The encoding tag that marks a payload as OS-encrypted. One constant because +// the writer (encryptDesktopSecret, here) and the reader +// (decryptDesktopSecret, in main.ts) have to agree on the exact string across +// a file boundary, and the native-token store round-trips the same shape. +const SAFE_STORAGE_ENCODING = 'safeStorage' + +interface SecretFileFs { + chmodSync: typeof fs.chmodSync + lstatSync: typeof fs.lstatSync + renameSync: typeof fs.renameSync + rmSync: typeof fs.rmSync + writeFileSync: typeof fs.writeFileSync +} + +interface SecretFileOptions { + encoding?: BufferEncoding + fs?: SecretFileFs + platform?: string +} + +/** + * Tighten an existing credential file to owner-only (0600), returning whether + * the file now has that mode. + * + * Exists because `fs.writeFileSync(path, data, { mode })` only applies `mode` + * when it CREATES the file — rewriting an existing path silently keeps the old + * bits. So a file already on disk at 0644 (every connection.json written + * before this change, since that write passed no mode at all) needs an explicit + * chmod; a fresh `mode:` alone would never tighten it. + * + * Guards match `readInstallationId` in desktop-installation.ts, which does the + * same job for the sibling userData credential file: only ever chmod a regular + * file we own, never a symlink and never another user's file. Without them a + * symlink planted at the path would send the chmod to whatever it resolves to. + * + * POSIX only. Windows has no meaningful chmod (Node maps it to the read-only + * bit), and userData there is already ACL'd to the user profile, so we report + * success without touching the file rather than flipping it read-only and + * breaking the next write. + * + * Never throws: a chmod can legitimately fail (read-only mount, file owned by + * another user), and failing to tighten a file is not a reason to lose the + * user's configured gateway. + */ +function tightenSecretFileMode(filePath, options: SecretFileOptions = {}) { + const fsImpl = options.fs || fs + const platform = options.platform || process.platform + + if (platform === 'win32') { + return true + } + + try { + const stat = fsImpl.lstatSync(filePath) + + if (!stat.isFile() || stat.isSymbolicLink()) { + return false + } + + if (typeof process.getuid === 'function' && stat.uid !== process.getuid()) { + return false + } + + if ((stat.mode & 0o777) === SECRET_FILE_MODE) { + return true + } + + fsImpl.chmodSync(filePath, SECRET_FILE_MODE) + + return true + } catch { + return false + } +} + +/** + * Atomically write a credential file, owner-only wherever the OS expresses + * permissions as mode bits. + * + * On POSIX the file is owner-only from the moment it exists. On Windows this + * only gets the atomic rename: `tightenSecretFileMode` no-ops there (Node maps + * chmod to the read-only bit), so the file inherits the userData directory's + * ACL rather than an explicit owner-only one. Tightening Windows ACLs is being + * handled once, for the Python `_secure_file`, in PR #77527 — the desktop + * should follow that rather than start a second ACL story here. + * + * The temp-then-rename dance is what makes the mode subtle: `renameSync` keeps + * the TEMP file's permissions, so writing the temp at the default umask (0644) + * hands those bits to the target — and a crashed earlier write can leave a + * stale temp file whose existing loose bits `mode:` will not correct. Hence the + * unlink of any stale temp (which also drops a planted symlink, so the write + * cannot be redirected), the create-time `mode`, and the chmod before the + * rename. + */ +function writeSecretFileAtomic(targetPath, data, options: SecretFileOptions = {}) { + const fsImpl = options.fs || fs + const tmp = targetPath + '.tmp' + + fsImpl.rmSync(tmp, { force: true }) + fsImpl.writeFileSync(tmp, data, { encoding: options.encoding, mode: SECRET_FILE_MODE }) + tightenSecretFileMode(tmp, options) + fsImpl.renameSync(tmp, targetPath) +} + function resolveTimeoutMs(timeoutMs, fallbackMs = DEFAULT_FETCH_TIMEOUT_MS) { const fallback = Number.isFinite(fallbackMs) && Number(fallbackMs) > 0 ? Math.round(Number(fallbackMs)) : DEFAULT_FETCH_TIMEOUT_MS @@ -86,7 +199,7 @@ function encryptDesktopSecret(value, safeStorageApi, options: { allowPlainText?: try { return { - encoding: 'safeStorage', + encoding: SAFE_STORAGE_ENCODING, value: safeStorageApi.encryptString(raw).toString('base64') } } catch (error) { @@ -440,6 +553,10 @@ export { resolveReadableFileForIpc, resolveRequestedPathForIpc, resolveTimeoutMs, + SAFE_STORAGE_ENCODING, + SECRET_FILE_MODE, sensitiveFileBlockReason, - TEXT_PREVIEW_SOURCE_MAX_BYTES + TEXT_PREVIEW_SOURCE_MAX_BYTES, + tightenSecretFileMode, + writeSecretFileAtomic } diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index d908d5fa18538..201c22919ce9c 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -142,7 +142,10 @@ import { resolveReadableFileForIpc, resolveRequestedPathForIpc, resolveTimeoutMs, - TEXT_PREVIEW_SOURCE_MAX_BYTES + SAFE_STORAGE_ENCODING, + TEXT_PREVIEW_SOURCE_MAX_BYTES, + tightenSecretFileMode, + writeSecretFileAtomic } from './hardening' import { cursorPointInWindow } from './hud-cursor' import { snapHudBounds } from './hud-snap' @@ -6676,7 +6679,7 @@ function decryptDesktopSecret(secret) { return '' } - if (secret.encoding === 'safeStorage') { + if (secret.encoding === SAFE_STORAGE_ENCODING) { try { return safeStorage.decryptString(Buffer.from(value, 'base64')) } catch { @@ -6684,6 +6687,10 @@ function decryptDesktopSecret(secret) { } } + // Any other encoding (a hand-edited config, or one written by a pre-release + // build) is returned verbatim on purpose: this fallback is what lets such a + // config connect at all. Not a plaintext-writing path — nothing in this file + // persists a token this way. return value } @@ -6789,6 +6796,22 @@ function readDesktopConnectionConfig() { const raw = fs.readFileSync(DESKTOP_CONNECTION_CONFIG_PATH, 'utf8') const parsed = JSON.parse(raw) + // Tighten an install written before this file was owner-only. Every write + // now goes out at 0600, but a file already on disk keeps its old 0644 bits + // until something chmods it, and waiting for the user's next Settings save + // would leave it group/other-readable indefinitely. Runs on a cache miss + // only (once per launch, plus after an external edit); chmod moves ctime, + // not mtime, so it cannot invalidate the cache it sits inside. + tightenSecretFileMode(DESKTOP_CONNECTION_CONFIG_PATH) + + // NOT done here: migrating a legacy non-safeStorage token payload to + // ciphertext at rest. Deferred deliberately — it has to honor the opt-in + // plaintext choice PR #62319 adds (re-encrypting it converts a portable + // credential into a keychain-bound one and can lose the token), write + // through sanitizeConnectionProfiles below rather than persisting raw + // `parsed`, and tell the user to ROTATE, since every existing backup copy + // still holds the old secret. Do not add it without those three. + if (parsed && typeof parsed === 'object') { const remote = parsed.remote && typeof parsed.remote === 'object' ? parsed.remote : {} // authMode lives on the remote sub-object: 'oauth' (cookie + ws-ticket) @@ -6816,7 +6839,14 @@ function readDesktopConnectionConfig() { function writeDesktopConnectionConfig(config) { fs.mkdirSync(path.dirname(DESKTOP_CONNECTION_CONFIG_PATH), { recursive: true }) - writeFileAtomic(DESKTOP_CONNECTION_CONFIG_PATH, JSON.stringify(config, null, 2)) + // Owner-only, not writeFileAtomic: this is the single choke point for every + // connection.json write (the IPC save/apply handlers and + // persistSshConnectionToken all land here), and the file carries the + // safeStorage-encrypted gateway token plus its URL and SSH host/user/keyPath. + // safeStorage keeps the token opaque; 0600 keeps the whole record — and the + // fields that are NOT encrypted — off other local accounts, matching + // native-oauth-tokens.json and desktop-installation.json. + writeSecretFileAtomic(DESKTOP_CONNECTION_CONFIG_PATH, JSON.stringify(config, null, 2)) connectionConfigCache = config connectionConfigCacheMtime = fs.statSync(DESKTOP_CONNECTION_CONFIG_PATH).mtimeMs }