From dd06af5f6c9e252d5b2974f744ef217c2cf854ad Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:45:39 +0200 Subject: [PATCH 1/2] Write the desktop settings store owner-only settings.json holds serverProfiles, which carries a remote server's bearer token or basic-auth password. conf defaults to 0o666, so the file was created 0644 -- readable by any other local account on Linux, where ~/.config is not reliably 0700. One option suffices: atomically chmods the temp inode when the requested mode differs from its default, and the rename carries it onto an existing loose file. Verified against the installed conf: no option -> 0644 fresh; configFileMode 0o600 -> 0600 fresh AND 0600 over an existing 0644 file. No automated test: this module and electron-store both import electron, which throws outside an Electron runtime, which is why the desktop package has no tests for these modules at all. --- .changeset/desktop-settings-owner-only.md | 9 +++++++++ apps/desktop/src/main/settings.ts | 10 ++++++++++ 2 files changed, 19 insertions(+) create mode 100644 .changeset/desktop-settings-owner-only.md diff --git a/.changeset/desktop-settings-owner-only.md b/.changeset/desktop-settings-owner-only.md new file mode 100644 index 0000000000..959e7fabc3 --- /dev/null +++ b/.changeset/desktop-settings-owner-only.md @@ -0,0 +1,9 @@ +--- +"executor": patch +--- + +**The desktop settings store is written owner-only** + +`settings.json` holds `serverProfiles`, which carries a remote server's credential — a bearer token, or a basic-auth username and password — for any "Custom server" the user connects to. `conf` (under electron-store) defaults to `configFileMode: 0o666`, so with no explicit mode the file was created `0644`. On Linux, where `~/.config` is not reliably `0700`, that is readable by every other account on the machine. macOS is protected by `~/Library` being `0700` and Windows by ACLs, so this is primarily a Linux-desktop exposure — but owner-only credential files are already this app's standard: `local-auth.ts` writes `auth.json` at `0o600`, and the sidecar manifest is chmodded the same way. + +One option is sufficient here, rather than the mode-plus-chmod pair used elsewhere: `atomically` chmods the temp inode only when the requested mode differs from its own default, and the atomic rename then carries the tight mode onto an already-loose file. Verified against the installed `conf` — with no option a fresh file lands `0644`; with `configFileMode: 0o600` a fresh file lands `0600` and an existing `0644` file becomes `0600` on the next write. diff --git a/apps/desktop/src/main/settings.ts b/apps/desktop/src/main/settings.ts index ff1c45856c..0de03d26fa 100644 --- a/apps/desktop/src/main/settings.ts +++ b/apps/desktop/src/main/settings.ts @@ -12,6 +12,16 @@ const store = new Store({ ...(process.env.EXECUTOR_DESKTOP_SETTINGS_DIR ? { cwd: process.env.EXECUTOR_DESKTOP_SETTINGS_DIR } : {}), + // `serverProfiles` carries a remote server's credential — a bearer token, or a + // basic-auth password — so this file is owner-only, the same standard + // `local-auth.ts` already applies to `auth.json` in this app. + // + // Without this, `conf` defaults to `0o666` and the file lands 0644 under a + // normal umask. One option is enough: `atomically` only chmods the temp inode + // when the requested mode DIFFERS from its own default, so setting it here is + // what turns that chmod on, and the atomic rename then carries the tight mode + // onto an existing loose file too. + configFileMode: 0o600, defaults: { server: DEFAULT_SERVER_SETTINGS }, }); From 15b664fb12130336bfe619d0165c4111e14b4a96 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:25:20 +0200 Subject: [PATCH 2/2] test(desktop): pin that one option is enough to write the settings store 0600 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR shipped with no test and a paragraph explaining why testing it is impractical. That was true of the WIRING — settings.ts builds its store at module scope and pulls in electron, which cannot be imported outside an Electron runtime — but it was never true of the part carrying the risk. The fix rests on a claim about a transitive dependency: that `configFileMode` alone is enough, where the rest of this app uses a mode-plus-chmod pair, because `atomically` chmods the temp inode and the rename carries the tight mode onto an already-loose file. A claim about a dependency is worth checking rather than repeating, and it can be checked without importing settings.ts at all — by driving the real electron-store with the same options. Three tests: a fresh store is created 0600, an existing 0644 store is tightened on rewrite, and — the control — without the option the file really is world-readable. The control is what stops the first two passing for free on a machine with a restrictive umask. Mutation-checked: setting the option to 0o666 fails the first two and leaves the control green. What it deliberately does NOT bind is the wiring in settings.ts, and the file says so rather than leaving it to be discovered. --- .../src/main/settings-permissions.test.ts | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 apps/desktop/src/main/settings-permissions.test.ts diff --git a/apps/desktop/src/main/settings-permissions.test.ts b/apps/desktop/src/main/settings-permissions.test.ts new file mode 100644 index 0000000000..42264fc950 --- /dev/null +++ b/apps/desktop/src/main/settings-permissions.test.ts @@ -0,0 +1,92 @@ +// --------------------------------------------------------------------------- +// The desktop settings store must be owner-only. +// +// `serverProfiles` carries a remote server's credential — a bearer token, or a +// basic-auth password — and `conf` (under electron-store) defaults to +// `configFileMode: 0o666`, so without an explicit mode the file lands 0644 under +// a normal umask. On Linux, where `~/.config` is not reliably 0700, that is +// readable by every other account on the machine. +// +// WHAT THIS TEST DOES AND DOES NOT BIND, stated plainly rather than left to be +// discovered. It exercises the real `electron-store` with the SAME options +// `settings.ts` passes, so it verifies the load-bearing uncertainty in that fix: +// that ONE option is enough, where the rest of this app uses a mode-plus-chmod +// pair. That shortcut rests on a claim about a transitive dependency — +// `atomically` chmods the temp inode only when the requested mode differs from +// its own default, and the atomic rename then carries the tight mode onto an +// already-loose file — and a claim about a dependency is worth checking rather +// than repeating. +// +// It does NOT bind the wiring in `settings.ts`. That module builds its store at +// module scope and pulls in `electron`, which cannot be imported outside an +// Electron runtime (`electron/index.js` throws from `getElectronPath`). That is +// why the repo has no `settings.test.ts` at all. Mocking `electron-store` away +// would leave the test asserting the mock rather than the behaviour that is +// actually in question, so the option itself is verified here and the wiring is +// left uncovered on purpose. +// --------------------------------------------------------------------------- + +import { describe, expect, it, onTestFinished } from "@effect/vitest"; +import Store from "electron-store"; +import { chmodSync, mkdtempSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +/** The exact options `settings.ts` constructs its store with. */ +const makeStore = (dir: string) => + new Store<{ readonly serverProfiles?: string }>({ + name: "settings", + cwd: dir, + configFileMode: 0o600, + defaults: {}, + }); + +const settingsPathIn = (dir: string) => join(dir, "settings.json"); + +/** A temp dir removed when the test ends. `try/finally` would do the same, but the repo's + * no-try-catch-or-throw rule rejects the construct and a registered hook reads better. */ +const tempDir = (prefix: string) => { + const dir = mkdtempSync(join(tmpdir(), prefix)); + onTestFinished(() => rmSync(dir, { recursive: true, force: true })); + return dir; +}; + +describe("electron-store configFileMode", () => { + it("creates the settings file owner-only", () => { + const dir = tempDir("executor-desktop-settings-"); + + makeStore(dir).set("serverProfiles", JSON.stringify({ token: "sk-secret" })); + + expect(statSync(settingsPathIn(dir)).mode & 0o777).toBe(0o600); + }); + + it("tightens a settings file that already exists world-readable", () => { + // This is the half that would silently not happen if the single option did + // not trigger `atomically`'s chmod: an existing 0644 file from a build + // without the mode set must not stay 0644 after the next write. + const dir = tempDir("executor-desktop-settings-loose-"); + + writeFileSync(settingsPathIn(dir), JSON.stringify({ serverProfiles: "old" })); + chmodSync(settingsPathIn(dir), 0o644); + + makeStore(dir).set("serverProfiles", JSON.stringify({ token: "sk-secret" })); + + expect(statSync(settingsPathIn(dir)).mode & 0o777).toBe(0o600); + }); + + it("without the option, the file really is world-readable — the defect this pins", () => { + // A control. Without it, both assertions above would pass just as happily on + // a machine with a restrictive umask, and would prove nothing about the fix. + const dir = tempDir("executor-desktop-settings-default-"); + + new Store<{ readonly serverProfiles?: string }>({ + name: "settings", + cwd: dir, + defaults: {}, + }).set("serverProfiles", JSON.stringify({ token: "sk-secret" })); + + const mode = statSync(settingsPathIn(dir)).mode & 0o777; + expect(mode).not.toBe(0o600); + expect(mode & 0o044).not.toBe(0); + }); +});