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,23 @@
+---
+"oxlint-plugin-react-doctor": patch
+---
+
+Fix false positives in three rules (#838, #837, #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.
+
+- `url-prefilled-privileged-action` (#837): the validating-helper lookbehind now
+  allows a member-access object between the helper call and the read, so
+  `sanitizeAuthCallbackURL(url.searchParams.get("callbackURL"))` and
+  `resolveSafeAuthCallbackURL(url.searchParams.get(...))` are recognized as
+  validated and suppressed.
+
+- `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,76 @@
+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 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/url-prefilled-privileged-action.regressions.test.ts

@@ -34,4 +34,28 @@ describe("security-scan/url-prefilled-privileged-action — regressions", () =>
     });
     expect(findings).toHaveLength(0);
   });
+
+  it("stays silent when a member-access read is wrapped in an infix-named validator (resolveSafe…)", () => {
+    const findings = runScanRule(urlPrefilledPrivilegedAction, {
+      relativePath: "src/app/login/page.tsx",
+      content: `url.searchParams.set("callbackURL",\n  resolveSafeAuthCallbackURL(url.searchParams.get("callbackURL")));\n`,
+    });
+    expect(findings).toHaveLength(0);
+  });
+
+  it("stays silent when an aliased sanitiz*-named validator wraps a member-access read", () => {
+    const findings = runScanRule(urlPrefilledPrivilegedAction, {
+      relativePath: "src/app/login/page.tsx",
+      content: `import { resolveSafeAuthCallbackURL as sanitizeAuthCallbackURL } from "~/lib/auth-callback";\nconst safe = sanitizeAuthCallbackURL(url.searchParams.get("callbackURL"));\n`,
+    });
+    expect(findings).toHaveLength(0);
+  });
+
+  it("still flags an unvalidated callbackUrl read passed through a non-validating call", () => {
+    const findings = runScanRule(urlPrefilledPrivilegedAction, {
+      relativePath: "src/app/login/page.tsx",
+      content: `const callback = decodeURIComponent(url.searchParams.get("callbackUrl"));\n`,
+    });
+    expect(findings).toHaveLength(1);
+  });
 });

packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/url-prefilled-privileged-action.ts

@@ -7,8 +7,12 @@ import { scanByPattern } from "./utils/scan-by-pattern.js";
 // No `email`/`user`: prefilled emails and username route params are benign
 // booking/contact UX, not privileged actions. The lookbehind skips reads
 // already wrapped in a validating helper (`getRelativeNextPath(...get("next"))`).
+// The validator name is matched as an infix (lookbehind scans every suffix, so
+// `resolveSafeAuthCallbackURL(...)` is recognized via its `Safe` segment), and
+// a member-access object between the helper and the read is allowed so
+// `sanitizeAuthCallbackURL(url.searchParams.get("callbackURL"))` is suppressed.
 const PRIVILEGED_QUERY_PARAM_PATTERN =
-  /(?<!(?:safe|valid|sanitiz|relativ|allowlist|whitelist)[\w$]*\(\s*(?:new\s+)?)\b(?:searchParams|useSearchParams\s*\(\s*\)|URLSearchParams\s*\([^)]{0,120}\))(?:[?!])?\.get(?:All)?\s*\(\s*["'](?:userstoinvite|role|permission|sharingaction|invite|admin|next|continue|returnTo|redirect_uri|callbackUrl)["']|\bsearchParams\.(?:userstoinvite|role|permission|sharingaction|invite|admin|returnTo|redirect_uri|callbackUrl)\b/i;
+  /(?<!(?:safe|valid|sanitiz|relativ|allowlist|whitelist)[\w$]*\(\s*(?:new\s+)?(?:[\w$]+\s*\.\s*)*)\b(?:searchParams|useSearchParams\s*\(\s*\)|URLSearchParams\s*\([^)]{0,120}\))(?:[?!])?\.get(?:All)?\s*\(\s*["'](?:userstoinvite|role|permission|sharingaction|invite|admin|next|continue|returnTo|redirect_uri|callbackUrl)["']|\bsearchParams\.(?:userstoinvite|role|permission|sharingaction|invite|admin|returnTo|redirect_uri|callbackUrl)\b/i;
 
 export const urlPrefilledPrivilegedAction = defineRule({
   id: "url-prefilled-privileged-action",

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,49 @@
+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\(" +"\);$/);
+  });
+});

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

@@ -0,0 +1,68 @@
+// Builds the "is this keyword in real code?" view used by capability/keyword
+// gates: string-literal *contents* are blanked with spaces so a keyword that
+// appears only in prose — a tool's human-readable `description`, "ALWAYS fetch
+// the numbers first" — can't satisfy a gate. Module-specifier strings
+// (`from "node:child_process"`, `require("node:fs")`) are kept, because an
+// import path is code, not prose, and naming a dangerous module is a real
+// capability signal. Delimiters, newlines, and every offset are preserved so a
+// blanked region still maps 1:1 onto the original file. Expects comment-stripped
+// input so a quote inside a comment is never treated as a string delimiter.
+const MODULE_SPECIFIER_KEYWORDS = new Set(["from", "import", "require"]);
+
+const isModuleSpecifierQuote = (content: string, quoteIndex: number): boolean => {
+  let cursor = quoteIndex - 1;
+  while (cursor >= 0 && /\s/.test(content[cursor])) cursor -= 1;
+  if (content[cursor] === "(") {
+    cursor -= 1;
+    while (cursor >= 0 && /\s/.test(content[cursor])) cursor -= 1;
+  }
+  const wordEnd = cursor;
+  while (cursor >= 0 && /[\w$]/.test(content[cursor])) cursor -= 1;
+  const precedingWord = content.slice(cursor + 1, wordEnd + 1);
+  if (!MODULE_SPECIFIER_KEYWORDS.has(precedingWord)) return false;
+  // A member access (`Buffer.from("…")`, `db.import("…")`) is not an import.
+  return content[cursor] !== ".";
+};
+
+export const stripStringLiteralsKeepingModuleSpecifiers = (content: string): string => {
+  const characters = content.split("");
+  let stringDelimiter: string | null = null;
+  let isModuleSpecifier = false;
+  let index = 0;
+
+  while (index < content.length) {
+    const character = content[index];
+
+    if (stringDelimiter !== null) {
+      if (character === "\\") {
+        if (!isModuleSpecifier) {
+          characters[index] = " ";
+          if (content[index + 1] !== undefined && content[index + 1] !== "\n") {
+            characters[index + 1] = " ";
+          }
+        }
+        index += 2;
+        continue;
+      }
+      if (character === stringDelimiter) {
+        stringDelimiter = null;
+        index += 1;
+        continue;
+      }
+      if (!isModuleSpecifier && character !== "\n") characters[index] = " ";
+      index += 1;
+      continue;
+    }
+
+    if (character === '"' || character === "'" || character === "`") {
+      stringDelimiter = character;
+      isModuleSpecifier = isModuleSpecifierQuote(content, index);
+      index += 1;
+      continue;
+    }
+
+    index += 1;
+  }
+
+  return characters.join("");
+};