mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-16 18:05:31 +02:00
fix(gbrain): quote cmd.exe arguments at a single gbrain invocation seam
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
ea780fed61
commit
4047e52bc6
+36
-6
@@ -196,6 +196,33 @@ export function bashScriptInvocation(
|
|||||||
return { cmd: bash, argv: [scriptPath.replace(/\\/g, "/"), ...args], shell: false };
|
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 {
|
export interface SpawnGbrainOptions {
|
||||||
/** Timeout in milliseconds. Defaults to 30s. */
|
/** Timeout in milliseconds. Defaults to 30s. */
|
||||||
timeout?: number;
|
timeout?: number;
|
||||||
@@ -219,13 +246,14 @@ export interface SpawnGbrainOptions {
|
|||||||
* `stderr` exactly as they would with `spawnSync` directly.
|
* `stderr` exactly as they would with `spawnSync` directly.
|
||||||
*/
|
*/
|
||||||
export function spawnGbrain(args: string[], opts: SpawnGbrainOptions = {}): SpawnSyncReturns<string> {
|
export function spawnGbrain(args: string[], opts: SpawnGbrainOptions = {}): SpawnSyncReturns<string> {
|
||||||
return spawnSync("gbrain", args, {
|
const inv = gbrainInvocation(args);
|
||||||
|
return spawnSync(inv.cmd, inv.argv, {
|
||||||
encoding: "utf-8",
|
encoding: "utf-8",
|
||||||
timeout: opts.timeout ?? 30_000,
|
timeout: opts.timeout ?? 30_000,
|
||||||
cwd: opts.cwd,
|
cwd: opts.cwd,
|
||||||
stdio: opts.stdio || ["ignore", "pipe", "pipe"],
|
stdio: opts.stdio || ["ignore", "pipe", "pipe"],
|
||||||
env: buildGbrainEnv({ baseEnv: opts.baseEnv, announce: opts.announce }),
|
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[],
|
args: string[],
|
||||||
opts: { stdio?: SpawnOptions["stdio"]; cwd?: string; baseEnv?: NodeJS.ProcessEnv } = {},
|
opts: { stdio?: SpawnOptions["stdio"]; cwd?: string; baseEnv?: NodeJS.ProcessEnv } = {},
|
||||||
): ChildProcess {
|
): ChildProcess {
|
||||||
return spawn("gbrain", args, {
|
const inv = gbrainInvocation(args);
|
||||||
|
return spawn(inv.cmd, inv.argv, {
|
||||||
stdio: opts.stdio || ["ignore", "pipe", "pipe"],
|
stdio: opts.stdio || ["ignore", "pipe", "pipe"],
|
||||||
cwd: opts.cwd,
|
cwd: opts.cwd,
|
||||||
env: buildGbrainEnv({ baseEnv: opts.baseEnv, announce: false }),
|
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.
|
* for callers that want to surface gbrain's stderr as the error message.
|
||||||
*/
|
*/
|
||||||
export function execGbrainText(args: string[], opts: SpawnGbrainOptions = {}): string {
|
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",
|
encoding: "utf-8",
|
||||||
timeout: opts.timeout ?? 30_000,
|
timeout: opts.timeout ?? 30_000,
|
||||||
cwd: opts.cwd,
|
cwd: opts.cwd,
|
||||||
stdio: opts.stdio || ["ignore", "pipe", "pipe"],
|
stdio: opts.stdio || ["ignore", "pipe", "pipe"],
|
||||||
env: buildGbrainEnv({ baseEnv: opts.baseEnv, announce: opts.announce }),
|
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)
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
+19
-18
@@ -12,7 +12,7 @@
|
|||||||
import { execFileSync, spawnSync } from "child_process";
|
import { execFileSync, spawnSync } from "child_process";
|
||||||
import { realpathSync } from "fs";
|
import { realpathSync } from "fs";
|
||||||
import { withErrorContext } from "./gstack-memory-helpers";
|
import { withErrorContext } from "./gstack-memory-helpers";
|
||||||
import { execGbrainJson, NEEDS_SHELL_ON_WINDOWS } from "./gbrain-exec";
|
import { execGbrainJson, gbrainInvocation } from "./gbrain-exec";
|
||||||
import {
|
import {
|
||||||
detectAutopilot,
|
detectAutopilot,
|
||||||
decideSourceRemove,
|
decideSourceRemove,
|
||||||
@@ -117,12 +117,13 @@ function samePath(registered: string | undefined, requested: string): boolean {
|
|||||||
export function probeSource(id: string, env?: NodeJS.ProcessEnv): SourceState {
|
export function probeSource(id: string, env?: NodeJS.ProcessEnv): SourceState {
|
||||||
let stdout: string;
|
let stdout: string;
|
||||||
try {
|
try {
|
||||||
stdout = execFileSync("gbrain", ["sources", "list", "--json"], {
|
const inv = gbrainInvocation(["sources", "list", "--json"]);
|
||||||
|
stdout = execFileSync(inv.cmd, inv.argv, {
|
||||||
encoding: "utf-8",
|
encoding: "utf-8",
|
||||||
timeout: 30_000,
|
timeout: 30_000,
|
||||||
stdio: ["ignore", "pipe", "pipe"],
|
stdio: ["ignore", "pipe", "pipe"],
|
||||||
env,
|
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) {
|
} catch (err) {
|
||||||
const e = err as NodeJS.ErrnoException & { stderr?: Buffer };
|
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}`);
|
throw new Error(`refusing drift re-register of ${id}: ${decision.reason}`);
|
||||||
}
|
}
|
||||||
|
|
||||||
const rm = spawnSync(
|
const rmInv = gbrainInvocation(["sources", "remove", id, "--yes", "--confirm-destructive", ...decision.extraArgs]);
|
||||||
"gbrain",
|
const rm = spawnSync(rmInv.cmd, rmInv.argv, {
|
||||||
["sources", "remove", id, "--yes", "--confirm-destructive", ...decision.extraArgs],
|
encoding: "utf-8",
|
||||||
{
|
timeout: 30_000,
|
||||||
encoding: "utf-8",
|
env,
|
||||||
timeout: 30_000,
|
shell: rmInv.shell, // #1731: gbrain is a .cmd shim on Windows (+#2471 quoting)
|
||||||
env,
|
});
|
||||||
shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows
|
|
||||||
},
|
|
||||||
);
|
|
||||||
if (rm.status !== 0) {
|
if (rm.status !== 0) {
|
||||||
throw new Error(`gbrain sources remove ${id} failed: ${rm.stderr || rm.stdout || `exit ${rm.status}`}`);
|
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];
|
const addArgs = ["sources", "add", id, "--path", path];
|
||||||
if (federated) addArgs.push("--federated");
|
if (federated) addArgs.push("--federated");
|
||||||
const add = spawnSync("gbrain", addArgs, {
|
const addInv = gbrainInvocation(addArgs);
|
||||||
|
const add = spawnSync(addInv.cmd, addInv.argv, {
|
||||||
encoding: "utf-8",
|
encoding: "utf-8",
|
||||||
timeout: 30_000,
|
timeout: 30_000,
|
||||||
env,
|
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) {
|
if (add.status !== 0) {
|
||||||
throw new Error(`gbrain sources add ${id} failed: ${add.stderr || add.stdout || `exit ${add.status}`}`);
|
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 {
|
export function sourcePageCount(id: string, env?: NodeJS.ProcessEnv): number | null {
|
||||||
let stdout: string;
|
let stdout: string;
|
||||||
try {
|
try {
|
||||||
stdout = execFileSync("gbrain", ["sources", "list", "--json"], {
|
const inv = gbrainInvocation(["sources", "list", "--json"]);
|
||||||
|
stdout = execFileSync(inv.cmd, inv.argv, {
|
||||||
encoding: "utf-8",
|
encoding: "utf-8",
|
||||||
timeout: 30_000,
|
timeout: 30_000,
|
||||||
stdio: ["ignore", "pipe", "pipe"],
|
stdio: ["ignore", "pipe", "pipe"],
|
||||||
env,
|
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 {
|
} catch {
|
||||||
return null;
|
return null;
|
||||||
|
|||||||
@@ -2,7 +2,7 @@ import { describe, test, expect } from "bun:test";
|
|||||||
import * as fs from "fs";
|
import * as fs from "fs";
|
||||||
import * as path from "path";
|
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 ROOT = path.resolve(import.meta.dir, "..");
|
||||||
const read = (rel: string) => fs.readFileSync(path.join(ROOT, rel), "utf-8");
|
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"/);
|
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
|
// #2471 upgraded the #1731 invariant for the seamed files: gbrain spawns
|
||||||
// shell:NEEDS_SHELL_ON_WINDOWS flag. Count openers vs flags as a cheap,
|
// there must build their (cmd, argv, shell) triple via gbrainInvocation()
|
||||||
// refactor-resistant invariant.
|
// (which owns BOTH the shell flag and cmd.exe quoting), so a direct
|
||||||
const gbrainSpawnFiles = [
|
// `spawn*("gbrain"` opener is itself the violation.
|
||||||
"lib/gbrain-exec.ts",
|
const seamedFiles = ["lib/gbrain-exec.ts", "lib/gbrain-sources.ts"];
|
||||||
"lib/gbrain-sources.ts",
|
for (const rel of seamedFiles) {
|
||||||
"lib/gbrain-local-status.ts",
|
test(`${rel}: gbrain spawns route through gbrainInvocation (no direct openers)`, () => {
|
||||||
];
|
|
||||||
for (const rel of gbrainSpawnFiles) {
|
|
||||||
test(`${rel}: every gbrain spawn has shell:NEEDS_SHELL_ON_WINDOWS`, () => {
|
|
||||||
const src = read(rel);
|
const src = read(rel);
|
||||||
const spawnOpeners = src.match(/(spawnSync|spawn|execFileSync)\("gbrain"/g)?.length ?? 0;
|
const directOpeners = src.match(/(spawnSync|spawn|execFileSync)\(\s*["']gbrain["']/g)?.length ?? 0;
|
||||||
const shellFlags = src.match(/shell:\s*NEEDS_SHELL_ON_WINDOWS/g)?.length ?? 0;
|
expect(directOpeners).toBe(0);
|
||||||
expect(spawnOpeners).toBeGreaterThan(0);
|
expect(src).toContain("gbrainInvocation(");
|
||||||
expect(shellFlags).toBeGreaterThanOrEqual(spawnOpeners);
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// 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
|
// 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 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
|
// 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();
|
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<b", "a^b", "a(b)", "a;b"]) {
|
||||||
|
expect(windowsShellQuote(bad).startsWith('"')).toBe(true);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test("gbrainInvocation on POSIX is a passthrough with shell:false", () => {
|
||||||
|
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(");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user