Skip to content

Commit ce14ae7

Browse files
NisargIOcursoragent
andcommitted
fix(nextjs): stop flagging response.headers and local Map/Set/Headers in GET handlers (#206)
Rewrites `nextjs-no-side-effect-in-get-handler` precision so it no longer floods Next.js codebases with false positives from `response.headers.set()` and request-scoped `new Map/Set/Headers/URLSearchParams/FormData(...)` mutations, while still catching real CSRF-relevant writes: drizzle/prisma ORM mutations, module-level mutable state, mutating fetch, and all three forms of `cookies().set/delete()` including aliased `const cs = await cookies(); cs.set(...)`. Also folds in the safe handler-resolution improvements from PR #251 (cron route skip and depth-bounded `const GET = withAuth(handler)` resolution). Supersedes #209, #211, #233, #238. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 0a52ca3 commit ce14ae7

16 files changed

Lines changed: 1093 additions & 80 deletions

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
"react-doctor": patch
3+
"eslint-plugin-react-doctor": patch
4+
"oxlint-plugin-react-doctor": patch
5+
---
6+
7+
Fix trust-breaking false positives in `nextjs-no-side-effect-in-get-handler`
8+
(issue #206). The rule used to flag any `<member>.<set|append|delete|create|
9+
insert|update|upsert|remove|destroy>()` call as a server-state mutation,
10+
which flooded one Next.js 14 codebase with 138 false errors — every single
11+
one a `response.headers.set(...)` response-shaping call.
12+
13+
The rule now skips these shapes, all of which only shape the outbound
14+
response or mutate request-scoped collections:
15+
16+
- `response.headers.set/append/delete(...)` and any chain ending in
17+
`.headers` (e.g. `NextResponse.json({...}).headers.set(...)`,
18+
`(await fetcher()).headers.append(...)`).
19+
- Locally-constructed `new Map/Set/WeakMap/WeakSet/Headers/URLSearchParams/
20+
FormData/Response/NextResponse(...)` bindings and any mutation on those
21+
aliases.
22+
- `new URL(...).searchParams.set(...)` and any `.searchParams.*()` chain.
23+
- `headers()` / `(await headers())` from `next/headers` (returns
24+
`ReadonlyHeaders`; any mutation would throw at runtime) and any aliased
25+
`const h = headers(); h.get(...)`.
26+
- Route handlers under `/cron/` or `/jobs/cron/` — Vercel Cron always
27+
invokes GET and is expected to do real work.
28+
29+
The rule still flags real CSRF-relevant side effects:
30+
31+
- ORM mutations like drizzle `db.update(table).set({...})`, `prisma.user.
32+
create(...)`, `db.insert(...)`, `repository.upsert(...)`.
33+
- Module-level mutable state (`const cache = new Map()` declared outside
34+
the handler, then `cache.set(...)` inside it).
35+
- `fetch(url, { method: "POST" | "PUT" | "DELETE" | "PATCH" })`.
36+
- `cookies().set/append/delete()` in all forms: direct, `(await cookies()).
37+
set(...)`, and aliased `const cookieStore = await cookies(); cookieStore.
38+
set(...)` — the alias resolution now flags previously-missed handlers.
39+
- Mutating route segments (`/logout`, `/signout`, `/unsubscribe`, `/delete`,
40+
…).
41+
42+
The rule also gained depth-bounded handler-binding resolution so
43+
`export const GET = withAuth(handler)` and `export const GET = app.get('/x',
44+
handler).get('/y', handler)` get scanned correctly.
45+
46+
Closes #206. Supersedes PRs #209, #211, #233, and #238.

packages/oxlint-plugin-react-doctor/src/plugin/constants/library.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,18 @@ export const MUTATION_METHOD_NAMES = new Set([
2929
]);
3030

3131
export const MUTATING_HTTP_METHODS = new Set(["POST", "PUT", "DELETE", "PATCH"]);
32+
33+
export const SAFE_MUTABLE_CONSTRUCTOR_NAMES = new Set([
34+
"Map",
35+
"Set",
36+
"WeakMap",
37+
"WeakSet",
38+
"Headers",
39+
"URLSearchParams",
40+
"FormData",
41+
"Response",
42+
"NextResponse",
43+
]);
44+
45+
export const RESPONSE_FACTORY_OBJECTS = new Set(["Response", "NextResponse"]);
46+
export const RESPONSE_FACTORY_METHODS = new Set(["json", "redirect", "next", "rewrite", "error"]);

packages/oxlint-plugin-react-doctor/src/plugin/constants/nextjs.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ export const APP_DIRECTORY_PATTERN = /\/app\//;
2424

2525
export const ROUTE_HANDLER_FILE_PATTERN = /\/route\.(tsx?|jsx?)$/;
2626

27+
export const CRON_ROUTE_PATTERN = /\/(?:cron|jobs\/cron)(?:\/|$)/i;
28+
2729
export const MUTATING_ROUTE_SEGMENTS = new Set([
2830
"logout",
2931
"log-out",

packages/oxlint-plugin-react-doctor/src/plugin/constants/thresholds.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,4 @@ export const SEQUENTIAL_AWAIT_THRESHOLD = 3;
77
export const PROPERTY_ACCESS_REPEAT_THRESHOLD = 3;
88
export const BOOLEAN_PROP_THRESHOLD = 4;
99
export const RENDER_PROP_PROLIFERATION_THRESHOLD = 3;
10+
export const GET_HANDLER_BINDING_RESOLUTION_DEPTH = 3;
Lines changed: 203 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,18 @@
1-
import { MUTATING_ROUTE_SEGMENTS, ROUTE_HANDLER_FILE_PATTERN } from "../../constants/nextjs.js";
1+
import {
2+
CRON_ROUTE_PATTERN,
3+
MUTATING_ROUTE_SEGMENTS,
4+
ROUTE_HANDLER_FILE_PATTERN,
5+
} from "../../constants/nextjs.js";
6+
import { GET_HANDLER_BINDING_RESOLUTION_DEPTH } from "../../constants/thresholds.js";
7+
import { collectLocallyScopedCookieBindings } from "../../utils/collect-locally-scoped-cookie-bindings.js";
8+
import { collectLocallyScopedSafeBindings } from "../../utils/collect-locally-scoped-safe-bindings.js";
29
import { defineRule } from "../../utils/define-rule.js";
310
import { findSideEffect } from "../../utils/find-side-effect.js";
411
import type { EsTreeNode } from "../../utils/es-tree-node.js";
12+
import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js";
13+
import { isNodeOfType } from "../../utils/is-node-of-type.js";
514
import type { Rule } from "../../utils/rule.js";
615
import type { RuleContext } from "../../utils/rule-context.js";
7-
import { isNodeOfType } from "../../utils/is-node-of-type.js";
8-
import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js";
916

1017
const extractMutatingRouteSegment = (filename: string): string | null => {
1118
const segments = filename.split("/");
@@ -16,30 +23,173 @@ const extractMutatingRouteSegment = (filename: string): string | null => {
1623
return null;
1724
};
1825

19-
const getExportedGetHandlerBody = (node: EsTreeNode): EsTreeNode | null => {
20-
if (!isNodeOfType(node, "ExportNamedDeclaration")) return null;
26+
const buildProgramBindingLookup = (
27+
programNode: EsTreeNode,
28+
): ((identifierName: string) => EsTreeNode | null) => {
29+
const topLevelBindings = new Map<string, EsTreeNode>();
30+
if (!isNodeOfType(programNode, "Program")) return () => null;
31+
32+
const collectFromStatements = (statements: EsTreeNode[]): void => {
33+
for (const statement of statements) {
34+
if (isNodeOfType(statement, "VariableDeclaration")) {
35+
for (const declarator of statement.declarations ?? []) {
36+
if (!isNodeOfType(declarator.id, "Identifier")) continue;
37+
if (!declarator.init) continue;
38+
topLevelBindings.set(declarator.id.name, declarator.init);
39+
}
40+
continue;
41+
}
42+
if (
43+
isNodeOfType(statement, "FunctionDeclaration") &&
44+
isNodeOfType(statement.id, "Identifier") &&
45+
statement.body
46+
) {
47+
topLevelBindings.set(statement.id.name, statement);
48+
continue;
49+
}
50+
if (isNodeOfType(statement, "ExportNamedDeclaration") && statement.declaration) {
51+
collectFromStatements([statement.declaration]);
52+
}
53+
}
54+
};
55+
56+
collectFromStatements(programNode.body ?? []);
57+
return (identifierName: string) => topLevelBindings.get(identifierName) ?? null;
58+
};
59+
60+
const isExportedGetHandler = (node: EsTreeNode): boolean => {
61+
if (!isNodeOfType(node, "ExportNamedDeclaration")) return false;
2162
const declaration = node.declaration;
22-
if (!declaration) return null;
63+
if (!declaration) return false;
2364

2465
if (isNodeOfType(declaration, "FunctionDeclaration") && declaration.id?.name === "GET") {
25-
return declaration.body;
66+
return true;
2667
}
2768

2869
if (isNodeOfType(declaration, "VariableDeclaration")) {
2970
for (const declarator of declaration.declarations ?? []) {
71+
if (isNodeOfType(declarator?.id, "Identifier") && declarator.id.name === "GET") {
72+
return true;
73+
}
74+
}
75+
}
76+
77+
return false;
78+
};
79+
80+
const isGetMethodCall = (callExpression: EsTreeNode): boolean =>
81+
isNodeOfType(callExpression, "CallExpression") &&
82+
isNodeOfType(callExpression.callee, "MemberExpression") &&
83+
isNodeOfType(callExpression.callee.property, "Identifier") &&
84+
callExpression.callee.property.name === "get";
85+
86+
const isStringLikeNode = (node: EsTreeNode): boolean =>
87+
(isNodeOfType(node, "Literal") && typeof node.value === "string") ||
88+
isNodeOfType(node, "TemplateLiteral");
89+
90+
const getHandlerCallbackBody = (
91+
callExpression: EsTreeNodeOfType<"CallExpression">,
92+
): EsTreeNode | null => {
93+
const callArguments = callExpression.arguments ?? [];
94+
if (callArguments.length < 2) return null;
95+
const routePatternArgument = callArguments[0];
96+
if (!isStringLikeNode(routePatternArgument)) return null;
97+
const handlerArgument = callArguments[callArguments.length - 1];
98+
if (
99+
(isNodeOfType(handlerArgument, "ArrowFunctionExpression") ||
100+
isNodeOfType(handlerArgument, "FunctionExpression")) &&
101+
handlerArgument.body
102+
) {
103+
return handlerArgument.body;
104+
}
105+
return null;
106+
};
107+
108+
const collectChainedGetHandlerBodies = (initNode: EsTreeNode): EsTreeNode[] => {
109+
const chainedBodies: EsTreeNode[] = [];
110+
let cursor: EsTreeNode | null | undefined = initNode;
111+
while (cursor && isNodeOfType(cursor, "CallExpression")) {
112+
if (isGetMethodCall(cursor)) {
113+
const body = getHandlerCallbackBody(cursor);
114+
if (body) chainedBodies.push(body);
115+
}
116+
cursor = isNodeOfType(cursor.callee, "MemberExpression") ? cursor.callee.object : null;
117+
}
118+
return chainedBodies;
119+
};
120+
121+
const resolveBodiesFromExpression = (
122+
expression: EsTreeNode,
123+
resolveBinding: (identifierName: string) => EsTreeNode | null,
124+
remainingDepth: number,
125+
): EsTreeNode[] => {
126+
if (remainingDepth <= 0) return [];
127+
128+
if (
129+
isNodeOfType(expression, "ArrowFunctionExpression") ||
130+
isNodeOfType(expression, "FunctionExpression") ||
131+
isNodeOfType(expression, "FunctionDeclaration")
132+
) {
133+
return expression.body ? [expression.body] : [];
134+
}
135+
136+
if (isNodeOfType(expression, "CallExpression")) {
137+
for (const callArgument of expression.arguments ?? []) {
30138
if (
31-
isNodeOfType(declarator?.id, "Identifier") &&
32-
declarator.id.name === "GET" &&
33-
declarator.init &&
34-
(isNodeOfType(declarator.init, "ArrowFunctionExpression") ||
35-
isNodeOfType(declarator.init, "FunctionExpression"))
139+
isNodeOfType(callArgument, "ArrowFunctionExpression") ||
140+
isNodeOfType(callArgument, "FunctionExpression")
36141
) {
37-
return declarator.init.body;
142+
if (callArgument.body) return [callArgument.body];
38143
}
144+
if (!isNodeOfType(callArgument, "Identifier")) continue;
145+
const argumentInit = resolveBinding(callArgument.name);
146+
if (!argumentInit) continue;
147+
const resolvedBodies = resolveBodiesFromExpression(
148+
argumentInit,
149+
resolveBinding,
150+
remainingDepth - 1,
151+
);
152+
if (resolvedBodies.length > 0) return resolvedBodies;
153+
const chainedBodies = collectChainedGetHandlerBodies(argumentInit);
154+
if (chainedBodies.length > 0) return chainedBodies;
39155
}
156+
return [];
40157
}
41158

42-
return null;
159+
if (isNodeOfType(expression, "Identifier")) {
160+
const boundInit = resolveBinding(expression.name);
161+
if (!boundInit) return [];
162+
return resolveBodiesFromExpression(boundInit, resolveBinding, remainingDepth - 1);
163+
}
164+
165+
return [];
166+
};
167+
168+
const resolveGetHandlerBodies = (
169+
exportNode: EsTreeNode,
170+
resolveBinding: (identifierName: string) => EsTreeNode | null,
171+
): EsTreeNode[] => {
172+
if (!isNodeOfType(exportNode, "ExportNamedDeclaration")) return [];
173+
const declaration = exportNode.declaration;
174+
if (!declaration) return [];
175+
176+
if (isNodeOfType(declaration, "FunctionDeclaration") && declaration.id?.name === "GET") {
177+
return declaration.body ? [declaration.body] : [];
178+
}
179+
180+
if (!isNodeOfType(declaration, "VariableDeclaration")) return [];
181+
182+
for (const declarator of declaration.declarations ?? []) {
183+
if (!isNodeOfType(declarator.id, "Identifier") || declarator.id.name !== "GET") continue;
184+
if (!declarator.init) return [];
185+
return resolveBodiesFromExpression(
186+
declarator.init,
187+
resolveBinding,
188+
GET_HANDLER_BINDING_RESOLUTION_DEPTH,
189+
);
190+
}
191+
192+
return [];
43193
};
44194

45195
export const nextjsNoSideEffectInGetHandler = defineRule<Rule>({
@@ -49,30 +199,44 @@ export const nextjsNoSideEffectInGetHandler = defineRule<Rule>({
49199
category: "Security",
50200
recommendation:
51201
"Move the side effect to a POST handler and use a <form> or fetch with method POST — GET requests can be triggered by prefetching and are vulnerable to CSRF",
52-
create: (context: RuleContext) => ({
53-
ExportNamedDeclaration(node: EsTreeNodeOfType<"ExportNamedDeclaration">) {
54-
const filename = context.getFilename?.() ?? "";
55-
if (!ROUTE_HANDLER_FILE_PATTERN.test(filename)) return;
56-
57-
const handlerBody = getExportedGetHandlerBody(node);
58-
if (!handlerBody) return;
59-
60-
const mutatingSegment = extractMutatingRouteSegment(filename);
61-
if (mutatingSegment) {
62-
context.report({
63-
node,
64-
message: `GET handler on "/${mutatingSegment}" route — use POST to prevent CSRF and unintended prefetch triggers`,
65-
});
66-
return;
67-
}
202+
create: (context: RuleContext) => {
203+
let resolveBinding: (identifierName: string) => EsTreeNode | null = () => null;
68204

69-
const sideEffect = findSideEffect(handlerBody);
70-
if (sideEffect) {
71-
context.report({
72-
node,
73-
message: `GET handler has side effects (${sideEffect}) — use POST to prevent CSRF and unintended prefetch triggers`,
74-
});
75-
}
76-
},
77-
}),
205+
return {
206+
Program(node: EsTreeNodeOfType<"Program">) {
207+
resolveBinding = buildProgramBindingLookup(node);
208+
},
209+
ExportNamedDeclaration(node: EsTreeNodeOfType<"ExportNamedDeclaration">) {
210+
const filename = context.getFilename?.() ?? "";
211+
if (!ROUTE_HANDLER_FILE_PATTERN.test(filename)) return;
212+
if (CRON_ROUTE_PATTERN.test(filename)) return;
213+
if (!isExportedGetHandler(node)) return;
214+
215+
const mutatingSegment = extractMutatingRouteSegment(filename);
216+
if (mutatingSegment) {
217+
context.report({
218+
node,
219+
message: `GET handler on "/${mutatingSegment}" route — use POST to prevent CSRF and unintended prefetch triggers`,
220+
});
221+
return;
222+
}
223+
224+
const handlerBodies = resolveGetHandlerBodies(node, resolveBinding);
225+
for (const handlerBody of handlerBodies) {
226+
const locallyScopedSafeBindings = collectLocallyScopedSafeBindings(handlerBody);
227+
const locallyScopedCookieBindings = collectLocallyScopedCookieBindings(handlerBody);
228+
const sideEffect = findSideEffect(handlerBody, {
229+
locallyScopedSafeBindings,
230+
locallyScopedCookieBindings,
231+
});
232+
if (!sideEffect) continue;
233+
context.report({
234+
node,
235+
message: `GET handler has side effects (${sideEffect}) — use POST to prevent CSRF and unintended prefetch triggers`,
236+
});
237+
return;
238+
}
239+
},
240+
};
241+
},
78242
});

packages/oxlint-plugin-react-doctor/src/plugin/rules/tanstack-start/tanstack-start-get-mutation.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
import { MUTATING_HTTP_METHODS } from "../../constants/library.js";
2+
import { collectLocallyScopedCookieBindings } from "../../utils/collect-locally-scoped-cookie-bindings.js";
3+
import { collectLocallyScopedSafeBindings } from "../../utils/collect-locally-scoped-safe-bindings.js";
24
import { defineRule } from "../../utils/define-rule.js";
35
import { findSideEffect } from "../../utils/find-side-effect.js";
46
import type { Rule } from "../../utils/rule.js";
@@ -34,7 +36,12 @@ export const tanstackStartGetMutation = defineRule<Rule>({
3436
const handlerFunction = node.arguments?.[0];
3537
if (!handlerFunction) return;
3638

37-
const sideEffect = findSideEffect(handlerFunction);
39+
const locallyScopedSafeBindings = collectLocallyScopedSafeBindings(handlerFunction);
40+
const locallyScopedCookieBindings = collectLocallyScopedCookieBindings(handlerFunction);
41+
const sideEffect = findSideEffect(handlerFunction, {
42+
locallyScopedSafeBindings,
43+
locallyScopedCookieBindings,
44+
});
3845
if (sideEffect) {
3946
context.report({
4047
node,

0 commit comments

Comments
 (0)