Conversation
ital0
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
cjroth
left a comment
cjroth
left a comment
There was a problem hiding this comment.
This is just a quick first-pass review - I'm going to do a deeper review when I have more brainspace later.
ital0
force-pushed
the
italomenezes/thu-348-get-mcps-working-including-authenticated-options
branch
from
b0de12e to
8c3df2d
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ReviewBugs & Data LossCredential deleted on transport config change — Fix: separate "remove from provider state" from "delete credential". Credentials should only be deleted when the user explicitly removes a server, not during config-change re-adds. Auth config changes not propagated — Invalid OAuth redirect URI fallback — Project Convention Violations
Dynamic imports without circular-dependency justification ( |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9e4525f. Configure here.
| } catch (err) { | ||
| // If the server requires OAuth, the SDK calls redirectToAuthorization then throws. | ||
| // On web: redirectToAuthorization does window.location.assign (page navigates away). | ||
| // The error is expected — on return, useMcpOAuthCallback exchanges the code. | ||
| // On desktop/mobile: waitForAuthCode captures the code, finishAuth exchanges it. | ||
| if (authProvider && err instanceof Error && err.message.includes('Unauthorized')) { | ||
| await transport.close() | ||
| setServers((prev) => | ||
| prev.map((s) => | ||
| s.id === config.id | ||
| ? { ...s, client: null, isConnected: false, error: null, errorMessage: 'needsAuth', enabled: true } | ||
| : s, | ||
| ), | ||
| ) | ||
| return new Promise<McpClient>(() => {}) // park — user must click Authorize | ||
| } | ||
| throw err | ||
| } |
There was a problem hiding this comment.
createMCPClient fails for non-OAuth reasons
When createMCPClient throws for a reason other than OAuth (e.g., network failure, server error), the transport created by createTransport is not stored in transportRefs and not explicitly closed before the error propagates. connectServer's catch block schedules a retry — the retry calls createTransport again creating a new transport, while the previous one remains open and unreachable.
With up to maxAttempts (6) retries, a persistently unreachable server leaks 6 transports. SSE transports in particular may hold open EventSource-style connections.
} catch (err) {
if (authProvider && err instanceof Error && err.message.includes('Unauthorized')) {
await transport.close()
// ... existing OAuth handling ...
return new Promise<McpClient>(() => {})
}
await transport.close() // close on all non-OAuth error paths too
throw err
}
| for (const providerServer of servers) { | ||
| for (const providerServer of currentServers) { | ||
| if (!dbServerIds.has(providerServer.id)) { | ||
| removeServer(providerServer.id) |
There was a problem hiding this comment.
removeServer is async (it calls disconnectServer + credential delete), but here it's fire-and-forget — errors are silently dropped and the cleanup runs concurrently with the rest of the loop. Line 83 below awaits the same function, which is the correct pattern. Change to await removeServer(providerServer.id) to match.
| authorizeServer: (serverId: string) => Promise<void> | ||
| addServer: (server: McpServerConfig) => Promise<void> | ||
| removeServer: (serverId: string) => void | ||
| updateServerStatus: (serverId: string, enabled: boolean) => void |
There was a problem hiding this comment.
removeServer and updateServerStatus are implemented as async functions but are typed here as () => void. TypeScript won't flag missing awaits at call sites — which is exactly how the unawaited removeServer call in use-mcp-sync.tsx:68 slipped through. Both should be (serverId: string) => Promise<void> and (serverId: string, enabled: boolean) => Promise<void> respectively.
ReviewSolid PR overall — the OAuth/PKCE flow is well-structured, the credential store design is sound, the SSRF fix ( Two issues, both stemming from the same root cause: Bugs
TypeScript treats void-returning functions as non-awaitable, which masks missing
When a server is deleted from the DB, |
cjroth
temporarily deployed
to
italomenezes/thu-348-get-mcps-working-including-authenticated-options - thunderbolt PR #508
Render
Destroyed
Review: PR #508 Get MCPs workingTwo issues found. Security — shell capabilities allow unrestricted argument injection
Bug — OAuth redirectUri falls back to serverUrl
OAuth providers require an exact redirect_uri match between the authorization request and token exchange. serverUrl is the MCP server root, not a registered redirect URI. If oauthState.redirectUrl is absent (expired state, partial DB corruption, or a future code path that omits it), exchangeAuthorization will receive an unregistered URI and fail. The fallback should throw rather than silently use an invalid value. |
| clientInformation: clientInfo, | ||
| authorizationCode: mcpOauth.code, | ||
| codeVerifier: oauthState.codeVerifier, | ||
| redirectUri: oauthState.redirectUrl || oauthState.serverUrl, |
There was a problem hiding this comment.
If redirectUrl is absent from state (expired, DB corruption, or a future code path that forgets to set it), serverUrl is used as the redirect_uri. OAuth providers enforce an exact match against the URI registered during authorization — serverUrl is the MCP server root, not a redirect endpoint, so this would fail. Throw instead of falling back to an invalid value.
| { | ||
| "name": "npx", | ||
| "cmd": "npx", | ||
| "args": true |
There was a problem hiding this comment.
"args": true grants unrestricted arguments to node/python3/npx/bun/uvx. validateStdioArgs only rejects null bytes, so flags like node -e "..." or python3 -c "..." pass validation and execute arbitrary inline code. A user importing a malicious server config (e.g., from a QR code or shared JSON) would trigger silent code execution. Block shell-interpreter flags (-e, --eval, -c, --input-type=module) in validateStdioArgs.
| <div className="grid gap-2"> | ||
| <Label htmlFor="auth-type">Authentication</Label> | ||
| <Select | ||
| value={formState.authType} | ||
| onValueChange={(value) => formDispatch({ type: 'SET_AUTH_TYPE', payload: value as McpAuthType })} | ||
| > | ||
| <SelectTrigger id="auth-type" className="w-full"> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="none">None</SelectItem> | ||
| <SelectItem value="bearer">API Key / Bearer Token</SelectItem> | ||
| <SelectItem value="oauth">OAuth 2.1</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
| </div> |
There was a problem hiding this comment.
The auth-type selector renders oauth as a valid option for all transport types, including stdio. However, discoverOAuth in mcp-provider.tsx calls createTransport, which for stdio returns no authProvider. discoverOAuth then immediately returns { error: 'Server does not require OAuth' }, and authorizeServer surfaces that as an error message.
The resulting user journey:
- User configures
stdio+oauthand saves. connectServerdetectsauthType === 'oauth', finds no stored credential, and sets state toneedsAuth.- User clicks "Authorize" →
discoverOAuthreturns "Server does not require OAuth". - The server is permanently stuck — the user must delete and re-add it to recover.
isValid() in useMcpServerFormState accepts any auth type for stdio with no guard:
if (state.transportType === 'stdio') {
validateStdioCommand(state.command)
validateStdioArgs(state.args)
return true // auth type not checked
}Fix: hide (or disable) the oauth option when transportType === 'stdio':
| <div className="grid gap-2"> | |
| <Label htmlFor="auth-type">Authentication</Label> | |
| <Select | |
| value={formState.authType} | |
| onValueChange={(value) => formDispatch({ type: 'SET_AUTH_TYPE', payload: value as McpAuthType })} | |
| > | |
| <SelectTrigger id="auth-type" className="w-full"> | |
| <SelectValue /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| <SelectItem value="none">None</SelectItem> | |
| <SelectItem value="bearer">API Key / Bearer Token</SelectItem> | |
| <SelectItem value="oauth">OAuth 2.1</SelectItem> | |
| </SelectContent> | |
| </Select> | |
| </div> | |
| <SelectContent> | |
| <SelectItem value="none">None</SelectItem> | |
| <SelectItem value="bearer">API Key / Bearer Token</SelectItem> | |
| {formState.transportType !== 'stdio' && ( | |
| <SelectItem value="oauth">OAuth 2.1</SelectItem> | |
| )} | |
| </SelectContent> |
2 participants
Summary
All MCP changes in a single PR for team validation. Use the desktop app (
bun tauri dev) to test all features.This PR contains the same code as PRs [1/6] through [6/6] combined, with all review fixes applied. Review individual PRs for focused diffs.
Test Scenarios
Note
High Risk
High risk because it introduces new credential storage/encryption, OAuth redirect handling, and desktop shell-based stdio process spawning, plus changes to proxy redirect handling for SSRF mitigation.
Overview
Adds end-to-end MCP server support across HTTP (Streamable), SSE, and desktop
stdiotransports, including a refactored MCP settings UI (new add dialog, server cards with retry/authorize, platform transport filtering) and provider-side reconnect/backoff handling.Introduces MCP authentication: bearer tokens and OAuth 2.1. OAuth now uses a dedicated callback route (
/mcp/oauth/callback) with deep-link parsing, CSRF state checks, and a callback hook to exchange codes; credentials are stored in a new local-onlymcp_credentialstable using AES-GCM encryption derived from a per-device key.Stops syncing
mcp_serversvia PowerSync (now local-only), updates tool wiring so chat sessions fetch enabled MCP clients via a provider getter, and namespaces MCP tools by sanitized server name to avoid collisions while surfacing a connected-server summary in the system prompt. Also hardens the backendmcp-proxyby following redirects undersafeFetchto block redirect-based SSRF, and enables required Tauri plugins/capabilities (httpalways on,shellspawn for specific commands) plus broader localhost HTTP allowlists.Reviewed by Cursor Bugbot for commit 19adf10. Bugbot is set up for automated code reviews on this repo. Configure here.