From 67c730e6166f68c393c2f809bac2173f31704768 Mon Sep 17 00:00:00 2001 From: ConsultingFuture4200 Date: Sat, 25 Jul 2026 10:06:25 -0700 Subject: [PATCH] fix(storage): write userData JSON files owner-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit writeJson backs secrets.json (provider API keys), sessions.json, settings.json and projects.json. It wrote with no mode, so the files landed at the process umask — 0644 normally, 0664 under the umask 002 several distributions ship — leaving their contents readable to anything that reaches them: another account on a shared machine, a backup or sync tool, an archive unpacked elsewhere. The userData directory is usually restrictive enough to cover that on a single-user desktop, but a stored credential shouldn't depend on its parent directory's mode. The mode has to go on the temp file rather than the destination: writeJson writes to a temp path and renames over the target, so the destination inode is replaced on every write and takes the temp file's mode with it. Chmod'ing the destination instead would leave a window where the contents are readable and be undone by the next write — which also means a user who chmod 600'd the file by hand had it silently reset. Because writeFileSync only applies `mode` when it creates the file, the temp path is removed first rather than chmod'd afterwards; that keeps a stale temp file from a crashed write from carrying its old mode through, without adding a syscall that could fail between the write and the rename and lose the data. Existing files are tightened on first read, once per path per run — a key set once and never changed is only ever read, so fixing this on write alone would never reach the installs that already have the problem. That step is best-effort and logs rather than throwing. No behaviour change on Windows, where the mode has no meaning and the tests are skipped. --- app/src/json-store.test.ts | 28 +++++++++++++++++++++++++ app/src/json-store.ts | 43 +++++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/app/src/json-store.test.ts b/app/src/json-store.test.ts index 114634d..62e2259 100644 --- a/app/src/json-store.test.ts +++ b/app/src/json-store.test.ts @@ -34,6 +34,34 @@ describe("json-store", () => { expect(entries).toEqual(["data.json"]); }); + // These files hold API keys and conversation history; the default umask + // would leave them readable by other accounts on the machine. + it.skipIf(process.platform === "win32")("writes owner-only files", () => { + writeJson(file, { token: "value" }); + expect(fs.statSync(file).mode & 0o777).toBe(0o600); + }); + + it.skipIf(process.platform === "win32")("keeps the file owner-only on rewrite", () => { + writeJson(file, { token: "first" }); + writeJson(file, { token: "second" }); + expect(fs.statSync(file).mode & 0o777).toBe(0o600); + }); + + it.skipIf(process.platform === "win32")("stays owner-only when a stale temp file exists", () => { + const stale = `${file}.tmp-${process.pid}`; + fs.writeFileSync(stale, "leftover", { mode: 0o666 }); + writeJson(file, { token: "value" }); + expect(fs.statSync(file).mode & 0o777).toBe(0o600); + }); + + // A file written by an older build is only ever read if its contents never + // change, so tightening on write alone would never reach it. + it.skipIf(process.platform === "win32")("tightens an existing world-readable file on read", () => { + fs.writeFileSync(file, JSON.stringify({ token: "value" }), { mode: 0o644 }); + expect(readJson(file, {})).toEqual({ token: "value" }); + expect(fs.statSync(file).mode & 0o777).toBe(0o600); + }); + it("backs up and falls back to the default when the file is corrupted", () => { fs.writeFileSync(file, "{ not valid json"); const result = readJson(file, { safe: true }); diff --git a/app/src/json-store.ts b/app/src/json-store.ts index 08732fb..99d2378 100644 --- a/app/src/json-store.ts +++ b/app/src/json-store.ts @@ -30,6 +30,8 @@ export function readJson(filePath: string, fallback: T): T { return fallback; } + restrictExistingPermissions(filePath); + try { return JSON.parse(raw) as T; } catch (err) { @@ -43,9 +45,48 @@ export function readJson(filePath: string, fallback: T): T { } } +// These files hold provider API keys (secrets.json) and full conversation +// history. Written with the process umask they land as 0644 — or 0664 under +// the umask 002 several distributions ship — leaving their contents readable +// to anything that reaches them: another account on a shared machine, a +// backup or sync tool, an archive unpacked somewhere else. The userData +// directory is usually restrictive enough to cover that on a single-user +// desktop, but a stored credential shouldn't depend on its parent directory's +// mode. +const PRIVATE_FILE_MODE = 0o600; + +// Files written before this existed keep the mode they were created with, and +// writeJson alone would never reach them: a key set once and never changed is +// only ever read. So the mode is also tightened on first read, once per path +// per run to keep it off the hot path. +const permissionsChecked = new Set(); + +function restrictExistingPermissions(filePath: string): void { + if (process.platform === "win32" || permissionsChecked.has(filePath)) return; + permissionsChecked.add(filePath); + try { + if ((fs.statSync(filePath).mode & 0o777) !== PRIVATE_FILE_MODE) { + fs.chmodSync(filePath, PRIVATE_FILE_MODE); + } + } catch (err) { + // Best effort — unusual ownership or an exotic filesystem must not + // stop the app from reading its own data. + logger.error(`Failed to restrict permissions on ${filePath}: ${(err as Error).message}`); + } +} + export function writeJson(filePath: string, data: unknown): void { fs.mkdirSync(path.dirname(filePath), { recursive: true }); const tmpPath = `${filePath}.tmp-${process.pid}`; - fs.writeFileSync(tmpPath, JSON.stringify(data, null, 2)); + // The mode goes on the temp file, because the rename below replaces the + // destination inode and takes the temp file's mode with it. Chmod'ing the + // destination afterwards would leave a window where the contents are + // readable, and would be undone by the next write. + // + // Removed first so writeFileSync always creates the file and so always + // applies `mode` — it ignores the option for a path that already exists, + // and an interrupted earlier write can leave one behind under this pid. + fs.rmSync(tmpPath, { force: true }); + fs.writeFileSync(tmpPath, JSON.stringify(data, null, 2), { mode: PRIVATE_FILE_MODE }); fs.renameSync(tmpPath, filePath); }