diff --git a/TODO.md b/TODO.md index 4fb1db5f..ea100690 100644 --- a/TODO.md +++ b/TODO.md @@ -307,6 +307,21 @@ everything still open, in the order it should be tackled. work. (Recorded 2026-08-20.) - [ ] **Are vault files protected from other users on Windows?** + SCOPE GREW 2026-08-20: this is no longer two Python tests. When + the JavaScript suite first ran on Windows, eight more `0600` + assertions failed for the same reason — the vault index itself, + index backups, the parent-seed phrase file, job profiles, the + recovery-drill key and log, 2FA exports, and the semantic index, + which holds stored secrets. All are now guarded by + `NO_POSIX_PERMISSIONS` in js/packages/cli/test/helpers/platform.ts + with the reason attached. + + Skipping states the gap, it does not close it. With TypeScript + becoming the only implementation, "are the user's secrets + protected from other accounts on Windows" is a shipping question, + not a test-suite question. Either verify the inherited ACL is + user-scoped and assert THAT on Windows, or set an explicit ACL and + assert it. `test_atomic_write_permissions` and `test_index_files_are_not_readable_by_other_users` assert mode 0o600 and are now skipped on Windows, because POSIX mode bits do diff --git a/js/packages/cli/src/api/routes.ts b/js/packages/cli/src/api/routes.ts index c8e039e2..0fb7471c 100644 --- a/js/packages/cli/src/api/routes.ts +++ b/js/packages/cli/src/api/routes.ts @@ -10,7 +10,7 @@ import { readFile, mkdir, rm } from "node:fs/promises"; import { existsSync } from "node:fs"; -import { join, resolve as resolvePath } from "node:path"; +import { join, resolve as resolvePath, relative, isAbsolute, sep } from "node:path"; import { addDocumentEntry, addKeyValueEntry, @@ -264,7 +264,20 @@ function optInt(value: unknown, field: string): number | undefined { function resolveWithinProfile(ctx: ApiContext, raw: string): string { const root = resolvePath(profileDir(ctx)); const target = resolvePath(root, raw); - if (target !== root && !target.startsWith(root + "/")) { + // `relative`, not string prefixes. The previous check was + // `target.startsWith(root + "/")` with a hardcoded forward slash, which on + // Windows never matched anything: resolve() returns + // `C:\profile\file.txt` and the comparison asked for `C:\profile/`. It + // failed CLOSED, so nothing leaked — it simply refused every path, + // including the legitimate ones, which meant document export and import + // did not work on Windows at all. Found the first time this suite ran + // there. + // + // A path inside root has a relative form that is neither empty-with-`..` + // nor absolute; that holds on every platform and needs no separator of our + // own choosing. + const rel = relative(root, target); + if (rel !== "" && (rel === ".." || rel.startsWith(`..${sep}`) || isAbsolute(rel))) { throw new HttpError(400, "path must stay inside the profile directory"); } return target; diff --git a/js/packages/cli/src/sinks.ts b/js/packages/cli/src/sinks.ts index c22a20a7..6a2d1e68 100644 --- a/js/packages/cli/src/sinks.ts +++ b/js/packages/cli/src/sinks.ts @@ -56,6 +56,9 @@ export function sinkEnv( * one quoted string (`--exec "wc -c"`) is the documented way to include * flags; split it here. * + * On Windows a backslash is a path separator and never an escape; on POSIX + * it escapes, as a shell would. Quotes group on both. + * * Quotes group, they do not pass through. Splitting on whitespace alone was * wrong for the single most common sink command there is: * `--exec 'sh -c "do the thing"'` handed sh the two tokens `"do` and @@ -63,15 +66,24 @@ export function sinkEnv( * tokenization only — no shell runs here, so there is no expansion, * globbing, or substitution, and a `$VAR` inside the spec stays literal. */ -export function parseCommandSpec(spec: string[]): [string, string[]] { - const parts = spec.length === 1 && /\s/.test(spec[0]!) ? tokenize(spec[0]!) : spec; +export function parseCommandSpec( + spec: string[], + // Explicit rather than read from process.platform, so BOTH branches are + // testable from either operating system. The Windows branch could + // otherwise only ever run on Windows, which is exactly how the bug below + // survived until this repository started testing there. + options: { windows?: boolean } = {}, +): [string, string[]] { + const onWindows = options.windows ?? process.platform === "win32"; + const parts = + spec.length === 1 && /\s/.test(spec[0]!) ? tokenize(spec[0]!, onWindows) : spec; const [cmd, ...args] = parts; if (!cmd) throw new Error("empty command"); return [cmd, args]; } /** Whitespace split that honours '...', "...", and backslash escapes. */ -function tokenize(spec: string): string[] { +function tokenize(spec: string, onWindows: boolean): string[] { const tokens: string[] = []; let current = ""; let started = false; @@ -90,9 +102,13 @@ function tokenize(spec: string): string[] { quote = ch; } else if (ch === quote) { quote = null; - } else if (ch === "\\" && quote !== "'" && i + 1 < spec.length) { - // Backslash escapes the next character, except inside single quotes - // where POSIX makes it literal. + } else if (ch === "\\" && !onWindows && quote !== "'" && i + 1 < spec.length) { + // POSIX: backslash escapes the next character, except inside single + // quotes. NOT on Windows, where it is the path separator -- treating it + // as an escape there turned `C:\\Program Files\\tool.exe` into + // `C:Program` plus `Filestool.exe`, so every --exec and --stdin-to + // naming a real Windows path was silently mangled before it was + // spawned. Found when this suite first ran on Windows. current += spec[++i]!; } else { current += ch; diff --git a/js/packages/cli/test/agentSecurity.test.ts b/js/packages/cli/test/agentSecurity.test.ts index 2cbb8f31..f7218fae 100644 --- a/js/packages/cli/test/agentSecurity.test.ts +++ b/js/packages/cli/test/agentSecurity.test.ts @@ -343,9 +343,24 @@ describe("second-audit findings", () => { } }); socket.on("error", (e) => { + // EPIPE and ECONNRESET here are the SERVER DOING ITS JOB, not a + // failure. It caps the line, answers, and hangs up — and on macOS + // that hangup lands while this client is still pushing the remaining + // megabyte, so the write fails. On Linux the socket buffer swallows + // the whole payload first and the client never notices. Treating the + // client's own broken pipe as fatal made this test pass on one + // platform and fail on the other for the same correct behaviour. + const code = (e as NodeJS.ErrnoException).code; + if (code === "EPIPE" || code === "ECONNRESET") return; clearTimeout(timer); reject(e); }); + socket.on("close", () => { + // Only reaches here if the reply never arrived: `data` resolves and + // destroys the socket as soon as it sees a newline. + clearTimeout(timer); + reject(new Error("the daemon closed the connection without replying")); + }); }); expect(reply["ok"]).toBe(false); expect(String(reply["error"])).toContain("too large"); @@ -527,7 +542,13 @@ describe("a use-scoped token cannot crash the agent", () => { const r = await rawRequest({ op: "use-sink", fingerprint: FINGERPRINT, id, token: useToken, - sink: "stdin", command: ["/bin/true"], + // `process.execPath` rather than /bin/true: that path does not exist on + // macOS (it is /usr/bin/true), so the spawn failed with ENOENT and the + // request was refused — which looked like the crash-safety fix having + // regressed when it had not. The Node binary running this test exists + // on every platform by definition, and `-e ""` exits immediately + // without reading stdin, which is exactly the case under test. + sink: "stdin", command: [process.execPath, "-e", ""], }); expect(r["ok"]).toBe(true); diff --git a/js/packages/cli/test/api.test.ts b/js/packages/cli/test/api.test.ts index 3138446c..e9c9e6b4 100644 --- a/js/packages/cli/test/api.test.ts +++ b/js/packages/cli/test/api.test.ts @@ -10,6 +10,7 @@ */ import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { NO_POSIX_PERMISSIONS } from "./helpers/platform.js"; import { mkdtemp, mkdir, readFile, writeFile, stat } from "node:fs/promises"; import { existsSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -1152,7 +1153,7 @@ describe("semantic index", () => { expect(hits.json.results.map((r: any) => r.label)).toContain("api-token"); }); - it("writes the index at 0600, not the process umask", async () => { + it.skipIf(NO_POSIX_PERMISSIONS)("writes the index at 0600, not the process umask", async () => { await call("POST", "/api/v1/semantic/build"); const path = join(app.profileDir(FINGERPRINT), "semantic_index", "records.json"); expect((await stat(path)).mode & 0o777).toBe(0o600); diff --git a/js/packages/cli/test/backups.test.ts b/js/packages/cli/test/backups.test.ts index f637f876..b751351b 100644 --- a/js/packages/cli/test/backups.test.ts +++ b/js/packages/cli/test/backups.test.ts @@ -13,6 +13,7 @@ */ import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { NO_POSIX_PERMISSIONS } from "./helpers/platform.js"; import { mkdtemp, mkdir, readdir, readFile, writeFile, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -77,7 +78,7 @@ async function backupNames(): Promise { } describe("createIndexBackup", () => { - it("writes a Python-named snapshot at 0600 and copies the ciphertext verbatim", async () => { + it.skipIf(NO_POSIX_PERMISSIONS)("writes a Python-named snapshot at 0600 and copies the ciphertext verbatim", async () => { const result = await createIndexBackup({ indexPath, config: {}, now: 1111 }); expect(result.written).toBe(join(profileDir, BACKUP_DIR_NAME, backupFilename(1111))); // Exactly Python's BACKUP_FILENAME_TEMPLATE. @@ -90,7 +91,7 @@ describe("createIndexBackup", () => { expect(await readFile(result.written!)).toEqual(await readFile(indexPath)); }); - it("mirrors to additional_backup_path, prefixed by fingerprint as Python does", async () => { + it.skipIf(NO_POSIX_PERMISSIONS)("mirrors to additional_backup_path, prefixed by fingerprint as Python does", async () => { const extra = await mkdtemp(join(tmpdir(), "seedpass-extra-")); const result = await createIndexBackup({ indexPath, diff --git a/js/packages/cli/test/cli.test.ts b/js/packages/cli/test/cli.test.ts index 81aa82b0..9773fc4f 100644 --- a/js/packages/cli/test/cli.test.ts +++ b/js/packages/cli/test/cli.test.ts @@ -217,12 +217,19 @@ describe("sink delivery keeps secrets out of CLI output", () => { it("use --exec injects SEEDPASS_SECRET into the child env only", async () => { const dir = await mkdtemp(join(tmpdir(), "seedpass-sink-")); const capture = join(dir, "captured.txt"); - const script = join(dir, "cap.sh"); - await writeFile(script, `#!/bin/sh\nprintf '%s' "$SEEDPASS_SECRET" > "${capture}"\n`, { - mode: 0o755, - }); + // A Node script run by the interpreter already running these tests, + // rather than a `#!/bin/sh` file. Shebangs and /bin/sh do not exist on + // Windows, so this whole describe block failed there the first time the + // suite ran on it. `process.execPath` exists on every platform by + // definition, and quoting the spec lets a path with spaces survive. + const script = join(dir, "cap.cjs"); + await writeFile( + script, + `require("fs").writeFileSync(${JSON.stringify(capture)}, process.env.SEEDPASS_SECRET ?? "");`, + ); - const r = await run("--vault", vaultPath, "use", "api-token", "--exec", script); + const spec = `"${process.execPath}" "${script}"`; + const r = await run("--vault", vaultPath, "use", "api-token", "--exec", spec); const delivered = JSON.parse(r.stdout); expect(delivered.sink).toBe("exec"); expect(delivered.exitCode).toBe(0); @@ -237,10 +244,17 @@ describe("sink delivery keeps secrets out of CLI output", () => { it("use --stdin-to pipes the secret to the command's stdin", async () => { const dir = await mkdtemp(join(tmpdir(), "seedpass-sink-")); const capture = join(dir, "captured.txt"); - const script = join(dir, "cap.sh"); - await writeFile(script, `#!/bin/sh\ncat > "${capture}"\n`, { mode: 0o755 }); + const script = join(dir, "cap.cjs"); + await writeFile( + script, + `let b = ""; + process.stdin.setEncoding("utf8"); + process.stdin.on("data", (d) => { b += d; }); + process.stdin.on("end", () => require("fs").writeFileSync(${JSON.stringify(capture)}, b));`, + ); - const r = await run("--vault", vaultPath, "use", "api-token", "--stdin-to", script); + const spec = `"${process.execPath}" "${script}"`; + const r = await run("--vault", vaultPath, "use", "api-token", "--stdin-to", spec); expect(JSON.parse(r.stdout).sink).toBe("stdin"); expect(await readFile(capture, "utf8")).toBe("abc123"); expect(r.stdout).not.toContain("abc123"); @@ -252,12 +266,18 @@ describe("sink delivery keeps secrets out of CLI output", () => { // way to include flags. const dir = await mkdtemp(join(tmpdir(), "seedpass-spec-")); const capture = join(dir, "captured.txt"); - const script = join(dir, "cap.sh"); - await writeFile(script, `#!/bin/sh\nprintf '%s %s' "$1" "$(cat)" > "${capture}"\n`, { - mode: 0o755, - }); + const script = join(dir, "cap.cjs"); + await writeFile( + script, + `let b = ""; + process.stdin.setEncoding("utf8"); + process.stdin.on("data", (d) => { b += d; }); + process.stdin.on("end", () => + require("fs").writeFileSync(${JSON.stringify(capture)}, process.argv[2] + " " + b));`, + ); const r = await run( - "--vault", vaultPath, "use", "api-token", "--stdin-to", `${script} -flagged`, + "--vault", vaultPath, "use", "api-token", + "--stdin-to", `"${process.execPath}" "${script}" -flagged`, ); expect(JSON.parse(r.stdout).sink).toBe("stdin"); expect(await readFile(capture, "utf8")).toBe("-flagged abc123"); diff --git a/js/packages/cli/test/durability.test.ts b/js/packages/cli/test/durability.test.ts index f84e1b4d..817dea41 100644 --- a/js/packages/cli/test/durability.test.ts +++ b/js/packages/cli/test/durability.test.ts @@ -7,6 +7,7 @@ */ import { beforeAll, describe, expect, it } from "vitest"; +import { NO_POSIX_PERMISSIONS } from "./helpers/platform.js"; import { mkdtemp, readFile, writeFile, stat, chmod } from "node:fs/promises"; import { existsSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -75,7 +76,7 @@ describe("numeric input validation", () => { }); describe("atomic, permission-correct writes", () => { - it("leaves no temp files and keeps the vault at 0600", async () => { + it.skipIf(NO_POSIX_PERMISSIONS)("leaves no temp files and keeps the vault at 0600", async () => { await chmod(vaultPath, 0o644); // simulate a loosened file await run("--vault", vaultPath, "entry", "add", "key-value", "perm", "k", "v"); const info = await stat(vaultPath); diff --git a/js/packages/cli/test/helpers/platform.ts b/js/packages/cli/test/helpers/platform.ts new file mode 100644 index 00000000..255f3ac8 --- /dev/null +++ b/js/packages/cli/test/helpers/platform.ts @@ -0,0 +1,42 @@ +/** + * Platform guards for tests, with the reason attached to each. + * + * These exist so a skip is a stated decision rather than a quiet gap. The + * JavaScript suite first ran on Windows and macOS on 2026-08-20 and produced + * seventeen failures. Most were real bugs and got fixed — a Windows path + * containment check that refused everything, a command parser that ate + * backslashes, tests naming `/bin/true` and `cat`. What is left behind these + * constants are the cases where the property under test does not exist on + * that platform at all. + * + * Use as `it.skipIf(NO_POSIX_PERMISSIONS)("...", ...)`. + */ + +import process from "node:process"; + +export const IS_WINDOWS = process.platform === "win32"; + +/** + * POSIX mode bits — every `0600` assertion. + * + * Windows has no such thing: `chmod` toggles a read-only flag and `stat` + * reports 0o666 whatever you asked for, so these assertions cannot hold. + * Skipped rather than relaxed, because the property is real and a weakened + * assertion would answer it falsely. + * + * Skipping does NOT answer the underlying question, which is tracked in + * TODO.md: on Windows these files inherit directory ACLs instead of carrying + * an explicit owner-only mode, and the vault index, the semantic index and + * the recovery-drill key all hold secrets. Verifying that the inherited ACL + * is user-scoped, and asserting THAT on Windows, is the real fix. + */ +export const NO_POSIX_PERMISSIONS = IS_WINDOWS; + +/** + * A working system clipboard. + * + * The Windows CI runner is headless and has none, so clipboardSink writes + * nowhere and reads back empty. Nothing about the code is wrong; there is no + * clipboard to put a secret on. + */ +export const NO_CLIPBOARD = IS_WINDOWS; diff --git a/js/packages/cli/test/jobsAndDrills.test.ts b/js/packages/cli/test/jobsAndDrills.test.ts index 47692010..526dc31d 100644 --- a/js/packages/cli/test/jobsAndDrills.test.ts +++ b/js/packages/cli/test/jobsAndDrills.test.ts @@ -8,6 +8,7 @@ */ import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { NO_POSIX_PERMISSIONS } from "./helpers/platform.js"; import { mkdtemp, readFile, writeFile, stat, appendFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -78,7 +79,7 @@ describe("job profiles", () => { ).rejects.toThrow(/job_id_required/); }); - it("stores the file at 0600", async () => { + it.skipIf(NO_POSIX_PERMISSIONS)("stores the file at 0600", async () => { expect((await stat(join(appDir, "agent_jobs.json"))).mode & 0o777).toBe(0o600); }); }); @@ -249,7 +250,7 @@ describe("recovery drills", () => { expect(await verifyRecoveryDrills(dir)).toEqual({ valid: true, checked: 0, errors: [] }); }); - it("keeps the key and log at 0600", async () => { + it.skipIf(NO_POSIX_PERMISSIONS)("keeps the key and log at 0600", async () => { const dir = await mkdtemp(join(tmpdir(), "seedpass-modes-")); const backup = join(dir, "b"); await writeFile(backup, "x"); diff --git a/js/packages/cli/test/seedCreation.test.ts b/js/packages/cli/test/seedCreation.test.ts index e90862c8..0f4bb6fa 100644 --- a/js/packages/cli/test/seedCreation.test.ts +++ b/js/packages/cli/test/seedCreation.test.ts @@ -10,6 +10,7 @@ */ import { describe, expect, it, beforeEach, afterEach } from "vitest"; +import { NO_POSIX_PERMISSIONS } from "./helpers/platform.js"; import { mkdtemp, readFile, writeFile, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -90,7 +91,7 @@ describe("fingerprint create", () => { expect((await app.readFingerprints()).fingerprints).toHaveLength(0); }); - it("writes the phrase to a 0600 file and creates a matching profile", async () => { + it.skipIf(NO_POSIX_PERMISSIONS)("writes the phrase to a 0600 file and creates a matching profile", async () => { const out = join(scratch, "seed.txt"); const r = await run("fingerprint", "create", "--name", "test", "--words", "24", "--out", out); const result = JSON.parse(r.stdout); diff --git a/js/packages/cli/test/sinks.test.ts b/js/packages/cli/test/sinks.test.ts index 041ce78f..971be11b 100644 --- a/js/packages/cli/test/sinks.test.ts +++ b/js/packages/cli/test/sinks.test.ts @@ -8,6 +8,7 @@ */ import { describe, expect, it } from "vitest"; +import { NO_CLIPBOARD } from "./helpers/platform.js"; import { parseCommandSpec } from "../src/sinks.js"; describe("parseCommandSpec", () => { @@ -102,7 +103,7 @@ async function fakeClipboard(): Promise<{ dir: string; stateFile: string; restor } describe("clipboardSink auto-clear", () => { - it("copies the secret and, by default, never wipes it", async () => { + it.skipIf(NO_CLIPBOARD)("copies the secret and, by default, never wipes it", async () => { const clip = await fakeClipboard(); try { await clipboardSink("s3cr3t"); @@ -115,7 +116,7 @@ describe("clipboardSink auto-clear", () => { } }); - it("wipes the secret after the delay when it is still ours", async () => { + it.skipIf(NO_CLIPBOARD)("wipes the secret after the delay when it is still ours", async () => { const clip = await fakeClipboard(); try { const r = await clipboardSink("wipe-me", { clearAfterSeconds: 0.15 }); @@ -207,3 +208,50 @@ describe("the environment a sink child receives", () => { expect(env["SEEDPASS_SECRET"]).toBe("the-secret"); }); }); + +describe("command specs on Windows", () => { + /** + * `windows` is an explicit parameter so both branches run on any machine. + * Without that, the Windows branch could only ever execute on Windows — + * which is precisely how this survived: the JavaScript suite had never run + * there until 2026-08-20, and when it did, three sink tests failed at once. + * + * The bug: tokenize treated backslash as a POSIX escape unconditionally, so + * every Windows path handed to --exec or --stdin-to lost its separators + * before it was spawned. + */ + it("keeps the separators in a Windows path", () => { + expect(parseCommandSpec(["C:\\Tools\\bin.exe -c"], { windows: true })).toEqual([ + "C:\\Tools\\bin.exe", + ["-c"], + ]); + }); + + it("keeps them inside quotes too, where a path with spaces has to live", () => { + expect( + parseCommandSpec(['"C:\\Program Files\\tool.exe" --flag'], { windows: true }), + ).toEqual(["C:\\Program Files\\tool.exe", ["--flag"]]); + }); + + it("pins what the POSIX branch does to the same input", () => { + // Not a hypothetical. This is what every Windows user got: a command + // named "C:Program" and an argument called "Filestool.exe". + expect( + parseCommandSpec(['"C:\\Program Files\\tool.exe" --flag'], { windows: false }), + ).toEqual(["C:Program Filestool.exe", ["--flag"]]); + }); + + it("still escapes on POSIX, where a shell would", () => { + expect(parseCommandSpec(["printf a\\ b"], { windows: false })).toEqual([ + "printf", + ["a b"], + ]); + }); + + it("quoting still groups on Windows", () => { + expect(parseCommandSpec(['sh -c "do the thing"'], { windows: true })).toEqual([ + "sh", + ["-c", "do the thing"], + ]); + }); +}); diff --git a/js/packages/cli/test/tuiMenus.test.ts b/js/packages/cli/test/tuiMenus.test.ts index 3084ca4d..3f9fe63c 100644 --- a/js/packages/cli/test/tuiMenus.test.ts +++ b/js/packages/cli/test/tuiMenus.test.ts @@ -10,6 +10,7 @@ */ import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { NO_POSIX_PERMISSIONS } from "./helpers/platform.js"; import { mkdtemp, writeFile, mkdir, readFile, readdir } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -643,7 +644,7 @@ describe("inactivity timeout locks the vault", () => { import { stat, chmod } from "node:fs/promises"; describe("secret-bearing exports get a fresh 0600 file", () => { - it("re-creates a pre-existing world-readable file as 0600 on 2FA export", async () => { + it.skipIf(NO_POSIX_PERMISSIONS)("re-creates a pre-existing world-readable file as 0600 on 2FA export", async () => { const dest = join(appDir, "totp-export.json"); // An attacker (or just history) left a 0644 file at the destination. // writeFile(mode) would keep 0644 — every TOTP secret world-readable.