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-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); + }); +}); 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 }, });