fix(mcp): accept MCP resource URL as valid OAuth audience#3459
Conversation
MCP clients on spec 2025-06-18 send resource=<mcp-url> in authorize/token requests. better-auth 1.5.6 validates that against validAudiences and our allowlist only contained the bare API origin, so tokens were rejected with "requested resource invalid". Add the MCP endpoint to validAudiences and to the JWT verifier's audience list.
📝 WalkthroughWalkthroughJWT token audience validation is expanded to accept a new audience value ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 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 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 |
Greptile SummaryThis PR fixes a broken OAuth sign-in path for MCP clients that conform to MCP spec 2025-06-18. Those clients include a The fix is a symmetric two-line addition:
Both changes are minimal and correct. Concerns:
Confidence Score: 4/5Safe to merge — the two-line production fix is correct and minimal; the main concern is the deletion of meaningful test coverage rather than any correctness issue. The actual bug fix is a straightforward, symmetrical addition to validAudiences and the JWT audience option with no logic errors. The score is 4 rather than 5 because the PR removes an assertion test that directly validates the audience array (the exact thing this PR changes), and separately deletes five utility tests that required zero updates — both of which are regressive. Restoring or updating those tests would make this a clean 5. The deleted test files (auth-flow.test.ts and oauth-metadata.test.ts) deserve attention — particularly the audience-assertion test in auth-flow.test.ts that should be updated rather than removed. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MCP Client Request] --> B{Has Bearer Token?}
B -- No --> C[Return 401 with resource_metadata]
C --> D[Client starts OAuth flow]
D --> E[Authorization request with resource param]
E --> F{validAudiences check in oauthProvider}
F -- Invalid --> G[Error: requested resource invalid]
F -- Valid after fix --> H[Authorization code issued]
H --> I[Token request with resource param]
I --> J[JWT minted with aud = resource URL]
J --> K[Client retries with Bearer JWT]
K --> B
B -- Yes --> L{Is API key prefix?}
L -- Yes --> M[Verify API key]
L -- No --> N{Looks like JWT?}
N -- Yes --> O{verifyAccessToken audience check}
O -- Match after fix --> P[OAuth AuthInfo built]
O -- No match --> Q[Try session fallback]
N -- No --> Q
P --> R[MCP transport handles request]
Q --> R
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| verifyOptions: { | ||
| issuer: apiUrl, | ||
| audience: [apiUrl, `${apiUrl}/`], | ||
| audience: [apiUrl, `${apiUrl}/`, `${apiUrl}/api/agent/mcp`], |
There was a problem hiding this comment.
The literal /api/agent/mcp is now spelled out in both apps/api/src/app/api/agent/[transport]/auth-flow.ts (here) and packages/auth/src/server.ts. If the route path ever changes, or a second MCP transport is introduced, both files need updating independently.
Consider extracting the path into a shared constant in packages/shared (or similar) and importing it in both locations:
// packages/shared/src/mcp.ts
export const MCP_PATH = "/api/agent/mcp";// auth-flow.ts
import { MCP_PATH } from "@superset/shared/mcp";
audience: [apiUrl, `${apiUrl}/`, `${apiUrl}${MCP_PATH}`],
// server.ts
import { MCP_PATH } from "@superset/shared/mcp";
validAudiences: [
env.NEXT_PUBLIC_API_URL,
`${env.NEXT_PUBLIC_API_URL}/`,
`${env.NEXT_PUBLIC_API_URL}${MCP_PATH}`,
],![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
🧹 Nitpick comments (1)
apps/api/src/app/api/agent/[transport]/auth-flow.ts (1)
224-224: Consider centralizing OAuth audience values to prevent config drift.Line 224 duplicates audience values already configured in
packages/auth/src/server.ts(Line 205-Line 209). A shared constant/helper for allowed audiences would reduce future auth breakage from one-sided edits.🤖 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.ts at line 224, The inline audience array in auth-flow.ts (the audience: [apiUrl, `${apiUrl}/`, `${apiUrl}/api/agent/mcp`]) duplicates values defined in packages/auth/src/server.ts; create a single exported constant (e.g., ALLOWED_OAUTH_AUDIENCES) in packages/auth (or a shared config module) that lists the allowed audiences, export it from there, and replace the inline array in auth-flow.ts with an import of that constant so both auth-server and the auth-flow use the same source of truth.
🤖 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/auth/src/server.ts`:
- Around line 205-209: The validAudiences array construction can produce a
double-slash when env.NEXT_PUBLIC_API_URL ends with '/', causing mismatched
audience strings; update the code that builds validAudiences (the validAudiences
array in this file) to normalize NEXT_PUBLIC_API_URL by trimming any trailing
slash before creating entries like `${base}/api/agent/mcp` and `${base}/` (or
construct audience URLs via a safe URL join/helper) so values never contain
`//api/...`.
---
Nitpick comments:
In `@apps/api/src/app/api/agent/`[transport]/auth-flow.ts:
- Line 224: The inline audience array in auth-flow.ts (the audience: [apiUrl,
`${apiUrl}/`, `${apiUrl}/api/agent/mcp`]) duplicates values defined in
packages/auth/src/server.ts; create a single exported constant (e.g.,
ALLOWED_OAUTH_AUDIENCES) in packages/auth (or a shared config module) that lists
the allowed audiences, export it from there, and replace the inline array in
auth-flow.ts with an import of that constant so both auth-server and the
auth-flow use the same source of truth.
🪄 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: 66c829ae-2702-42e8-9c29-d32e8b610de0
📒 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.tspackages/auth/src/server.ts
💤 Files with no reviewable changes (2)
- apps/api/src/lib/oauth-metadata.test.ts
- apps/api/src/app/api/agent/[transport]/auth-flow.test.ts
| validAudiences: [ | ||
| env.NEXT_PUBLIC_API_URL, | ||
| `${env.NEXT_PUBLIC_API_URL}/`, | ||
| `${env.NEXT_PUBLIC_API_URL}/api/agent/mcp`, | ||
| ], |
There was a problem hiding this comment.
Normalize MCP audience construction to avoid //api/agent/mcp mismatches.
At Line 208, the audience is built from raw env.NEXT_PUBLIC_API_URL; if that env var ends with /, the result becomes ...//api/agent/mcp, which can break resource/audience matching.
Proposed fix
validAudiences: [
env.NEXT_PUBLIC_API_URL,
`${env.NEXT_PUBLIC_API_URL}/`,
- `${env.NEXT_PUBLIC_API_URL}/api/agent/mcp`,
+ `${env.NEXT_PUBLIC_API_URL.replace(/\/+$/, "")}/api/agent/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.
| validAudiences: [ | |
| env.NEXT_PUBLIC_API_URL, | |
| `${env.NEXT_PUBLIC_API_URL}/`, | |
| `${env.NEXT_PUBLIC_API_URL}/api/agent/mcp`, | |
| ], | |
| validAudiences: [ | |
| env.NEXT_PUBLIC_API_URL, | |
| `${env.NEXT_PUBLIC_API_URL}/`, | |
| `${env.NEXT_PUBLIC_API_URL.replace(/\/+$/, "")}/api/agent/mcp`, | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth/src/server.ts` around lines 205 - 209, The validAudiences array
construction can produce a double-slash when env.NEXT_PUBLIC_API_URL ends with
'/', causing mismatched audience strings; update the code that builds
validAudiences (the validAudiences array in this file) to normalize
NEXT_PUBLIC_API_URL by trimming any trailing slash before creating entries like
`${base}/api/agent/mcp` and `${base}/` (or construct audience URLs via a safe
URL join/helper) so values never contain `//api/...`.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…h#3459) MCP clients on spec 2025-06-18 send resource=<mcp-url> in authorize/token requests. better-auth 1.5.6 validates that against validAudiences and our allowlist only contained the bare API origin, so tokens were rejected with "requested resource invalid". Add the MCP endpoint to validAudiences and to the JWT verifier's audience list.
chore(upstream): PR2 MCP OAuth audience + tray menu refactor (superset-sh#3459 superset-sh#3458)
-s ours merge to record that upstream commits a3e34bf through de70163 (13 commits) are semantically already present on origin/main via the PR1-6 cherry-pick series (PRs #176, #177, #178, #179, #180, #182), plus fork-adaptation fixes layered on top. This merge target is de70163 specifically (not upstream/main) so newer upstream commits (9fff075 and later) remain visible in future behind counts. Upstream commits covered by this audit merge: - a3e34bf fix(desktop): restore cmd+click requirement for v1 terminal file links (superset-sh#3457) [PR1/#176] - 57557f8 fix(desktop): gate v2 workspace children on collection readiness (superset-sh#3464) [PR1/#176] - 4ee2e61 fix(desktop): use native clipboard for copy path in v2 sidebar (superset-sh#3462) [PR1/#176] - 87d6e93 feat(desktop): close settings with Escape key (superset-sh#3466) [PR1/#176] - 9c7f5f4 chore(desktop): auto-restart host-service on bundle change in dev (superset-sh#3461) [PR1/#176] - 93140d9 fix(mcp): accept MCP resource URL as valid OAuth audience (superset-sh#3459) [PR2/#177] - be9e000 fix(desktop): drive tray menu off events, fetch real org name (superset-sh#3458) [PR2/#177] - c5f791e feat(v2): unify workspace delete through host-service (superset-sh#3443) [PR3/#178] - 2c24d93 feat(desktop): paginated branch picker with checkout + open actions (superset-sh#3397) [PR4/#179] - 2bf1049 feat(desktop/hotkeys): v1 directional pane focus + best-effort v1 override migrator (superset-sh#3460) [PR5/#180] - 1294a7d feat(desktop/hotkeys): restore Cmd+Alt+Arrow for tab/workspace nav (superset-sh#3472) [PR5/#180] - de70163 feat(desktop): v2 review tab first pass — PR info, checks, comments (superset-sh#3463) [PR6/#182] Intentionally skipped (version bump, fork has independent versioning): - 1e23353 chore(desktop): bump version to 1.5.5 (superset-sh#3473) Fork-adaptation fixes layered on top of the cherry-picks: - PR1: host-service-coordinator alias import fix, settings Escape selector narrowing (role-based + popper wrapper), Escape close uses replace navigation - PR2: dual quit mode preservation (requestQuit "release"/"stop"), trayUpdateToken guard for stale async fetchHostInfo results - PR4: ChangesHeader.normalizeBranchName regex rewrite (lint false positive), worktree add uses fullRef for remote-tracking refs, syncTimedOut reset on pendingId change, GIT_REFS.md barrel example fix - PR5: migrate.ts re-sanitize of existing localStorage overrides (v2 marker bump intent), FOCUS_PANE_* enabled:isActive for KeepAliveWorkspaces, CATEGORY_ORDER merges Navigation (upstream) and Browser (fork) - PR6: normalizeThreadsToComments flattens all thread.comments (not just first), CommentPane overrides <a> (openUrl) and <img> (SafeImage), zero-badge suppression, pr-null comments gate Fork features verified intact (Explore agent audit of combined 36d4de4..35d95f3 range): - BROWSER_RELOAD / BROWSER_HARD_RELOAD hotkeys - dual quit mode menu in tray - v1 terminal cold-restore + retry reconnect (out of range but unaffected) - KeepAliveWorkspaces (FOCUS_PANE_* gated on isActive) - useCommandPalette + addMemoTab in v2 workspace - host-service-coordinator rename alias pattern
1 participant
Summary
resource=https://api.superset.sh/api/agent/mcpon authorize + token requests per RFC 8707. better-auth 1.5.6 validates that againstvalidAudiencesincheckResource()and throwsrequested resource invalidwhen it's not in the set. Our allowlist only contained the bare API origin, so OAuth sign-in from MCP clients broke with "Error: requested resource invalid".validAudiencesinpackages/auth/src/server.tsso better-auth mints tokens with the correctaud, and mirror the change in the MCP route's JWT verifier (apps/api/src/app/api/agent/[transport]/auth-flow.ts) so it accepts those tokens.auth-flow.test.ts,oauth-metadata.test.ts) that didn't catch this and would need hand-updating for the new audience shape.Test plan
/mcpagainst the Superset MCP server in Claude Code, confirm the OAuth browser flow completes and the server reports authenticated./api/agent/mcp.Summary by cubic
Accept the MCP resource URL as a valid OAuth audience to restore OAuth sign-in for MCP clients (e.g., Claude Code). Tokens are now minted and verified with the MCP endpoint as an allowed
aud.https://api.superset.sh/api/agent/mcptovalidAudiencesinpackages/auth/src/server.tsforbetter-auth.audiencelist inapps/api/src/app/api/agent/[transport]/auth-flow.ts.auth-flow.test.ts,oauth-metadata.test.ts).Written for commit d212e6c. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Tests
Bug Fixes