diff --git a/.changeset/cli-credential-store-permissions.md b/.changeset/cli-credential-store-permissions.md new file mode 100644 index 000000000..66572ba16 --- /dev/null +++ b/.changeset/cli-credential-store-permissions.md @@ -0,0 +1,11 @@ +--- +"executor": patch +--- + +**The CLI's server-connection store is written owner-only, and two of its tests now actually run** + +`~/.executor/server-connections.json` holds live credentials for a hosted server — a bearer token, or an OAuth access token together with its long-lived refresh token, rewritten on every silent refresh. It was created with no explicit mode, so the process umask applied and it landed world-readable (0644 by default). Any other account on the machine — or anything that copies a home directory, such as a backup or a container layer — could read a durable credential until the user ran `executor logout`. + +It is now created `0600` with a follow-up `chmod`, matching what the local-server manifest already does for the sibling secret it keeps under `server-control/`. Both steps are needed: `mode` applies only on create, and the `chmod` covers rewriting a store that already exists with looser permissions — which is the common path here, since the file is rewritten on every token refresh. + +Separately, two tests in `server-profile.test.ts` were written as `it("…", () => Effect.gen(…))`. An `Effect` is not a thenable, so Vitest treated each as passing without ever running its body — a deliberately falsified assertion still passed. They are now `it.effect` and execute for real. No production behaviour was wrong; the tests simply were not checking it. diff --git a/apps/cli/src/server-profile.test.ts b/apps/cli/src/server-profile.test.ts index 2c12ea27a..95831b0ab 100644 --- a/apps/cli/src/server-profile.test.ts +++ b/apps/cli/src/server-profile.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it } from "@effect/vitest"; import { BunServices } from "@effect/platform-bun"; -import { mkdtempSync, rmSync } from "node:fs"; +import { chmodSync, mkdtempSync, rmSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import * as Effect from "effect/Effect"; @@ -25,7 +25,7 @@ afterEach(() => { }); describe("CLI server connection profiles", () => { - it("round-trips named server connections and default selection", () => + it.effect("round-trips named server connections and default selection", () => Effect.gen(function* () { const dataDir = mkdtempSync(join(tmpdir(), "executor-server-profiles-")); process.env.EXECUTOR_DATA_DIR = dataDir; @@ -60,9 +60,10 @@ describe("CLI server connection profiles", () => { } finally { rmSync(dataDir, { recursive: true, force: true }); } - }).pipe(Effect.provide(BunServices.layer))); + }).pipe(Effect.provide(BunServices.layer)), + ); - it("switches and removes the default profile", () => + it.effect("switches and removes the default profile", () => Effect.gen(function* () { const dataDir = mkdtempSync(join(tmpdir(), "executor-server-profiles-")); process.env.EXECUTOR_DATA_DIR = dataDir; @@ -88,7 +89,8 @@ describe("CLI server connection profiles", () => { } finally { rmSync(dataDir, { recursive: true, force: true }); } - }).pipe(Effect.provide(BunServices.layer))); + }).pipe(Effect.provide(BunServices.layer)), + ); it("drops malformed profiles when parsing", () => { const store = parseCliServerConnectionStore( @@ -143,4 +145,68 @@ describe("CLI server connection profiles", () => { "CF-Access-Client-Id": { kind: "env", name: "EXECUTOR_CF_ACCESS_CLIENT_ID" }, }); }); + + // ------------------------------------------------------------------------- + // File permissions. + // + // This store holds live credentials for a hosted server — a bearer token, or + // an OAuth access token and its long-lived refresh token. Nothing pinned its + // mode before, so it was created with the process umask (0644 by default) and + // readable by every other account on the machine. + // + // Both cases are covered because they need different mechanisms: `mode` on + // create, and a `chmod` for the overwrite. The overwrite is the common path + // here, since the store is rewritten on every silent token refresh. + // ------------------------------------------------------------------------- + + it.effect("creates the credential store owner-only", () => + Effect.gen(function* () { + const dataDir = mkdtempSync(join(tmpdir(), "executor-server-profiles-mode-")); + process.env.EXECUTOR_DATA_DIR = dataDir; + + try { + yield* upsertCliServerConnectionProfile({ + name: "remote", + connection: { + origin: "https://executor.example", + auth: { kind: "bearer", token: "key_should_not_be_world_readable" }, + }, + makeDefault: true, + }); + + const mode = statSync(join(dataDir, "server-connections.json")).mode & 0o777; + expect(mode).toBe(0o600); + } finally { + rmSync(dataDir, { recursive: true, force: true }); + } + }).pipe(Effect.provide(BunServices.layer)), + ); + + it.effect("tightens a pre-existing world-readable credential store on rewrite", () => + Effect.gen(function* () { + const dataDir = mkdtempSync(join(tmpdir(), "executor-server-profiles-chmod-")); + process.env.EXECUTOR_DATA_DIR = dataDir; + const storePath = join(dataDir, "server-connections.json"); + + try { + // A store left behind by an older version, world-readable. `mode` on + // write is ignored for an existing file, so only the chmod fixes this. + writeFileSync(storePath, JSON.stringify({ version: 1, profiles: [] })); + chmodSync(storePath, 0o644); + + yield* upsertCliServerConnectionProfile({ + name: "remote", + connection: { + origin: "https://executor.example", + auth: { kind: "bearer", token: "key_should_not_be_world_readable" }, + }, + makeDefault: true, + }); + + expect(statSync(storePath).mode & 0o777).toBe(0o600); + } finally { + rmSync(dataDir, { recursive: true, force: true }); + } + }).pipe(Effect.provide(BunServices.layer)), + ); }); diff --git a/apps/cli/src/server-profile.ts b/apps/cli/src/server-profile.ts index 27e4d3d15..44f5a4a81 100644 --- a/apps/cli/src/server-profile.ts +++ b/apps/cli/src/server-profile.ts @@ -150,10 +150,22 @@ export const writeCliServerConnectionStore = ( const path = yield* Path.Path; const dataDir = resolveDataDir(path); yield* fs.makeDirectory(dataDir, { recursive: true }); - yield* fs.writeFileString( - serverConnectionStorePath(path), - serializeCliServerConnectionStore(store), - ); + const storePath = serverConnectionStorePath(path); + // This store holds live credentials for a hosted server — a bearer token, or + // an OAuth access token AND its long-lived refresh token — so create it + // owner-only, exactly as the local-server manifest does for the sibling + // secret it keeps under `server-control/`. + // + // Both steps are needed, and the second matters more here than it does + // there. `mode` applies only when the file is CREATED, so it closes the + // window where a fresh store is briefly world-readable. The `chmod` covers + // overwriting a store that already exists with looser permissions — and this + // file is rewritten on every silent token refresh, so overwrite is the + // common path, not the rare one. + yield* fs.writeFileString(storePath, serializeCliServerConnectionStore(store), { + mode: 0o600, + }); + yield* fs.chmod(storePath, 0o600).pipe(Effect.ignore); }); export const upsertCliServerConnectionProfile = (input: {