From c6548b98f4aff5feedcada4731abe44bd95beaf3 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Sun, 13 Sep 2026 16:08:56 -0400 Subject: [PATCH] test(scripts): widen the Windows shim ratchet to catch package bin spawns (#20285) * Widen Windows shim ratchet to detect package bin spawns Follow local program expressions into node_modules/.bin while preserving the existing literal check, roots, and allow-list. Document static-analysis limits and cover unsafe and resolver-based invocations. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(scripts): fold dot segments before matching node_modules/.bin The predicate joins call arguments textually, so a literal '..' segment hid a path that resolves into node_modules/.bin at runtime. Folds '.' and '..' (and Windows separators) first. A '..' that genuinely escapes .bin still does not match. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * style(scripts): use .at(-1) in the dot-segment fold oxlint's prefer-at rule; the repo-wide lint gate is an error, not a warning. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../scripts/windows-bin-spawn-predicate.mjs | 100 ++++++++++++++++++ .../windows-bin-spawn-predicate.test.mjs | 70 ++++++++++++ .../windows-cmd-shim-spawn-boundary.test.mjs | 52 ++++----- 3 files changed, 189 insertions(+), 33 deletions(-) create mode 100644 config/scripts/windows-bin-spawn-predicate.mjs create mode 100644 config/scripts/windows-bin-spawn-predicate.test.mjs diff --git a/config/scripts/windows-bin-spawn-predicate.mjs b/config/scripts/windows-bin-spawn-predicate.mjs new file mode 100644 index 00000000000..5a24fbcc61a --- /dev/null +++ b/config/scripts/windows-bin-spawn-predicate.mjs @@ -0,0 +1,100 @@ +import ts from 'typescript-api' + +const SPAWN_METHODS = new Set(['spawn', 'spawnSync', 'execFile', 'execFileSync']) +const BIN_PATH = /(?:^|[\\/])node_modules[\\/]\.bin(?:[\\/]|$)/i + +export function hasNodeModulesBinSpawn(contents) { + const source = ts.createSourceFile( + 'script.mjs', + contents, + ts.ScriptTarget.Latest, + true, + ts.ScriptKind.JS + ) + const bindings = new Map() + const calls = [] + function visit(node) { + if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name) && node.initializer) { + const initializers = bindings.get(node.name.text) ?? [] + initializers.push(node.initializer) + bindings.set(node.name.text, initializers) + } + if (ts.isCallExpression(node)) { + calls.push(node) + } + ts.forEachChild(node, visit) + } + visit(source) + + function name(node) { + return ts.isIdentifier(node) + ? node.text + : ts.isPropertyAccessExpression(node) + ? node.name.text + : '' + } + + function paths(node, seen = new Set()) { + if (!node || seen.has(node)) { + return ['?'] + } + const next = new Set(seen).add(node) + if (ts.isStringLiteral(node) || ts.isNoSubstitutionTemplateLiteral(node)) { + return [node.text] + } + if (ts.isIdentifier(node)) { + return (bindings.get(node.text) ?? []).flatMap((value) => paths(value, next)) + } + if (ts.isParenthesizedExpression(node)) { + return paths(node.expression, next) + } + if (ts.isConditionalExpression(node)) { + return [...paths(node.whenTrue, next), ...paths(node.whenFalse, next)] + } + if (ts.isTemplateExpression(node)) { + return node.templateSpans.reduce( + (prefixes, span) => combine(prefixes, paths(span.expression, next), '', span.literal.text), + [node.head.text] + ) + } + if (ts.isBinaryExpression(node) && node.operatorToken.kind === ts.SyntaxKind.PlusToken) { + return combine(paths(node.left, next), paths(node.right, next)) + } + if (ts.isCallExpression(node) && ['join', 'resolve'].includes(name(node.expression))) { + return node.arguments.reduce( + (prefixes, arg) => combine(prefixes, paths(arg, next), '/'), + [''] + ) + } + return ['?'] + } + + function combine(left, right, separator = '', suffix = '') { + return (left.length ? left : ['?']).flatMap((prefix) => + (right.length ? right : ['?']).map((part) => `${prefix}${separator}${part}${suffix}`) + ) + } + + // Why: arguments are folded textually, so a literal '..' segment would otherwise hide a + // path that resolves into node_modules/.bin at runtime. + function foldDotSegments(value) { + const folded = [] + for (const segment of value.replace(/\\/g, '/').split('/')) { + if (segment === '.' || segment === '') { + continue + } + if (segment === '..' && folded.length && folded.at(-1) !== '..') { + folded.pop() + continue + } + folded.push(segment) + } + return folded.join('/') + } + + return calls.some( + (call) => + SPAWN_METHODS.has(name(call.expression)) && + paths(call.arguments[0]).some((value) => BIN_PATH.test(foldDotSegments(value))) + ) +} diff --git a/config/scripts/windows-bin-spawn-predicate.test.mjs b/config/scripts/windows-bin-spawn-predicate.test.mjs new file mode 100644 index 00000000000..85c539dd39b --- /dev/null +++ b/config/scripts/windows-bin-spawn-predicate.test.mjs @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest' +import { hasNodeModulesBinSpawn } from './windows-bin-spawn-predicate.mjs' + +describe('node_modules bin spawn predicate', () => { + it.each(['spawn', 'spawnSync', 'execFile', 'execFileSync'])( + 'catches %s through local aliases', + (method) => { + expect( + hasNodeModulesBinSpawn(` + const bin = path.join(root, 'node_modules', '.bin', 'oxfmt') + const program = bin + cp.${method}(program, ['--write', file]) + `) + ).toBe(true) + } + ) + + it.each([ + "path.resolve(root, 'node_modules/.bin/oxfmt')", + '`' + '${root}/node_modules/.bin/oxfmt' + '`', + "root + '/node_modules/' + '.bin/oxfmt'", + "'C:\\\\repo\\\\node_modules\\\\.bin\\\\oxfmt'", + "enabled ? 'node_modules/.bin/oxfmt' : 'oxfmt'" + ])('catches the path expression %s', (expression) => { + expect(hasNodeModulesBinSpawn(`execFileSync(${expression}, [])`)).toBe(true) + }) + + it.each([ + "// spawnSync('node_modules/.bin/oxfmt', [])", + 'const example = "spawnSync(\'node_modules/.bin/oxfmt\', [])"', + "const bin = path.join(root, 'node_modules', '.bin', 'oxfmt'); existsSync(bin)", + "spawnSync(process.execPath, ['node_modules/.bin/oxfmt'])", + "spawnSync('node_modules/.binary/oxfmt', [])", + "spawnSync('other_node_modules/.bin/oxfmt', [])", + `const invocation = resolveOxcCliInvocation('oxfmt', 'oxfmt', root) + execFileSync(invocation.command, [...invocation.prefixArgs, '--write', file])`, + 'const a = b; const b = a; spawnSync(a, [])' + ])('does not flag non-program paths or safe invocations: %s', (contents) => { + expect(hasNodeModulesBinSpawn(contents)).toBe(false) + }) + + it('folds dot segments, so a .. detour into node_modules/.bin is still caught', () => { + expect( + hasNodeModulesBinSpawn( + "import { execFileSync } from 'node:child_process'\n" + + "import path from 'node:path'\n" + + "execFileSync(path.join(root, 'node_modules', 'tools', '..', '.bin', 'oxfmt'), [])\n" + ) + ).toBe(true) + }) + + it('folds Windows-separator dot segments too', () => { + expect( + hasNodeModulesBinSpawn( + "import { execFileSync } from 'node:child_process'\n" + + "execFileSync('root\\\\node_modules\\\\tools\\\\..\\\\.bin\\\\oxfmt', [])\n" + ) + ).toBe(true) + }) + + it('does not fold a .. back into .bin when it escapes the directory', () => { + expect( + hasNodeModulesBinSpawn( + "import { execFileSync } from 'node:child_process'\n" + + "import path from 'node:path'\n" + + "execFileSync(path.join(root, 'node_modules', '.bin', '..', 'oxfmt', 'cli.js'), [])\n" + ) + ).toBe(false) + }) +}) diff --git a/config/scripts/windows-cmd-shim-spawn-boundary.test.mjs b/config/scripts/windows-cmd-shim-spawn-boundary.test.mjs index 253605781cf..fd9e1790d56 100644 --- a/config/scripts/windows-cmd-shim-spawn-boundary.test.mjs +++ b/config/scripts/windows-cmd-shim-spawn-boundary.test.mjs @@ -1,35 +1,20 @@ import { readdirSync, readFileSync } from 'node:fs' import path from 'node:path' import { describe, expect, it } from 'vitest' +import { hasNodeModulesBinSpawn } from './windows-bin-spawn-predicate.mjs' /** - * Guard the one idiom that keeps re-killing Windows tooling. - * - * Node >= 20 refuses to spawn a Windows batch shim without `shell: true` (the - * CVE-2024-27980 mitigation), so `spawnSync('pnpm.cmd', …)` throws EINVAL - * before the command runs at all. On Windows that reads as a broken toolchain - * rather than a failing check, so the failure gets shrugged off — which is - * exactly how `check:code-quality:changed` ran dead for months. - * - * `src/` has its own chokepoint (runProcess) and its own ratchet. These trees - * are plain `.mjs` run by bare `node`, outside that module boundary, so they - * need this narrower one: a batch-shim command literal may not appear in a new - * script. The list only shrinks. Resolve the real executable instead — - * `oxlint-cli-invocation.mjs` and `windows-process-tree-gyp-rebuild.mjs` show - * the shape. - * - * Deliberately a text match on any `.cmd`/`.bat` literal, not on a list of - * runner names: these trees already spawn vitest, playwright, electron-builder - * and tsc, and the next offender is as likely to be one of those as it is to be - * pnpm. A literal is all a copy-paste carries. - * - * Two shapes this does not catch, both accepted. A shim assembled in a template - * literal, and a drive-lettered path — 'C:\tools\pnpm.cmd' — since a colon is - * not in the class. Real code builds those with path.join, whose 'pnpm.cmd' - * argument is caught. Also note codeText only drops lines that BEGIN with a - * comment marker, so a trailing `// 'pnpm.cmd'` false-positives; that fails - * closed. All of which is the ceiling of a text ratchet, and the reason `src/` - * gets a real chokepoint instead. + * Bare JS scripts cannot use the TS runProcess chokepoint; resolve package bins + * under process.execPath instead (see oxc-cli-invocation.mjs). The list only shrinks. + * The literal check catches quoted .cmd/.bat paths, excluding drive letters and + * templates; comment-only lines are dropped, but trailing comments fail closed. + * The AST check catches spawn/spawnSync/execFile/execFileSync first arguments + * containing node_modules/.bin: literals, templates, concatenation, join/resolve, + * local variable initializers and ternaries. It does not follow imports, function + * returns, assignments, destructuring, aliased spawn names or computed members. + * Names are matched without scope/import resolution; shadowed names and non-path + * join/resolve calls can fail closed. Paths are joined textually, without reducing + * dot segments. Neither check proves a Windows branch or an actual unsafe spawn. */ const WINDOWS_SHIM_LITERAL = /['"][\w./\\-]*\.(?:cmd|bat)['"]/i @@ -106,25 +91,26 @@ describe('windows batch shim spawn boundary', () => { const scripts = SCANNED_ROOTS.flatMap((root) => collectScripts(path.join(repoRoot, root), repoRoot) ) - const offenders = scripts.filter((relativePath) => - WINDOWS_SHIM_LITERAL.test(codeText(readFileSync(path.join(repoRoot, relativePath), 'utf8'))) - ) + const offenders = scripts.filter((relativePath) => { + const contents = readFileSync(path.join(repoRoot, relativePath), 'utf8') + return WINDOWS_SHIM_LITERAL.test(codeText(contents)) || hasNodeModulesBinSpawn(contents) + }) it('scans a plausible number of scripts', () => { // A broken root or extension filter would make the guard silently vacuous. expect(scripts.length).toBeGreaterThan(100) }) - it('has no unlisted script naming a Windows batch shim', () => { + it('has no unlisted script naming a Windows batch shim or spawning a package bin shim', () => { const unlisted = offenders.filter((name) => !WINDOWS_SHIM_SPAWN_ALLOWLIST.includes(name)) expect( unlisted, - 'Node cannot spawn a Windows batch shim without a shell. Resolve the real executable — see oxlint-cli-invocation.mjs.' + 'Node cannot spawn a Windows batch shim without a shell. Resolve the real executable — see oxc-cli-invocation.mjs.' ).toEqual([]) }) it('has no stale allowlist entry', () => { const stale = WINDOWS_SHIM_SPAWN_ALLOWLIST.filter((name) => !offenders.includes(name)) - expect(stale, 'Script no longer names a batch shim — delete the line.').toEqual([]) + expect(stale, 'Script no longer matches either shim predicate — delete the line.').toEqual([]) }) })