fix(mcp): use dynamic client registration for OAuth instead of hardcoded client ID#1313
Conversation
…ded client ID The hardcoded clientId "claude-code" in .mcp.json was never registered in the oauth_applications table, causing invalid_client errors. Remove the oauth config to let Claude Code use the standard MCP OAuth discovery and dynamic client registration flow. Also resolve the consent form to show the registered application name from the database.
📝 WalkthroughWalkthroughThe changes add client name fetching and display to the OAuth consent flow while removing OAuth configuration from the superset MCP server. The ConsentForm component now accepts an optional clientName prop, and the consent page queries the database for the OAuth application name to pass to the form. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 |
![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
🤖 Fix all issues with AI agents
In `@apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx`:
- Around line 145-150: The consent screen currently renders the
attacker-controlled displayName in ConsentForm; update ConsentForm (where
displayName is used) to show the raw clientId alongside displayName and/or a
visual "verified" badge for trusted clients: fetch or accept a isVerified flag
for the client and render a small badge component next to displayName when true,
and render clientId in muted text under or beside the name when not verified;
ensure displayName/clientId are output as plain text (React JSX escaping) or
explicitly sanitize them before rendering to avoid any HTML injection.
🧹 Nitpick comments (1)
apps/web/src/app/oauth/consent/page.tsx (1)
67-70: Directdbaccess bypasses the tRPC layer used elsewhere in this file.Line 65 fetches
userOrganizationsthrough tRPC, but the newoauthApplicationslookup goes straight to the DB client. This is a minor inconsistency — consider wrapping this in a tRPC procedure (or a shared query helper) so all data access in this page follows the same pattern. Not blocking given the query's simplicity.
| Authorize {displayName} | ||
| </h1> | ||
| <p className="text-muted-foreground text-sm"> | ||
| <span className="font-medium text-foreground">{clientName}</span> is | ||
| <span className="font-medium text-foreground">{displayName}</span> is | ||
| requesting access to your Superset account | ||
| </p> |
There was a problem hiding this comment.
Consider whether displaying an attacker-controlled name needs sanitization or a visual trust indicator.
With dynamic client registration, any caller can register an application with an arbitrary name (e.g., "Superset Desktop" or "Google"). The consent screen currently renders that name without any trust differentiation. This could be used to impersonate a known/trusted application.
Consider either:
- Showing the raw
clientIdalongside the display name so users can distinguish dynamically registered apps. - Adding a "verified" badge for known/trusted clients only.
🤖 Prompt for AI Agents
In `@apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx` around
lines 145 - 150, The consent screen currently renders the attacker-controlled
displayName in ConsentForm; update ConsentForm (where displayName is used) to
show the raw clientId alongside displayName and/or a visual "verified" badge for
trusted clients: fetch or accept a isVerified flag for the client and render a
small badge component next to displayName when true, and render clientId in
muted text under or beside the name when not verified; ensure
displayName/clientId are output as plain text (React JSX escaping) or explicitly
sanitize them before rendering to avoid any HTML injection.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
invalid_clientbecause the hardcodedclientId: "claude-code"in.mcp.jsonwas never registered in theoauth_applicationsdatabase tableChanges
.mcp.json: Removedoauthblock (clientId, authorizationUrl, tokenUrl, scopes) — Claude Code will auto-discover these via/.well-known/oauth-authorization-serverapps/web/src/app/oauth/consent/page.tsx: Queryoauth_applicationstable for the client'snamefield and pass it asclientNamepropapps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx: AcceptclientNameprop and prefer it over the hardcodedknownClientsfallback mapTest Plan
/mcpand authenticate the Superset MCP server — should complete OAuth flow withoutinvalid_clienterrorknownClientsfallback still works for clients without a DB nameSummary by CodeRabbit
New Features
Chores