Fix MCP auth ordering and protected-resource discovery#2887
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCentralizes MCP authentication into a new auth-flow module (API key → OAuth/JWT → session fallback), delegates agent routes to it, adds OAuth protected-resource metadata helpers and tests, and updates .well-known routes to fetch OAuth server configuration and return metadata built via the new helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as Agent Route
participant AuthFlow as verifyToken
participant AuthAPI as Auth API
participant OAuth as JWKS/OAuth Server
participant Session as Session Store
Client->>Route: Request to /api/agent/[transport]\nAuthorization: Bearer <token>
Route->>AuthFlow: verifyToken(request, deps)
alt API key (sk_live_...)
AuthFlow->>AuthAPI: verifyApiKey(key)
AuthAPI-->>AuthFlow: API key record
AuthFlow-->>Route: AuthInfo (from API key)
else JWT-like token
AuthFlow->>OAuth: verifyAccessToken(token, jwksUrl)
alt OAuth valid
OAuth-->>AuthFlow: decoded claims
AuthFlow-->>Route: AuthInfo (from token)
else OAuth invalid
AuthFlow->>Session: authApi.getSession(headers)
Session-->>AuthFlow: session (user, activeOrganizationId)
AuthFlow-->>Route: AuthInfo (from session) or undefined
end
else Fallback to session
AuthFlow->>Session: authApi.getSession(headers)
Session-->>AuthFlow: session data
AuthFlow-->>Route: AuthInfo or undefined
end
alt Auth succeeded
Route->>Route: createTransport/createServer
Route->>Route: transport.handleRequest(req, { authInfo })
Route-->>Client: 200 OK
else Auth failed
Route-->>Client: 401 Unauthorized\nWWW-Authenticate: resource_metadata="<origin>/.well-known/oauth-protected-resource/<path>"
end
sequenceDiagram
participant Client
participant MetadataRoute as .well-known/oauth-protected-resource/[...path]
participant Auth as auth.api
participant OAuthConfig as OAuth Server Config
participant Metadata as buildProtectedResourceMetadata
Client->>MetadataRoute: GET /.well-known/oauth-protected-resource/… (request)
MetadataRoute->>MetadataRoute: await params -> resourcePath
MetadataRoute->>Auth: getOAuthServerConfig(request.headers)
Auth->>OAuthConfig: fetch server metadata
OAuthConfig-->>Auth: { issuer, scopes_supported, ... }
Auth-->>MetadataRoute: authServerMetadata
MetadataRoute->>Metadata: buildProtectedResourceMetadata(request, resourcePath, { authorizationServerUrl, resourceName, scopesSupported })
Metadata-->>MetadataRoute: metadata object
MetadataRoute-->>Client: 200 { resource, authorization_servers, scopes_supported?, resource_name? }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/app/api/agent/[transport]/auth-flow.ts">
<violation number="1" location="apps/api/src/app/api/agent/[transport]/auth-flow.ts:49">
P2: Parse the `Authorization` scheme case-insensitively; strict `startsWith("Bearer ")` rejects valid lowercase/mixed-case bearer headers.</violation>
</file>
<file name="apps/api/src/lib/oauth-metadata.ts">
<violation number="1" location="apps/api/src/lib/oauth-metadata.ts:10">
P2: Parse forwarded host/proto header lists before composing the origin. Using raw `x-forwarded-*` values can generate malformed `resource_metadata` URLs behind multi-proxy deployments.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/api/src/lib/oauth-metadata.ts (1)
33-37: Consider using a more specific return type.
Record<string, unknown>loses type information. A discriminated union or explicit interface would improve type safety for consumers.💡 Suggested type
interface ProtectedResourceMetadata { resource: string; authorization_servers?: string[]; scopes_supported?: string[]; resource_name?: string; resource_documentation?: string; } export function buildProtectedResourceMetadata( req: Request, resourcePath: string, options: ProtectedResourceMetadataOptions, ): ProtectedResourceMetadata {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/oauth-metadata.ts` around lines 33 - 37, The function buildProtectedResourceMetadata currently returns a loose Record<string, unknown>; define a concrete interface (e.g. ProtectedResourceMetadata with fields resource, optional authorization_servers, scopes_supported, resource_name, resource_documentation) and change the function signature to return that interface instead of Record<string, unknown>. Add the new ProtectedResourceMetadata interface near ProtectedResourceMetadataOptions and adjust the function implementation to produce that shape, then update any call sites or tests that rely on the previous loose type to use the new properties.apps/api/src/app/.well-known/oauth-protected-resource/route.ts (1)
4-19: Consider handling errors fromgetOAuthServerConfig.If
auth.api.getOAuthServerConfigthrows (e.g., due to misconfiguration or transient failure), the request will fail with a 500 error. Since this is a.well-knowndiscovery endpoint, clients may retry frequently on failure.Consider wrapping in try/catch to return partial metadata (without authorization server info) or a more graceful error response.
💡 Suggested approach
export async function GET(request: Request): Promise<Response> { - const authServerMetadata = await auth.api.getOAuthServerConfig({ - headers: request.headers, - }); + let authServerMetadata: Awaited<ReturnType<typeof auth.api.getOAuthServerConfig>> | null = null; + try { + authServerMetadata = await auth.api.getOAuthServerConfig({ + headers: request.headers, + }); + } catch (error) { + console.error("[oauth-protected-resource] Failed to fetch OAuth server config:", error); + } return Response.json( buildProtectedResourceMetadata(request, "/", { authorizationServerUrl: - typeof authServerMetadata.issuer === "string" - ? authServerMetadata.issuer + typeof authServerMetadata?.issuer === "string" + ? authServerMetadata?.issuer : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/.well-known/oauth-protected-resource/route.ts` around lines 4 - 19, Wrap the call to auth.api.getOAuthServerConfig inside a try/catch within the GET handler so failures don't crash the endpoint; if getOAuthServerConfig throws, catch the error (log it) and call buildProtectedResourceMetadata(request, "/", { resourceName: "Superset MCP Server", authorizationServerUrl: undefined, scopesSupported: undefined }) to return partial discovery metadata instead of letting the request fail, preserving existing behavior when the call succeeds by using auth.api.getOAuthServerConfig().issuer and .scopes_supported as before.apps/api/src/lib/oauth-metadata.test.ts (1)
35-50: Consider adding a test case for required-only metadata fields.The test validates the full options case. Consider adding a test that verifies
buildProtectedResourceMetadatacorrectly omits optional fields when they're not provided:💡 Suggested test case
it("builds protected resource metadata with only required fields", () => { const request = new Request("https://api.superset.sh/anything"); expect( buildProtectedResourceMetadata(request, "/api/agent/mcp", {}), ).toEqual({ resource: "https://api.superset.sh/api/agent/mcp", }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/oauth-metadata.test.ts` around lines 35 - 50, Add a new unit test that calls buildProtectedResourceMetadata with only the required args (e.g., new Request("https://api.superset.sh/anything"), path "/api/agent/mcp", and an empty options object) and assert the result equals { resource: "https://api.superset.sh/api/agent/mcp" } so optional fields (authorization_servers, resource_name, scopes_supported) are omitted; implement this as a new it("builds protected resource metadata with only required fields", ...) test alongside the existing tests to validate required-only behavior.apps/api/src/bun-test.d.ts (1)
1-25: Consider adopting official Bun type definitions like other packages in the monorepo.Other packages (shared, mcp, db, chat, etc.) already use
@types/bunorbun-typesdirectly. Adding@types/bunto apps/api's dependencies would eliminate the need to maintain custom declarations, provide complete type coverage (includingbeforeEach,afterEach,beforeAll,afterAllhooks and additional matchers), and automatically stay synchronized with Bun releases.The current custom types are functional for existing tests and follow good practices (using
unknownoverany), but consolidating with official types would reduce maintenance overhead and align with patterns already established in the repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/bun-test.d.ts` around lines 1 - 25, Replace the local custom declaration for module "bun:test" with the official Bun types: remove or stop exporting the custom declarations (MockResult, Mock, describe/it/mock/expect) from apps/api/src/bun-test.d.ts and add the appropriate package (e.g., `@types/bun` or bun-types) to apps/api dependencies; then update the app's tsconfig or project references to include the new types so the global Bun test hooks and matchers (beforeEach/afterEach/beforeAll/afterAll and extended expect matchers) are available. Ensure any code referencing MockResult/Mock or the custom expect signatures still compiles after switching to the official types and remove the custom file once validation is complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/app/.well-known/oauth-protected-resource/route.ts`:
- Around line 4-19: Wrap the call to auth.api.getOAuthServerConfig inside a
try/catch within the GET handler so failures don't crash the endpoint; if
getOAuthServerConfig throws, catch the error (log it) and call
buildProtectedResourceMetadata(request, "/", { resourceName: "Superset MCP
Server", authorizationServerUrl: undefined, scopesSupported: undefined }) to
return partial discovery metadata instead of letting the request fail,
preserving existing behavior when the call succeeds by using
auth.api.getOAuthServerConfig().issuer and .scopes_supported as before.
In `@apps/api/src/bun-test.d.ts`:
- Around line 1-25: Replace the local custom declaration for module "bun:test"
with the official Bun types: remove or stop exporting the custom declarations
(MockResult, Mock, describe/it/mock/expect) from apps/api/src/bun-test.d.ts and
add the appropriate package (e.g., `@types/bun` or bun-types) to apps/api
dependencies; then update the app's tsconfig or project references to include
the new types so the global Bun test hooks and matchers
(beforeEach/afterEach/beforeAll/afterAll and extended expect matchers) are
available. Ensure any code referencing MockResult/Mock or the custom expect
signatures still compiles after switching to the official types and remove the
custom file once validation is complete.
In `@apps/api/src/lib/oauth-metadata.test.ts`:
- Around line 35-50: Add a new unit test that calls
buildProtectedResourceMetadata with only the required args (e.g., new
Request("https://api.superset.sh/anything"), path "/api/agent/mcp", and an empty
options object) and assert the result equals { resource:
"https://api.superset.sh/api/agent/mcp" } so optional fields
(authorization_servers, resource_name, scopes_supported) are omitted; implement
this as a new it("builds protected resource metadata with only required fields",
...) test alongside the existing tests to validate required-only behavior.
In `@apps/api/src/lib/oauth-metadata.ts`:
- Around line 33-37: The function buildProtectedResourceMetadata currently
returns a loose Record<string, unknown>; define a concrete interface (e.g.
ProtectedResourceMetadata with fields resource, optional authorization_servers,
scopes_supported, resource_name, resource_documentation) and change the function
signature to return that interface instead of Record<string, unknown>. Add the
new ProtectedResourceMetadata interface near ProtectedResourceMetadataOptions
and adjust the function implementation to produce that shape, then update any
call sites or tests that rely on the previous loose type to use the new
properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1858c41-125e-4ddf-a59a-88a7e92efc44
📒 Files selected for processing (8)
apps/api/src/app/.well-known/oauth-protected-resource/[...path]/route.tsapps/api/src/app/.well-known/oauth-protected-resource/route.tsapps/api/src/app/api/agent/[transport]/auth-flow.test.tsapps/api/src/app/api/agent/[transport]/auth-flow.tsapps/api/src/app/api/agent/[transport]/route.tsapps/api/src/bun-test.d.tsapps/api/src/lib/oauth-metadata.test.tsapps/api/src/lib/oauth-metadata.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/api/agent/`[transport]/auth-flow.ts:
- Around line 188-189: The catch blocks in the credential verification flow (the
catch handling API key/bearer verification where you currently call
console.error("[mcp/auth] API key verification failed:", error)) must stop
logging the raw error object; instead log only a sanitized identifier such as
error.name || error.code || 'AuthVerificationError' and an opaque message.
Replace the current console.error calls in those catch blocks (and the similar
one around lines handling token/claim verification) with a call that emits a
fixed message plus the sanitizedName only (no error.message, stack, or the error
object), e.g. build const sanitized = error && (error.name || error.code) ||
'AuthVerificationError' and log that value; do not include the original error or
any tokens/claims in the log output.
- Around line 197-202: The verifyAccessToken call is building jwksUrl and
audience from deps.apiUrl which may contain a trailing slash; normalize
deps.apiUrl (e.g., trim any trailing slash into a local normalizedApiUrl) before
constructing jwksUrl (`${normalizedApiUrl}/api/auth/jwks`) and the audience
array (`[normalizedApiUrl, `${normalizedApiUrl}/`]`) so URLs never end up with
double slashes; make this change where the token verification happens (around
the verifyAccessToken call using bearerToken) or add a small helper to normalize
deps.apiUrl before use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 685404eb-9d5f-4c7c-bb51-4549b59c134e
📒 Files selected for processing (4)
apps/api/src/app/api/agent/[transport]/auth-flow.test.tsapps/api/src/app/api/agent/[transport]/auth-flow.tsapps/api/src/lib/oauth-metadata.test.tsapps/api/src/lib/oauth-metadata.ts
✅ Files skipped from review due to trivial changes (2)
- apps/api/src/lib/oauth-metadata.test.ts
- apps/api/src/lib/oauth-metadata.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/api/agent/[transport]/auth-flow.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/api/agent/`[transport]/auth-flow.ts:
- Around line 228-240: The current flow falls back to session auth after
verifyAccessToken() succeeded if buildOAuthAuthInfo(bearerToken, payload)
returns undefined, allowing a verified-but-invalid OAuth token to be accepted
via cookie; change the logic in auth-flow.ts so that once verifyAccessToken()
succeeds you immediately return the result of buildOAuthAuthInfo (or
throw/return an error) instead of continuing to call deps.authApi.getSession and
buildSessionAuthInfo, and preserve oauthVerificationError for proper error
reporting; also add a regression test that constructs a verified token missing
required fields (e.g., no sub or no organizationId) to ensure the request fails
closed rather than falling back to session auth.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f736e77-2567-40e7-8197-46741e0749e2
📒 Files selected for processing (2)
apps/api/src/app/api/agent/[transport]/auth-flow.test.tsapps/api/src/app/api/agent/[transport]/auth-flow.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/src/app/api/agent/[transport]/auth-flow.test.ts (2)
279-295: Assert the forwardedauthInfo, not just the startup path.This still passes if
handleMcpRequest()callstransport.handleRequest()without the authenticated context. I'd store the request and assert the second argument as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/api/agent/`[transport]/auth-flow.test.ts around lines 279 - 295, The test "starts MCP transport when auth succeeds" should also assert that the authenticated context is forwarded to the transport: capture the arguments passed to deps.transportHandleSpy (the saved call for transport.handleRequest invoked by handleMcpRequest) and add an assertion that the second argument contains the expected authInfo (e.g., user.id "user-4" and session.activeOrganizationId "org-4"); update the test using deps.transportHandleSpy.mock.calls or equivalent to fetch the second argument and assert its auth properties alongside the existing call-count assertions so the transport is proven to receive the authenticated context from handleMcpRequest.
95-125: Cover the object-metadata branch too.
parseApiKeyMetadata()supports both JSON strings and already-parsed objects, but this suite only locks down the string case. A small object-shaped metadata test would keep the second supported input from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/api/agent/`[transport]/auth-flow.test.ts around lines 95 - 125, Add a test that exercises the object-shaped metadata branch of parseApiKeyMetadata by mocking authApi.verifyApiKey to return key.metadata as an already-parsed object (e.g. { organizationId: "org-1" }) instead of a JSON string; use the same pattern as the "accepts valid API keys" test (createDeps, verifyToken, createRequest) and assert that authInfo.extra.mcpContext includes the expected organizationId and userId, ensuring parseApiKeyMetadata handles object inputs correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/app/api/agent/`[transport]/auth-flow.test.ts:
- Around line 279-295: The test "starts MCP transport when auth succeeds" should
also assert that the authenticated context is forwarded to the transport:
capture the arguments passed to deps.transportHandleSpy (the saved call for
transport.handleRequest invoked by handleMcpRequest) and add an assertion that
the second argument contains the expected authInfo (e.g., user.id "user-4" and
session.activeOrganizationId "org-4"); update the test using
deps.transportHandleSpy.mock.calls or equivalent to fetch the second argument
and assert its auth properties alongside the existing call-count assertions so
the transport is proven to receive the authenticated context from
handleMcpRequest.
- Around line 95-125: Add a test that exercises the object-shaped metadata
branch of parseApiKeyMetadata by mocking authApi.verifyApiKey to return
key.metadata as an already-parsed object (e.g. { organizationId: "org-1" })
instead of a JSON string; use the same pattern as the "accepts valid API keys"
test (createDeps, verifyToken, createRequest) and assert that
authInfo.extra.mcpContext includes the expected organizationId and userId,
ensuring parseApiKeyMetadata handles object inputs correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 746435f8-6450-43e4-ab5b-931d25c0c1ea
📒 Files selected for processing (2)
apps/api/src/app/api/agent/[transport]/auth-flow.test.tsapps/api/src/app/api/agent/[transport]/auth-flow.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
resource_metadatachallenge for/api/agent/mcpand serve matching protected-resource metadataReview Notes
During review I found and fixed two regressions before opening this PR:
.well-known[mcp/auth] Access token verification failederrors before session fallbackVerification
bun testinapps/apibun run typecheckinapps/apibunx @biomejs/biome@2.4.2 checkon changed filesbun run --cwd apps/api devsmoke tests:POST /api/agent/mcpwith no auth returns401and a path-specificWWW-AuthenticatechallengePOST /api/agent/mcpwithAuthorization: Bearer sk_live_invalidreturns a fast401and logs the expected Better Auth invalid API key errorPOST /api/agent/mcpwith an opaque bearer token no longer produces a false OAuth verification error/.well-known/oauth-protected-resourceand/.well-known/oauth-protected-resource/api/agent/mcpreturn resource-specific metadataSummary by cubic
Reordered MCP auth to check API keys first, then OAuth (JWKS), with session fallback, and only start the MCP transport on success. Added path‑specific OAuth protected‑resource discovery and
WWW-Authenticatechallenges for/api/agent/mcp, using forwarded host/proto.New Features
auth-flowwith ordered checks: API key (sk_live_), OAuth via JWKS (better-auth/oauth2), then session./.well-known/oauth-protected-resourceusingoauth-metadatahelpers with issuer and scopes from@superset/auth/server.Bug Fixes
Bearerand correctly parsex-forwarded-host/x-forwarded-protolists when building origins and challenges.[mcp/auth] Access token verification failedlogs by skipping OAuth verification for opaque session tokens and falling back correctly.[...path]protected-resource metadata.Written for commit 6697ce9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores