From 02a33a8f586ded0203d1791d655bb1ca5377ecb3 Mon Sep 17 00:00:00 2001 From: Kaoru0822 Date: Fri, 13 Nov 2026 06:10:25 -0800 Subject: [PATCH] fix(javascript): reject invalid fs open argument roles (#10346) --- ...nticAnalysis.fs_function_arguments.test.ts | 94 ++++++++++++ .../javascript/javascriptSemanticResources.ts | 49 +++++- .../javascriptFsFunctionArgumentsCli.test.ts | 143 ++++++++++++++++++ 3 files changed, 283 insertions(+), 3 deletions(-) create mode 100644 src/domain/javascript/javascriptSemanticAnalysis.fs_function_arguments.test.ts create mode 100644 tests/acceptance/applications/javascriptFsFunctionArgumentsCli.test.ts diff --git a/src/domain/javascript/javascriptSemanticAnalysis.fs_function_arguments.test.ts b/src/domain/javascript/javascriptSemanticAnalysis.fs_function_arguments.test.ts new file mode 100644 index 0000000000..53a3602f3c --- /dev/null +++ b/src/domain/javascript/javascriptSemanticAnalysis.fs_function_arguments.test.ts @@ -0,0 +1,94 @@ +import { expect, it } from "vitest"; +import { analyzeJavaScriptSemantics } from "./javascriptSemanticAnalysis.js"; +import { analyzeCommonJsSemanticFixture } from "../../../tests/fixtures/javascriptCommonJsAnalysis.js"; + +const functionValues = [ + ["arrow", "() => {}"], + ["function", "function () {}"], + ["async function", "async function () {}"], + ["generator", "function* () {}"], + ["class", "class {}"], +] as const; + +const resourcesFor = (call: string) => + analyzeJavaScriptSemantics(` + import {open, openSync} from "node:fs"; + import {open as openPromise} from "node:fs/promises"; + function f(p, cb, ...args) { ${call}; } + `).resourceOperations; + +it.each([ + "open(p)", + 'open(p, "r")', + 'open(p, "r", 0o644)', + 'open(p, "r", 0o644, 17)', + 'open(p, "r", 0o644, null)', + 'open(p, "r", 0o644, {})', + 'open(p, "r", 0o644, void sideEffect())', +])( + "omits an open whose normalized callback is missing or nonfunction: %s", + (call) => { + expect(resourcesFor(call)).toEqual([]); + }, +); + +it.each([ + 'open(p, "r", cb, 17)', + 'open(p, "r", 0o644, cb)', + 'open(p, "r", 0o644, ...args)', +])("preserves an unknown callback or overload: %s", (call) => { + expect(resourcesFor(call)).toHaveLength(1); +}); + +it.each(functionValues)( + "rejects a %s in fs flag/mode value slots", + (_kind, value) => { + for (const call of [ + `openSync(p, ${value})`, + `openSync(p, "r", ${value})`, + `openPromise(p, ${value})`, + `openPromise(p, "r", ${value})`, + `open(p, ${value}, cb)`, + `open(p, ${value}, ...args, cb)`, + ]) + expect(resourcesFor(call), call).toEqual([]); + }, +); + +it.each(functionValues)( + "preserves %s callback overloads and an empty trailing spread", + (_kind, value) => { + for (const call of [ + `open(p, ${value})`, + `open(p, "r", ${value})`, + `open(p, "r", ${value}, 17)`, + `open(p, ${value}, ...args)`, + ]) + expect(resourcesFor(call), call).toMatchObject([ + { kind: "acquire", method: "open" }, + ]); + }, +); + +it.each([ + "openSync(...args, () => {})", + "openPromise(p, ...args, () => {})", + "open(...args, () => {}, cb)", + "openSync(p, flags)", + 'openPromise(p, "r", mode)', +])("leaves unresolved argument positions and values undecided: %s", (call) => { + expect(resourcesFor(call)).toHaveLength(1); +}); + +it.each(["openSync(p, () => {})", 'openSync(p, "r", () => {})'])( + "applies Node's rejection only to an established builtin: %s", + (call) => { + const source = `const {openSync}=require("node:fs"); function f(p) { ${call}; }`; + expect(analyzeJavaScriptSemantics(source).resourceOperations).toMatchObject( + [{ kind: "acquire", method: "openSync", builtinAuthority: "candidate" }], + ); + expect(analyzeCommonJsSemanticFixture(source).resourceOperations).toEqual( + [], + ); + }, +); diff --git a/src/domain/javascript/javascriptSemanticResources.ts b/src/domain/javascript/javascriptSemanticResources.ts index 0bff28ed2b..358d54c601 100644 --- a/src/domain/javascript/javascriptSemanticResources.ts +++ b/src/domain/javascript/javascriptSemanticResources.ts @@ -1,4 +1,5 @@ import * as t from "@babel/types"; +import { rejectsSemanticEventListener } from "./javascriptSemanticAsyncEffects.js"; import { BUILTIN_RESOURCE_SPECIFIERS, @@ -95,9 +96,16 @@ export const collectJavaScriptSemanticResources = ( ]; }; +/** These expressions have typeof "function", including a class constructor. */ +const isFunctionValuedSyntax = (value: t.Node): boolean => + t.isFunctionExpression(value) || + t.isArrowFunctionExpression(value) || + t.isClassExpression(value); + /** Literal kinds Node rejects for both open flags and modes. */ const rejectsFlagOrModeLiteral = (value: t.Node): boolean => { return ( + isFunctionValuedSyntax(value) || t.isBigIntLiteral(value) || t.isBooleanLiteral(value) || t.isRegExpLiteral(value) || @@ -118,6 +126,7 @@ const rejectsFlagOrModeLiteral = (value: t.Node): boolean => { const rejectsOpenFlagLiteral = ( node: t.CallExpression | t.OptionalCallExpression, context: DataEffectTraversalContext, + callbackForm: boolean, ): boolean => { // A spread before the flag slot shifts the runtime position, so argument 1 is // not provably the flag. `openSync(...args, "0600")` with @@ -126,6 +135,16 @@ const rejectsOpenFlagLiteral = ( const flag = node.arguments[1]; if (flag === undefined || t.isSpreadElement(flag)) return false; const value = unwrapJavaScriptExpression(flag).node; + // fs.open(path, callback) defaults the flags. Later spreads may all be + // empty; a fixed third argument is needed to exclude that overload. + if ( + callbackForm && + isFunctionValuedSyntax(value) && + node.arguments.every( + (argument, index) => index < 2 || t.isSpreadElement(argument), + ) + ) + return false; // A no-expression template literal is exactly readable, so it is the same flag // string Node receives. const flagText = t.isStringLiteral(value) @@ -289,6 +308,7 @@ const isNonFiniteNumber = ( const rejectsOpenModeLiteral = ( node: t.CallExpression | t.OptionalCallExpression, context: DataEffectTraversalContext, + callbackForm: boolean, ): boolean => { // A spread before the mode slot shifts the runtime position. if ( @@ -299,6 +319,8 @@ const rejectsOpenModeLiteral = ( const mode = node.arguments[2]; if (mode === undefined || t.isSpreadElement(mode)) return false; const value = unwrapJavaScriptExpression(mode).node; + // Only callback-form fs.open treats a function in this slot as its callback. + if (callbackForm && isFunctionValuedSyntax(value)) return false; // `null`, `undefined` and `void ` all default the mode. if (t.isNullLiteral(value)) return false; // A no-expression template literal is exactly readable, so it is the same @@ -339,6 +361,21 @@ const openFlagInFirstArgument = (reference: AcquisitionReference): boolean => ? reference.specifier === "fs" || reference.specifier === "fs/promises" : false; +/** Node normalizes fs.open's callback before validating its function type. */ +const rejectsOpenCallbackLiteral = ( + node: t.CallExpression | t.OptionalCallExpression, +): boolean => { + // A spread can change both arity and which operand is the callback. + if (node.arguments.some((argument) => t.isSpreadElement(argument))) + return false; + if (node.arguments.length < 3) + return rejectsSemanticEventListener(node.arguments[1]); + // A function-valued mode is the callback; an unknown mode may be one too. + // Both can supersede a missing or invalid fourth operand. + if (!rejectsSemanticEventListener(node.arguments[2])) return false; + return rejectsSemanticEventListener(node.arguments[3]); +}; + const collectAcquisitions = ( program: t.Program, context: DataEffectTraversalContext, @@ -348,9 +385,15 @@ const collectAcquisitions = ( if (!t.isCallExpression(node) && !t.isOptionalCallExpression(node)) return; const reference = acquisitionMethod(node.callee, context.state); if (reference === null) return; - if (openFlagInFirstArgument(reference)) { - if (rejectsOpenFlagLiteral(node, context)) return; - if (rejectsOpenModeLiteral(node, context)) return; + if ( + openFlagInFirstArgument(reference) && + reference.builtinAuthority !== "candidate" + ) { + const callbackForm = + reference.specifier === "fs" && reference.method === "open"; + if (callbackForm && rejectsOpenCallbackLiteral(node)) return; + if (rejectsOpenFlagLiteral(node, context, callbackForm)) return; + if (rejectsOpenModeLiteral(node, context, callbackForm)) return; } output.push({ node, diff --git a/tests/acceptance/applications/javascriptFsFunctionArgumentsCli.test.ts b/tests/acceptance/applications/javascriptFsFunctionArgumentsCli.test.ts new file mode 100644 index 0000000000..8a733ad398 --- /dev/null +++ b/tests/acceptance/applications/javascriptFsFunctionArgumentsCli.test.ts @@ -0,0 +1,143 @@ +import { expect } from "vitest"; +import { createHash } from "node:crypto"; +import { readFile } from "node:fs/promises"; +import { join } from "node:path"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { cliTest } from "../../support/cli/cliFixture.js"; +import { + analyzeJavaScriptDirectory, + runNodeJsonControl, +} from "../../support/cli/javascriptApplication.js"; +import { writeFixtureFiles } from "../../support/javascriptApplicationFixture.js"; +import { resolveJavaScriptSemanticEvidence } from "../../support/javascriptSemanticGraphFixture.js"; + +cliTest.for(["CLI", "stdio MCP"] as const)( + "matches Node's fs function-valued argument rejection through %s", + async (surface, { cli }) => { + const root = await createTestTempDirectory("rea-fs-function-arguments-"); + await writeFixtureFiles(root, { + input: "fixture", + "entry.mjs": ` + import {open, openSync, closeSync} from "node:fs"; + import {open as openPromise} from "node:fs/promises"; + export const badSyncFlag = p => openSync(p, () => {}); + export const badSyncMode = p => openSync(p, "r", () => {}); + export const badSyncClassFlag = p => openSync(p, class {}); + export const badSyncClassMode = p => openSync(p, "r", class {}); + export const badPromiseFlag = p => openPromise(p, () => {}); + export const badPromiseClassFlag = p => openPromise(p, class {}); + export const badCallbackFlag = p => open(p, () => {}, () => {}); + export const missingCallback = p => open(p); + export const missingShortCallback = p => open(p, "r"); + export const nonfunctionCallback = p => open(p, "r", 0o644, 17); + export async function badPromiseMode(p) { + const h = await openPromise(p, "r", () => {}); h.close(); + } + export async function badPromiseClassMode(p) { + const h = await openPromise(p, "r", class {}); h.close(); + } + export const callbackOnly = p => new Promise((resolve, reject) => + open(p, (error, fd) => { + if (error) reject(error); + else { closeSync(fd); resolve("opened"); } + })); + export const callbackMode = p => new Promise((resolve, reject) => + open(p, "r", (error, fd) => { + if (error) reject(error); + else { closeSync(fd); resolve("opened"); } + })); + export const emptySpread = (p, ...args) => new Promise((resolve, reject) => + open(p, (error, fd) => { + if (error) reject(error); + else { closeSync(fd); resolve("opened"); } + }, ...args)); + `, + "control.mjs": ` + import * as fixture from "./entry.mjs"; + import {closeSync} from "node:fs"; + const observed = {}; + for (const [name, fn] of Object.entries(fixture)) { + try { + const value = await fn("input"); + if (typeof value === "number") closeSync(value); + else if (typeof value?.close === "function") await value.close(); + observed[name] = value === "opened" ? value : "unexpected success"; + } catch (error) { observed[name] = error.code; } + } + console.log(JSON.stringify(observed)); + `, + }); + expect(await runNodeJsonControl(root, "control.mjs")).toEqual({ + badSyncFlag: "ERR_INVALID_ARG_VALUE", + badSyncMode: "ERR_INVALID_ARG_TYPE", + badSyncClassFlag: "ERR_INVALID_ARG_VALUE", + badSyncClassMode: "ERR_INVALID_ARG_TYPE", + badPromiseFlag: "ERR_INVALID_ARG_VALUE", + badPromiseClassFlag: "ERR_INVALID_ARG_VALUE", + badCallbackFlag: "ERR_INVALID_ARG_VALUE", + missingCallback: "ERR_INVALID_ARG_TYPE", + missingShortCallback: "ERR_INVALID_ARG_TYPE", + nonfunctionCallback: "ERR_INVALID_ARG_TYPE", + badPromiseMode: "ERR_INVALID_ARG_TYPE", + badPromiseClassMode: "ERR_INVALID_ARG_TYPE", + callbackOnly: "opened", + callbackMode: "opened", + emptySpread: "opened", + }); + + const result = await analyzeJavaScriptDirectory(surface, cli, root, { + sourcePaths: ["entry.mjs"], + }); + const resources = result.semantic_graph.nodes.filter( + ({ kind }) => kind === "resource", + ); + expect(resources).toHaveLength(3); + expect(resources.map(({ properties }) => properties.method)).toEqual([ + "open", + "open", + "open", + ]); + const source = await readFile(join(root, "entry.mjs"), "utf8"); + const expectedLines = source + .split("\n") + .flatMap((line, index) => + line.includes("open(p, (error, fd)") || + line.includes('open(p, "r", (error, fd)') + ? [index + 1] + : [], + ); + const lines = resources.map(({ evidence: reference }) => { + const evidence = resolveJavaScriptSemanticEvidence( + result.semantic_graph, + reference, + ); + expect(evidence).toMatchObject({ + state: "observed", + authority: "ast-static-analysis", + artifact: { + available: true, + sha256: createHash("sha256").update(source).digest("hex"), + }, + location: { + available: true, + value: { kind: "source-range", source: "entry.mjs" }, + }, + }); + const location = evidence.location; + if (!location.available || location.value.kind !== "source-range") + throw new Error("Expected a source range for the accepted open"); + return location.value.start.line; + }); + expect(lines.sort((a, b) => a - b)).toEqual(expectedLines); + expect( + result.semantic_graph.relations.filter( + ({ relation }) => relation === "acquires", + ), + ).toHaveLength(3); + expect( + result.semantic_graph.relations.filter( + ({ relation }) => relation === "releases", + ), + ).toEqual([]); + }, +);