History

Write the CLI server-connection store owner-only, and make its tests run (#1576)

* 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. * fix(cli): correct the changeset before it reaches the released changelog 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. ---------

Sergio Fernández committed 2 months ago

Commit 6535a741 parent 31e17c73 files · +98 −9Browse files

Unified · Split

3 files
  1. .changeset/cli-credential-store-permissions.md +11 −0
  2. apps/cli/src/server-profile.test.ts +71 −5
  3. apps/cli/src/server-profile.ts +16 −4
.changeset/cli-credential-store-permissions.md+11−0
@@ -0,0 +1,11 @@
1+---
2+"executor": patch
3+---
4+
5+**The CLI's server-connection store is written owner-only, and two of its tests now actually run**
6+
7+`~/.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`.
8+
9+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.
10+
11+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+71−5
@@ -1,6 +1,6 @@
11 import { afterEach, describe, expect, it } from "@effect/vitest";
22 import { BunServices } from "@effect/platform-bun";
3−import { mkdtempSync, rmSync } from "node:fs";
3+import { chmodSync, mkdtempSync, rmSync, statSync, writeFileSync } from "node:fs";
44 import { tmpdir } from "node:os";
55 import { join } from "node:path";
66 import * as Effect from "effect/Effect";
@@ -25,7 +25,7 @@ afterEach(() => { · 18 unmodified lines
2525 });
2626
2727 describe("CLI server connection profiles", () => {
28− it("round-trips named server connections and default selection", () =>
28+ it.effect("round-trips named server connections and default selection", () =>
2929 Effect.gen(function* () {
3030 const dataDir = mkdtempSync(join(tmpdir(), "executor-server-profiles-"));
3131 process.env.EXECUTOR_DATA_DIR = dataDir;
@@ -60,9 +60,10 @@ describe("CLI server connection profiles", () => { · 28 unmodified lines
6060 } finally {
6161 rmSync(dataDir, { recursive: true, force: true });
6262 }
63− }).pipe(Effect.provide(BunServices.layer)));
63+ }).pipe(Effect.provide(BunServices.layer)),
64+ );
6465
65− it("switches and removes the default profile", () =>
66+ it.effect("switches and removes the default profile", () =>
6667 Effect.gen(function* () {
6768 const dataDir = mkdtempSync(join(tmpdir(), "executor-server-profiles-"));
6869 process.env.EXECUTOR_DATA_DIR = dataDir;
@@ -88,7 +89,8 @@ describe("CLI server connection profiles", () => { · 19 unmodified lines
8889 } finally {
8990 rmSync(dataDir, { recursive: true, force: true });
9091 }
91− }).pipe(Effect.provide(BunServices.layer)));
92+ }).pipe(Effect.provide(BunServices.layer)),
93+ );
9294
9395 it("drops malformed profiles when parsing", () => {
9496 const store = parseCliServerConnectionStore(
@@ -143,4 +145,68 @@ describe("CLI server connection profiles", () => { · 48 unmodified lines
143145 "CF-Access-Client-Id": { kind: "env", name: "EXECUTOR_CF_ACCESS_CLIENT_ID" },
144146 });
145147 });
148+
149+ // -------------------------------------------------------------------------
150+ // File permissions.
151+ //
152+ // This store holds live credentials for a hosted server — a bearer token, or
153+ // an OAuth access token and its long-lived refresh token. Nothing pinned its
154+ // mode before, so it was created with the process umask (0644 by default) and
155+ // readable by every other account on the machine.
156+ //
157+ // Both cases are covered because they need different mechanisms: `mode` on
158+ // create, and a `chmod` for the overwrite. The overwrite is the common path
159+ // here, since the store is rewritten on every silent token refresh.
160+ // -------------------------------------------------------------------------
161+
162+ it.effect("creates the credential store owner-only", () =>
163+ Effect.gen(function* () {
164+ const dataDir = mkdtempSync(join(tmpdir(), "executor-server-profiles-mode-"));
165+ process.env.EXECUTOR_DATA_DIR = dataDir;
166+
167+ try {
168+ yield* upsertCliServerConnectionProfile({
169+ name: "remote",
170+ connection: {
171+ origin: "https://executor.example",
172+ auth: { kind: "bearer", token: "key_should_not_be_world_readable" },
173+ },
174+ makeDefault: true,
175+ });
176+
177+ const mode = statSync(join(dataDir, "server-connections.json")).mode & 0o777;
178+ expect(mode).toBe(0o600);
179+ } finally {
180+ rmSync(dataDir, { recursive: true, force: true });
181+ }
182+ }).pipe(Effect.provide(BunServices.layer)),
183+ );
184+
185+ it.effect("tightens a pre-existing world-readable credential store on rewrite", () =>
186+ Effect.gen(function* () {
187+ const dataDir = mkdtempSync(join(tmpdir(), "executor-server-profiles-chmod-"));
188+ process.env.EXECUTOR_DATA_DIR = dataDir;
189+ const storePath = join(dataDir, "server-connections.json");
190+
191+ try {
192+ // A store left behind by an older version, world-readable. `mode` on
193+ // write is ignored for an existing file, so only the chmod fixes this.
194+ writeFileSync(storePath, JSON.stringify({ version: 1, profiles: [] }));
195+ chmodSync(storePath, 0o644);
196+
197+ yield* upsertCliServerConnectionProfile({
198+ name: "remote",
199+ connection: {
200+ origin: "https://executor.example",
201+ auth: { kind: "bearer", token: "key_should_not_be_world_readable" },
202+ },
203+ makeDefault: true,
204+ });
205+
206+ expect(statSync(storePath).mode & 0o777).toBe(0o600);
207+ } finally {
208+ rmSync(dataDir, { recursive: true, force: true });
209+ }
210+ }).pipe(Effect.provide(BunServices.layer)),
211+ );
146212 });
apps/cli/src/server-profile.ts+16−4
@@ -150,10 +150,22 @@ export const writeCliServerConnectionStore = ( · 149 unmodified lines
150150 const path = yield* Path.Path;
151151 const dataDir = resolveDataDir(path);
152152 yield* fs.makeDirectory(dataDir, { recursive: true });
153− yield* fs.writeFileString(
154− serverConnectionStorePath(path),
155− serializeCliServerConnectionStore(store),
156− );
153+ const storePath = serverConnectionStorePath(path);
154+ // This store holds live credentials for a hosted server — a bearer token, or
155+ // an OAuth access token AND its long-lived refresh token — so create it
156+ // owner-only, exactly as the local-server manifest does for the sibling
157+ // secret it keeps under `server-control/`.
158+ //
159+ // Both steps are needed, and the second matters more here than it does
160+ // there. `mode` applies only when the file is CREATED, so it closes the
161+ // window where a fresh store is briefly world-readable. The `chmod` covers
162+ // overwriting a store that already exists with looser permissions — and this
163+ // file is rewritten on every silent token refresh, so overwrite is the
164+ // common path, not the rare one.
165+ yield* fs.writeFileString(storePath, serializeCliServerConnectionStore(store), {
166+ mode: 0o600,
167+ });
168+ yield* fs.chmod(storePath, 0o600).pipe(Effect.ignore);
157169 });
158170
159171 export const upsertCliServerConnectionProfile = (input: {