Fix what three platforms found: two shipped bugs, four unportable tests

The platform workflow's first run produced seventeen failures on Windows and
macOS, where the JavaScript suite had never executed. Two were real defects
in shipped code.

A CONTAINMENT CHECK THAT REFUSED EVERYTHING ON WINDOWS

`resolveWithinProfile` compared `target.startsWith(root + "/")` with a
hardcoded forward slash. On Windows `resolve()` returns
`C:\profile\file.txt` and the comparison asked for `C:\profile/`, so it
matched nothing: document export and import refused every path, including
legitimate ones. It failed CLOSED, so nothing leaked — the feature simply did
not work. Now uses `relative()`, which needs no separator of our choosing.

A COMMAND PARSER THAT ATE WINDOWS PATHS

`parseCommandSpec` treated backslash as a POSIX escape unconditionally, so
`--exec "C:\Program Files\tool.exe"` was spawned as the command `C:Program`
with an argument `Filestool.exe`. Every Windows path handed to --exec or
--stdin-to was mangled before the spawn. This is the SECOND instance of this
exact bug today — the Python TUI palette had it too — which is what a whole
class looks like rather than a one-off.

`windows` is an explicit parameter, not a `process.platform` read, so both
branches are tested from either platform. That parameter is the actual fix
for the class: the reason these survived is that the Windows branch could
only ever run on Windows.

THE REST WERE TESTS, NOT CODE

- `/bin/true` does not exist on macOS (it is /usr/bin/true), so a spawn failed
  with ENOENT and the refusal looked like the daemon's crash-safety fix having
  regressed. Now uses process.execPath, which exists everywhere by definition.
- The oversized-request test treated its own EPIPE as fatal. The daemon caps
  the line, answers and hangs up; on macOS that hangup lands while the client
  is still pushing the remaining megabyte. Correct server behaviour was
  failing the test on one platform and passing on the other.
- Three sink tests wrote `#!/bin/sh` files. Shebangs and /bin/sh do not exist
  on Windows. Now Node scripts run by the interpreter already running the
  tests.
- Eight `0600` assertions and two clipboard tests are guarded by documented
  constants in test/helpers/platform.ts. Windows has no POSIX mode bits and
  the runner has no clipboard.

The skips state a gap rather than closing it, and TODO.md now records that the
Windows permissions question grew from two Python tests to ten across both
languages — including the vault index and the semantic index, which hold
secrets. With TypeScript becoming the only implementation that is a shipping
question, not a test-suite one.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Vtx94nXDmNgXEeThUZP4QN
This commit is contained in:
thePR0M3TH3ANandClaude Opus 5 committed 2026-08-20 14:39:52 -04:00
1 parent 4edf50e5de
commit e2e9aa851e
13 files changed
+213 -32

No files matched your search

+15
View File
@@ -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
+15 -2
View File
@@ -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;
+22 -6
View File
@@ -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;
+22 -1
View File
@@ -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);
+2 -1
View File
@@ -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);
+3 -2
View File
@@ -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<string[]> {
}
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,
+33 -13
View File
@@ -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");
+2 -1
View File
@@ -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);
+42
View File
@@ -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;
+3 -2
View File
@@ -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");
+2 -1
View File
@@ -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);
+50 -2
View File
@@ -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"],
]);
});
});
+2 -1
View File
@@ -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.