SOU-948: Validate OAuth scopes for MCP access#1396
Conversation
WalkthroughAdds ChangesMCP OAuth Scope Enforcement
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthorizePage
participant ConsentScreen
participant approveAuthorization
participant generateAndStoreAuthCode
participant TokenEndpoint
participant withOptionalAuth
participant MCPRoute
Client->>AuthorizePage: GET /oauth/authorize?scope=mcp
AuthorizePage->>AuthorizePage: resolveGrantedOAuthScopes(scope)
alt invalid scope
AuthorizePage-->>Client: ErrorPage (invalid_scope)
else valid scope
AuthorizePage->>ConsentScreen: requestedScope=scope
ConsentScreen->>approveAuthorization: { requestedScope }
approveAuthorization->>approveAuthorization: resolveGrantedOAuthScopes(requestedScope)
approveAuthorization->>generateAndStoreAuthCode: { scope }
generateAndStoreAuthCode-->>Client: auth code (with scope persisted)
end
Client->>TokenEndpoint: POST /oauth/token (code)
TokenEndpoint->>TokenEndpoint: verifyAndExchangeCode → scope from authCode
TokenEndpoint-->>Client: { access_token, scope: 'mcp', expires_in }
Client->>withOptionalAuth: POST /mcp (Bearer token)
withOptionalAuth->>withOptionalAuth: getAuthContext({ requiredOAuthScopes: ['mcp'] })
alt token missing scope
withOptionalAuth-->>Client: 403 OAUTH_INSUFFICIENT_SCOPE + WWW-Authenticate
else scope valid
withOptionalAuth->>MCPRoute: proceed
MCPRoute-->>Client: MCP response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
…s-token-scope-validation-in-resource-server # Conflicts: # packages/web/src/app/api/(server)/ee/.well-known/oauth-protected-resource/[...path]/route.ts # packages/web/src/app/api/(server)/ee/mcp/route.ts # packages/web/src/app/api/(server)/ee/oauth/token/route.ts # packages/web/src/app/oauth/authorize/components/consentScreen.tsx # packages/web/src/app/oauth/authorize/page.tsx # packages/web/src/ee/features/oauth/actions.ts # packages/web/src/ee/features/oauth/server.ts # packages/web/src/middleware/withAuth.test.ts # packages/web/src/middleware/withAuth.ts
…n-in-resource-server
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/web/src/app/oauth/authorize/page.tsx`:
- Line 20: Normalize the OAuth `scope` query param in `authorize/page.tsx`
before passing it to `resolveGrantedOAuthScopes()`, since Next.js 16 may provide
repeated values as `string[]` and that helper will fail on arrays. Update the
page’s query-param handling to treat `scope` the same way as `resource` and
`dpop_jkt`, and replace the unsafe `new URLSearchParams(params as Record<string,
string>)` usage for the callback URL with explicit string-safe construction so
array values are handled correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6369ba85-ed38-430b-9192-af298b6e071d
📒 Files selected for processing (20)
CHANGELOG.mdpackages/db/prisma/migrations/20260629190000_backfill_sourcebot_mcp_oauth_scope/migration.sqlpackages/db/prisma/schema.prismapackages/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/mcp/route.tspackages/web/src/app/api/(server)/ee/oauth/token/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/ee/features/oauth/constants.test.tspackages/web/src/ee/features/oauth/constants.tspackages/web/src/ee/features/oauth/server.test.tspackages/web/src/ee/features/oauth/server.tspackages/web/src/lib/apiHandler.test.tspackages/web/src/lib/errorCodes.tspackages/web/src/lib/serviceError.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.ts
| code_challenge_method?: string; | ||
| response_type?: string; | ||
| state?: string; | ||
| scope?: string; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json (Next.js version) =="
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8'));
const next = pkg.dependencies?.next || pkg.devDependencies?.next || '(not found)';
console.log(next);
JS
echo
echo "== file outline =="
ast-grep outline packages/web/src/app/oauth/authorize/page.tsx --view expanded || true
echo
echo "== relevant file excerpt =="
sed -n '1,220p' packages/web/src/app/oauth/authorize/page.tsx
echo
echo "== search for searchParams typing / repeated param handling =="
rg -n "searchParams|URLSearchParams|Array\\.isArray\\(|scope" packages/web/src/app/oauth/authorize/page.tsx packages/web/src -g '!**/node_modules/**' || trueRepository: sourcebot-dev/sourcebot
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/web/package.json Next.js version =="
sed -n '1,220p' packages/web/package.json
echo
echo "== authorize page excerpt =="
sed -n '1,140p' packages/web/src/app/oauth/authorize/page.tsx
echo
echo "== comparable searchParams typings in app routes =="
rg -n "searchParams: Promise<|searchParams\\?: Promise<|Record<string, string \\| string\\[\\] \\| undefined>" packages/web/src/app -g '*.ts' -g '*.tsx' | head -n 80Repository: sourcebot-dev/sourcebot
Length of output: 14242
🌐 Web query:
Next.js 16 App Router page searchParams repeated query parameters string[] Promise searchParams type
💡 Result:
In Next.js 16, the searchParams prop in App Router pages is a Promise that must be awaited or unwrapped [1][2][3]. The expected TypeScript type for searchParams is a Promise resolving to an object where values can be a string, an array of strings, or undefined [1][4]: type PageProps = { searchParams: Promise<{ [key: string]: string | string[] | undefined }>; }; To access the parameters, you should await the promise in a Server Component [1][3]: export default async function Page({ searchParams }: PageProps) { const params = await searchParams; const myValue = params.myKey; // can be string, string[], or undefined } In Client Components, you can use React's use hook to unwrap the searchParams promise [1][4]: 'use client'; import { use } from 'react'; export default function ClientPage({ searchParams }: PageProps) { const params = use(searchParams); //... } Handling Repeated Query Parameters: Next.js 16 handles repeated query parameters by automatically mapping them to an array of strings [4][5]. For example, a URL like?tag=a&tag=b will resolve to { tag: ['a', 'b'] } [4]. Note on Recent Router Issues: Users should be aware that versions of Next.js 16 (specifically around 16.2.0–16.2.6) experienced issues with client-side cache collisions when using repeated search parameter keys [5][6][7]. In these cases, the router incorrectly collapsed repeated keys into a single value, causing stale UI [5][7]. If you encounter this, ensure you are updated to the latest patch version of Next.js 16, as multiple fixes were released to address cache key generation [5][6][7]. Additionally, for highly dynamic search pages, opting into dynamic rendering (e.g., via force-dynamic) is recommended to ensure consistent data freshness [8].
Citations:
- 1: https://nextjs.org/docs/app/api-reference/file-conventions/page
- 2: https://dev.to/peterlidee/async-params-and-searchparams-in-next-16-5ge9
- 3: https://coffey.codes/articles/fixing-broken-routes-after-nextjs-16-upgrade
- 4: https://vercel-next-js.mintlify.app/api-reference/file-conventions/page
- 5: Fix client router cache collision for repeated search param keys vercel/next.js#93368
- 6: Next 16.2 router cache issue, if search param name is used multiple times vercel/next.js#92152
- 7: Object.fromEntries(URLSearchParams) drops duplicate keys → stale UI on navigation vercel/next.js#92787
- 8: Next.js 16 searchParams receiving stale cached values after router.push navigation with Cache Components vercel/next.js#88535
Normalize scope before parsing OAuth scopes. Next.js 16 can surface repeated query params as string[], and resolveGrantedOAuthScopes() will throw on an array. Handle scope like resource/dpop_jkt here and avoid the new URLSearchParams(params as Record<string, string>) cast for the callback URL too.
🤖 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/oauth/authorize/page.tsx` at line 20, Normalize the
OAuth `scope` query param in `authorize/page.tsx` before passing it to
`resolveGrantedOAuthScopes()`, since Next.js 16 may provide repeated values as
`string[]` and that helper will fail on arrays. Update the page’s query-param
handling to treat `scope` the same way as `resource` and `dpop_jkt`, and replace
the unsafe `new URLSearchParams(params as Record<string, string>)` usage for the
callback URL with explicit string-safe construction so array values are handled
correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@packages/db/prisma/migrations/20260630005335_add_oauth_scope_to_authorization_code/migration.sql`:
- Line 2: The new scope field for OAuth authorization codes is defaulting to an
empty string, which will propagate the wrong scope into tokens; update the
authorization-code migration to backfill with the canonical MCP scope instead,
and make the same default change in the Prisma schema so
`OAuthAuthorizationCode` consistently uses `mcp` for existing and future rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c8407ea-82ef-47b6-a4be-2bea5dd475d4
📒 Files selected for processing (11)
packages/db/prisma/migrations/20260630005335_add_oauth_scope_to_authorization_code/migration.sqlpackages/db/prisma/schema.prismapackages/web/src/app/oauth/authorize/components/consentScreen.tsxpackages/web/src/app/oauth/authorize/page.tsxpackages/web/src/ee/features/oauth/actions.tspackages/web/src/ee/features/oauth/constants.tspackages/web/src/ee/features/oauth/server.test.tspackages/web/src/ee/features/oauth/server.tspackages/web/src/ee/features/oauth/utils.test.tspackages/web/src/ee/features/oauth/utils.tspackages/web/src/middleware/withAuth.ts
✅ Files skipped from review due to trivial changes (1)
- packages/web/src/ee/features/oauth/constants.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/web/src/ee/features/oauth/actions.ts
- packages/web/src/app/oauth/authorize/page.tsx
- packages/web/src/app/oauth/authorize/components/consentScreen.tsx
- packages/web/src/middleware/withAuth.ts
| @@ -0,0 +1,2 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "OAuthAuthorizationCode" ADD COLUMN "scope" TEXT NOT NULL DEFAULT ''; | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Backfill authorization-code scopes to mcp, not empty.
DEFAULT '' gives existing/in-flight auth codes an empty scope; token exchange then copies that empty value into new access/refresh tokens, which won’t satisfy MCP’s required mcp scope. Use the canonical MCP scope here and mirror it in the Prisma schema default.
Proposed migration fix
-ALTER TABLE "OAuthAuthorizationCode" ADD COLUMN "scope" TEXT NOT NULL DEFAULT '';
+ALTER TABLE "OAuthAuthorizationCode" ADD COLUMN "scope" TEXT NOT NULL DEFAULT 'mcp';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ALTER TABLE "OAuthAuthorizationCode" ADD COLUMN "scope" TEXT NOT NULL DEFAULT ''; | |
| ALTER TABLE "OAuthAuthorizationCode" ADD COLUMN "scope" TEXT NOT NULL DEFAULT 'mcp'; |
🤖 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/db/prisma/migrations/20260630005335_add_oauth_scope_to_authorization_code/migration.sql`
at line 2, The new scope field for OAuth authorization codes is defaulting to an
empty string, which will propagate the wrong scope into tokens; update the
authorization-code migration to backfill with the canonical MCP scope instead,
and make the same default change in the Prisma schema so
`OAuthAuthorizationCode` consistently uses `mcp` for existing and future rows.
1 participant
Summary
mcpscope/api/mcpto include the required scope while leaving sessions, API keys, and anonymous access unchangedLinear
SOU-948
Tests
yarn workspace @sourcebot/web test src/ee/features/oauth/constants.test.ts src/ee/features/oauth/server.test.ts src/middleware/withAuth.test.tsyarn workspace @sourcebot/web lint(passes with existing unrelated warnings)yarn with-env yarn workspace @sourcebot/db prisma validateNote: full web build was started earlier but interrupted before completion.
Summary by CodeRabbit
New Features
Bug Fixes