🛂 fix: Respect requiresOAuth Config in User MCP Connections - #12511
danny-avila wants to merge 5 commits into
Conversation
UserConnectionManager hardcoded useOAuth: true for all user connections, causing non-OAuth servers (e.g., private MCP with OIDC token headers) to incorrectly trigger OAuth flows on 401 responses. - Derive useOAuth from config.requiresOAuth/config.oauthMetadata, matching the logic already used in MCPManager.discoverServerTools() - Widen MCPConnectionFactory.create() to accept UserConnectionContext for non-OAuth user connections - Register a fallback oauthRequired handler on non-OAuth connections that immediately emits oauthFailed, preventing a 120s timeout hang when the server returns 401 for expired tokens
Verify that MCPConnectionFactory registers a fallback handler on non-OAuth connections that emits oauthFailed when oauthRequired fires.
There was a problem hiding this comment.
Pull request overview
This PR fixes user-scoped MCP connections incorrectly forcing OAuth behavior for servers configured as non-OAuth (requiresOAuth: false), which previously caused OAuth registration attempts and connection failures on standard 401 auth errors (e.g., expired pass-through OIDC tokens).
Changes:
- Derives
useOAuthfor user connections fromconfig.requiresOAuth || config.oauthMetadatainUserConnectionManager. - Broadens
MCPConnectionFactory.create()to accept a lightweightUserConnectionContextwhen OAuth is not used. - Adds a non-OAuth
oauthRequiredfallback handler inMCPConnectionFactoryand adds Jest coverage for that behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/api/src/mcp/UserConnectionManager.ts | Stops hardcoding useOAuth: true; chooses OAuth vs non-OAuth options based on server config. |
| packages/api/src/mcp/MCPConnectionFactory.ts | Accepts non-OAuth user context and adds a fallback oauthRequired listener for non-OAuth connections. |
| packages/api/src/mcp/tests/MCPConnectionFactory.test.ts | Adds a unit test asserting the fallback handler is registered and emits oauthFailed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ); | ||
| connection.emit('oauthFailed', new Error('Server does not use OAuth')); | ||
| }; | ||
| connection.once('oauthRequired', nonOAuthHandler); |
There was a problem hiding this comment.
The non-OAuth fallback handler is registered with once('oauthRequired', ...), but the factory’s connectTo() retries up to 3 times on failures when useOAuth is false. If the first attempt hits a 401/403, the once handler fires and is removed; a subsequent retry that triggers oauthRequired will have no listener and connectClient() will wait for oauthHandledPromise until timeout. Register this handler with on('oauthRequired', ...) (and clean it up afterwards), or alternatively ensure retries are skipped for 401/403 on non-OAuth connections so the handler isn’t needed more than once.
| connection.once('oauthRequired', nonOAuthHandler); | |
| connection.on('oauthRequired', nonOAuthHandler); |
| const oauthOptions: t.OAuthConnectionOptions | t.UserConnectionContext = useOAuth | ||
| ? { | ||
| useOAuth: true as const, | ||
| user, | ||
| customUserVars, | ||
| flowManager: flowManager!, |
There was a problem hiding this comment.
flowManager is asserted with flowManager! when useOAuth is true. Because getConnection() accepts flowManager? (and some call sites are JS), it’s possible to reach this branch with flowManager undefined and get a runtime crash later (e.g., when loading tokens). Add an explicit validation when useOAuth is true (and ideally user.id), and throw a clear McpError/Error explaining that OAuth servers require a flow manager, instead of relying on a non-null assertion.
| const oauthOptions: t.OAuthConnectionOptions | t.UserConnectionContext = useOAuth | |
| ? { | |
| useOAuth: true as const, | |
| user, | |
| customUserVars, | |
| flowManager: flowManager!, | |
| if (useOAuth) { | |
| if (!flowManager) { | |
| throw new McpError( | |
| ErrorCode.InvalidRequest, | |
| 'OAuth-enabled MCP servers require a flow manager to be provided.', | |
| ); | |
| } | |
| if (!user?.id) { | |
| throw new McpError( | |
| ErrorCode.InvalidRequest, | |
| 'OAuth-enabled MCP servers require a user with a valid id.', | |
| ); | |
| } | |
| } | |
| const oauthOptions: t.OAuthConnectionOptions | t.UserConnectionContext = useOAuth | |
| ? { | |
| useOAuth: true as const, | |
| user, | |
| customUserVars, | |
| flowManager, |
- Change connection.once to connection.on for the non-OAuth fallback handler so it survives retry attempts 2 and 3 in connectTo() - Extract isOAuthServer() into utils.ts as a single source of truth for the requiresOAuth/oauthMetadata check, used by both MCPManager and UserConnectionManager
- Add explicit early throw when an OAuth server is missing flowManager, replacing the silent non-null assertion that would crash deep in getOAuthTokens with an undiagnosable error - Add UserMCPConnectionOptions type with optional OAuth fields, so callers connecting to non-OAuth servers are not forced to supply flowManager/tokenMethods/signal they don't need
- Fix fallback handler test to use connection.on (not once), remove redundant assertion, and verify removeListener cleanup - Add MCPManager test for non-OAuth discoverServerTools path - Add getUserConnection tests verifying useOAuth is derived from config.requiresOAuth, not hardcoded - Add test that OAuth server without flowManager throws early
|
@danny-avila you planning to merge this in? I am seeing what I think issue with an MCP server that takes a token as user input. The UI state is mixed “successfully initialized” then error. I separately tried this locally with success: In packages/api/src/mcp/UserConnectionManager.ts, changed hardcoded useOAuth: true to:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Closing in favor of #13254, which now folds in the policy-layer fix from this PR along with the proactive OAuth flow changes. The combined PR keeps |
Summary
Private MCP servers configured with
requiresOAuth: falseand OIDC token pass-through headers ({{LIBRECHAT_OPENID_ACCESS_TOKEN}}) were incorrectly triggering OAuth flows when the token expired (401 response). This happened becauseUserConnectionManagerhardcodeduseOAuth: truefor all user connections, causing the OAuth machinery to intercept standard auth failures. The server would attempt OAuth client registration against a private endpoint with no OAuth support, resulting in a 404 and total connection failure.useOAuthfromconfig.requiresOAuthandconfig.oauthMetadatainUserConnectionManager.createUserConnectionInternal(), aligning with the conditional logic already used inMCPManager.discoverServerTools().isOAuthServer()utility intoutils.tsas a single source of truth for the OAuth check, replacing duplicatedBoolean(config.requiresOAuth || config.oauthMetadata)in bothMCPManagerandUserConnectionManager.MCPConnectionFactory.create()signature to acceptUserConnectionContextfor non-OAuth user connections, since OAuth fields (flowManager, tokenMethods, etc.) are unnecessary whenuseOAuthis false.oauthRequiredhandler on non-OAuth connections usingconnection.on(notonce) so it survives all three retry attempts inconnectTo(), immediately emittingoauthFailedto prevent a 120-second timeout hang.UserMCPConnectionOptionstype with optional OAuth-specific fields, sogetUserConnectioncallers connecting to non-OAuth servers are not forced to supplyflowManager/tokenMethods/signal.flowManager, replacing a silentflowManager!non-null assertion that would crash deep ingetOAuthTokens()with no actionable context.useOAuthderivation throughgetUserConnection, fallback handler behavior, listener cleanup, and theflowManagerguard.Change Type
Testing
MCPConnectionFactorytest suite (20 tests pass), including updated test for fallback handler usingconnection.on, cleanup verification, and removal of redundant assertion.MCPManagertest suite (30 tests pass, up from 26), including new tests for:discoverServerToolspath (requiresOAuth: false→ nouseOAuth: true)getUserConnectionwithrequiresOAuth: true→ passesuseOAuth: trueto factorygetUserConnectionwithrequiresOAuth: false→ does NOT passuseOAuth: trueflowManager→ throws early with descriptive messageVerification steps:
clickhouse-cloudwithrequiresOAuth: false) — should connect without OAuth prompts using OIDC token headers.requiresOAuth: true) still complete OAuth flows correctly.Checklist