8ed0bcf5ca
Some checks failed
Docker Dev Images Build / build (Dockerfile, librechat-dev, node) (push) Has been cancelled
Docker Dev Images Build / build (Dockerfile.multi, librechat-dev-api, api-build) (push) Has been cancelled
Sync Locize Translations & Create Translation PR / Sync Translation Keys with Locize (push) Has been cancelled
Sync Locize Translations & Create Translation PR / Create Translation PR on Version Published (push) Has been cancelled
* fix: reuse existing OAuth client registrations to prevent client_id mismatch When using auto-discovered OAuth (DCR), LibreChat calls /register on every flow initiation, getting a new client_id each time. When concurrent connections or reconnections happen, the client_id used during /authorize differs from the one used during /token, causing the server to reject the exchange. Before registering a new client, check if a valid client registration already exists in the database and reuse it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Handle re-registration of OAuth clients when redirect_uri changes * Add undefined fields for logo_uri and tos_uri in OAuth metadata tests * test: add client registration reuse tests for horizontal scaling race condition Reproduces the client_id mismatch bug that occurs in multi-replica deployments where concurrent initiateOAuthFlow calls each register a new OAuth client. Tests verify that the findToken-based client reuse prevents re-registration. * fix: address review findings for client registration reuse - Fix empty redirect_uris bug: invert condition so missing/empty redirect_uris triggers re-registration instead of silent reuse - Revert undocumented config?.redirect_uri in auto-discovery path - Change DB error logging from debug to warn for operator visibility - Fix import order: move package type import to correct section - Remove redundant type cast and misleading JSDoc comment - Test file: remove dead imports, restore process.env.DOMAIN_SERVER, rename describe blocks, add empty redirect_uris edge case test, add concurrent reconnection test with pre-seeded token, scope documentation to reconnection stabilization * fix: resolve type check errors for OAuthClientInformation redirect_uris The SDK's OAuthClientInformation type lacks redirect_uris (only on OAuthClientInformationFull). Cast to the local OAuthClientInformation type in handler.ts when accessing deserialized client info from DB, and use intersection types in tests for clientInfo with redirect_uris. * fix: address follow-up review findings R1, R2, R3 - R1: Move `import type { TokenMethods }` to the type-imports section, before local types, per CLAUDE.md import order rules - R2: Add unit test for empty redirect_uris in handler.test.ts to verify the inverted condition triggers re-registration - R3: Use delete for process.env.DOMAIN_SERVER restoration when the original value was undefined to avoid coercion to string "undefined" * fix: clear stale client registration on OAuth flow failure When a stored client_id is no longer recognized by the OAuth server, the flow fails but the stale client stays in MongoDB, causing every retry to reuse the same invalid registration in an infinite loop. On OAuth failure, clear the stored client registration so the next attempt falls through to fresh Dynamic Client Registration. - Add MCPTokenStorage.deleteClientRegistration() for targeted cleanup - Call it from MCPConnectionFactory's OAuth failure path - Add integration test proving recovery from stale client reuse * fix: validate auth server identity and target cleanup to reused clients - Gate client reuse on authorization server identity: compare stored issuer against freshly discovered metadata before reusing, preventing wrong-client reuse when the MCP server switches auth providers - Add reusedStoredClient flag to MCPOAuthFlowMetadata so cleanup only runs when the failed flow actually reused a stored registration, not on unrelated failures (timeouts, user-denied consent, etc.) - Add cleanup in returnOnOAuth path: when a prior flow that reused a stored client is detected as failed, clear the stale registration before re-initiating - Add tests for issuer mismatch and reusedStoredClient flag assertions * fix: address minor review findings N3, N5, N6 - N3: Type deleteClientRegistration param as TokenMethods['deleteTokens'] instead of Promise<unknown> - N5: Elevate deletion failure logging from debug to warn for operator visibility when stale client cleanup fails - N6: Use getLogPrefix() instead of hardcoded log prefix to respect system-user privacy convention * fix: correct stale-client cleanup in both OAuth paths - Blocking path: remove result?.clientInfo guard that made cleanup unreachable (handleOAuthRequired returns null on failure, so result?.clientInfo was always false in the failure branch) - returnOnOAuth path: only clear stored client when the prior flow status is FAILED, not on COMPLETED or PENDING flows, to avoid deleting valid registrations during normal flow replacement * fix: remove redundant cast on clientMetadata clientMetadata is already typed as Record<string, unknown>; the as Record<string, unknown> cast was a no-op. * fix: thread reusedStoredClient through return type instead of re-reading flow state FlowStateManager.createFlow() deletes FAILED flow state before rejecting, so getFlowState() after handleOAuthRequired() returns null would find nothing — making the stale-client cleanup dead code. Fix: hoist reusedStoredClient flag from flowMetadata into a local variable, include it in handleOAuthRequired()'s return type (both success and catch paths), and use result.reusedStoredClient directly in the caller instead of a second getFlowState() round-trip. * fix: selective stale-client cleanup in returnOnOAuth path The returnOnOAuth cleanup was unreliable: it depended on reading FAILED flow state, but FlowStateManager.monitorFlow() deletes FAILED state before rejecting. Move cleanup into createFlow's catch handler where flowMetadata.reusedStoredClient is still in scope. Make cleanup selective in both paths: add isClientRejection() helper that only matches errors indicating the OAuth server rejected the client_id (invalid_client, unauthorized_client, client not found). Timeouts, user-cancelled flows, and other transient failures no longer wipe valid stored registrations. Thread the error from handleOAuthRequired() through the return type so the blocking path can also check isClientRejection(). * fix: tighten isClientRejection heuristic Narrow 'client_id' match to 'client_id mismatch' to avoid false-positive cleanup on unrelated errors that happen to mention client_id. * test: add isClientRejection tests and enforced client_id on test server - Add isClientRejection unit tests: invalid_client, unauthorized_client, client_id mismatch, client not found, unknown client, and negative cases (timeout, flow state not found, user denied, null, undefined) - Enhance OAuth test server with enforceClientId option: binds auth codes to the client_id that initiated /authorize, rejects token exchange with mismatched or unregistered client_id (401 invalid_client) - Add integration tests proving the test server correctly rejects stale client_ids and accepts matching ones at /token * fix: issuer validation, callback error propagation, and cleanup DRY - Issuer check: re-register when storedIssuer is absent or non-string instead of silently reusing. Narrows unknown type with typeof guard and inverts condition so missing issuer → fresh DCR (safer default). - OAuth callback route: call failFlow with the OAuth error when the authorization server redirects back with error= parameter, so the waiting flow receives the actual rejection instead of timing out. This lets isClientRejection match stale-client errors correctly. - Extract duplicated cleanup block to clearStaleClientIfRejected() private method, called from both returnOnOAuth and blocking paths. - Test fixes: add issuer to stored metadata in reuse tests, reset server to undefined in afterEach to prevent double-close. * fix: gate failFlow behind callback validation, propagate reusedStoredClient on join - OAuth callback: move failFlow call to after CSRF/session/active-flow validation so an attacker with only a leaked state parameter cannot force-fail a flow without passing the same integrity checks required for legitimate callbacks - PENDING join path: propagate reusedStoredClient from flow metadata into the return object so joiners can trigger stale-client cleanup if the joined flow later fails with a client rejection * fix: restore early oauthError/code redirects, gate only failFlow behind CSRF The previous restructuring moved oauthError and missing-code checks behind CSRF validation, breaking tests that expect those redirects without cookies. The redirect itself is harmless (just shows an error page). Only the failFlow call needs CSRF gating to prevent DoS. Restructure: oauthError check stays early (redirects immediately), but failFlow inside it runs the full CSRF/session/active-flow validation before marking the flow as FAILED. * fix: require deleteTokens for client reuse, add missing import in MCP.js Client registration reuse without cleanup capability creates a permanent failure loop: if the reused client is stale, the code detects the rejection but cannot clear the stored registration because deleteTokens is missing, so every retry reuses the same broken client_id. - MCPConnectionFactory: only pass findToken to initiateOAuthFlow when deleteTokens is also available, ensuring reuse is only enabled when recovery is possible - api/server/services/MCP.js: add deleteTokens to the tokenMethods object (was the only MCP call site missing it) * fix: set reusedStoredClient before createFlow in joined-flow path When joining a PENDING flow, reusedStoredClient was only set on the success return but not before the await. If createFlow throws (e.g. invalid_client during token exchange), the outer catch returns the local variable which was still false, skipping stale-client cleanup. * fix: require browser binding (CSRF/session) for failFlow on OAuth error hasActiveFlow only proves a PENDING flow exists, not that the caller is the same browser that initiated it. An attacker with a leaked state could force-fail the flow without any user binding. Require hasCsrf or hasSession before calling failFlow on the oauthError path. * fix: guard findToken with deleteTokens check in blocking OAuth path Match the returnOnOAuth path's defense-in-depth: only enable client registration reuse when deleteTokens is also available, ensuring cleanup is possible if the reused client turns out to be stale. * fix: address review findings — tests, types, normalization, docs - Add deleteTokens method to InMemoryTokenStore matching TokenMethods contract; update test call site from deleteToken to deleteTokens - Add MCPConnectionFactory test: returnOnOAuth flow fails with invalid_client → clearStaleClientIfRejected invoked automatically - Add mcp.spec.js tests: OAuth error with CSRF → failFlow called; OAuth error without cookies → failFlow NOT called (DoS prevention) - Add JSDoc to isClientRejection with RFC 6749 and vendor attribution - Add inline comment explaining findToken/deleteTokens coupling guard - Normalize issuer comparison: strip trailing slashes to prevent spurious re-registrations from URL formatting differences - Fix dead-code: use local reusedStoredClient variable in PENDING join return instead of re-reading flowMeta * fix: address final review nits N1-N4 - N1: Add session cookie failFlow test — validates the hasSession branch triggers failFlow on OAuth error callback - N2: Replace setTimeout(50) with setImmediate for microtask drain - N3: Add 'unknown client' attribution to isClientRejection JSDoc - N4: Remove dead getFlowState mock from failFlow tests --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Danny Avila <danny@librechat.ai>