mirror of
https://github.com/stablyai/orca.git
synced 2026-09-21 16:02:20 +00:00
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
This commit is contained in:
@@ -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)))
|
||||
)
|
||||
}
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
@@ -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([])
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user