fix(rules): silence false positives in agent/mcp-tool-capability-risk and server-sequential-independent-await (#838, #839)

.changeset/fix-security-server-rule-false-positives.md

@@ -0,0 +1,17 @@
+---
+"oxlint-plugin-react-doctor": patch
+---
+
+Fix false positives in two rules (#838, #839).
+
+- `agent-tool-capability-risk` / `mcp-tool-capability-risk` (#838): capability
+  keywords are now matched in code only. Words inside a tool's `description`
+  string (e.g. "ALWAYS fetch the underlying numbers first") and the AI-SDK
+  `execute:` handler key no longer satisfy the dangerous-capability gate. A
+  handler that actually calls `exec`/`fetch`/`readFile`/etc. still fires.
+
+- `server-sequential-independent-await` (#839): declared-name collection now
+  recurses into nested destructuring patterns, so
+  `const [{ slug }, { isEnabled }] = await Promise.all(...)` followed by an
+  `await` that reads `slug`/`isEnabled` is correctly treated as dependent and
+  not flagged as a waterfall.

packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/agent-tool-capability-risk.regressions.test.ts

@@ -0,0 +1,91 @@
+import { describe, expect, it } from "vite-plus/test";
+import { runScanRule } from "../../../test-utils/run-scan-rule.js";
+import { agentToolCapabilityRisk } from "./agent-tool-capability-risk.js";
+
+describe("security-scan/agent-tool-capability-risk — regressions", () => {
+  it("stays silent when capability words appear only in the description and execute handler key", () => {
+    const content = [
+      'import { tool } from "ai";',
+      "export const listItems = tool({",
+      '  description: "List items. ALWAYS fetch the underlying numbers first.",',
+      "  inputSchema: z.object({ organizationId: z.string() }),",
+      "  execute: async ({ organizationId }) => {",
+      '    if (organizationId !== allowedOrgId) return { error: "Access denied" };',
+      "    return prisma.item.findMany({ where: { organizationId } });",
+      "  },",
+      "});",
+    ].join("\n");
+    const findings = runScanRule(agentToolCapabilityRisk, {
+      relativePath: "src/lib/tools/campaign-stats.ts",
+      content,
+    });
+    expect(findings).toHaveLength(0);
+  });
+
+  it("flags a tool handler that actually shells out", () => {
+    const content = [
+      'import { tool } from "ai";',
+      "export const runShell = tool({",
+      '  description: "Run a command",',
+      "  inputSchema: z.object({ cmd: z.string() }),",
+      "  execute: async ({ cmd }) => {",
+      "    return exec(cmd);",
+      "  },",
+      "});",
+    ].join("\n");
+    const findings = runScanRule(agentToolCapabilityRisk, {
+      relativePath: "src/lib/tools/run-shell.ts",
+      content,
+    });
+    expect(findings).toHaveLength(1);
+  });
+
+  it("flags a tool handler that actually calls fetch in code", () => {
+    const content = [
+      'import { tool } from "ai";',
+      "export const getData = tool({",
+      '  description: "Get data",',
+      "  execute: async ({ url }) => fetch(url),",
+      "});",
+    ].join("\n");
+    const findings = runScanRule(agentToolCapabilityRisk, {
+      relativePath: "src/lib/tools/get-data.ts",
+      content,
+    });
+    expect(findings).toHaveLength(1);
+  });
+
+  it("flags a tool whose handler calls a capability inside a template interpolation", () => {
+    const content = [
+      'import { tool } from "ai";',
+      "export const proxy = tool({",
+      '  description: "Proxy a request",',
+      "  execute: async ({ url }) => `result: ${await fetch(url)}`,",
+      "});",
+    ].join("\n");
+    const findings = runScanRule(agentToolCapabilityRisk, {
+      relativePath: "src/lib/tools/proxy.ts",
+      content,
+    });
+    expect(findings).toHaveLength(1);
+  });
+
+  it("flags a tool that names a dangerous module in its import specifier", () => {
+    const content = [
+      'import { tool } from "ai";',
+      'import { execFile } from "node:child_process";',
+      "export const runCommandTool = tool({",
+      '  description: "Run a repository maintenance command",',
+      "  execute: async ({ command }) => {",
+      "    execFile(command, []);",
+      "    return { ok: true };",
+      "  },",
+      "});",
+    ].join("\n");
+    const findings = runScanRule(agentToolCapabilityRisk, {
+      relativePath: "src/agents/tools/run-command-tool.ts",
+      content,
+    });
+    expect(findings).toHaveLength(1);
+  });
+});

packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/agent-tool-capability-risk.ts

@@ -20,7 +20,9 @@ export const agentToolCapabilityRisk = defineRule({
       isProductionSourcePath(file.relativePath) &&
       AGENT_TOOL_CONTEXT_PATH_PATTERN.test(file.relativePath),
     pattern: AGENT_TOOL_DEFINITION_PATTERN,
-    requireAll: [AGENT_TOOL_DANGEROUS_CAPABILITY_PATTERN],
+    // In code only: the AI-SDK `execute:` handler key and capability words in
+    // `description` prose ("...fetch the numbers...") must not satisfy the gate.
+    requireAllInCode: [AGENT_TOOL_DANGEROUS_CAPABILITY_PATTERN],
     message:
       "An agent-callable tool appears to expose network, filesystem, shell, or code-execution capability.",
   }),

packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/mcp-tool-capability-risk.ts

@@ -24,7 +24,10 @@ export const mcpToolCapabilityRisk = defineRule({
   scan: scanByPattern({
     shouldScan: (file) => isProductionSourcePath(file.relativePath),
     pattern: MCP_TOOL_SURFACE_PATTERN,
-    requireAll: [MCP_IMPORT_PATTERN, AGENT_TOOL_DANGEROUS_CAPABILITY_PATTERN],
+    requireAll: [MCP_IMPORT_PATTERN],
+    // In code only: a capability word inside a tool `description` string is
+    // documentation, not an exposed primitive, so it must not satisfy the gate.
+    requireAllInCode: [AGENT_TOOL_DANGEROUS_CAPABILITY_PATTERN],
     message:
       "An MCP tool/resource/prompt handler appears to expose file, shell, network, or code-execution capability.",
   }),

packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/scan-by-pattern.ts

@@ -2,6 +2,7 @@ import { SOURCE_FILE_PATTERN } from "../../../constants/security-scan.js";
 import type { FileScan, ScannedFile } from "../../../utils/file-scan.js";
 import { getMatchLocation } from "./get-match-location.js";
 import { stripCommentsPreservingPositions } from "./strip-comments-preserving-positions.js";
+import { stripStringLiteralsKeepingModuleSpecifiers } from "./strip-string-literals-keeping-module-specifiers.js";
 
 export interface ScanByPatternInput {
   readonly shouldScan: (file: ScannedFile) => boolean;
@@ -11,13 +12,19 @@ export interface ScanByPatternInput {
   // Conjunction gates: every pattern must also match somewhere in the file
   // (e.g. an MCP import that proves the matched tool surface is MCP).
   readonly requireAll?: ReadonlyArray<RegExp>;
+  // Conjunction gates that must match in CODE only — comments and prose
+  // string-literal contents are blanked before testing (module-specifier
+  // strings are kept, since an import path is code), so a capability keyword
+  // that appears solely inside a `description` string doesn't count.
+  readonly requireAllInCode?: ReadonlyArray<RegExp>;
   // Veto: a match anywhere in the file suppresses the finding (e.g. a
   // signature-verification call that answers the rule's concern).
   readonly suppressWhen?: RegExp;
   readonly message: string;
 }
 
 const strippedContentCache = new WeakMap<ScannedFile, string>();
+const codeOnlyContentCache = new WeakMap<ScannedFile, string>();
 
 // Comments are a recurring false-positive source ("Ajv compiles schemas via
 // `new Function(...)`"); blank them for JS/TS files before pattern matching.
@@ -31,14 +38,39 @@ export const getScannableContent = (file: ScannedFile): string => {
   return strippedContent;
 };
 
+// Comments AND prose string-literal contents blanked (import paths kept) — the
+// "is this keyword in real code?" view used by `requireAllInCode` gates so prose
+// inside a `description` string can't satisfy a capability gate, while a
+// dangerous import like `from "node:child_process"` still counts.
+const getCodeOnlyContent = (file: ScannedFile): string => {
+  const cachedContent = codeOnlyContentCache.get(file);
+  if (cachedContent !== undefined) return cachedContent;
+  const codeOnlyContent = stripStringLiteralsKeepingModuleSpecifiers(getScannableContent(file));
+  codeOnlyContentCache.set(file, codeOnlyContent);
+  return codeOnlyContent;
+};
+
 export const scanByPattern =
-  ({ shouldScan, pattern, requireAll, suppressWhen, message }: ScanByPatternInput): FileScan =>
+  ({
+    shouldScan,
+    pattern,
+    requireAll,
+    requireAllInCode,
+    suppressWhen,
+    message,
+  }: ScanByPatternInput): FileScan =>
   (file) => {
     if (!shouldScan(file)) return [];
     const content = getScannableContent(file);
     if (requireAll !== undefined && !requireAll.every((gate) => gate.test(content))) {
       return [];
     }
+    if (
+      requireAllInCode !== undefined &&
+      !requireAllInCode.every((gate) => gate.test(getCodeOnlyContent(file)))
+    ) {
+      return [];
+    }
     const patterns = pattern instanceof RegExp ? [pattern] : pattern;
     const matchedPattern = patterns.find((candidate) => candidate.test(content));
     if (matchedPattern === undefined) return [];

packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/strip-string-literals-keeping-module-specifiers.test.ts

@@ -0,0 +1,64 @@
+import { describe, expect, it } from "vite-plus/test";
+import { stripStringLiteralsKeepingModuleSpecifiers } from "./strip-string-literals-keeping-module-specifiers.js";
+
+describe("security-scan/utils/strip-string-literals-keeping-module-specifiers", () => {
+  it("blanks string contents while preserving offsets, delimiters, and newlines", () => {
+    const source = `const description = "ALWAYS fetch the numbers first";\nconst run = exec(cmd);`;
+    const stripped = stripStringLiteralsKeepingModuleSpecifiers(source);
+    expect(stripped).toHaveLength(source.length);
+    expect(stripped).not.toContain("fetch");
+    expect(stripped.split("\n")[0]).toMatch(/^const description = " +";$/);
+    expect(stripped.split("\n")[1]).toBe("const run = exec(cmd);");
+  });
+
+  it("blanks template-literal text but keeps newlines for multi-line strings", () => {
+    const source = "const help = `line one\nfetch line two`;\nconst keep = true;";
+    const stripped = stripStringLiteralsKeepingModuleSpecifiers(source);
+    expect(stripped).not.toContain("fetch");
+    expect(stripped.split("\n")).toHaveLength(3);
+    expect(stripped.split("\n")[2]).toBe("const keep = true;");
+  });
+
+  it("does not let an escaped quote close the string early", () => {
+    const source = `const note = "say \\"exec\\" out loud"; const real = spawn(cmd);`;
+    const stripped = stripStringLiteralsKeepingModuleSpecifiers(source);
+    expect(stripped).not.toContain("exec");
+    expect(stripped).toContain("spawn(cmd)");
+  });
+
+  it("keeps module-specifier strings: import, export-from, and require paths", () => {
+    const source = [
+      `import { execFile } from "node:child_process";`,
+      `export { readFile } from "node:fs/promises";`,
+      `const vm = require("node:vm");`,
+      `const dynamic = import("node:child_process");`,
+    ].join("\n");
+    const stripped = stripStringLiteralsKeepingModuleSpecifiers(source);
+    expect(stripped).toContain("node:child_process");
+    expect(stripped).toContain("node:fs/promises");
+    expect(stripped).toContain("node:vm");
+    expect(stripped).toBe(source);
+  });
+
+  it("blanks member-access strings that merely look like an import", () => {
+    const source = `const copy = Buffer.from("fetch the bytes");`;
+    const stripped = stripStringLiteralsKeepingModuleSpecifiers(source);
+    expect(stripped).not.toContain("fetch");
+    expect(stripped).toMatch(/^const copy = Buffer\.from\(" +"\);$/);
+  });
+
+  it("keeps template `${…}` interpolation code while blanking the surrounding text", () => {
+    const source = "const out = `please fetch ${exec(cmd)} right now`;";
+    const stripped = stripStringLiteralsKeepingModuleSpecifiers(source);
+    expect(stripped).toContain("exec(cmd)");
+    expect(stripped).not.toContain("please fetch");
+    expect(stripped).not.toContain("right now");
+  });
+
+  it("blanks prose strings nested inside an interpolation but keeps the call", () => {
+    const source = 'const out = `${runCommand({ shell: "fetch the data" })}`;';
+    const stripped = stripStringLiteralsKeepingModuleSpecifiers(source);
+    expect(stripped).toContain("runCommand({ shell:");
+    expect(stripped).not.toContain("fetch the data");
+  });
+});