From c2d946c8040850f8b23fa11d41019f12221df6b8 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:37:58 +0200 Subject: [PATCH 1/2] Write the CLI server-connection store owner-only, and make its tests run server-connections.json holds a bearer token, or an OAuth access token and its long-lived refresh token, rewritten on every silent refresh. It was created with no mode, so the umask applied and it landed world-readable. Create it 0600 with a follow-up chmod, matching the local-server manifest in the same directory. Both steps matter: mode applies only on create, and the chmod covers rewriting an existing looser file -- the common path here. Separately, four tests in this file were it(..., () => Effect.gen(...)). An Effect is not thenable, so vitest passed them without running their bodies; a deliberately falsified assertion still passed. They are now it.effect. --- .../cli-credential-store-permissions.md | 11 +++ apps/cli/src/server-profile.test.ts | 76 +++++++++++++++++-- apps/cli/src/server-profile.ts | 20 ++++- 3 files changed, 98 insertions(+), 9 deletions(-) create mode 100644 .changeset/cli-credential-store-permissions.md diff --git a/.changeset/cli-credential-store-permissions.md b/.changeset/cli-credential-store-permissions.md new file mode 100644 index 0000000000..6763a838fb --- /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 four 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 in the same directory. 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, four 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 2c12ea27a4..95831b0abc 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 27e4d3d159..cf3911eec6 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 in this same directory. + // + // 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: { From 056a9b96ff3c99898560ceeb5778cb80ce288a3c Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:04:08 +0200 Subject: [PATCH 2/2] fix(cli): correct the changeset before it reaches the released changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changeset claimed four tests were revived. It was two. The other two it.effect tests in that file are new ones this PR adds, not revived ones — the PR body already said so two paragraphs further down, so the two contradicted each other. Changeset text ships verbatim into the released changelog, which would have made the wrong count permanent. Also corrects where the sibling secret lives. The local-server manifest keeps it under server-control/, not in the same directory as server-connections.json, so "in the same directory" was wrong in both the changeset and the code comment. --- .changeset/cli-credential-store-permissions.md | 6 +++--- apps/cli/src/server-profile.ts | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.changeset/cli-credential-store-permissions.md b/.changeset/cli-credential-store-permissions.md index 6763a838fb..66572ba16c 100644 --- a/.changeset/cli-credential-store-permissions.md +++ b/.changeset/cli-credential-store-permissions.md @@ -2,10 +2,10 @@ "executor": patch --- -**The CLI's server-connection store is written owner-only, and four of its tests now actually run** +**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 in the same directory. 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. +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, four 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. +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.ts b/apps/cli/src/server-profile.ts index cf3911eec6..44f5a4a813 100644 --- a/apps/cli/src/server-profile.ts +++ b/apps/cli/src/server-profile.ts @@ -154,7 +154,7 @@ export const writeCliServerConnectionStore = ( // 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 in this same directory. + // 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