From 7f3e4c43f5ab1496c6d0be0201ab346ac72009e6 Mon Sep 17 00:00:00 2001 From: ework-agent Date: Tue, 6 Oct 2026 00:51:25 +0800 Subject: [PATCH 1/2] fix: keep user argv out of cmd.exe on the Windows launcher path (#2196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default npm Codex install is a codex.cmd shim whose %* forwarding re-parses every user argument through cmd.exe's line parser — embedded double quotes split tokens, %VAR% expands, &|<>^() execute, empty args vanish. Plain spaced prompts survived, which is why the #679 space-truncation fix did not expose it. - codex resolution (win32): per PATH dir the native codex.exe wins over the shim; a .cmd/.bat hit beside a trusted npm layout (node_modules/@openai/codex named @openai/codex with a resolvable codex bin entry, shim text referencing that package) upgrades to node — same vendor-lookup/env-init/signal-forwarding behavior, lossless CreateProcess argv encoding. BILI_CLIENT_BIN override precedence unchanged. - every remaining comspec wrap site (launcher planClientSpawn + dsh-channel planDshSpawn) now refuses argv the line parser cannot carry verbatim with an actionable error before any spawn, instead of mangling or interpreting it. Direct-spawn .exe paths are untouched (Node encodes argv losslessly). Refusal messages name position + class, never token content (log-masking discipline). - tests/win-cmd-argv.test.ts: predicate table, guard behavior incl. byte-identical safe-plan pins for #679, layout-detection cases, resolution-order cases, and win32-gated real round-trips asserting the child's RECEIVED argv equals the caller array item-by-item over the full corpus (empty arg, spaces, embedded quotes, TOML -c, JSON, Unicode, trailing backslashes, %COMSPEC%, !VAR!, metacharacters) — plus detection against the real global @openai/codex install and the wrapper's --help booting through plain node. - CI: hard gate in ci-windows-codex.yml running the new suite on windows-latest. - docs: CLIENTS.md (+zh) codex section on the new resolution order, pass-through contract, and workaround. --- .github/workflows/ci-windows-codex.yml | 8 + CLIENTS.md | 40 +++ CLIENTS.zh-CN.md | 10 + src/dsh-channel.ts | 12 + src/launcher.ts | 112 +++++- src/win-cmd.ts | 49 +++ tests/fix-679-windows-launch.test.ts | 7 +- tests/win-cmd-argv.test.ts | 457 +++++++++++++++++++++++++ 8 files changed, 687 insertions(+), 8 deletions(-) create mode 100644 src/win-cmd.ts create mode 100644 tests/win-cmd-argv.test.ts diff --git a/.github/workflows/ci-windows-codex.yml b/.github/workflows/ci-windows-codex.yml index 70a0cf6e3..4bd4e8795 100644 --- a/.github/workflows/ci-windows-codex.yml +++ b/.github/workflows/ci-windows-codex.yml @@ -33,6 +33,14 @@ jobs: - name: Install codex CLI shell: pwsh run: npm install -g @openai/codex + # #2196 hard gate (deterministic, no model calls — unlike the soft-gated + # verify below): the Windows argv pass-through contract must hold on a real + # machine — full corpus round-trip with the child's RECEIVED argv asserted + # item-by-item, official-npm-layout detection against the real global + # install, and the wrapper's --help booting through plain node. + - name: Windows argv pass-through tests (#2196) + shell: pwsh + run: node --import tsx --test tests/win-cmd-argv.test.ts # Non-obvious on purpose: do NOT remove continue-on-error until one clean # run proves codex boots unauthenticated on windows-latest. The transcript # artifact shows what happened so this can then become a hard gate. diff --git a/CLIENTS.md b/CLIENTS.md index 2f0bdf034..f6d0a911f 100644 --- a/CLIENTS.md +++ b/CLIENTS.md @@ -365,6 +365,46 @@ The #321 budget `-c` args are kept verbatim (embedded mode honors them identically). If you want the shared background server, run native `codex` directly — no compression, but tools still work via `bili plugin install codex`. +### Windows launch path: user argv never re-enters cmd.exe (#2196) + +On Windows the default npm install puts a `codex.cmd` shim on PATH, and its +`%*` forwarding re-parses every user argument through cmd.exe's LINE parser — +which has no escape mechanism: embedded double quotes split tokens +(`Please say "hello world" exactly` arrived as four arguments), `%VAR%` +expands, `&|<>^()` act as command operators, and empty arguments vanish. +Plain spaced prompts survived, which is why the #679 space-truncation fix did +not expose it. #2196 makes the launcher refuse to feed user argv into that +parser: + +- **Resolution order (win32 only):** within each PATH directory the native + `codex.exe` wins over `codex.cmd`/`codex.bat` (earliest directory still wins + overall); a `.cmd`/`.bat` hit is upgraded to `node ` + when it sits beside a trusted npm layout — `/node_modules/@openai/codex` + whose package.json is named `@openai/codex` with a resolvable `"codex"` bin + entry, AND shim text referencing that package (a hand-written `codex.cmd` + placed beside an unrelated tree must not hijack the launch). Running the + official wrapper under Node reproduces exactly what the shim does — vendor + binary lookup, env init, signal forwarding — while Node's own CreateProcess + argv encoding carries every argument verbatim. Unrecognized layouts + (yarn-classic `.bin` trees, pnpm store shims without the local link, …) keep + the legacy cmd path under the contract below. +- **Pass-through contract:** the remaining cmd-wrapped launches — any client's + `.cmd`/`.bat`/extensionless binary, plus dsh-channel spawns — accept only + argv the line parser can carry verbatim. Anything else (embedded quotes, + `%VAR%`, metacharacters, empty args, line breaks, odd trailing-backslash + runs) fails loudly with an actionable error *before* any process starts, + instead of arriving corrupted or executing unintended commands. Direct-spawn + `.exe` launches are unaffected: Node encodes their argv losslessly itself. +- **Workaround / power-user knob:** `BILI_CLIENT_BIN=` still outranks + everything — point it at the real `codex.exe` (or at a script entry run + under node) to bypass the shim entirely. + +Verified on windows-latest CI against the real global `@openai/codex` install +(`tests/win-cmd-argv.test.ts`, hard gate in `ci-windows-codex.yml`): the full +corpus — empty arg, plain spaces, embedded quotes, TOML `-c` values, JSON, +Unicode, trailing backslashes, `%COMSPEC%`, `!VAR!`, `&|<>^()` — arrives at the +child process item-by-item identical to the caller array. + ## Gemini family (Gemini CLI / iFlow CLI / Qwen Code / Antigravity) Four launchers for the gemini-cli architecture family (#1043 tier 1). Three of diff --git a/CLIENTS.zh-CN.md b/CLIENTS.zh-CN.md index 397b3bd89..00b36a7c2 100644 --- a/CLIENTS.zh-CN.md +++ b/CLIENTS.zh-CN.md @@ -120,6 +120,16 @@ Codex 是唯一一个插件安装无法自给自足的客户端。接缝矩阵 自 ~0.156 起,Codex 可以附着到(或自动拉起)一台机器全局共享的后台 server,其模型流量用的是 **daemon 启动时**存在的环境变量,而不是会话启动时的。启动器的代理是会话级的(端口随进程消亡),所以长寿 daemon 没法安全地经它路由:若 codex 先于 bili env 启动,之后的 `bili codex` 会话会静默附着上去、**完全绕过压缩**;若 bili 先启动,幸存的 daemon 则一直指向死端口。因此启动器显式传 `--no-daemon` —— 先探测 `codex --help` 是否有该 flag(旧版本二进制原样启动),且仅当用户未自行钉死模式(`--no-daemon` 或 `--remote`)时。结果:确定性的 embedded 运行,无逐次启动的回退警告,无静默绕过。#321 的预算 `-c` 参数原样保留(embedded 模式同等生效)。想要共享后台 server 的话,直接跑原生 `codex` —— 没有压缩,但工具仍可通过 `bili plugin install codex` 使用。 +### Windows 启动路径:用户 argv 不再重新进入 cmd.exe(#2196) + +Windows 上默认 npm 安装把 `codex.cmd` shim 放上 PATH,它的 `%*` 转发会让每个用户参数再经 **cmd.exe 行解析器**解析一遍——该解析器没有任何转义机制:嵌入双引号拆词(`Please say "hello world" exactly` 到达时变成四个参数)、`%VAR%` 被展开、`&|<>^()` 成为命令操作符、空参数消失。普通带空格提示词恰好能幸存,所以 #679 的空格截断修复没有暴露它。#2196 让启动器拒绝把用户 argv 喂进这个解析器: + +- **解析顺序(win32 专属):** 每个 PATH 目录内原生 `codex.exe` 优先于 `codex.cmd`/`codex.bat`(最早目录仍整体优先);`.cmd`/`.bat` 命中若位于可信 npm 布局旁——`/node_modules/@openai/codex`,package.json name 为 `@openai/codex`、有可解析的 `"codex"` bin 条目、且 shim 文本引用该包(放在无关树旁的手写 `codex.cmd` 不得劫持启动)——则升级为 `node <官方 bin/codex.js>`。Node 下运行官方 wrapper 与 shim 行为完全一致——vendor 二进制查找、env 初始化、信号转发——而 Node 自己的 CreateProcess argv 编码逐字无损携带每个参数。未识别布局(yarn-classic `.bin` 树、无本地链接的 pnpm store shim……)保留旧 cmd 路径并遵循下述契约。 +- **透传契约:** 其余 cmd 包装启动——任何客户端的 `.cmd`/`.bat`/无扩展名二进制,加上 dsh-channel spawn——只接受行解析器能逐字携带的 argv;其余(嵌入引号、`%VAR%`、元字符、空参数、换行、奇数尾反斜杠)在**任何进程启动之前**以可操作的错误明确失败,而不是损坏到达或执行非预期命令。直接 spawn 的 `.exe` 不受影响:Node 自身无损编码其 argv。 +- **绕行 / power-user 旋钮:** `BILI_CLIENT_BIN=` 仍最高优先——指向真实 `codex.exe`(或经 node 运行的脚本入口)即可完全绕开 shim。 + +已在 windows-latest CI 上对真实全局 `@openai/codex` 安装验证(`tests/win-cmd-argv.test.ts`,`ci-windows-codex.yml` 硬门禁):完整语料——空参数、普通空格、嵌入引号、TOML `-c` 值、JSON、Unicode、尾反斜杠、`%COMSPEC%`、`!VAR!`、`&|<>^()`——逐项与调用方数组相等地到达子进程。 + ## Pi(pi.dev coding agent) Pi 有完整原生模式(`bili plugin install pi`,README 快速上手方案 1);这一节只讲一行表格装不下的内容——**原生拦截实际覆盖哪些模型传输**。pi 是唯一把 WebSocket 模型流量带进环路的宿主。 diff --git a/src/dsh-channel.ts b/src/dsh-channel.ts index 2673ee0e1..8092430a3 100644 --- a/src/dsh-channel.ts +++ b/src/dsh-channel.ts @@ -14,6 +14,7 @@ import path from "node:path"; import { execFile, spawnSync } from "node:child_process"; import { promisify } from "node:util"; import { resolveDshHome } from "./client-config.js"; +import { winCmdUnsafeToken, winCmdRefusalError } from "./win-cmd.js"; export const DSH_PACKAGE = "billion-context"; // The profile owned by dsh's own desktop app. Its billion-context copy is @@ -262,6 +263,17 @@ export function planDshSpawn( const base = command.slice(Math.max(command.lastIndexOf("/"), command.lastIndexOf("\\")) + 1); const needsCmd = lower.endsWith(".cmd") || lower.endsWith(".bat") || !path.extname(base); if (!needsCmd) return { command, args: [...args] }; + // #2196: same safe-set contract as the launcher's comspec path — profile + // names and specs are user-chosen strings that cmd.exe would otherwise + // mangle or interpret silently. + [command, ...args].forEach((token, i) => { + const reason = winCmdUnsafeToken(token); + if (reason) throw winCmdRefusalError( + i === 0 ? "resolved dsh command" : `argument #${i - 1}`, + reason, + "Set BILI_DSH_BIN to the dsh executable, or reword the argument.", + ); + }); const comspec = env.COMSPEC?.trim() || "cmd.exe"; const line = `"${[command, ...args].map(quoteWinToken).join(" ")}"`; return { command: comspec, args: ["/d", "/s", "/c", line], windowsVerbatimArguments: true }; diff --git a/src/launcher.ts b/src/launcher.ts index 080b28670..849724b9a 100644 --- a/src/launcher.ts +++ b/src/launcher.ts @@ -54,6 +54,7 @@ import { import { selfPackageRoot, isBiliPiEntry, ompPluginLoadedFrom, dshNativeInstalled, claudeNativeInstalled } from "./plugin-install.js"; import { applyOmpFirstEventTimeout } from "./agent/native-bootstrap.js"; import { log as teeLog } from "./logger.js"; +import { winCmdUnsafeToken, winCmdRefusalError } from "./win-cmd.js"; /** Absolute path of a file inside our dist/, resolved via the package root * (import.meta.url-based) so it survives global-installed symlink bins @@ -4059,10 +4060,10 @@ async function abortLaunchOnBusyOverlay(handle: ProxyHandle, err: OverlayBusyErr /** #679: quote one token for cmd.exe's line parser. Only whitespace-bearing * tokens get wrapped in double quotes, so a space-free launch produces a - * byte-identical line to the old shell:true form. A token containing an - * embedded double quote stays bare: cmd.exe has no escape mechanism for - * quotes, so wrapping would only change how it is mangled (today's behavior - * preserved). */ + * byte-identical line to the old shell:true form. Pure formatter: callers + * must pre-validate their tokens through the #2196 safe-set guard + * (winCmdUnsafeToken) — an embedded double quote stays bare here only + * because such tokens never reach this path anymore. */ export function quoteWinToken(token: string): string { if (!/\s/.test(token) || token.includes('"')) return token; return `"${token}"`; @@ -4083,7 +4084,11 @@ export function buildWindowsCommandLine(cmd: string, args: readonly string[]): s * extension) spawns directly and the OS quotes the executable and argv * itself, spaces included. shell:true is never used anymore: no DEP0190, no * cmd.exe re-splitting of spaced paths at their first space (which truncated - * both the command and its args). */ + * both the command and its args). + * #2196: the comspec form re-feeds user argv into cmd.exe's LINE parser, + * which has no escape mechanism — so before building the line, every token + * (command included) must pass the safe-set guard; unsafe tokens throw with + * an actionable error instead of being mangled or interpreted silently. */ export function planClientSpawn( cmd: string, args: readonly string[], @@ -4095,6 +4100,15 @@ export function planClientSpawn( const base = cmd.slice(Math.max(cmd.lastIndexOf("/"), cmd.lastIndexOf("\\")) + 1); const needsCmd = lower.endsWith(".cmd") || lower.endsWith(".bat") || !path.extname(base); if (!needsCmd) return { command: cmd, args: [...args] }; + const tokens = [cmd, ...args]; + tokens.forEach((token, i) => { + const reason = winCmdUnsafeToken(token); + if (reason) throw winCmdRefusalError( + i === 0 ? "resolved command" : `argument #${i - 1}`, + reason, + "Set BILI_CLIENT_BIN to the native executable (e.g. the real codex.exe) or to a script entry run under node, or reword the argument.", + ); + }); const comspec = nonEmpty(env.COMSPEC) ? env.COMSPEC : "cmd.exe"; return { command: comspec, @@ -4146,9 +4160,82 @@ export function isOnPath(name: string, env: NodeJS.ProcessEnv): boolean { return resolveOnPath(name, env) !== undefined; } +/** #2196: given a resolved codex .cmd/.bat shim, locate the official npm + * package's JS entry when this is a trusted npm install layout: the shim sits + * beside /node_modules/@openai/codex whose package.json is named + * "@openai/codex" and declares a resolvable "codex" bin entry, AND the shim + * text references that package (npm-generated shims always do; a hand-written + * codex.cmd placed beside an unrelated tree must not hijack the launch). + * Running that entry under Node reproduces exactly what the shim does — its + * vendor-binary lookup, env init, signal forwarding — while Node's own + * CreateProcess argv encoding carries user arguments verbatim instead of + * cmd.exe mangling them (#2196). Returns undefined for any other layout + * (yarn-classic .bin trees, pnpm store shims without the local node_modules + * link, ...): those keep the legacy cmd path under the safe-set guard. */ +export function resolveCodexOfficialJsEntry(shimPath: string): string | undefined { + const pkgDir = path.join(path.dirname(shimPath), "node_modules", "@openai", "codex"); + let pkgJson: unknown; + try { + pkgJson = JSON.parse(fs.readFileSync(path.join(pkgDir, "package.json"), "utf8")); + } catch { + return undefined; + } + if (!pkgJson || typeof pkgJson !== "object") return undefined; + const meta = pkgJson as { name?: unknown; bin?: unknown }; + if (meta.name !== "@openai/codex") return undefined; + let rel: string | undefined; + if (typeof meta.bin === "string") rel = meta.bin; + else if (meta.bin && typeof meta.bin === "object" && !Array.isArray(meta.bin)) { + const v = (meta.bin as Record)["codex"]; + if (typeof v === "string") rel = v; + } + if (!rel) return undefined; + const entry = path.resolve(pkgDir, rel); + try { + if (!fs.statSync(entry).isFile()) return undefined; + } catch { + return undefined; + } + try { + const shimText = fs.readFileSync(shimPath, "utf8").slice(0, 65536); + if (!shimText.includes("@openai\\codex") && !shimText.includes("@openai/codex")) return undefined; + } catch { + return undefined; + } + return entry; +} + +/** #2196: Windows-only codex resolution that never routes through cmd.exe's + * line parser when it can be avoided: per PATH directory the native + * codex.exe wins over codex.cmd/codex.bat (earliest directory still wins + * overall), and a .cmd/.bat hit is upgraded to `node ` + * whenever the trusted npm layout is present. Returns undefined when no + * codex.{exe,cmd,bat} exists anywhere — the caller falls through to the + * generic resolution (which also covers the extensionless case). */ +function resolveCodexWin32(env: NodeJS.ProcessEnv): { command: string; prefixArgs: string[] } | undefined { + const p = env.PATH; + if (!p) return undefined; + for (const dir of p.split(path.delimiter)) { + if (!dir) continue; + for (const ext of [".exe", ".cmd", ".bat"]) { + const f = path.join(dir, "codex" + ext); + try { + if (fs.existsSync(f) && fs.statSync(f).isFile()) { + if (ext === ".exe") return { command: f, prefixArgs: [] }; + const entry = resolveCodexOfficialJsEntry(f); + if (entry) return { command: process.execPath, prefixArgs: [entry] }; + return { command: f, prefixArgs: [] }; + } + } catch {} + } + } + return undefined; +} + export function resolveClientCommand( client: ClientName, env: NodeJS.ProcessEnv, + platform: NodeJS.Platform = process.platform, ): { command: string; prefixArgs: string[] } { const binOverride = env.BILI_CLIENT_BIN?.trim(); if (binOverride) { @@ -4223,6 +4310,17 @@ export function resolveClientCommand( } return { command: base, prefixArgs: [] }; } + if (client === "codex" && platform === "win32") { + // #2196: the default npm install is a codex.cmd shim whose %* + // forwarding re-parses every user argument through cmd.exe (embedded + // quotes split tokens, %VAR% expands, &|<>^() execute). Prefer forms + // that never enter that parser; a leftover .cmd/.bat only reaches + // cmd.exe when neither exists — where the safe-set guard in + // planClientSpawn then refuses argv it cannot carry verbatim instead + // of corrupting it silently. + const win32Hit = resolveCodexWin32(env); + if (win32Hit) return win32Hit; + } const resolved = resolveOnPath(client, env); return { command: resolved ?? client, prefixArgs: [] }; } @@ -4756,7 +4854,7 @@ export async function runLaunch(params: RunLaunchParams, deps: LauncherDeps = {} env = buildCodexEnv(origin, codexCaPath, stripInheritedProxy(process.env)); clientArgs = buildCodexArgs(origin, routes.httpRewrites, routes.httpsRewrites, clientArgs); if (!codexRunModePinned(params.clientArgs)) { - const { command: codexBin, prefixArgs: codexPrefix } = resolveClientCommand(base, process.env); + const { command: codexBin, prefixArgs: codexPrefix } = resolveClientCommand(base, process.env, deps.platform ?? process.platform); if (codexSupportsNoDaemon(codexBin, codexPrefix, deps.platform ?? process.platform)) { clientArgs = ["--no-daemon", ...clientArgs]; console.error("bili: codex pinned to embedded mode (--no-daemon) — the launcher proxy is session-scoped; a shared background server would outlive it and bypass compression."); @@ -4867,7 +4965,7 @@ export async function runLaunch(params: RunLaunchParams, deps: LauncherDeps = {} } } - const { command, prefixArgs } = resolveClientCommand(base, process.env); + const { command, prefixArgs } = resolveClientCommand(base, process.env, deps.platform ?? process.platform); const effectiveClientArgs = piTestArgs(params.client, clientArgs); let code = 0; try { diff --git a/src/win-cmd.ts b/src/win-cmd.ts new file mode 100644 index 000000000..ec3c68d88 --- /dev/null +++ b/src/win-cmd.ts @@ -0,0 +1,49 @@ +/** #2196: argv safety contract for the cmd.exe pass-through path. + * + * On Windows, .cmd/.bat shims and extensionless names cannot be executed by + * CreateProcess, so bili wraps them as `comspec /d /s /c ""` (#679) and + * lets cmd.exe re-parse that line. cmd.exe has NO escape mechanism: embedded + * double quotes split tokens, %VAR% expands, &|<>^() act as command operators, + * empty tokens vanish, and line breaks terminate the command. An argument in + * any of those shapes CANNOT survive that trip verbatim — the old code just + * let it be mangled (or worse, interpreted) silently. + * + * This module defines which tokens are unsafe for that path so every comspec + * wrap site (launcher planClientSpawn, dsh-channel planDshSpawn) refuses with + * an actionable error instead of corrupting user argv. Direct-spawn forms + * (.exe and friends) never go through this check: Node encodes their argv + * losslessly itself. + */ + +/** Returns a human-readable reason why this token cannot cross the cmd.exe + * line parser verbatim, or undefined when it can (the safe set): plain text, + * whitespace-bearing text quoted whole, Unicode, semicolons/commas/equals, + * backslashes in even runs or outside quoted positions, and `!` (delayed + * expansion is opt-in via ^setlocal enabledelayedexpansion^ and standard npm + * shims never enable it). The offending CONTENT is deliberately not part of + * the return value — callers may log the reason but must not echo the token + * (prompts routinely carry secrets; see AGENTS.md log-masking rule). */ +export function winCmdUnsafeToken(token: string): string | undefined { + if (token === "") return "an empty argument"; + if (token.includes('"')) return "a double quote"; + if (token.includes("%")) return "% (cmd.exe expands %VAR%)"; + if (/[&|<>^()]/.test(token)) return "cmd metacharacters (& | < > ^ ( ))"; + if (/[\r\n]/.test(token)) return "a line break"; + // Odd trailing-backslash run matters only when quoteWinToken will wrap the + // token in quotes (whitespace-bearing, quote-free): the final `\` then + // escapes the closing quote in MSVCRT parsing and swallows it. + if (/\s/.test(token)) { + const trail = /\\+$/.exec(token)?.[0].length ?? 0; + if (trail % 2 === 1) return "an odd number of trailing backslashes"; + } + return undefined; +} + +/** Builds the refusal error for `what` + `reason` with a lane-specific hint + * pointing at the lossless alternatives. */ +export function winCmdRefusalError(what: string, reason: string, hint: string): Error { + return new Error( + `this ${what} cannot be passed through cmd.exe: it contains ${reason}. ` + + `cmd.exe re-parses the whole command line — embedded quotes split tokens, %VAR% expands, &|<>^() are command operators — so such arguments arrive corrupted or execute unintended commands. ${hint}`, + ); +} diff --git a/tests/fix-679-windows-launch.test.ts b/tests/fix-679-windows-launch.test.ts index ee4e43c7e..72f35c871 100644 --- a/tests/fix-679-windows-launch.test.ts +++ b/tests/fix-679-windows-launch.test.ts @@ -30,7 +30,12 @@ test("quoteWinToken: whitespace-bearing tokens get wrapped in double quotes", () assert.equal(quoteWinToken("a\tb"), '"a\tb"'); }); -test("quoteWinToken: embedded double quotes stay bare (cmd.exe has no quote escape — today's behavior preserved)", () => { +// Formatter-level only: quoteWinToken is a pure formatter whose callers must +// pre-validate through the #2196 safe-set guard — planClientSpawn refuses +// embedded-quote tokens before they ever reach this function (see +// win-cmd-argv.test.ts). The bare output here documents what the formatter +// does with such input, NOT that it is a safe pass-through. +test("quoteWinToken: embedded double quotes stay bare at the formatter level (rejected upstream since #2196)", () => { assert.equal(quoteWinToken('key="value"'), 'key="value"'); assert.equal(quoteWinToken('C:\\x "q" y'), 'C:\\x "q" y'); }); diff --git a/tests/win-cmd-argv.test.ts b/tests/win-cmd-argv.test.ts new file mode 100644 index 000000000..5bf0c93a2 --- /dev/null +++ b/tests/win-cmd-argv.test.ts @@ -0,0 +1,457 @@ +// #2196: launching Codex through its default npm shim (codex.cmd) re-fed user +// argv into cmd.exe's line parser — embedded double quotes split tokens, +// %VAR% expanded, &|<>^() executed, empty args vanished. Fix: the codex path +// bypasses the shell (native codex.exe, or the official npm package's JS entry +// under Node), and every remaining comspec wrap site (launcher + dsh channel) +// refuses argv the line parser cannot carry verbatim instead of mangling it. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { spawnSync } from "node:child_process"; +import { + planClientSpawn, + resolveClientCommand, + resolveCodexOfficialJsEntry, + resolveOnPath, + runClient, + type SpawnChild, + type SpawnFn, +} from "../src/launcher.ts"; +import { planDshSpawn } from "../src/dsh-channel.ts"; +import { winCmdUnsafeToken } from "../src/win-cmd.ts"; +import { rmrf } from "./tmp-rm.ts"; + +const WIN_ENV: NodeJS.ProcessEnv = { COMSPEC: "C:\\Windows\\System32\\cmd.exe" }; + +function fakeChild(exitCode = 0): SpawnChild { + return { + pid: 42424, + unref() {}, + kill() { + return true; + }, + on(event, listener) { + if (event === "exit") setImmediate(() => listener(exitCode, null)); + return undefined; + }, + }; +} + +function captureSpawn(): { captured: { cmd?: string; args?: readonly string[] }; spawnImpl: SpawnFn } { + const captured: { cmd?: string; args?: readonly string[] } = {}; + const spawnImpl: SpawnFn = (cmd, args) => { + captured.cmd = cmd; + captured.args = [...args]; + return fakeChild(); + }; + return { captured, spawnImpl }; +} + +// ---- the safe-set predicate itself ---------------------------------------- + +test("winCmdUnsafeToken: the safe set passes", () => { + const safe = [ + "hello", + "hello world", + "-c", + "a;b,c=d", + String.raw`C:\x\y\z`, + "tailbs-nospace\\", + String.raw`even trailing \\ with space`, + "!VAR! delayed expansion marker", + "tab\there", + "unicode ✓ 日本語 🎉", + ]; + for (const t of safe) assert.equal(winCmdUnsafeToken(t), undefined, `expected safe: ${JSON.stringify(t)}`); +}); + +test("winCmdUnsafeToken: every #2196 corruption class is refused", () => { + const unsafe: Array<[string, string]> = [ + ["", "an empty argument"], + ['key="value"', "a double quote"], + ["literal %COMSPEC% marker", "% (cmd.exe expands %VAR%)"], + ["hello&rem", "cmd metacharacters (& | < > ^ ( ))"], + ["a|b", "cmd metacharacters (& | < > ^ ( ))"], + ["a ^ ( ))"], + ["a>b", "cmd metacharacters (& | < > ^ ( ))"], + ["a^b", "cmd metacharacters (& | < > ^ ( ))"], + ["(a)", "cmd metacharacters (& | < > ^ ( ))"], + ["line\nbreak", "a line break"], + ["odd trailing \\", "an odd number of trailing backslashes"], + ]; + for (const [token, reason] of unsafe) { + assert.equal(winCmdUnsafeToken(token), reason, `expected unsafe: ${JSON.stringify(token)}`); + } +}); + +// ---- planClientSpawn guard -------------------------------------------------- + +test("planClientSpawn win32: safe argv keeps the exact #679 plan (byte-identical regression pin)", () => { + const p = planClientSpawn(String.raw`C:\npm\codex.cmd`, ["exec", "hello world"], WIN_ENV, "win32"); + assert.equal(p.command, "C:\\Windows\\System32\\cmd.exe"); + assert.deepEqual(p.args.slice(0, 3), ["/d", "/s", "/c"]); + assert.equal(p.args[3], String.raw`"C:\npm\codex.cmd exec "hello world""`); + assert.equal(p.windowsVerbatimArguments, true); +}); + +test("planClientSpawn win32: '!' and other non-metacharacters stay allowed on the comspec path", () => { + const p = planClientSpawn("tool.cmd", ["!VAR!", "a;b,c=d"], WIN_ENV, "win32"); + assert.equal(p.command, "C:\\Windows\\System32\\cmd.exe"); + assert.equal(p.args[3], '"tool.cmd !VAR! a;b,c=d"'); +}); + +test("planClientSpawn win32: .exe direct spawn never triggers the guard (Node encodes argv losslessly)", () => { + const p = planClientSpawn(String.raw`C:\Program Files\nodejs\node.exe`, ['a"b', "c%d", "e&f", ""], WIN_ENV, "win32"); + assert.equal(p.command, String.raw`C:\Program Files\nodejs\node.exe`); + assert.deepEqual(p.args, ['a"b', "c%d", "e&f", ""]); + assert.equal(p.windowsVerbatimArguments, undefined); +}); + +test("planClientSpawn: non-win32 never triggers the guard", () => { + const p = planClientSpawn("/usr/local/bin/tool.sh", ['a"b', "c&d"], {}, "linux"); + assert.deepEqual(p, { command: "/usr/local/bin/tool.sh", args: ['a"b', "c&d"] }); +}); + +for (const [label, args] of [ + ["embedded double quotes (the issue repro)", ["exec", 'Please say "hello world" exactly']], + ["TOML -c value", ["-c", 'model_providers.demo.name="My Provider"', "exec", "hello"]], + ["empty argument", ["exec", ""]], + ["%VAR%", ["exec", "literal %COMSPEC% marker"]], + ["& metacharacter", ["exec", "hello&rem"]], + ["odd trailing backslash", ["exec", "odd tail \\"]], +] as const) { + test(`planClientSpawn win32: refuses ${label} on the comspec path with an actionable error`, () => { + assert.throws( + () => planClientSpawn(String.raw`C:\npm\codex.cmd`, [...args], WIN_ENV, "win32"), + (err: Error) => { + assert.ok(err.message.includes("BILI_CLIENT_BIN"), `hint missing: ${err.message}`); + return true; + }, + ); + }); +} + +test("planClientSpawn win32: the refusal names the position and class but never the token content", () => { + try { + planClientSpawn("tool.cmd", ['sk-live-supersecret-123 "quoted"'], WIN_ENV, "win32"); + assert.fail("must throw"); + } catch (err) { + const msg = (err as Error).message; + assert.ok(msg.includes("argument #0"), `position missing: ${msg}`); + assert.ok(msg.includes("double quote"), `class missing: ${msg}`); + assert.ok(!msg.includes("sk-live-supersecret-123"), "token content must not leak into the error"); + } +}); + +test("planClientSpawn win32: an unsafe COMMAND path is refused too", () => { + assert.throws( + () => planClientSpawn(String.raw`C:\bad&dir\tool.cmd`, [], WIN_ENV, "win32"), + (err: Error) => err.message.includes("resolved command"), + ); +}); + +test("runClient win32: the issue repro refuses before any spawn happens", async () => { + const { captured, spawnImpl } = captureSpawn(); + let threw: unknown; + try { + await runClient(String.raw`C:\npm\codex.cmd`, ["exec", 'Please say "hello world" exactly'], WIN_ENV, { + spawnImpl, + platform: "win32", + }); + } catch (err) { + threw = err; + } + assert.ok(threw instanceof Error, "must throw/reject"); + assert.ok((threw as Error).message.includes("BILI_CLIENT_BIN")); + assert.equal(captured.cmd, undefined, "spawn must never be reached"); +}); + +// ---- dsh channel shares the same contract ----------------------------------- + +test("planDshSpawn win32: safe profile args keep the #679 plan", () => { + const p = planDshSpawn(String.raw`C:\tools\dsh.cmd`, ["plugin", "--profile", "my profile", "add", "billion-context"], WIN_ENV, "win32"); + assert.equal(p.command, "C:\\Windows\\System32\\cmd.exe"); + assert.deepEqual(p.args.slice(0, 3), ["/d", "/s", "/c"]); + assert.equal(p.args[3], String.raw`"C:\tools\dsh.cmd plugin --profile "my profile" add billion-context"`); +}); + +test("planDshSpawn win32: a metacharacter-laden profile name is refused, not mangled", () => { + assert.throws( + () => planDshSpawn(String.raw`C:\tools\dsh.cmd`, ["plugin", "--profile", "p&q", "add", "billion-context"], WIN_ENV, "win32"), + (err: Error) => err.message.includes("BILI_DSH_BIN"), + ); +}); + +test("planDshSpawn non-win32: untouched", () => { + const p = planDshSpawn("/usr/local/bin/dsh", ["plugin", "--profile", "p&q"], {}, "linux"); + assert.deepEqual(p, { command: "/usr/local/bin/dsh", args: ["plugin", "--profile", "p&q"] }); +}); + +// ---- official npm layout detection ------------------------------------------- + +const NPM_SHIM_TEXT = [ + "@ECHO off", + "GOTO start", + ":find_dp0", + "set dp0=%~dp0", + "EXIT /b", + ":start", + "setlocal", + "call :find_dp0", + String.raw`"%dp0%\node_modules\@openai\codex\bin\codex.js" %*`, +].join("\r\n"); + +function makeNpmLayout(root: string, opts: { name?: string; bin?: unknown; binFile?: boolean; shimText?: string; pkgJsonRaw?: string } = {}): { shim: string; entry: string } { + const globalDir = path.join(root, "npm-global"); + const pkgDir = path.join(globalDir, "node_modules", "@openai", "codex"); + fs.mkdirSync(path.join(pkgDir, "bin"), { recursive: true }); + const entry = path.join(pkgDir, "bin", "codex.js"); + if (opts.binFile !== false) fs.writeFileSync(entry, "// fake official codex entry\n"); + const pkgJson = opts.pkgJsonRaw ?? JSON.stringify({ name: opts.name ?? "@openai/codex", version: "0.160.0", bin: opts.bin ?? { codex: "bin/codex.js" } }); + fs.writeFileSync(path.join(pkgDir, "package.json"), pkgJson); + const shim = path.join(globalDir, "codex.cmd"); + fs.writeFileSync(shim, opts.shimText ?? NPM_SHIM_TEXT); + return { shim, entry }; +} + +test("resolveCodexOfficialJsEntry: full trusted npm layout resolves the JS entry", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-layout-")); + try { + const { shim, entry } = makeNpmLayout(root); + assert.equal(resolveCodexOfficialJsEntry(shim), entry); + } finally { + rmrf(root); + } +}); + +test("resolveCodexOfficialJsEntry: string-form bin field also resolves", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-layout-")); + try { + const { shim, entry } = makeNpmLayout(root, { bin: "bin/codex.js" }); + assert.equal(resolveCodexOfficialJsEntry(shim), entry); + } finally { + rmrf(root); + } +}); + +test("resolveCodexOfficialJsEntry: anything off the trusted layout stays undefined", () => { + const cases: Array<[string, (r: string) => string]> = [ + ["wrong package name", (r) => makeNpmLayout(r, { name: "@evil/codex" }).shim], + ["bin without codex key", (r) => makeNpmLayout(r, { bin: { other: "bin/codex.js" } }).shim], + ["bin pointing at a missing file", (r) => makeNpmLayout(r, { binFile: false }).shim], + ["malformed package.json", (r) => makeNpmLayout(r, { pkgJsonRaw: "{not json" }).shim], + ["shim text not referencing the package", (r) => makeNpmLayout(r, { shimText: "@echo off\r\nnode capture.mjs %*\r\n" }).shim], + ["missing package.json", (r) => { + const { shim } = makeNpmLayout(r); + fs.rmSync(path.join(r, "npm-global", "node_modules", "@openai", "codex", "package.json")); + return shim; + }], + ]; + for (const [label, build] of cases) { + const r = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-layout-")); + try { + assert.equal(resolveCodexOfficialJsEntry(build(r)), undefined, label); + } finally { + rmrf(r); + } + } +}); + +// ---- codex resolution order ---------------------------------------------------- + +function makePathEnv(dirs: string[]): NodeJS.ProcessEnv { + return { PATH: dirs.join(path.delimiter) }; +} + +test("resolveClientCommand codex win32: npm shim with trusted layout upgrades to node + official JS entry", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-resolve-")); + try { + const { shim, entry } = makeNpmLayout(root); + const resolved = resolveClientCommand("codex", makePathEnv([path.dirname(shim)]), "win32"); + assert.equal(resolved.command, process.execPath); + assert.deepEqual(resolved.prefixArgs, [entry]); + } finally { + rmrf(root); + } +}); + +test("resolveClientCommand codex win32: native codex.exe beats the shim in the same directory", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-resolve-")); + try { + const { shim } = makeNpmLayout(root); + const dir = path.dirname(shim); + const exe = path.join(dir, "codex.exe"); + fs.writeFileSync(exe, ""); + const resolved = resolveClientCommand("codex", makePathEnv([dir]), "win32"); + assert.equal(resolved.command, exe, "exe-first within a directory — no shell layer at all"); + assert.deepEqual(resolved.prefixArgs, []); + } finally { + rmrf(root); + } +}); + +test("resolveClientCommand codex win32: earlier PATH directory still wins overall (cmd there beats exe later)", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-resolve-")); + try { + const dirA = path.join(root, "a"); + const dirB = path.join(root, "b"); + fs.mkdirSync(dirA, { recursive: true }); + fs.mkdirSync(dirB, { recursive: true }); + const cmdA = path.join(dirA, "codex.cmd"); + fs.writeFileSync(cmdA, NPM_SHIM_TEXT); + const exeB = path.join(dirB, "codex.exe"); + fs.writeFileSync(exeB, ""); + const resolved = resolveClientCommand("codex", makePathEnv([dirA, dirB]), "win32"); + assert.equal(resolved.command, cmdA, "earliest-dir rule preserved — the guard downstream decides safety"); + } finally { + rmrf(root); + } +}); + +test("resolveClientCommand codex win32: shim without trusted layout keeps the legacy cmd path", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-resolve-")); + try { + const dir = path.join(root, "bin"); + fs.mkdirSync(dir, { recursive: true }); + const shim = path.join(dir, "codex.cmd"); + fs.writeFileSync(shim, "@echo off\r\nnode capture.mjs %*\r\n"); + const resolved = resolveClientCommand("codex", makePathEnv([dir]), "win32"); + assert.equal(resolved.command, shim); + assert.deepEqual(resolved.prefixArgs, []); + } finally { + rmrf(root); + } +}); + +test("resolveClientCommand codex win32: no codex anywhere falls through to the bare-name fallback", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-resolve-")); + try { + const resolved = resolveClientCommand("codex", makePathEnv([root]), "win32"); + assert.deepEqual(resolved, { command: "codex", prefixArgs: [] }); + } finally { + rmrf(root); + } +}); + +test("resolveClientCommand: BILI_CLIENT_BIN override still wins over the codex branch", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-resolve-")); + try { + const { entry } = makeNpmLayout(root); + const env = makePathEnv([path.dirname(path.join(root, "npm-global", "codex.cmd"))]); + env.BILI_CLIENT_BIN = "my-codex"; + const resolved = resolveClientCommand("codex", env, "win32"); + assert.equal(resolved.command, "my-codex"); + assert.deepEqual(resolved.prefixArgs, []); + assert.notEqual(resolved.command, process.execPath); + assert.notEqual(resolved.prefixArgs[0], entry); + } finally { + rmrf(root); + } +}); + +test("resolveClientCommand: non-codex clients are untouched by the win32 branch", () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-resolve-")); + try { + const dir = path.join(root, "bin"); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, "claude.cmd"), "@echo off\r\n"); + // A trusted codex layout sits on the same PATH — it must not leak + // onto another client's launch (the codex branch is client-scoped). + makeNpmLayout(root); + const resolved = resolveClientCommand("claude", makePathEnv([dir, path.join(root, "npm-global")]), "win32"); + assert.notEqual(resolved.command, process.execPath, "no node-entry hijack for non-codex clients"); + assert.deepEqual(resolved.prefixArgs, []); + } finally { + rmrf(root); + } +}); + +// ---- real round-trips (Windows only) --------------------------------------------- +// These are the acceptance lane: the child process's RECEIVED argv must equal +// the caller's array item-by-item — not the constructed command line. + +const ARGV_CORPUS: string[][] = [ + [], + ["exec", "hello world"], + ["exec", 'Please say "hello world" exactly'], + ["-c", 'model_providers.demo.name="My Provider"', "exec", "hello"], + ["--json", '{"a": 1, "b": [true, null]}'], + ["exec", ""], + ["exec", "unicode ✓ 日本語 🎉"], + ["exec", "trailing backslash \\"], + ["exec", String.raw`even trailing \\ ok`], + ["exec", "literal %COMSPEC% marker"], + ["exec", "delayed !VAR! marker"], + ["exec", "meta & | < > ^ ( ) chars"], +]; + +const CAPTURE_SCRIPT = 'require("node:fs").writeFileSync(process.env.CAPTURE_OUT, JSON.stringify(process.argv.slice(2)));'; + +async function roundTripVia(label: string, command: string, prefixArgs: string[], corpus: string[], outFile: string): Promise { + const code = await runClient(command, [...prefixArgs, ...corpus], { ...process.env, CAPTURE_OUT: outFile }, { platform: "win32" }); + assert.equal(code, 0, `${label}: exit 0`); + const seen = JSON.parse(fs.readFileSync(outFile, "utf8")) as string[]; + assert.deepEqual(seen, corpus, `${label}: received argv must equal the caller array item-by-item`); +} + +test("#2196 real win32: node + official-wrapper-shaped entry carries the full corpus verbatim", { skip: process.platform !== "win32" }, async (t) => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-rt-")); + try { + // A fake @openai/codex whose bin/codex.js mirrors the real wrapper's + // forwarding semantics (spawn/record process.argv.slice(2)): detection + // picks it up exactly like the production install. + const { entry } = makeNpmLayout(root); + fs.writeFileSync(entry, CAPTURE_SCRIPT); + const resolved = resolveClientCommand("codex", makePathEnv([path.dirname(path.join(root, "npm-global", "codex.cmd"))]), "win32"); + assert.equal(resolved.command, process.execPath); + assert.deepEqual(resolved.prefixArgs, [entry]); + for (const corpus of ARGV_CORPUS) { + const outFile = path.join(root, `argv-${t.name}-${ARGV_CORPUS.indexOf(corpus)}.json`); + await roundTripVia(JSON.stringify(corpus), resolved.command, resolved.prefixArgs, corpus, outFile); + } + } finally { + rmrf(root); + } +}); + +test("#2196 real win32: the direct-spawn leg alone is lossless for the full corpus", { skip: process.platform !== "win32" }, async (t) => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "bili-2196-rt-")); + try { + const script = path.join(root, "capture.cjs"); + fs.writeFileSync(script, CAPTURE_SCRIPT); + for (const corpus of ARGV_CORPUS) { + const outFile = path.join(root, `direct-${ARGV_CORPUS.indexOf(corpus)}.json`); + await roundTripVia(JSON.stringify(corpus), process.execPath, [script], corpus, outFile); + } + } finally { + rmrf(root); + } +}); + +test("#2196 real win32: a global @openai/codex install is detected (production layout check)", { skip: process.platform !== "win32" }, () => { + const shim = resolveOnPath("codex", process.env); + if (!shim || !/\.cmd$/i.test(shim)) return; // not installed here — CI installs it + const entry = resolveCodexOfficialJsEntry(shim); + assert.ok(entry, "the production npm layout must match the detector"); + const expected = path.join(path.dirname(shim), "node_modules", "@openai", "codex"); + const pkgJson = JSON.parse(fs.readFileSync(path.join(expected, "package.json"), "utf8")) as { name: string; bin: unknown }; + assert.equal(pkgJson.name, "@openai/codex"); + const resolved = resolveClientCommand("codex", process.env, "win32"); + assert.equal(resolved.command, process.execPath); + assert.deepEqual(resolved.prefixArgs, [entry]); +}); + +test("#2196 real win32: the official wrapper answers --help through plain node (probe path intact)", { skip: process.platform !== "win32" }, () => { + const shim = resolveOnPath("codex", process.env); + if (!shim || !/\.cmd$/i.test(shim)) return; + const entry = resolveCodexOfficialJsEntry(shim); + if (!entry) return; + // The production launch shape for this path is exactly `node ` + // — prove the official wrapper boots its vendor binary through it, offline. + const res = spawnSync(process.execPath, [entry, "--help"], { timeout: 15000, stdio: ["ignore", "pipe", "ignore"] }); + assert.equal(res.status, 0, `wrapper --help must exit 0 (stderr: ${res.stderr.toString("utf8").slice(0, 200)})`); + assert.ok(res.stdout.toString("utf8").length > 0); +}); From 3b28155116eb37fbd7b0e16c7b642619d06ac6df Mon Sep 17 00:00:00 2001 From: ework-agent Date: Tue, 6 Oct 2026 00:59:14 +0800 Subject: [PATCH 2/2] test: fix eager null-stderr interpolation in the wrapper --help probe test (#2196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The assertion message interpolated res.stderr while stderr was set to 'ignore' (null) — the interpolation threw unconditionally before the status check could report. Pipe stderr and assert.fail with a guarded message. --- tests/win-cmd-argv.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/win-cmd-argv.test.ts b/tests/win-cmd-argv.test.ts index 5bf0c93a2..3578c6493 100644 --- a/tests/win-cmd-argv.test.ts +++ b/tests/win-cmd-argv.test.ts @@ -451,7 +451,9 @@ test("#2196 real win32: the official wrapper answers --help through plain node ( if (!entry) return; // The production launch shape for this path is exactly `node ` // — prove the official wrapper boots its vendor binary through it, offline. - const res = spawnSync(process.execPath, [entry, "--help"], { timeout: 15000, stdio: ["ignore", "pipe", "ignore"] }); - assert.equal(res.status, 0, `wrapper --help must exit 0 (stderr: ${res.stderr.toString("utf8").slice(0, 200)})`); + const res = spawnSync(process.execPath, [entry, "--help"], { timeout: 15000, stdio: ["ignore", "pipe", "pipe"] }); + if (res.status !== 0 || res.error) { + assert.fail(`wrapper --help exited ${res.status} (${res.error ?? ""}): ${res.stderr?.toString("utf8").slice(0, 300) ?? ""}`); + } assert.ok(res.stdout.toString("utf8").length > 0); });