fix(javascript): reject invalid fs open argument roles (#10346)

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(
+      [],
+    );
+  },
+);

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 <expr>` 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,

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([]);
+  },
+);