fix(nextjs): stop flagging response.headers and local Map/Set/Headers in GET handlers (#206)#260
Conversation
|
🔴 React Review — 0/100 (unchanged) · Copy prompt for agentReviewed by react-review for commit 4428e7f. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
||
| // Verbatim repro from issue #206 — used to flood codebases with false | ||
| // positives. Response header shaping must not fire the rule. | ||
| export async function GET(req: NextRequest, ctx: RouteContext) { |
There was a problem hiding this comment.
Caution
GET handler has side effects (.set()) — use POST to prevent CSRF and unintended prefetch triggers
Move the side effect to a POST handler and use a
or fetch with method POST — GET requests can be triggered by prefetching and are vulnerable to CSRFRule: nextjs-no-side-effect-in-get-handler
Disable nextjs-no-side-effect-in-get-handler for this line
| export async function GET(req: NextRequest, ctx: RouteContext) { | |
| // react-doctor-disable-next-line nextjs-no-side-effect-in-get-handler | |
| export async function GET(req: NextRequest, ctx: RouteContext) { |
Copy prompt for agent
Check if this React Review issue is valid. If so, understand the root cause and fix it.
Run this before and after your changes to verify the result:
npx react-doctor@latest --verbose --diff
Do not modify the react-doctor configuration unless explicitly asked.
Fix the underlying code issue instead of changing or suppressing the rule.
<file name="packages/react-doctor/tests/fixtures/nextjs-app/src/app/api/documents/[id]/route.tsx">
<violation number="1" location="packages/react-doctor/tests/fixtures/nextjs-app/src/app/api/documents/[id]/route.tsx:13">
Severity: Error
GET handler has side effects (.set()) — use POST to prevent CSRF and unintended prefetch triggers
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
Rule: `nextjs-no-side-effect-in-get-handler`
</violation>
</file>
Reviewed by react-review for commit ce14ae7. Configure here.
|
|
||
| // Vercel Cron always invokes GET — real side effects are expected | ||
| // and the rule must not fire here. | ||
| export async function GET() { |
There was a problem hiding this comment.
Caution
GET handler has side effects (db.insert()) — use POST to prevent CSRF and unintended prefetch triggers
Move the side effect to a POST handler and use a
or fetch with method POST — GET requests can be triggered by prefetching and are vulnerable to CSRFRule: nextjs-no-side-effect-in-get-handler
Disable nextjs-no-side-effect-in-get-handler for this line
| export async function GET() { | |
| // react-doctor-disable-next-line nextjs-no-side-effect-in-get-handler | |
| export async function GET() { |
Copy prompt for agent
Check if this React Review issue is valid. If so, understand the root cause and fix it.
Run this before and after your changes to verify the result:
npx react-doctor@latest --verbose --diff
Do not modify the react-doctor configuration unless explicitly asked.
Fix the underlying code issue instead of changing or suppressing the rule.
<file name="packages/react-doctor/tests/fixtures/nextjs-app/src/app/api/cron/refresh/route.tsx">
<violation number="1" location="packages/react-doctor/tests/fixtures/nextjs-app/src/app/api/cron/refresh/route.tsx:9">
Severity: Error
GET handler has side effects (db.insert()) — use POST to prevent CSRF and unintended prefetch triggers
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
Rule: `nextjs-no-side-effect-in-get-handler`
</violation>
</file>
Reviewed by react-review for commit ce14ae7. Configure here.
| // side effect (server state leaks across requests). | ||
| const cache = new Map<string, unknown>(); | ||
|
|
||
| export async function GET() { |
There was a problem hiding this comment.
Caution
GET handler has side effects (cache.set()) — use POST to prevent CSRF and unintended prefetch triggers
Move the side effect to a POST handler and use a
or fetch with method POST — GET requests can be triggered by prefetching and are vulnerable to CSRFRule: nextjs-no-side-effect-in-get-handler
Disable nextjs-no-side-effect-in-get-handler for this line
| export async function GET() { | |
| // react-doctor-disable-next-line nextjs-no-side-effect-in-get-handler | |
| export async function GET() { |
Copy prompt for agent
Check if this React Review issue is valid. If so, understand the root cause and fix it.
Run this before and after your changes to verify the result:
npx react-doctor@latest --verbose --diff
Do not modify the react-doctor configuration unless explicitly asked.
Fix the underlying code issue instead of changing or suppressing the rule.
<file name="packages/react-doctor/tests/fixtures/nextjs-app/src/app/api/admin/route.tsx">
<violation number="1" location="packages/react-doctor/tests/fixtures/nextjs-app/src/app/api/admin/route.tsx:15">
Severity: Error
GET handler has side effects (cache.set()) — use POST to prevent CSRF and unintended prefetch triggers
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
Rule: `nextjs-no-side-effect-in-get-handler`
</violation>
</file>
Reviewed by react-review for commit ce14ae7. Configure here.
| (issue #206). The rule used to flag any `<member>.<set|append|delete|create| | ||
| insert|update|upsert|remove|destroy>()` call as a server-state mutation, | ||
| which flooded one Next.js 14 codebase with 138 false errors — every single | ||
| one a `response.headers.set(...)` response-shaping call. |
|
CI |
… 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>
The `isHeadersPropertyAccess` predicate was added during the issue #206 work but never imported — its check (`.headers` member access) is fully covered by `isSafeIntrinsicMemberAccess` in `is-safe-mutable-receiver- source.ts` (which also handles `.searchParams`). Removing the dead file to comply with the workspace "remove unused code and don't repeat yourself" rule. Co-authored-by: Cursor <cursoragent@cursor.com>
…ies-call utils `isCookiesCall` in `collect-locally-scoped-cookie-bindings.ts` and `isDirectCookiesCall` in `find-side-effect.ts` were byte-identical, and `isCookiesInit` was structurally the same as `(direct || awaited)` from `find-side-effect.ts`. Extract to single-purpose utils (one per file, per workspace convention) and rewire both consumers so the cookies-detection predicate has one definition. Co-authored-by: Cursor <cursoragent@cursor.com>
…ollectors `collectLocallyScopedSafeBindings` / `collectLocallyScopedCookieBindings` use `walkInsideStatementBlocks`, which returns immediately at any function boundary. `tanstack-start-get-mutation` was passing the `.handler(fn)` callback NODE itself, so both binding sets came back empty — every aliased shape inside a TanStack Start handler was misclassified: - `const customHeaders = new Headers(); customHeaders.set(...)` leaked a false positive. - `const cs = cookies(); cs.set(...)` got reported as `cs.set()` instead of the canonical `cookies().set()`. Unwrap to `handlerFunction.body` (mirroring how the Next.js rule passes `handlerBody` after `resolveGetHandlerBodies`) and add four regression cases that pin this down. `findSideEffect` still gets the body too, which is a no-op for it (walkAst descends through functions) but keeps the inputs consistent across both consumers. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
bd9778a to
71ecf0a
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 71ecf0a. Configure here.
Resolve add/add conflict in `packages/oxlint-plugin-react-doctor/src/plugin/utils/get-react-doctor-setting.ts` by keeping this branch's superset version (extracted `readReactDoctorSettingsBag` / `readOwnPropertyValue` helpers + both `getReactDoctorStringSetting` and `getReactDoctorStringArraySetting`), which preserves the consumer added on `main` (`is-react-native-file.ts` uses `getReactDoctorStringSetting`) while keeping this branch's `server-auth-actions.ts` consumer of `getReactDoctorStringArraySetting` working.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit beadd1e. Configure here.
| !isNodeOfType(handlerFunction, "ArrowFunctionExpression") && | ||
| !isNodeOfType(handlerFunction, "FunctionExpression") | ||
| ) | ||
| return; |
There was a problem hiding this comment.
Tanstack handler skips Identifier-referenced handlers entirely
Low Severity
The new guard early-returns when the handler argument is not an ArrowFunctionExpression or FunctionExpression. Previously, passing an Identifier reference (e.g. createServerFn().handler(myHandler)) would still call findSideEffect on the identifier node (which finds nothing). With the new code, that path returns before calling findSideEffect at all — meaning collectLocallyScopedSafeBindings and collectLocallyScopedCookieBindings also never run. While the old behavior also missed Identifier handlers, the new early return additionally skips the entire rule for any non-inline handler, making it impossible to extend this code path in the future without removing the guard.
Reviewed by Cursor Bugbot for commit beadd1e. Configure here.


Summary
Rewrites
nextjs-no-side-effect-in-get-handlerprecision so it no longer floods Next.js codebases with false positives from response-shaping calls, while still catching real CSRF-relevant server-state mutations.Fixes #206. Supersedes #209, #211, #233, #238. The handler-resolution improvements from #251 (cron route skip + depth-bounded
const GET = withAuth(handler)resolution) are also folded in; the broader rule expansion in #251 remains an independent PR.What changes for users
No longer flagged (false positives — were ~138 errors in one Next.js 14 codebase)
res.headers.set/append/delete(...)and any chain ending in.headers(handlesNextResponse.json({...}).headers.set(...),(await fetcher()).headers.append(...)).new Map/Set/WeakMap/WeakSet/Headers/URLSearchParams/FormData/Response/NextResponse(...)bindings and any mutation on those aliases.new URL(...).searchParams.set(...)and any.searchParams.*()chain.headers()/(await headers())fromnext/headers(returnsReadonlyHeaders; any mutation would throw at runtime) and any aliasedconst h = headers(); h.get(...)./cron/or/jobs/cron/— Vercel Cron always invokes GET and is expected to do real work.Still flagged (real CSRF-relevant side effects)
db.update(table).set({...}).where(...),prisma.user.create(...),db.insert(...),repository.upsert(...).const cache = new Map()declared outside the handler, thencache.set(...)inside.fetch(url, { method: 'POST' | 'PUT' | 'DELETE' | 'PATCH' }).cookies().set/append/delete()in all forms — direct,(await cookies()).set(...), and (new) aliasedconst cs = await cookies(); cs.set(...)(the previously-missed aliased form)./logout,/signout,/unsubscribe,/delete, …).Implementation
packages/oxlint-plugin-react-doctor/src/plugin/utils/find-side-effect.tsnow takes an options bag and short-circuits any mutation call whose receiver chain is structurally "safe" (a.headers/.searchParamsaccess, anew X()of a safe constructor, aResponse/NextResponsefactory, or an Identifier in a per-handler safe-bindings set). The rule itself collects two binding sets before scanning:collectLocallyScopedSafeBindings(handlerBody)—const X = new Map(),const X = new Headers(),const X = NextResponse.json(...),const X = await headers(), etc.collectLocallyScopedCookieBindings(handlerBody)—const X = cookies()andconst X = await cookies(). Any<alias>.set/append/delete()is reported ascookies().<method>()so the diagnostic stays consistent.The rule also gained depth-bounded handler-binding resolution (
GET_HANDLER_BINDING_RESOLUTION_DEPTH = 3) soexport const GET = withAuth(handler)andexport const GET = app.get('/x', handler)chains get scanned correctly.Test plan
packages/react-doctor/tests/regressions/nextjs-get-side-effects.test.ts(new) builds 23 isolated synthetic projects covering:packages/react-doctor/tests/run-oxlint/nextjs.test.tsalso gains three integration assertions on new fixtures undertests/fixtures/nextjs-app/src/app/api/to lock the behavior in the shared Next.js fixture project.Suggested follow-ups
main.Eval results
Re-ran against the larger sandbox corpus (500 repos, scanned via Vercel Sandbox in parallel) on
mainvs this branch:main(8a3a1be)nextjs-no-side-effect-in-get-handlerZero functional changes across 500 repos. The 51 line-level diffs the tool surfaces are 100% message-text encoding artifacts (same file/line/column/rule, just em-dash UTF-8 rendering); no diagnostic actually moves. All 8
nextjs-no-side-effect-in-get-handlerhits are valid true positives in the corpus and remain flagged:supabase/supabase:app/auth/confirm/route.ts:6—supabase.auth.verifyOtp(...)in a GET handler (textbook CSRF risk on email confirm links)unkeyed/unkey:app/auth/sso-callback/[[...sso-callback]]/route.ts:6,app/join/route.ts:9— OAuth/invite acceptance with side effectsbetter-auth/better-auth:app/.well-known/oauth-*/route.ts(4 hits) — discovery routes that mutate session/cacheumami-software/umami:src/app/api/websites/[websiteId]/event-data/route.ts:8— event-data write inside GETThe targeted false positives (
response.headers.set, localnew Map/Set/Headers,headers()reads, aliasedcookies()) and the new true positive (depth-boundedexport const GET = withAuth(handler)resolution) seen on the earlier 14-repo set don't appear in this 500-repo slice — but the corpus-wide result is 0 regressions in any rule across 66,276 diagnostics, plus the 23-case synthetic regression suite covering every pattern in the issue.Made with Cursor
Note
Medium Risk
Medium risk: changes security-focused lint rules and their side-effect/auth-call detection logic, which could introduce false negatives/positives in real projects despite added regression coverage.
Overview
Improves security lint precision for server GET handlers and server actions.
nextjs-no-side-effect-in-get-handleris rewritten to avoid flagging response/header shaping and locally-created mutable objects (e.g.response.headers.*,new Map/Set/Headers,URL.searchParams) while still reporting real state changes (DB mutations, mutatingfetchmethods, cookie writes), and it now skips cron routes and resolves wrapped/aliased GET handlers via bounded binding resolution.Enhances shared side-effect detection and TanStack Start parity.
findSideEffectnow accepts per-handler binding context to treat “safe” receiver chains as non-mutations and to correctly attribute aliasedcookies()mutations; TanStack Start’stanstack-start-get-mutationis updated to analyze the handler body with the same binding-aware logic.Extends
server-auth-actionsrecognition and configurability. The rule now detects auth checks in member calls and TS-wrapped/optional-chained callees, avoids counting nested-helper auth calls, supports additional server action export forms, and allows projects to provideserverAuthFunctionNames, which is plumbed fromreact-doctorconfig through core oxlint settings. Extensive new regression/integration tests and fixtures lock in these behaviors.Reviewed by Cursor Bugbot for commit beadd1e. Bugbot is set up for automated code reviews on this repo. Configure here.