chore(web): add ESLint rule require-auth-wrapper#1199
Merged
Conversation
Adds an authz/require-auth-wrapper rule that flags exported route handlers and server actions whose body doesn't textually reference withAuth() or withOptionalAuth(). The check is boundary-only by design — false positives are allowlisted with a // eslint-disable-next-line comment plus a reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis PR introduces a new ESLint rule, ChangesAuthorization Wrapper ESLint Rule & Exemptions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/app/api/(server)/ee/oauth/register/route.ts (1)
17-68:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffAdd rate limiting to the unauthenticated OAuth client registration endpoint.
The
/api/ee/oauth/registerendpoint allows unauthenticated client creation per RFC 7591, but lacks rate limiting or abuse protections. Without throttling, the endpoint is vulnerable to DoS attacks (excessive requests) and database bloat (rapid client creation). Implement rate limiting using IP address, trusted client origin, or both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/api/`(server)/ee/oauth/register/route.ts around lines 17 - 68, Add request throttling to the unauthenticated POST handler exported as POST (wrapped by oauthApiHandler) to prevent mass client creation; before parsing the body and creating a record via __unsafePrisma.oAuthClient.create, check a rate-limit store keyed by requester IP (and optionally by Origin header for trusted origins) and reject when limits are exceeded with a 429 JSON response. Implement or call a shared rateLimit helper (token-bucket or fixed-window) that exposes e.g. allowRequest(ipOrKey) and remaining/reset metadata, enforce a low quota for anonymous registration attempts, and short-circuit the handler to return Response.json({ error: 'rate_limit_exceeded', error_description: 'Too many registration attempts' }, { status: 429 }) when allowRequest returns false; ensure the check runs before DB access and that legitimate trusted origins can be whitelisted by consulting Origin header or a config list.
🧹 Nitpick comments (1)
packages/web/src/actions.ts (1)
856-862: ⚡ Quick winRedundant condition can be removed.
The
if (!existingRequest)check on line 856 is always true at this point because ifexistingRequestwere truthy, the function would have returned early on lines 848–853. You can safely remove this conditional wrapper and unindent the block.♻️ Simplify by removing redundant check
- if (!existingRequest) { - await __unsafePrisma.accountRequest.create({ - data: { - requestedById: user.id, - orgId: org.id, - }, - }); + await __unsafePrisma.accountRequest.create({ + data: { + requestedById: user.id, + orgId: org.id, + }, + });Then unindent lines 863–915 by one level.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/actions.ts` around lines 856 - 862, The check "if (!existingRequest)" is redundant because the function already returns when existingRequest is truthy (lines with that early return), so remove the conditional wrapper and unindent the block that calls __unsafePrisma.accountRequest.create({ data: { requestedById: user.id, orgId: org.id } }); (and the subsequent lines that were nested under that if) so the create call runs directly at the same scope as the earlier return; update code around the existingRequest reference accordingly to keep logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/web/src/app/api/`(server)/ee/oauth/register/route.ts:
- Around line 17-68: Add request throttling to the unauthenticated POST handler
exported as POST (wrapped by oauthApiHandler) to prevent mass client creation;
before parsing the body and creating a record via
__unsafePrisma.oAuthClient.create, check a rate-limit store keyed by requester
IP (and optionally by Origin header for trusted origins) and reject when limits
are exceeded with a 429 JSON response. Implement or call a shared rateLimit
helper (token-bucket or fixed-window) that exposes e.g. allowRequest(ipOrKey)
and remaining/reset metadata, enforce a low quota for anonymous registration
attempts, and short-circuit the handler to return Response.json({ error:
'rate_limit_exceeded', error_description: 'Too many registration attempts' }, {
status: 429 }) when allowRequest returns false; ensure the check runs before DB
access and that legitimate trusted origins can be whitelisted by consulting
Origin header or a config list.
---
Nitpick comments:
In `@packages/web/src/actions.ts`:
- Around line 856-862: The check "if (!existingRequest)" is redundant because
the function already returns when existingRequest is truthy (lines with that
early return), so remove the conditional wrapper and unindent the block that
calls __unsafePrisma.accountRequest.create({ data: { requestedById: user.id,
orgId: org.id } }); (and the subsequent lines that were nested under that if) so
the create call runs directly at the same scope as the earlier return; update
code around the existingRequest reference accordingly to keep logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7a255d7-b94e-4879-a748-d173804d5c71
📒 Files selected for processing (41)
CHANGELOG.mdpackages/web/eslint.config.mjspackages/web/src/actions.tspackages/web/src/app/api/(server)/[...slug]/route.tspackages/web/src/app/api/(server)/auth/[...nextauth]/route.tspackages/web/src/app/api/(server)/blame/route.tspackages/web/src/app/api/(server)/chat/blocking/route.tspackages/web/src/app/api/(server)/commit/route.tspackages/web/src/app/api/(server)/commits/authors/route.tspackages/web/src/app/api/(server)/commits/route.tspackages/web/src/app/api/(server)/diff/route.tspackages/web/src/app/api/(server)/ee/.well-known/oauth-authorization-server/route.tspackages/web/src/app/api/(server)/ee/.well-known/oauth-protected-resource/[...path]/route.tspackages/web/src/app/api/(server)/ee/accountPermissionSyncJobStatus/route.tspackages/web/src/app/api/(server)/ee/audit/route.tspackages/web/src/app/api/(server)/ee/oauth/register/route.tspackages/web/src/app/api/(server)/ee/oauth/revoke/route.tspackages/web/src/app/api/(server)/ee/oauth/token/route.tspackages/web/src/app/api/(server)/ee/permissionSyncStatus/route.tspackages/web/src/app/api/(server)/files/route.tspackages/web/src/app/api/(server)/find_definitions/route.tspackages/web/src/app/api/(server)/find_references/route.tspackages/web/src/app/api/(server)/health/route.tspackages/web/src/app/api/(server)/mcp/route.tspackages/web/src/app/api/(server)/openapi.json/route.tspackages/web/src/app/api/(server)/repo-status/[repoId]/route.tspackages/web/src/app/api/(server)/repos/route.tspackages/web/src/app/api/(server)/search/route.tspackages/web/src/app/api/(server)/source/route.tspackages/web/src/app/api/(server)/stream_search/route.tspackages/web/src/app/api/(server)/tree/route.tspackages/web/src/app/api/(server)/version/route.tspackages/web/src/app/api/(server)/webhook/route.tspackages/web/src/app/api/minidenticon/route.tspackages/web/src/app/api/repos/[repoId]/image/route.tspackages/web/src/app/invite/actions.tspackages/web/src/ee/features/sso/actions.tspackages/web/src/features/chat/actions.tspackages/web/tools/eslint-plugin-local/index.mjspackages/web/tools/eslint-plugin-local/rules/requireAuthWrapper.mjspackages/web/tools/eslint-plugin-local/rules/requireAuthWrapper.test.mjs
Removed mention of the `authz/require-auth-wrapper` ESLint rule from the changelog.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an
authz/require-auth-wrapperESLint rule that fires on exported API route handlers (underpackages/web/src/app/api/**/route.ts) and server-action exports (files with a'use server'directive) whose body doesn't textually referencewithAuth(orwithOptionalAuth(. Goal: catch endpoints that ship without an auth wrapper before they merge.withMinimumOrgRolealone does not satisfy — it always nests insidewithAuth, so missingwithAuthmakes the role gate meaningless.// eslint-disable-next-line authz/require-auth-wrapper -- <reason>.Test plan
yarn workspace @sourcebot/web lintruns cleanly against the rule (no new errors beyond pre-existing unrelated ones)yarn workspace @sourcebot/web vitest run tools/eslint-plugin-local— rule unit tests pass; covers withAuth, withOptionalAuth, neither, withMinimumOrgRole-only, allowlist comment, action with/without wrapper, non-route/non-action file, destructuredexport const { GET, POST } = ..., and re-export specifiers (export { handler as GET })export const GET = async () => Response.json({ leak: true });to a route file and confirm lint fails🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores