Write the CLI server-connection store owner-only, and make its tests run

.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.

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

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: {