From 5639b1e40e66650cb7042206b19807b2f785d8ff Mon Sep 17 00:00:00 2001 From: Aiden Bai Date: Mon, 29 Jun 2026 20:19:11 -0700 Subject: [PATCH 01/11] fix(server-auth-actions): don't flag non-privileged (cache/navigation) server actions (#983) --- ...-server-auth-actions-cache-revalidation.md | 24 ++ packages/language-server/vite.config.ts | 6 + .../src/plugin/constants/nextjs.ts | 13 + .../rules/server/server-auth-actions.ts | 6 + .../utils/is-non-privileged-server-action.ts | 168 +++++++++ .../regressions/server-auth-actions.test.ts | 333 ++++++++++++++++++ 6 files changed, 550 insertions(+) create mode 100644 .changeset/fix-server-auth-actions-cache-revalidation.md create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/utils/is-non-privileged-server-action.ts diff --git a/.changeset/fix-server-auth-actions-cache-revalidation.md b/.changeset/fix-server-auth-actions-cache-revalidation.md new file mode 100644 index 0000000000..d4c858b7a0 --- /dev/null +++ b/.changeset/fix-server-auth-actions-cache-revalidation.md @@ -0,0 +1,24 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +fix: stop flagging non-privileged server actions in server-auth-actions + +`server-auth-actions` flagged any exported server action without an auth check, +including actions that touch no protected data. It now exempts an action whose +body only: + +- busts the Next.js cache — `revalidateTag`, `revalidatePath`, `expireTag`, + `expirePath`, and the `unstable_` variants, and/or +- navigates — `redirect`, `permanentRedirect`, `notFound`, `forbidden`, + `unauthorized`. + +An unauthenticated caller gains nothing by invoking such actions, so requiring +an auth guard was a false positive. + +The exemption is deliberately conservative — the body must contain at least one +cache- or navigation call (matched only as a bare imported identifier, never a +same-named method like `obj.redirect()`) and **no** other effect. Any DB query, +`fetch`, imported helper, raw-SQL tagged template (`sql\`DELETE …\``), +constructor, or assignment keeps the action flagged, so a genuinely sensitive +action is never silently allowed through. diff --git a/packages/language-server/vite.config.ts b/packages/language-server/vite.config.ts index 1e587c4b4e..831a183e9d 100644 --- a/packages/language-server/vite.config.ts +++ b/packages/language-server/vite.config.ts @@ -44,5 +44,11 @@ export default defineConfig({ ], test: { testTimeout: 30_000, + // The integration suite boots a real LSP server subprocess and waits up + // to 20s for it to publish diagnostics inside `beforeAll`. The default + // 10s hook timeout is shorter than that wait, so a slow cold start on + // macOS / Windows CI runners trips the hook before the server is ready. + // Match it to `testTimeout` so the hook gets the same budget as the tests. + hookTimeout: 30_000, }, }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/constants/nextjs.ts b/packages/oxlint-plugin-react-doctor/src/plugin/constants/nextjs.ts index 6cc13204bd..0a6a590f0c 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/constants/nextjs.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/constants/nextjs.ts @@ -33,6 +33,19 @@ export const NEXTJS_NAVIGATION_FUNCTIONS = new Set([ "unauthorized", ]); +// Next.js cache-invalidation helpers from `next/cache`. Calling one only +// busts the data cache — it reads no data and mutates no records — so a +// server action whose only work is revalidation is not a privileged +// operation and needs no auth guard. +export const CACHE_REVALIDATION_FUNCTION_NAMES = new Set([ + "revalidateTag", + "revalidatePath", + "expireTag", + "expirePath", + "unstable_expireTag", + "unstable_expirePath", +]); + export const GOOGLE_FONTS_PATTERN = /fonts\.googleapis\.com/; export const POLYFILL_SCRIPT_PATTERN = /polyfill\.io|polyfill\.min\.js|cdn\.polyfill/; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-auth-actions.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-auth-actions.ts index 22fc49d7d6..f05ba5b6a0 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-auth-actions.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-auth-actions.ts @@ -10,6 +10,7 @@ import { hasDirective } from "../../utils/has-directive.js"; import { hasUseServerDirective } from "../../utils/has-use-server-directive.js"; import { isAuthGuardName } from "../../utils/is-auth-guard-name.js"; import { isFunctionLike } from "../../utils/is-function-like.js"; +import { isNonPrivilegedServerAction } from "../../utils/is-non-privileged-server-action.js"; import { walkAst } from "../../utils/walk-ast.js"; import type { EsTreeNode } from "../../utils/es-tree-node.js"; import type { RuleContext } from "../../utils/rule-context.js"; @@ -166,6 +167,11 @@ const inspectServerAction = ( const rootNodes = getAuthScanRoots(candidate.functionNode); if (containsAuthCheck(rootNodes, allowedFunctionNames, GENERIC_AUTH_METHOD_NAMES)) return; + // A cache-busting / navigation-only action touches no protected data, so it + // is safe to call unauthenticated. Checked after the bounded auth scan so + // the full-body walk is skipped for the common authenticated case. + if (isNonPrivilegedServerAction(candidate.functionNode)) return; + context.report({ node: candidate.reportNode, message: `Anyone can call server action "${candidate.displayName}" without logging in, since it has no auth check.`, diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-non-privileged-server-action.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-non-privileged-server-action.ts new file mode 100644 index 0000000000..814566d4d2 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/is-non-privileged-server-action.ts @@ -0,0 +1,168 @@ +import { + CACHE_REVALIDATION_FUNCTION_NAMES, + NEXTJS_NAVIGATION_FUNCTIONS, +} from "../constants/nextjs.js"; +import type { EsTreeNode } from "./es-tree-node.js"; +import type { EsTreeNodeOfType } from "./es-tree-node-of-type.js"; +import { isFunctionLike } from "./is-function-like.js"; +import { isNodeOfType } from "./is-node-of-type.js"; +import { walkAst } from "./walk-ast.js"; + +type FunctionLikeNode = + | EsTreeNodeOfType<"FunctionDeclaration"> + | EsTreeNodeOfType<"FunctionExpression"> + | EsTreeNodeOfType<"ArrowFunctionExpression">; + +// Calls that change neither protected data nor server state: Next.js cache +// invalidation (`revalidateTag`/`revalidatePath`/…) only busts the data +// cache, and navigation (`redirect`/`notFound`/…) only steers the response. +// An unauthenticated caller gains nothing by triggering either. +const NON_DATA_EFFECT_FUNCTION_NAMES: ReadonlySet = new Set([ + ...CACHE_REVALIDATION_FUNCTION_NAMES, + ...NEXTJS_NAVIGATION_FUNCTIONS, +]); + +// Matched only as a BARE identifier callee. A member call (`obj.redirect()`, +// `db.revalidateTag()`) shares the name but not the import, and could touch +// data on an arbitrary receiver, so it must not satisfy the exemption. +const isCacheOrNavigationCall = (node: EsTreeNode): boolean => + isNodeOfType(node, "CallExpression") && + isNodeOfType(node.callee, "Identifier") && + NON_DATA_EFFECT_FUNCTION_NAMES.has(node.callee.name); + +// Reduce an expression to the value it actually yields: strip TS / optional- +// chain wrappers, and collapse a comma sequence to its last operand (the value +// a `(revalidateTag(x), secret)` body returns). +const unwrapExpression = (node: EsTreeNode | null | undefined): EsTreeNode | null => { + let current: EsTreeNode | null | undefined = node; + while (current) { + if ( + isNodeOfType(current, "TSAsExpression") || + isNodeOfType(current, "TSNonNullExpression") || + isNodeOfType(current, "TSSatisfiesExpression") || + isNodeOfType(current, "ChainExpression") + ) { + current = current.expression; + continue; + } + if (isNodeOfType(current, "SequenceExpression")) { + current = current.expressions?.[current.expressions.length - 1]; + continue; + } + return current; + } + return null; +}; + +// A value-yielding expression hands data back to the (possibly unauthenticated) +// caller. Only a purely literal value (or a cache/navigation call, whose result +// is void) is safe; anything referencing a binding could carry protected data. +const isDataExposingValue = (node: EsTreeNode | null | undefined): boolean => { + const value = unwrapExpression(node); + if (!value) return false; + if (isCacheOrNavigationCall(value)) return false; + return !isLiteralOnlyExpression(value); +}; + +// An expression built purely from literals — `true`, `"ok"`, `{ revalidated: +// true }`, `[1, 2]`, a template with only literal interpolations. It carries +// no reference to a binding, so returning it leaks nothing. +const isLiteralOnlyExpression = (node: EsTreeNode | null | undefined): boolean => { + if (!node) return false; + if (isNodeOfType(node, "Literal")) return true; + if (isNodeOfType(node, "TemplateLiteral")) { + return (node.expressions ?? []).every(isLiteralOnlyExpression); + } + if (isNodeOfType(node, "UnaryExpression")) return isLiteralOnlyExpression(node.argument); + if (isNodeOfType(node, "ArrayExpression")) { + return (node.elements ?? []).every( + (element) => + element === null || + (!isNodeOfType(element, "SpreadElement") && isLiteralOnlyExpression(element)), + ); + } + if (isNodeOfType(node, "ObjectExpression")) { + return (node.properties ?? []).every( + (property) => + isNodeOfType(property, "Property") && + (!property.computed || isLiteralOnlyExpression(property.key)) && + isLiteralOnlyExpression(property.value), + ); + } + return false; +}; + +const getReturnedOrThrownArgument = (node: EsTreeNode): EsTreeNode | null => { + if (isNodeOfType(node, "ReturnStatement")) return node.argument ?? null; + if (isNodeOfType(node, "ThrowStatement")) return node.argument ?? null; + return null; +}; + +// `return ` / `throw ` hands a value back to the (possibly +// unauthenticated) caller — i.e. potential data exposure, the read half of the +// threat (a thrown binding reaches the client via the error path). A returned +// identifier, member access, await, call, conditional, or a non-literal nested +// inside an object/array could carry protected data, so it disqualifies the +// exemption. +const isDataExposingReturnOrThrow = (node: EsTreeNode): boolean => + isDataExposingValue(getReturnedOrThrownArgument(node)); + +// Any node that can reach state beyond the action's own locals: a non-cache/ +// non-navigation call (DB query, `fetch`, cookie mutation, an imported +// helper), a tagged template (raw-SQL clients like `sql\`DELETE …\``), a +// constructor, an assignment, a `delete`, or a `return`/`throw` that exposes +// data. +const isPrivilegedEffect = (node: EsTreeNode): boolean => + isNodeOfType(node, "CallExpression") || + isNodeOfType(node, "TaggedTemplateExpression") || + isNodeOfType(node, "NewExpression") || + isNodeOfType(node, "AssignmentExpression") || + isNodeOfType(node, "UpdateExpression") || + (isNodeOfType(node, "UnaryExpression") && node.operator === "delete") || + isDataExposingReturnOrThrow(node); + +// A server action is "non-privileged" when nothing it does can read or mutate +// protected data: its body busts the cache and/or navigates, and contains no +// other effect. Such an action is safe to call unauthenticated, so the +// missing-auth-check rule must not flag it. +// +// The check is conservative: the body must contain at least one cache- or +// navigation call AND no other privileged effect. Anything else — a DB write, +// a `fetch`, an imported helper, a raw-SQL tagged template, a constructor, or +// returning a value to the caller — disqualifies the exemption, so a genuinely +// sensitive action is never silently allowed through. +export const isNonPrivilegedServerAction = (functionNode: FunctionLikeNode): boolean => { + const functionBody = functionNode.body; + if (!functionBody) return false; + + // A concise-body arrow (`async () => expr`) implicitly returns its body, with + // no `ReturnStatement` for the walk to catch. Treat that implicit return as a + // data exposure check; the walk below still flags any privileged effect in + // the expression itself (e.g. an earlier operand of a comma sequence). + if (!isNodeOfType(functionBody, "BlockStatement") && isDataExposingValue(functionBody)) { + return false; + } + + let hasNonDataEffectCall = false; + let hasPrivilegedEffect = false; + + walkAst(functionBody, (child: EsTreeNode) => { + if (hasPrivilegedEffect) return false; + // Prune nested function bodies: a call inside a closure the action + // never invokes shouldn't count for or against the exemption. + if (child !== functionBody && isFunctionLike(child)) return false; + + // Keep descending after a cache/navigation call so a privileged effect + // hidden in its arguments (`revalidateTag(db.get())`) is still caught. + if (isCacheOrNavigationCall(child)) { + hasNonDataEffectCall = true; + return; + } + if (isPrivilegedEffect(child)) { + hasPrivilegedEffect = true; + return false; + } + }); + + return hasNonDataEffectCall && !hasPrivilegedEffect; +}; diff --git a/packages/react-doctor/tests/regressions/server-auth-actions.test.ts b/packages/react-doctor/tests/regressions/server-auth-actions.test.ts index e9170f17cd..bcdce5fbd8 100644 --- a/packages/react-doctor/tests/regressions/server-auth-actions.test.ts +++ b/packages/react-doctor/tests/regressions/server-auth-actions.test.ts @@ -489,6 +489,339 @@ export async function updateBio(bio: string) { await expect(collectAuthActionIssues(projectDirectory)).resolves.toEqual([]); }); + it("accepts an action that only revalidates a cache tag (caching needs no auth)", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-tag-only", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; + +export async function refreshPosts() { + revalidateTag("posts"); +}`), + }, + }); + + await expect(collectAuthActionIssues(projectDirectory)).resolves.toEqual([]); + }); + + it("accepts an action that only revalidates a path", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-path-only", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidatePath } from "next/cache"; + +export const refreshDashboard = async () => { + revalidatePath("/dashboard"); +};`), + }, + }); + + await expect(collectAuthActionIssues(projectDirectory)).resolves.toEqual([]); + }); + + it("accepts an action that revalidates then redirects", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-then-redirect", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidatePath } from "next/cache"; +import { redirect } from "next/navigation"; + +export async function refresh() { + revalidatePath("/"); + redirect("/"); +}`), + }, + }); + + await expect(collectAuthActionIssues(projectDirectory)).resolves.toEqual([]); + }); + + it("accepts an action whose only effect is a navigation", async () => { + const projectDirectory = setupReactProject(tempRoot, "redirect-only", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { redirect } from "next/navigation"; + +export async function go() { + redirect("/dashboard"); +}`), + }, + }); + + await expect(collectAuthActionIssues(projectDirectory)).resolves.toEqual([]); + }); + + it("still flags a revalidation action that also reads data from a non-parameter source", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-db-read", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { db } from "@/lib/db"; + +export async function refresh(userId: string) { + const user = await db.user.findUnique({ where: { id: userId } }); + revalidateTag(\`user-\${user.id}\`); +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("refresh"); + }); + + it("still flags an action that mutates data alongside a cache revalidation", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-mutation", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { db } from "@/lib/db"; + +export async function deletePost(postId: string) { + await db.post.delete({ where: { id: postId } }); + revalidateTag("posts"); +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("deletePost"); + }); + + it("still flags a raw-SQL tagged-template write next to a revalidation", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-sql-template", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { sql } from "@vercel/postgres"; + +export async function deleteUser(formData: FormData) { + await sql\`DELETE FROM users WHERE id = \${formData.get("id")}\`; + revalidateTag("users"); +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("deleteUser"); + }); + + it("still flags a constructor side effect next to a revalidation", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-new", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { AuditLogger } from "@/lib/audit"; + +export async function refresh() { + new AuditLogger("secret"); + revalidateTag("posts"); +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("refresh"); + }); + + it("still flags a module-state assignment next to a revalidation", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-assignment", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { sessionStore } from "@/lib/store"; + +export async function grantAdmin(formData: FormData) { + sessionStore[formData.get("user") as string] = "admin"; + revalidateTag("users"); +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("grantAdmin"); + }); + + it("still flags an action that revalidates then returns a referenced value", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-data-return", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { serverConfig } from "@/lib/config"; + +export async function refresh() { + revalidateTag("posts"); + return serverConfig.apiSecret; +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("refresh"); + }); + + it("accepts an action that revalidates then returns a plain status literal", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-literal-return", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; + +export async function refresh() { + revalidateTag("posts"); + return { revalidated: true }; +}`), + }, + }); + + await expect(collectAuthActionIssues(projectDirectory)).resolves.toEqual([]); + }); + + it("still flags an action that revalidates then returns an awaited value", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-awaited-return", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { sessionPromise } from "@/lib/session"; + +export async function refresh() { + revalidateTag("posts"); + return await sessionPromise; +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("refresh"); + }); + + it("still flags an action that revalidates then returns data nested in an object", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-nested-return", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { serverConfig } from "@/lib/config"; + +export async function refresh() { + revalidateTag("posts"); + return { token: serverConfig.apiSecret }; +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("refresh"); + }); + + it("still flags an action that revalidates then returns a conditional referencing data", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-conditional-return", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { currentUser } from "@/lib/user"; + +export async function refresh(flag: boolean) { + revalidateTag("posts"); + return flag ? currentUser.email : null; +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("refresh"); + }); + + it("still flags a `delete` mutation next to a revalidation", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-delete", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { sessionStore } from "@/lib/store"; + +export async function evict(key: string) { + delete sessionStore[key]; + revalidateTag("sessions"); +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("evict"); + }); + + it("still flags an action that revalidates then throws a referenced value", async () => { + const projectDirectory = setupReactProject(tempRoot, "revalidate-plus-data-throw", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { serverConfig } from "@/lib/config"; + +export async function refresh() { + revalidateTag("posts"); + throw serverConfig.apiSecret; +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("refresh"); + }); + + it("accepts a concise-arrow action whose body is a single revalidation", async () => { + const projectDirectory = setupReactProject(tempRoot, "concise-arrow-revalidate", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; + +export const refresh = async () => revalidateTag("posts");`), + }, + }); + + await expect(collectAuthActionIssues(projectDirectory)).resolves.toEqual([]); + }); + + it("still flags a concise-arrow action that revalidates then yields data via a sequence", async () => { + const projectDirectory = setupReactProject(tempRoot, "concise-arrow-sequence-leak", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { revalidateTag } from "next/cache"; +import { serverConfig } from "@/lib/config"; + +export const refresh = async () => (revalidateTag("posts"), serverConfig.apiSecret);`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("refresh"); + }); + + it("does not treat a same-named method call (obj.redirect()) as a safe navigation", async () => { + const projectDirectory = setupReactProject(tempRoot, "member-named-redirect", { + packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, + files: { + "src/app/actions.ts": buildServerActionFile(`import { emitter } from "@/lib/emitter"; + +export async function go(payload: string) { + emitter.redirect(payload); +}`), + }, + }); + + const issues = await collectAuthActionIssues(projectDirectory); + expect(issues).toHaveLength(1); + expect(issues[0].message).toContain("go"); + }); + it("still flags actions whose only top-level call is a non-auth helper", async () => { const projectDirectory = setupReactProject(tempRoot, "issue-829-non-auth-helper", { packageJsonExtras: { dependencies: NEXTJS_PACKAGE_DEPENDENCIES }, From 0b64af58b16329c5cae7a210463d2842e34b150d Mon Sep 17 00:00:00 2001 From: Aiden Bai Date: Mon, 29 Jun 2026 21:57:24 -0700 Subject: [PATCH 02/11] fix(security): skip test-like files in no-eval and auth-token-in-web-storage (#984) --- .../fix-security-rules-skip-test-files.md | 13 +++++++ .../auth-token-in-web-storage.test.ts | 14 +++++++ .../security/auth-token-in-web-storage.ts | 5 ++- .../src/plugin/rules/security/no-eval.test.ts | 38 +++++++++++++++++++ .../src/plugin/rules/security/no-eval.ts | 5 ++- .../src/plugin/utils/define-rule.ts | 29 ++++---------- .../plugin/utils/skip-non-production-files.ts | 14 +++++++ 7 files changed, 93 insertions(+), 25 deletions(-) create mode 100644 .changeset/fix-security-rules-skip-test-files.md create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/rules/security/no-eval.test.ts create mode 100644 packages/oxlint-plugin-react-doctor/src/plugin/utils/skip-non-production-files.ts diff --git a/.changeset/fix-security-rules-skip-test-files.md b/.changeset/fix-security-rules-skip-test-files.md new file mode 100644 index 0000000000..8aea52d58a --- /dev/null +++ b/.changeset/fix-security-rules-skip-test-files.md @@ -0,0 +1,13 @@ +--- +"react-doctor": patch +"oxlint-plugin-react-doctor": patch +"eslint-plugin-react-doctor": patch +--- + +Stop `no-eval` and `auth-token-in-web-storage` from firing in non-production files + +`eval` / `new Function` / a stringy `setTimeout`, and a token written to web +storage, are only vulnerabilities in code that ships to users. Both rules now +skip test, spec, fixture, story, and script files (`isTestlikeFilename`), so a +`new Function(...)` inside a `*.test.ts` or a throwaway token in `__tests__/` is +no longer reported. The rules stay fully enabled in production code. diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/auth-token-in-web-storage.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/auth-token-in-web-storage.test.ts index 5b52de1f46..1c879da771 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/auth-token-in-web-storage.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/auth-token-in-web-storage.test.ts @@ -57,4 +57,18 @@ describe("auth-token-in-web-storage", () => { const result = runRule(authTokenInWebStorage, `const t = localStorage.token;`); expect(result.diagnostics).toHaveLength(0); }); + + it("does not flag a token write in a `.test.ts` file (throwaway test token)", () => { + const result = runRule(authTokenInWebStorage, `localStorage.setItem("authToken", token);`, { + filename: "src/auth.test.ts", + }); + expect(result.diagnostics).toHaveLength(0); + }); + + it("does not flag a token write inside a `__tests__` directory", () => { + const result = runRule(authTokenInWebStorage, `localStorage.setItem("jwt", x);`, { + filename: "src/__tests__/auth.ts", + }); + expect(result.diagnostics).toHaveLength(0); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/auth-token-in-web-storage.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/auth-token-in-web-storage.ts index bcc24036d8..d8b9383d95 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/auth-token-in-web-storage.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/auth-token-in-web-storage.ts @@ -1,4 +1,5 @@ import { defineRule } from "../../utils/define-rule.js"; +import { skipNonProductionFiles } from "../../utils/skip-non-production-files.js"; import type { EsTreeNode } from "../../utils/es-tree-node.js"; import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; import { isNodeOfType } from "../../utils/is-node-of-type.js"; @@ -51,7 +52,7 @@ export const authTokenInWebStorage = defineRule({ severity: "warn", recommendation: "Don't persist auth tokens (JWTs, access/refresh tokens, secrets) in `localStorage`/`sessionStorage`; they're readable by any XSS. Use an `HttpOnly` cookie set by the server.", - create: (context) => ({ + create: skipNonProductionFiles((context) => ({ // `localStorage.setItem("authToken", t)` CallExpression(node: EsTreeNodeOfType<"CallExpression">) { const callee = node.callee; @@ -79,5 +80,5 @@ export const authTokenInWebStorage = defineRule({ if (!propertyName || !SENSITIVE_KEY_PATTERN.test(propertyName)) return; context.report({ node: target, message: MESSAGE }); }, - }), + })), }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/no-eval.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/no-eval.test.ts new file mode 100644 index 0000000000..49a6b8bfbe --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/no-eval.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { noEval } from "./no-eval.js"; + +describe("no-eval", () => { + it("flags eval() in production code", () => { + const result = runRule(noEval, `eval(userInput);`, { filename: "src/run.ts" }); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("eval()"); + }); + + it("flags `new Function(...)` in production code", () => { + const result = runRule(noEval, `const fn = new Function("return 1");`, { + filename: "src/run.ts", + }); + expect(result.diagnostics).toHaveLength(1); + expect(result.diagnostics[0].message).toContain("new Function()"); + }); + + it("flags a stringy setTimeout in production code", () => { + const result = runRule(noEval, `setTimeout("doThing()", 100);`, { filename: "src/run.ts" }); + expect(result.diagnostics).toHaveLength(1); + }); + + it("does not flag `new Function(...)` in a `.test.ts` file", () => { + const result = runRule(noEval, `const fn = new Function("return 1");`, { + filename: "lib/pages/document/_applyThemeForDocument.test.ts", + }); + expect(result.diagnostics).toHaveLength(0); + }); + + it("does not flag eval() inside a `__tests__` directory", () => { + const result = runRule(noEval, `eval(userInput);`, { + filename: "src/__tests__/run.ts", + }); + expect(result.diagnostics).toHaveLength(0); + }); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/no-eval.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/no-eval.ts index ef89f52df8..3f2758f4e5 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/no-eval.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/security/no-eval.ts @@ -1,4 +1,5 @@ import { defineRule } from "../../utils/define-rule.js"; +import { skipNonProductionFiles } from "../../utils/skip-non-production-files.js"; import type { RuleContext } from "../../utils/rule-context.js"; import { isNodeOfType } from "../../utils/is-node-of-type.js"; import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; @@ -9,7 +10,7 @@ export const noEval = defineRule({ severity: "error", recommendation: "Use `JSON.parse` for data, or rewrite the code so it doesn't build and run code from strings.", - create: (context: RuleContext) => ({ + create: skipNonProductionFiles((context: RuleContext) => ({ CallExpression(node: EsTreeNodeOfType<"CallExpression">) { if (isNodeOfType(node.callee, "Identifier") && node.callee.name === "eval") { context.report({ @@ -40,5 +41,5 @@ export const noEval = defineRule({ }); } }, - }), + })), }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.ts index 8e490c380d..dffdca6c73 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/define-rule.ts @@ -1,9 +1,9 @@ import type { FileScan } from "./file-scan.js"; -import { isTestlikeFilename } from "./is-testlike-filename.js"; import { fileImportsNonReactJsxDialect, jsxAttributeIsNonReactDialectMarker, } from "./non-react-jsx-dialect.js"; +import { skipNonProductionFiles } from "./skip-non-production-files.js"; import type { Rule } from "./rule.js"; import type { EsTreeNodeOfType } from "./es-tree-node-of-type.js"; @@ -15,21 +15,6 @@ import type { EsTreeNodeOfType } from "./es-tree-node-of-type.js"; // registration, tags, and severity flow identically either way. export type RuleDefinition = Rule | (Omit & { scan: FileScan }); -// Rules tagged `"test-noise"` are by-design noisy in tests / stories / -// playgrounds — design-system style preferences, deprecated-API hints, -// auto-parallelizable awaits, etc. None of these apply to files that -// don't ship to users. We wrap `create()` once here so every such rule -// auto-skips testlike files without each one re-implementing the check. -const wrapCreateForTestNoise = < - CreateFn extends (context: { filename?: string }) => Record, ->( - create: CreateFn, -): CreateFn => - ((context) => { - if (isTestlikeFilename(context.filename)) return {}; - return create(context); - }) as CreateFn; - // Rules tagged `"react-jsx-only"` apply React-flavoured semantics // (a11y semantics tuned for React's synthetic-event listener naming, // React-cased prop names, etc.) and should pass through for files @@ -101,13 +86,15 @@ export const defineRule = (rule: RuleDefinition): Rule => { } const tags = rule.tags; let wrappedCreate = rule.create; - // `migration-hint` wins over `test-noise` — deprecated API usage in - // test code is the very surface that needs migration (`react-dom/test-utils` - // imports, legacy lifecycle methods in test class fixtures, etc.). - // Skip the test-noise wrapper when the rule has both tags. + // Rules tagged `"test-noise"` are by-design noisy in non-production files + // (design-system style preferences, deprecated-API hints, auto-parallelizable + // awaits, …), so we auto-skip testlike files for them. `migration-hint` wins: + // deprecated API usage in test code is the very surface that needs migration + // (`react-dom/test-utils` imports, legacy lifecycle methods in test fixtures), + // so a rule carrying both tags keeps firing there. const honorsTestNoise = tags?.includes("test-noise") && !tags?.includes("migration-hint"); if (honorsTestNoise) { - wrappedCreate = wrapCreateForTestNoise(wrappedCreate as never) as never; + wrappedCreate = skipNonProductionFiles(wrappedCreate); } if (tags?.includes("react-jsx-only")) { wrappedCreate = wrapCreateForReactJsxOnly(wrappedCreate as never) as never; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/skip-non-production-files.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/skip-non-production-files.ts new file mode 100644 index 0000000000..9e9a9da142 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/skip-non-production-files.ts @@ -0,0 +1,14 @@ +import { isTestlikeFilename } from "./is-testlike-filename.js"; +import type { RuleContext } from "./rule-context.js"; +import type { RuleVisitors } from "./rule-visitors.js"; + +// Wrap a rule's `create` so it produces no visitors in non-production files +// (tests, specs, fixtures, stories, scripts, …). Used by `defineRule` for the +// `test-noise` tag and directly by rules whose finding is only actionable in +// code that ships to users — e.g. the security rules, where `eval` / +// `new Function` / a token in web storage is not a real vulnerability in test +// scaffolding that never reaches a browser. +export const skipNonProductionFiles = + (create: (context: RuleContext) => RuleVisitors) => + (context: RuleContext): RuleVisitors => + isTestlikeFilename(context.filename) ? {} : create(context); From f028d8b19daec982c6248ffc067ee37f8fb700a4 Mon Sep 17 00:00:00 2001 From: Ray Arayilakath Date: Thu, 2 Jul 2026 00:12:30 -0500 Subject: [PATCH 03/11] feat(telemetry): track which rules users disable and suppress (#1016) Co-authored-by: Claude Fable 5 --- .changeset/rule-ignore-telemetry.md | 5 ++ .../core/src/build-diagnostic-pipeline.ts | 34 ++++++++-- packages/core/src/rule-key-aliases.ts | 13 ++++ packages/core/src/run-inspect.ts | 12 ++++ packages/core/src/types/diagnostic.ts | 14 ++++ packages/core/src/types/index.ts | 1 + .../merge-and-filter-diagnostics.test.ts | 64 ++++++++++++++++++- .../src/cli/utils/build-run-event.ts | 32 +++++++++- .../react-doctor/src/cli/utils/constants.ts | 10 ++- .../src/cli/utils/record-scan-metrics.ts | 52 ++++++++++++++- .../src/cli/utils/scan-result-cache.ts | 7 ++ packages/react-doctor/src/inspect.ts | 4 ++ .../tests/build-run-event.test.ts | 21 ++++++ .../tests/record-scan-metrics.test.ts | 37 ++++++++++- .../tests/scan-result-cache.test.ts | 2 + 15 files changed, 298 insertions(+), 10 deletions(-) create mode 100644 .changeset/rule-ignore-telemetry.md diff --git a/.changeset/rule-ignore-telemetry.md b/.changeset/rule-ignore-telemetry.md new file mode 100644 index 0000000000..88bdd27d5c --- /dev/null +++ b/.changeset/rule-ignore-telemetry.md @@ -0,0 +1,5 @@ +--- +"react-doctor": patch +--- + +Add anonymized telemetry for which rules users silence. A `rule.disabled` counter records config off-switches (`rules: "off"` and `ignore.rules`, keyed by canonicalized rule + source) once per scan, and a `rule.suppressed` counter records findings the diagnostic pipeline dropped per user intent — config off-switch, per-path `ignore.overrides` entry, or inline `react-doctor-disable*` comment — with per-source rollups (`diag.suppressed*`) on the per-scan wide event. No rule identity ever rode telemetry for silenced rules before, so rule-rejection (the strongest false-positive signal) was unmeasurable. diff --git a/packages/core/src/build-diagnostic-pipeline.ts b/packages/core/src/build-diagnostic-pipeline.ts index cad61b9ecf..dfd4a063d9 100644 --- a/packages/core/src/build-diagnostic-pipeline.ts +++ b/packages/core/src/build-diagnostic-pipeline.ts @@ -4,6 +4,7 @@ import type { DiagnosticFileContext, ReactDoctorConfig, RuleSeverityOverride, + SuppressedRuleCount, } from "./types/index.js"; import { compileIgnoreOverrides, @@ -43,6 +44,16 @@ interface BuildDiagnosticPipelineInput { export interface DiagnosticPipeline { readonly apply: (diagnostic: Diagnostic) => Diagnostic | null; + /** + * Per-rule tallies of the diagnostics `apply` dropped because the user + * explicitly silenced the rule — the config off switches (severity `"off"`, + * `ignore.rules`), per-path `ignore.overrides`, and inline disable + * comments. Engine-owned drops (test-file auto-suppression, the library + * gate, the global warnings hide, `ignore.files` patterns, the + * `textComponents` / `runtimeGlobals` feature knobs) are deliberately not + * counted: they say nothing about the user rejecting a specific rule. + */ + readonly summarizeSuppressions: () => SuppressedRuleCount[]; } const collectStringSet = (values: unknown): ReadonlySet => { @@ -103,6 +114,18 @@ export const buildDiagnosticPipeline = ( const fileLinesCache = new Map(); const fileContextCache = new Map(); const libraryFileCache = new Map(); + const suppressions = new Map(); + + const suppress = (diagnostic: Diagnostic, source: SuppressedRuleCount["source"]): null => { + const { ruleKey } = getDiagnosticRuleIdentity(diagnostic); + const suppressionKey = `${ruleKey}\u0000${source}`; + const existing = suppressions.get(suppressionKey); + suppressions.set( + suppressionKey, + existing ? { ...existing, count: existing.count + 1 } : { rule: ruleKey, source, count: 1 }, + ); + return null; + }; // App-only rules (`static-components`, `no-render-prop-children`) describe // patterns that are noise in published libraries — silence them on files @@ -213,7 +236,7 @@ export const buildDiagnosticPipeline = ( { ruleKey, category }, severityControls, ); - if (explicitSeverityOverride === "off") return null; + if (explicitSeverityOverride === "off") return suppress(current, "config"); if (explicitSeverityOverride !== undefined) { current = restampSeverity(current, explicitSeverityOverride); } @@ -239,11 +262,13 @@ export const buildDiagnosticPipeline = ( if (userConfig) { const ruleIdentifier = `${current.plugin}/${current.rule}`; - if (isRuleIgnored(ruleIdentifier)) return null; + if (isRuleIgnored(ruleIdentifier)) return suppress(current, "config"); if (isFileIgnoredByPatterns(current.filePath, rootDirectory, ignoredFilePatterns)) { return null; } - if (isDiagnosticIgnoredByOverrides(current, rootDirectory, compiledOverrides)) return null; + if (isDiagnosticIgnoredByOverrides(current, rootDirectory, compiledOverrides)) { + return suppress(current, "override"); + } if (isRnRawTextSuppressedByConfig(current)) return null; if (isJsxNoUndefSuppressedByConfig(current)) return null; } @@ -254,7 +279,7 @@ export const buildDiagnosticPipeline = ( const ruleIdentifier = `${current.plugin}/${current.rule}`; const diagnosticLineIndex = current.line - 1; const evaluation = evaluateSuppression(lines, diagnosticLineIndex, ruleIdentifier); - if (evaluation.isSuppressed) return null; + if (evaluation.isSuppressed) return suppress(current, "inline"); if (evaluation.nearMissHint) { current = { ...current, suppressionHint: evaluation.nearMissHint }; } @@ -268,5 +293,6 @@ export const buildDiagnosticPipeline = ( return current; }, + summarizeSuppressions: () => [...suppressions.values()], }; }; diff --git a/packages/core/src/rule-key-aliases.ts b/packages/core/src/rule-key-aliases.ts index 537509eadc..abae923dc3 100644 --- a/packages/core/src/rule-key-aliases.ts +++ b/packages/core/src/rule-key-aliases.ts @@ -168,6 +168,19 @@ const isReactDoctorShortIdOf = (bareRuleKey: string, qualifiedRuleKey: string): !bareRuleKey.includes("/") && qualifiedRuleKey === `${REACT_DOCTOR_RULE_KEY_PREFIX}${bareRuleKey}`; +/** + * Canonicalizes a rule key as users write it in config: a legacy alias + * (`react/jsx-key`) maps to its native key, and a bare short id (`no-eval`) + * qualifies as `react-doctor/` — mirroring `isSameRuleKey`'s matching — + * so telemetry groups every spelling of one rule under one key. + */ +export const canonicalizeUserRuleKey = (ruleKey: string): string => { + const nativeRuleKey = canonicalizeRuleKey(ruleKey); + return nativeRuleKey.includes("/") + ? nativeRuleKey + : `${REACT_DOCTOR_RULE_KEY_PREFIX}${nativeRuleKey}`; +}; + export const isSameRuleKey = (candidateRuleKey: string, targetRuleKey: string): boolean => { const canonicalCandidate = canonicalizeRuleKey(candidateRuleKey); const canonicalTarget = canonicalizeRuleKey(targetRuleKey); diff --git a/packages/core/src/run-inspect.ts b/packages/core/src/run-inspect.ts index adbea57f65..9826ec8a18 100644 --- a/packages/core/src/run-inspect.ts +++ b/packages/core/src/run-inspect.ts @@ -11,6 +11,7 @@ import type { ProjectInfo, ReactDoctorConfig, ScoreResult, + SuppressedRuleCount, } from "./types/index.js"; import { assignFixGroups } from "./utils/assign-fix-groups.js"; import { sortDiagnosticsStable } from "./utils/sort-diagnostics-stable.js"; @@ -220,6 +221,16 @@ export interface InspectOutput { */ readonly lintCacheHitFileCount: number | null; readonly lintCacheTotalFileCount: number | null; + /** + * Per-rule tallies of diagnostics the pipeline dropped because the user + * explicitly silenced the rule (config off switches, per-path overrides, + * inline disable comments) — see `DiagnosticPipeline.summarizeSuppressions`. + * Telemetry-only; NOT part of the public `inspect()` `InspectResult`. Note + * that a `rules: "off"` lint rule is removed from the generated oxlint + * config upstream and never fires, so its findings can't be counted here — + * the CLI's scan-level `rule.disabled` counter covers that case. + */ + readonly suppressedRuleCounts: ReadonlyArray; } /** @@ -912,6 +923,7 @@ export const runInspect = ( supplyChainOverlapTimedOut: supplyChainResult.timedOut, lintCacheHitFileCount, lintCacheTotalFileCount, + suppressedRuleCounts: transform.summarizeSuppressions(), }; }).pipe( Effect.withSpan("runInspect", { diff --git a/packages/core/src/types/diagnostic.ts b/packages/core/src/types/diagnostic.ts index d4f7e41c61..3108ca0aa8 100644 --- a/packages/core/src/types/diagnostic.ts +++ b/packages/core/src/types/diagnostic.ts @@ -104,6 +104,20 @@ export interface CleanedDiagnostic { help: string; } +/** + * Per-rule tally of diagnostics the user explicitly silenced, aggregated by + * how: a config-level off switch (`rules: "off"` / `ignore.rules`), a + * per-path `ignore.overrides` entry, or an inline `react-doctor-disable*` + * comment. Telemetry-only — the rule-quality signal for which rules users + * reject — never rendered, scored, or part of the JSON report. + */ +export interface SuppressedRuleCount { + /** Canonical `/` key (see `getDiagnosticRuleIdentity`). */ + readonly rule: string; + readonly source: "config" | "override" | "inline"; + readonly count: number; +} + /** * A discovered source file paired with its on-disk byte size. The size is * the single `fs.statSync` the minified-file gate already pays during diff --git a/packages/core/src/types/index.ts b/packages/core/src/types/index.ts index 5e17c9b8ca..b110b1c3d5 100644 --- a/packages/core/src/types/index.ts +++ b/packages/core/src/types/index.ts @@ -25,6 +25,7 @@ export type { DiagnosticRelatedLocation, OxlintOutput, SourceFileEntry, + SuppressedRuleCount, } from "./diagnostic.js"; export type { HandleErrorOptions } from "./handle-error.js"; export type { diff --git a/packages/core/tests/merge-and-filter-diagnostics.test.ts b/packages/core/tests/merge-and-filter-diagnostics.test.ts index d9adeb09b2..58b77b257e 100644 --- a/packages/core/tests/merge-and-filter-diagnostics.test.ts +++ b/packages/core/tests/merge-and-filter-diagnostics.test.ts @@ -3,8 +3,9 @@ import os from "node:os"; import * as path from "node:path"; import { afterAll, describe, expect, it } from "vite-plus/test"; -import type { Diagnostic } from "@react-doctor/core"; +import type { Diagnostic, ReactDoctorConfig } from "@react-doctor/core"; import { + buildDiagnosticPipeline, clearAutoSuppressionCaches, createNodeReadFileLinesSync, mergeAndFilterDiagnostics, @@ -163,3 +164,64 @@ describe("mergeAndFilterDiagnostics — test-noise tag auto-suppression for asyn expect(filtered).toHaveLength(1); }); }); + +describe("buildDiagnosticPipeline — summarizeSuppressions", () => { + const readNoop = () => null; + const buildPipeline = ( + userConfig: ReactDoctorConfig | null, + rootDirectory: string = path.join(tempRoot, "suppression-summary"), + readFileLinesSync: (filePath: string) => string[] | null = readNoop, + ) => + buildDiagnosticPipeline({ + rootDirectory, + userConfig, + readFileLinesSync, + respectInlineDisables: true, + showWarnings: true, + }); + + it("tallies rules dropped via severity `off` and `ignore.rules` as `config`", () => { + const pipeline = buildPipeline({ + rules: { "react-doctor/no-derived-state-effect": "off" }, + ignore: { rules: ["react-doctor/test-rule"] }, + }); + expect(pipeline.apply(baseDiagnostic())).toBeNull(); + expect(pipeline.apply(baseDiagnostic({ filePath: "src/other.tsx" }))).toBeNull(); + expect(pipeline.apply(buildDiagnostic({ line: 3 }))).toBeNull(); + expect(pipeline.summarizeSuppressions()).toEqual([ + { rule: "react-doctor/no-derived-state-effect", source: "config", count: 2 }, + { rule: "react-doctor/test-rule", source: "config", count: 1 }, + ]); + }); + + it("tallies per-path `ignore.overrides` drops as `override` and leaves survivors uncounted", () => { + const pipeline = buildPipeline({ + ignore: { + overrides: [{ files: ["src/legacy/**"], rules: ["react-doctor/no-derived-state-effect"] }], + }, + }); + expect(pipeline.apply(baseDiagnostic({ filePath: "src/legacy/app.tsx" }))).toBeNull(); + expect(pipeline.apply(baseDiagnostic())).not.toBeNull(); + expect(pipeline.summarizeSuppressions()).toEqual([ + { rule: "react-doctor/no-derived-state-effect", source: "override", count: 1 }, + ]); + }); + + it("tallies inline disable comments as `inline`", () => { + const projectDir = setupCase( + "suppression-summary-inline", + `// react-doctor-disable-next-line react-doctor/no-derived-state-effect\nconst x = 1;\n`, + ); + const pipeline = buildPipeline(null, projectDir, createNodeReadFileLinesSync(projectDir)); + expect(pipeline.apply(baseDiagnostic())).toBeNull(); + expect(pipeline.summarizeSuppressions()).toEqual([ + { rule: "react-doctor/no-derived-state-effect", source: "inline", count: 1 }, + ]); + }); + + it("does not count file-level `ignore.files` drops — they reject a path, not a rule", () => { + const pipeline = buildPipeline({ ignore: { files: ["src/skip.tsx"] } }); + expect(pipeline.apply(baseDiagnostic({ filePath: "src/skip.tsx" }))).toBeNull(); + expect(pipeline.summarizeSuppressions()).toEqual([]); + }); +}); diff --git a/packages/react-doctor/src/cli/utils/build-run-event.ts b/packages/react-doctor/src/cli/utils/build-run-event.ts index 96049ad54f..8f6523279d 100644 --- a/packages/react-doctor/src/cli/utils/build-run-event.ts +++ b/packages/react-doctor/src/cli/utils/build-run-event.ts @@ -4,7 +4,12 @@ import { resolveGithubActionsScoreMetadata, summarizeDiagnostics, } from "@react-doctor/core"; -import type { BlockingLevel, InspectResult, ReactDoctorConfig } from "@react-doctor/core"; +import type { + BlockingLevel, + InspectResult, + ReactDoctorConfig, + SuppressedRuleCount, +} from "@react-doctor/core"; import { buildRuleBlastRadii } from "./diagnostic-grouping.js"; import { ACTION_INPUT_ENVIRONMENT_VARIABLES, detectRunnerOs } from "./is-ci-environment.js"; import { summarizeRuleFirings } from "./record-scan-metrics.js"; @@ -75,6 +80,14 @@ export interface RunEventInput { // A degraded baseline run (no delta computed) skips the CI gate, so the // `wouldBlock` prediction must match — never block on its plain-diff findings. readonly gateExempt?: boolean; + /** + * Per-rule tallies of findings the user explicitly silenced (config off + * switch / per-path override / inline disable comment), from the scan + * payload — so a cache hit replays them. Rolled up to the `diag.suppressed*` + * dims; per-rule identity rides the `rule.suppressed` counter instead + * (100+ rules would blow up the attribute set). Omitted on the failure path. + */ + readonly suppressedRuleCounts?: ReadonlyArray; /** Present only when the scan threw. */ readonly error?: unknown; } @@ -195,6 +208,22 @@ const buildOutcomeAttributes = (input: RunEventInput): RunEventAttributes => { categoryRollup[`category.${toCategoryKey(category)}`] = count; } + // Findings the user explicitly silenced, by mechanism — the per-scan + // complement of the `rule.suppressed` counter (which carries rule identity). + // Absent (not zero) when the caller couldn't supply the tallies. + const suppressionRollup: RunEventAttributes = {}; + if (input.suppressedRuleCounts) { + const countBySource = { config: 0, override: 0, inline: 0 }; + for (const suppression of input.suppressedRuleCounts) { + countBySource[suppression.source] += suppression.count; + } + suppressionRollup.suppressed = + countBySource.config + countBySource.override + countBySource.inline; + suppressionRollup.suppressedConfig = countBySource.config; + suppressionRollup.suppressedOverride = countBySource.override; + suppressionRollup.suppressedInline = countBySource.inline; + } + const attributes: RunEventAttributes = { ...withNamespace("outcome", { status: outcome, @@ -216,6 +245,7 @@ const buildOutcomeAttributes = (input: RunEventInput): RunEventAttributes => { fixGroups: findingsPerFixGroup.size, fixGroupedFindings, ...categoryRollup, + ...suppressionRollup, }), ...withNamespace("score", { value: result.score ? result.score.score : null, diff --git a/packages/react-doctor/src/cli/utils/constants.ts b/packages/react-doctor/src/cli/utils/constants.ts index 6f19d7e570..7807694be5 100644 --- a/packages/react-doctor/src/cli/utils/constants.ts +++ b/packages/react-doctor/src/cli/utils/constants.ts @@ -26,7 +26,8 @@ export const BASELINE_FILES_TEMP_DIR_PREFIX = "react-doctor-baseline-"; // `readPersistedCache` instead of deserializing into an invalid payload. // Bumped to 2: `CachedScanPayload` gained the required `supplyChainOverlapTimedOut` // (supply-chain overlap) and `deadCodeOverlapped` (dead-code overlap) fields. -export const SCAN_RESULT_CACHE_SCHEMA_VERSION = 2; +// Bumped to 3: gained the required `suppressedRuleCounts` field (suppression telemetry). +export const SCAN_RESULT_CACHE_SCHEMA_VERSION = 3; export const SCAN_RESULT_CACHE_MAX_ENTRY_COUNT = 20; export const CACHE_FILENAME_HASH_LENGTH_CHARS = 16; @@ -168,6 +169,13 @@ export const METRIC = { scanCheckSkipped: "scan.check_skipped", baselineDegraded: "baseline.degraded", ruleFired: "rule.fired", + // Rule-rejection telemetry, both keyed by `rule` + `source` attributes: + // `rule.disabled` counts one per scan per config-off rule (`rules: "off"` / + // `ignore.rules` — the former never fires, so this is its only signal); + // `rule.suppressed` counts findings the pipeline dropped per user silencing + // (config / per-path override / inline disable comment). + ruleDisabled: "rule.disabled", + ruleSuppressed: "rule.suppressed", lintFailed: "lint.failed", deadCodeFailed: "deadcode.failed", scoreUnavailable: "score.unavailable", diff --git a/packages/react-doctor/src/cli/utils/record-scan-metrics.ts b/packages/react-doctor/src/cli/utils/record-scan-metrics.ts index edd08a37aa..851e44cb65 100644 --- a/packages/react-doctor/src/cli/utils/record-scan-metrics.ts +++ b/packages/react-doctor/src/cli/utils/record-scan-metrics.ts @@ -1,5 +1,10 @@ -import { getDiagnosticRuleIdentity } from "@react-doctor/core"; -import type { Diagnostic, InspectResult } from "@react-doctor/core"; +import { canonicalizeUserRuleKey, getDiagnosticRuleIdentity } from "@react-doctor/core"; +import type { + Diagnostic, + InspectResult, + ReactDoctorConfig, + SuppressedRuleCount, +} from "@react-doctor/core"; import { METRIC } from "./constants.js"; import { recordCount, recordDistribution } from "./record-metric.js"; @@ -43,6 +48,36 @@ export const summarizeRuleFirings = (diagnostics: ReadonlyArray): Ru return [...firings.values()]; }; +export interface DisabledRule { + readonly rule: string; + readonly source: "rules" | "ignore"; +} + +/** + * Enumerates the rules a config turns off entirely — `rules: { x: "off" }` + * entries and the `ignore.rules` list — with keys canonicalized so every + * spelling of one rule (legacy alias, bare short id) groups under one + * `rule` attribute in Sentry. Deliberately scan-independent: an `"off"` + * lint rule is removed from the generated oxlint config and never fires, + * so the per-diagnostic `rule.suppressed` counter can't see it. + */ +export const summarizeDisabledRules = (userConfig: ReactDoctorConfig | null): DisabledRule[] => { + const disabledRules = new Map(); + const record = (configuredRuleKey: string, source: DisabledRule["source"]): void => { + const rule = canonicalizeUserRuleKey(configuredRuleKey); + disabledRules.set(`${rule}\u0000${source}`, { rule, source }); + }; + for (const [ruleKey, severity] of Object.entries(userConfig?.rules ?? {})) { + if (severity === "off") record(ruleKey, "rules"); + } + if (Array.isArray(userConfig?.ignore?.rules)) { + for (const ruleKey of userConfig.ignore.rules) { + if (typeof ruleKey === "string") record(ruleKey, "ignore"); + } + } + return [...disabledRules.values()]; +}; + export interface ScanMetricsInput { readonly result: InspectResult; /** `"diff"` (changed/staged files) or `"full"` (whole project). */ @@ -65,6 +100,10 @@ export interface ScanMetricsInput { readonly didDeadCodeFail: boolean; /** A baseline run that couldn't compute a delta and fell back to a plain diff. */ readonly baselineDegraded: boolean; + /** Feeds the scan-level `rule.disabled` counter (see `summarizeDisabledRules`). */ + readonly userConfig: ReactDoctorConfig | null; + /** `CachedScanPayload["suppressedRuleCounts"]` — present on fresh and cache-hit paths. */ + readonly suppressedRuleCounts: ReadonlyArray; } /** @@ -117,6 +156,15 @@ export const recordScanMetrics = (input: ScanMetricsInput): void => { severity: firing.severity, }); } + for (const disabled of summarizeDisabledRules(input.userConfig)) { + recordCount(METRIC.ruleDisabled, 1, { rule: disabled.rule, source: disabled.source }); + } + for (const suppression of input.suppressedRuleCounts) { + recordCount(METRIC.ruleSuppressed, suppression.count, { + rule: suppression.rule, + source: suppression.source, + }); + } // "Clean" means the scan actually completed and found nothing — not that a // failed/incomplete run (lint or dead-code failed, a check was skipped) // happened to produce zero diagnostics. `skippedChecks` already includes diff --git a/packages/react-doctor/src/cli/utils/scan-result-cache.ts b/packages/react-doctor/src/cli/utils/scan-result-cache.ts index f631bb4736..caa0de2a68 100644 --- a/packages/react-doctor/src/cli/utils/scan-result-cache.ts +++ b/packages/react-doctor/src/cli/utils/scan-result-cache.ts @@ -10,6 +10,7 @@ import type { InspectResult, ReactDoctorConfig, ScoreResult, + SuppressedRuleCount, } from "@react-doctor/core"; import { CACHE_FILENAME_HASH_LENGTH_CHARS, @@ -47,6 +48,12 @@ export interface CachedScanPayload { */ readonly scanConcurrency?: number; readonly supplyChainOverlapTimedOut: boolean; + /** + * `InspectOutput["suppressedRuleCounts"]` — deterministic for a given + * commit + config (part of the cache key), so a cache hit replays the same + * suppression telemetry the fresh scan emitted. + */ + readonly suppressedRuleCounts: ReadonlyArray; } interface PersistedScanResultCacheEntry { diff --git a/packages/react-doctor/src/inspect.ts b/packages/react-doctor/src/inspect.ts index c4e8e1a9ff..ef9ccac163 100644 --- a/packages/react-doctor/src/inspect.ts +++ b/packages/react-doctor/src/inspect.ts @@ -709,6 +709,7 @@ const runInspectWithRuntime = async ( ? "native-binding-missing" : output.lintFailureReasonKind, supplyChainOverlapTimedOut: output.supplyChainOverlapTimedOut, + suppressedRuleCounts: output.suppressedRuleCounts, }; if (cacheKey !== null && scanResultCache !== null && shouldStoreScanPayload(payload)) { scanResultCache.store(cacheKey, payload); @@ -844,6 +845,8 @@ const renderAndRecordScan = async (input: RenderAndRecordScanInput): Promise { expect(withoutDrops["lint.droppedFileCount"]).toBeUndefined(); }); + it("rolls suppressed findings up by source and drops the dims when tallies are absent", () => { + const attributes = buildRunEventAttributes( + baseInput({ + result: buildResult(), + suppressedRuleCounts: [ + { rule: "react-doctor/no-danger", source: "config", count: 3 }, + { rule: "react-doctor/jsx-key", source: "inline", count: 2 }, + { rule: "react-doctor/alt-text", source: "inline", count: 1 }, + ], + }), + ); + expect(attributes["diag.suppressed"]).toBe(6); + expect(attributes["diag.suppressedConfig"]).toBe(3); + expect(attributes["diag.suppressedOverride"]).toBe(0); + expect(attributes["diag.suppressedInline"]).toBe(3); + + // Absent tallies (e.g. the failure path) read as "unknown", not zero. + const withoutTallies = buildRunEventAttributes(baseInput({ result: buildResult() })); + expect(withoutTallies["diag.suppressed"]).toBeUndefined(); + }); + it("captures config shape and drops null/undefined-valued attributes", () => { const attributes = buildRunEventAttributes( baseInput({ diff --git a/packages/react-doctor/tests/record-scan-metrics.test.ts b/packages/react-doctor/tests/record-scan-metrics.test.ts index 4b4c752c7b..d3de2fb7f8 100644 --- a/packages/react-doctor/tests/record-scan-metrics.test.ts +++ b/packages/react-doctor/tests/record-scan-metrics.test.ts @@ -1,6 +1,9 @@ import { describe, expect, it } from "vite-plus/test"; import type { Diagnostic } from "@react-doctor/core"; -import { summarizeRuleFirings } from "../src/cli/utils/record-scan-metrics.js"; +import { + summarizeDisabledRules, + summarizeRuleFirings, +} from "../src/cli/utils/record-scan-metrics.js"; const buildDiagnostic = (overrides: Partial): Diagnostic => ({ filePath: "src/App.tsx", @@ -60,3 +63,35 @@ describe("summarizeRuleFirings", () => { expect(summarizeRuleFirings([])).toEqual([]); }); }); + +describe("summarizeDisabledRules", () => { + it("lists `rules: off` entries with canonicalized keys and skips warn/error overrides", () => { + const disabledRules = summarizeDisabledRules({ + rules: { + "react/jsx-key": "off", + "no-eval": "off", + "react-doctor/no-danger": "warn", + }, + }); + expect(disabledRules).toEqual([ + { rule: "react-doctor/jsx-key", source: "rules" }, + { rule: "react-doctor/no-eval", source: "rules" }, + ]); + }); + + it("lists `ignore.rules` entries, deduping alias spellings of one rule per source", () => { + const disabledRules = summarizeDisabledRules({ + rules: { "react-doctor/jsx-key": "off" }, + ignore: { rules: ["react/jsx-key", "react-doctor/jsx-key"] }, + }); + expect(disabledRules).toEqual([ + { rule: "react-doctor/jsx-key", source: "rules" }, + { rule: "react-doctor/jsx-key", source: "ignore" }, + ]); + }); + + it("returns an empty list for a null or rule-free config", () => { + expect(summarizeDisabledRules(null)).toEqual([]); + expect(summarizeDisabledRules({})).toEqual([]); + }); +}); diff --git a/packages/react-doctor/tests/scan-result-cache.test.ts b/packages/react-doctor/tests/scan-result-cache.test.ts index 6f7c0b7f43..71347b99ce 100644 --- a/packages/react-doctor/tests/scan-result-cache.test.ts +++ b/packages/react-doctor/tests/scan-result-cache.test.ts @@ -269,6 +269,8 @@ describe("scan result cache", () => { scanElapsedMilliseconds: firstResult.scanElapsedMilliseconds ?? 0, baselineDelta: undefined, lintFailureReasonKind: null, + supplyChainOverlapTimedOut: false, + suppressedRuleCounts: [], }); const verboseResult = await inspect(projectDirectory, { From 6e6762667838caa518cea203fe985184ab0bd31f Mon Sep 17 00:00:00 2001 From: Aiden Bai Date: Thu, 2 Jul 2026 00:13:17 -0700 Subject: [PATCH 04/11] FP-FIX(rules): shared FP-hardening infra (test harness + cross-cutting helpers) (#988) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(rules): shared FP-hardening infra (test harness + cross-cutting helpers) Foundation for the rule false-positive split off #986. Cross-cutting pieces every domain build/test depends on: - test-utils/attach-source-locations: attach `range` to AST nodes so eslint-scope-backed rules resolve correctly under the test harness. - constants/react: shared hook/name tables used by ~10 rule domains. - utils/contains-fetch-call, utils/build-same-file-memo-registry: shared detectors consumed by multiple domains. No changeset (intentional). * fix(rules): descend into synchronously invoked functions in containsFetchCall The stopAtFunctionBoundary walk pruned every nested function, so effects that kick off fetches via an async IIFE or a name-invoked inner function (`async function loadData(){...} loadData()`, `const loadData = async () => {...}; void loadData()`) were missed. Replace the blanket prune with a fixed-point walk that descends only into functions the body itself invokes, still skipping escaping callbacks like event handlers and returned cleanups. * fix(rules): drop `watch` from cleanup-returning subscription methods Returning a `watch` handle is not cleanup: react-hook-form's `form.watch(cb)` returns `{ unsubscribe }` (not callable) and `fs.watch` returns an FSWatcher needing `.close()`. Keep `listen`, whose returned disposer follows the `subscribe` contract, and pin the behavior with effect-needs-cleanup regression tests. * FP-FIX(a11y): eliminate false positives across accessibility rules (#989) * fix(a11y): eliminate false positives across accessibility rules Domain slice of the rule false-positive hardening split off #986: rule detector refinements + regression tests for this domain (and its domain-local utils/constants/fixtures). No changeset (intentional). * refactor(a11y): deslop FP-fix rules (reuse helpers, de-nest, relocate getImplicitRole) Behavior-preserving cleanup of the accessibility false-positive fixes: - interactive-supports-focus: replace the inline spread-attribute check with the existing `hasJsxSpreadAttribute` util (drops a byte-for-byte reimplementation and a now-dead `isNodeOfType` import). - no-redundant-roles: de-nest the implicit-role ternary into an if/else chain (AGENTS.md: no nested ternaries) and de-shadow the filter parameter. - prefer-tag-over-role: merge the two disjoint role sets that were only ever used together in one boolean union into one `ROLES_WITHOUT_CLEAN_TAG`. - role-supports-aria-props / no-redundant-roles: relocate `getImplicitRole` into `utils/get-implicit-role.ts`, removing the only rule-to-rule import in the a11y directory (shared helpers belong in utils/). - control-has-associated-label: un-glue and tighten a comment block. No behavior change: full oxlint-plugin suite (7380 tests) green; typecheck, lint, and format all clean. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(a11y): flag role={undefined} in no-noninteractive-element-interactions `collectRoleBranches` marked `null`/`false` literals as role-less but ignored the `undefined` Identifier, so `role={undefined}` — and ternary branches resolving to it — fell through the empty-branch early return and silenced the rule, a false negative on a genuinely role-less non-interactive element. Treat the `undefined` identifier like a `null`/`false` literal (`hasNonRoleBranch`), while leaving a genuinely opaque variable role (`role={dynamicRole}`) silent. Adds regression coverage for both. Addresses the Cursor Bugbot "Undefined role silences rule" finding on #989. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(a11y): treat spread children as content in objectHasAccessibleChild `objectHasAccessibleChild` counted an explicit `children` prop as content but not a `{...props}` spread, so `

` (forwardRef card titles, markdown component overrides) and `` were flagged as empty. The RDE 500-repo run surfaced this as heading-has-content's dominant false positive (~83% of sampled hits, all ``). Treat a JSX spread as possible children in the shared util — one fix covers heading-has-content, anchor-has-content, and alt-text's check — matching the spread-bail pattern six sibling a11y rules already use. Genuinely empty elements (no spread) are still flagged; a11y suite green (3206), no OXC fixture divergence. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(rules): treat dot-prefixed tool dirs (.dumi, .storybook) as non-production in isTestlikeFilename * fix(a11y): revert the ARIA-1.2 implicit-combobox upgrade in getImplicitRole (oxc parity) * fix(a11y): exempt