9 commits

Author	SHA1	Message	Date
Danny Avila	fa913148fb	🔒 fix: Refresh MCP OBO Tokens From the Live OpenID Session (#15334) ... * 🧊 fix: Inline-refresh OpenID session tokens at MCP OBO call time Resolves the walk-away failure mode where MCP tool calls using OBO auth fail with "No valid OpenID access token is available for OBO exchange" after a user idles past their access-token lifetime. The strategy-time snapshot on `user.federatedTokens` could expire mid-stream before `resolveOboToken` ran, while `req.session.openidTokens` carried a still- valid (or refreshable) token that nothing read. - New OpenIDSessionRefresh service: per-user single-flighted closure that reads `req.session.openidTokens` at OBO time and inline-refreshes via `openid-client.refreshTokenGrant` when expired (30s skew), persisting via `req.session.save()`. No cookie writes (headers already flushed). - `resolveOboToken` gains a required UpstreamTokenProvider parameter (typed as `() => Promise<OIDCTokens | null>`, reusing the shared shape from @librechat/data-schemas). Compile-time guarantee that every call site is updated. - New `session_refresh_failed` OboTokenResolutionReason distinguishes "session expired and IdP rejected refresh" from "no upstream token ever existed." - `req` threaded through createMCPTool/createMCPTools/createToolInstance to construct the closure with captured request, plus fail-closed guards in MCPConnectionFactory.getOboTokens and MCPManager.callTool when the closure isn't plumbed. - Startup warning in MCPServersInitializer when OBO is configured but OPENID_REUSE_TOKENS is unset (the strategy populating user.federatedTokens is only registered under reuse, so OBO would fail every call without it). Tests: 16 new in OpenIDSessionRefresh.spec.js; obo.spec.ts extended for the new param + error reason; wiring smoke tests in MCPManager, MCPConnectionFactory, MCPServersInitializer, and MCP.spec.js. * 🛡️ fix: Harden OBO inline-refresh against token type and session edge cases - Token-preference asymmetry: live-token reuse and expires_at derivation now strictly gate on the access_token, not the id_token. Added a required `tokenPreference` parameter on isLiveSessionTokenStillValid, buildOIDCTokensFromSession, and createOpenIDSessionTokenProvider so every call site is explicit. Dropped the bogus id_token-exp fallback in performIdpRefresh — id_token TTL is governed by IdP session policy and would mark a short-lived access_token reusable past its real lifetime. - Missing req in /reinitialize route: the manual reconnect endpoint now forwards req into reinitMCPServer, so OBO servers can build a session-aware upstream-token closure instead of failing with missing_upstream_token. - Single-flight key collisions: composed key as tenantId:openidIssuer:openidId:sessionId via getSingleFlightKey. Concurrent calls in the same session still coalesce; separate sessions never share an in-flight refresh, preventing refresh-token rotation from breaking sibling sessions and preventing cross-tenant token crossover when distinct users share an IdP sub. - Opaque access token reuse): persist accessTokenExpiresAt (unix seconds, from tokenset.expires_in) on each refresh AND on initial login / SPA refresh in setOpenIDAuthTokens. New getAccessTokenExp helper falls back to it when the access token isn't a JWT, avoiding redundant inline refreshes for Microsoft Graph and Auth0 default audiences. - Log hygiene: the single-flight key (containing sessionId, openidId, openidIssuer, tenantId) is now SHA-256-hashed in the "Joining in-flight refresh" debug log. Preserves cross-line correlation via a 12-char prefix without leaking credential or PII material. Documented req.session.openidTokens shape contract via JSDoc typedef so the new accessTokenExpiresAt field has a discoverable home alongside the existing accessToken/idToken/refreshToken/expiresAt/lastRefreshedAt. Tests: OpenIDSessionRefresh.spec.js up to 30 passing (added coverage for opaque-token reuse, JWT-access-token-exp fallback, no-id_token-fallback regression, cross-session no-coalesce, persistence on refresh, and a guard against stale accessTokenExpiresAt carryover). AuthService.spec.js adds two cases covering accessTokenExpiresAt persistence on login. mcp.spec.js (route) gains a regression test asserting req flows into reinitMCPServer. * 🔍 fix: Detect OBO-only MCP admin config overrides Admin Config overlays for YAML-defined MCP servers compare only ADMIN_CONFIGURABLE_FIELDS to decide whether to lazy-init a config-tier override. The OBO config field was added after that fingerprint list, so an override that only added or changed `obo` was treated as unchanged YAML and skipped. Include `obo` in the admin-configurable field list and add a regression test for an OBO-only override. * 🔊 fix: Mock MCP OAuth timeout in SDK integration test MCPConnectionFactory.attemptToConnect reads mcpConfig.OAUTH_HANDLING_TIMEOUT when building the OAuth connection timeout. The SDK OAuth integration test mocked mcpConfig without that field, which made the timeout calculation produce NaN and caused the test to fail before the OAuth refresh/start path completed. Add OAUTH_HANDLING_TIMEOUT to the test mock. * ♻️ refactor: Pass OBO upstream-token closure into MCP instead of req Build the OpenID upstream-token provider at the request boundary and thread only the closure through MCP handling, so the MCP service layer no longer receives the raw Express request. The closure still reads/refreshes the live session at tool-call time, preserving the walk-away recovery. - Drop `req`/`capturedReq` from createMCPTools, createMCPTool, reconnectServer, createToolInstance, and reinitMCPServer; forward `upstreamTokenProvider` instead. Closure is constructed in loadTools, loadToolDefinitionsWrapper, and the reinitialize route, where req/res are in scope. - OBO: fall back to user.federatedTokens when the provider yields no live session, so OIDC remote-agent calls (verified bearer, no session) still work. - Inline refresh: mirror a rotated refresh token to the refreshToken cookie via a shared setRefreshTokenCookie helper, guarded by !res.headersSent (no-op on the streaming path; session copy stays authoritative). - Single-flight: hydrate a joining request's own session from the resolved tokens so a later OBO call doesn't replay a rotated-away refresh token. Addresses owner feedback and three review findings. * 🔒 fix: Recover OIDC refresh-token rotation after SSE OBO refresh When an inline OBO refresh rotates the OpenID refresh token after SSE headers have already been sent, the browser refreshToken cookie cannot be updated. Store a short-lived encrypted bridge from the stale cookie token to the rotated token so /api/auth/refresh can recover after express-session loss. Use the signed openid_user_id cookie to load user context for bridge validation, retry only on invalid_grant, and delete the bridge only after the bridged refresh succeeds. * 🔨 fix: hydrate joined OIDC refresh sessions with stable refresh tokens Update single-flight OIDC refresh joiners whenever refreshed access token state changes, even if the IdP keeps the refresh token unchanged. This prevents joined requests from retaining stale accessToken or accessTokenExpiresAt values and redundantly refreshing later in the same run. * 🌉 Persist OIDC refresh-token recovery bridges in MongoDB Store SSE OBO refresh-token recovery bridges in MongoDB instead of process-local memory so /api/auth/refresh can recover after worker restarts or cross-worker routing. Derive bridge expiry from REFRESH_TOKEN_EXPIRY so the recovery window matches the stale refreshToken cookie it repairs, and delete bridges after successful recovery. * 🤝 Coordinate OIDC inline refreshes across workers Add a short-lived Mongo-backed refresh-flight record so concurrent OBO refreshes for the same OpenID session do not redeem the same rotating refresh token on different workers. The winning worker performs the IdP refresh and stores an encrypted result; joiners wait for that result, hydrate their request session, and return without calling the IdP. * ⚓ Keep OpenID marker cookies aligned on inline refresh Refresh token_provider and openid_user_id with the same expiry as the rotated refreshToken cookie when an inline OBO refresh can still write headers. Share the marker-cookie writer with the normal OpenID auth refresh path so the fallback /api/auth/refresh branch continues to recognize valid OpenID refresh tokens after session expiry. * 🔑 fix: include refresh token in OIDC local refresh flight key Key the process-local OIDC refresh coalescing by the current session refresh token, matching the Mongo-backed flight key. This prevents a request with a newly rotated token from joining an older pending refresh and inheriting its failure/result. * 🌉 fix: store OIDC refresh bridge without cookie response Treat missing or non-cookie responses like headers-sent streaming responses during inline OIDC refresh. When the IdP rotates the refresh token and cookies cannot be written, persist a recovery bridge so a later /auth/refresh can recover after session expiry. * 🫙 fix: preserve stale OIDC cookie bridge key Track the refresh token last written to the browser cookie separately from the current session refresh token. When inline OIDC refreshes rotate tokens without a writable response, keep bridging from the browser-stale token directly to the latest session token. * 🙌 fix: keep OIDC bridge recovery success on cleanup failure Make refresh-token bridge cleanup best-effort after a bridged OIDC refresh succeeds. A transient delete failure now logs a warning but does not convert the already-refreshed session and cookies into a 403 response. * 📦 test: Exclude RefreshTokenBridge from tenant-isolation coverage Add RefreshTokenBridge to the tenant-isolation coverage allowlist because refresh bridge lookups run during unauthenticated OpenID refresh recovery. The controller first recovers user context from the signed OpenID marker cookie, then the bridge methods apply explicit user and tenant filters. Ambient tenant isolation would bind this recovery path to request-local tenant context that is not available at the point the stale cookie is being resolved * ⚡ Fix OpenID refresh flight retry and marker hydration Allow failed OpenID refresh flights to be reclaimed immediately instead of pinning transient errors. Preserve the browser refresh-token marker when joined refreshes hydrate session tokens from a shared flight result. Stabilize AuthService tests by isolating mocked module imports from prior suites. * 🛠️ fix: centralize OBO identity scoping Add shared auth identity helpers for app user ids, OpenID subjects, tenant ids, and normalized OpenID issuers. Thread a non-placeholder-visible OBO identity context from the real request user through MCP connection, tool-call, reinit, and refresh paths. Keep tenantId and openidIssuer out of createSafeUser so MCP user placeholders do not expose those fields. Scope OBO token cache and in-flight exchange keys by tenant, issuer, OpenID subject, scopes, and a SHA-256 hash of the upstream assertion. This prevents cross-tenant/cross-issuer collisions and avoids reusing tokens minted from stale rotated assertions. Use the shared identity helpers for OpenID refresh-flight keys and refresh-token bridge recovery records so related OBO refresh paths share the same identity normalization rules. The helper is intended for auth-boundary and credential-cache code, not as a blanket replacement for ordinary app user id ownership checks. * 🛠️ fix: preserve OIDC refresh-token sync on save failures Sync OpenID refresh-token cookie/bridge state before persisting the session so a transient session-store failure cannot lose an IdP-rotated refresh token. Also trigger sync when the session refresh token differs from the browser refresh-token marker, not only when the current grant rotates the token. This lets later writable refreshes repair stale browser cookies left behind by SSE refreshes. Route refresh bridge identity through the shared identity helper with the threaded OBO identity context, falling back to request/user context when needed. Add regression coverage for session-save failures, stale browser cookie repair, non-writable bridge storage, and shared-helper identity fallback. * 🛠️ fix: keep OIDC refresh bridge during recovery grace After successful bridged refresh recovery, re-store the stale-cookie bridge with a short grace TTL instead of deleting it immediately. This lets parallel /api/auth/refresh requests that already sent the stale browser cookie recover before they can observe the first response's Set-Cookie. Retarget the bridge to the refresh token returned by the bridged retry so B-to-C refresh-token rotation remains recoverable. The grace TTL is parsed with math() and defaults to 60s, which shrinks the replay window from the original REFRESH_TOKEN_EXPIRY bridge lifetime to the short recovery grace period. Remove the now-unused explicit bridge delete path from the service and data-schemas method surface. Add regression coverage for grace re-store, identity symmetry, retry failure behavior, and same-key upsert replacement. * 🛠️ fix: fail closed on OBO MCP user identity mismatch Add an OBO-specific guard before MCP tool execution that requires the effective invocation user and captured request user to both have ids and to match. This prevents OBO tool calls from falling back to a separate configurable.user_id identity after request-bound OBO context has already been captured. Keep the existing user id fallback behavior for non-OBO MCP calls. Tests cover mismatched OBO users, missing user ids, and the matching-user path ignoring a conflicting configurable.user_id. * 🛠️ fix: Guard OpenID bridge retry user identity Extract the shared OpenID refresh/user-resolution flow in AuthController so the normal refresh path and bridge-recovery retry use the same grant, claims, issuer, user lookup, and diagnostic logging code. Preserve the existing path-specific behavior: the normal path still owns migration updates and 401 login redirects, while the bridge retry still falls through to the existing 403 invalid-token response. Add a bridge-recovery guard that rejects retry results whose resolved user id differs from the signed openid_user_id cookie before issuing tokens or re-storing the grace bridge. Cover both the successful matching-user recovery and the mismatched-user rejection. * 🛠️ fix: type-safety polish on OBO data layer Replace refresh token bridge query/update Record<string, unknown> usage with typed Mongoose FilterQuery and UpdateQuery definitions. Harden OpenID marker cookie JWT expiry handling by converting refresh expiry milliseconds to integer seconds and rejecting invalid or non-positive durations. Add focused CSRF tests for fractional refresh expiry values and invalid expiry configuration. * 🛠️ fix: Bind OpenID session tokens to authenticated identity Stamp OpenID session token state with the LibreChat user id, OpenID subject, tenant id, and normalized issuer when tokens are stored. Fail closed before OBO inline token reuse/refresh when the session token identity does not match the current authenticated identity, preventing a stale or mixed Express session from supplying another user's upstream assertion. Also validate the normal /api/auth/refresh session-token reuse shortcut against the signed marker-cookie user before returning cached session tokens. Note: sessions created before this change carry no identity stamp and are treated as a mismatch. This is self-healing — the reuse path forces a full IdP refresh (which re-stamps the session) and the OBO path throws, surfacing as a one-time re-authentication for active OBO users at deploy time. The session re-stamps within one session lifetime (SESSION_EXPIRY, default 15 min). * 🛠️ fix: Recover OpenID refresh token drift Prefer the browser refresh-token cookie when it differs from the server-side OpenID session state, and force a real IdP refresh in that case instead of reusing stale session tokens. Store a short-lived refresh-token bridge when inline OBO refresh writes a rotated browser cookie but session persistence fails, so follow-up refreshes can still recover from the old token. Keep the bridge grace TTL centralized in RefreshTokenBridge so both recovery paths use the same env-backed value. Note: drift is measured against the last-synced browserRefreshToken marker, so the SSE path (intentionally stale cookie, authoritative session) does not false-positive. Sessions predating the marker have no browserRefreshToken; for those, drift falls back to comparing the cookie against the session refresh token and prefers the cookie on difference. This is the same self-healing pre-change-session window as the identity binding fix and re-syncs within one session lifetime. Tests cover cookie/session drift selection, reusable-session bypass on drift, bridge storage after session-save failure, and the shared bridge constant wiring. * 🛠️ fix: Harden OBO token caching and expiry handling Reject malformed OBO grant responses before writing them to the exchanged-token cache so a missing access_token cannot poison the cache. Store absolute expires_at values with cached OBO tokens and ignore legacy cache entries without usable expiry metadata. This keeps cached-token freshness based on the token’s real remaining lifetime instead of reusing the original relative expires_in on cache hits. Move OBO expiry normalization and skew helpers into packages/api and use them from both the JS exchange service and the TS MCP resolver. Apply a 30-second safety margin with a one- second floor for short-lived tokens, covered by direct helper tests and caller-level regression tests. Tests: - packages/api: npm run build - packages/api: npx jest src/mcp/oauth/expiry.spec.ts src/mcp/oauth/obo.spec.ts - api: npx jest server/services/OboTokenService.spec.js * 🛠️ fix: Harden OBO refresh-token bridge lookup and indexing Reuse getValidOpenIDReuseUserId for the bridge-recovery user lookup in refreshController instead of re-verifying openid_user_id inline. The shared helper enforces the JWT_REFRESH_SECRET presence check and a strict typeof payload.id === 'string' guard, rejecting tokens whose id claim is present but not a string (e.g. a numeric id) that the inline check accepted. Fail closed on issuer mismatch in getRefreshTokenBridge. Both the stored and the expected issuer are now normalized and compared for equality, so a bridge is recovered only when both sides agree (both absent, or both present and equal after normalization). Previously the check was skipped whenever the stored issuer was absent, allowing recovery across mismatched issuer context. Drop the unused {oldRefreshTokenHash, userId, tenantId, openidIssuer} index and the openidIssuer field on RefreshTokenBridgeQuery. The data-layer filter only queries the 3-field {oldRefreshTokenHash, userId, tenantId} index; the issuer is verified in application code, not the query. Hoist the repeated model accessor into getRefreshTokenBridgeModel. Note: issuer is now load-bearing for recovery. A bridge stored with an issuer recovers only when the lookup supplies a matching issuer; the recovery lookup reads user.openidIssuer via AUTH_REFRESH_USER_PROJECTION (an exclusion projection that retains the field). If a user's persisted openidIssuer is empty while the stored bridge has one, recovery fails closed (falls through to normal re-authentication) until the bridge TTLs out — no security regression. Tests cover invalid signed-cookie payloads bypassing the bridge, both asymmetric issuer-presence cases, issuer normalization before comparison, and an index-alignment assertion guarding against re-adding the dropped index. * 🛠️ fix: Degrade OBO discovery on token resolution failures Catch expected OboTokenResolutionError failures during MCP tool discovery and fall back to unauthenticated tool listing instead of aborting discovery. This keeps discovery aligned with the existing unauthenticated listing behavior while preserving unexpected errors as real failures. Also correct OBO tool-call freshness comment and tighten the OBO trust-check permissions type to the existing role permission shape. Tests: - npx jest src/mcp/__tests__/MCPConnectionFactory.test.ts --runInBand --coverage=false - npx jest src/mcp/oauth/obo.spec.ts --runInBand --coverage=false * 🛠️ fix: tighten OBO tool-call errors, bridge logging, and flight typing Move resolveToolCallUserId inside the tool-call try/catch so an OBO identity mismatch surfaces with serverName/toolName context and the standard tool-call-failed message instead of an opaque bare Error. Raise the refresh-token bridge lookup failure log from debug to warn so transient infrastructure failures on the unauthenticated /api/auth/refresh path are observable, and guard the message access against non-Error values. Replace the unknown+cast in isDuplicateKeyError with a hasErrorCode type predicate so the duplicate-key check reads error.code without an assertion. Preserve real math/isEnabled in the MCPConnectionFactory test mock (mock only processMCPEnv) so mcpConfig timeouts no longer resolve to NaN, fixing the TimeoutNaNWarning that masked slow OAuth retry behavior. * 🧪 fix: Restore the Flight Uniqueness Index and Buffer the Graph Cache TTL Two CI failures on the merge, both in suites this environment cannot run (their MongoDB binary download is blocked). `GraphApiService.spec.js` still asserted the unbuffered TTL. Graph tokens route through the same `getTokenCacheTtlMs` as the OBO and openidStrategy caches, so the entry now expires 30s before the credential does. `openidRefreshFlight.spec.ts` dropped the database between tests, which takes the indexes with it, and Mongoose builds them only once when the model is compiled. Whether the unique `key` index survived into a test was a race with that one-time build. Without it a second `create` inserts instead of raising a duplicate-key error, so every worker believes it won the flight — the mutual exclusion the file exists to prove. Indexes are now rebuilt after each drop, which also makes the reclaim and complete cases reach those paths for the right reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * fix: address OBO review findings * 🔐 fix: Install Bridge Indexes and Carry OBO Through Assistant Recovery Two findings from the Codex pass on d08c82f0d. The refresh-token bridge relied on Mongoose auto-indexing for both of its indexes, and `MONGO_AUTO_INDEX=false` is a supported deployment setting. A bridge holds an encrypted refresh token and the TTL index is the only thing that ever deletes one, so under that setting they would accumulate for the life of the collection while concurrent upserts lost the compound uniqueness the filter assumes. Installed before the first write, matching the flight methods and the session and schedule methods before them. `recoverServerTools`, the assistant create/update path that reruns `reinitMCPServer` when a referenced server's catalog and connection snapshot are both missing, was the last reinit site not carrying the upstream-token closure. For an OBO server the factory rejects the connection outright, so the assistant write failed with unavailable MCP definitions. It now builds the provider at that request boundary like the other entry points; assistant writes have no `res`, so a rotation there falls back to the recovery bridge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * fix: harden OBO refresh coordination * ⏳ fix: Keep Elapsed Expiries Elapsed and Revoke the Superseded Session Two of the five findings from the Codex pass on fcdc15885 — the two that are defects in code this branch introduced rather than design questions about the bridge. `getSkewedTokenExpiresAtMs` floored every result at a second in the future, including an expiry the provider had already declared elapsed. An exchange answering `expires_in: 0` or a past `expires_at` was handed to the MCP connection stamped valid for another second, which only moves the failure downstream. The floor now applies to a lifetime that is still live, which is what it was for; an elapsed one stays elapsed so the caller rejects it. Same for the cache TTL, which falls back to the elapsed-credential floor. Bridge recovery left the stale token's durable Session behind. Only the token it recovered through was passed as `existingRefreshToken`, so that one's session was replaced while the token the browser actually presented kept its record until its original expiry. That record, with the marker cookie still bound to it, is what authorizes local image access for OpenID users — so a copy of the stale cookie outlived the rotation it had lost. Revoked explicitly on successful recovery. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * fix: close OBO refresh review findings * 🎟️ fix: Carry the Bridged Token Through a Non-Rotating Recovery Bridge recovery passes the browser's stale token as `existingRefreshToken` so the durable Session naming it is the record replaced. That also makes the stale token the fallback the installed session and the refresh cookie use when a tokenset carries no `refresh_token` of its own, which holds only while the recovery grant rotates. An IdP that answers that grant without rotating sends the browser back to the very token the bridge exists to retire: `storeOpenIDSession` installs and deletes the same stale record in one call, and the cookie is rewritten to a token the IdP already rejected — a sign-out on the next refresh. The grace bridge one line above already guards this with `|| bridgedRefreshToken`; the resolved tokenset now does the same, so leader and followers alike publish the recovered token. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * 🚫 fix: Reject an OBO Exchange That Returns an Expired Credential Preserving an elapsed expiry through the skew helper only helps if something acts on it, and nothing did: `MCPManager.callTool` checks the access token and nothing else before setting the Authorization header, so a credential the IdP declared spent still went downstream to fail there. It is rejected at the exchange now, where the reason is known, and retryably — the exchange itself worked, so a fresh grant can succeed. Completes the elapsed-expiry change in cb26a6f7d, which made the stamp honest without giving anyone a reason to look at it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * fix: close OpenID refresh review findings * fix: coordinate OpenID refresh entry points * chore: sort OpenID flight imports * fix: fence OpenID refreshes during logout * fix: close OpenID logout publication races * fix: narrow completed refresh flight * test: cover bridge cleanup failure after ownership loss The compensating delete in storeRefreshTokenBridgeWithLease swallows its own failure so the lease error stays the one the caller sees. Nothing asserted that, so removing the inner catch left every suite green while callers began receiving the cleanup error instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * fix: compensate OpenID bridges only on proven ownership loss The post-write lease assertion deletes the bridge it just published when it throws, but it threw for two different reasons: a coordination record that is no longer ours, and a coordination read that simply failed. Treating the second as the first destroys the only mapping from the token the browser still holds to the one the IdP already rotated to, so a transient Mongo error on the headers-already-sent path signed the user out. Tag the ownership error where the lease raises it and compensate only for that, preserving the bridge whenever ownership is merely undetermined. A preserved bridge stays behind the logout revocation fence, so the safe default costs nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * fix: reject spent OpenID refresh results Two ways a refresh could report success while handing back a credential nothing can use. An inline refresh carries the previous id_token forward when the IdP omits one on rotation, so tokenset.id_token is not necessarily freshly issued. setOpenIDAuthTokens applied its freshness guard only to the session copy and took tokenset.id_token unconditionally, so /refresh returned an expired bearer even though the grant produced a usable access token. Skip it only when it is provably expired: an id_token whose expiry cannot be read stays preferred, since access_token may be opaque or scoped to another audience. normalizeExpiresIn preserves a zero or negative lifetime rather than discarding it, so a grant declaring an already-spent access token still published, rotating the refresh token and returning a token every freshness check rejects. Each OBO call then repeated the grant. Reject an elapsed lifetime before publishing; an unknown lifetime still publishes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * fix: harden OpenID refresh publication * fix: keep identity and results intact through OpenID refresh cleanup Two follow-ons from the last round's fixes. Stripping an expired carried-forward id_token from the refresh result removed the only identity material a rotation without id_token leaves behind. The result is rebuilt by buildOIDCTokensFromSession, so it carries no provider claims() either, and getTokenClaims accepts only those two — bridge recovery failed with "no usable identity claims" before setOpenIDAuthTokens could hand back the fresh access token. The stripped token now travels in a non-enumerable marker, alongside the existing browser and predecessor markers, which identity resolution reads and the authentication response never sees. The lease drained a pending renewal by awaiting it in finally, so a transient coordination failure there threw from finally and replaced the operation's result. The refresh had already settled and published, so the caller saw a failure on credentials that had rotated. Proven ownership loss is recorded on ownershipLost and checked before the return, so the drain has nothing to add but noise; it now absorbs and logs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F * refactor: fence OpenID recovery publication * fix: close OpenID publication transaction * refactor: make OpenID publication transactional * fix: satisfy OpenID publication type checks * fix: fence OpenID session publication * fix: authorize OpenID refresh publication * fix: bind OpenID replay generations * fix: fence OpenID response generations * fix: authorize OpenID token delivery * fix: linearize OpenID publication delivery * chore: sort OpenID refresh flight imports --------- Co-authored-by: J.C. Bartle <jcbartle@users.noreply.github.com> Co-authored-by: jbartle <jbartle@rand.org> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: jcbartle <7274202+jcbartle@users.noreply.github.com>	2026-08-29 23:55:58 -04:00
Dustin Healy	f02ce63d57	✂️ fix: Strip Redundant Server-Name Prefixes from MCP Tool Keys (#14732) ... * ✂️ fix: Strip Redundant Server-Name Prefixes from MCP Tool Keys MCP servers that prefix every tool with their own name produce model-facing keys that embed the server twice once the _mcp_<server> suffix is appended, pushing long tool names past provider 64-character function-name limits. Tool keys now drop a leading <normalizedServerName>_ prefix (case-insensitive, skipped when a sibling tool already owns the stripped name). The original upstream name is recorded as serverToolName on the cached definition and is always what tool calls send to the server, and runtime lookups also try the stripped spelling of persisted pre-strip keys so existing agents keep resolving. * 🩹 fix: Keep Stripped MCP Tool Keys Provider-Safe and Collision-Free Assistant writers submit catalog entries verbatim, so the internal serverToolName mapping is now removed from provider-facing definitions before they reach create/update payloads. Prefix stripping is collision-guarded over the resulting name set rather than raw siblings only, which also covers case-variant prefixed pairs under the case-insensitive match. Assistant payload healing now rewrites a pre-strip persisted key to the stripped catalog key when that key actually exists in the loaded definitions, and legacy agent references keep their persisted spelling as the runtime instance name so per-tool options stay applied while the upstream call still uses the matched entry's raw name. * 🧷 fix: Harden Stripped MCP Tool Keys Against Heal, Collision, and Cache Edges The pre-strip heal now resolves the key boundary against both raw and normalized server spellings, mapping back to the raw name for the shadow and membership guards, so keys persisted after server-name normalization heal too. Collision detection iterates to a fixpoint so a fallback to a raw name cannot silently collide with another sibling's stripped result, and a stripped remainder equal to a synthetic marker (wildcard or server pin) is never produced. MCP catalog cache slices are versioned so replicas that predate serverToolName never read stripped entries during a rolling deploy; stale slices expire on their own. * 🔎 fix: Resolve Pre-Strip Keys in Event-Driven Definitions and Reinspect Persisted Catalogs The event-driven definitions loader now tries the stripped spelling of a persisted key when the exact lookup misses, keeping the persisted name so it matches the runtime instance, which stops legacy agents from failing initialization with expected tools unavailable. The registry storage schema version is bumped so followers rebuild persisted toolFunctions instead of republishing pre-strip definitions into the versioned catalog namespace. The assistants heal also fails closed when a normalized-suffix reference lands on a contested server-name slot, since rewriting persisted data must not bind an ambiguous reference to the tie-break winner. * 🛰️ fix: Reserve the Synthetic OAuth Name and Heal User-Owned Server Keys A stripped remainder equal to oauth would make the client stream handlers treat a real tool call as a synthetic authentication prompt, so it joins the reserved remainders alongside the wildcard and pin markers. The assistants heal now audits the FULL accessible server set on every run instead of operator config names only, since assistants reference user-owned servers whose catalogs the definitions loader already resolves; an unavailable audit still skips healing entirely. * 🧬 fix: Verify Upstream Identity for Legacy Keys and Reserve Sibling Raw Names Stripped results now reserve every sibling's raw name even when that sibling itself strips, so a stripped key can never shadow another tool's pre-rollout persisted references within the same snapshot. Every legacy fallback (runtime lookup, event-driven definitions, assistants heal) accepts a stripped-spelling match only when the entry's recorded serverToolName proves the same upstream tool, so a stale key for a removed tool degrades to unavailable instead of calling a different sibling. To keep that identity visible to the heal, assistant tool definitions retain serverToolName and the controllers sanitize entries through toProviderToolDefinition at the provider submission boundary instead. The agent editor migrates pre-strip persisted ids the same identity-verified way, with the upstream name exposed on the MCP tools payload. * 🧭 fix: Heal Wildcard Tool Options and Reserve the OAuth Namespace Wildcard-expanded catalogs rename stripped tools without any agent.tools entry to preserve the spelling, so buildToolClassification now aliases persisted pre-strip tool_options keys onto the current instance names in place, identity-gated on the definition's recorded upstream name and never overriding an explicit entry. Both loading modes flow through it: instances carry mcpServerToolName from createToolInstance and event-driven definitions thread serverToolName from the catalog. stripServerNamePrefix also reserves the entire oauth namespace rather than the exact name, since the client stream handlers classify every oauth-prefixed key as a synthetic authentication call. * 🛡️ fix: Derive the Full Reserved Namespace and Heal Approval Policies The reservation guard now covers every namespace consumers classify by prefix: the wildcard and server-pin markers alongside oauth, plus the server-scoped mcp_ pluginKey namespace that pre-strip keys could never enter. Stripping also never produces a key whose isActionTool classification differs from the raw key's, since a server whose normalized name contains _action_ would otherwise see a real MCP tool routed down the OpenAPI action path past MCP authorization. Admin toolApproval globs written against upstream tool naming keep applying: pattern lists are healed at run wiring with the current names of tools whose pre-strip spelling matches, list-level so deny, ask, and allow precedence is unchanged and a non-matching deny can no longer fail open. The MCP tools wire type also declares serverToolName end to end. * 🪪 fix: Alias Both Key Spellings for Approval Policies and Hook Matchers Identity aliases are now collected once at tool classification, in both directions: a stripped instance aliases its pre-strip spelling and a legacy-named instance aliases its current catalog spelling, with the current name recorded on legacy matches by the runtime lookup and the event-driven definitions loader alike. The aliases ride the agent config through both loading modes, so approval pattern healing applies to deny rules written against either spelling, closing the bypass where a rule targeting the current name missed an unedited agent's legacy instance. Programmatic approval hook matchers get the same treatment: each hook is additionally registered under an anchored exact-name pattern for tools whose other spelling its regex matches, keeping the admin's matcher semantics intact while argument, user, and tenant specific deny or ask decisions keep executing for renamed tools. * 🔁 fix: Alias Tool Options in Both Spelling Directions Options aliasing now consumes the same bidirectional alias pairs as policy healing and hook matchers, so options the editor migrated to the current catalog spelling still reach a legacy-named instance retained by an unedited agent.tools entry. The previous serverToolName-only derivation skipped exactly that case since the legacy key equals the instance name there. * 🤝 fix: Reserve the Agent Handoff Namespace Before Stripping The client renders any lc_transfer_to_ prefixed call as an agent handoff and the background and intent passes exclude such names, so a stripped remainder inside that namespace would misclassify a real upstream tool. It joins the mcp_ pluginKey namespace as a bare-prefix reservation, which pre-strip keys could never enter. * ⚡ fix: Reuse the Loader's Server Snapshot and Index the Editor Catalog getAssistantToolDefinitions now returns the accessible-server snapshot from the same merged registry read that resolved the catalogs, and the heal consumes it instead of repeating the app-config and registry round trips on the assistant write path; without a snapshot the heal still fetches and fails closed as before. The agent editor's id migration uses a memoized tool_id map, so the per-key form heal does constant-time lookups instead of scanning the catalog per option. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-08-21 14:30:29 -04:00
Danny Avila	1bccc2bc18	📡 fix: Refresh MCP Tools After List-Changed Notifications (#14686) ... * fix(mcp): handle dynamic tool list changes Co-authored-by: Pascal Garber <pascal@artandcode.studio> * test(mcp): fix CI validation * fix(mcp): keep dynamic tool catalogs live * fix(mcp): harden dynamic catalog lifecycle * test(mcp): use typed startup connection * test(mcp): isolate dynamic e2e fixtures * fix(mcp): refresh tools after reconnect * fix(mcp): close dynamic catalog cache gaps * test(mcp): update OAuth connection mocks * fix(mcp): preserve app snapshot ownership * style(mcp): sort connection imports * fix(mcp): close review race conditions * fix(mcp): preserve cache ownership edges * fix(mcp): harden recovery lifecycle * fix(mcp): guard tool-less app refresh * fix(mcp): fence distributed cache races * fix(mcp): retire stale connection state * fix(mcp): keep tool snapshots authoritative * fix(mcp): fence stale app tool publications * style(mcp): sort repository test imports * test(mcp): mock empty startup publication * fix(mcp): preserve app publication generations * fix(mcp): harden publication recovery races * fix(mcp): address tool catalogs by runtime config * fix(mcp): load scoped catalogs for assistant writes * fix(mcp): harden catalog publication recovery * fix(mcp): serialize forced connection replacement * fix(mcp): serialize ordinary creation with replacements * fix(mcp): harden catalog fallback boundaries * fix(mcp): close lifecycle fencing gaps * fix(mcp): preserve catalog authority on failures * fix(mcp): compensate failed catalog mutations * fix(mcp): fence catalog refresh ordering * style(mcp): sort agent loader imports * fix(mcp): cancel stale connection creation * fix(mcp): fence catalog coordination * fix(mcp): close catalog race windows * fix(mcp): harden cross-pod catalog fencing * fix(mcp): close catalog lifecycle edges * style(mcp): sort assistant imports * fix(mcp): reject stale recovery authority * fix(mcp): restore static catalog on every startup * fix(mcp): order app catalog publications * style(mcp): sort catalog revision imports * fix(mcp): separate catalog allocation and commit fences --------- Co-authored-by: Pascal Garber <pascal@artandcode.studio>	2026-08-08 13:50:21 -04:00
Danny Avila	6f45a9e32e	🔗 fix: Normalize MCP Tool Keys at Every Producer, Resolve Raw Names via Aliases (#14553) ... * 🔗 fix: Normalize MCP Tool Keys at Every Producer, Resolve Raw Names via Aliases Tool keys had two spellings that could diverge for any server whose name contains characters outside [a-zA-Z0-9_.-]: the tool cache (and registry inspector) built keys with the RAW server name, while runtime instances are named with normalizeServerName(serverName). Three code comments already asserted "tool keys embed the normalized server name" - no producer honored it. For a special-character server that meant: - definitions-only mode shipped raw def names the model echoed back, but the executor's tool map held the normalized instance name, so every call failed with "Tool not found"; - per-tool tool_options (defer_loading / allowed_callers / run_in_background / describe_intent) were persisted under raw keys that never matched the definition names the option passes resolve against, so builder settings were silently inert; - tool-key parsing against normalized candidate lists silently fell back to last-delimiter splitting, which mis-parses delimiter-bearing tool names. The reconciliation is one contract enforced in three moves: 1. PRODUCERS NORMALIZE. The tool cache (packages/api/src/mcp/tools.ts) and the registry inspector build keys with the normalized server name, matching the instance names MCP.js has always assigned. The builder's tool ids, agent.tools entries, tool_options keys, and definition names all flow from these keys, so every model-facing name now agrees. The cache STORE stays keyed by the raw config name. 2. CONFIG LOOKUPS RESOLVE ALIASES. New shared helpers in data-provider (buildServerNameAliases, normalizeMCPToolKey) map a parsed normalized name back to the raw config name that the registry, config maps, tool cache, and plugin-auth rows are keyed by. Applied in the definitions loader closure, handleTools grouping, createMCPTool's parsing fallback, getUserMCPAuthMap, and the MCP tools endpoint - matching both spellings so legacy raw keys keep resolving. 3. LEGACY DATA HEALS AT ONE BOUNDARY. initializeAgent rewrites raw-keyed agent.tools entries and tool_options keys to the normalized form (normalizeAgentToolKeys) before anything consumes them, so agents persisted under the old convention load their tools AND have all four per-tool options honored. Placeholder and server-pin tokens stay raw - they are config-identity references, not model-facing names. Servers whose names are already in the safe character set (the common case) produce byte-identical keys before and after; the fast path allocates nothing. Stale Redis-cached raw keys self-heal via the existing reconnect-on-missing path within one cache cycle. * 🧯 fix: Deterministic Alias Collisions + Raw Names in Definition Metadata Two review findings on the normalization contract: - Two configured server names that normalize to the same segment (e.g. 'Sales Force' and 'Sales:Force' -> 'Sales_Force') produce inherently ambiguous tool keys; the alias map silently resolved last-wins, so a tool selected from one server could execute against the other's config. buildServerNameAliases now resolves collisions to the FIRST configured name deterministically, and resolveMCPServerContext warns once per colliding pair per process so the operator can rename one server. A collision-resistant identifier would change every existing tool key, so detection + stable routing is the right treatment here; startup-time config validation can follow separately. - The definitions loader resolved parsed (normalized) server names to raw only inside the ToolService closure, while the definition metadata (serverName -> mcpRawServerName) kept the normalized value. Server instructions are keyed by raw config names, so a special-character server's instructions were silently omitted in definitions-only mode. loadToolDefinitions now takes rawServerNames, resolves the boundary against both spellings, and stores the RAW name in definition metadata - consistent with the instance path. * 🧯 fix: Heal Stale Caches, Skill Allowed-Tools, and Builder Selectors Three review findings on the normalization rollout, all in the transition class: - Stale cache entries (P1): the definitions-only loader treats the per-server tool map as authoritative and never reconnects on a per-key miss, so a pre-change raw-keyed Redis entry would make a special-character server's tools vanish for up to the cache TTL. getMCPServerTools now heals legacy raw-keyed entries to the normalized format at read time (keys and function names), covering every consumer with no coordinated invalidation; safe names return the map untouched. - Skill allowed-tools: a skill declaring a raw MCP key in allowed-tools bypassed the initialize-boundary heal (the union runs after it) and would neither dedupe against healed agent tools nor match the normalized tool map. The primes' allowedTools now pass through the same normalizeAgentToolKeys heal before unioning. - Builder selectors: matchesMcpServer and useVisibleTools parsed tool ids against raw server names only, so an attached special-character server rendered as an unselected orphan card. Both now accept the normalized spelling and resolve it back to the raw map key, keeping legacy raw ids working. * 🧯 fix: Fail Closed on Normalized Server-Name Collisions Escalation of the collision finding: a deterministic first-wins alias plus a warning still let the tools listing publish BOTH colliding servers, so a tool selected under the shadowed second server would silently execute against the first server's configuration (their model-facing keys are identical, so routing cannot ever distinguish them). - findShadowedServerNames identifies later-configured names whose normalized form an earlier different name claimed. - getMCPTools excludes shadowed servers from the published listing entirely (with a warn naming the collision), so their tools are never selectable - nothing ambiguous can be picked. - Server creation reserves both spellings: a generated slug may not collide with a raw config name OR the normalized form its tool keys would carry. Collision-resistant model-facing IDs remain out of scope: changing normalizeServerName's output would rewrite every existing tool key (agent documents, caches, instance names) for ALL servers to handle a misconfiguration that is now blocked from exposure instead. * ✅ fix: Dedupe Reserved Server-Name Spellings at Creation The reservation list appended normalized forms unconditionally, which duplicated every safe name (raw === normalized) and broke the route-level contract test pinning the exact list. Dedupe via a Set so safe names contribute one entry, while special-character names still reserve both spellings; adds the special-character reservation case. * 🧯 fix: Never Heal a Shadowed Server's Keys; Align Authorization Tie-Break Persisted references were the remaining collision vector: an agent or skill saved with the shadowed later server's raw key was HEALED into the shared normalized key, authorized through a last-wins map, and routed first-wins - authorized as one server, executed as another. - normalizeAgentToolKeys now refuses to rewrite keys of shadowed servers (findShadowedServerNames): rewriting would produce exactly the first server's key. Left raw, the key cannot match the normalized-keyed tool map and the tool fails visibly - broken beats misrouted. Covers agent.tools, tool_options, and skill allowed-tools through the shared heal. - filterAuthorizedTools (agents/v1.js) builds its normalized-to-raw map via the shared buildServerNameAliases instead of a last-wins Map constructor, so authorization resolves a colliding key to the SAME first server execution routes to. * 🧯 fix: Direct Identity Wins Over Aliases; Heal Client Forms and Degraded Contexts Four review findings on the normalization edges: - Alias hijack (P1): a user-DB server named exactly like an operator server's normalized form ('foo' vs YAML 'foo!') had its tools rerouted to the operator server by unconditional alias resolution. Resolution is now DIRECT-FIRST everywhere: the parsed name is tried as-is, and only when nothing resolves is it treated as a normalized spelling (definitions loader, handleTools grouping, createMCPTool fallback). buildServerNameAliases seats identity entries before derived ones so a literal name owns its slot regardless of config order, findShadowedServerNames and the collision warning derive from the same construction, and getUserMCPAuthMap fetches auth under both spellings so either owner finds its rows. - Builder double-match: a normalized name containing the delimiter ('foo mcp bar' -> 'foo_mcp_bar') also suffix-matched a server named 'bar', selecting both cards and making removal strip the wrong tool. matchesMcpServer now resolves the token ONCE against the full configured list (longest boundary, both spellings) when the caller supplies it; selection and removal share the resolution. - Builder legacy ids: an agent saved with raw-keyed ids showed its tools unchecked while the runtime heal kept them active, and selection updates never replaced the legacy entries. McpSection maps legacy raw ids to their current normalized ids when deriving and rewriting this server's selection. - Degraded context: a transient ensureConfigServers failure returned an entirely empty context, leaving normalized keys unresolvable for the request. resolveMCPServerContext now keeps the name lists (they derive from the config snapshot alone) and degrades only the lazy-init configs. * 🧯 fix: Collision Detection Sees Accessible Servers; Shadowed Refs Fail Closed End to End Round follow-ups on the collision design, all in the DB-server-visibility class: - The legacy-key heal detected collisions against operator-config names only, so healing could still produce a key that direct-first resolution routes to an invisible user-DB server. initializeAgent gains an optional getAccessibleMcpServerNames dep (wired through ToolService for controllers that mock it, directly elsewhere), consulted ONLY when a configured name needs normalization - zero cost for safe-name deployments. The heal then sees the full accessible set and skips shadowed servers' keys. - Wildcard and legacy raw tokens bypassed catalog filtering, letting a shadowed server's instances join a run under the same normalized names as the winner's. filterAuthorizedTools rejects tools of shadowed servers at authorization (its merged map sees DB + config), and handleTools skips them at execution. - The builder migrated only tool selection, not tool_options: legacy raw option keys showed disabled while the runtime honored them, and toggles could not clear them. McpSection now migrates option keys to the current normalized ids (existing normalized entries win). - A transient ensureConfigServers failure degraded to an EMPTY server context, leaving normalized keys unresolvable for the request. resolveMCPServerContext keeps the name lists (derived from the config snapshot alone) and degrades only the lazy-init configs. * 🧯 fix: Complete the Collision Audit at Every Gate; Safer Heal Semantics Round follow-ups hardening the collision audit: - Execution guards now consult the FULL accessible set: the caller's heal threads its already-fetched names through loadTools, and handleTools fetches them itself when a configured name needs normalization (never for safe-name deployments) - so a cross-tier collision (user-DB 'foo' vs operator 'foo!') fails closed at eager execution instead of joining the run under one normalized name. - Healing is SKIPPED when the collision audit cannot complete (transient lookup failure, or no dep): un-healed raw keys still resolve through the direct-first candidates, so skipping is safe while rewriting against an incomplete audit is not. - The audit lookup is gated on the agent actually carrying delimiter-bearing keys (tools, tool_options, or skill allowed-tools), so non-MCP agents never pay a registry round-trip even on specially named deployments. - normalizeAgentToolKeys gives the CURRENT (normalized) entry precedence when both spellings carry options, matching the builder's migration semantics instead of letting insertion order decide. - The builder's toCurrentToolId resolves entries boundary-exactly against every configured server (longest match, both spellings), so a raw suffix shared with a LONGER server name can no longer reassign that server's selection or options while another dialog is open. * 🧯 fix: Shared Collision Audit for Definitions Loading; Fail Closed on Audit Failure Round follow-ups closing the remaining audit gaps: - The definitions-only loader now consumes the same collision audit as eager loading: shadowed servers' entries (wildcards included) are dropped before definitions are emitted, so the default execution path can never resolve a shadowed server's normalized function name to another server. The audit names thread from initializeAgent's heal; the loader self-fetches only when a configured name needs normalization. - resolveCollisionAuditNames centralizes the audit-resolution policy (threaded set > self-fetch when needed > incomplete on failure), and BOTH loaders now fail closed under an incomplete audit: any normalization-sensitive reference (its own name needs normalizing, or it equals the normalized form of a configured special-character name) is skipped with a warning instead of being audited against operator names alone. isNormalizationSensitiveName lives in packages/api as a pure helper so test mocks use the real predicate. - normalizeAgentToolKeys collapses duplicate ids after healing (order-preserving): a document carrying both spellings converges on one key, never two instances with the same function name. * 🧯 fix: Thread the Audit Everywhere; Identity-Aware Alias Fallback Round follow-ups on audit plumbing: - The OpenAI-compatible and Responses tool loaders now forward the already-resolved accessibleMcpServerNames instead of discarding it, so the definitions loader neither repeats the registry lookup nor fails closed on a transient second lookup after the first succeeded. - The skill-only path threads its audit: when the baseline agent has no MCP keys but a primed skill's allowed-tools fetched the complete set, that set (not the operator-only list) reaches the loader, so the collision remains visible and the shadowed reference stays rejected end to end. - OAuth discovery iterates the collision-FILTERED tool list, so a request can no longer emit an OAuth prompt, wait out the connection timeout, and reconnect a server whose definitions were deliberately rejected. - The definitions loader's alias fallback is identity-aware: when the parsed name IS a known accessible server, a null tool fetch means temporarily unavailable (OAuth pending, missing user variables, disconnected) and no longer reroutes to the raw alias - previously the aliased operator server's definitions could be emitted under the unavailable DB server's names. * 🧯 fix: Legacy-Key Definition Lookup; Retain Audit for Deferred Execution - createMCPTool resolves tool definitions by BOTH spellings: the key as persisted plus the canonical normalized key built from the resolved server name. Assistants and direct tool calls persisted before the rollout bypass the agent-boundary heal and arrive with raw keys, while availableTools is now indexed canonically - previously every such call missed the index, burned a reconnect, and returned the unavailable stub permanently via the negative cache. - The initialized agent retains accessibleMcpServerNames (the COMPLETE collision audit this initialization resolved), buildAgentToolContext copies it into every per-agent tool context, and loadToolsForExecution threads it into the eager loader as bare options. Deferred/event-driven execution therefore reuses the snapshot instead of repeating the merged registry read - a transient failure there could fail-closed a tool the same turn already advertised from the successful first audit. - MCP.spec.js keeps @librechat/api pure helpers REAL (requireActual spread) so normalization paths are exercised rather than mirrored. * 🧯 fix: Parse Legacy Keys Against Both Server-Name Spellings createMCPTool's boundary candidates were normalized-only, so a legacy raw key whose server name contains the delimiter (foo_mcp_bar!) missed the suffix match and fell to the generic last-delimiter split - the canonical rebuild then produced a key that could never hit the index and the persisted call stubbed out. The candidate list now carries the RAW resolved name (and raw config names on the parse-only path) next to the normalized spellings. * 🧯 fix: Honest Audit Completeness; Shadowed-Server Form-Key Guard - resolveAllMcpConfigs tolerates ensureConfigServers failures, so the merged registry read can silently omit config-only servers while the audit still reported complete: true - a foo/foo! collision would go unseen and a persisted key could route to the wrong server. Both audit consumers now union the snapshot-derived raw config names back in (resolveCollisionAuditNames unions the caller's rawServerNames; the initializeAgent heal unions configRawServerNames), keeping the completeness label honest without an extra read: operator names come from the registry-independent config snapshot, user-DB names from the merged read that fails loudly into the existing incomplete path. - The client tool_options migration now mirrors the runtime heal's fail-closed rule for SHADOWED servers: when the dialog's server has lost its normalized slot to another catalog name, legacy raw keys stay raw instead of being rewritten onto the winning server's key, where a later save would apply the wrong server's per-tool settings. The dialog's own server joins the alias construction so a stale catalog map can't misread as a collision. * 🧯 fix: Heal Legacy Assistant MCP Tool Names on Save The assistants create/update controllers look tools up in the cached definitions by exact key, and the cache is now normalized-keyed - an assistant saved before the convention resubmits its raw-suffixed MCP name on every edit, so any save silently removed the tool. healMcpToolNames pre-heals the payload's tool list: a delimiter-bearing string that misses the cache resolves through the configured raw names (longest-suffix, boundary-exact) and rewrites to the normalized key only when that key actually exists in the cache. SHADOWED raw names stay raw and fail closed, mirroring the runtime heal; the config read happens only when a delimiter-bearing name actually misses, and read failures propagate (write path) rather than silently dropping tools. v2's update loop also stops re-reading the tool cache per iteration. * 🧯 fix: Full-Audit Shadow Set + Dedupe in the Assistant Key Heal - The assistant-save heal built its shadow set from operator config names alone, so a cross-tier collision (user-DB `foo` owning the normalized slot of operator `foo!`) looked unshadowed and the legacy key healed into the shared normalized name - which direct-first execution then binds to the DB server. The shadow set now comes from resolveCollisionAuditNames' full accessible audit, and an incomplete audit skips healing outright (every rewrite candidate is normalization-sensitive by construction, so raw-and-fail-closed is the only safe answer). - Healed string entries dedupe order-preserving: a payload carrying both spellings of the same tool collapses to one entry instead of expanding into duplicate function definitions the provider rejects.	2026-08-01 07:39:24 -04:00
Danny Avila	250aca375a	🔗 fix: Resolve MCP Tool-Key Boundary Against Configured Server Names (#14448) ... * fix: resolve MCP tool-name delimiter collision at invocation time MCP tool keys are identified internally as `${rawToolName}${mcp_delimiter}${serverName}` (delimiter `_mcp_`). Several call sites parsed this back apart with a naive `toolKey.split(Constants.mcp_delimiter)`, assuming the delimiter occurs exactly once. When the raw upstream tool name itself contains the delimiter substring - which happens whenever it's exposed through a gateway that prefixes aggregated tool names by server (e.g. a gateway's own "gitlab-get_mcp_server_version" for GitLab's "get_mcp_server_version" tool) - the combined key has the delimiter more than once. `.split()` then produces more than two segments, and destructuring `[toolName, serverName]` silently keeps only the first two, yielding a bogus server name that matches no configured server. Tool listing still worked (a different code path builds keys directly without re-splitting), but invocation failed with `Tool {name} not found`, and `filterAuthorizedTools` rejected such keys outright as malformed. Add `splitMCPToolKey`, which splits on the *last* occurrence of the delimiter instead: the server-name half is always LibreChat's own normalized suffix (guaranteed not to contain the delimiter), while the raw tool-name half is untrusted and may legitimately contain it. This matches `.split()`'s result whenever the delimiter occurs once, and correctly resolves the collision case. Update the four call sites that parsed this manually (`handleTools.js`, `MCP.js`, `mcp.js` controller, `filterAuthorizedTools` in `v1.js`) plus one in the client (`useVisibleTools.ts`) to use it. Fixes #14440 * fix: resolve MCP tool-key boundary against configured server names splitMCPToolKey moves to librechat-data-provider so the client and backend share one parser, and takes the configured server names when the caller has them: the longest name the key actually ends with wins, which is exact. Position alone cannot identify the boundary because both halves may contain the delimiter. lastIndexOf alone fixes gateway-prefixed tool names but regresses servers whose own name contains it, which ToolService.spec.js already covered; the last-delimiter path now only serves as the fallback for callers with no configured set. Also converts the remaining first-occurrence parsers that the delimiter fix missed - mcp/auth.ts (custom user vars silently unresolved), mcp/oauth/events.ts, agents/initialize.ts, and the three client parsers that labelled tool calls with the wrong server. * fix: keep client tool-call labels on first-delimiter parsing The three client parsers had deliberate, tested first-delimiter semantics (ToolCall.test.tsx asserts the full server name for 'foo_mcp_bar' and the synthetic 'oauth_mcp_server' call), and the client has no configured server list in scope to resolve the boundary exactly, so they are left as they were. Threads the configured names into the event-driven definition loader so it resolves the same boundary as the authorization filter that admits the key, and documents the one case that stays undecidable without provenance. * fix: resolve tool-key boundary against all configured servers resolveConfigServers only returns lazily-initialized config overrides - ensureConfigServers skips unmodified YAML servers - so on a stock deployment the known-name list was empty and suffix resolution never engaged. Adds resolveMcpServerNames, which keeps every configured server in the normalized form tool keys carry, and uses it at the loading, auth-map and definition sites. Background-tool eligibility now resolves against all configured names before testing ephemeral membership, so a non-ephemeral server whose name ends in an ephemeral one is no longer misclassified, and useVisibleTools resolves against the server map it already receives. * fix: use resolved server provenance and one app-config read createMCPTool now uses the serverName loadTools already resolved for the key and only parses as a fallback, so an unmodified YAML server whose name contains the delimiter no longer resolves to the wrong server for auth, reconnection and callTool. resolveMcpServerContext derives config servers and all configured names from a single getAppConfigForRequest, replacing two independent lookups on the chat startup path, and degrades to empty like resolveConfigServers instead of aborting tool loading when the config lookup fails. * chore: drop unused resolveConfigServers import * fix: forward server provenance on the all-tools path and read config once createMCPTools builds each toolKey from the server name it already has but did not forward it, so the sys__all__sys path re-derived it by parsing and bound an unmodified YAML server whose name contains the delimiter to the wrong auth and invocation context. loadAgentTools now resolves the MCP server context once and threads it into loadTools, replacing the second app-config read it had introduced on the non-event-driven chat startup path. * fix: carry resolved MCP server name through tool classification definitions.ts resolves the server for each key and then dropped it when building loadedTools, so buildToolClassification re-derived it with a last-segment split and recorded 'Workspace' for a server configured as 'Google_mcp_Workspace'. The resolved name now rides along on the tool instance and classification prefers it over re-parsing. * fix: consume carried server name when extracting MCP servers extractMCPServers re-derived the name with a last-segment split, so a server configured as Google_mcp_Workspace resolved to Workspace and its instructions were silently omitted. Prefers the name carried on the tool definition instance, falling back to the split. * fix: fail closed on ambiguous MCP keys when persisting server names Persisted mcpServerNames grant agent-scoped access to a DB server by name (ServerConfigsDB.getAccessibleServers), so a wrong guess exposes an unrelated server to everyone who can view the agent. The last-segment split turned search_mcp_Google_mcp_workspace into 'workspace'; such keys were previously rejected outright at agent save, so admitting them opened this path. Derives a name only from unambiguous single-delimiter keys. This is #12250's guard moved to the boundary it was actually protecting, instead of blocking tool admission. * fix: keep DB server access for multi-delimiter tool keys The fail-closed guard was wrong for the case this PR exists to fix. This index only grants DB-backed servers, and DB names are slugs that cannot contain the delimiter (generateServerNameFromTitle strips underscores), so the trailing segment is always the real server for them - dropping it cost every consumer of a gateway-prefixed tool their shared-agent access. Also gates the MCP server-context lookup on the filtered MCP set, so an agent with no MCP tools no longer pays an app-config read on startup. * fix: resolve tool-call display names without breaking OAuth calls The display parsers could not use the shared boundary parser because their tested behavior depends on first-delimiter semantics. That constraint only applies to synthetic MCP OAuth calls, whose tool half is always exactly 'oauth', so everything after the first delimiter is the server even when the server name carries one. splitToolCallName special-cases that form and defers to splitMCPToolKey for real tool keys, so a gateway-prefixed tool now renders its own name and server while oauth_mcp_foo_mcp_bar still resolves to foo_mcp_bar. * fix: persist resolved MCP server provenance on agents Deriving mcpServerNames from the tool key cannot tell a config server's trailing segment from a real DB server name, so a config server named a_mcp_b indexed an unrelated DB server b and shared the agent's viewers into it. Neither string rule works: the suffix guess exposes, and failing closed drops legitimate DB access for gateway-prefixed tools. filterAuthorizedTools already resolves each tool's server against the merged registry config, so it now collects those names and create, update and duplicate persist them. No extra registry queries: the update path unions the newly resolved names with what the agent already had, and duplicate replaces the copied list rather than inheriting the source's servers. Display parsing also takes the configured names, so a real tool call on a delimiter-bearing server renders the right server and icon. * test: teach MCP hook mocks about useMCPServerNames Three specs mock ~/hooks/MCP with a hand-listed factory, so adding the hook to ToolCall made useMCPServerNames undefined under test and every render threw. Returns a stable array so the mock cannot perturb render counts. * fix: rebuild agent MCP server index from surviving tools Unioning the prior names kept a server indexed after its last tool was detached, so viewers of a shared agent retained agent-scoped access to it. The index is now rebuilt from the tools that survive the edit: a prior name carries forward only while some retained tool still resolves to it, using the agent's own persisted names as the candidate set, and the rebuild runs on any tool change rather than only when a new MCP tool is added. * fix: keep duplicate indexes on registry fallback and harden the oauth split Duplication blanked mcpServerNames when the registry was unavailable, because filterAuthorizedTools grandfathers the source's tools without resolving them - the copy kept tools it could no longer resolve. Source names now carry forward for the tools that still point at them. splitToolCallName also treated any oauth_mcp_ prefix as a synthetic OAuth call, so a genuine upstream tool by that name resolved to the wrong server. A configured server name now decides when one matches, since a real key always ends in its server, and the prefix only breaks ties for unconfigured servers. * fix: thread configured server names through display parsing parseToolName and getMCPServerName resolved context-free, so a configured server whose name contains the delimiter showed the wrong server in grouped tool summaries and subagent tool labels, and stacked icons missed its entry in the icon map. Both take the configured names now, supplied by the components that render them. Adds the hook to SubagentCall's mock factory: the spec renders the real component, so an unmocked useMCPServerNames would reach the query with no provider. * test: cover the auth-map boundary, server provenance and context fallback Adds regression coverage for three behaviors this PR changed that no test exercised: customUserVars resolving under the right plugin key for a gateway-prefixed tool name (the failure that made these tools loadable but unusable), the resolved server name reaching createMCPTool instead of being re-parsed, and resolveMcpServerContext degrading to empty rather than aborting tool loading when the config lookup fails. Each was checked against a mutated source to confirm it fails when the behavior is broken. * fix: normalize server-name candidates and cover the boundary guard Tool keys embed normalizeServerName's output while the config is keyed by the raw name, so callers passing raw keys never matched a server whose name needs normalizing and silently fell back to the last delimiter. filterAuthorizedTools now maps normalized names back to their config key, and createMCPTool normalizes its candidates. Adds the cases an audit found surviving mutation: a configured name that is a bare but not delimiter-aligned suffix must not match, an empty candidate list behaves as no list, and splitToolCallName still falls back to the oauth prefix when a list is supplied but nothing in it matches. * fix: keep resolved server names when a non-owner retains MCP tools The shared-agent path keeps an agent's existing MCP tools verbatim but supplied no mcpServerNames, so persistence re-derived them and reduced a configured server like Google_mcp_Workspace to Workspace - which ServerConfigsDB then treats as a DB server, granting the agent's viewers access to an unrelated one. Carries the existing resolved names across instead, and clears the index on the owner path where every MCP tool is removed. * fix: preserve resolved MCP names for every tools update extractMCPServerNames was reachable from any caller that writes tools without mcpServerNames - the Action edit path does exactly that - so a configured Google_mcp_Workspace was reindexed as Workspace and ServerConfigsDB granted shared-agent viewers an unrelated DB server by that name. updateAgent now rebuilds the index from the agent's own resolved names: one carries forward while a retained tool still resolves to it, and only keys matching none of them fall back to derivation. Callers are safe by default rather than by remembering to pass the set. normalizeServerName moves to librechat-data-provider so the client can match its candidates against tool keys, which embed the normalized form; the icon map is keyed the same way since it is looked up with a parsed server name. * refactor: move MCP context resolution into packages/api New backend logic belongs in the TypeScript workspace per CLAUDE.md, with /api kept to a thin wrapper. resolveMCPServerContext now lives in packages/api/src/mcp/context.ts and takes ensureConfigServers by injection, since the registry accessor is still legacy-only; the /api function is reduced to loading the request app config and translating failures into the empty degrade it already promised. * test: teach the MCP service mock about resolveMCPServerContext The spec mocks @librechat/api with a hand-listed factory, so moving the resolver into that package left it undefined and the wrapper degraded into its own catch, returning empty config servers. The stub mirrors the real resolver so these tests still cover what the wrapper owns - loading the request config and degrading on failure - while the resolution logic is unit-tested in packages/api. * fix: only persist an authoritative MCP server index on update Assigning the resolved set unconditionally pinned the index to [] whenever nothing authoritative was available - a legacy agent holding MCP tools with no stored mcpServerNames - which suppressed updateAgent's derivation and stripped agent-scoped access to its DB-backed server. The field is now supplied only when the result is authoritative: names were resolved, or no MCP tool survives so the index genuinely is empty. The retained-tools branch likewise leaves it unset when the agent has none stored. --------- Co-authored-by: Jens Schumann <schumajs@gmail.com>	2026-07-27 14:45:38 -04:00
jcbartle	268f095c1a	🔒 feat: Add On-Behalf-Of (OBO) token exchange support for MCP Servers (#13429) ... * Add OBO (On-Behalf-Of) token exchange support for MCP server connections Enables transparent authentication to Entra ID-backed MCP servers using the logged-in user's federated token via the OAuth 2.0 jwt-bearer grant. Configured via obo.scopes in librechat.yaml server config. - Extract generic OboTokenService from GraphTokenService (jwt-bearer grant + cache) - Refactor GraphTokenService to thin wrapper delegating to OboTokenService - Add obo schema field to BaseOptionsSchema in data-provider - Add resolveOboToken in packages/api/src/mcp/oauth/obo.ts (validates federated token, calls resolver, returns MCPOAuthTokens) - Wire oboTokenResolver through MCPConnectionFactory, MCPManager, UserConnectionManager - OBO tokens injected via request headers (not OAuth transport), refreshed on each tool call - Explicit error on OBO failure (no fallthrough to standard OAuth redirect) - Add unit tests for both resolveOboToken (9 tests) and exchangeOboToken (14 tests) * Add OBO authentication option to MCP server UI configuration Enable users to configure On-Behalf-Of (OBO) token exchange for MCP servers created via the UI (MongoDB-stored), in addition to the existing YAML-based configuration. - Add "On-Behalf-Of (OBO)" radio option to MCP server auth section with scopes input field - Remove obo from omitServerManagedFields so the field passes UI schema validation - Add OBO to AuthTypeEnum, obo_scopes to AuthConfig, and OBO handling in form defaults and submission - Add .min(1) validation on obo.scopes to reject empty strings - Add English localization keys: com_ui_obo, com_ui_obo_scopes, com_ui_obo_scopes_description - Add 5 schema validation tests for OBO field acceptance, transport compatibility, and edge cases * 🧊 fix: Add obo to safe properties in redactServerSecrets. Fixes the OBO configuration not showing up in the MCP UI after app restart * Address linter errors * 🧊 fix: fail closed on OBO refresh errors and retry transient token exchange failures - stop tool calls from falling back to stale Authorization headers when per-call OBO refresh fails - add one-time retry for transient Entra OBO exchange failures (network/429/5xx) - preserve structured OBO failure reasons and retryability in resolveOboToken - improve OBO auth error messaging for connection setup and tool execution - add tests for transient vs permanent OBO failure paths * Addressing linting errors / warnings * 🧊 fix: isolate OBO MCP auth to user-scoped connections - block OBO-enabled servers from app-level shared MCP connections - bypass shared connection lookup for OBO servers in MCPManager.getConnection - add regressions covering OBO connection scoping and preserve non-OBO app connection reuse * 🛠️ refactor: centralize MCP user-scoped connection policy - add shared requiresUserScopedConnection helper for OAuth, OBO, and customUserVars - use the shared predicate in MCPManager and ConnectionsRepository - add utils coverage for user-scoped connection policy * 🧊 fix: restrict MCP OBO config to header-capable transports - Move OBO configuration out of the shared MCP base options schema and allow it only on SSE and streamable-http transports, where request headers are applied. - Explicitly reject OBO on stdio and websocket configs to avoid accepted-but- nonfunctional server definitions. Add schema coverage for admin/config parsing and user-input websocket validation. * 🧊 fix: single-flight concurrent OBO token exchanges Concurrent tool calls that arrive on a cache miss were each issuing their own jwt-bearer request to the IdP. Under that fan-out, Entra intermittently returned errors that the retry classifier saw as non-retryable, surfacing as: "The identity provider rejected the OBO token exchange. Cannot execute tool <name>. Re-authenticate the user or verify the configured OBO scopes and retry." A user retry then hit the populated cache and succeeded, which matches the observed flakiness — the cache was empty at the moment of fan-out but populated by the time the user clicked retry. - Coalesce concurrent exchanges in `OboTokenService.exchangeOboToken` keyed by `${openidId}:${scopes}`. Callers that arrive while an exchange is in flight share the same upstream request and receive the same result. `fromCache=false` continues to force a fresh, independent exchange (and is not joined by `fromCache=true` callers). The IdP call, single-retry path, and cache write are unchanged — they were moved into a `performOboExchange` helper so the coalescing wrapper stays small. - Tests cover: coalescing on the same key, isolation between different keys, cleanup on success, cleanup on failure, and the `fromCache=false` bypass. * 🔒 feat: gate MCP OBO config behind MCP_SERVERS.CONFIGURE_OBO permission OBO silently mints per-user delegated tokens from the caller's federated access token and forwards them to whatever URL the server config points at. Previously, anyone with MCP_SERVERS.CREATE could configure obo.scopes — so if server creation is ever delegated beyond admins, a user could stand up an attacker-controlled server, attach it to a shared agent, and exfiltrate other users' downstream tokens on tool invocation. Add a dedicated MCP_SERVERS.CONFIGURE_OBO permission (ADMIN: true, USER: false by default) and enforce it at three layers so the safety property no longer depends on CREATE staying admin-only: - Create/update: POST/PATCH /api/mcp/servers returns 403 when the body carries `obo` and the caller's role lacks the permission. - Runtime fail-closed: for DB-sourced configs, MCPConnectionFactory and MCPManager.callTool re-check the original author's role before each OBO exchange. If the author has been downgraded, the exchange is skipped (factory) or refused (callTool) — retained configs lose their privileges automatically. - UI: the OBO option is hidden in the MCP server dialog for users without the permission; a CONFIGURE_OBO toggle is exposed in the MCP admin role editor. Existing role docs receive the new sub-key via the permission backfill in updateInterfacePermissions on next startup, preserving any operator-set values. YAML/Config-sourced server configs are unaffected since they're admin-controlled at the deployment level. * 🧊 fix: wire OBO machinery for servers with requiresOAuth: false The discovery and user-connection paths gated OAuth wiring (flow manager, token methods, oboTokenResolver, oboTrustChecker) behind isOAuthServer(), which only considers requiresOAuth/oauth fields. A DB-stored OBO server with requiresOAuth: false therefore landed in the non-OAuth branch, never received an oboTokenResolver, and the factory's usesObo getter evaluated to false — sending a bare request that the upstream rejected with invalid_token. Add requiresOAuthMachinery() (OAuth OR OBO) and use it at those two gates. isOAuthServer remains for the OAuth-handshake-only check (shouldInitiateOAuthBeforeConnect), where OBO must not initiate a handshake. Plumb the OBO resolver/trust-checker through ToolDiscoveryOptions so reinitMCPServer can pass them on the discovery path. * 🧊 fix: lock all OBO-target fields (URL, proxy, headers, auth) without CONFIGURE_OBO The CONFIGURE_OBO permission was meant to gate control of the endpoint that receives OBO-minted per-user delegated tokens and the scopes that are requested. The previous frontend lock + backend gate only covered obo.scopes and the auth section, leaving url/proxy/headers/etc. editable by anyone with UPDATE — meaning a non-permission user could still redirect an existing OBO server's token flow to an attacker endpoint. Switch to an allowlist policy: when editing an OBO server without CONFIGURE_OBO, only title/description/iconPath are mutable. Backend rejects any other field change with 403; frontend disables the non-allowlist sections (URL, transport, auth, trust) via fieldset. The comparison surface (MCP_USER_INPUT_FIELDS) is derived from MCPServerUserInputSchema's union members so it stays in sync with the schema. New schema fields land in the locked set by default — adding to the allowlist is the only way to unlock them, which preserves the security-review boundary. * 🧊 fix: skip unauthenticated MCP inspection for OBO-only servers MCPServerInspector.inspectServer() ran an unauthenticated temp connection unless the config had requiresOAuth or customUserVars set. For OBO-only servers without standard MCP OAuth advertisement, this caused MCPConnectionFactory.create to attempt the connection without a user or oboTokenResolver — failing on servers that reject the MCP initialize handshake without a valid bearer token, which surfaced as MCP_INSPECTION_FAILED on create/update. Add `obo` to the skip list alongside requiresOAuth and customUserVars, matching the existing pattern for user-scoped auth modes. * Addressed linting error: watchedTitle is declared but never referenced (the auto-fill logic at line 156 uses getValues('title') instead). Deleted constant.	2026-06-01 22:36:18 -04:00
Danny Avila	bd64251eb9	🪪 fix: Prevent MCP Server Name Collisions (#13256) ... * fix: prevent MCP server name collisions * chore: address MCP registry review nits * fix: reserve MCP config names from request context * chore: format MCP registry changes * chore: address MCP collision review findings	2026-05-22 20:46:14 -04:00
Danny Avila	749eb06e67	🧭 fix: Reduce MCP Registry ACL Lookups (#13195)	2026-05-19 17:16:37 -04:00
Danny Avila	935288f841	🏗️ feat: 3-Tier MCP Server Architecture with Config-Source Lazy Init (#12435) ... * feat: add MCPServerSource type, tenantMcpPolicy schema, and source-based dbSourced wiring - Add `tenantMcpPolicy` to `mcpSettings` in YAML config schema with `enabled`, `maxServersPerTenant`, `allowedTransports`, and `allowedDomains` - Add `MCPServerSource` type ('yaml' | 'config' | 'user') and `source` field to `ParsedServerConfig` - Change `dbSourced` determination from `!!config.dbId` to `config.source === 'user'` across MCPManager, ConnectionsRepository, UserConnectionManager, and MCPServerInspector - Set `source: 'user'` on all DB-sourced servers in ServerConfigsDB * feat: three-layer MCPServersRegistry with config cache and lazy init - Add `configCacheRepo` as third repository layer between YAML cache and DB for admin-defined config-source MCP servers - Implement `ensureConfigServers()` that identifies config-override servers from resolved `getAppConfig()` mcpConfig, lazily inspects them, and caches parsed configs with `source: 'config'` - Add `lazyInitConfigServer()` with timeout, stub-on-failure, and concurrent-init deduplication via `pendingConfigInits` map - Extend `getAllServerConfigs()` with optional `configServers` param for three-way merge: YAML → Config → User - Add `getServerConfig()` lookup through config cache layer - Add `invalidateConfigCache()` for clearing config-source inspection results on admin config mutations - Tag `source: 'yaml'` on CACHE-stored servers and `source: 'user'` on DB-stored servers in `addServer()` and `addServerStub()` * feat: wire tenant context into MCP controllers, services, and cache invalidation - Resolve config-source servers via `getAppConfig({ role, tenantId })` in `getMCPTools()` and `getMCPServersList()` controllers - Pass `ensureConfigServers()` results through `getAllServerConfigs()` for three-way merge of YAML + Config + User servers - Add tenant/role context to `getMCPSetupData()` and connection status routes via `getTenantId()` from ALS - Add `clearMcpConfigCache()` to `invalidateConfigCaches()` so admin config mutations trigger re-inspection of config-source MCP servers * feat: enforce tenantMcpPolicy on admin config mcpServers mutations - Add `validateMcpServerPolicy()` helper that checks mcpServers against operator-defined `tenantMcpPolicy` (enabled, maxServersPerTenant, allowedTransports, allowedDomains) - Wire validation into `upsertConfigOverrides` and `patchConfigField` handlers — rejects with 403 when policy is violated - Infer transport type from config shape (command → stdio, url protocol → websocket/sse, type field → streamable-http) - Validate server domains against policy allowlist when configured * revert: remove tenantMcpPolicy schema and enforcement The existing admin config CRUD routes already provide the mechanism for granular MCP server prepopulation (groups, roles, users). The tenantMcpPolicy gating adds unnecessary complexity that can be revisited if needed in the future. - Remove tenantMcpPolicy from mcpSettings Zod schema - Remove validateMcpServerPolicy helper and TenantMcpPolicy interface - Remove policy enforcement from upsertConfigOverrides and patchConfigField handlers * test: update test assertions for source field and config-server wiring - Use objectContaining in MCPServersRegistry reset test to account for new source: 'yaml' field on CACHE-stored configs - Add getTenantId and ensureConfigServers mocks to MCP route tests - Add getAppConfig mock to route test Config service mock - Update getMCPSetupData assertion to expect second options argument - Update getAllServerConfigs assertions for new configServers parameter * fix: disconnect active connections when config-source servers are evicted When admin config overrides change and config-source MCP servers are removed, the invalidation now proactively disconnects active connections for evicted servers instead of leaving them lingering until timeout. - Return evicted server names from invalidateConfigCache() - Disconnect app-level connections for evicted servers in clearMcpConfigCache() via MCPManager.appConnections.disconnect() * fix: address code review findings (CRITICAL, MAJOR, MINOR) CRITICAL fixes: - Scope configCacheRepo keys by config content hash to prevent cross-tenant cache poisoning when two tenants define the same server name with different configurations - Change dbSourced checks from `source === 'user'` to `source !== 'yaml' && source !== 'config'` so undefined source (pre-upgrade cached configs) fails closed to restricted mode MAJOR fixes: - Derive OAuth servers from already-computed mcpConfig instead of calling getOAuthServers() separately — config-source OAuth servers are now properly detected - Add parseInt radix (10) and NaN guard with fallback to 30_000 for CONFIG_SERVER_INIT_TIMEOUT_MS - Add CONFIG_CACHE_NAMESPACE to aggregate-key branch in ServerConfigsCacheFactory to avoid SCAN-based Redis stalls - Remove `if (role || tenantId)` guard in getMCPSetupData — config servers now always resolve regardless of tenant context MINOR fixes: - Extract resolveAllMcpConfigs() helper in mcp controller to eliminate 3x copy-pasted config resolution boilerplate - Distinguish "not initialized" from real errors in clearMcpConfigCache — log actual failures instead of swallowing - Remove narrative inline comments per style guide - Remove dead try/catch inside Promise.allSettled in ensureConfigServers (inner method never throws) - Memoize YAML server names to avoid repeated cacheConfigsRepo.getAll() calls per request Test updates: - Add ensureConfigServers mock to registry test fixtures - Update getMCPSetupData assertions for inline OAuth derivation * fix: address code review findings (CRITICAL, MAJOR, MINOR) CRITICAL fixes: - Break circular dependency: move CONFIG_CACHE_NAMESPACE from MCPServersRegistry to ServerConfigsCacheFactory - Fix dbSourced fail-closed: use source field when present, fall back to legacy dbId check when absent (backward-compatible with pre-upgrade cached configs that lack source field) MAJOR fixes: - Add CONFIG_CACHE_NAMESPACE to aggregate-key set in ServerConfigsCacheFactory to avoid SCAN-based Redis stalls - Add comprehensive test suite (ensureConfigServers.test.ts, 18 tests) covering lazy init, stub-on-failure, cross-tenant isolation via config hash keys, concurrent deduplication, merge order, and cache invalidation MINOR fixes: - Update MCPServerInspector test assertion for dbSourced change * fix: restore getServerConfig lookup for config-source servers (NEW-1) Add configNameToKey map that indexes server name → hash-based cache key for O(1) lookup by name in getServerConfig. This restores the config cache layer that was dropped when hash-based keys were introduced. Without this fix, config-source servers appeared in tool listings (via getAllServerConfigs) but getServerConfig returned undefined, breaking all connection and tool call paths. - Populate configNameToKey in ensureSingleConfigServer - Clear configNameToKey in invalidateConfigCache and reset - Clear stale read-through cache entries after lazy init - Remove dead code in invalidateConfigCache (config.title, key parsing) - Add getServerConfig tests for config-source server lookup * fix: eliminate configNameToKey race via caller-provided configServers param Replace the process-global configNameToKey map (last-writer-wins under concurrent multi-tenant load) with a configServers parameter on getServerConfig. Callers pass the pre-resolved config servers map directly — no shared mutable state, no cross-tenant race. - Add optional configServers param to getServerConfig; when provided, returns matching config directly without any global lookup - Remove configNameToKey map entirely (was the source of the race) - Extract server names from cache keys via lastIndexOf in invalidateConfigCache (safe for names containing colons) - Use mcpConfig[serverName] directly in getMCPTools instead of a redundant getServerConfig call - Add cross-tenant isolation test for getServerConfig * fix: populate read-through cache after config server lazy init After lazyInitConfigServer succeeds, write the parsed config to readThroughCache keyed by serverName so that getServerConfig calls from ConnectionsRepository, UserConnectionManager, and MCPManager.callTool find the config without needing configServers. Without this, config-source servers appeared in tool listings but every connection attempt and tool call returned undefined. * fix: user-scoped getServerConfig fallback to server-only cache key When getServerConfig is called with a userId (e.g., from callTool or UserConnectionManager), the cache key is serverName::userId. Config-source servers are cached under the server-only key (no userId). Add a fallback so user-scoped lookups find config-source servers in the read-through cache. * fix: configCacheRepo fallback, isUserSourced DRY, cross-process race CRITICAL: Add findInConfigCache fallback in getServerConfig so config-source servers remain reachable after readThroughCache TTL expires (5s). Without this, every tool call after 5s returned undefined for config-source servers. MAJOR: Extract isUserSourced() helper to mcp/utils.ts and replace all 5 inline dbSourced ternary expressions (MCPManager x2, ConnectionsRepository, UserConnectionManager, MCPServerInspector). MAJOR: Fix cross-process Redis race in lazyInitConfigServer — when configCacheRepo.add throws (key exists from another process), fall back to reading the existing entry instead of returning undefined. MINOR: Parallelize invalidateConfigCache awaits with Promise.all. Remove redundant .catch(() => {}) inside Promise.allSettled. Tighten dedup test assertion to toBe(1). Add TTL-expiry tests for getServerConfig (with and without userId). * feat: thread configServers through getAppToolFunctions and formatInstructionsForContext Add optional configServers parameter to getAppToolFunctions, getInstructions, and formatInstructionsForContext so config-source server tools and instructions are visible to agent initialization and context injection paths. Existing callers (boot-time init, tests) pass no argument and continue to work unchanged. Agent runtime paths can now thread resolved config servers from request context. * fix: stale failure stubs retry after 5 min, upsert for cross-process races - Add CONFIG_STUB_RETRY_MS (5 min) — stale failure stubs are retried instead of permanently disabling config-source servers after transient errors (DNS outage, cold-start race) - Extract upsertConfigCache() helper that tries add then falls back to update, preventing cross-process Redis races where a second instance's successful inspection result was discarded - Add test for stale-stub retry after CONFIG_STUB_RETRY_MS * fix: stamp updatedAt on failure stubs, null-guard callTool config, test cleanup - Add updatedAt: Date.now() to failure stubs in lazyInitConfigServer so CONFIG_STUB_RETRY_MS (5 min) window works correctly — without it, stubs were always considered stale (updatedAt ?? 0 → epoch → always expired) - Add null guard for rawConfig in MCPManager.callTool before passing to preProcessGraphTokens — prevents unsafe `as` cast on undefined - Log double-failure in upsertConfigCache instead of silently swallowing - Replace module-scope Date.now monkey-patch with jest.useFakeTimers / jest.setSystemTime / jest.useRealTimers in ensureConfigServers tests * fix: server-only readThrough fallback only returns truthy values Prevents a cached undefined from a prior no-userId lookup from short-circuiting the DB query on a subsequent userId-scoped lookup. * fix: remove findInConfigCache to eliminate cross-tenant config leakage The findInConfigCache prefix scan (serverName:*) could return any tenant's config after readThrough TTL expires, violating tenant isolation. Config-source servers are now ONLY resolvable through: 1. The configServers param (callers with tenant context from ALS) 2. The readThrough cache (populated by ensureSingleConfigServer, 5s TTL, repopulated on every HTTP request via resolveAllMcpConfigs) Connection/tool-call paths without tenant context rely exclusively on the readThrough cache. If it expires before the next HTTP request repopulates it, the server is not found — which is correct because there is no tenant context to determine which config to return. - Remove findInConfigCache method and its call in getServerConfig - Update server-only readThrough fallback to only return truthy values (prevents cached undefined from short-circuiting user-scoped DB lookup) - Update tests to document tenant isolation behavior after cache expiry * style: fix import order per AGENTS.md conventions Sort package imports shortest-to-longest, local imports longest-to-shortest across MCPServersRegistry, ConnectionsRepository, MCPManager, UserConnectionManager, and MCPServerInspector. * fix: eliminate cross-tenant readThrough contamination and TTL-expiry tool failures Thread pre-resolved serverConfig from tool creation context into callTool, removing dependency on the readThrough cache for config-source servers. This fixes two issues: - Cross-tenant contamination: the readThrough cache key was unscoped (just serverName), so concurrent multi-tenant requests for same-named servers would overwrite each other's entries - TTL expiry: tool calls happening >5s after config resolution would fail with "Configuration not found" because the readThrough entry had expired Changes: - Add optional serverConfig param to MCPManager.callTool — uses provided config directly, falling back to getServerConfig lookup for YAML/user servers - Thread serverConfig from createMCPTool through createToolInstance closure to callTool - Remove readThrough write from ensureSingleConfigServer — config-source servers are only accessible via configServers param (tenant-scoped) - Remove server-only readThrough fallback from getServerConfig - Increase config cache hash from 8 to 16 hex chars (64-bit) - Add isUserSourced boundary tests for all source/dbId combinations - Fix double Object.keys call in getMCPTools controller - Update test assertions for new getServerConfig behavior * fix: cache base configs for config-server users; narrow upsertConfigCache error handling - Refactor getAllServerConfigs to separate base config fetch (YAML + DB) from config-server layering. Base configs are cached via readThroughCacheAll regardless of whether configServers is provided, eliminating uncached MongoDB queries per request for config-server users - Narrow upsertConfigCache catch to duplicate-key errors only; infrastructure errors (Redis timeouts, network failures) now propagate instead of being silently swallowed, preventing inspection storms during outages * fix: restore correct merge order and document upsert error matching - Restore YAML → Config → User DB precedence in getAllServerConfigs (user DB servers have highest precedence, matching the JSDoc contract) - Add source comment on upsertConfigCache duplicate-key detection linking to the two cache implementations that define the error message * feat: complete config-source server support across all execution paths Wire configServers through the entire agent execution pipeline so config-source MCP servers are fully functional — not just visible in listings but executable in agent sessions. - Thread configServers into handleTools.js agent tool pipeline: resolve config servers from tenant context before MCP tool iteration, pass to getServerConfig, createMCPTools, and createMCPTool - Thread configServers into agent instructions pipeline: applyContextToAgent → getMCPInstructionsForServers → formatInstructionsForContext, resolved in client.js before agent context application - Add configServers param to createMCPTool and createMCPTools for reconnect path fallback - Add source field to redactServerSecrets allowlist for client UI differentiation of server tiers - Narrow invalidateConfigCache to only clear readThroughCacheAll (merged results), preserving YAML individual-server readThrough entries - Update context.spec.ts assertions for new configServers parameter * fix: add missing mocks for config-source server dependencies in client.test.js Mock getMCPServersRegistry, getAppConfig, and getTenantId that were added to client.js but not reflected in the test file's jest.mock declarations. * fix: update formatInstructionsForContext assertions for configServers param The test assertions expected formatInstructionsForContext to be called with only the server names array, but it now receives configServers as a second argument after the config-source server feature wiring. * fix: move configServers resolution before MCP tool loop to avoid TDZ configServers was declared with `let` after the first tool loop but referenced inside it via getServerConfig(), causing a ReferenceError temporal dead zone. Move declaration and resolution before the loop, using tools.some(mcpToolPattern) to gate the async resolution. * fix: address review findings — cache bypass, discoverServerTools gap, DRY - #2: getAllServerConfigs now always uses getBaseServerConfigs (cached via readThroughCacheAll) instead of bypassing it when configServers is present. Extracts user-DB entries from cached base by diffing against YAML keys to maintain YAML → Config → User DB merge order without extra MongoDB calls. - #3: Add configServers param to ToolDiscoveryOptions and thread it through discoverServerTools → getServerConfig so config-source servers are discoverable during OAuth reconnection flows. - #6: Replace inline import() type annotations in context.ts with proper import type { ParsedServerConfig } per AGENTS.md conventions. - #7: Extract resolveConfigServers(req) helper in MCP.js and use it from handleTools.js and client.js, eliminating the duplicated 6-line config resolution pattern. - #10: Restore removed "why" comment explaining getLoaded() vs getAll() choice in getMCPSetupData — documents non-obvious correctness constraint. - #11: Fix incomplete JSDoc param type on resolveAllMcpConfigs. * fix: consolidate imports, reorder constants, fix YAML-DB merge edge case - Merge duplicate @librechat/data-schemas requires in MCP.js into one - Move resolveConfigServers after module-level constants - Fix getAllServerConfigs edge case where user-DB entry overriding a YAML entry with the same name was excluded from userDbConfigs; now uses reference equality check to detect DB-overwritten YAML keys * fix: replace fragile string-match error detection with proper upsert method Add upsert() to IServerConfigsRepositoryInterface and all implementations (InMemory, Redis, RedisAggregateKey, DB). This eliminates the brittle error message string match ('already exists in cache') in upsertConfigCache that was the only thing preventing cross-process init races from silently discarding inspection results. Each implementation handles add-or-update atomically: - InMemory: direct Map.set() - Redis: direct cache.set() - RedisAggregateKey: read-modify-write under write lock - DB: delegates to update() (DB servers use explicit add() with ACL setup) * fix: wire configServers through remaining HTTP endpoints - getMCPServerById: use resolveAllMcpConfigs instead of bare getServerConfig - reinitialize route: resolve configServers before getServerConfig - auth-values route: resolve configServers before getServerConfig - getOAuthHeaders: accept configServers param, thread from callers - Update mcp.spec.js tests to mock getAllServerConfigs for GET by name * fix: thread serverConfig through getConnection for config-source servers Config-source servers exist only in configCacheRepo, not in YAML cache or DB. When callTool → getConnection → getUserConnection → getServerConfig runs without configServers, it returns undefined and throws. Fix by threading the pre-resolved serverConfig (providedConfig) from callTool through getConnection → getUserConnection → createUserConnectionInternal, using it as a fallback before the registry lookup. * fix: thread configServers through reinit, reconnect, and tool definition paths Wire configServers through every remaining call chain that creates or reconnects MCP server connections: - reinitMCPServer: accepts serverConfig and configServers, uses them for getServerConfig fallback, getConnection, and discoverServerTools - reconnectServer: accepts and passes configServers to reinitMCPServer - createMCPTools/createMCPTool: pass configServers to reconnectServer - ToolService.loadToolDefinitionsWrapper: resolves configServers from req, passes to both reinitMCPServer call sites - reinitialize route: passes serverConfig and configServers to reinitMCPServer * fix: address review findings — simplify merge, harden error paths, fix log labels - Simplify getAllServerConfigs merge: replace fragile reference-equality loop with direct spread { ...yamlConfigs, ...configServers, ...base } - Guard upsertConfigCache in lazyInitConfigServer catch block so cache failures don't mask the original inspection error - Deduplicate getYamlServerNames cold-start with promise dedup pattern - Remove dead `if (!mcpConfig)` guard in getMCPSetupData - Fix hardcoded "App server" in ServerConfigsCacheRedisAggregateKey error messages — now uses this.namespace for correct Config/App labeling - Remove misleading OAuth callback comment about readThrough cache - Move resolveConfigServers after module-level constants in MCP.js * fix: clear rejected yamlServerNames promise, fix config-source reinspect, fix reset log label - Clear yamlServerNamesPromise on rejection so transient cache errors don't permanently prevent ensureConfigServers from working - Skip reinspectServer for config-source servers (source: 'config') in reinitMCPServer — they lack a CACHE/DB storage location; retry is handled by CONFIG_STUB_RETRY_MS in ensureConfigServers - Use source field instead of dbId for storageLocation derivation - Fix remaining hardcoded "App" in reset() leaderCheck message * fix: persist oauthHeaders in flow state for config-source OAuth servers The OAuth callback route has no JWT auth context and cannot resolve config-source server configs. Previously, getOAuthHeaders would silently return {} for config-source servers, dropping custom token exchange headers. Now oauthHeaders are persisted in MCPOAuthFlowMetadata during flow initiation (which has auth context), and the callback reads them from the stored flow state with a fallback to the registry lookup for YAML/user-DB servers. * fix: update tests for getMCPSetupData null guard removal and ToolService mock - MCP.spec.js: update test to expect graceful handling of null mcpConfig instead of a throw (getAllServerConfigs always returns an object) - MCP.js: add defensive || {} for Object.entries(mcpConfig) in case of null from test mocks - ToolService.spec.js: add missing mock for ~/server/services/MCP (resolveConfigServers) * fix: address review findings — DRY, naming, logging, dead code, defensive guards - #1: Simplify getAllServerConfigs to single getBaseServerConfigs call, eliminating redundant double-fetch of cacheConfigsRepo.getAll() - #2: Add warning log when oauthHeaders absent from OAuth callback flow state - #3: Extract resolveAllMcpConfigs to MCP.js service layer; controller imports shared helper instead of reimplementing - #4: Rename _serverConfig/_provider to capturedServerConfig/capturedProvider in createToolInstance — these are actively used, not unused - #5: Log rejected results from ensureConfigServers Promise.allSettled so cache errors are visible instead of silently dropped - #6: Remove dead 'MCP config not found' error handlers from routes - #7: Document circular-dependency reason for dynamic require in clearMcpConfigCache - #8: Remove logger.error from withTimeout to prevent double-logging timeouts - #10: Add explicit userId guard in ServerConfigsDB.upsert with clear error message - #12: Use spread instead of mutation in addServer for immutability consistency - Add upsert mock to ensureConfigServers.test.ts DB mock - Update route tests for resolveAllMcpConfigs import change * fix: restore correct merge priority, use immutable spread, fix test mock - getAllServerConfigs: { ...configServers, ...base } so userDB wins over configServers, matching documented "User DB (highest)" priority - lazyInitConfigServer: use immutable spread instead of direct mutation for parsedConfig.source, consistent with addServer fix - Fix test to mock getAllServerConfigs as {} instead of null, remove unnecessary || {} defensive guard in getMCPSetupData * fix: error handling, stable hashing, flatten nesting, remove dead param - Wrap resolveConfigServers/resolveAllMcpConfigs in try/catch with graceful {} fallback so transient DB/cache errors don't crash tool pipeline - Sort keys in configCacheKey JSON.stringify for deterministic hashing regardless of object property insertion order - Flatten clearMcpConfigCache from 3 nested try-catch to early returns; document that user connections are cleaned up lazily (accepted tradeoff) - Remove dead configServers param from getAppToolFunctions (never passed) - Add security rationale comment for source field in redactServerSecrets * fix: use recursive key-sorting replacer in configCacheKey to prevent cross-tenant cache collision The array replacer in JSON.stringify acts as a property allowlist at every nesting depth, silently dropping nested keys like headers['X-API-Key'], oauth.client_secret, etc. Two configs with different nested values but identical top-level structure produced the same hash, causing cross-tenant cache hits and potential credential contamination. Switch to a function replacer that recursively sorts keys at all depths without dropping any properties. Also document the known gap in getOAuthServers: config-source OAuth servers are not covered by auto-reconnection or uninstall cleanup because callers lack request context. * fix: move clearMcpConfigCache to packages/api to eliminate circular dependency The function only depends on MCPServersRegistry and MCPManager, both of which live in packages/api. Import it directly from @librechat/api in the CJS layer instead of using dynamic require('~/config'). * chore: imports/fields ordering * fix: address review findings — error handling, targeted lookup, test gaps - Narrow resolveAllMcpConfigs catch to only wrap ensureConfigServers so getAppConfig/getAllServerConfigs failures propagate instead of masking infrastructure errors as empty server lists. - Use targeted getServerConfig in getMCPServerById instead of fetching all server configs for a single-server lookup. - Forward configServers to inner createMCPTool calls so reconnect path works for config-source servers. - Update getAllServerConfigs JSDoc to document disjoint-key design. - Add OAuth callback oauthHeaders fallback tests (flow state present vs registry fallback). - Add resolveConfigServers/resolveAllMcpConfigs unit tests covering happy path and error propagation. * fix: add getOAuthReconnectionManager mock to OAuth callback tests * chore: imports ordering	2026-03-28 10:36:43 -04:00