From 4047e52bc6faf25d1a15e11727bfcf932dc6fe7b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 16 Aug 2026 08:54:06 -0700 Subject: [PATCH] fix(gbrain): quote cmd.exe arguments at a single gbrain invocation seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #2471. With shell:true on Windows, node/bun join argv into one cmd.exe string without quoting, so a repo path with a space — the default C:\Users\First Last\ layout — split into two arguments and every gbrain call carrying a path silently targeted the wrong location (worst: `sources add --path`). All gbrain CLI invocations now build their (cmd, argv, shell) triple through gbrainInvocation(), which quotes risky arguments for cmd.exe's re-parse (embedded quotes doubled). The four direct spawn sites in lib/gbrain-sources.ts route through the seam; the #1731 static invariant is upgraded for seamed files (any direct "gbrain" opener is the violation) and kept as-is for lib/gbrain-local-status.ts. POSIX behavior unchanged (shell:false, passthrough argv). Co-Authored-By: Claude Fable 5 --- lib/gbrain-exec.ts | 42 +++++++++++-- lib/gbrain-sources.ts | 37 ++++++------ test/gbrain-spawn-windows-shell.test.ts | 79 ++++++++++++++++++++----- 3 files changed, 119 insertions(+), 39 deletions(-) diff --git a/lib/gbrain-exec.ts b/lib/gbrain-exec.ts index 188d84b8f..f13ea4a88 100644 --- a/lib/gbrain-exec.ts +++ b/lib/gbrain-exec.ts @@ -196,6 +196,33 @@ export function bashScriptInvocation( return { cmd: bash, argv: [scriptPath.replace(/\\/g, "/"), ...args], shell: false }; } +/** + * Quote one argument for cmd.exe's re-parse (#2471). With `shell: true` on + * Windows, Node/Bun JOIN the argv into a single cmd.exe string WITHOUT + * quoting, so any argument containing a space — the default + * `C:\Users\First Last\repo` home layout — splits into two arguments and the + * gbrain call silently targets the wrong path. Pass-through for the safe + * charset; everything else is double-quoted with embedded quotes doubled + * (cmd.exe's escape). POSIX callers never see this (shell is false there). + */ +export function windowsShellQuote(arg: string): string { + if (arg !== "" && /^[A-Za-z0-9_\-.:\\/=,@+]+$/.test(arg)) return arg; + return '"' + arg.replace(/"/g, '""') + '"'; +} + +/** + * The single seam for building a gbrain CLI invocation (#2471). Every + * spawnSync/execFileSync of the `gbrain` shim must construct its + * (cmd, argv, shell) triple here so the Windows quoting fix lives in exactly + * one place — a direct spawn of the literal "gbrain" string with a shell + * flag reopens the space-in-path split this exists to close. + */ +export function gbrainInvocation(args: string[]): { cmd: string; argv: string[]; shell: boolean } { + return NEEDS_SHELL_ON_WINDOWS + ? { cmd: "gbrain", argv: args.map(windowsShellQuote), shell: true } + : { cmd: "gbrain", argv: args, shell: false }; +} + export interface SpawnGbrainOptions { /** Timeout in milliseconds. Defaults to 30s. */ timeout?: number; @@ -219,13 +246,14 @@ export interface SpawnGbrainOptions { * `stderr` exactly as they would with `spawnSync` directly. */ export function spawnGbrain(args: string[], opts: SpawnGbrainOptions = {}): SpawnSyncReturns { - return spawnSync("gbrain", args, { + const inv = gbrainInvocation(args); + return spawnSync(inv.cmd, inv.argv, { encoding: "utf-8", timeout: opts.timeout ?? 30_000, cwd: opts.cwd, stdio: opts.stdio || ["ignore", "pipe", "pipe"], env: buildGbrainEnv({ baseEnv: opts.baseEnv, announce: opts.announce }), - shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows + shell: inv.shell, // #1731: gbrain is a .cmd shim on Windows (+#2471 quoting) }); } @@ -254,11 +282,12 @@ export function spawnGbrainAsync( args: string[], opts: { stdio?: SpawnOptions["stdio"]; cwd?: string; baseEnv?: NodeJS.ProcessEnv } = {}, ): ChildProcess { - return spawn("gbrain", args, { + const inv = gbrainInvocation(args); + return spawn(inv.cmd, inv.argv, { stdio: opts.stdio || ["ignore", "pipe", "pipe"], cwd: opts.cwd, env: buildGbrainEnv({ baseEnv: opts.baseEnv, announce: false }), - shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows + shell: inv.shell, // #1731: gbrain is a .cmd shim on Windows (+#2471 quoting) }); } @@ -267,12 +296,13 @@ export function spawnGbrainAsync( * for callers that want to surface gbrain's stderr as the error message. */ export function execGbrainText(args: string[], opts: SpawnGbrainOptions = {}): string { - return execFileSync("gbrain", args, { + const inv = gbrainInvocation(args); + return execFileSync(inv.cmd, inv.argv, { encoding: "utf-8", timeout: opts.timeout ?? 30_000, cwd: opts.cwd, stdio: opts.stdio || ["ignore", "pipe", "pipe"], env: buildGbrainEnv({ baseEnv: opts.baseEnv, announce: opts.announce }), - shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows + shell: inv.shell, // #1731: gbrain is a .cmd shim on Windows (+#2471 quoting) }); } diff --git a/lib/gbrain-sources.ts b/lib/gbrain-sources.ts index 826e905ac..34b42319c 100644 --- a/lib/gbrain-sources.ts +++ b/lib/gbrain-sources.ts @@ -12,7 +12,7 @@ import { execFileSync, spawnSync } from "child_process"; import { realpathSync } from "fs"; import { withErrorContext } from "./gstack-memory-helpers"; -import { execGbrainJson, NEEDS_SHELL_ON_WINDOWS } from "./gbrain-exec"; +import { execGbrainJson, gbrainInvocation } from "./gbrain-exec"; import { detectAutopilot, decideSourceRemove, @@ -117,12 +117,13 @@ function samePath(registered: string | undefined, requested: string): boolean { export function probeSource(id: string, env?: NodeJS.ProcessEnv): SourceState { let stdout: string; try { - stdout = execFileSync("gbrain", ["sources", "list", "--json"], { + const inv = gbrainInvocation(["sources", "list", "--json"]); + stdout = execFileSync(inv.cmd, inv.argv, { encoding: "utf-8", timeout: 30_000, stdio: ["ignore", "pipe", "pipe"], env, - shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows + shell: inv.shell, // #1731: gbrain is a .cmd shim on Windows (+#2471 quoting) }); } catch (err) { const e = err as NodeJS.ErrnoException & { stderr?: Buffer }; @@ -224,29 +225,28 @@ export async function ensureSourceRegistered( throw new Error(`refusing drift re-register of ${id}: ${decision.reason}`); } - const rm = spawnSync( - "gbrain", - ["sources", "remove", id, "--yes", "--confirm-destructive", ...decision.extraArgs], - { - encoding: "utf-8", - timeout: 30_000, - env, - shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows - }, - ); + const rmInv = gbrainInvocation(["sources", "remove", id, "--yes", "--confirm-destructive", ...decision.extraArgs]); + const rm = spawnSync(rmInv.cmd, rmInv.argv, { + encoding: "utf-8", + timeout: 30_000, + env, + shell: rmInv.shell, // #1731: gbrain is a .cmd shim on Windows (+#2471 quoting) + }); if (rm.status !== 0) { throw new Error(`gbrain sources remove ${id} failed: ${rm.stderr || rm.stdout || `exit ${rm.status}`}`); } } - // Add. + // Add. `path` is a user repo path — the #2471 space-in-path victim; the + // invocation seam quotes it for cmd.exe's re-parse. const addArgs = ["sources", "add", id, "--path", path]; if (federated) addArgs.push("--federated"); - const add = spawnSync("gbrain", addArgs, { + const addInv = gbrainInvocation(addArgs); + const add = spawnSync(addInv.cmd, addInv.argv, { encoding: "utf-8", timeout: 30_000, env, - shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows + shell: addInv.shell, // #1731: gbrain is a .cmd shim on Windows (+#2471 quoting) }); if (add.status !== 0) { throw new Error(`gbrain sources add ${id} failed: ${add.stderr || add.stdout || `exit ${add.status}`}`); @@ -267,12 +267,13 @@ export async function ensureSourceRegistered( export function sourcePageCount(id: string, env?: NodeJS.ProcessEnv): number | null { let stdout: string; try { - stdout = execFileSync("gbrain", ["sources", "list", "--json"], { + const inv = gbrainInvocation(["sources", "list", "--json"]); + stdout = execFileSync(inv.cmd, inv.argv, { encoding: "utf-8", timeout: 30_000, stdio: ["ignore", "pipe", "pipe"], env, - shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows + shell: inv.shell, // #1731: gbrain is a .cmd shim on Windows (+#2471 quoting) }); } catch { return null; diff --git a/test/gbrain-spawn-windows-shell.test.ts b/test/gbrain-spawn-windows-shell.test.ts index eb319684e..76a8b68ab 100644 --- a/test/gbrain-spawn-windows-shell.test.ts +++ b/test/gbrain-spawn-windows-shell.test.ts @@ -2,7 +2,7 @@ import { describe, test, expect } from "bun:test"; import * as fs from "fs"; import * as path from "path"; -import { bashScriptInvocation } from "../lib/gbrain-exec"; +import { bashScriptInvocation, gbrainInvocation, windowsShellQuote } from "../lib/gbrain-exec"; const ROOT = path.resolve(import.meta.dir, ".."); const read = (rel: string) => fs.readFileSync(path.join(ROOT, rel), "utf-8"); @@ -18,24 +18,30 @@ describe("#1731 gbrain spawns carry the Windows shell flag", () => { expect(src).toMatch(/export const NEEDS_SHELL_ON_WINDOWS\s*=\s*process\.platform === "win32"/); }); - // Every direct `gbrain` child spawn in these files must be matched by a - // shell:NEEDS_SHELL_ON_WINDOWS flag. Count openers vs flags as a cheap, - // refactor-resistant invariant. - const gbrainSpawnFiles = [ - "lib/gbrain-exec.ts", - "lib/gbrain-sources.ts", - "lib/gbrain-local-status.ts", - ]; - for (const rel of gbrainSpawnFiles) { - test(`${rel}: every gbrain spawn has shell:NEEDS_SHELL_ON_WINDOWS`, () => { + // #2471 upgraded the #1731 invariant for the seamed files: gbrain spawns + // there must build their (cmd, argv, shell) triple via gbrainInvocation() + // (which owns BOTH the shell flag and cmd.exe quoting), so a direct + // `spawn*("gbrain"` opener is itself the violation. + const seamedFiles = ["lib/gbrain-exec.ts", "lib/gbrain-sources.ts"]; + for (const rel of seamedFiles) { + test(`${rel}: gbrain spawns route through gbrainInvocation (no direct openers)`, () => { const src = read(rel); - const spawnOpeners = src.match(/(spawnSync|spawn|execFileSync)\("gbrain"/g)?.length ?? 0; - const shellFlags = src.match(/shell:\s*NEEDS_SHELL_ON_WINDOWS/g)?.length ?? 0; - expect(spawnOpeners).toBeGreaterThan(0); - expect(shellFlags).toBeGreaterThanOrEqual(spawnOpeners); + const directOpeners = src.match(/(spawnSync|spawn|execFileSync)\(\s*["']gbrain["']/g)?.length ?? 0; + expect(directOpeners).toBe(0); + expect(src).toContain("gbrainInvocation("); }); } + // Not-yet-seamed file: every direct gbrain spawn must still carry the + // #1731 shell flag. (Migrate to gbrainInvocation when next touched.) + test("lib/gbrain-local-status.ts: every gbrain spawn has shell:NEEDS_SHELL_ON_WINDOWS", () => { + const src = read("lib/gbrain-local-status.ts"); + const spawnOpeners = src.match(/(spawnSync|spawn|execFileSync)\("gbrain"/g)?.length ?? 0; + const shellFlags = src.match(/shell:\s*NEEDS_SHELL_ON_WINDOWS/g)?.length ?? 0; + expect(spawnOpeners).toBeGreaterThan(0); + expect(shellFlags).toBeGreaterThanOrEqual(spawnOpeners); + }); + // NOT the brain-sync script. `shell: true` is right for the gbrain.cmd shim // and wrong for a bash shebang script: cmd.exe resolves .cmd/.bat via PATHEXT // and has no concept of a shebang, so gstack-brain-sync came back as "is not @@ -115,3 +121,46 @@ describe("bashScriptInvocation", () => { expect(inv).toBeNull(); }); }); + +// #2471: with `shell: true` on Windows, node/bun JOIN argv into one cmd.exe +// string without quoting — a path with a space (`C:\Users\First Last\repo`) +// splits into two arguments and `gbrain sources add --path` targets the wrong +// directory. The invocation seam quotes every risky argument exactly once. +describe("#2471 gbrain invocation seam quotes for cmd.exe", () => { + test("safe charset passes through untouched", () => { + expect(windowsShellQuote("sources")).toBe("sources"); + expect(windowsShellQuote("--json")).toBe("--json"); + expect(windowsShellQuote("C:\\Users\\j\\repo")).toBe("C:\\Users\\j\\repo"); + }); + + test("a path with a space is double-quoted", () => { + expect(windowsShellQuote("C:\\Users\\First Last\\repo")).toBe('"C:\\Users\\First Last\\repo"'); + }); + + test("embedded quotes are doubled (cmd.exe escape)", () => { + expect(windowsShellQuote('we"ird')).toBe('"we""ird"'); + }); + + test("empty argument stays a quoted empty string, not vanishing", () => { + expect(windowsShellQuote("")).toBe('""'); + }); + + test("shell metacharacters are wrapped so cmd.exe cannot interpret them", () => { + for (const bad of ["a b", "a&b", "a|b", "a>b", "a { + if (process.platform === "win32") return; // the win32 half is the map+quote path above + const inv = gbrainInvocation(["sources", "add", "id", "--path", "/a dir/with space"]); + expect(inv).toEqual({ cmd: "gbrain", argv: ["sources", "add", "id", "--path", "/a dir/with space"], shell: false }); + }); + + test("no direct un-seamed gbrain spawn remains in gbrain-sources.ts", () => { + const src = read("lib/gbrain-sources.ts"); + expect(src).not.toMatch(/spawnSync\(\s*["']gbrain["']/); + expect(src).not.toMatch(/execFileSync\(\s*["']gbrain["']/); + expect(src).toContain("gbrainInvocation("); + }); +});