mirror of
https://github.com/morluto/rea.git
synced 2026-11-10 00:36:35 +00:00
fix(javascript): reject child-process option literals Node refuses at the call site (#9594)
* fix(javascript): reject a spawn stdio string Node refuses at the call site
`node:child_process` validates a literal `stdio` string before it creates anything, so a
bad value throws from the call itself and no child process exists. The analysis reported the
raw string as `stdioMode` and kept the spawn.
Measured on Node 24.22.1:
| `stdio` | outcome |
| --- | --- |
| `"pipe"`, `"overlapped"`, `"inherit"`, `"ignore"` | child starts |
| `"ipc"`, `"PIPE"`, `"pipe "`, `"pipeline"`, `"stream"`, `"stdio"` | THREW `ERR_INVALID_ARG_VALUE` |
The accepted set is exact and case-sensitive, so it is a closed list rather than a pattern.
`"ipc"` is the interesting one: it is valid as an array element but refused as the scalar
shorthand, which is the shape a caller reaches for when they mean the array form.
`fork` differs on one value because it appends `"ipc"` to a string and splits it, so `""`
becomes the lone element `"ipc"` and is refused:
| call | outcome |
| --- | --- |
| `spawn(..., {stdio: ""})` | child starts |
| `fork(..., {stdio: ""})` | THREW `ERR_INVALID_ARG_VALUE` |
`exec` is left alone, because it spreads its options and validates none of them. A number,
boolean or object `stdio` is also refused by Node, but `stdioMode` already reports those as
`"dynamic"`; deciding them here would claim more than the reported mode explains, so only
the string case is decided.
18 cases, 9 failing before this commit. `src/domain/javascript`: 5106 to 5124 passing.
`tsc` 0; `build` 0; `npm run format:check` 0; `npm run lint` 0 errors.
* fix(javascript): reject a non-string cwd or argv0 Node refuses at the call site
The same call-site validation the `stdio` string gets applies to `cwd` and `argv0`:
Node checks them before the child exists, so a non-string literal throws
`ERR_INVALID_ARG_TYPE` and the analysis still reported a spawn.
Measured on Node 24.22.1, identical on `spawn`, `fork`, `execFile` and `exec`:
| `cwd` / `argv0` | outcome |
| --- | --- |
| `"/tmp"` / `"x"` | child starts |
| `null` | child starts |
| `5`, `0`, `true`, `{}`, `[]` | THREW `ERR_INVALID_ARG_TYPE` |
`env` is deliberately not in the set: Node accepts any `env` value, including `5` and
`"x"`, so a non-object `env` is not a call-site failure.
Only an exactly readable literal is decided; a binding, a call or a member read stays
undecided, because its value is not settled by this expression.
The test file is renamed from `spawn_stdio_literal` to `spawn_option_literals` because it
now covers all three members. 16 cases added; reverting this check fails 9 of them.
`src/domain/javascript`: 5124 to 5140 passing. `tsc` 0; `build` 0;
`npm run format:check` 0; `npm run lint` 0 errors.
* fix(javascript): read the stdio and path options by value, not by AST kind
Seven review findings, each measured on Node 24.22.1. All seven were real, and two of them
were removing real children.
**`execFile` does not forward `stdio`.** Async `execFile` never passes `options.stdio` to its
underlying `spawn`, so `execFile(process.execPath, ["-e", ""], {stdio: "bad"})` returns a
real child — `stdioMode` already forces it to `"default"`. The new filter rejected it anyway
and dropped the spawn plus its linked interactions. `execFile` is now exempt, like `exec`.
**A no-substitution template literal is the same string to Node.** `readExactJavaScriptLiteral`
already resolves it, but the filter tested `t.isStringLiteral`, so ``{stdio: `ipc`}`` kept a
spawn Node refuses while ``{cwd: `/tmp`}`` — a valid path — was rejected because the same
branch called `readExactJavaScriptLiteral` and treated any hit as a non-string. Both sides
now read the value and test its type:
| call | Node 24.22.1 |
| --- | --- |
| ``spawn(..., {stdio: `ipc`})`` | THREW `ERR_INVALID_ARG_VALUE` |
| ``spawn(..., {stdio: `pipe`})`` | child starts |
| ``spawn(..., {cwd: `/tmp`})`` | child starts |
| ``spawn(..., {argv0: `x`})`` | child starts |
**A shape the literal owner does not price still has a settled type.** `{cwd: 1n}`,
`{cwd: /x/}`, `{cwd: () => {}}`, `{cwd: class {}}` and `{cwd: new String("x")}` all throw
`ERR_INVALID_ARG_TYPE`, and none matched the previous object/array/boolean arms.
**The `fork` explanation was wrong.** Node's `stdioStringToArray` rejects `stdio: ""` with an
explicit check before anything is appended; it does not split the string and it never builds
an IPC-only array. The rejection was right, the reason was not, so the comment now states the
real rule.
44 cases; reverting the `execFile` exemption fails 1, reverting the value-based read fails 1,
and reverting the extended non-string arms fails 6.
`src/domain/javascript`: 5158 to 5168 passing. `tsc` 0; `build` 0;
`npm run format:check` 0; `npm run lint` 0 errors.
* fix(javascript): keep a path-shaped cwd that Node accepts
`cwd` is a path, so Node takes a byte array or a file `URL` there, not only a string. The
blanket `NewExpression` arm rejected every constructor for both members, which dropped real
children.
Measured on Node 24.22.1, identical on `spawn`, `fork`, `execFile` and `exec`:
| `cwd` | outcome |
| --- | --- |
| `new Uint8Array([47, 116, 109, 112])` | child starts |
| `new Buffer("/tmp")` | child starts |
| `new URL("file:///C:/tmp/")` | child starts |
| `new String("/tmp")`, `new Number(1)`, `new Foo("/tmp")` | THREW `ERR_INVALID_ARG_TYPE` |
`argv0` is different: it is an argument string only, so `new Uint8Array([120])` and
`new URL(...)` both throw there. `isPathConstructor` names the constructors that can produce
a path, and only `cwd` consults it.
`URL` stays undecided rather than accepted, because whether a file URL resolves is a
platform property: `new URL("file:///tmp/")` throws `ERR_INVALID_FILE_URL_PATH` on Windows
while a drive-qualified path does not. Syntax alone cannot settle that, so the answer is
left open instead of guessed.
8 cases added; reverting the arm fails 3 of them.
`src/domain/javascript`: 5168 to 5176 passing. `tsc` 0; `build` 0;
`npm run format:check` 0; `npm run lint` 0 errors.
* test(javascript): distinguish package scope in route fixtures
* test(apple): separate identity and refusal fixture groups
* fix(javascript): keep unresolved cwd constructors unknown
* refactor(javascript): name process option admission by its contract
---------
Co-authored-by: morluto <76467478+morluto@users.noreply.github.com>
This commit is contained in:
1 parent
759817ccbe
commit
7a1d89a801
3 files changed
+272
No files matched your search
@@ -0,0 +1,115 @@
|
||||
import { expect, it } from "vitest";
|
||||
|
||||
import { analyzeJavaScriptSemantics } from "./javascriptSemanticAnalysis.js";
|
||||
|
||||
/**
|
||||
* `node:child_process` validates a literal `stdio` string before it creates anything, so a
|
||||
* rejected value throws from the call itself and no child exists.
|
||||
*
|
||||
* Measured on Node 24.22.1: `"pipe"`, `"overlapped"`, `"inherit"` and `"ignore"` start a
|
||||
* child, while `"ipc"`, `"PIPE"`, `"pipe "`, `"pipeline"`, `"stream"` and `"stdio"` throw
|
||||
* `ERR_INVALID_ARG_VALUE`. The set is exact and case-sensitive. `fork` appends `"ipc"` to a
|
||||
* string and splits it, so an empty string becomes the lone element `"ipc"`, which `fork`
|
||||
* refuses — `spawn(..., {stdio: ""})` still starts a child.
|
||||
*/
|
||||
const spawnsFor = (body: string) =>
|
||||
analyzeJavaScriptSemantics(
|
||||
`import cp from "node:child_process";\nfunction g() { ${body}; }`,
|
||||
).childProcessSpawns;
|
||||
|
||||
it.each([
|
||||
[`cp.spawn("x", [], { stdio: "bad" })`],
|
||||
[`cp.spawn("x", [], { stdio: "PIPE" })`],
|
||||
[`cp.spawn("x", [], { stdio: "pipe " })`],
|
||||
[`cp.spawn("x", [], { stdio: "ipc" })`],
|
||||
[`cp.spawn("x", [], { stdio: "pipeline" })`],
|
||||
[`cp.spawn("x", [], { stdio: "stdio" })`],
|
||||
[`cp.fork("x", [], { stdio: "bad" })`],
|
||||
[`cp.fork("x", [], { stdio: "" })`],
|
||||
['cp.spawn("x", [], { stdio: `ipc` })'],
|
||||
] as const)("emits no spawn for a rejected stdio string: %s", (call) => {
|
||||
expect(spawnsFor(call)).toEqual([]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
[`cp.spawn("x", [], { stdio: "pipe" })`],
|
||||
[`cp.spawn("x", [], { stdio: "overlapped" })`],
|
||||
[`cp.spawn("x", [], { stdio: "inherit" })`],
|
||||
[`cp.spawn("x", [], { stdio: "ignore" })`],
|
||||
[`cp.spawn("x", [], { stdio: "" })`],
|
||||
[`cp.fork("x", [], { stdio: "pipe" })`],
|
||||
['cp.spawn("x", [], { stdio: `pipe` })'],
|
||||
// `exec` spreads its options and validates none of them, and async `execFile` does not
|
||||
// forward `stdio` to its underlying spawn at all, so both return a real child.
|
||||
[`cp.exec("x", { stdio: "bad" })`],
|
||||
[`cp.execFile("x", [], { stdio: "bad" })`],
|
||||
] as const)("keeps the spawn for an accepted stdio string: %s", (call) => {
|
||||
expect(spawnsFor(call)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it.each([
|
||||
// A binding is not a literal, so this file leaves the answer to the runtime.
|
||||
[`const s = "bad"; cp.spawn("x", [], { stdio: s })`],
|
||||
[`cp.spawn("x", [], { stdio: ["pipe"] })`],
|
||||
] as const)("leaves an undecided stdio undecided: %s", (call) => {
|
||||
expect(spawnsFor(call)).toHaveLength(1);
|
||||
});
|
||||
|
||||
// Invalid literal paths and argv0 values fail before child creation. Unknown
|
||||
// constructor results are covered separately and must retain their candidates.
|
||||
it.each([
|
||||
[`cp.spawn("x", [], { cwd: 5 })`],
|
||||
[`cp.spawn("x", [], { cwd: true })`],
|
||||
[`cp.spawn("x", [], { cwd: {} })`],
|
||||
[`cp.spawn("x", [], { cwd: [] })`],
|
||||
[`cp.spawn("x", [], { cwd: 0 })`],
|
||||
[`cp.spawn("x", [], { argv0: 5 })`],
|
||||
[`cp.fork("x", [], { cwd: 5 })`],
|
||||
[`cp.exec("x", { cwd: 5 })`],
|
||||
[`cp.execFile("x", [], { cwd: 5 })`],
|
||||
[`cp.spawn("x", [], { cwd: 1n })`],
|
||||
[`cp.spawn("x", [], { cwd: /x/ })`],
|
||||
[`cp.spawn("x", [], { cwd: () => {} })`],
|
||||
[`cp.spawn("x", [], { cwd: class {} })`],
|
||||
[`cp.spawn("x", [], { argv0: 1n })`],
|
||||
] as const)("emits no spawn for a non-string cwd or argv0: %s", (call) => {
|
||||
expect(spawnsFor(call)).toEqual([]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
[`cp.spawn("x", [], { cwd: "/tmp" })`],
|
||||
[`cp.spawn("x", [], { cwd: null })`],
|
||||
[`cp.spawn("x", [], { cwd: "" })`],
|
||||
[`cp.spawn("x", [], { argv0: "x" })`],
|
||||
[`cp.spawn("x", [], { argv0: null })`],
|
||||
// Node accepts any `env` value, so a non-object one is not a call-site failure.
|
||||
[`cp.spawn("x", [], { env: 5 })`],
|
||||
// A binding is not a literal, so this file leaves the answer to the runtime.
|
||||
[`const c = "/tmp"; cp.spawn("x", [], { cwd: c })`],
|
||||
// A no-substitution template is the same string to Node.
|
||||
['cp.spawn("x", [], { cwd: `/tmp` })'],
|
||||
['cp.spawn("x", [], { argv0: `x` })'],
|
||||
] as const)("keeps the spawn for an accepted cwd or argv0: %s", (call) => {
|
||||
expect(spawnsFor(call)).toHaveLength(1);
|
||||
});
|
||||
|
||||
// Constructor names cannot establish the returned cwd value or its validity.
|
||||
it.each([
|
||||
[`cp.spawn("x", [], { cwd: new Uint8Array([47, 116, 109, 112]) })`],
|
||||
[`cp.spawn("x", [], { cwd: new Buffer("/tmp") })`],
|
||||
[`cp.spawn("x", [], { cwd: new URL("file:///C:/tmp/") })`],
|
||||
[`cp.spawn("x", [], { cwd: new globalThis.URL("file:///tmp/") })`],
|
||||
[
|
||||
`class DirectoryURL { constructor() { return new URL("file:///tmp/"); } }; cp.spawn("x", [], { cwd: new DirectoryURL() })`,
|
||||
],
|
||||
[`cp.spawn("x", [], { cwd: new Foo("/tmp") })`],
|
||||
] as const)("keeps unresolved cwd constructor candidates: %s", (call) => {
|
||||
expect(spawnsFor(call)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it.each([
|
||||
[`cp.spawn("x", [], { argv0: new Uint8Array([120]) })`],
|
||||
[`cp.spawn("x", [], { argv0: new URL("file:///C:/t/") })`],
|
||||
] as const)("emits no spawn for a constructor Node refuses: %s", (call) => {
|
||||
expect(spawnsFor(call)).toEqual([]);
|
||||
});
|
||||
@@ -76,6 +76,8 @@ export const collectJavaScriptSemanticChildProcesses = (
|
||||
const candidates = collectSpawns(program, state, callables).filter(
|
||||
(candidate) =>
|
||||
!forkRejectsStdioArray(candidate, context) &&
|
||||
!rejectsStdioLiteral(candidate, context) &&
|
||||
!rejectsProcessOptionLiteral(candidate, context) &&
|
||||
!rejectsOptionsShape(candidate, context, state),
|
||||
);
|
||||
const childProcessSpawns = candidates.map((candidate) =>
|
||||
@@ -449,6 +451,42 @@ const environmentSupplied = (
|
||||
return staticJavaScriptTruthiness(value) !== false;
|
||||
};
|
||||
|
||||
/**
|
||||
* The `stdio` string values Node accepts for `spawn`, `execFile` and `fork`.
|
||||
*
|
||||
* Measured on Node 24.22.1: `"pipe"`, `"overlapped"`, `"inherit"` and `"ignore"` start a
|
||||
* child; `"ipc"`, `"PIPE"`, `"pipe "`, `"pipeline"`, `"stream"` and `"stdio"` all throw
|
||||
* `ERR_INVALID_ARG_VALUE` before anything is created. The set is exact and case-sensitive.
|
||||
*/
|
||||
const STREAM_STDIO_MODES = new Set([
|
||||
"pipe",
|
||||
"overlapped",
|
||||
"inherit",
|
||||
"ignore",
|
||||
] as const);
|
||||
|
||||
/**
|
||||
* Whether an exact string `stdio` makes the call throw before a child exists.
|
||||
*
|
||||
* `spawn` passes the string to `getValidStdio`, so every value outside
|
||||
* `STREAM_STDIO_MODES` throws. `fork` sends the string through `stdioStringToArray`, which
|
||||
* refuses `""` on its own — it does not split the string and it never appends `"ipc"`, so
|
||||
* the rejection is that one explicit check rather than an IPC-count rule. Measured on Node
|
||||
* 24.22.1: `spawn(..., {stdio: ""})` starts a child while `fork(..., {stdio: ""})` throws
|
||||
* `ERR_INVALID_ARG_VALUE`.
|
||||
*
|
||||
* `exec` and `execFile` are excluded by the caller: async `execFile` does not forward
|
||||
* `stdio` to its underlying `spawn` at all, so `execFile(..., {stdio: "bad"})` returns a
|
||||
* real child, which `stdioMode` already records as `"default"`.
|
||||
*/
|
||||
const rejectsStdioString = (
|
||||
value: string,
|
||||
method: JavaScriptSemanticChildProcessSpawn["method"],
|
||||
): boolean =>
|
||||
(STREAM_STDIO_MODES as ReadonlySet<string>).has(value)
|
||||
? false
|
||||
: method === "fork" || value !== "";
|
||||
|
||||
const stdioMode = (
|
||||
options: t.Node | undefined,
|
||||
method: JavaScriptSemanticChildProcessSpawn["method"],
|
||||
@@ -474,6 +512,33 @@ const stdioMode = (
|
||||
return "dynamic";
|
||||
};
|
||||
|
||||
/**
|
||||
* Whether a `stdio` member is a string Node refuses at the call site.
|
||||
*
|
||||
* The value is read through the shared literal owner rather than the AST kind, because a
|
||||
* no-substitution template literal is the same string to Node: measured on Node 24.22.1,
|
||||
* `{stdio: \`ipc\`}` throws `ERR_INVALID_ARG_VALUE` while `{stdio: \`pipe\`}` starts a
|
||||
* child. A non-string value is left to `stdioMode`, which already reports it as
|
||||
* `"dynamic"`, so this filter only speaks about strings.
|
||||
*/
|
||||
const rejectsStdioLiteral = (
|
||||
candidate: SpawnCandidate,
|
||||
context: ChildSpawnContext,
|
||||
): boolean => {
|
||||
// Async exec/execFile do not forward options.stdio to their underlying spawn.
|
||||
if (candidate.method === "exec" || candidate.method === "execFile")
|
||||
return false;
|
||||
const { options } = spawnArguments(candidate, context);
|
||||
if (options === undefined) return false;
|
||||
const expression = unwrapJavaScriptExpression(options).node;
|
||||
if (context.isNullish(expression)) return false;
|
||||
const lookup = staticObjectProperty(expression, "stdio", context.isNullish);
|
||||
if (lookup.status !== "explicit") return false;
|
||||
const literal = readExactJavaScriptLiteral(lookup.value);
|
||||
if (!literal.found || typeof literal.value !== "string") return false;
|
||||
return rejectsStdioString(literal.value, candidate.method);
|
||||
};
|
||||
|
||||
/**
|
||||
* Node's `fork` requires an explicit `stdio` array to carry exactly one IPC
|
||||
* channel. An array that provably omits `"ipc"` throws
|
||||
@@ -510,6 +575,48 @@ const forkRejectsStdioArray = (
|
||||
return ipcCount !== 1;
|
||||
};
|
||||
|
||||
/** Only proven invalid cwd paths or argv0 strings establish call-site rejection. */
|
||||
const rejectsProcessOptionLiteral = (
|
||||
candidate: SpawnCandidate,
|
||||
context: ChildSpawnContext,
|
||||
): boolean => {
|
||||
const { options } = spawnArguments(candidate, context);
|
||||
if (options === undefined) return false;
|
||||
const expression = unwrapJavaScriptExpression(options).node;
|
||||
if (context.isNullish(expression)) return false;
|
||||
for (const member of ["cwd", "argv0"] as const) {
|
||||
const lookup = staticObjectProperty(expression, member, context.isNullish);
|
||||
if (lookup.status !== "explicit") continue;
|
||||
const value = unwrapJavaScriptExpression(lookup.value).node;
|
||||
if (context.isNullish(value)) continue;
|
||||
// The value is read through the shared literal owner, so a no-substitution template
|
||||
// literal counts as the string Node receives: measured on Node 24.22.1,
|
||||
// `{cwd: \`/tmp\`}` and `{argv0: \`x\`}` both start a child.
|
||||
const literal = readExactJavaScriptLiteral(value);
|
||||
if (literal.found) {
|
||||
if (typeof literal.value === "string") continue;
|
||||
return true;
|
||||
}
|
||||
if (t.isStringLiteral(value)) continue;
|
||||
// A constructor can return a URL or another accepted path value. Its name
|
||||
// cannot prove that cwd is invalid, including qualified and custom constructors.
|
||||
if (member === "cwd" && t.isNewExpression(value)) continue;
|
||||
if (
|
||||
t.isObjectExpression(value) ||
|
||||
t.isArrayExpression(value) ||
|
||||
t.isBooleanLiteral(value) ||
|
||||
t.isBigIntLiteral(value) ||
|
||||
t.isRegExpLiteral(value) ||
|
||||
t.isArrowFunctionExpression(value) ||
|
||||
t.isFunctionExpression(value) ||
|
||||
t.isClassExpression(value) ||
|
||||
t.isNewExpression(value)
|
||||
)
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
};
|
||||
|
||||
/**
|
||||
* Node validates the options object before creating anything, so a literal
|
||||
* primitive or literal array in that slot throws ERR_INVALID_ARG_TYPE
|
||||
|
||||
@@ -144,3 +144,53 @@ cliTest.for(["CLI", "stdio MCP"] as const)(
|
||||
}
|
||||
},
|
||||
);
|
||||
|
||||
cliTest.for(["CLI", "stdio MCP"] as const)(
|
||||
"preserves definite rejection and unknown builtin admission through %s",
|
||||
async (surface, { cli }) => {
|
||||
const root = await createTestTempDirectory("rea-builtin-admission-");
|
||||
const cases = [
|
||||
["rejected", 'const child=cp.spawn("tool",[],{cwd:5});', false],
|
||||
[
|
||||
"qualified",
|
||||
'const child=cp.spawn("tool",[],{cwd:new globalThis.URL("file:///tmp/")});',
|
||||
true,
|
||||
],
|
||||
[
|
||||
"unknown",
|
||||
'const child=cp.spawn("tool",[],{cwd:new DirectoryURL()});',
|
||||
true,
|
||||
],
|
||||
] as const;
|
||||
for (const [name, body] of cases) {
|
||||
await writeFile(
|
||||
join(root, name + ".mjs"),
|
||||
'import cp from "node:child_process";' +
|
||||
"function entry(){" +
|
||||
body +
|
||||
"}",
|
||||
);
|
||||
}
|
||||
const { semantic_graph: graph } = await analyzeJavaScriptDirectory(
|
||||
surface,
|
||||
cli,
|
||||
root,
|
||||
);
|
||||
for (const [name, , retained] of cases) {
|
||||
const nodes = graph.nodes.filter(
|
||||
(node) =>
|
||||
node.identity.module_path === name + ".mjs" &&
|
||||
node.kind === "child-process",
|
||||
);
|
||||
expect(nodes, name).toHaveLength(retained ? 1 : 0);
|
||||
const ids = new Set(nodes.map((node) => node.node_id));
|
||||
const links = graph.relations.filter(
|
||||
(relation) =>
|
||||
relation.relation === "spawns" &&
|
||||
(ids.has(relation.source_node_id) ||
|
||||
ids.has(relation.target_node_id)),
|
||||
);
|
||||
expect(links, name).toHaveLength(retained ? 1 : 0);
|
||||
}
|
||||
},
|
||||
);
|
||||
Reference in new issue
Block a user