173 commits

Author	SHA1	Message	Date
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	100871c3ec	🛂 fix: Enforce MCP Permissions for Agent Tools (#13174) ... * fix: Enforce MCP Permissions for Agent Tools * fix: Measure MCP Image Limit by Decoded Size * fix: gate cached MCP tools and tighten remote image URL detection Addresses Codex review findings on the MCP permissions PR: - filterAuthorizedTools previously fast-accepted any tool present in the global tool cache before reaching the MCP-use permission gate. App-level MCP tools (keyed `name_mcp_server` by MCPServerInspector and merged into the cache via mergeAppTools) therefore bypassed the canUseMCP check, letting a user without MCP_SERVERS.USE persist/bind them. Route all MCP-delimited tools through the permission + server-access gate regardless of cache presence. - assertImageDataWithinLimit / image formatter used startsWith("http") to skip the size cap, which also matched base64 payloads that happen to begin with those chars. Require http:// or https:// via a shared isRemoteImageUrl helper so oversized inline base64 can no longer bypass MCP_IMAGE_DATA_MAX_BYTES. Adds regression tests for both paths. * fix: address Codex round-2 findings on MCP permissions PR - parsers.ts: parseAsString dropped the image payload for unrecognized providers, returning only `Image result: <mimeType>`. Pre-PR these items survived via JSON.stringify(item). Keep the size guard but fall through to JSON.stringify so the data/URL is preserved. - MCP.js: the runtime MCP-use check only read `configurable.user`, so paths that propagate `user_id` only (e.g. the OpenAI-compatible API in agents/openai/service.ts) rejected every MCP tool call for an authenticated user. Add resolveMCPPermissionUser: use the safe user directly when it already carries a role (no extra DB call), otherwise fall back to loading the role by user_id. Update fail-closed tests to the resolved behavior. - v1.js: the update path only re-filtered newly added MCP tools, so a user who lost MCP_SERVERS.USE kept existing MCP bindings on edit while create/duplicate/revert stripped them. Strip all MCP tools on update when the permission is revoked; keep the narrower new-tool gating (and disconnect/registry preservation) when it is intact. Updates and adds regression tests for all three paths. * fix: populate safe user at producer instead of resolving in runtime MCP check Corrects the Finding B approach from the previous commit. Rather than loading the user by id inside the runtime MCP permission check, populate `configurable.user` (and createRun's `user`) with the full safe user at the producer, matching the in-repo agent controllers (responses.js / openai.js) which already pass `createSafeUser(req.user)`. - service.ts: derive `safeUser` via createSafeUser(req.user) and pass it to both createRun and processStream's configurable, so the role-bearing identity reaches the runtime `userCanUseMCPServers(configurable.user)` check. Falls back to a bare id when the host app attached no user, which correctly leaves MCP gated (fail closed). - MCP.js: revert the resolveMCPPermissionUser DB-load fallback; the runtime check again reads configurable.user directly and fails closed when absent (defense in depth). - MCP.spec.js: revert to the matching runtime test expectations. * test: cover safe-user propagation in createAgentChatCompletion Adds a focused spec for the OpenAI-compatible chat completion service (the producer fixed for Codex Finding B). Injects mocked deps and asserts that createRun and processStream's configurable.user carry the role from req.user (with sensitive fields stripped by createSafeUser), and that an unauthenticated request falls back to a bare { id: 'api-user' } so the runtime MCP check fails closed. * fix: address Codex round-3 findings + TS6133 - MCP.js (P1): the assistants required-action path invokes tool._call( toolInput) with no LangChain config, so the runtime check saw no configurable.user and rejected authorized users. createToolInstance now captures the creation-time user (req.user via createMCPTool) and _call falls back to it for both the permission check and userId. Still fails closed when neither config nor captured user carries a role. - v1.js (P2): the update-path isMCPTool used a bare mcp_delimiter substring check, misclassifying action tools whose operationId contains "_mcp_" (e.g. sync_mcp_state_action_...) as MCP and dropping them on a permission-revoked edit. Delegate to the canonical isActionTool so only real MCP tools are gated. Regression test added. - service.ts: drop the now-unused IUser import (TS6133); derive reqUser's type from createSafeUser's own parameter instead. * fix: resolve TS7022 self-reference in service.spec mock res The mock response object referenced `res` inside its own `status`/`json` initializers without a type annotation, so tsc inferred `res` as `any` (TS7022). Annotate the object and assign the self-referencing chainable methods after declaration. * fix: correct round-4 findings (isActionTool import, captured user, partial-update) - v1.js: import isActionTool from librechat-data-provider (its real export; @librechat/api does not export it, so the prior import was undefined and threw TypeError). Exclude action tools from MCP classification in both the main filterAuthorizedTools loop and the update path, so action tools whose operationId contains _mcp_ (e.g. sync_mcp_state_action_...) are preserved regardless of MCP permission. - v1.js: evaluate the effective tool set (updateData.tools ?? existingAgent.tools) so a tools-less PATCH by a user who lost MCP_SERVERS.USE still strips stale MCP bindings, matching create/duplicate/revert. - MCP.js: createToolInstance now receives the construction-time user and _call falls back to it (permissionUser) when configurable.user is absent, fixing the assistants required-action path that invokes _call without a config and resolving the capturedUser no-undef/ReferenceError. - Tests: action-tool preservation (authorized + denied), tools-less revocation PATCH, updated revocation test to expect all MCP tools stripped. Affected specs pass locally: MCP 49/49, filterAuthorizedTools 49/49. * fix: guard isActionTool against non-string tools; correct actionDelimiter import Two test regressions from the prior commit: - The main filterAuthorizedTools loop called isActionTool(tool) directly, but isActionTool does toolName.indexOf(...) and throws on null/undefined. Compute isActionToolName = typeof tool === 'string' && isActionTool(tool) once and reuse it, restoring graceful null/undefined handling. - The action-tool test referenced Constants.actionDelimiter (undefined); actionDelimiter is a standalone librechat-data-provider export. Import and use it directly. filterAuthorizedTools 36/36 and MCP 40/40 pass locally. * fix: address MCP permission review follow-ups * fix: preserve shared agent MCP tools	2026-05-30 16:19:49 -04:00
Danny Avila	5bfef51ed2	🏟️ fix: Restrict MCP OAuth Audience in User-Managed Configs (#13418)	2026-05-30 14:39:54 -04:00
Freudator86	c6a6f2e3ae	🪪 feat: MCP OAuth - Support audience parameter for Auth0/Cognito-style providers (#13402) ... * feat(mcp/oauth): support audience parameter for Auth0/Cognito-style providers LibreChat already follows RFC 9728 (Protected Resource Metadata discovery) and RFC 8707 (resource indicators on /authorize). However, authorization servers that pre-date RFC 8707 — most prominently Auth0 — issue API-scoped access tokens only when an Auth0-specific 'audience' parameter is supplied on /authorize and /token. Without it, refresh_token responses strip the API audience and the next MCP call 401s. This change adds an optional 'audience' field to OAuthOptionsSchema and forwards it on: * pre-configured authorize URL build * discovered (DCR + RFC 9728) authorize URL build * refresh_token grant body 'resource' (RFC 8707) is left untouched and remains the standards-conformant route; 'audience' covers providers that ignore 'resource'. The two are independent — providers may accept either, both, or neither, so we forward whichever the operator configures. Schema tests added; no behavioral change for existing configs (field is optional with no default). Refs: MCP Authorization Spec 2025-06-18, RFC 9728, RFC 8707. * ci: build audience-fix branch image to ghcr.io/freudator86/librechat:audience-fix * Revert "ci: build audience-fix branch image to ghcr.io/freudator86/librechat:audience-fix" This reverts commit 7b3dfa6cd7. * tests: assert audience param in authorize URL + refresh body; tighten schema (.min(1)); refine comment to reflect actual code paths Adresses PR review: - audience: z.string().min(1).optional() rejects empty strings - schema comment now precisely lists the two code paths (authorize + refresh_token grant); explicitly notes the authorization_code exchange intentionally does not receive audience because Auth0 binds it from the initial /authorize request - new MCPOAuthAudience.test.ts: 4 cases — authorize URL with/without audience, refresh body with/without audience — using a local recording HTTP server (no shared helper changes) - new schema test: empty-string audience is rejected * style: inline two logger.debug calls (prettier) * style: inline third audience-debug log (prettier) * feat(mcp/oauth): add forward_audience_on_refresh opt-out for strict token endpoints (Cognito) Addresses Codex review P2 'Avoid sending audience on refresh grants': the previous behavior forwarded audience on every refresh_token grant, which is correct for Auth0 (strips the audience claim otherwise) but is non-standard for Cognito and other strict OAuth 2.0 token endpoints that document refresh as grant_type + client_id + refresh_token only. New optional boolean 'forward_audience_on_refresh' (default: true) preserves the existing Auth0-friendly default while letting operators of strict tenants opt out cleanly. Schema + handler tests cover both cases. No behavioral change for existing configs. * style: format MCP OAuth refresh audience log --------- Co-authored-by: Tim Freudenthal <tim@allesknut.de> Co-authored-by: Danny Avila <danny@librechat.ai>	2026-05-30 06:59:39 -07:00
Danny Avila	a83bcb3490	🪃 fix: Retry MCP OAuth Token Refresh Without Scope on Server Rejection (#13412) ... LibreChat sends `scope` on the refresh_token grant by default (PR #7924) because some authorization servers expect it. Salesforce rejects any scope on refresh with HTTP 400 "scope parameter not supported" (confirmed in production logs for case 00046259), which broke token refresh and forced re-authentication — amplifying the multi-replica PKCE retry storm. RFC 6749 §6 makes scope optional on refresh (the server reuses the original grant). postRefreshRequest now sends scope as before and retries once WITHOUT it only when the failure is specifically a scope-parameter rejection (isScopeParameterRejection), so servers that need scope are unaffected and Salesforce-like servers self-heal with no operator config.	2026-05-30 01:52:43 -04:00
Dustin Healy	2bcf3e8582	🪟 fix: Apply Admin-Panel Config Overrides To YAML-Defined MCP Servers (#13173) ... * 🪟 fix: Apply Admin-Panel Config Overrides To YAML-Defined MCP Servers Admin-panel saves of MCP server fields for YAML-defined servers were silently dropped by the registry. ensureConfigServers filtered out any merged config entry whose name appeared in YAML, so overrides such as iconPath, title, and description never reached getAllServerConfigs even though the override row had been written to the configs collection and the AppConfig merge layer had produced the correct merged result. The filter is removed and replaced with a content-equivalence short-circuit in ensureSingleConfigServer. YAML-defined servers whose merged config matches the YAML cache entry skip lazy-init, so unmodified YAML servers still avoid a redundant inspection round trip. The new private helper matchesYamlConfig reuses the existing content-hash function on configurable fields only. getAllServerConfigs now overlays config-tier entries onto the YAML base while preserving user-DB entries (source: 'user'), giving precedence of YAML, then Config tier, then User DB. The docstring is updated to describe the new order. Multi-tenancy is already enforced upstream of the registry by the AppConfig layer, so the registry stays tenant-agnostic and overrides remain isolated per tenant. Tests cover the new behavior: config-tier override on YAML-defined server flows through to getAllServerConfigs, YAML servers without effective overrides skip lazy-init, user-DB entries win over config-tier overlays, pure config-tier servers still lazy-init, and the merged config passed to lazy-init preserves all YAML fields when the override only adds new ones. * 🛠️ fix: Address Review Feedback For YAML Override Precedence Both Copilot and Codex flagged matchesYamlConfig as broken in production: the cached YAML config carries inspector-derived defaults (requiresOAuth defaulted to false when YAML omits it, serverInstructions rewritten from the YAML toggle to the fetched server-instructions string, and so on) that are absent from appConfig.mcpConfig. The content-hash comparison reports a mismatch for every YAML server with no admin-panel override, so ensureSingleConfigServer re-inspects all of them anyway. The optimization never fires in practice and reintroduces the source tagging it was supposed to avoid. Remove the short-circuit and the matchesYamlConfig helper. YAML-defined servers that appear in the merged config go through lazy-init like any other entry. A smarter optimization that overlays cosmetic-only override fields onto the YAML cache without re-inspection belongs in a follow-up once the boundary between configurable fields and inspector-derived fields is well defined. Update the existing ensureConfigServers tests that asserted the old filter behavior (should exclude YAML servers from config-source detection, should return empty when all servers are YAML) to assert the new behavior: YAML servers pass through ensureConfigServers and are lazy-initialized when they appear in the merged config. Make the inspector mock more realistic by spreading the raw input first and overlaying only runtime fields, so the test fixtures match production where the inspector preserves configurable fields. Drop the companion test in MCPServersRegistry.test.ts that asserted the short-circuit fires for unchanged YAML servers; the hand-crafted fixture skipped inspector defaults and was not representative. Copilot also flagged that getServerConfig short-circuits to configServers before checking the user DB, so the precedence enforced in getAllServerConfigs (user-DB beats config-tier) was bypassed in the single-server lookup. When configServers carries an entry and a userId is available, check the user DB first and prefer a source: 'user' entry so per-user servers are never shadowed by an admin-panel override. * 🛡️ fix: Harden Admin Override Overlay Against Failure Stubs Hardens the admin-panel override path against transient inspect failures and removes a defensive branch that guarded an impossible state. The getAllServerConfigs overlay now skips failed-inspection stubs so a healthy YAML or DB entry stays visible during the 5-minute retry window instead of being clobbered by a stub. When the overlay does land, the base entry's source tier is preserved, which keeps Tools/mcp.js routing its failed-inspection recovery to the correct storage location. The user-DB precedence block in getServerConfig is removed: configServers is built from appConfig.mcpConfig which only ever carries admin-tier entries, so the DB lookup defended a state that cannot occur via the current call graph. The dead yamlServerNames memoization is also gone. Adds two regression tests covering inspection-failure preservation and source-tier preservation on successful overlay, and adds a debug log when an admin override is suppressed by a user-tier entry. The makeParsedConfig test factory now honors overrides correctly. * 🧪 test: Strengthen Admin Override Coverage And Docs Adds an end-to-end regression test that chains MCPServerInspector.inspect failure through ensureConfigServers and getAllServerConfigs, asserting the healthy YAML base entry survives a transient inspect failure intact. The previous regression test hand-built a failure stub and skipped ensureSingleConfigServer, leaving the production chain itself untested. The getAllServerConfigs docstring now spells out both overlay guards (failed-stub skip and user-tier preservation) and the source-field preservation contract that downstream recovery logic depends on. The yamlLangfuseConfig test fixture is frozen so a future test cannot mutate it and contaminate sibling tests in the describe block. * 🔧 fix: Skip Lazy-Init For Unchanged YAML MCP Servers Adds an admin-configurable-field equivalence check so YAML-defined MCP servers that carry no admin override skip lazy-init in ensureConfigServers. This avoids the per-request inspect storm and keeps unmodified YAML servers out of the config-tier cache, so admin saves that touch unrelated overrides no longer evict and tear down those YAML connections. A second guard in getServerConfig prevents failed-inspection stubs in configServers from shadowing the healthy YAML base entry for the duration of the retry window. The aggregate path already had this guard via getAllServerConfigs; this brings the single-server path to parity, so Tools/mcp.js recovery routes to YAML reinspection rather than bailing on the config retry timer. Adds three regression tests covering the unmodified-YAML skip, the admin-override lazy-init trigger, and the failure-stub fallthrough. Updates two existing ensureConfigServers tests that previously documented the now-incorrect "always lazy-init YAML" behavior. * 🔓 feat: Expose baseOnly Flag On Admin Config Base Endpoint The admin getBaseConfig handler now reads req.query.baseOnly and forwards it to getAppConfig so an admin panel client can request the un-merged YAML and AppService base configuration without DB overrides applied. The flag is opt-in; existing callers see no behaviour change because the default remains the merged response. The query value is coerced through String() so Express array forms like baseOnly=true&baseOnly=true are treated as false rather than truthy by accident. A handler test pins the forwarding behaviour and the default-merged behaviour against future regressions. * 🧹 fix: Address Codex Review Findings On MCP Registry Precedence Path Four follow-ups from the Codex review of PR #13173: P1. getServerConfig now preserves the configServers candidate as a last-resort fallback when both YAML cache and user DB return nothing, so admin-defined config-only servers carrying inspectionFailed=true still surface the failure stub to callers in api/server/services/Tools/mcp.js that rely on it to return the still-unreachable message. The not-found memoization is preserved. P2a. proxy is added to ADMIN_CONFIGURABLE_FIELDS so an admin override on SSE/streamable-http proxy is no longer treated as an unchanged YAML server and correctly triggers lazy-init. P2b. isUnmodifiedYamlServer now treats absent-on-rawConfig fields as equal, so inspector-derived values on the cached YAML entry (notably requiresOAuth filled in by detectOAuth at startup) do not force unmodified YAML servers to re-init on every request. P3. getBaseConfig parses ?baseOnly strictly against the literal string true instead of String-coercing, so array shapes like baseOnly[]=true no longer pass through. Regression tests cover all four paths. * 🧹 fix: Drop Misleading Shadow Warning On Config Vs User-DB Collisions The Config-tier branch of warnOnOperatorManagedNameCollisions logged that Config MCP servers shadow DB-backed servers, but getAllServerConfigs actually preserves the user-tier entry on a Config-vs-user collision and skips the override. The warning was describing the opposite of what the code does and would mislead operational debugging. The YAML-tier call is unchanged because YAML still legitimately shadows DB-backed servers. The per-entry debug log inside the collision branch already captures the actual outcome. Test renamed and rewritten to assert the user-tier entry is preserved and no shadow warning is emitted. * 🔒 fix: Keep Tenant-Scoped configServers Candidate Out Of The Global Read-Through Cache The prior fix for surfacing inspectionFailed stubs from admin-defined config-only servers wrote the per-call configServers candidate into readThroughCache when YAML and DB both missed. The cache key is keyed by serverName plus userId, so a failed stub from one tenant could satisfy a later no-userId lookup made by another tenant before any configServers resolution ran. getServerConfig now caches only the global YAML/DB resolution (still caching undefined to memoize not-found lookups) and uses the candidate strictly as an unmemoized function-level fallback that surfaces the failure stub to the caller without leaking it across tenants. Regression test exercises a no-userId call after a tenant-scoped failure and asserts the cache returns undefined rather than the stub, and that a second tenant sees their own healthy candidate. * 🔄 fix: Mirror getAllServerConfigs Precedence Exactly In getServerConfig getServerConfig was short-circuiting with the configServers candidate on every healthy lookup, which made single-server callers diverge from the aggregate path for name collisions between config-tier overrides and user-DB entries. The aggregate path preserves the user-tier entry on such collisions, so single-server callers saw the admin override while list views saw the user server for the same name. getServerConfig now resolves the YAML/DB base first and applies the same four-step precedence used in getAllServerConfigs: 1. user-tier base wins absolutely over a config-tier candidate 2. healthy YAML/DB base wins over a failed (inspectionFailed) candidate 3. healthy candidate overlays its fields onto the base, preserving the base entry's source tag so downstream recovery routes to the correct storage location 4. with no base, the candidate is returned as-is for config-only servers readThroughCache still memoizes only the global YAML/DB lookup, so the per-call configServers candidate never enters the cache and the tenant-isolation guarantee from the previous fix is preserved. Regression tests cover the user-wins-over-config case and the YAML-overlay-with-yaml-source-preserved case. * ⚡ perf: Batch YAML Cache Read In ensureConfigServers isUnmodifiedYamlServer was calling cacheConfigsRepo.get(serverName) per entry. In the Redis aggregate-key backend, get() is implemented as getAll() then map lookup, so N concurrent per-server lookups inflate into N full-map reads and deserializations on every ensureConfigServers pass. The loop now takes a single getAll() snapshot at the top and hands it into a synchronous isUnmodifiedYamlServer helper, turning O(n) remote reads into O(1) regardless of how many MCP entries are resolved. The snapshot also gives the unchanged-YAML comparison one consistent view of YAML across all entries. Regression test spies on cacheConfigsRepo.get and asserts it is never called from ensureConfigServers, with getAll called exactly once.	2026-05-23 17:11:13 -04:00
Danny Avila	5071b2b617	🧷 fix: Pin MCP OAuth Client Secrets (#13276) ... * fix: Pin MCP OAuth client secrets * fix: Require MCP OAuth client id for secrets	2026-05-23 16:51:45 -04:00
Danny Avila	7cd467a528	🧷 fix: Harden MCP Proxy SSRF Checks (#13274)	2026-05-23 16:30:13 -04:00
Max	1746153c17	🪬 fix: Skip MCP Tools When Required Custom User Vars Are Unset (#13152) ... * fix: skip MCP tools when required customUserVars are unset (#10969) * fix: whitespace-only values Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: guard MCP registry lookup and unknown server config in customUserVars gate * fix: fail closed on MCP registry lookup errors --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Danny Avila <danny@librechat.ai>	2026-05-23 09:24:29 -04:00
janluedemann-esome	abda15f4eb	🛂 fix: Detect OAuth Errors From HTTP 400 Responses (#11961) ... * fix(mcp): detect non-standard OAuth errors from servers returning HTTP 400 * add tests for oauth error check * fix(mcp): align factory OAuth error detection	2026-05-23 09:09:13 -04:00
apuzikov	fb851cae63	🪪 fix: Allow Optional client_secret for MCP OAuth (#12460) ... * fix: Allow empty client_secret for MCP OAuth configuration * fix: Enhance OAuth client registration logic to support predefined client_id and handle empty client_secret	2026-05-23 09:01:44 -04:00
Dev Chohan	01af63cb52	⌛ fix: Use JWT exp claim for MCP when OAuth token omits expires_in (#13248) ... MCP OAuth access tokens are stored with a 365-day default expiry when the provider's token response omits `expires_in` (only RECOMMENDED per RFC 6749 §5.1). Providers that issue short-lived JWT access tokens but omit `expires_in` (e.g. Salesforce) therefore get tokens treated as valid for a year and never refreshed, so every call 401s once the real token lapses until the user manually reconnects. When the access token is a JWT (RFC 9068), read its `exp` claim and use it as the authoritative expiry, falling back to the 365-day default only for opaque tokens. Explicit `expires_at`/`expires_in` still take precedence. Adds unit tests for storeTokens expiry resolution. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>	2026-05-22 21:20:59 -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	d462bf4113	🪪 fix: Consolidate MCP OAuth Policy (#13254) ... * 🦉 feat: Implement proactive OAuth flow for connections without stored tokens * 🤝 fix: Enhance proactive OAuth flow handling in MCPConnectionFactory * fix: Add timeout handling for proactive OAuth flow in MCPConnectionFactory Co-authored-by: Copilot <copilot@github.com> * fix: Refine proactive MCP OAuth flow * test: Cover proactive OAuth missing handler * fix: Require explicit MCP OAuth signal * fix: Consolidate MCP OAuth policy --------- Co-authored-by: Gil Assunção <gil.assuncao@parceiros.nos.pt> Co-authored-by: Copilot <copilot@github.com>	2026-05-22 20:43:34 -04:00
Danny Avila	c1e071b7a0	🛡️ fix: Harden MCP OAuth Request Handling (#13264) ... * fix: Harden MCP OAuth request handling * fix: Bound MCP OAuth dispatcher cache * fix: Harden OAuth DNS lookup handling	2026-05-22 20:39:16 -04:00
Danny Avila	05a3d1ed81	🛣️ feat: Add MCP Remote Proxy Support (#13076) ... * feat: add MCP remote proxy support * fix: Harden MCP Proxy Review Findings * fix: Honor MCP Proxy Env Precedence * fix: Harden MCP proxy routing * fix: Align MCP proxy bypass semantics * test: Pin MCP proxy admin scope	2026-05-21 15:28:54 -04:00
Danny Avila	f34150e8e8	🧵 chore: Raise MCP SSE Line Default (#13224)	2026-05-21 09:06:02 -04:00
Danny Avila	1ed84ee4eb	🦣 fix: Response Size Limits for Streamable HTTP MCP Responses (#13219) ... * fix: implement response size limits for streamable HTTP MCP responses - Added environment variables for maximum response and line sizes in streamable HTTP responses. - Introduced functions to handle response size validation and error logging. - Updated MCP connection logic to enforce these limits, ensuring safe handling of large responses. * fix: address MCP response guard review findings * fix: satisfy logger spy typings * fix: clarify blocked MCP response errors * fix: harden MCP guard review edge cases * test: cover MCP oversized SSE call failures	2026-05-21 01:29:43 -04:00
Peter Nancarrow	2418f854df	📡 fix: Handle Pre-Session 406 for Optional SSE MCP Stream (#13202)	2026-05-19 20:59:45 -04:00
Danny Avila	749eb06e67	🧭 fix: Reduce MCP Registry ACL Lookups (#13195)	2026-05-19 17:16:37 -04:00
Gaurav Dubey	6466263493	🧹 chore: Type Agent MCP lean projection in ServerConfigsDB (#13171) ... Replace the (a: any) cast and its eslint-disable directive in ServerConfigsDB.getAll() with a precise lean-projection generic. The Mongoose query already projects only mcpServerNames, so we can use Pick<IAgent, 'mcpServerNames'> and let .lean<T[]>() carry the shape through without resorting to any. Switches the empty-array guard from || to ?? for clarity; both behave identically for string[] | undefined. Aligns with the project's "Never use any" rule (CLAUDE.md -> Type Safety). No runtime change.	2026-05-18 15:32:43 -04:00
Danny Avila	ca8c212c0d	🗝️ fix: Protect Model Spec Instructions (#13125) ... * fix: prevent instruction exposure * fix: tighten model spec preset restoration * refactor: type model spec preset handling	2026-05-14 10:07:23 -04:00
Danny Avila	9e8b8c3705	🧰 fix: Scope MCP Registry Initialization To Config Fingerprints (#13115)	2026-05-13 16:50:07 -04:00
Danny Avila	9441563b95	🛡️ refactor: Scope allowedAddresses By Port (#13022) ... * fix: Scope allowedAddresses by port * test: Fix SSRF agent spec typing	2026-05-08 12:28:34 -04:00
Danny Avila	4cce88be42	🪟 feat: Add allowedAddresses Exemption List For SSRF-Guarded Targets (#12933) ... * 🪟 feat: Add allowedAddresses Exemption List For SSRF-Guarded Targets LibreChat already blocks SSRF-prone targets (private IPs, loopback, link-local, .internal/.local TLDs) at every server-side fetch site that consumes user-controllable URLs — custom-endpoint baseURLs, MCP servers, OpenAPI Actions, and OAuth endpoints. The only existing escape hatch is `allowedDomains`, but that flips the field into a strict whitelist: adding `127.0.0.1` to permit a self-hosted Ollama also blocks every public destination that isn't in the list. Introduce `allowedAddresses` as the orthogonal primitive: a private- IP-space exemption list. When a hostname or its resolved IP appears in the list, the SSRF block is bypassed for that target. Public destinations remain reachable. Operators can now run self-hosted LLMs / MCP servers / Action endpoints on private addresses without weakening the default-deny posture for everything else. Schema additions in `packages/data-provider/src/config.ts`: - `endpoints.allowedAddresses` (new — gates `validateEndpointURL`) - `mcpSettings.allowedAddresses` (parallel to `allowedDomains`) - `actions.allowedAddresses` (parallel to `allowedDomains`) Core changes in `packages/api/src/auth/`: - New `isAddressAllowed(hostnameOrIP, allowedAddresses)` — pure, case-insensitive, bracket-stripped literal match. - Threaded the list through `isSSRFTarget`, `resolveHostnameSSRF`, `isDomainAllowedCore`, `isActionDomainAllowed`, `isMCPDomainAllowed`, `isOAuthUrlAllowed`, and `validateEndpointURL`. - Extended `createSSRFSafeAgents` and `createSSRFSafeUndiciConnect` to accept the list, building an SSRF-safe DNS lookup that exempts matching hostnames/IPs at TCP connect time (TOCTOU-safe). Wiring: - Custom and OpenAI endpoint initialize sites pass `endpoints.allowedAddresses` to `validateEndpointURL`. - `MCPServersRegistry` stores `allowedAddresses` and exposes it via `getAllowedAddresses()`. The factory, connection class, manager, `UserConnectionManager`, and `ConnectionsRepository` all thread it through to the SSRF utilities. - `MCPOAuthHandler.initiateOAuthFlow`, `refreshOAuthTokens`, and `validateOAuthUrl` accept the list and consult it on every URL validation along the OAuth chain. - `ToolService`, `ActionService`, and the assistants/agents action routes pass `actions.allowedAddresses` to `isActionDomainAllowed` and to `createSSRFSafeAgents` for runtime action calls. - `initializeMCPs.js` reads `mcpSettings.allowedAddresses` from the app config and forwards it to the registry constructor. Documentation: - `librechat.example.yaml` shows the new field next to each existing `allowedDomains` block, with a note clarifying that `allowedAddresses` is an exemption list (not a whitelist). Tests: - Unit tests for `isAddressAllowed` covering literal IPs, hostnames, IPv6 brackets, case insensitivity, and partial-match rejection. - Exemption tests for every entry point: `isSSRFTarget`, `resolveHostnameSSRF`, `validateEndpointURL`, `isActionDomainAllowed`, `isMCPDomainAllowed`, `isOAuthUrlAllowed`. - Existing tests updated to reflect the new optional parameter. Default behavior is unchanged: omitted = empty list = no exemptions. * 🩹 fix: Plumb allowedAddresses Through AppConfig endpoints Type The initial PR added `endpoints.allowedAddresses` to the data-provider config schema and consumed it in the endpoint initialize sites, but the runtime `AppConfig.endpoints` shape in `@librechat/data-schemas` was a hand-maintained subset that didn't include the new field — so `tsc` rejected `appConfig.endpoints.allowedAddresses`. Add the field to `AppConfig['endpoints']` in `packages/data-schemas/src/types/app.ts` and forward it from the loaded config in `packages/data-schemas/src/app/endpoints.ts` so the runtime config carries the value. Update `initializeMCPs.spec.js` to expect the third positional argument (`allowedAddresses`) on the `createMCPServersRegistry` call. * 🩹 fix: Enforce allowedDomains Before allowedAddresses In isOAuthUrlAllowed The initial implementation checked the address exemption first, so a URL whose hostname appeared in `allowedAddresses` would return true even when the admin had configured `allowedDomains` as a strict bound on OAuth endpoints. A malicious MCP server could advertise OAuth metadata, token, or revocation URLs at any address the admin had permitted for an unrelated reason (a self-hosted LLM at `127.0.0.1`, for example) and pass validation, expanding SSRF reach beyond the configured domain whitelist. Reorder: when `allowedDomains` is set, treat it as authoritative — return true only if the URL matches a domain entry, otherwise fall through to false. The address exemption only applies when no `allowedDomains` is configured (mirrors how the downstream SSRF check in `validateOAuthUrl` consults `allowedAddresses`). Add a regression test asserting that an `allowedAddresses` entry does not broaden a configured `allowedDomains` list. Reported by chatgpt-codex-connector on PR #12933. * 🩹 fix: Forward allowedAddresses To Remaining OAuth Callers Two `MCPOAuthHandler` callers still used the pre-feature signatures and were silently dropping the new `allowedAddresses` argument: - `api/server/routes/mcp.js` invoked `initiateOAuthFlow` with the old 5-argument shape, so OAuth flows initiated through the route handler ignored the registry's `getAllowedAddresses()` and would reject any metadata/authorization/token URL on a permitted private host. - `api/server/controllers/UserController.js#maybeUninstallOAuthMCP` invoked `revokeOAuthToken` without the address exemption, so uninstalling an OAuth-backed MCP server on a permitted private host would fail at the revocation step even though the rest of the MCP connection path now permits it. Both sites now read `allowedAddresses` from the registry alongside `allowedDomains` and forward it. Reported by Copilot on PR #12933. * 🩹 fix: Update Test Mocks And Assertions For OAuth allowedAddresses The previous commit started passing `allowedAddresses` to `MCPOAuthHandler.initiateOAuthFlow` from `api/server/routes/mcp.js` and to `MCPOAuthHandler.revokeOAuthToken` from `api/server/controllers/UserController.js`, but the corresponding test files mocked the registry without `getAllowedAddresses` (causing `TypeError`s) and asserted the old positional shape on `toHaveBeenCalledWith`. Update the mocks and assertions to match the new arity: - `api/server/routes/__tests__/mcp.spec.js`: add `getAllowedDomains`/`getAllowedAddresses` to the registry mock and expect the additional positional args on `initiateOAuthFlow`. - `api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js`: add a `getAllowedAddresses` mock alongside the existing `getAllowedDomains` and seed it in `setupOAuthServerFound`. - `api/server/controllers/__tests__/UserController.mcpOAuth.spec.js`: add `getAllowedAddresses` to the registry mock and expect the trailing `null` arg on the three `revokeOAuthToken` assertions. * 🛡️ fix: Address Comprehensive Review — Scope allowedAddresses To Private IP Space Major findings from the comprehensive PR review (severity → fix): **CRITICAL — `validateOAuthUrl` SSRF fallback bypass.** When `allowedDomains` is configured and a URL fails the whitelist, the SSRF fallback in `validateOAuthUrl` was still passing `allowedAddresses` to `isSSRFTarget` / `resolveHostnameSSRF`, letting a malicious MCP server advertise OAuth endpoints at any address the admin had permitted for an unrelated reason. Suppress `allowedAddresses` in the fallback when `allowedDomains` is active — the address exemption is opt-in for the no-whitelist mode only. **MAJOR — WebSocket transport SSRF check ignored exemptions.** The `constructTransport` WebSocket branch called `resolveHostnameSSRF(wsHostname)` without `this.allowedAddresses`, so a permitted private MCP server would pass `isMCPDomainAllowed` but be blocked at transport creation. Forward the exemption. **Scope `allowedAddresses` to private IP space only (operator directive).** The exemption list is for permitting private/internal targets; it must not be a back-door to broaden trust to public destinations. - Schema (`packages/data-provider/src/config.ts`): new `allowedAddressesSchema` rejects URLs (`://`), paths/CIDR (`/`), whitespace, and public IPv4/IPv6 literals at config-load time. Wired into `endpoints`, `mcpSettings`, and `actions`. - Runtime (`packages/api/src/auth/domain.ts`): `isAddressAllowed` now drops public-IP candidates and public-IP entries on the match path — defense in depth so a misconfigured runtime list never grants exemption. - Hot path (`packages/api/src/auth/agent.ts`): `buildSSRFSafeLookup` pre-normalizes the list into a `Set<string>` once at construction and applies the same scoping filter, so the connect-time DNS lookup is an O(1) Set membership check instead of a full re-iterate-and-normalize on every outbound request. **Test coverage for the connect-time and OAuth-fallback paths.** - `agent.spec.ts`: new describe block exercising `buildSSRFSafeLookup` and `createSSRFSafe*` with `allowedAddresses` — hostname-literal exemption, resolved-IP exemption, public-IP scoping, URL/CIDR/whitespace rejection, and the default no-list block. - `handler.allowedAddresses.test.ts` (new): integration tests for `validateOAuthUrl` — covers both the no-domains-set "permit private" path and the strict-bound regression where `allowedAddresses` must NOT bypass `allowedDomains`. **Documentation & cleanup.** - `connection.ts` redirect SSRF check: explicit comment that `allowedAddresses` is intentionally NOT consulted for redirect targets (server-controlled, must not inherit the admin's exemption). - `MCPConnectionFactory.test.ts`: replaced an `eslint-disable` with a proper `import { getTenantId } from '@librechat/data-schemas'`. The disable was added to make a pre-existing `require()` quiet — the cleaner fix is to use the existing top-level import. Updated `MCPConnectionSSRF.test.ts` WebSocket SSRF assertions to match the new two-argument call shape (`hostname, allowedAddresses`). * 🩹 fix: Require Absolute URL Before allowedAddresses Trust Bypass In isOAuthUrlAllowed `parseDomainSpec` is lenient — it silently prepends `https://` to schemeless inputs so it can match patterns like bare `example.com`. That leniency leaked into `isOAuthUrlAllowed`'s new `allowedAddresses` short-circuit: a value like `10.0.0.5/oauth` (no scheme) would parse successfully via the prepended default, hit the address-exemption path, return `true`, and skip `validateOAuthUrl`'s strict `new URL(url)` parse-or-throw — only to fail later in OAuth discovery with a less clear runtime error. Add a strict `new URL(url)` gate at the top of `isOAuthUrlAllowed`. Schemeless inputs now fall through to `validateOAuthUrl`'s explicit "Invalid OAuth <field>" rejection. Tests added in both `auth/domain.spec.ts` (unit) and the OAuth handler integration spec (end-to-end). Reported by chatgpt-codex-connector (P2) on PR #12933. * 🛡️ fix: Address Follow-Up Comprehensive Review — Schema Tests, Shared Normalization, host:port Auditing the second comprehensive review: **F1 MAJOR — schema validation untested.** `allowedAddressesSchema` had zero coverage, so a regression in the three refinement stages or the three wiring locations (`endpoints` / `mcpSettings` / `actions`) would silently let invalid entries reach the runtime. Added a dedicated `describe('allowedAddressesSchema')` block in `config.spec.ts` covering: valid private IPs (v4 + v6, including the previously-missed 192.0.0.0/24 range), accepted hostnames, all rejection categories (URLs, CIDR, paths, whitespace tabs/newlines, host:port, public IP literals), and full `configSchema.parse()` integration at each of the three nesting points. **F2 MINOR — `isPrivateIPv4Literal` divergence.** The schema reimpl in `packages/data-provider` was discarding the `c` octet, so the `192.0.0.0/24` (RFC 5736 IETF protocol assignments) range that the authoritative `isPrivateIPv4` accepts was being rejected with a misleading "public IP" error. Destructure `c` and add the missing range check; covered by the new schema tests. **F3 MINOR — DRY violation across `domain.ts` and `agent.ts`.** Both files had independent normalization implementations with a subtle whitespace-check divergence (`/\s/` vs `.includes(' ')`). Extracted the shared logic into a new `packages/api/src/auth/allowedAddresses.ts` module that both consumers import: - `normalizeAddressEntry(entry)` — single-entry shape check - `looksLikeHostPort(entry)` — host:port detector (used by F4) - `normalizeAllowedAddressesSet(list)` — pre-normalized Set for the connect-time hot path - `isAddressInAllowedSet(candidate, set)` — membership check that enforces private-IP scoping on the candidate Both `isAddressAllowed` (preflight) and `buildSSRFSafeLookup` (connect) now go through the same primitives; the whitespace divergence is gone. To break the import cycle (`allowedAddresses` needs `isPrivateIP`, `domain` previously owned it), extracted IP private-range detection into a leaf `auth/ip.ts` module. `domain.ts` re-exports `isPrivateIP` for backward compatibility with existing call sites. **F4 MINOR — `host:port` silently misclassified.** Entries like `localhost:8080` previously slipped through the URL/path guard, were mis-detected as IPv6, failed `isPrivateIP`, and were silently dropped with a misleading "public IP" schema error. Added an explicit `looksLikeHostPort` check with a clear error: "allowedAddresses entries must not include a port — list the bare hostname or IP only." Bare `::1`, `[::1]`, and other valid IPv6 literals are intentionally not matched (regex distinguishes by colon count and the bracketed `[ipv6]:port` form). **F5 MINOR — hostname-trust documentation gap.** Hostname entries short-circuit `resolveHostnameSSRF` before any DNS lookup — that's a deliberate design (admin trusts the name) but it means the exemption follows whatever the name resolves to at runtime. Added an explicit note in `librechat.example.yaml` for both `mcpSettings.allowedAddresses` and `endpoints.allowedAddresses`: "a hostname entry trusts whatever IP that name resolves to. Only list hostnames whose DNS you control. Prefer literal IPs when you can." **F6** (8 positional params) is flagged for follow-up; refactor to an options object is a breaking-API change deferred to a separate PR. **F7** (redirect/WebSocket asymmetry, NIT, conf 40) — skipping; the existing inline comment is sufficient. * 🧹 chore: Address Follow-Up NITs — Import Order And Mirror-Function Naming Three NITs from the latest comprehensive review: **NIT #1 (conf 85) — local import order.** AGENTS.md requires local imports sorted longest-to-shortest. Both `domain.ts` and `agent.ts` had `./ip` (shorter) before `./allowedAddresses` (longer). Swapped. **NIT #2 (conf 60) — missing cross-reference.** The schema-side `isHostPortShape` in `packages/data-provider/src/config.ts` had no note pointing at the canonical runtime mirror. Added a JSDoc paragraph explaining the mirror relationship and why a local copy exists (the data-provider package can't import from `@librechat/api` without creating a circular dependency). **NIT #3 (conf 50) — naming inconsistency.** Renamed `isHostPortShape` → `looksLikeHostPort` so the schema mirror matches the runtime helper exactly. Kept as a separate function (not a shared import) for the same circular-dependency reason; the matching name makes it obvious they should stay in lockstep.	2026-05-03 21:43:59 -04:00
Danny Avila	41a6d6d11c	🛡️ fix: Harden MCP Redirect SSRF Checks (#12931) ... * fix: Harden MCP redirect SSRF checks * fix: Address MCP redirect review feedback * test: Tighten MCP SSRF redirect assertions	2026-05-04 09:58:47 +09:00
Nick Haffie-Emslie	3758380c61	🔌 fix: Follow 307/308 redirects in MCP streamable HTTP transport (#12850) ... * 🔌 fix: Follow 307/308 redirects in MCP streamable HTTP transport Some MCP servers (e.g. Coda) return 308 Permanent Redirect to route doc-scoped tool calls to a different endpoint path. The fetch wrapper used `redirect: 'manual'` for SSRF protection, which silently dropped these redirects and caused tool calls to fail with empty error bodies. Follow 307/308 redirects (method-preserving per RFC 7538) up to a depth of 5. SSRF safety is preserved because the same undici Agent with its SSRF-safe connect function validates redirect targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * 🛡️ fix: Harden MCP 307/308 redirect handling against SSRF and credential leaks - Validate every redirect target against `resolveHostnameSSRF` so allowlist deployments (which disable connect-time SSRF protection) still block hops to private/reserved IPs. - Strip `Authorization`, `Cookie`, `mcp-session-id`, and any user-injected headers when a 307/308 crosses an origin boundary, mirroring browser/Fetch behavior so a redirecting MCP server can't exfiltrate credentials. - Cancel the intermediate response body before each next hop so undici can reuse pooled sockets rather than holding them until GC. - Restructure redirect test helpers to be same-origin (matching real-world Coda-style routing), drop dead setup code, fix the misleading "5 hops successfully" test, and add coverage for SSRF-blocked redirects, cross- origin credential stripping, and same-origin credential preservation. * 🛡️ fix: Also strip `serverConfig.headers` on cross-origin MCP redirects Previously only runtime `setRequestHeaders` keys were treated as secret on a 307/308 cross-origin hop. API keys baked into `serverConfig.headers` (passed through `requestInit.headers` at transport construction time) survived stripping, so a malicious MCP endpoint could exfiltrate them by returning a cross-origin `Location`. Pass the configured header keys through to `createFetchFunction` so both runtime and config secrets are stripped. The cross-origin credential test now also configures `serverConfig.headers` to lock in this behavior. * 🧹 chore: Tighten MCP redirect-stripping coverage and helper duplication - Add `proxy-authorization` to the cross-origin forbidden header set so a forward-proxy credential header would also be stripped on a cross-origin hop, matching the Fetch-spec list. - Strengthen the cross-origin credential test with positive assertions that benign protocol headers (`accept`, `content-type`) survive the hop, so a regression that strips everything indiscriminately would now fail. - Extract the duplicated MCP request handler / session-teardown logic from three test helpers into shared `createMCPRequestHandler` and `closeMCPSessions` utilities. * 🛠️ fix: Handle `Request` inputs in MCP `customFetch` URL derivation `customFetch` is typed to accept `UndiciRequestInfo` (`string | URL | Request`), but `Request.prototype.toString()` returns `"[object Request]"`. The previous implementation derived `originalOrigin` and the redirect base via `url.toString()`, so a `Request` input would throw inside `new URL(...)` before any network call — a regression even when no redirect was involved. Add a `getRequestUrlString` helper that extracts the URL string for all three shapes, track the URL string alongside the fetch input through the redirect loop, and add parameterized tests that exercise `customFetch` with each shape. * 🛠️ fix: Don't override `Request` input headers in MCP `buildFetchInit` Previously `buildFetchInit` always set `headers` on the returned init — even when neither `init.headers` nor runtime headers contributed anything. Passing `headers: {}` to `undiciFetch` overrides the headers carried on a `Request` input (auth tokens, MCP session, protocol negotiation), so Request-based wrappers could fail authentication even without a redirect in play. Skip the `headers` override entirely when there is nothing to merge. Adds a regression test that supplies `Authorization` and a custom header on the `Request` itself and asserts both reach the target server. * 🛠️ fix: Preserve `Request` method/body across MCP redirects + guard cross-origin strip Two regressions surfaced by extending `customFetch` to accept `Request` inputs: 1. **307/308 method/body loss.** The redirect loop switches `url` to the new `Location` string, but the original `Request`'s method and body stayed bound to the (now-discarded) `Request` object. A redirected POST silently became a GET with no payload — the exact behavior the method-preserving codes are designed to prevent. Added a `resolveFetchInput` helper that runs once at the top of `customFetch`, extracts a `Request`'s method/body/headers into the shared init, and buffers the body via `arrayBuffer()` so 307/308 retries can replay it. 2. **Cross-origin strip crashed on absent headers.** After the previous fix that stopped `buildFetchInit` from setting `headers: {}`, `currentInit.headers` could legitimately be `undefined`. The cross-origin branch read it as a `Record` and called `Object.entries` on `undefined`, throwing `TypeError`. Guard the branch on `currentInit.headers != null` — when there are no headers there is nothing to strip. Adds two regression tests: a POST-with-body `Request` that 308-redirects cross-origin (asserts both method and body survive) and a no-headers cross-origin redirect (asserts the strip path no longer crashes). * 🛠️ fix: Forward `Request.signal` through MCP `customFetch` normalization `resolveFetchInput` was copying method/body/headers off a `Request` input but dropping `Request.signal` on the floor, so a caller that wired an `AbortController` onto the `Request` for cancellation/timeouts lost that wiring as soon as we re-shaped the input into the `(string, init)` pair used by the redirect loop. Subsequent aborts no longer reached the in-flight fetch — a regression from the pre-PR code, which forwarded the original `Request` directly to undici. Forward the signal alongside method/body/headers, with explicit `init.signal` still winning per Fetch-spec semantics. Regression test aborts a controller before calling \`customFetch\` with the wired `Request` and asserts the call rejects. * 🧪 test: Pin URL.origin contract for protocol-downgrade redirect handling Audit follow-up. The cross-origin strip path keys off `targetUrl.origin !== originalOrigin`, and `URL.origin` is defined as `scheme + "://" + host + ":" + port`, so a same-host `https → http` redirect produces a different origin and trips the strip path through the existing logic — no separate code path needed. Pin that contract with a small unit test so a future change to URL semantics (or a refactor that swaps in a different comparison) doesn't silently regress protocol-downgrade stripping. Standing up a TLS fixture (self-signed cert, undici skip-verify, etc.) just to re-prove the URL spec is wasted complexity. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-29 22:12:29 +09:00
David Newman	1f37ec842a	🔌 fix: Prevent Repeated Idle Check Triggers for Users With Failed MCP Connections (#12853)	2026-04-29 09:19:35 +09:00
Danny Avila	738003b220	🛡️ fix: Prevent silent crash from unhandled MCP OAuth reconnect rejections (#12812) ... * 🛡️ fix: Install global `unhandledRejection` handler Node 15+ terminates the process by default when a promise rejection goes unhandled. Under MCP OAuth reconnect storms and streamable-HTTP transport resets, fire-and-forget async paths can emit transient rejections (ECONNRESET, token refresh races) that would otherwise silently kill the server — no uncaught exception log, no OOM signal. Register a listener so these paths log and the process keeps serving other requests. Refs: #12078 * 🔧 fix: Guard MCP OAuth reconnect fire-and-forget calls `OAuthReconnectionManager.tryReconnect` awaits `getServerConfig` outside its inner try/catch, so a rejection from the registry (or any throw before the guarded block) would escape the fire-and-forget `void` call sites and propagate as an unhandled rejection — the failure mode behind the silent crashes reported in #12078. Route both call sites through a `safeTryReconnect` wrapper that attaches a terminal `.catch` so unexpected rejections are surfaced via the logger instead. Refs: #12078 * 🧹 fix: Address review findings on MCP OAuth reconnect crash fix - Move `getServerConfig` inside `tryReconnect`'s try/catch so the registry rejection path is handled by the inner cleanup (the structural root cause behind the silent crash). The outer `safeTryReconnect` wrapper remains as defense-in-depth. - Extract the failed-reconnect cleanup as a private `cleanupOnFailedReconnect` method and invoke it from `safeTryReconnect`'s catch as well, so any rejection that does escape the inner try (e.g. a future regression) still resets tracker state instead of leaving the server stuck in `active` for the full `RECONNECTION_TIMEOUT_MS` window. - Update the regression test to assert tracker state is cleaned up (`isActive` cleared, `isFailed` set, `disconnectUserConnection` called) so it can detect the stale-state failure mode it was meant to guard against. - Forward non-Error rejection reasons as-is in the global handler so structured payloads like `{ code: "ECONNRESET", errno: -104 }` survive instead of being collapsed to "[object Object]" by `String()`. Refs: #12078, review of #12812 * 🚑 fix: Restore fail-fast on boot rejection in primary server entry `startServer()` was invoked bare in `api/server/index.js`. Before installing the global `unhandledRejection` handler, a startup rejection (`connectDb`, `getAppConfig`, `performStartupChecks`) terminated the process via Node's default — Kubernetes / the orchestrator restarted the pod immediately. After the handler was added, the same rejection was caught and logged, then the process kept running half-initialized (no HTTP listener) until the liveness probe eventually timed out — slow, indirect recovery instead of a fast restart. Wrap `startServer()` with the same `.catch(() => process.exit(1))` pattern already used in `experimental.js` so boot failures fail-fast. Refs: #12078, codex review of #12812 * 🚑 fix: Fail-fast on post-listen init failure in both server entries The `app.listen` callback in `index.js` and `experimental.js` is async and awaits `initializeMCPs`, `initializeOAuthReconnectManager`, and `checkMigrations`. The callback's promise is detached from `startServer().catch(...)` (the outer catch only sees errors that occurred before `app.listen` was called), so without explicit handling those init rejections used to terminate the process via Node's default and now would be swallowed by the new `unhandledRejection` handler — leaving the HTTP server listening (and passing liveness probes) while MCP / OAuth / migration state is broken. Wrap the post-listen init block in a try/catch that logs and calls `process.exit(1)` so initialization failures stay fail-fast. Refs: #12078, codex review of #12812	2026-04-24 23:18:49 -07:00
Dustin Healy	fc3189b718	🔐 fix: Restore Tenant Context in MCP OAuth Callback (#12782) ... * fix: restore tenant context in MCP OAuth callback for multi-tenant deployments The MCP OAuth callback is a cross-origin redirect from the OAuth provider. SameSite=Strict cookies (including the JWT) are not sent, leaving the callback with no tenant context. With TENANT_ISOLATION_STRICT=true, all DB writes fail. Stores tenantId in flow metadata at OAuth initiation time (when the user is authenticated), then restores it via tenantStorage.run in the callback, wrapping the entire post-validation body. * test: address review findings for tenant context tests - Assert tenantId flows through to initFlow in MCPConnectionFactory test - Add beforeEach to tenant context tests to reset mocks independently	2026-04-22 14:05:51 -07:00
Danny Avila	f5c4deac28	🔐 fix: Prefer WWW-Authenticate resource_metadata Hint for MCP OAuth (#12763) ... * 📦 chore: Bump @modelcontextprotocol/sdk to v1.29.0 * ♻️ refactor: Extract WWW-Authenticate Probe Helper for MCP OAuth * 🔐 fix: Prefer WWW-Authenticate resource_metadata Hint for MCP OAuth Per RFC 9728 §5.1, the `resource_metadata=<url>` parameter in a 401 `WWW-Authenticate: Bearer` challenge is the authoritative protected-resource metadata source. Path-aware `.well-known` discovery was winning over the hint, so split deployments that serve valid-but-wrong metadata at the path-aware endpoint stranded OAuth at defunct authorization servers. Threads the hint through `discoverOAuthProtectedResourceMetadata` via `opts.resourceMetadataUrl` in both startup detection and the OAuth handler, matching the behavior of Claude Desktop, the MCP Inspector, OpenAI tooling, and Microsoft Copilot Studio. Fixes #12761. * 🧵 fix: Thread OAuth-Aware fetchFn Through Resource-Metadata Probe Without this, admin-configured `oauthHeaders` (e.g. a gateway API key that fronts the MCP endpoint) were stripped from the probe, causing the gateway to 401 for the wrong reason and masking the real `WWW-Authenticate` hint. The helper now accepts a FetchLike and defaults to global fetch, so the startup detection path is unchanged while the handler passes its OAuth- aware wrapper through. * 🧹 refactor: Address MCP OAuth Probe Review Findings - Thread `fetchFn` through `probeResourceMetadataHint` so admin-configured `oauthHeaders` reach the probe (a gateway API key that fronts the MCP endpoint would otherwise 401 us for the wrong reason and hide the real Bearer challenge). - Skip the redundant HEAD request in `checkAuthErrorFallback` when the probe already observed a 401/403; fall back to a fresh HEAD only when every probe attempt threw (transient network error). - Narrow the oauth barrel: drop `export * from './resourceHint'` so the helper stays an internal module. - Add `scope` extraction coverage (`Bearer scope="read write"`) and a 403-only observation path; isolate `MCP_OAUTH_ON_AUTH_ERROR=true` in a dedicated suite so precise-outcome tests aren't muddied by the safety net. * ✅ fix: Use Zod Schema in MCP Reconnection-Storm Test Tool MCP SDK 1.28 tightened `McpServer.tool()` to require Zod schemas instead of plain JSON-Schema objects. Swap the `{ message: { type: 'string' } }` shape for `z.string()` so the fixture server spins up under SDK 1.29. * 🛡️ fix: Harden MCP OAuth resource_metadata Hint Against SSRF The `resource_metadata` URL is echoed from an untrusted MCP server, so handing it straight to the SDK lets a malicious server redirect discovery at private IPs, the cloud metadata service, or any host the admin did not intend to reach. Caught by the Copilot review on #12763. - `handler.ts`: run the hint through the same `validateOAuthUrl` / `allowedDomains` gate that already guards the authorization-server URL; drop it and fall back to path-aware discovery on rejection. - `detectOAuth.ts`: no admin-scoped allowedDomains here, so apply a strict `isSSRFTarget` + DNS resolution check and silently discard any hint pointing at a private/loopback/metadata address. - Tests cover both the hostname-list and DNS-resolution rejection paths and assert the SDK falls back to path-aware discovery unharmed. * 🧪 test: Mock ~/auth in fallback Suite for Consistency Matches the main `detectOAuth.test.ts` mock so the SSRF guards added in the previous commit don't touch the real `~/auth` module at test time. * 🔍 fix: Scope OAuth Fallback to HEAD + Parse Multi-Scheme WWW-Authenticate Two codex findings on #12763: - **P1**: the merged `authChallenge` flag was letting POST-only 401/403 flip the `MCP_OAUTH_ON_AUTH_ERROR` fallback, misclassifying WAF/CSRF-hardened endpoints (HEAD 200 + POST 403) as OAuth-required. Rename to `headAuthChallenge` and derive it only from the HEAD probe, matching the legacy fallback's HEAD-only semantics. Add a regression test. - **P2**: the SDK's `extractWWWAuthenticateParams` only inspects the first scheme token, so multi-scheme headers like `Basic realm="api", Bearer resource_metadata="..."` silently dropped the authoritative Bearer hint. Fall back to a regex across the full header when the SDK returns nothing but Bearer is present. Add a regression test covering the multi-scheme case. * 🧽 refactor: Tighten MCP OAuth Probe Semantics Addresses the second external review pass plus codex P2: - Merge the two stacked JSDoc blocks on `probeResourceMetadataHint` into one with a proper `@returns` section. - Only short-circuit HEAD when it delivered the `resource_metadata` hint itself — a Bearer-without-params HEAD now lets POST run, since some servers surface their hint only on POST and we were missing it. - Drop the unused `scope` field from `ResourceHintProbeResult`; no caller read it, and YAGNI beats a reserved field. - Remove the redundant `OAUTH_ON_AUTH_ERROR` guard inside `checkAuthErrorFallback` — the only call site already gates on it. - Codex P2: signal "HEAD status unknown" via `null` when the HEAD probe threw and POST returned non-auth. Previously that combination leaked a `{headAuthChallenge: false}` result and silently skipped the fallback's retry HEAD, which could misclassify OAuth-required servers after a transient HEAD failure. Regression tests cover every path: Bearer-no-hint-on-HEAD + hint-on-POST, multi-scheme `Basic + Bearer` headers, HEAD-threw + POST-200 retry, and the WAF/CSRF-only POST 403 case. * 🪥 polish: Tighten Probe Null-Guard Ordering + Add Malformed-Hint Test Two NITs from the follow-up review: - Move `bearerChallenge` computation after the `!wwwAuth` guard so the variable is only derived when it can be meaningfully `true`. The early-return path is now a clean unconditional exit. - Add a regression test that asserts `Bearer resource_metadata="not-a-url"` yields `resourceMetadataUrl: undefined` without throwing, locking in the try/catch safety net in `extractHintFromHeader` and the SDK parser alike.	2026-04-21 16:26:20 -07:00
Danny Avila	d1573a8493	🔒 fix: Validate MCP OAuth Protected Resource Metadata binding (#12755) ... * 🔒 fix: Validate MCP OAuth Protected Resource Metadata binding (GHSA-gvpj-vm2f-2m23) RFC 9728 §3.3/§7.3 requires clients to verify that the `resource` identifier advertised by an OAuth Protected Resource Metadata document matches the URL used to fetch it. Without this check, a malicious MCP server can serve metadata pointing at a legitimate server's `authorization_servers`, causing LibreChat to obtain an access token for the real server and send it to the attacker in subsequent API calls. Validation happens at discovery time so the entire metadata document is discarded on mismatch — `authorization_servers` on a spoofed document is equally untrustworthy and is the primary theft vector in the PoC. Uses the MCP SDK's own `checkResourceAllowed` (origin + path-prefix) for semantic parity with `selectResourceURL`, a SDK code path LibreChat bypasses. This is looser than RFC-strict equality (handles common cases like `/mcp/sse` server vs `/mcp` advertised resource, or trailing-slash normalization) while still rejecting cross-origin spoofs and same-origin sibling-path confusion. * 🛡️ fix: Re-validate Resource Binding at MCP OAuth Token Exchange Adds a defense-in-depth re-assertion of the RFC 9728 §3.3 resource/server binding inside `completeOAuthFlow`. Flows have a 10-minute TTL, so a flow initiated under pre-fix (vulnerable) code could still be in-flight at upgrade time carrying unvalidated resource metadata. Re-checking here closes that window without requiring operators to flush flow state on deploy. Also guards against future regressions that might re-introduce unvalidated paths into the flow-metadata pipeline (GHSA-gvpj-vm2f-2m23). * ✅ test: Address Review Findings on MCP OAuth Resource Validation Follow-up to GHSA-gvpj-vm2f-2m23 fix. Resolves three reviewer findings: - Assert `failFlow` is called when `completeOAuthFlow` rejects at re-validation — locks in the "no stuck PENDING entry" guarantee that the catch block already provides in production code. - Update the "no resource metadata" warning in `initiateOAuthFlow`: post-fix, that branch is only reachable when PRM discovery returned nothing (404, network error, server without RFC 9728). A document with a missing `resource` field now throws earlier in `assertResourceBoundToServer`, so the old "missing 'resource' property" phrasing described a case that can no longer reach this branch. - Add a test for an unparseable `resource` string triggering the error-wrapping path in `assertResourceBoundToServer` (verifies the wrapper surfaces a descriptive message instead of leaking a raw `TypeError: Invalid URL` from the SDK's `new URL()` call).	2026-04-21 05:44:09 -07:00
Danny Avila	c4bb41137d	🔑 fix: Clear Stale Client Registration on invalid_client During OAuth Token Refresh (#12643) ... * fix: clear stale client registration on invalid_client during token refresh When a token refresh fails with `invalid_client`, the stored DCR client registration is no longer valid on the authorization server. The existing error handler only checked for `unauthorized_client` and returned null, leaving the stale client_id cached in the database permanently. Every subsequent token refresh attempt would fail with the same error. Now when `invalid_client` is detected during refresh: 1. The stale client registration is deleted from the database 2. A `ReauthenticationRequiredError` is thrown to trigger a fresh OAuth flow with new dynamic client registration Also passes `deleteTokens` from MCPConnectionFactory to getTokens() so the cleanup has access to the token deletion method. * fix: address review findings for stale client cleanup on token refresh - Delete stale refresh token alongside client registration on invalid_client (Finding 1) - Add tests for all new code paths: cleanup, warning, case-insensitivity, cleanup failure (Finding 2) - Detect all vendor-specific client rejection patterns (client_id mismatch, client not found, unknown client) with case-insensitive matching (Finding 3) - Use else-if for mutually exclusive error branches (Finding 4) - Log warning when deleteTokens is not available on client rejection (Finding 6) - Fix log message to say "attempting to clear" before async cleanup (Finding 7) - Extract isClientRejectionMessage to shared utility, refactor MCPConnectionFactory.isClientRejection to use it * fix: address followup review findings - Extract isInvalidClientMessage (4 stale-client patterns) from isClientRejectionMessage to eliminate pattern duplication between utils.ts and tokens.ts (Finding 1) - Remove redundant staleIdentifier variable, reuse identifier already in scope (Finding 2) - Separate await from .then() on Promise.allSettled for readability (Finding 3) - Add dedicated unit tests for isInvalidClientMessage and isClientRejectionMessage (Finding 4) - Assert error message content in primary invalid_client test (Finding 5) - Add JSDoc on deleteTokens in GetTokensParams (Finding 6) --------- Co-authored-by: Mani Japra <mani@muonspace.com>	2026-04-13 10:12:54 -04:00
Denis Palnitsky	8ed0bcf5ca	♻️ fix: Reuse Existing MCP OAuth Client Registrations to Prevent client_id Mismatch (#11925) ... * fix: reuse existing OAuth client registrations to prevent client_id mismatch When using auto-discovered OAuth (DCR), LibreChat calls /register on every flow initiation, getting a new client_id each time. When concurrent connections or reconnections happen, the client_id used during /authorize differs from the one used during /token, causing the server to reject the exchange. Before registering a new client, check if a valid client registration already exists in the database and reuse it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Handle re-registration of OAuth clients when redirect_uri changes * Add undefined fields for logo_uri and tos_uri in OAuth metadata tests * test: add client registration reuse tests for horizontal scaling race condition Reproduces the client_id mismatch bug that occurs in multi-replica deployments where concurrent initiateOAuthFlow calls each register a new OAuth client. Tests verify that the findToken-based client reuse prevents re-registration. * fix: address review findings for client registration reuse - Fix empty redirect_uris bug: invert condition so missing/empty redirect_uris triggers re-registration instead of silent reuse - Revert undocumented config?.redirect_uri in auto-discovery path - Change DB error logging from debug to warn for operator visibility - Fix import order: move package type import to correct section - Remove redundant type cast and misleading JSDoc comment - Test file: remove dead imports, restore process.env.DOMAIN_SERVER, rename describe blocks, add empty redirect_uris edge case test, add concurrent reconnection test with pre-seeded token, scope documentation to reconnection stabilization * fix: resolve type check errors for OAuthClientInformation redirect_uris The SDK's OAuthClientInformation type lacks redirect_uris (only on OAuthClientInformationFull). Cast to the local OAuthClientInformation type in handler.ts when accessing deserialized client info from DB, and use intersection types in tests for clientInfo with redirect_uris. * fix: address follow-up review findings R1, R2, R3 - R1: Move `import type { TokenMethods }` to the type-imports section, before local types, per CLAUDE.md import order rules - R2: Add unit test for empty redirect_uris in handler.test.ts to verify the inverted condition triggers re-registration - R3: Use delete for process.env.DOMAIN_SERVER restoration when the original value was undefined to avoid coercion to string "undefined" * fix: clear stale client registration on OAuth flow failure When a stored client_id is no longer recognized by the OAuth server, the flow fails but the stale client stays in MongoDB, causing every retry to reuse the same invalid registration in an infinite loop. On OAuth failure, clear the stored client registration so the next attempt falls through to fresh Dynamic Client Registration. - Add MCPTokenStorage.deleteClientRegistration() for targeted cleanup - Call it from MCPConnectionFactory's OAuth failure path - Add integration test proving recovery from stale client reuse * fix: validate auth server identity and target cleanup to reused clients - Gate client reuse on authorization server identity: compare stored issuer against freshly discovered metadata before reusing, preventing wrong-client reuse when the MCP server switches auth providers - Add reusedStoredClient flag to MCPOAuthFlowMetadata so cleanup only runs when the failed flow actually reused a stored registration, not on unrelated failures (timeouts, user-denied consent, etc.) - Add cleanup in returnOnOAuth path: when a prior flow that reused a stored client is detected as failed, clear the stale registration before re-initiating - Add tests for issuer mismatch and reusedStoredClient flag assertions * fix: address minor review findings N3, N5, N6 - N3: Type deleteClientRegistration param as TokenMethods['deleteTokens'] instead of Promise<unknown> - N5: Elevate deletion failure logging from debug to warn for operator visibility when stale client cleanup fails - N6: Use getLogPrefix() instead of hardcoded log prefix to respect system-user privacy convention * fix: correct stale-client cleanup in both OAuth paths - Blocking path: remove result?.clientInfo guard that made cleanup unreachable (handleOAuthRequired returns null on failure, so result?.clientInfo was always false in the failure branch) - returnOnOAuth path: only clear stored client when the prior flow status is FAILED, not on COMPLETED or PENDING flows, to avoid deleting valid registrations during normal flow replacement * fix: remove redundant cast on clientMetadata clientMetadata is already typed as Record<string, unknown>; the as Record<string, unknown> cast was a no-op. * fix: thread reusedStoredClient through return type instead of re-reading flow state FlowStateManager.createFlow() deletes FAILED flow state before rejecting, so getFlowState() after handleOAuthRequired() returns null would find nothing — making the stale-client cleanup dead code. Fix: hoist reusedStoredClient flag from flowMetadata into a local variable, include it in handleOAuthRequired()'s return type (both success and catch paths), and use result.reusedStoredClient directly in the caller instead of a second getFlowState() round-trip. * fix: selective stale-client cleanup in returnOnOAuth path The returnOnOAuth cleanup was unreliable: it depended on reading FAILED flow state, but FlowStateManager.monitorFlow() deletes FAILED state before rejecting. Move cleanup into createFlow's catch handler where flowMetadata.reusedStoredClient is still in scope. Make cleanup selective in both paths: add isClientRejection() helper that only matches errors indicating the OAuth server rejected the client_id (invalid_client, unauthorized_client, client not found). Timeouts, user-cancelled flows, and other transient failures no longer wipe valid stored registrations. Thread the error from handleOAuthRequired() through the return type so the blocking path can also check isClientRejection(). * fix: tighten isClientRejection heuristic Narrow 'client_id' match to 'client_id mismatch' to avoid false-positive cleanup on unrelated errors that happen to mention client_id. * test: add isClientRejection tests and enforced client_id on test server - Add isClientRejection unit tests: invalid_client, unauthorized_client, client_id mismatch, client not found, unknown client, and negative cases (timeout, flow state not found, user denied, null, undefined) - Enhance OAuth test server with enforceClientId option: binds auth codes to the client_id that initiated /authorize, rejects token exchange with mismatched or unregistered client_id (401 invalid_client) - Add integration tests proving the test server correctly rejects stale client_ids and accepts matching ones at /token * fix: issuer validation, callback error propagation, and cleanup DRY - Issuer check: re-register when storedIssuer is absent or non-string instead of silently reusing. Narrows unknown type with typeof guard and inverts condition so missing issuer → fresh DCR (safer default). - OAuth callback route: call failFlow with the OAuth error when the authorization server redirects back with error= parameter, so the waiting flow receives the actual rejection instead of timing out. This lets isClientRejection match stale-client errors correctly. - Extract duplicated cleanup block to clearStaleClientIfRejected() private method, called from both returnOnOAuth and blocking paths. - Test fixes: add issuer to stored metadata in reuse tests, reset server to undefined in afterEach to prevent double-close. * fix: gate failFlow behind callback validation, propagate reusedStoredClient on join - OAuth callback: move failFlow call to after CSRF/session/active-flow validation so an attacker with only a leaked state parameter cannot force-fail a flow without passing the same integrity checks required for legitimate callbacks - PENDING join path: propagate reusedStoredClient from flow metadata into the return object so joiners can trigger stale-client cleanup if the joined flow later fails with a client rejection * fix: restore early oauthError/code redirects, gate only failFlow behind CSRF The previous restructuring moved oauthError and missing-code checks behind CSRF validation, breaking tests that expect those redirects without cookies. The redirect itself is harmless (just shows an error page). Only the failFlow call needs CSRF gating to prevent DoS. Restructure: oauthError check stays early (redirects immediately), but failFlow inside it runs the full CSRF/session/active-flow validation before marking the flow as FAILED. * fix: require deleteTokens for client reuse, add missing import in MCP.js Client registration reuse without cleanup capability creates a permanent failure loop: if the reused client is stale, the code detects the rejection but cannot clear the stored registration because deleteTokens is missing, so every retry reuses the same broken client_id. - MCPConnectionFactory: only pass findToken to initiateOAuthFlow when deleteTokens is also available, ensuring reuse is only enabled when recovery is possible - api/server/services/MCP.js: add deleteTokens to the tokenMethods object (was the only MCP call site missing it) * fix: set reusedStoredClient before createFlow in joined-flow path When joining a PENDING flow, reusedStoredClient was only set on the success return but not before the await. If createFlow throws (e.g. invalid_client during token exchange), the outer catch returns the local variable which was still false, skipping stale-client cleanup. * fix: require browser binding (CSRF/session) for failFlow on OAuth error hasActiveFlow only proves a PENDING flow exists, not that the caller is the same browser that initiated it. An attacker with a leaked state could force-fail the flow without any user binding. Require hasCsrf or hasSession before calling failFlow on the oauthError path. * fix: guard findToken with deleteTokens check in blocking OAuth path Match the returnOnOAuth path's defense-in-depth: only enable client registration reuse when deleteTokens is also available, ensuring cleanup is possible if the reused client turns out to be stale. * fix: address review findings — tests, types, normalization, docs - Add deleteTokens method to InMemoryTokenStore matching TokenMethods contract; update test call site from deleteToken to deleteTokens - Add MCPConnectionFactory test: returnOnOAuth flow fails with invalid_client → clearStaleClientIfRejected invoked automatically - Add mcp.spec.js tests: OAuth error with CSRF → failFlow called; OAuth error without cookies → failFlow NOT called (DoS prevention) - Add JSDoc to isClientRejection with RFC 6749 and vendor attribution - Add inline comment explaining findToken/deleteTokens coupling guard - Normalize issuer comparison: strip trailing slashes to prevent spurious re-registrations from URL formatting differences - Fix dead-code: use local reusedStoredClient variable in PENDING join return instead of re-reading flowMeta * fix: address final review nits N1-N4 - N1: Add session cookie failFlow test — validates the hasSession branch triggers failFlow on OAuth error callback - N2: Replace setTimeout(50) with setImmediate for microtask drain - N3: Add 'unknown client' attribution to isClientRejection JSDoc - N4: Remove dead getFlowState mock from failFlow tests --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-03 22:15:08 -04:00
Danny Avila	fda72ac621	🏗️ refactor: Remove Redundant Caching, Migrate Config Services to TypeScript (#12466) ... * ♻️ refactor: Remove redundant scopedCacheKey caching, support user-provided key model fetching Remove redundant cache layers that used `scopedCacheKey()` (tenant-only scoping) on top of `getAppConfig()` which already caches per-principal (role+user+tenant). This caused config overrides for different principals within the same tenant to be invisible due to stale cached data. Changes: - Add `requireJwtAuth` to `/api/endpoints` route for proper user context - Remove ENDPOINT_CONFIG, STARTUP_CONFIG, PLUGINS, TOOLS, and MODELS_CONFIG cache layers — all derive from `getAppConfig()` with cheap computation - Enhance MODEL_QUERIES cache: hash(baseURL+apiKey) keys, 2-minute TTL, caching centralized in `fetchModels()` base function - Support fetching models with user-provided API keys in `loadConfigModels` via `getUserKeyValues` lookup (no caching for user keys) - Update all affected tests Closes #1028 * ♻️ refactor: Migrate config services to TypeScript in packages/api Move core config logic from CJS /api wrappers to typed TypeScript in packages/api using dependency injection factories: - `createEndpointsConfigService` — endpoint config merging + checkCapability - `createLoadConfigModels` — custom endpoint model loading with user key support - `createMCPToolCacheService` — MCP tool cache operations (update, merge, cache) /api files become thin wrappers that wire dependencies (getAppConfig, loadDefaultEndpointsConfig, getUserKeyValues, getCachedTools, etc.) into the typed factories. Also moves existing `endpoints/config.ts` → `endpoints/config/providers.ts` to accommodate the new `config/` directory structure. * 🔄 fix: Invalidate models query when user API key is set or revoked Without this, users had to refresh the page after entering their API key to see the updated model list fetched with their credentials. - Invalidate QueryKeys.models in useUpdateUserKeysMutation onSuccess - Invalidate QueryKeys.models in useRevokeUserKeyMutation onSuccess - Invalidate QueryKeys.models in useRevokeAllUserKeysMutation onSuccess * 🗺️ fix: Remap YAML-level override keys to AppConfig equivalents in mergeConfigOverrides Config overrides stored in the DB use YAML-level keys (TCustomConfig), but they're merged into the already-processed AppConfig where some fields have been renamed by AppService. This caused mcpServers overrides to land on a nonexistent key instead of mcpConfig, so config-override MCP servers never appeared in the UI. - Add OVERRIDE_KEY_MAP to remap mcpServers→mcpConfig, interface→interfaceConfig - Apply remapping before deep merge in mergeConfigOverrides - Add test for YAML-level key remapping behavior - Update existing tests to use AppConfig field names in assertions * 🧪 test: Update service.spec to use AppConfig field names after override key remapping * 🛡️ fix: Address code review findings — reliability, types, tests, and performance - Pass tenant context (getTenantId) in importers.js getEndpointsConfig call - Add 5 tests for user-provided API key model fetching (key found, no key, DB error, missing userId, apiKey-only with fixed baseURL) - Distinguish NO_USER_KEY (debug) from infrastructure errors (warn) in catch - Switch fetchPromisesMap from Promise.all to Promise.allSettled so one failing provider doesn't kill the entire model config - Parallelize getUserKeyValues DB lookups via batched Promise.allSettled instead of sequential awaits in the loop - Hoist standardCache instance in fetchModels to avoid double instantiation - Replace Record<string, unknown> types with Partial<TConfig>-based types; remove as unknown as T double-cast in endpoints config - Narrow Bedrock availableRegions to typed destructure - Narrow version field from string|number|undefined to string|undefined - Fix import ordering in mcp/tools.ts and config/models.ts per AGENTS.md - Add JSDoc to getModelsConfig alias clarifying caching semantics * fix: Guard against null getCachedTools in mergeAppTools * 🔍 fix: Address follow-up review — deduplicate extractEnvVariable, fix error discrimination, add log-level tests - Deduplicate extractEnvVariable calls: resolve apiKey/baseURL once, reuse for both the entry and isUserProvided checks (Finding A) - Move ResolvedEndpoint interface from function closure to module scope (Finding B) - Replace fragile msg.includes('NO_USER_KEY') with ErrorTypes.NO_USER_KEY enum check against actual error message format (Finding C). Also handle ErrorTypes.INVALID_USER_KEY as an expected "no key" case. - Add test asserting logger.warn is called for infra errors (not debug) - Add test asserting logger.debug is called for NO_USER_KEY errors (not warn) * fix: Preserve numeric assistants version via String() coercion * 🐛 fix: Address secondary review — Ollama cache bypass, cache tests, type safety - Fix Ollama success path bypassing cache write in fetchModels (CRITICAL): store result before returning so Ollama models benefit from 2-minute TTL - Add 4 fetchModels cache behavior tests: cache write with TTL, cache hit short-circuits HTTP, skipCache bypasses read+write, empty results not cached - Type-safe OVERRIDE_KEY_MAP: Partial<Record<keyof TCustomConfig, keyof AppConfig>> so compiler catches future field rename mismatches - Fix import ordering in config/models.ts (package types longest→shortest) - Rename ToolCacheDeps → MCPToolCacheDeps for naming consistency - Expand getModelsConfig JSDoc to explain caching granularity * fix: Narrow OVERRIDE_KEY_MAP index to satisfy strict tsconfig * 🧩 fix: Add allowedProviders to TConfig, remove Record<string, unknown> from PartialEndpointEntry The agents endpoint config includes allowedProviders (used by the frontend AgentPanel to filter available providers), but it was missing from TConfig. This forced PartialEndpointEntry to use & Record<string, unknown> as an escape hatch, violating AGENTS.md type policy. - Add allowedProviders?: (string | EModelEndpoint)[] to TConfig - Remove Record<string, unknown> from PartialEndpointEntry — now just Partial<TConfig> * 🛡️ fix: Isolate Ollama cache write from fetch try-catch, add Ollama cache tests - Separate Ollama fetch and cache write into distinct scopes so a cache failure (e.g., Redis down) doesn't misattribute the error as an Ollama API failure and fall through to the OpenAI-compatible path (Issue A) - Add 2 Ollama-specific cache tests: models written with TTL on fetch, cached models returned without hitting server (Issue B) - Replace hardcoded 120000 with Time.TWO_MINUTES constant in cache TTL test assertion (Issue C) - Fix OVERRIDE_KEY_MAP JSDoc to accurately describe runtime vs compile-time type enforcement (Issue D) - Add global beforeEach for cache mock reset to prevent cross-test leakage * 🧪 fix: Address third review — DI consistency, cache key width, MCP tests - Inject loadCustomEndpointsConfig via EndpointsConfigDeps with default fallback, matching loadDefaultEndpointsConfig DI pattern (Finding 3) - Widen modelsCacheKey from 64-bit (.slice(0,16)) to 128-bit (.slice(0,32)) for collision-sensitive cross-credential cache key (Finding 4) - Add fetchModels.mockReset() in loadConfigModels.spec beforeEach to prevent mock implementation leaks across tests (Finding 5) - Add 11 unit tests for createMCPToolCacheService covering all three functions: null/empty input, successful ops, error propagation, cold-cache merge (Finding 2) - Simplify getModelsConfig JSDoc to @see reference (Finding 10) * ♻️ refactor: Address remaining follow-ups from reviews OVERRIDE_KEY_MAP completeness: - Add missing turnstile→turnstileConfig mapping - Add exhaustiveness test verifying all three renamed keys are remapped and original YAML keys don't leak through Import role context: - Pass userRole through importConversations job → importLibreChatConvo so role-based endpoint overrides are honored during conversation import - Update convos.js route to include req.user.role in the job payload createEndpointsConfigService unit tests: - Add 8 tests covering: default+custom merge, Azure/AzureAssistants/ Anthropic Vertex/Bedrock config enrichment, assistants version coercion, agents allowedProviders, req.config bypass Plugins/tools efficiency: - Use Set for includedTools/filteredTools lookups (O(1) vs O(n) per plugin) - Combine auth check + filter into single pass (eliminates intermediate array) - Pre-compute toolDefKeys Set for O(1) tool definition lookups * fix: Scope model query cache by user when userIdQuery is enabled * fix: Skip model cache for userIdQuery endpoints, fix endpoints test types - When userIdQuery is true, skip caching entirely (like user_provided keys) to avoid cross-user model list leakage without duplicating cache data - Fix AgentCapabilities type error in endpoints.spec.ts — use enum values and appConfig() helper for partial mock typing * 🐛 fix: Restore filteredTools+includedTools composition, add checkCapability tests - Fix filteredTools regression: whitelist and blacklist are now applied independently (two flat guards), matching original behavior where includedTools=['a','b'] + filteredTools=['b'] produces ['a'] (Finding A) - Fix Set spread in toolkit loop: pre-compute toolDefKeysList array once alongside the Set, reuse for .some() without per-plugin allocation (Finding B) - Add 2 filteredTools tests: blacklist-only path and combined whitelist+blacklist composition (Finding C) - Add 3 checkCapability tests: capability present, capability absent, fallback to defaultAgentCapabilities for non-agents endpoints (Finding D) * 🔑 fix: Include config-override MCP servers in filterAuthorizedTools Config-override MCP servers (defined via admin config overrides for roles/groups) were rejected by filterAuthorizedTools because it called getAllServerConfigs(userId) without the configServers parameter. Only YAML and DB-backed user servers were included in the access check. - Add configServers parameter to filterAuthorizedTools - Resolve config servers via resolveConfigServers(req) at all 4 callsites (create, update, duplicate, revert) using parallel Promise.all - Pass configServers through to getAllServerConfigs(userId, configServers) so the registry merges config-source servers into the access check - Update filterAuthorizedTools.spec.js mock for resolveConfigServers * fix: Skip model cache for userIdQuery endpoints, fix endpoints test types For user-provided key endpoints (userProvide: true), skip the full model list re-fetch during message validation — the user already selected from a list we served them, and re-fetching with skipCache:true on every message send is both slow and fragile (5s provider timeout = rejected model). Instead, validate the model string format only: - Must be a string, max 256 chars - Must match [a-zA-Z0-9][a-zA-Z0-9_.:\-/@+ ]* (covers all known provider model ID formats while rejecting injection attempts) System-configured endpoints still get full model list validation as before. * 🧪 test: Add regression tests for filterAuthorizedTools configServers and validateModel filterAuthorizedTools: - Add test verifying configServers is passed to getAllServerConfigs and config-override server tools are allowed through - Guard resolveConfigServers in createAgentHandler to only run when MCP tools are present (skip for tool-free agent creates) validateModel (12 new tests): - Format validation: missing model, non-string, length overflow, leading special char, script injection, standard model ID acceptance - userProvide early-return: next() called immediately, getModelsConfig not invoked (regression guard for the exact bug this fixes) - System endpoint list validation: reject unknown model, accept known model, handle null/missing models config Also fix unnecessary backslash escape in MODEL_PATTERN regex. * 🧹 fix: Remove space from MODEL_PATTERN, trim input, clean up nits - Remove space character from MODEL_PATTERN regex — no real model ID uses spaces; prevents spurious violation logs from whitespace artifacts - Add model.trim() before validation to handle accidental whitespace - Remove redundant filterUniquePlugins call on already-deduplicated output - Add comment documenting intentional whitelist+blacklist composition - Add getUserKeyValues.mockReset() in loadConfigModels.spec beforeEach - Remove narrating JSDoc from getModelsConfig one-liner - Add 2 tests: trim whitespace handling, reject spaces in model ID * fix: Match startup tool loader semantics — includedTools takes precedence over filteredTools The startup tool loader (loadAndFormatTools) explicitly ignores filteredTools when includedTools is set, with a warning log. The PluginController was applying both independently, creating inconsistent behavior where the same config produced different results at startup vs plugin listing time. Restored mutually exclusive semantics: when includedTools is non-empty, filteredTools is not evaluated. * 🧹 chore: Simplify validateModel flow, note auth requirement on endpoints route - Separate missing-model from invalid-model checks cleanly: type+presence guard first, then trim+format guard (reviewer NIT) - Add route comment noting auth is required for role/tenant scoping * fix: Write trimmed model back to req.body.model for downstream consumers	2026-03-30 16:49:48 -04:00
Danny Avila	0d94881c2d	🧹 refactor: Tighten Config Schema Typing and Remove Deprecated Fields (#12452) ... * refactor: Remove deprecated and unused fields from endpoint schemas - Remove summarize, summaryModel from endpointSchema and azureEndpointSchema - Remove plugins from azureEndpointSchema - Remove customOrder from endpointSchema and azureEndpointSchema - Remove baseURL from all and agents endpoint schemas - Type paramDefinitions with full SettingDefinition-based schema - Clean up summarize/summaryModel references in initialize.ts and config.spec.ts * refactor: Improve MCP transport schema typing - Add defaults to transport type discriminators (stdio, websocket, sse) - Type stderr field as IOType union instead of z.any() * refactor: Add narrowed preset schema for model specs - Create tModelSpecPresetSchema omitting system/DB/deprecated fields - Update tModelSpecSchema to use the narrowed preset schema * test: Add explicit type field to MCP test fixtures Add transport type discriminator to test objects that construct MCPOptions/ParsedServerConfig directly, required after type field changed from optional to default in schema definitions. * chore: Bump librechat-data-provider to 0.8.404 * refactor: Tighten z.record(z.any()) fields to precise value types - Type headers fields as z.record(z.string()) in endpoint, assistant, and azure schemas - Type addParams as z.record(z.union([z.string(), z.number(), z.boolean(), z.null()])) - Type azure additionalHeaders as z.record(z.string()) - Type memory model_parameters as z.record(z.union([z.string(), z.number(), z.boolean()])) - Type firecrawl changeTrackingOptions.schema as z.record(z.string()) * refactor: Type supportedMimeTypes schema as z.array(z.string()) Replace z.array(z.any()).refine() with z.array(z.string()) since config input is always strings that get converted to RegExp via convertStringsToRegex() after parsing. Destructure supportedMimeTypes from spreads to avoid string[]/RegExp[] type mismatch. * refactor: Tighten enum, role, and numeric constraint schemas - Type engineSTT as enum ['openai', 'azureOpenAI'] - Type engineTTS as enum ['openai', 'azureOpenAI', 'elevenlabs', 'localai'] - Constrain playbackRate to 0.25–4 range - Type titleMessageRole as enum ['system', 'user', 'assistant'] - Add int().nonnegative() to MCP timeout and firecrawl timeout * chore: Bump librechat-data-provider to 0.8.405 * fix: Accept both string and RegExp in supportedMimeTypes schema The schema must accept both string[] (config input) and RegExp[] (post-merge runtime) since tests validate merged output against the schema. Use z.union([z.string(), z.instanceof(RegExp)]) to handle both. * refactor: Address review findings for schema tightening PR - Revert changeTrackingOptions.schema to z.record(z.unknown()) (JSON Schema is nested, not flat strings) - Remove dead contextStrategy code from BaseClient.js and cleanup.js - Extract paramDefinitionSchema to named exported constant - Add .int() constraint to columnSpan and columns - Apply consistent .int().nonnegative() to initTimeout, sseReadTimeout, scraperTimeout - Update stale stderr JSDoc to match actual accepted types - Add comprehensive tests for paramDefinitionSchema, tModelSpecPresetSchema, endpointSchema deprecated field stripping, and azureEndpointSchema * fix: Address second review pass findings - Revert supportedMimeTypesSchema to z.array(z.string()) and remove as string[] casts — fix tests to not validate merged RegExp[] output against the config input schema - Remove unused tModelSpecSchema import from test file - Consolidate duplicate '../src/schemas' imports - Add expiredAt coverage to tModelSpecPresetSchema test - Assert plugins is absent in azureEndpointSchema test - Add sync comments for engineSTT/engineTTS enum literals * refactor: Omit preset-management fields from tModelSpecPresetSchema Omit conversationId, presetId, title, defaultPreset, and order from the model spec preset schema — these are preset-management fields that don't belong in model spec configuration.	2026-03-29 01:10:57 -04:00
Danny Avila	fda1bfc3cc	🔬 ci: Add TypeScript Type Checks to Backend Workflow and Fix All Type Errors (#12451) ... * fix(data-schemas): resolve TypeScript strict type check errors in source files - Constrain ConfigSection to string keys via `string & keyof TCustomConfig` - Replace broken `z` import from data-provider with TCustomConfig derivation - Add `_id: Types.ObjectId` to IUser matching other Document interfaces - Add `federatedTokens` and `openidTokens` optional fields to IUser - Type mongoose model accessors as `Model<IRole>` and `Model<IUser>` - Widen `getPremiumRate` param to accept `number | null` - Widen `bulkWriteAclEntries` ops to untyped `AnyBulkWriteOperation[]` - Fix `getUserPrincipals` return type to use `PrincipalType` enum - Add non-null assertions for `connection.db` in migration files - Import DailyRotateFile constructor directly instead of relying on broken module augmentation across mismatched node_modules trees - Add winston-daily-rotate-file as devDependency for type resolution * fix(data-schemas): resolve TypeScript type errors in test files - Replace arbitrary test keys with valid TCustomConfig properties in config.spec - Use non-null assertions for permission objects in role.methods.spec - Replace `.SHARED_GLOBAL` access with `.not.toHaveProperty()` for legacy field - Add non-null assertions for balance, writeRate, readRate in spendTokens.spec - Update mock user _id to use ObjectId in user.test - Remove unused Schema import in tenantIndexes.spec * fix(api): resolve TypeScript strict type check errors across source and test files - Widen getUserPrincipals dep type in capabilities middleware - Fix federatedTokens type in createSafeUser return - Use proper mock req type for read-only properties in preAuthTenant.spec - Replace `as IUser` casts with ObjectId-typed mocks in openid/oidc specs - Use TokenExchangeMethodEnum values instead of string literals in MCP specs - Fix SessionStore type compatibility in sessionCache specs - Replace `catch (error: any)` with `(error as Error)` in redis specs - Remove invalid properties from test data in initialize and MCP specs - Add String.prototype.isWellFormed declaration for sanitizeTitle spec * fix(client): resolve TypeScript type errors in shared client components - Add default values for destructured bindings in OGDialogTemplate - Replace broken ExtendedFile import with inline type in FileIcon * ci: add TypeScript type-check job to backend review workflow Add a `typecheck` job that runs `tsc --noEmit` on all four TypeScript workspaces (data-provider, data-schemas, @librechat/api, @librechat/client) after the build step. Catches type errors that rollup builds may miss. * fix(data-schemas): add local type declaration for DailyRotateFile transport The `winston-daily-rotate-file` package ships a module augmentation for `winston/lib/winston/transports`, but it fails when winston and winston-daily-rotate-file resolve from different node_modules trees (which happens in this monorepo due to npm hoisting). Add a local `.d.ts` declaration that augments the same module path from within data-schemas' compilation unit, so `tsc --noEmit` passes while keeping the original runtime pattern (`new winston.transports.DailyRotateFile`). * fix: address code review findings from PR #12451 - Restore typed `AnyBulkWriteOperation<AclEntry>[]` on bulkWriteAclEntries, cast to untyped only at the tenantSafeBulkWrite call site (Finding 1) - Type `findUser` model accessor consistently with `findUsers` (Finding 2) - Replace inline `import('mongoose').ClientSession` with top-level import type - Use `toHaveLength` for spy assertions in playwright-expect spec file - Replace numbered Record casts with `.not.toHaveProperty()` in role.methods.spec for SHARED_GLOBAL assertions - Use per-test ObjectIds instead of shared testUserId in openid.spec - Replace inline `import()` type annotations with top-level SessionData import in sessionCache spec - Remove extraneous blank line in user.ts searchUsers * refactor: address remaining review findings (4–7) - Extract OIDCTokens interface in user.ts; deduplicate across IUser fields and oidc.ts FederatedTokens (Finding 4) - Move String.isWellFormed declaration from spec file to project-level src/types/es2024-string.d.ts (Finding 5) - Replace verbose `= undefined` defaults in OGDialogTemplate with null coalescing pattern (Finding 6) - Replace `Record<string, unknown>` TestConfig with named interface containing explicit test fields (Finding 7)	2026-03-28 21:06:39 -04:00
Danny Avila	877c2efc85	🏗️ feat: bulkWrite isolation, pre-auth context, strict-mode fixes (#12445) ... * fix: wrap seedDatabase() in runAsSystem() for strict tenant mode seedDatabase() was called without tenant context at startup, causing every Mongoose operation inside it to throw when TENANT_ISOLATION_STRICT=true. Wrapping in runAsSystem() gives it the SYSTEM_TENANT_ID sentinel so the isolation plugin skips filtering, matching the pattern already used for performStartupChecks and updateInterfacePermissions. * fix: chain tenantContextMiddleware in optionalJwtAuth optionalJwtAuth populated req.user but never established ALS tenant context, unlike requireJwtAuth which chains tenantContextMiddleware after successful auth. Authenticated users hitting routes with optionalJwtAuth (e.g. /api/banner) had no tenant isolation. * feat: tenant-safe bulkWrite wrapper and call-site migration Mongoose's bulkWrite() does not trigger schema-level middleware hooks, so the applyTenantIsolation plugin cannot intercept it. This adds a tenantSafeBulkWrite() utility that injects the current ALS tenant context into every operation's filter/document before delegating to native bulkWrite. Migrates all 8 runtime bulkWrite call sites: - agentCategory (seedCategories, ensureDefaultCategories) - conversation (bulkSaveConvos) - message (bulkSaveMessages) - file (batchUpdateFiles) - conversationTag (updateTagsForConversation, bulkIncrementTagCounts) - aclEntry (bulkWriteAclEntries) systemGrant.seedSystemGrants is intentionally not migrated — it uses explicit tenantId: { $exists: false } filters and is exempt from the isolation plugin. * feat: pre-auth tenant middleware and tenant-scoped config cache Adds preAuthTenantMiddleware that reads X-Tenant-Id from the request header and wraps downstream in tenantStorage ALS context. Wired onto /oauth, /api/auth, /api/config, and /api/share — unauthenticated routes that need tenant scoping before JWT auth runs. The /api/config cache key is now tenant-scoped (STARTUP_CONFIG:${tenantId}) so multi-tenant deployments serve the correct login page config per tenant. The middleware is intentionally minimal — no subdomain parsing, no OIDC claim extraction. The private fork's reverse proxy or auth gateway sets the header. * feat: accept optional tenantId in updateInterfacePermissions When tenantId is provided, the function re-enters inside tenantStorage.run({ tenantId }) so all downstream Mongoose queries target that tenant's roles instead of the system context. This lets the private fork's tenant provisioning flow call updateInterfacePermissions per-tenant after creating tenant-scoped ADMIN/USER roles. * fix: tenant-filter $lookup in getPromptGroup aggregation The $lookup stage in getPromptGroup() queried the prompts collection without tenant filtering. While the outer PromptGroup aggregate is protected by the tenantIsolation plugin's pre('aggregate') hook, $lookup runs as an internal MongoDB operation that bypasses Mongoose hooks entirely. Converts from simple field-based $lookup to pipeline-based $lookup with an explicit tenantId match when tenant context is active. * fix: replace field-level unique indexes with tenant-scoped compounds Field-level unique:true creates a globally-unique single-field index in MongoDB, which would cause insert failures across tenants sharing the same ID values. - agent.id: removed field-level unique, added { id, tenantId } compound - convo.conversationId: removed field-level unique (compound at line 50 already exists: { conversationId, user, tenantId }) - message.messageId: removed field-level unique (compound at line 165 already exists: { messageId, user, tenantId }) - preset.presetId: removed field-level unique, added { presetId, tenantId } compound * fix: scope MODELS_CONFIG, ENDPOINT_CONFIG, PLUGINS, TOOLS caches by tenant These caches store per-tenant configuration (available models, endpoint settings, plugin availability, tool definitions) but were using global cache keys. In multi-tenant mode, one tenant's cached config would be served to all tenants. Appends :${tenantId} to cache keys when tenant context is active. Falls back to the unscoped key when no tenant context exists (backward compatible for single-tenant OSS deployments). Covers all read, write, and delete sites: - ModelController.js: get/set MODELS_CONFIG - PluginController.js: get/set PLUGINS, get/set TOOLS - getEndpointsConfig.js: get/set/delete ENDPOINT_CONFIG - app.js: delete ENDPOINT_CONFIG (clearEndpointConfigCache) - mcp.js: delete TOOLS (updateMCPTools, mergeAppTools) - importers.js: get ENDPOINT_CONFIG * fix: add getTenantId to PluginController spec mock The data-schemas mock was missing getTenantId, causing all PluginController tests to throw when the controller calls getTenantId() for tenant-scoped cache keys. * fix: address review findings — migration, strict-mode, DRY, types Addresses all CRITICAL, MAJOR, and MINOR review findings: F1 (CRITICAL): Add agents, conversations, messages, presets to SUPERSEDED_INDEXES in tenantIndexes.ts so dropSupersededTenantIndexes() drops the old single-field unique indexes that block multi-tenant inserts. F2 (CRITICAL): Unknown bulkWrite op types now throw in strict mode instead of silently passing through without tenant injection. F3 (MAJOR): Replace wildcard export with named export for tenantSafeBulkWrite, hiding _resetBulkWriteStrictCache from the public package API. F5 (MAJOR): Restore AnyBulkWriteOperation<IAclEntry>[] typing on bulkWriteAclEntries — the unparameterized wrapper accepts parameterized ops as a subtype. F7 (MAJOR): Fix config.js tenant precedence — JWT-derived req.user.tenantId now takes priority over the X-Tenant-Id header for authenticated requests. F8 (MINOR): Extract scopedCacheKey() helper into tenantContext.ts and replace all 11 inline occurrences across 7 files. F9 (MINOR): Use simple localField/foreignField $lookup for the non-tenant getPromptGroup path (more efficient index seeks). F12 (NIT): Remove redundant BulkOp type alias. F13 (NIT): Remove debug log that leaked raw tenantId. * fix: add new superseded indexes to tenantIndexes test fixture The test creates old indexes to verify the migration drops them. Missing fixture entries for agents.id_1, conversations.conversationId_1, messages.messageId_1, and presets.presetId_1 caused the count assertion to fail (expected 22, got 18). * fix: restore logger.warn for unknown bulk op types in non-strict mode * fix: block SYSTEM_TENANT_ID sentinel from external header input CRITICAL: preAuthTenantMiddleware accepted any string as X-Tenant-Id, including '__SYSTEM__'. The tenantIsolation plugin treats SYSTEM_TENANT_ID as an explicit bypass — skipping ALL query filters. A client sending X-Tenant-Id: __SYSTEM__ to pre-auth routes (/api/share, /api/config, /api/auth, /oauth) would execute Mongoose operations without tenant isolation. Fixes: - preAuthTenantMiddleware rejects SYSTEM_TENANT_ID in header - scopedCacheKey returns the base key (not key:__SYSTEM__) in system context, preventing stale cache entries during runAsSystem() - updateInterfacePermissions guards tenantId against SYSTEM_TENANT_ID - $lookup pipeline separates $expr join from constant tenantId match for better index utilization - Regression test for sentinel rejection in preAuthTenant.spec.ts - Remove redundant getTenantId() call in config.js * test: add missing deleteMany/replaceOne coverage, fix vacuous ALS assertions bulkWrite spec: - deleteMany: verifies tenant-scoped deletion leaves other tenants untouched - replaceOne: verifies tenantId injected into both filter and replacement - replaceOne overwrite: verifies a conflicting tenantId in the replacement document is overwritten by the ALS tenant (defense-in-depth) - empty ops array: verifies graceful handling preAuthTenant spec: - All negative-case tests now use the capturedNext pattern to verify getTenantId() inside the middleware's execution context, not the test runner's outer frame (which was always undefined regardless) * feat: tenant-isolate MESSAGES cache, FLOWS cache, and GenerationJobManager MESSAGES cache (streamAudio.js): - Cache key now uses scopedCacheKey(messageId) to prefix with tenantId, preventing cross-tenant message content reads during TTS streaming. FLOWS cache (FlowStateManager): - getFlowKey() now generates ${type}:${tenantId}:${flowId} when tenant context is active, isolating OAuth flow state per tenant. GenerationJobManager: - tenantId added to SerializableJobData and GenerationJobMetadata - createJob() captures the current ALS tenant context (excluding SYSTEM_TENANT_ID) and stores it in job metadata - SSE subscription endpoint validates job.metadata.tenantId matches req.user.tenantId, blocking cross-tenant stream access - Both InMemoryJobStore and RedisJobStore updated to accept tenantId * fix: add getTenantId and SYSTEM_TENANT_ID to MCP OAuth test mocks FlowStateManager.getFlowKey() now calls getTenantId() for tenant-scoped flow keys. The 4 MCP OAuth test files mock @librechat/data-schemas without these exports, causing TypeError at runtime. * fix: correct import ordering per AGENTS.md conventions Package imports sorted shortest to longest line length, local imports sorted longest to shortest — fixes ordering violations introduced by our new imports across 8 files. * fix: deserialize tenantId in RedisJobStore — cross-tenant SSE guard was no-op in Redis mode serializeJob() writes tenantId to the Redis hash via Object.entries, but deserializeJob() manually enumerates fields and omitted tenantId. Every getJob() from Redis returned tenantId: undefined, causing the SSE route's cross-tenant guard to short-circuit (undefined && ... → false). * test: SSE tenant guard, FlowStateManager key consistency, ALS scope docs SSE stream tenant tests (streamTenant.spec.js): - Cross-tenant user accessing another tenant's stream → 403 - Same-tenant user accessing own stream → allowed - OSS mode (no tenantId on job) → tenant check skipped FlowStateManager tenant tests (manager.tenant.spec.ts): - completeFlow finds flow created under same tenant context - completeFlow does NOT find flow under different tenant context - Unscoped flows are separate from tenant-scoped flows Documentation: - JSDoc on getFlowKey documenting ALS context consistency requirement - Comment on streamAudio.js scopedCacheKey capture site * fix: SSE stream tests hang on success path, remove internal fork references The success-path tests entered the SSE streaming code which never closes, causing timeout. Mock subscribe() to end the response immediately. Restructured assertions to verify non-403/non-404. Removed "private fork" and "OSS" references from code and test descriptions — replaced with "deployment layer", "multi-tenant deployments", and "single-tenant mode". * fix: address review findings — test rigor, tenant ID validation, docs F1: SSE stream tests now mock subscribe() with correct signature (streamId, writeEvent, onDone, onError) and assert 200 status, verifying the tenant guard actually allows through same-tenant users. F2: completeFlow logs the attempted key and ALS tenantId when flow is not found, so reverse proxy misconfiguration (missing X-Tenant-Id on OAuth callback) produces an actionable warning. F3/F10: preAuthTenantMiddleware validates tenant ID format — rejects colons, special characters, and values exceeding 128 chars. Trims whitespace. Prevents cache key collisions via crafted headers. F4: Documented cache invalidation scope limitation in clearEndpointConfigCache — only the calling tenant's key is cleared; other tenants expire via TTL. F7: getFlowKey JSDoc now lists all 8 methods requiring consistent ALS context. F8: Added dedicated scopedCacheKey unit tests — base key without context, base key in system context, scoped key with tenant, no ALS leakage across scope boundaries. * fix: revert flow key tenant scoping, fix SSE test timing FlowStateManager: Reverts tenant-scoped flow keys. OAuth callbacks arrive without tenant ALS context (provider redirects don't carry X-Tenant-Id), so completeFlow/failFlow would never find flows created under tenant context. Flow IDs are random UUIDs with no collision risk, and flow data is ephemeral (TTL-bounded). SSE tests: Use process.nextTick for onDone callback so Express response headers are flushed before res.write/res.end are called. * fix: restore getTenantId import for completeFlow diagnostic log * fix: correct completeFlow warning message, add missing flow test The warning referenced X-Tenant-Id header consistency which was only relevant when flow keys were tenant-scoped (since reverted). Updated to list actual causes: TTL expiry, missing flow, or routing to a different instance without shared Keyv storage. Removed the getTenantId() call and import — no longer needed since flow keys are unscoped. Added test for the !flowState branch in completeFlow — verifies return false and logger.warn on nonexistent flow ID. * fix: add explicit return type to recursive updateInterfacePermissions The recursive call (tenantId branch calls itself without tenantId) causes TypeScript to infer circular return type 'any'. Adding explicit Promise<void> satisfies the rollup typescript plugin. * fix: update MCPOAuthRaceCondition test to match new completeFlow warning * fix: clearEndpointConfigCache deletes both scoped and unscoped keys Unauthenticated /api/endpoints requests populate the unscoped ENDPOINT_CONFIG key. Admin config mutations clear only the tenant-scoped key, leaving the unscoped entry stale indefinitely. Now deletes both when in tenant context. * fix: tenant guard on abort/status endpoints, warn logs, test coverage F1: Add tenant guard to /chat/status/:conversationId and /chat/abort matching the existing guard on /chat/stream/:streamId. The status endpoint exposes aggregatedContent (AI response text) which requires tenant-level access control. F2: preAuthTenantMiddleware now logs warn for rejected __SYSTEM__ sentinel and malformed tenant IDs, providing observability for bypass probing attempts. F3: Abort fallback path (getActiveJobIdsForUser) now has tenant check after resolving the job. F4: Test for strict mode + SYSTEM_TENANT_ID — verifies runAsSystem bypasses tenantSafeBulkWrite without throwing in strict mode. F5: Test for job with tenantId + user without tenantId → 403. F10: Regex uses idiomatic hyphen-at-start form. F11: Test descriptions changed from "rejects" to "ignores" since middleware calls next() (not 4xx). Also fixes MCPOAuthRaceCondition test assertion to match updated completeFlow warning message. * fix: test coverage for logger.warn, status/abort guards, consistency A: preAuthTenant spec now mocks logger and asserts warn calls for __SYSTEM__ sentinel, malformed characters, and oversized headers. B: streamTenant spec expanded with status and abort endpoint tests — cross-tenant status returns 403, same-tenant returns 200 with body, cross-tenant abort returns 403. C: Abort endpoint uses req.user.tenantId (not req.user?.tenantId) matching stream/status pattern — requireJwtAuth guarantees req.user. D: Malformed header warning now includes ip in log metadata, matching the sentinel warning for consistent SOC correlation. * fix: assert ip field in malformed header warn tests * fix: parallelize cache deletes, document tenant guard, fix import order - clearEndpointConfigCache uses Promise.all for independent cache deletes instead of sequential awaits - SSE stream tenant guard has inline comment explaining backward-compat behavior for untenanted legacy jobs - conversation.ts local imports reordered longest-to-shortest per AGENTS.md * fix: tenant-qualify userJobs keys, document tenant guard backward-compat Job store userJobs keys now include tenantId when available: - Redis: stream:user:{tenantId:userId}:jobs (falls back to stream:user:{userId}:jobs when no tenant) - InMemory: composite key tenantId:userId in userJobMap getActiveJobIdsByUser/getActiveJobIdsForUser accept optional tenantId parameter, threaded through from req.user.tenantId at all call sites (/chat/active and /chat/abort fallback). Added inline comments on all three SSE tenant guards explaining the backward-compat design: untenanted legacy jobs remain accessible when the userId check passes. * fix: parallelize cache deletes, document tenant guard, fix import order Fix InMemoryJobStore.getActiveJobIdsByUser empty-set cleanup to use the tenant-qualified userKey instead of bare userId — prevents orphaned empty Sets accumulating in userJobMap for multi-tenant users. Document cross-tenant staleness in clearEndpointConfigCache JSDoc — other tenants' scoped keys expire via TTL, not active invalidation. * fix: cleanup userJobMap leak, startup warning, DRY tenant guard, docs F1: InMemoryJobStore.cleanup() now removes entries from userJobMap before calling deleteJob, preventing orphaned empty Sets from accumulating with tenant-qualified composite keys. F2: Startup warning when TENANT_ISOLATION_STRICT is active — reminds operators to configure reverse proxy to control X-Tenant-Id header. F3: mergeAppTools JSDoc documents that tenant-scoped TOOLS keys are not actively invalidated (matching clearEndpointConfigCache pattern). F5: Abort handler getActiveJobIdsForUser call uses req.user.tenantId (not req.user?.tenantId) — consistent with stream/status handlers. F6: updateInterfacePermissions JSDoc clarifies SYSTEM_TENANT_ID behavior — falls through to caller's ALS context. F7: Extracted hasTenantMismatch() helper, replacing three identical inline tenant guard blocks across stream/status/abort endpoints. F9: scopedCacheKey JSDoc documents both passthrough cases (no context and SYSTEM_TENANT_ID context). * fix: clean userJobMap in evictOldest — same leak as cleanup()	2026-03-28 16:43:50 -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
Danny Avila	5e3b7bcde3	🌊 refactor: Local Snapshot for Aggregate Key Cache to Avoid Redundant Redis GETs (#12422) ... * perf: Add local snapshot to aggregate key cache to avoid redundant Redis GETs getAll() was being called 20+ times per chat request (once per tool, per server config lookup, per connection check). Each call hit Redis even though the data doesn't change within a request cycle. Add an in-memory snapshot with 5s TTL that collapses all reads within the window into a single Redis GET. Writes (add/update/remove/reset) invalidate the snapshot immediately so mutations are never stale. Also removes the debug logger that was producing noisy per-call logs. * fix: Prevent snapshot mutation and guarantee cleanup on write failure - Never mutate the snapshot object in-place during writes. Build a new object (spread) so concurrent readers never observe uncommitted state. - Move invalidateLocalSnapshot() into withWriteLock's finally block so cleanup is guaranteed even when successCheck throws on Redis failure. - After successful writes, populate the snapshot with the committed state to avoid an unnecessary Redis GET on the next read. - Use Date.now() after the await in getAll() so the TTL window isn't shortened by Redis latency. - Strengthen tests: spy on underlying Keyv cache to verify N getAll() calls collapse into 1 Redis GET, verify snapshot reference immutability. * fix: Remove dead populateLocalSnapshot calls from write callbacks populateLocalSnapshot was called inside withWriteLock callbacks, but the finally block in withWriteLock always calls invalidateLocalSnapshot immediately after — undoing the populate on every execution path. Remove the dead method and its three call sites. The snapshot is correctly cleared by finally on both success and failure paths. The next getAll() after a write hits Redis once to fetch the committed state, which is acceptable since writes only occur during init and rare manual reinspection. * fix: Derive local snapshot TTL from MCP_REGISTRY_CACHE_TTL config Use cacheConfig.MCP_REGISTRY_CACHE_TTL (default 5000ms) instead of a hardcoded 5s constant. When TTL is 0 (operator explicitly wants no caching), the snapshot is disabled entirely — every getAll() hits Redis. * fix: Add TTL expiry test, document 2×TTL staleness, clarify comments - Add missing test for snapshot TTL expiry path (force-expire via localSnapshotExpiry mutation, verify Redis is hit again) - Document 2×TTL max cross-instance staleness in localSnapshot JSDoc - Document reset() intentionally bypasses withWriteLock - Add inline comments explaining why early invalidateLocalSnapshot() in write callbacks is distinct from the finally-block cleanup - Update cacheConfig.MCP_REGISTRY_CACHE_TTL JSDoc to reflect both use sites and the staleness implication - Rename misleading test name for snapshot reference immutability - Add epoch sentinel comment on localSnapshotExpiry initialization	2026-03-26 16:39:09 -04:00
Danny Avila	8e2721011e	🔑 fix: Robust MCP OAuth Detection in Tool-Call Flow (#12418) ... * fix(api): add buildOAuthToolCallName utility for MCP OAuth flows Extract a shared utility that builds the synthetic tool-call name used during MCP OAuth flows (oauth_mcp_{normalizedServerName}). Uses startsWith on the raw serverName (not the normalized form) to guard against double-wrapping, so names that merely normalize to start with oauth_mcp_ (e.g., oauth@mcp@server) are correctly prefixed while genuinely pre-wrapped names are left as-is. Add 8 unit tests covering normal names, pre-wrapped names, _mcp_ substrings, special characters, non-ASCII, and empty string inputs. * fix(backend): use buildOAuthToolCallName in MCP OAuth flows Replace inline tool-call name construction in both reconnectServer (MCP.js) and createOAuthEmitter (ToolService.js) with the shared buildOAuthToolCallName utility. Remove unused normalizeServerName import from ToolService.js. Fix import ordering in both files. This ensures the oauth_mcp_ prefix is consistently applied so the client correctly identifies MCP OAuth flows and binds the CSRF cookie to the right server. * fix(client): robust MCP OAuth detection and split handling in ToolCall - Fix split() destructuring to preserve tail segments for server names containing _mcp_ (e.g., foo_mcp_bar no longer truncated to foo). - Add auth URL redirect_uri fallback: when the tool-call name lacks the _mcp_ delimiter, parse redirect_uri for the MCP callback path. Set function_name to the extracted server name so progress text shows the server, not the raw tool-call ID. - Display server name instead of literal "oauth" as function_name, gated on auth presence to avoid misidentifying real tools named "oauth". - Consolidate three independent new URL(auth) parses into a single parsedAuthUrl useMemo shared across detection, actionId, and authDomain hooks. - Replace any type on ProgressText test mock with structural type. - Add 8 tests covering delimiter detection, multi-segment names, function_name display, redirect_uri fallback, normalized _mcp_ server names, and non-MCP action auth exclusion. * chore: fix import order in utils.test.ts * fix(client): drop auth gate on OAuth displayName so completed flows show server name The createOAuthEnd handler re-emits the toolCall delta without auth, so auth is cleared on the client after OAuth completes. Gating displayName on `func === 'oauth' && auth` caused completed OAuth steps to render "Completed oauth" instead of "Completed my-server". Remove the `&& auth` gate — within the MCP delimiter branch the func="oauth" check alone is sufficient. Also remove `auth` from the useMemo dep array since only `parsedAuthUrl` is referenced. Update the test to assert correct post-completion display.	2026-03-26 14:45:13 -04:00
Danny Avila	359cc63b41	⚡ refactor: Use in-memory cache for App MCP configs to avoid Redis SCAN (#12410) ... * ⚡ perf: Use in-memory cache for App MCP configs to avoid Redis SCAN The 'App' namespace holds static YAML-loaded configs identical on every instance. Storing them in Redis and retrieving via SCAN + batch-GET caused 60s+ stalls under concurrent load (#11624). Since these configs are already loaded into memory at startup, bypass Redis entirely by always returning ServerConfigsCacheInMemory for the 'App' namespace. * ♻️ refactor: Extract APP_CACHE_NAMESPACE constant and harden tests - Extract magic string 'App' to a shared `APP_CACHE_NAMESPACE` constant used by both ServerConfigsCacheFactory and MCPServersRegistry - Document that `leaderOnly` is ignored for the App namespace - Reset `cacheConfig.USE_REDIS` in test `beforeEach` to prevent ordering-dependent flakiness - Fix import order in test file (longest to shortest) * 🐛 fix: Populate App cache on follower instances in cluster mode In cluster deployments, only the leader runs MCPServersInitializer to inspect and cache MCP server configs. Followers previously read these from Redis, but with the App namespace now using in-memory storage, followers would have an empty cache. Add populateLocalCache() so follower processes independently initialize their own in-memory App cache from the same YAML configs after the leader signals completion. The method is idempotent — if the cache is already populated (leader case), it's a no-op. * 🐛 fix: Use static flag for populateLocalCache idempotency Replace getAllServerConfigs() idempotency check with a static localCachePopulated flag. The previous check merged App + DB caches, causing false early returns in deployments with publicly shared DB configs, and poisoned the TTL read-through cache with stale results. The static flag is zero-cost (no async/Redis/DB calls), immune to DB config interference, and is reset alongside hasInitializedThisProcess in resetProcessFlag() for test teardown. Also set localCachePopulated=true after leader initialization completes, so subsequent calls on the leader don't redundantly re-run populateLocalCache. * 📝 docs: Document process-local reset() semantics for App cache With the App namespace using in-memory storage, reset() only clears the calling process's cache. Add JSDoc noting this behavioral change so callers in cluster deployments know each instance must reset independently. * ✅ test: Add follower cache population tests for MCPServersInitializer Cover the populateLocalCache code path: - Follower populates its own App cache after leader signals completion - localCachePopulated flag prevents redundant re-initialization - Fresh follower process independently initializes all servers * 🧹 style: Fix import order to longest-to-shortest convention * 🔬 test: Add Redis perf benchmark to isolate getAll() bottleneck Benchmarks that run against a live Redis instance to measure: 1. SCAN vs batched GET phases independently 2. SCAN cost scaling with total keyspace size (noise keys) 3. Concurrent getAll() at various concurrency levels (1/10/50/100) 4. Alternative: single aggregate key vs SCAN+GET 5. Alternative: raw MGET vs Keyv batch GET (serialization overhead) Run with: npx jest --config packages/api/jest.config.mjs \ --testPathPatterns="perf_benchmark" --coverage=false * ⚡ feat: Add aggregate-key Redis cache for MCP App configs ServerConfigsCacheRedisAggregateKey stores all configs under a single Redis key, making getAll() a single GET instead of SCAN + N GETs. This eliminates the O(keyspace_size) SCAN that caused 60s+ stalls in large deployments while preserving cross-instance visibility — all instances read/write the same Redis key, so reinspection results propagate automatically after readThroughCache TTL expiry. * ♻️ refactor: Use aggregate-key cache for App namespace in factory Update ServerConfigsCacheFactory to return ServerConfigsCacheRedisAggregateKey for the App namespace when Redis is enabled, instead of ServerConfigsCacheInMemory. This preserves cross-instance visibility (reinspection results propagate through Redis) while eliminating SCAN. Non-App namespaces still use the standard per-key ServerConfigsCacheRedis. * 🗑️ revert: Remove populateLocalCache — no longer needed with aggregate key With App configs stored under a single Redis key (aggregate approach), followers read from Redis like before. The populateLocalCache mechanism and its localCachePopulated flag are no longer necessary. Also reverts the process-local reset() JSDoc since reset() is now cluster-wide again via Redis. * 🐛 fix: Add write mutex to aggregate cache and exclude perf benchmark from CI - Add promise-based write lock to ServerConfigsCacheRedisAggregateKey to prevent concurrent read-modify-write races during parallel initialization (Promise.allSettled runs multiple addServer calls concurrently, causing last-write-wins data loss on the aggregate key) - Rename perf benchmark to cache_integration pattern so CI skips it (requires live Redis) * 🔧 fix: Rename perf benchmark to *.manual.spec.ts to exclude from all CI The cache_integration pattern is picked up by test:cache-integration:mcp in CI. Rename to *.manual.spec.ts which isn't matched by any CI runner. * ✅ test: Add cache integration tests for ServerConfigsCacheRedisAggregateKey Tests against a live Redis instance covering: - CRUD operations (add, get, update, remove) - getAll with empty/populated cache - Duplicate add rejection, missing update/remove errors - Concurrent write safety (20 parallel adds without data loss) - Concurrent read safety (50 parallel getAll calls) - Reset clears all configs * 🔧 fix: Rename perf benchmark to *.manual.spec.ts to exclude from all CI The perf benchmark file was renamed to *.manual.spec.ts but no testPathIgnorePatterns existed for that convention. Add .*manual\.spec\. to both test and test:ci scripts, plus jest.config.mjs, so manual-only tests never run in CI unit test jobs. * fix: Address review findings for aggregate key cache - Add successCheck() to all write paths (add/update/remove) so Redis SET failures throw instead of being silently swallowed - Override reset() to use targeted cache.delete(AGGREGATE_KEY) instead of inherited SCAN-based cache.clear() — consistent with eliminating SCAN operations - Document cross-instance write race invariant in class JSDoc: the promise-based writeLock is process-local only; callers must enforce single-writer semantics externally (leader-only init) - Use definite-assignment assertion (let resolve!:) instead of non-null assertion at call site - Fix import type convention in integration test - Verify Promise.allSettled rejections explicitly in concurrent write test - Fix broken run command in benchmark file header * style: Fix import ordering per AGENTS.md convention Local/project imports sorted longest to shortest. * chore: Update import ordering and clean up unused imports in MCPServersRegistry.ts * chore: import order * chore: import order	2026-03-26 14:44:31 -04:00
Danny Avila	221e49222d	⚡ refactor: Fast-Fail MCP Tool Discovery on 401 for Non-OAuth Servers (#12395) ... * fix: fast-fail MCP discovery for non-OAuth servers on auth errors Always attach oauthHandler in discoverToolsInternal regardless of useOAuth flag. Previously, non-OAuth servers hitting 401 would hang for 30s because connectClient's oauthHandledPromise had no listener to emit oauthFailed, waiting until withTimeout killed it. * chore: import order	2026-03-25 13:18:02 -04:00
Danny Avila	7829fa9eca	🪄 refactor: Simplify MCP Tool Content Formatting to Unified String Output (#12352) ... * refactor: Simplify content formatting in MCP service and parser - Consolidated content handling in `formatToolContent` to return a plain-text string instead of an array for all providers, enhancing clarity and consistency. - Removed unnecessary checks for content array providers, streamlining the logic for handling text and image artifacts. - Updated related tests to reflect changes in expected output format, ensuring comprehensive coverage for the new implementation. * fix: Return empty string for image-only tool responses instead of '(No response)' When artifacts exist (images/UI resources) but no text content is present, return an empty string rather than the misleading '(No response)' fallback. Adds missing test assertions for image-only content and standardizes length checks to explicit `> 0` comparisons.	2026-03-21 14:28:56 -04:00
Danny Avila	8ba2bde5c1	📦 refactor: Consolidate DB models, encapsulating Mongoose usage in data-schemas (#11830) ... * chore: move database model methods to /packages/data-schemas * chore: add TypeScript ESLint rule to warn on unused variables * refactor: model imports to streamline access - Consolidated model imports across various files to improve code organization and reduce redundancy. - Updated imports for models such as Assistant, Message, Conversation, and others to a unified import path. - Adjusted middleware and service files to reflect the new import structure, ensuring functionality remains intact. - Enhanced test files to align with the new import paths, maintaining test coverage and integrity. * chore: migrate database models to packages/data-schemas and refactor all direct Mongoose Model usage outside of data-schemas * test: update agent model mocks in unit tests - Added `getAgent` mock to `client.test.js` to enhance test coverage for agent-related functionality. - Removed redundant `getAgent` and `getAgents` mocks from `openai.spec.js` and `responses.unit.spec.js` to streamline test setup and reduce duplication. - Ensured consistency in agent mock implementations across test files. * fix: update types in data-schemas * refactor: enhance type definitions in transaction and spending methods - Updated type definitions in `checkBalance.ts` to use specific request and response types. - Refined `spendTokens.ts` to utilize a new `SpendTxData` interface for better clarity and type safety. - Improved transaction handling in `transaction.ts` by introducing `TransactionResult` and `TxData` interfaces, ensuring consistent data structures across methods. - Adjusted unit tests in `transaction.spec.ts` to accommodate new type definitions and enhance robustness. * refactor: streamline model imports and enhance code organization - Consolidated model imports across various controllers and services to a unified import path, improving code clarity and reducing redundancy. - Updated multiple files to reflect the new import structure, ensuring all functionalities remain intact. - Enhanced overall code organization by removing duplicate import statements and optimizing the usage of model methods. * feat: implement loadAddedAgent and refactor agent loading logic - Introduced `loadAddedAgent` function to handle loading agents from added conversations, supporting multi-convo parallel execution. - Created a new `load.ts` file to encapsulate agent loading functionalities, including `loadEphemeralAgent` and `loadAgent`. - Updated the `index.ts` file to export the new `load` module instead of the deprecated `loadAgent`. - Enhanced type definitions and improved error handling in the agent loading process. - Adjusted unit tests to reflect changes in the agent loading structure and ensure comprehensive coverage. * refactor: enhance balance handling with new update interface - Introduced `IBalanceUpdate` interface to streamline balance update operations across the codebase. - Updated `upsertBalanceFields` method signatures in `balance.ts`, `transaction.ts`, and related tests to utilize the new interface for improved type safety. - Adjusted type imports in `balance.spec.ts` to include `IBalanceUpdate`, ensuring consistency in balance management functionalities. - Enhanced overall code clarity and maintainability by refining type definitions related to balance operations. * feat: add unit tests for loadAgent functionality and enhance agent loading logic - Introduced comprehensive unit tests for the `loadAgent` function, covering various scenarios including null and empty agent IDs, loading of ephemeral agents, and permission checks. - Enhanced the `initializeClient` function by moving `getConvoFiles` to the correct position in the database method exports, ensuring proper functionality. - Improved test coverage for agent loading, including handling of non-existent agents and user permissions. * chore: reorder memory method exports for consistency - Moved `deleteAllUserMemories` to the correct position in the exported memory methods, ensuring a consistent and logical order of method exports in `memory.ts`.	2026-03-21 14:28:53 -04:00
crossagent	290984c514	🔑 fix: Type-Safe User Context Forwarding for Non-OAuth Tool Discovery (#12348) ... * fix(mcp): pass missing customUserVars and user during unauthenticated tool discovery * fix(mcp): type-safe user context forwarding for non-OAuth tool discovery Extract UserConnectionContext from OAuthConnectionOptions to properly model the non-OAuth case where user/customUserVars/requestBody need placeholder resolution without requiring OAuth-specific fields. - Remove prohibited `as unknown as` double-cast - Forward requestBody and connectionTimeout (previously omitted) - Add unit tests for argument forwarding at Manager and Factory layers - Add integration test exercising real processMCPEnv substitution --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-03-21 12:46:23 -04:00
Danny Avila	c68066a636	🪝 fix: MCP Refresh token on OAuth Discovery Failure (#12266) ... * 🔒 fix: Prevent token leaks to MCP server on OAuth discovery failure When OAuth metadata discovery fails, refresh logic was falling back to POSTing refresh tokens to /token on the MCP resource server URL instead of the authorization server. A malicious MCP server could exploit this by blocking .well-known discovery to harvest refresh tokens. Changes: - Replace unsafe /token fallback with hard error in both refresh paths - Thread stored token_endpoint (SSRF-validated during initial flow) through the refresh chain so legacy servers without .well-known still work after the first successful auth - Fix revokeOAuthToken to always SSRF-validate the revocation URL, including the /revoke fallback path - Redact refresh token and credentials from debug-level log output - Split branch 2 compound condition for consistent error messages * ✅ test: Add stored endpoint fallback tests and improve refresh coverage - Add storedTokenEndpoint fallback tests for both refresh branches - Add missing test for branch 2 metadata-without-token_endpoint case - Rename misleading test name to match actual mock behavior - Split auto-discovered throw test into undefined vs missing-endpoint - Remove redundant afterEach mockFetch.mockClear() calls (already covered by jest.clearAllMocks() in beforeEach)	2026-03-16 09:31:01 -04:00
Danny Avila	acd07e8085	🗝️ fix: Exempt Admin-Trusted Domains from MCP OAuth Validation (#12255) ... * fix: exempt allowedDomains from MCP OAuth SSRF checks (#12254) The SSRF guard in validateOAuthUrl was context-blind — it blocked private/internal OAuth endpoints even for admin-trusted MCP servers listed in mcpSettings.allowedDomains. Add isHostnameAllowed() to domain.ts and skip SSRF checks in validateOAuthUrl when the OAuth endpoint hostname matches an allowed domain. * refactor: thread allowedDomains through MCP connection stack Pass allowedDomains from MCPServersRegistry through BasicConnectionOptions, MCPConnectionFactory, and into MCPOAuthHandler method calls so the OAuth layer can exempt admin-trusted domains from SSRF validation. * test: add allowedDomains bypass tests and fix registry mocks Add isHostnameAllowed unit tests (exact, wildcard, case-insensitive, private IPs). Add MCPOAuthSecurity tests covering the allowedDomains bypass for initiateOAuthFlow, refreshOAuthTokens, and revokeOAuthToken. Update registry mocks to include getAllowedDomains. * fix: enforce protocol/port constraints in OAuth allowedDomains bypass Replace isHostnameAllowed (hostname-only check) with isOAuthUrlAllowed which parses the full OAuth URL and matches against allowedDomains entries including protocol and explicit port constraints — mirroring isDomainAllowedCore's allowlist logic. Prevents a port-scoped entry like 'https://auth.internal:8443' from also exempting other ports. * test: cover auto-discovery and branch-3 refresh paths with allowedDomains Add three new integration tests using a real OAuth test server: - auto-discovered OAuth endpoints allowed when server IP is in allowedDomains - auto-discovered endpoints rejected when allowedDomains doesn't match - refreshOAuthTokens branch 3 (no clientInfo/config) with allowedDomains bypass Also rename describe block from ephemeral issue number to durable name. * docs: explain intentional absence of allowedDomains in completeOAuthFlow Prevents future contributors from assuming a missing parameter during security audits — URLs are pre-validated during initiateOAuthFlow. * test: update initiateOAuthFlow assertion for allowedDomains parameter * perf: avoid redundant URL parse for admin-trusted OAuth endpoints Move isOAuthUrlAllowed check before the hostname extraction so admin-trusted URLs short-circuit with a single URL parse instead of two. The hostname extraction (new URL) is now deferred to the SSRF-check path where it's actually needed.	2026-03-15 23:03:12 -04:00
Danny Avila	8318446704	💁 refactor: Better Config UX for MCP STDIO with customUserVars (#12226) ... * refactor: Better UX for MCP stdio with Custom User Variables - Updated the ConnectionsRepository to prevent connections when customUserVars are defined, improving security and access control. - Modified the MCPServerInspector to skip capabilities fetch when customUserVars are present, streamlining server inspection. - Added tests to validate connection restrictions with customUserVars, ensuring robust handling of various server configurations. This change enhances the overall integrity of the connection management process by enforcing stricter rules around custom user variables. * fix: guard against empty customUserVars and add JSDoc context - Extract `hasCustomUserVars()` helper to guard against truthy `{}` (Zod's `.record().optional()` yields `{}` on empty input, not `undefined`) - Add JSDoc to `isAllowedToConnectToServer` explaining why customUserVars servers are excluded from app-level connections * test: improve customUserVars test coverage and fixture hygiene - Add no-connection-provided test for MCPServerInspector (production path) - Fix test descriptions to match actual fixture values - Replace real package name with fictional @test/mcp-stdio-server	2026-03-14 21:22:25 -04:00
Danny Avila	c6982dc180	🛡️ fix: Agent Permission Check on Image Upload Route (#12219) ... * fix: add agent permission check to image upload route * refactor: remove unused SystemRoles import and format test file for clarity * fix: address review findings for image upload agent permission check * refactor: move agent upload auth logic to TypeScript in packages/api Extract pure authorization logic from agentPermCheck.js into checkAgentUploadAuth() in packages/api/src/files/agentUploadAuth.ts. The function returns a structured result ({ allowed, status, error }) instead of writing HTTP responses directly, eliminating the dual responsibility and confusing sentinel return value. The JS wrapper in /api is now a thin adapter that translates the result to HTTP. * test: rewrite image upload permission tests as integration tests Replace mock-heavy images-agent-perm.spec.js with integration tests using MongoMemoryServer, real models, and real PermissionService. Follows the established pattern in files.agents.test.js. Moves test to sibling location (images.agents.test.js) matching backend convention. Adds temp file cleanup assertions on 403/404 responses and covers message_file exemption paths (boolean true, string "true", false). * fix: widen AgentUploadAuthDeps types to accept ObjectId from Mongoose The injected getAgent returns Mongoose documents where _id and author are Types.ObjectId at runtime, not string. Widen the DI interface to accept string | Types.ObjectId for _id, author, and resourceId so the contract accurately reflects real callers. * chore: move agent upload auth into files/agents/ subdirectory * refactor: delete agentPermCheck.js wrapper, move verifyAgentUploadPermission to packages/api The /api-only dependencies (getAgent, checkPermission) are now passed as object-field params from the route call sites. Both images.js and files.js import verifyAgentUploadPermission from @librechat/api and inject the deps directly, eliminating the intermediate JS wrapper. * style: fix import type ordering in agent upload auth * fix: prevent token TTL race in MCPTokenStorage.storeTokens When expires_in is provided, use it directly instead of round-tripping through Date arithmetic. The previous code computed accessTokenExpiry as a Date, then after an async encryptV2 call, recomputed expiresIn by subtracting Date.now(). On loaded CI runners the elapsed time caused Math.floor to truncate to 0, triggering the 1-year fallback and making the token appear permanently valid — so refresh never fired.	2026-03-14 02:57:56 -04:00