diff options
| -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); + } + }); +}); |