diff options
| author | Aman Kumar <145339995+amankumarsingh77@users.noreply.github.com> | 2026-08-06 00:47:15 +0530 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2026-08-06 00:47:15 +0530 |
| commit | d49068d9ae55ca15f3b58c000fcbb27a59bdb895 (patch) | |
| tree | c848dacf3d4e79995c9012a40e43a7d064a0fd07 | |
| parent | d9bc46b508f8c6f7e0f0ff5d54d804340f3e6c80 (diff) | |
fix: accept Windows script paths and start the daemon from a real install (#23)
Two independent Windows/production defects reported in #22.
browser_run_script rejected every absolute path on Windows. The allowed-root
check compared with a hardcoded `${root}/`, but path.resolve returns
backslash-separated paths there, so a file inside os.tmpdir() was refused even
though the rejection message listed tmpdir as allowed. Decide containment with
path.relative instead: it is separator-agnostic and carries each platform's own
case rules, so it also accepts a differing drive-letter case that a path.sep
patch would still reject. Escapes via `..` and sibling directories sharing the
root's name prefix stay rejected, as before.
The daemon could not start under `npm omit=dev`: it is spawned through tsx,
which was declared only as a dev dependency. Moving tsx to dependencies is not
enough on its own — pi installs extensions into one flat hoisted tree, so tsx
lands beside the package and the fixed <pkg>/node_modules/.bin/tsx lookup still
misses, falling through to an `npx tsx` that resolves against the wrong cwd and
dies against stdio: "ignore". Resolve the CLI entry with Node's own resolver,
which walks up to the hoisted copy. That removes the shell entirely, and with it
the Windows .cmd shim, windowsVerbatimArguments, and the manual quoting that
paths containing spaces needed.
Both fixes live in platform-branching code, so their tests are registered in the
three-OS CI matrix that already exists for that purpose — a Linux-only run
cannot exercise either one.
| -rw-r--r-- | .github/workflows/ci.yml | 18 | ||||
| -rw-r--r-- | CHANGELOG.md | 2 | ||||
| -rw-r--r-- | package-lock.json | 31 | ||||
| -rw-r--r-- | package.json | 2 | ||||
| -rw-r--r-- | src/daemon/spawn.ts | 34 | ||||
| -rw-r--r-- | src/domains/js.ts | 6 | ||||
| -rw-r--r-- | src/util/paths.ts | 12 | ||||
| -rw-r--r-- | test/util/daemon-socket-test.ts | 25 | ||||
| -rw-r--r-- | test/util/paths-test.ts | 50 |
9 files changed, 121 insertions, 59 deletions
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f307368..9550d94 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,14 +35,16 @@ jobs: - name: Verify npm package run: npm pack --dry-run > /dev/null - # Profile discovery and pinning are the only parts of the harness whose - # behavior differs per operating system: user-data-dir locations, how the - # running browser is identified, and whether a --profile-directory launch is - # handed to the running browser by Chromium's ProcessSingleton. Chromium's - # source says all three behave the same everywhere; this job is what turns - # that into evidence on real macOS and Windows machines. + # The parts of the harness whose behavior differs per operating system: + # profile discovery and pinning (user-data-dir locations, how the running + # browser is identified, whether a --profile-directory launch is handed to the + # running browser by Chromium's ProcessSingleton), script-path containment + # (separators and case rules), and how the daemon process is spawned. + # Chromium's source and node:path's docs say these behave as expected + # everywhere; this job is what turns that into evidence on real macOS and + # Windows machines. profiles: - name: Profiles (${{ matrix.os }}) + name: Platform behavior (${{ matrix.os }}) runs-on: ${{ matrix.os }} strategy: fail-fast: false @@ -70,6 +72,8 @@ jobs: npx tsx test/profile/list-test.ts npx tsx test/profile/store-test.ts npx tsx test/profile/browser-process-test.ts + npx tsx test/util/paths-test.ts + npx tsx test/util/daemon-socket-test.ts shell: bash # The runner images ship Google Chrome on all three platforms. Linux has diff --git a/CHANGELOG.md b/CHANGELOG.md index 988e5bb..5810ea3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ All notable changes to pi-browser-harness will be documented in this file. ### Fixed - **Chrome remote-debugging consent is no longer retriggered indefinitely in the background.** A cancelled, failed, or disconnected CDP connection now remains disconnected until the next explicit browser request, which makes one on-demand connection attempt instead of running an unbounded retry loop that can repeatedly interrupt normal browsing. +- **`browser_run_script` accepts paths on Windows again.** The allowed-directory check compared against a hardcoded `/` separator, so every backslash path — including files inside `os.tmpdir()`, which the rejection message itself listed as allowed — was refused. Containment is now decided by `path.relative`, which carries each platform's own separator and case rules. +- **The daemon starts from a production install.** `tsx` is needed to run the daemon but was declared only as a dev dependency, so `browser_setup` reported `Could not start the browser daemon` under `npm omit=dev`. It is now a runtime dependency, and the daemon is launched via the CLI entry that Node's resolver finds — a fixed `node_modules/.bin` path does not exist in a hoisted install, which is how pi lays extensions out. ## 0.11.0 — 2026-08-02 diff --git a/package-lock.json b/package-lock.json index 9ba24fc..012ee11 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,13 +9,13 @@ "version": "0.11.0", "license": "MIT", "dependencies": { + "tsx": "^4.22.4", "ws": "^8.20.0" }, "devDependencies": { "@mariozechner/pi-coding-agent": "*", "@mariozechner/pi-tui": "*", "@types/ws": "^8.18.1", - "tsx": "^4.22.4", "typebox": "*", "typescript": "^5.0.0" }, @@ -884,7 +884,6 @@ "cpu": [ "ppc64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -901,7 +900,6 @@ "cpu": [ "arm" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -918,7 +916,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -935,7 +932,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -952,7 +948,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -969,7 +964,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -986,7 +980,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1003,7 +996,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1020,7 +1012,6 @@ "cpu": [ "arm" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1037,7 +1028,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1054,7 +1044,6 @@ "cpu": [ "ia32" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1071,7 +1060,6 @@ "cpu": [ "loong64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1088,7 +1076,6 @@ "cpu": [ "mips64el" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1105,7 +1092,6 @@ "cpu": [ "ppc64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1122,7 +1108,6 @@ "cpu": [ "riscv64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1139,7 +1124,6 @@ "cpu": [ "s390x" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1156,7 +1140,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1173,7 +1156,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1190,7 +1172,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1207,7 +1188,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1224,7 +1204,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1241,7 +1220,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1258,7 +1236,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1275,7 +1252,6 @@ "cpu": [ "arm64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1292,7 +1268,6 @@ "cpu": [ "ia32" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -1309,7 +1284,6 @@ "cpu": [ "x64" ], - "dev": true, "license": "MIT", "optional": true, "os": [ @@ -3245,7 +3219,6 @@ "version": "0.28.1", "resolved": "https://registry.npmjs.org/esbuild/-/esbuild-0.28.1.tgz", "integrity": "sha512-HrJrvZv5ayxBzPfwphOoNzkzOIIlifzk0KJrGK2c8R4+LKpMtpYLQeUdjnwjWv/LZlkH2laZk+4w78pi99D4Vw==", - "dev": true, "hasInstallScript": true, "license": "MIT", "bin": { @@ -3485,7 +3458,6 @@ "version": "2.3.3", "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.3.tgz", "integrity": "sha512-5xoDfX+fL7faATnagmWPpbFtwh/R77WmMMqqHGS65C3vvB0YHrgF+B1YmZ3441tMj5n63k0212XNoJwzlhffQw==", - "dev": true, "hasInstallScript": true, "license": "MIT", "optional": true, @@ -4556,7 +4528,6 @@ "version": "4.22.4", "resolved": "https://registry.npmjs.org/tsx/-/tsx-4.22.4.tgz", "integrity": "sha512-X8EX+XV4QR5xCsrgxaED954zTDfY8KqlDtskKEL0cHhyS/P8b4IFOvGDQpsC9Q1XnLq915wEfwwY/zzskCtmhg==", - "dev": true, "license": "MIT", "dependencies": { "esbuild": "~0.28.0" diff --git a/package.json b/package.json index a0d724d..177383d 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,6 @@ "@mariozechner/pi-coding-agent": "*", "@mariozechner/pi-tui": "*", "@types/ws": "^8.18.1", - "tsx": "^4.22.4", "typebox": "*", "typescript": "^5.0.0" }, @@ -70,6 +69,7 @@ "sharp": "^0.33.0" }, "dependencies": { + "tsx": "^4.22.4", "ws": "^8.20.0" } } diff --git a/src/daemon/spawn.ts b/src/daemon/spawn.ts index a88855f..ffba92d 100644 --- a/src/daemon/spawn.ts +++ b/src/daemon/spawn.ts @@ -1,6 +1,6 @@ import { spawn, type ChildProcess } from "node:child_process"; import { access, unlink } from "node:fs/promises"; -import { existsSync } from "node:fs"; +import { createRequire } from "node:module"; import { fileURLToPath } from "node:url"; import { dirname, join } from "node:path"; import { createConnection } from "node:net"; @@ -8,6 +8,8 @@ import { DAEMON_SOCKET_PATH, serialize, type WireControl } from "./protocol"; const moduleDir = dirname(fileURLToPath(import.meta.url)); +const requireFromHere = createRequire(import.meta.url); + const isWindows = process.platform === "win32"; // No-op on Windows: a named pipe is not a filesystem entry and unlinking a pipe path throws. @@ -78,26 +80,28 @@ export const isDaemonRunning = async (timeoutMs = 2_000): Promise<boolean> => { }); }; -export const spawnDaemon = (): ChildProcess | null => { +// Node's own resolver, not a guessed `node_modules/.bin` path: pi installs every extension into +// one flat, hoisted tree, so tsx lands beside the package rather than inside it and a fixed +// `<pkg>/node_modules/.bin/tsx` lookup always misses. Resolving the CLI entry also means no shell, +// so the Windows `.cmd` shim and its argument quoting are gone with it. +export const daemonSpawnCommand = (): { readonly command: string; readonly args: ReadonlyArray<string> } | null => { const daemonScript = join(moduleDir, "index.ts"); + try { + return { command: process.execPath, args: [requireFromHere.resolve("tsx/cli"), daemonScript] }; + } catch { + return null; + } +}; - // Windows npm binaries are `.cmd` shims, which only launch through a shell — spawning one directly yields EINVAL. - const tsxBinBase = join(moduleDir, "..", "..", "node_modules", ".bin", "tsx"); - const tsxBin = isWindows ? `${tsxBinBase}.cmd` : tsxBinBase; - const hasLocalTsx = existsSync(tsxBin); - - const cmd = hasLocalTsx ? tsxBin : isWindows ? "npx.cmd" : "npx"; - const args = hasLocalTsx ? [daemonScript] : ["tsx", daemonScript]; - - // With shell: true Node passes the command line verbatim, so a path containing spaces must be quoted or it splits into tokens. - const quote = (s: string): string => (isWindows ? `"${s}"` : s); +export const spawnDaemon = (): ChildProcess | null => { + const spec = daemonSpawnCommand(); + if (!spec) return null; - const child = spawn(quote(cmd), args.map(quote), { + const child = spawn(spec.command, [...spec.args], { detached: true, stdio: "ignore", env: { ...process.env }, - shell: isWindows, - windowsVerbatimArguments: isWindows, + windowsHide: true, }); child.unref(); diff --git a/src/domains/js.ts b/src/domains/js.ts index 6ba21cd..19c8913 100644 --- a/src/domains/js.ts +++ b/src/domains/js.ts @@ -1,5 +1,6 @@ import { readFile } from "node:fs/promises"; import { resolve, isAbsolute } from "node:path"; +import { isPathWithin } from "../util/paths"; import { tmpdir } from "node:os"; import { createRequire } from "node:module"; import { Type } from "typebox"; @@ -121,10 +122,7 @@ const allowedRoots = (): ReadonlyArray<string> => { return [tmpdir(), process.cwd(), ...(env !== undefined ? [env] : [])].map((d) => resolve(d)); }; -const isPathAllowed = (p: string): boolean => { - const abs = resolve(p); - return allowedRoots().some((root) => abs === root || abs.startsWith(`${root}/`)); -}; +const isPathAllowed = (p: string): boolean => allowedRoots().some((root) => isPathWithin(root, p)); const MAX_SOURCE_BYTES = 1_000_000; diff --git a/src/util/paths.ts b/src/util/paths.ts index 30a1bbd..3aa1237 100644 --- a/src/util/paths.ts +++ b/src/util/paths.ts @@ -1,6 +1,16 @@ import { randomUUID } from "node:crypto"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { isAbsolute, join, relative, resolve } from "node:path"; + +// `relative` rather than a string prefix: it is separator-agnostic and carries each platform's +// own case rules, so a Windows path is not rejected for its backslashes or its drive-letter case. +export const isPathWithin = (root: string, target: string): boolean => { + const absRoot = resolve(root); + const absTarget = resolve(target); + if (absTarget === absRoot) return true; + const rel = relative(absRoot, absTarget); + return rel !== "" && !rel.startsWith("..") && !isAbsolute(rel); +}; const NAMESPACE_RE = /^[a-zA-Z0-9_-]+$/; diff --git a/test/util/daemon-socket-test.ts b/test/util/daemon-socket-test.ts index eee0500..80a03e4 100644 --- a/test/util/daemon-socket-test.ts +++ b/test/util/daemon-socket-test.ts @@ -1,10 +1,11 @@ import { test, describe } from "node:test"; import assert from "node:assert/strict"; import { fileURLToPath } from "node:url"; +import { existsSync } from "node:fs"; import { Type } from "typebox"; import type { ExtensionAPI, ToolDefinition } from "@mariozechner/pi-coding-agent"; import type { TSchema } from "typebox"; -import { daemonSocketMayExist } from "../../src/daemon/spawn"; +import { daemonSocketMayExist, daemonSpawnCommand } from "../../src/daemon/spawn"; import { type AnyBrowserToolDefinition, registerBrowserTool } from "../../src/util/tool"; import type { BrowserClient } from "../../src/client"; import { ok } from "../../src/util/result"; @@ -32,6 +33,28 @@ describe("daemon socket presence check", () => { }); }); +describe("daemon spawn command", () => { + test("the daemon runs on this process's node, not a shell", () => { + const spec = daemonSpawnCommand(); + assert.notEqual(spec, null); + assert.equal(spec?.command, process.execPath); + }); + + test("the resolved tsx CLI is a real file on disk", () => { + const cli = daemonSpawnCommand()?.args[0]; + assert.equal(typeof cli, "string"); + // A hoisted install puts tsx beside the package, so a `<pkg>/node_modules/.bin` guess would miss it. + assert.equal(existsSync(String(cli)), true, `tsx CLI not found at ${String(cli)}`); + assert.match(String(cli), /cli\.mjs$/); + }); + + test("the daemon entrypoint is passed to tsx", () => { + const script = daemonSpawnCommand()?.args[1]; + assert.match(String(script), /[\\/]daemon[\\/]index\.ts$/); + assert.equal(existsSync(String(script)), true); + }); +}); + type GateProbe = { readonly ran: ReadonlyArray<string>; readonly text: string }; const runGate = async (ensureAlive: boolean | undefined): Promise<GateProbe> => { diff --git a/test/util/paths-test.ts b/test/util/paths-test.ts new file mode 100644 index 0000000..4fff247 --- /dev/null +++ b/test/util/paths-test.ts @@ -0,0 +1,50 @@ +// Runs on every OS in the CI matrix on purpose: `isPathWithin` delegates to `node:path`, so +// only a real Windows run proves the backslash and drive-letter-case cases the old check failed. + +import { test, describe } from "node:test"; +import assert from "node:assert/strict"; +import { tmpdir } from "node:os"; +import { join, resolve, sep } from "node:path"; +import { isPathWithin } from "../../src/util/paths"; + +const root = resolve(tmpdir()); + +describe("path containment", () => { + test("a directory contains itself", () => { + assert.equal(isPathWithin(root, root), true); + }); + + test("a direct child is contained", () => { + assert.equal(isPathWithin(root, join(root, "script.js")), true); + }); + + test("a nested child is contained", () => { + assert.equal(isPathWithin(root, join(root, "a", "b", "script.js")), true); + }); + + test("a native-separator path is contained", () => { + assert.equal(isPathWithin(root, `${root}${sep}script.js`), true); + }); + + test("an unnormalised path that still lands inside is contained", () => { + assert.equal(isPathWithin(root, join(root, "a", "..", "script.js")), true); + }); + + test("an unnormalised path that escapes the root is rejected", () => { + assert.equal(isPathWithin(root, join(root, "..", "..", "evil.js")), false); + }); + + test("a sibling directory sharing the root's name prefix is rejected", () => { + assert.equal(isPathWithin(root, `${root}-sibling${sep}script.js`), false); + }); + + test("the root's own parent is rejected", () => { + assert.equal(isPathWithin(root, join(root, "..")), false); + }); + + test("the roots the run-script tool allows accept a file written into them", () => { + for (const allowed of [tmpdir(), process.cwd()]) { + assert.equal(isPathWithin(allowed, join(allowed, "bh-script.mjs")), true, allowed); + } + }); +}); |
