feat(web): [EE] OAuth2 authorization server with MCP support#977
Merged
brendan-kellam merged 15 commits intomainfrom Mar 5, 2026
Merged
feat(web): [EE] OAuth2 authorization server with MCP support#977brendan-kellam merged 15 commits intomainfrom
brendan-kellam merged 15 commits intomainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a full OAuth 2.0 authorization server and integration: DB schema and migrations, token generation/rotation/revocation logic, discovery and protected-resource endpoints, client registration, authorization UI and flows, entitlement gating, MCP error response augmentation, tests, and supporting constants/crypto. Changes
Sequence DiagramssequenceDiagram
participant User as End User
participant Browser as Browser
participant Client as OAuth Client (MCP)
participant AuthServer as Sourcebot Auth UI/Server
participant DB as Database
participant TokenEP as /oauth/token
Browser->>AuthServer: GET /oauth/authorize (client_id, redirect_uri, code_challenge)
AuthServer->>DB: Validate client_id and redirect_uri
AuthServer->>Browser: Render consent (requires session)
User->>AuthServer: Approve
AuthServer->>DB: Store hashed authorization code
AuthServer->>Browser: Redirect back to Client with code
Client->>TokenEP: POST /oauth/token (grant_type=authorization_code, code, code_verifier, redirect_uri)
TokenEP->>DB: Verify & delete code, create access + refresh tokens (transaction)
TokenEP->>Client: Return access_token + refresh_token
sequenceDiagram
participant Client as OAuth Client
participant TokenEP as /oauth/token
participant DB as Database
participant MCP as MCP Server
Client->>TokenEP: POST refresh_token grant (refresh_token)
TokenEP->>DB: Lookup refresh token by hash
TokenEP->>TokenEP: Validate clientId, resource, expiry
alt valid
TokenEP->>DB: Delete old refresh token (rotate)
TokenEP->>DB: Create new access + refresh tokens
TokenEP->>Client: Return new tokens
Client->>MCP: Call MCP with access_token
MCP->>DB: Validate access token
else invalid
TokenEP->>DB: Optionally revoke tokens (theft mitigation)
TokenEP->>Client: Return error (invalid_grant)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
brendan-kellam
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
packages/web/src/app/oauth/complete/page.tsx (1)
10-13:⚠️ Potential issue | 🔴 CriticalBlock untrusted redirect targets and dangerous URL schemes.
On Line 12, the redirect uses a user-controlled query param directly. This enables open redirect and can allow
javascript:-scheme execution/XSS. Validate protocol/target against a strict allowlist (and ideally a server-issued signed value) before navigation.Suggested patch
useEffect(() => { - const url = new URLSearchParams(window.location.search).get('url'); - if (url) { - window.location.href = decodeURIComponent(url); - } + const raw = new URLSearchParams(window.location.search).get('url'); + if (!raw) return; + + try { + const target = new URL(raw); + const allowedProtocols = new Set(['https:', 'http:', 'cursor:', 'claude:']); + if (!allowedProtocols.has(target.protocol)) return; + window.location.assign(target.toString()); + } catch { + // ignore invalid URL + } }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/oauth/complete/page.tsx` around lines 10 - 13, The current redirect reads a user-controlled param via URLSearchParams, decodeURIComponent and assigns it directly to window.location.href, which enables open-redirects and dangerous schemes; replace this with strict validation: parse the value with the URL constructor, reject non-http/https schemes (no javascript:, data:, vbscript:), and enforce an allowlist for hosts (or require same-origin) before setting window.location.href; alternatively accept a server-signed token instead of a raw URL; update the code paths referencing URLSearchParams, decodeURIComponent and window.location.href to perform these checks and fall back to a safe internal route if validation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 12: Update the CHANGELOG entry that currently reads "Oauth 2.1" to use
the correct canonical capitalization "OAuth 2.1" (the line containing "- [EE]
Added support for Oauth 2.1 to the remote MCP server hosted at `/api/mcp`.
[`#977`]"); replace "Oauth" with "OAuth" so the protocol name is consistently
capitalized.
In `@packages/db/prisma/schema.prisma`:
- Around line 496-497: Update the stale documentation comment above the Prisma
model OAuthClient to reference the correct endpoint path: change the comment
text that currently says "POST /api/oauth/register" to "POST
/api/ee/oauth/register" so the model comment matches the exposed endpoint;
locate the comment directly above the model OAuthClient declaration and adjust
only the path text.
In
`@packages/web/src/app/api/`(server)/ee/.well-known/oauth-protected-resource/[...path]/route.ts:
- Around line 7-8: The GET handler in route.ts currently skips entitlement
gating; add an entitlement check at the top of the exported GET function to
enforce the "oauth" entitlement and return a 403 response when the caller lacks
it. Locate the GET function (the route handler for [...path]) and call the
existing entitlement-check helper (or the same mechanism used in /register,
/token, /revoke) to verify the "oauth" entitlement before any metadata logic
runs; if the check fails, short-circuit and return a 403 Forbidden response so
tests and enterprise gating are satisfied.
In `@packages/web/src/app/api/`(server)/ee/oauth/register/route.ts:
- Around line 10-14: The registerRequestSchema is too permissive: change
redirect_uris and logo_uri validations to explicitly allow only safe schemes.
Replace redirect_uris: z.array(z.string().url()).min(1) with a validator that
parses each value as a URL and only accepts protocol "https:" (and optionally
"http:" for localhost/127.0.0.1), e.g. use z.string().refine(...) on each array
element to check new URL(value).protocol is allowed; similarly change logo_uri
from z.string().nullish() to an optional URL validator that only allows "https:"
(or a narrowly-scoped data:image mime whitelist if inline images are required)
using z.string().url().nullish().refine(...) to enforce the scheme. Ensure error
messages mention the allowed schemes and reference redirect_uris and logo_uri in
the schema.
In `@packages/web/src/app/api/`(server)/ee/oauth/token/route.ts:
- Around line 17-44: Replace the manual formData.get + null checks and
.toString() coercions with a Zod schema that validates grant_type ===
"authorization_code" and required string fields code, client_id, redirect_uri,
code_verifier; use schema.safeParse on the object built from await
request.formData(), and if safeParse fails return
requestBodySchemaValidationError(parseResult.error). On success, pass the
validated string values directly into verifyAndExchangeCode (use the exact
symbol names used now: verifyAndExchangeCode and
requestBodySchemaValidationError) and remove the old grantType !==
'authorization_code' and missing-parameter checks and .toString() calls.
In `@packages/web/src/app/api/`(server)/mcp/route.ts:
- Around line 21-27: The 401 handler currently always sets the WWW-Authenticate
header (using response.headers.set and env.AUTH_URL with
StatusCodes.UNAUTHORIZED), which can expose an OAuth flow even when the
caller/plan is not entitled; wrap the header-setting block in a check for the
oauth entitlement (e.g., verify the current request/plan entitlements or feature
flag such as plan.entitlements.oauth or an existing enableOauth() helper) so
that the header is only emitted when OAuth is enabled/entitled for that caller;
keep the same header value using env.AUTH_URL if the entitlement check passes.
In `@packages/web/src/app/oauth/authorize/page.tsx`:
- Around line 63-96: handleAllow and handleDeny capture the render-time session;
re-authenticate inside each server action and wrap them with sew() and the
appropriate auth helper instead of using session!.user.id. Update handleAllow to
call withAuthV2 (or withOptionalAuthV2 if optional) inside the server action to
obtain a fresh user object and pass its id to generateAndStoreAuthCode, and wrap
the whole action body with sew() for error handling; similarly ensure handleDeny
uses the same auth wrapper and sew() before constructing the redirect URL and
calling redirect. Keep generateAndStoreAuthCode and redirect calls unchanged but
use the authenticated user id from the auth helper and surface errors via sew().
In `@packages/web/src/features/oauth/server.ts`:
- Around line 8-10: The access token TTL constants ACCESS_TOKEN_TTL_MS and
ACCESS_TOKEN_TTL_SECONDS are set to 1 year; change them to a much shorter secure
default (e.g., 1 hour) and make the value configurable via an environment
variable (e.g., process.env.ACCESS_TOKEN_TTL_MS) with a validated numeric
fallback to the short default, update any other duplicated constants (the
similar definitions around lines 98-103) to use the same config source, and
ensure token issuance/verification logic in the OAuth code reads the configured
TTL rather than the hardcoded constant.
- Around line 57-90: The current single-use deletion (await
prisma.oAuthAuthorizationCode.delete({ where: { codeHash } })) can throw P2025
under concurrent exchanges; instead perform a deleteMany and check the returned
count to enforce single-use atomically: replace the delete call with something
like const result = await prisma.oAuthAuthorizationCode.deleteMany({ where: {
codeHash } }); then if (result.count === 0) return { error: 'invalid_grant',
errorDescription: 'Authorization code has already been used.' }; update the code
around prisma.oAuthAuthorizationCode.findUnique / codeHash / codeChallenge
verification to use this deleteMany check before issuing tokens so concurrent
requests get a clean invalid_grant rather than an exception.
In `@packages/web/src/withAuthV2.ts`:
- Around line 124-141: In withAuthV2, the code accepts sourcebot-oauth-* tokens
without checking the oauth entitlement; modify the bearerToken handling so the
OAuth path runs only when the runtime/user has the oauth entitlement (e.g. guard
the block that calls hashSecret and __unsafePrisma.oAuthToken.findUnique with an
entitlement check like hasEntitlement('oauth') or a similar existing entitlement
flag), otherwise fall through to the API key handling (getVerifiedApiObject).
Ensure the entitlement check surrounds the entire OAuth lookup/update logic so
issued OAuth tokens cannot authenticate when the oauth entitlement is disabled.
---
Duplicate comments:
In `@packages/web/src/app/oauth/complete/page.tsx`:
- Around line 10-13: The current redirect reads a user-controlled param via
URLSearchParams, decodeURIComponent and assigns it directly to
window.location.href, which enables open-redirects and dangerous schemes;
replace this with strict validation: parse the value with the URL constructor,
reject non-http/https schemes (no javascript:, data:, vbscript:), and enforce an
allowlist for hosts (or require same-origin) before setting
window.location.href; alternatively accept a server-signed token instead of a
raw URL; update the code paths referencing URLSearchParams, decodeURIComponent
and window.location.href to perform these checks and fall back to a safe
internal route if validation fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a47a7b5-6e65-42f2-b5aa-29cac879fdfc
📒 Files selected for processing (23)
CHANGELOG.mdCLAUDE.mdpackages/db/prisma/migrations/20260304051539_add_oauth_tables/migration.sqlpackages/db/prisma/migrations/20260304212808_add_oauth_client_logo_uri/migration.sqlpackages/db/prisma/schema.prismapackages/shared/src/crypto.tspackages/shared/src/entitlements.tspackages/shared/src/index.server.tspackages/web/next.config.mjspackages/web/src/__mocks__/prisma.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/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)/mcp/route.tspackages/web/src/app/oauth/authorize/components/clientIcon.tsxpackages/web/src/app/oauth/authorize/page.tsxpackages/web/src/app/oauth/complete/page.tsxpackages/web/src/features/oauth/server.tspackages/web/src/proxy.tspackages/web/src/withAuthV2.test.tspackages/web/src/withAuthV2.ts
💤 Files with no reviewable changes (1)
- packages/web/src/proxy.ts
packages/web/src/app/api/(server)/ee/.well-known/oauth-protected-resource/[...path]/route.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/web/src/app/oauth/authorize/page.tsx (1)
64-98:⚠️ Potential issue | 🟠 MajorRe-authenticate inside server actions; don’t rely on captured
session.Line 68 uses render-time
session!.user.id.handleAllowandhandleDenyshould authenticate at action execution and wrap action bodies withsew()+withAuthV2/withOptionalAuthV2.As per coding guidelines, "Server actions should wrap logic with
sew()for error handling and usewithAuthV2orwithOptionalAuthV2from@/withAuthV2."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/oauth/authorize/page.tsx` around lines 64 - 98, The server actions handleAllow and handleDeny currently rely on the render-time captured session (session!.user.id); update both to re-authenticate inside the action by wrapping their bodies with sew() and the appropriate auth wrapper (withAuthV2 or withOptionalAuthV2 from '@/withAuthV2') so authentication happens at execution time; inside handleAllow call withAuthV2/withOptionalAuthV2 to obtain the authenticated user id to pass into generateAndStoreAuthCode (instead of session!.user.id), and wrap both action implementations with sew() to handle errors before performing the redirect logic that builds callbackUrl.packages/web/src/features/oauth/server.ts (1)
8-11:⚠️ Potential issue | 🟠 MajorAccess-token default lifetime is too permissive for bearer tokens.
Line 9 sets a 1-year TTL. Please switch to a short secure default and make it configurable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/oauth/server.ts` around lines 8 - 11, The ACCESS_TOKEN_TTL_MS constant currently sets a 1-year default which is too long for bearer tokens; change the default to a much shorter secure value (e.g., 1 hour) and make it configurable: read a numeric TTL (ms) from configuration or environment (e.g., process.env.ACCESS_TOKEN_TTL_MS) with parsing, validation and a safe fallback to the new short default, then compute ACCESS_TOKEN_TTL_SECONDS from that value; update any uses of ACCESS_TOKEN_TTL_MS/ACCESS_TOKEN_TTL_SECONDS to rely on the configurable value and ensure no hard-coded 1-year constant remains (refer to AUTH_CODE_TTL_MS, ACCESS_TOKEN_TTL_MS and ACCESS_TOKEN_TTL_SECONDS to locate the code).
🧹 Nitpick comments (1)
packages/db/prisma/schema.prisma (1)
510-535: Consider indexingexpiresAtfor token/code cleanup efficiency.If you run expiry sweeps or reporting later, lack of an
expiresAtindex can force full-table scans.🛠️ Suggested schema tweak
model OAuthAuthorizationCode { @@ expiresAt DateTime createdAt DateTime `@default`(now()) + + @@index([expiresAt]) } @@ model OAuthToken { @@ expiresAt DateTime createdAt DateTime `@default`(now()) lastUsedAt DateTime? + + @@index([expiresAt]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/prisma/schema.prisma` around lines 510 - 535, Add an index on the expiresAt column for both OAuthAuthorizationCode and OAuthToken to avoid full-table scans during expiry sweeps; modify the Prisma models (OAuthAuthorizationCode and OAuthToken) to declare an index for the expiresAt field (e.g., add a field-level `@index` or a model-level @@index([expiresAt]) on each model), then run prisma migrate to generate the migration so the DB will have the index for efficient cleanup queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/app/oauth/authorize/page.tsx`:
- Around line 34-44: The validation currently permits a missing
code_challenge_method but downstream enforcement requires S256; update the early
checks in authorize/page.tsx so that if code_challenge_method is falsy or not
equal to 'S256' the handler returns an ErrorPage with a precise message;
specifically modify the existing checks around client_id, redirect_uri,
code_challenge, response_type and the code_challenge_method branch (referencing
variables client_id, redirect_uri, code_challenge, response_type,
code_challenge_method and the ErrorPage return) to reject when
code_challenge_method is absent or not 'S256', yielding a clear protocol error
at authorization time.
In `@packages/web/src/features/oauth/server.ts`:
- Around line 71-73: The expiry cleanup should use deleteMany to avoid P2025
when another concurrent request already removed the row: replace the
prisma.oAuthAuthorizationCode.delete({ where: { codeHash } }) call in the
authCode.expiresAt branch with prisma.oAuthAuthorizationCode.deleteMany({ where:
{ codeHash } }) so deletion becomes idempotent and won’t throw if the record is
gone, then continue returning the existing { error: 'invalid_grant',
errorDescription: 'Authorization code has expired.' } response.
In `@packages/web/src/withAuthV2.ts`:
- Around line 135-140: Wrap the __unsafePrisma.oAuthToken.update call inside a
try/catch and handle Prisma "P2025" (record not found) by treating it as an
unauthenticated path instead of rethrowing: when calling
__unsafePrisma.oAuthToken.update({ where: { hash }, data: { lastUsedAt: new
Date() } }) in the oauthToken branch, catch errors and if error.code === 'P2025'
(or instance of PrismaClientKnownRequestError with code 'P2025') simply proceed
as if the token is missing (do not return oauthToken.user or throw), otherwise
rethrow; apply the same pattern to the apiKey.update call in the apiKey branch
so token/key deletion races become unauthenticated rather than 500s.
---
Duplicate comments:
In `@packages/web/src/app/oauth/authorize/page.tsx`:
- Around line 64-98: The server actions handleAllow and handleDeny currently
rely on the render-time captured session (session!.user.id); update both to
re-authenticate inside the action by wrapping their bodies with sew() and the
appropriate auth wrapper (withAuthV2 or withOptionalAuthV2 from '@/withAuthV2')
so authentication happens at execution time; inside handleAllow call
withAuthV2/withOptionalAuthV2 to obtain the authenticated user id to pass into
generateAndStoreAuthCode (instead of session!.user.id), and wrap both action
implementations with sew() to handle errors before performing the redirect logic
that builds callbackUrl.
In `@packages/web/src/features/oauth/server.ts`:
- Around line 8-11: The ACCESS_TOKEN_TTL_MS constant currently sets a 1-year
default which is too long for bearer tokens; change the default to a much
shorter secure value (e.g., 1 hour) and make it configurable: read a numeric TTL
(ms) from configuration or environment (e.g., process.env.ACCESS_TOKEN_TTL_MS)
with parsing, validation and a safe fallback to the new short default, then
compute ACCESS_TOKEN_TTL_SECONDS from that value; update any uses of
ACCESS_TOKEN_TTL_MS/ACCESS_TOKEN_TTL_SECONDS to rely on the configurable value
and ensure no hard-coded 1-year constant remains (refer to AUTH_CODE_TTL_MS,
ACCESS_TOKEN_TTL_MS and ACCESS_TOKEN_TTL_SECONDS to locate the code).
---
Nitpick comments:
In `@packages/db/prisma/schema.prisma`:
- Around line 510-535: Add an index on the expiresAt column for both
OAuthAuthorizationCode and OAuthToken to avoid full-table scans during expiry
sweeps; modify the Prisma models (OAuthAuthorizationCode and OAuthToken) to
declare an index for the expiresAt field (e.g., add a field-level `@index` or a
model-level @@index([expiresAt]) on each model), then run prisma migrate to
generate the migration so the DB will have the index for efficient cleanup
queries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b3f8699-74ac-4497-9765-70aca47bafee
📒 Files selected for processing (9)
packages/db/prisma/migrations/20260304230353_add_oauth_resource_parameter/migration.sqlpackages/db/prisma/schema.prismapackages/web/src/app/api/(server)/ee/.well-known/oauth-protected-resource/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/oauth/authorize/page.tsxpackages/web/src/features/oauth/server.tspackages/web/src/withAuthV2.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/web/src/app/api/(server)/ee/oauth/register/route.ts
- packages/web/src/app/api/(server)/ee/oauth/token/route.ts
- packages/web/src/app/api/(server)/ee/oauth/revoke/route.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
packages/web/src/app/oauth/authorize/page.tsx (2)
64-98:⚠️ Potential issue | 🟠 MajorRe-authenticate inside
handleAllow/handleDenyinstead of trusting capturedsession.Line 68 uses render-time
session!.user.id. These server actions should authenticate at execution time and be wrapped with the standard error/auth helpers.As per coding guidelines, "Server actions should wrap logic with
sew()for error handling and usewithAuthV2orwithOptionalAuthV2from@/withAuthV2."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/oauth/authorize/page.tsx` around lines 64 - 98, handleAllow and handleDeny use the render-time session (session!.user.id) but must authenticate at action execution; update both server actions to call the appropriate auth wrapper (withAuthV2 or withOptionalAuthV2 from "@/withAuthV2") inside the action to obtain the up-to-date user info and replace session!.user.id with the authenticated user id, and wrap the action body with sew() for error handling; ensure generateAndStoreAuthCode is called with the userId from the auth result and preserve the existing redirect logic unchanged.
34-44:⚠️ Potential issue | 🟡 MinorRequire
code_challenge_methodand enforceS256at authorization time.Line 34 still permits missing
code_challenge_method, while Line 42 only rejects non-S256when present. This should fail early with a clear protocol error.Suggested patch
- if (!client_id || !redirect_uri || !code_challenge || !response_type) { + if (!client_id || !redirect_uri || !code_challenge || !code_challenge_method || !response_type) { return <ErrorPage message="Missing required OAuth parameters." />; } @@ - if (code_challenge_method && code_challenge_method !== 'S256') { + if (code_challenge_method !== 'S256') { return <ErrorPage message={`Unsupported code_challenge_method: ${code_challenge_method}. Only "S256" is supported.`} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/oauth/authorize/page.tsx` around lines 34 - 44, The authorization parameter validation currently allows a missing code_challenge_method; update the validation block in authorize/page.tsx to treat code_challenge_method as required and to reject anything other than 'S256'. Specifically, alongside the existing checks for client_id, redirect_uri, code_challenge and response_type, add/modify the check for code_challenge_method so that if it's missing or not equal to 'S256' you return an ErrorPage (use the same ErrorPage component) with a clear protocol error message referencing code_challenge_method and that only "S256" is supported.packages/web/src/app/api/(server)/ee/oauth/revoke/route.ts (1)
17-25:⚠️ Potential issue | 🟠 MajorValidate revoke payload via Zod and return standardized JSON success response.
Please replace manual
formData.get()parsing withsafeParse, returnserviceErrorResponse(requestBodySchemaValidationError(...))on parse failures, and useResponse.json(...)for the 200 path.Suggested structure
+const revokeBodySchema = z.object({ + token: z.string().min(1).optional(), +}); + const formData = await request.formData(); -const token = formData.get('token'); +const parsed = revokeBodySchema.safeParse(Object.fromEntries(formData.entries())); +if (!parsed.success) { + return serviceErrorResponse(requestBodySchemaValidationError(parsed.error)); +} +const { token } = parsed.data; @@ -return new Response(null, { status: 200 }); +return Response.json({}, { status: 200 });As per coding guidelines, "Request body (POST/PUT/PATCH) should use Zod schemas with safeParse and return requestBodySchemaValidationError if validation fails ... and return
Response.json()for successful responses."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/api/`(server)/ee/oauth/revoke/route.ts around lines 17 - 25, Replace manual formData parsing in the revoke route with Zod validation: parse request.formData() into an object and run requestBodySchema.safeParse(...) (reference requestBodySchema and requestBodySchemaValidationError). If safeParse fails, return serviceErrorResponse(requestBodySchemaValidationError(parseResult.error)); if it succeeds, extract the token string from parseResult.data and call revokeToken(token) (reference revokeToken) and return Response.json({ success: true }) for the 200 path instead of new Response(null, ...). Ensure you import/use serviceErrorResponse and requestBodySchemaValidationError where needed.packages/web/src/app/api/(server)/ee/oauth/token/route.ts (1)
17-101:⚠️ Potential issue | 🟠 MajorSwitch token request parsing to Zod
safeParse(both grant branches).Manual
formData.get()checks +.toString()coercions should be replaced with schema-validated parsing and standardized validation errors.Suggested direction
+// Parse once from form data +const body = Object.fromEntries((await request.formData()).entries()); + +// Validate with a grant_type-driven schema (authorization_code | refresh_token) +// and return serviceErrorResponse(requestBodySchemaValidationError(...)) on failure. + -const formData = await request.formData(); -const grantType = formData.get('grant_type'); -...As per coding guidelines, "Request body (POST/PUT/PATCH) should use Zod schemas with safeParse and return requestBodySchemaValidationError if validation fails."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/api/`(server)/ee/oauth/token/route.ts around lines 17 - 101, Replace the manual formData.get() checks and .toString() coercions in the token route with Zod schema validation using safeParse: define separate request schemas (e.g., AuthorizationCodeRequestSchema and RefreshTokenRequestSchema) that require the fields used by verifyAndExchangeCode and verifyAndRotateRefreshToken, call schema.safeParse(parsedForm) in each grant branch, and if parsing fails return requestBodySchemaValidationError with the Zod errors; on success, pass the typed values (no .toString coercions) into verifyAndExchangeCode and verifyAndRotateRefreshToken and keep the existing response shapes.
🧹 Nitpick comments (1)
packages/db/prisma/schema.prisma (1)
541-552: Add an index forclientIdonOAuthTokento support revocation paths.
verifyAndRotateRefreshTokenperforms client-wide token deletes byclientId; without an index this can become expensive at scale.📈 Suggested schema update
model OAuthToken { hash String `@id` // hashSecret(rawToken secret portion) clientId String client OAuthClient `@relation`(fields: [clientId], references: [id], onDelete: Cascade) userId String user User `@relation`(fields: [userId], references: [id], onDelete: Cascade) scope String `@default`("") resource String? // RFC 8707: canonical URI of the target resource server expiresAt DateTime createdAt DateTime `@default`(now()) lastUsedAt DateTime? + + @@index([clientId]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/prisma/schema.prisma` around lines 541 - 552, Add a database index on OAuthToken.clientId to speed up client-wide deletes used by verifyAndRotateRefreshToken: update the OAuthToken model (symbol: OAuthToken) to add an index for the clientId field (symbol: clientId), then run the Prisma migration (or prisma db push) and regenerate the Prisma client so the change is applied and queries/deletes by clientId become efficient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/app/oauth/authorize/page.tsx`:
- Around line 52-54: The current check of
client.redirectUris.includes(redirect_uri) allows arbitrary schemes and later
forwards non-http(s) callbacks to /oauth/complete; tighten validation by parsing
redirect_uri (using the URL constructor or equivalent) and allow only http:,
https:, or an explicit custom-protocol allowlist (e.g., ["cursor:", "claude:"])
before forwarding to /oauth/complete. Update the logic around
client.redirectUris, redirect_uri, and the code paths that build/return the
/oauth/complete handoff (the branches that currently forward non-http(s)
callbacks) to reject or return an error for any scheme not in the allowlist, and
ensure comparisons still confirm redirect_uri is an exact registered URI in
client.redirectUris.
In `@packages/web/src/ee/features/oauth/server.test.ts`:
- Around line 215-229: The test and implementation currently perform client-wide
revocation (calling prisma.oAuthToken.deleteMany and
prisma.oAuthRefreshToken.deleteMany) when a refresh token lookup returns null in
verifyAndRotateRefreshToken; change the logic so that when
prisma.oAuthRefreshToken.findUnique returns null you simply return { error:
'invalid_grant' } without triggering clientId-wide deleteMany calls, or if you
must revoke, restrict deletion to the specific token family/user (e.g., by
tokenFamily or userId) only when that identity is verifiably known; update
verifyAndRotateRefreshToken to guard or remove the
prisma.oAuthToken.deleteMany/prisma.oAuthRefreshToken.deleteMany branch
triggered on a null refresh token and adjust the test to expect no bulk
deletions.
In `@packages/web/src/ee/features/oauth/server.ts`:
- Around line 106-139: Replace the separate delete + create steps with a single
atomic transaction: generate the access/refresh tokens (generateOAuthToken,
generateOAuthRefreshToken) as before, then call prisma.$transaction(async (tx)
=> { await tx.oAuthAuthorizationCode.delete({ where: { codeHash } }); await
tx.oAuthToken.create({ data: { hash, clientId, userId: authCode.userId,
resource: authCode.resource, expiresAt: new Date(Date.now() +
ACCESS_TOKEN_TTL_MS) } }); await tx.oAuthRefreshToken.create({ data: { hash:
refreshHash, clientId, userId: authCode.userId, resource: authCode.resource,
expiresAt: new Date(Date.now() + REFRESH_TOKEN_TTL_MS) } }); }); and wrap the
transaction in the same Prisma.PrismaClientKnownRequestError P2025 check (from
the original delete catch) to return the invalid_grant error if the delete fails
(code already consumed), otherwise rethrow other errors.
- Around line 186-206: The transaction that deletes and recreates tokens can
throw Prisma's P2025 on concurrent refresh attempts; catch errors around the
prisma.$transaction([...]) call, detect Prisma's "P2025" (e.g. by checking
err.code or instanceof Prisma.PrismaClientKnownRequestError) from the
prisma.oAuthRefreshToken.delete step, and convert it to an OAuth-style error
response (invalid_grant) instead of letting the exception bubble as a 500.
Update the refresh handler in this file to wrap the prisma.$transaction call in
a try/catch, map P2025 to the same invalid_grant error path used elsewhere, and
rethrow or propagate other errors unchanged.
- Around line 156-167: rawRefreshToken is sliced without validating
OAUTH_REFRESH_TOKEN_PREFIX and the code currently deletes all tokens for
clientId when the lookup returns no row, enabling DoS; first check
rawRefreshToken.startsWith(OAUTH_REFRESH_TOKEN_PREFIX) and return { error:
'invalid_grant' } immediately if it does not, then compute hash via
hashSecret(rawRefreshToken.slice(OAUTH_REFRESH_TOKEN_PREFIX.length)) and lookup
prisma.oAuthRefreshToken.findUnique; do NOT run the prisma.$transaction
revoke-all block when existing is null — only perform revocation for confirmed
theft cases (e.g., when an existing token is found but other abuse signals are
present) so that a bogus token cannot trigger mass deletion for clientId.
In `@packages/web/src/withAuthV2.test.ts`:
- Around line 198-220: Tests currently rely on getAuthenticatedUser but never
enable entitlements, so the function may return early and tests pass for the
wrong reason; before calling getAuthenticatedUser() in each OAuth-focused test,
enable the entitlement feature used by the module (e.g., call the test helper
that turns on the entitlement flag or set the feature flag/environment variable
your code checks) so the logic exercises the OAuth token lookup and expiration
branches (references: getAuthenticatedUser, prisma.oAuthToken.findUnique,
prisma.apiKey.findUnique).
---
Duplicate comments:
In `@packages/web/src/app/api/`(server)/ee/oauth/revoke/route.ts:
- Around line 17-25: Replace manual formData parsing in the revoke route with
Zod validation: parse request.formData() into an object and run
requestBodySchema.safeParse(...) (reference requestBodySchema and
requestBodySchemaValidationError). If safeParse fails, return
serviceErrorResponse(requestBodySchemaValidationError(parseResult.error)); if it
succeeds, extract the token string from parseResult.data and call
revokeToken(token) (reference revokeToken) and return Response.json({ success:
true }) for the 200 path instead of new Response(null, ...). Ensure you
import/use serviceErrorResponse and requestBodySchemaValidationError where
needed.
In `@packages/web/src/app/api/`(server)/ee/oauth/token/route.ts:
- Around line 17-101: Replace the manual formData.get() checks and .toString()
coercions in the token route with Zod schema validation using safeParse: define
separate request schemas (e.g., AuthorizationCodeRequestSchema and
RefreshTokenRequestSchema) that require the fields used by verifyAndExchangeCode
and verifyAndRotateRefreshToken, call schema.safeParse(parsedForm) in each grant
branch, and if parsing fails return requestBodySchemaValidationError with the
Zod errors; on success, pass the typed values (no .toString coercions) into
verifyAndExchangeCode and verifyAndRotateRefreshToken and keep the existing
response shapes.
In `@packages/web/src/app/oauth/authorize/page.tsx`:
- Around line 64-98: handleAllow and handleDeny use the render-time session
(session!.user.id) but must authenticate at action execution; update both server
actions to call the appropriate auth wrapper (withAuthV2 or withOptionalAuthV2
from "@/withAuthV2") inside the action to obtain the up-to-date user info and
replace session!.user.id with the authenticated user id, and wrap the action
body with sew() for error handling; ensure generateAndStoreAuthCode is called
with the userId from the auth result and preserve the existing redirect logic
unchanged.
- Around line 34-44: The authorization parameter validation currently allows a
missing code_challenge_method; update the validation block in authorize/page.tsx
to treat code_challenge_method as required and to reject anything other than
'S256'. Specifically, alongside the existing checks for client_id, redirect_uri,
code_challenge and response_type, add/modify the check for code_challenge_method
so that if it's missing or not equal to 'S256' you return an ErrorPage (use the
same ErrorPage component) with a clear protocol error message referencing
code_challenge_method and that only "S256" is supported.
---
Nitpick comments:
In `@packages/db/prisma/schema.prisma`:
- Around line 541-552: Add a database index on OAuthToken.clientId to speed up
client-wide deletes used by verifyAndRotateRefreshToken: update the OAuthToken
model (symbol: OAuthToken) to add an index for the clientId field (symbol:
clientId), then run the Prisma migration (or prisma db push) and regenerate the
Prisma client so the change is applied and queries/deletes by clientId become
efficient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f98e3fd4-11bd-4300-a792-5fb18db8e47e
📒 Files selected for processing (14)
packages/db/prisma/migrations/20260304233124_add_oauth_refresh_tokens/migration.sqlpackages/db/prisma/schema.prismapackages/shared/src/constants.tspackages/shared/src/crypto.tspackages/shared/src/index.server.tspackages/web/src/__mocks__/prisma.tspackages/web/src/app/api/(server)/ee/.well-known/oauth-authorization-server/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/oauth/authorize/page.tsxpackages/web/src/ee/features/oauth/server.test.tspackages/web/src/ee/features/oauth/server.tspackages/web/src/withAuthV2.test.tspackages/web/src/withAuthV2.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/web/src/withAuthV2.ts
- packages/web/src/mocks/prisma.ts
- packages/shared/src/index.server.ts
packages/web/src/app/oauth/authorize/components/consentScreen.tsx
Dismissed
Show dismissed
Hide dismissed
packages/web/src/app/oauth/authorize/components/consentScreen.tsx
Dismissed
Show dismissed
Hide dismissed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/web/src/app/oauth/authorize/page.tsx (2)
30-40:⚠️ Potential issue | 🟠 MajorRequire
code_challenge_methodand enforceS256up front.Line 30 currently permits a missing
code_challenge_method; fail early at authorization-time instead of allowing a later flow failure.🔧 Suggested fix
- if (!client_id || !redirect_uri || !code_challenge || !response_type) { + if (!client_id || !redirect_uri || !code_challenge || !code_challenge_method || !response_type) { return <ErrorPage message="Missing required OAuth parameters." />; } - if (code_challenge_method && code_challenge_method !== 'S256') { + if (code_challenge_method !== 'S256') { return <ErrorPage message={`Unsupported code_challenge_method: ${code_challenge_method}. Only "S256" is supported.`} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/oauth/authorize/page.tsx` around lines 30 - 40, The authorization page currently allows a missing code_challenge_method; update the validation in the authorize handler (the block that checks client_id, redirect_uri, code_challenge, response_type and code_challenge_method) to treat code_challenge_method as required and to immediately return an ErrorPage when it's missing or not equal to 'S256'. Specifically, add code_challenge_method to the required-params check (the same conditional that returns "Missing required OAuth parameters.") and keep the existing check that returns an error if code_challenge_method !== 'S256', ensuring no flow continues without an explicit 'S256' value.
48-50:⚠️ Potential issue | 🔴 CriticalBlock unsafe redirect URI schemes before rendering consent.
Line 48 only checks string membership in registered URIs. Add protocol validation (
http:,https:, and explicit native-app allowlist such ascursor:/claude:) to prevent dangerous schemes from entering the authorize flow.🔒 Suggested hardening
+ let parsedRedirectUri: URL; + try { + parsedRedirectUri = new URL(redirect_uri); + } catch { + return <ErrorPage message="Invalid redirect_uri format." />; + } + + const allowedCustomProtocols = new Set(['cursor:', 'claude:']); + const isWebProtocol = parsedRedirectUri.protocol === 'http:' || parsedRedirectUri.protocol === 'https:'; + if (!isWebProtocol && !allowedCustomProtocols.has(parsedRedirectUri.protocol)) { + return <ErrorPage message={`Unsupported redirect_uri protocol: ${parsedRedirectUri.protocol}`} />; + } + if (!client.redirectUris.includes(redirect_uri)) { return <ErrorPage message="redirect_uri does not match the registered redirect URIs for this client." />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/oauth/authorize/page.tsx` around lines 48 - 50, The current check only verifies membership in client.redirectUris; before rendering consent in the authorize flow, parse and validate the redirect_uri scheme and reject unsafe schemes. Update the logic around the redirect URI check (the block that uses client.redirectUris.includes(redirect_uri) and returns ErrorPage) to first safely parse redirect_uri (handle parse errors), extract its protocol, and allow only http:, https: or an explicit native-app allowlist (e.g., cursor:, claude:); if the protocol is not in that allowlist or parsing fails, return an ErrorPage with a clear message; after scheme validation, continue to verify that redirect_uri is among client.redirectUris as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/ee/features/oauth/actions.ts`:
- Around line 23-68: Re-validate clientId and redirectUri inside the server
actions: inside approveAuthorization and denyAuthorization, lookup the OAuth
client (e.g. via the same client-retrieval used elsewhere) and verify the
provided clientId exists and that the redirectUri exactly matches one of the
client's allowed redirect URIs before proceeding (keep using withAuthV2,
generateAndStoreAuthCode and resolveCallbackUrl after validation). Change
denyAuthorization to accept clientId (in signature and calls) and perform the
same client+redirectUri validation as approveAuthorization before returning an
error callback URL. Finally update the consentScreen.tsx call site to pass
clientId into denyAuthorization. Ensure all validation failures return a safe
error (do not perform code generation or redirect to arbitrary URIs).
---
Duplicate comments:
In `@packages/web/src/app/oauth/authorize/page.tsx`:
- Around line 30-40: The authorization page currently allows a missing
code_challenge_method; update the validation in the authorize handler (the block
that checks client_id, redirect_uri, code_challenge, response_type and
code_challenge_method) to treat code_challenge_method as required and to
immediately return an ErrorPage when it's missing or not equal to 'S256'.
Specifically, add code_challenge_method to the required-params check (the same
conditional that returns "Missing required OAuth parameters.") and keep the
existing check that returns an error if code_challenge_method !== 'S256',
ensuring no flow continues without an explicit 'S256' value.
- Around line 48-50: The current check only verifies membership in
client.redirectUris; before rendering consent in the authorize flow, parse and
validate the redirect_uri scheme and reject unsafe schemes. Update the logic
around the redirect URI check (the block that uses
client.redirectUris.includes(redirect_uri) and returns ErrorPage) to first
safely parse redirect_uri (handle parse errors), extract its protocol, and allow
only http:, https: or an explicit native-app allowlist (e.g., cursor:, claude:);
if the protocol is not in that allowlist or parsing fails, return an ErrorPage
with a clear message; after scheme validation, continue to verify that
redirect_uri is among client.redirectUris as currently done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf8a55b8-f010-40ad-b3bf-08f46729dd95
📒 Files selected for processing (5)
packages/web/src/app/api/(server)/ee/oauth/register/route.tspackages/web/src/app/oauth/authorize/components/consentScreen.tsxpackages/web/src/app/oauth/authorize/page.tsxpackages/web/src/ee/features/oauth/actions.tspackages/web/src/lib/posthogEvents.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/app/api/(server)/ee/oauth/register/route.ts
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
oauthentitlement/api/mcplogo_urisupport/api/ee/to reflect enterprise gatingcursor://,claude://) via client-side redirect at/oauth/completeNew endpoints
POST /api/ee/oauth/register— dynamic client registrationGET /api/ee/oauth/authorize— authorization consent pagePOST /api/ee/oauth/token— token exchange (authorization code → access token)POST /api/ee/oauth/revoke— token revocationGET /.well-known/oauth-authorization-server— server metadata (RFC 8414)GET /.well-known/oauth-protected-resource/api/mcp— resource metadata (RFC 9728)POST /api/mcp— MCP Streamable HTTP transport (authenticated)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation