diff --git a/.changeset/fix-security-server-rule-false-positives.md b/.changeset/fix-security-server-rule-false-positives.md new file mode 100644 index 000000000..99a0ea1e2 --- /dev/null +++ b/.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. diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/agent-tool-capability-risk.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/agent-tool-capability-risk.regressions.test.ts new file mode 100644 index 000000000..5f1e75629 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/agent-tool-capability-risk.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/agent-tool-capability-risk.ts index 0fbb18d09..c11c6abcc 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/agent-tool-capability-risk.ts +++ b/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.", }), diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/mcp-tool-capability-risk.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/mcp-tool-capability-risk.ts index 675ce8d3b..ca92ce835 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/mcp-tool-capability-risk.ts +++ b/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.", }), diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/scan-by-pattern.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/scan-by-pattern.ts index ef524c34e..4815acbf3 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/scan-by-pattern.ts +++ b/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,6 +12,11 @@ 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; + // 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; // 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; @@ -18,6 +24,7 @@ export interface ScanByPatternInput { } const strippedContentCache = new WeakMap(); +const codeOnlyContentCache = new WeakMap(); // 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 []; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/strip-string-literals-keeping-module-specifiers.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/strip-string-literals-keeping-module-specifiers.test.ts new file mode 100644 index 000000000..451f85e5a --- /dev/null +++ b/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"); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/strip-string-literals-keeping-module-specifiers.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/strip-string-literals-keeping-module-specifiers.ts new file mode 100644 index 000000000..d39071ec2 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security-scan/utils/strip-string-literals-keeping-module-specifiers.ts @@ -0,0 +1,135 @@ +// 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. Three things stay, because they are +// code rather than prose: module-specifier strings (`from "node:child_process"`, +// `require("node:fs")`), template-literal `${…}` interpolations (e.g. a real +// `${exec(cmd)}` call), and every delimiter/newline/offset (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"]); + +interface StringFrame { + readonly kind: "string"; + readonly delimiter: string; + readonly shouldBlank: boolean; +} + +interface TemplateFrame { + readonly kind: "template"; +} + +interface InterpolationFrame { + readonly kind: "interpolation"; + braceDepth: number; +} + +type ScanFrame = StringFrame | TemplateFrame | InterpolationFrame; + +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(""); + const frames: ScanFrame[] = []; + let index = 0; + + const blankCharacter = (characterIndex: number): void => { + if (content[characterIndex] !== "\n") characters[characterIndex] = " "; + }; + + while (index < content.length) { + const character = content[index]; + const currentFrame = frames.at(-1); + + if (currentFrame?.kind === "string") { + if (character === "\\") { + if (currentFrame.shouldBlank) { + blankCharacter(index); + if (content[index + 1] !== undefined) blankCharacter(index + 1); + } + index += 2; + continue; + } + if (character === currentFrame.delimiter) { + frames.pop(); + index += 1; + continue; + } + if (currentFrame.shouldBlank) blankCharacter(index); + index += 1; + continue; + } + + if (currentFrame?.kind === "template") { + if (character === "\\") { + blankCharacter(index); + if (content[index + 1] !== undefined) blankCharacter(index + 1); + index += 2; + continue; + } + if (character === "`") { + frames.pop(); + index += 1; + continue; + } + // `${…}` is executable code, not prose, so descend into it instead of + // blanking — a capability call written only there must still count. + if (character === "$" && content[index + 1] === "{") { + frames.push({ kind: "interpolation", braceDepth: 1 }); + index += 2; + continue; + } + blankCharacter(index); + index += 1; + continue; + } + + // Top-level code or an interpolation expression: keep everything, but + // balance interpolation braces so the matching `}` returns to the template. + if (currentFrame?.kind === "interpolation") { + if (character === "{") { + currentFrame.braceDepth += 1; + index += 1; + continue; + } + if (character === "}") { + currentFrame.braceDepth -= 1; + if (currentFrame.braceDepth === 0) frames.pop(); + index += 1; + continue; + } + } + + if (character === '"' || character === "'") { + frames.push({ + kind: "string", + delimiter: character, + shouldBlank: !isModuleSpecifierQuote(content, index), + }); + index += 1; + continue; + } + if (character === "`") { + frames.push({ kind: "template" }); + index += 1; + continue; + } + + index += 1; + } + + return characters.join(""); +}; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.test.ts new file mode 100644 index 000000000..e8b8e321d --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { serverSequentialIndependentAwait } from "./server-sequential-independent-await.js"; + +describe("server-sequential-independent-await", () => { + it("flags two genuinely independent sequential awaits", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export const load = async () => { + const user = await fetchUser(); + const posts = await fetchPosts(); + return { user, posts }; +};`, + { filename: "/repo/app/page.tsx" }, + ); + + expect(result.diagnostics).toHaveLength(1); + }); + + it("stays silent when the second await reads names destructured from the first (array of object patterns)", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export default async function Page({ params }: PageProps<"/preview/blog/[slug]">) { + const [{ slug }, { isEnabled }] = await Promise.all([params, draftMode()]); + const data = await client.fetch( + BlogPostQuery, + { slug }, + isEnabled ? { perspective: "drafts", stega: true } : { next: { revalidate: 3600 } }, + ); + if (!data) notFound(); + return ; +}`, + { filename: "/repo/app/(site)/preview/blog/[slug]/page.tsx" }, + ); + + expect(result.diagnostics).toHaveLength(0); + }); + + it("stays silent when the second await reads a deeply nested destructured binding", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export const load = async () => { + const { data: { token } } = await getSession(); + const profile = await fetchProfile(token); + return profile; +};`, + { filename: "/repo/app/page.tsx" }, + ); + + expect(result.diagnostics).toHaveLength(0); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.ts index 14ef8f3f8..8733eaaf8 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.ts @@ -1,3 +1,4 @@ +import { collectPatternNames } from "../../utils/collect-pattern-names.js"; import { defineRule } from "../../utils/define-rule.js"; import { walkAst } from "../../utils/walk-ast.js"; import type { EsTreeNode } from "../../utils/es-tree-node.js"; @@ -19,24 +20,7 @@ const collectDeclaredNames = (declaration: EsTreeNode): Set => { const names = new Set(); if (!isNodeOfType(declaration, "VariableDeclaration")) return names; for (const declarator of declaration.declarations ?? []) { - if (isNodeOfType(declarator.id, "Identifier")) { - names.add(declarator.id.name); - } else if (isNodeOfType(declarator.id, "ObjectPattern")) { - for (const property of declarator.id.properties ?? []) { - if (isNodeOfType(property, "Property") && isNodeOfType(property.value, "Identifier")) { - names.add(property.value.name); - } else if ( - isNodeOfType(property, "RestElement") && - isNodeOfType(property.argument, "Identifier") - ) { - names.add(property.argument.name); - } - } - } else if (isNodeOfType(declarator.id, "ArrayPattern")) { - for (const element of declarator.id.elements ?? []) { - if (isNodeOfType(element, "Identifier")) names.add(element.name); - } - } + collectPatternNames(declarator.id, names); } return names; };