mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
250aca375a
* fix: resolve MCP tool-name delimiter collision at invocation time
MCP tool keys are identified internally as `${rawToolName}${mcp_delimiter}${serverName}`
(delimiter `_mcp_`). Several call sites parsed this back apart with a naive
`toolKey.split(Constants.mcp_delimiter)`, assuming the delimiter occurs exactly once.
When the raw upstream tool name itself contains the delimiter substring - which
happens whenever it's exposed through a gateway that prefixes aggregated tool names by
server (e.g. a gateway's own "gitlab-get_mcp_server_version" for GitLab's
"get_mcp_server_version" tool) - the combined key has the delimiter more than once.
`.split()` then produces more than two segments, and destructuring
`[toolName, serverName]` silently keeps only the first two, yielding a bogus server
name that matches no configured server. Tool listing still worked (a different code
path builds keys directly without re-splitting), but invocation failed with
`Tool {name} not found`, and `filterAuthorizedTools` rejected such keys outright as
malformed.
Add `splitMCPToolKey`, which splits on the *last* occurrence of the delimiter instead:
the server-name half is always LibreChat's own normalized suffix (guaranteed not to
contain the delimiter), while the raw tool-name half is untrusted and may legitimately
contain it. This matches `.split()`'s result whenever the delimiter occurs once, and
correctly resolves the collision case. Update the four call sites that parsed this
manually (`handleTools.js`, `MCP.js`, `mcp.js` controller, `filterAuthorizedTools` in
`v1.js`) plus one in the client (`useVisibleTools.ts`) to use it.
Fixes #14440
* fix: resolve MCP tool-key boundary against configured server names
splitMCPToolKey moves to librechat-data-provider so the client and backend
share one parser, and takes the configured server names when the caller has
them: the longest name the key actually ends with wins, which is exact.
Position alone cannot identify the boundary because both halves may contain
the delimiter. lastIndexOf alone fixes gateway-prefixed tool names but
regresses servers whose own name contains it, which ToolService.spec.js
already covered; the last-delimiter path now only serves as the fallback for
callers with no configured set.
Also converts the remaining first-occurrence parsers that the delimiter fix
missed - mcp/auth.ts (custom user vars silently unresolved), mcp/oauth/events.ts,
agents/initialize.ts, and the three client parsers that labelled tool calls
with the wrong server.
* fix: keep client tool-call labels on first-delimiter parsing
The three client parsers had deliberate, tested first-delimiter semantics
(ToolCall.test.tsx asserts the full server name for 'foo_mcp_bar' and the
synthetic 'oauth_mcp_server' call), and the client has no configured server
list in scope to resolve the boundary exactly, so they are left as they were.
Threads the configured names into the event-driven definition loader so it
resolves the same boundary as the authorization filter that admits the key,
and documents the one case that stays undecidable without provenance.
* fix: resolve tool-key boundary against all configured servers
resolveConfigServers only returns lazily-initialized config overrides -
ensureConfigServers skips unmodified YAML servers - so on a stock deployment
the known-name list was empty and suffix resolution never engaged. Adds
resolveMcpServerNames, which keeps every configured server in the normalized
form tool keys carry, and uses it at the loading, auth-map and definition
sites.
Background-tool eligibility now resolves against all configured names before
testing ephemeral membership, so a non-ephemeral server whose name ends in an
ephemeral one is no longer misclassified, and useVisibleTools resolves against
the server map it already receives.
* fix: use resolved server provenance and one app-config read
createMCPTool now uses the serverName loadTools already resolved for the key
and only parses as a fallback, so an unmodified YAML server whose name
contains the delimiter no longer resolves to the wrong server for auth,
reconnection and callTool.
resolveMcpServerContext derives config servers and all configured names from
a single getAppConfigForRequest, replacing two independent lookups on the
chat startup path, and degrades to empty like resolveConfigServers instead of
aborting tool loading when the config lookup fails.
* chore: drop unused resolveConfigServers import
* fix: forward server provenance on the all-tools path and read config once
createMCPTools builds each toolKey from the server name it already has but did
not forward it, so the sys__all__sys path re-derived it by parsing and bound
an unmodified YAML server whose name contains the delimiter to the wrong auth
and invocation context.
loadAgentTools now resolves the MCP server context once and threads it into
loadTools, replacing the second app-config read it had introduced on the
non-event-driven chat startup path.
* fix: carry resolved MCP server name through tool classification
definitions.ts resolves the server for each key and then dropped it when
building loadedTools, so buildToolClassification re-derived it with a
last-segment split and recorded 'Workspace' for a server configured as
'Google_mcp_Workspace'. The resolved name now rides along on the tool
instance and classification prefers it over re-parsing.
* fix: consume carried server name when extracting MCP servers
extractMCPServers re-derived the name with a last-segment split, so a server
configured as Google_mcp_Workspace resolved to Workspace and its instructions
were silently omitted. Prefers the name carried on the tool definition
instance, falling back to the split.
* fix: fail closed on ambiguous MCP keys when persisting server names
Persisted mcpServerNames grant agent-scoped access to a DB server by name
(ServerConfigsDB.getAccessibleServers), so a wrong guess exposes an unrelated
server to everyone who can view the agent. The last-segment split turned
search_mcp_Google_mcp_workspace into 'workspace'; such keys were previously
rejected outright at agent save, so admitting them opened this path.
Derives a name only from unambiguous single-delimiter keys. This is #12250's
guard moved to the boundary it was actually protecting, instead of blocking
tool admission.
* fix: keep DB server access for multi-delimiter tool keys
The fail-closed guard was wrong for the case this PR exists to fix. This index
only grants DB-backed servers, and DB names are slugs that cannot contain the
delimiter (generateServerNameFromTitle strips underscores), so the trailing
segment is always the real server for them - dropping it cost every consumer
of a gateway-prefixed tool their shared-agent access.
Also gates the MCP server-context lookup on the filtered MCP set, so an agent
with no MCP tools no longer pays an app-config read on startup.
* fix: resolve tool-call display names without breaking OAuth calls
The display parsers could not use the shared boundary parser because their
tested behavior depends on first-delimiter semantics. That constraint only
applies to synthetic MCP OAuth calls, whose tool half is always exactly
'oauth', so everything after the first delimiter is the server even when the
server name carries one.
splitToolCallName special-cases that form and defers to splitMCPToolKey for
real tool keys, so a gateway-prefixed tool now renders its own name and
server while oauth_mcp_foo_mcp_bar still resolves to foo_mcp_bar.
* fix: persist resolved MCP server provenance on agents
Deriving mcpServerNames from the tool key cannot tell a config server's
trailing segment from a real DB server name, so a config server named
a_mcp_b indexed an unrelated DB server b and shared the agent's viewers into
it. Neither string rule works: the suffix guess exposes, and failing closed
drops legitimate DB access for gateway-prefixed tools.
filterAuthorizedTools already resolves each tool's server against the merged
registry config, so it now collects those names and create, update and
duplicate persist them. No extra registry queries: the update path unions the
newly resolved names with what the agent already had, and duplicate replaces
the copied list rather than inheriting the source's servers.
Display parsing also takes the configured names, so a real tool call on a
delimiter-bearing server renders the right server and icon.
* test: teach MCP hook mocks about useMCPServerNames
Three specs mock ~/hooks/MCP with a hand-listed factory, so adding the hook
to ToolCall made useMCPServerNames undefined under test and every render
threw. Returns a stable array so the mock cannot perturb render counts.
* fix: rebuild agent MCP server index from surviving tools
Unioning the prior names kept a server indexed after its last tool was
detached, so viewers of a shared agent retained agent-scoped access to it.
The index is now rebuilt from the tools that survive the edit: a prior name
carries forward only while some retained tool still resolves to it, using the
agent's own persisted names as the candidate set, and the rebuild runs on any
tool change rather than only when a new MCP tool is added.
* fix: keep duplicate indexes on registry fallback and harden the oauth split
Duplication blanked mcpServerNames when the registry was unavailable, because
filterAuthorizedTools grandfathers the source's tools without resolving them -
the copy kept tools it could no longer resolve. Source names now carry forward
for the tools that still point at them.
splitToolCallName also treated any oauth_mcp_ prefix as a synthetic OAuth
call, so a genuine upstream tool by that name resolved to the wrong server. A
configured server name now decides when one matches, since a real key always
ends in its server, and the prefix only breaks ties for unconfigured servers.
* fix: thread configured server names through display parsing
parseToolName and getMCPServerName resolved context-free, so a configured
server whose name contains the delimiter showed the wrong server in grouped
tool summaries and subagent tool labels, and stacked icons missed its entry in
the icon map. Both take the configured names now, supplied by the components
that render them.
Adds the hook to SubagentCall's mock factory: the spec renders the real
component, so an unmocked useMCPServerNames would reach the query with no
provider.
* test: cover the auth-map boundary, server provenance and context fallback
Adds regression coverage for three behaviors this PR changed that no test
exercised: customUserVars resolving under the right plugin key for a
gateway-prefixed tool name (the failure that made these tools loadable but
unusable), the resolved server name reaching createMCPTool instead of being
re-parsed, and resolveMcpServerContext degrading to empty rather than
aborting tool loading when the config lookup fails.
Each was checked against a mutated source to confirm it fails when the
behavior is broken.
* fix: normalize server-name candidates and cover the boundary guard
Tool keys embed normalizeServerName's output while the config is keyed by the
raw name, so callers passing raw keys never matched a server whose name needs
normalizing and silently fell back to the last delimiter. filterAuthorizedTools
now maps normalized names back to their config key, and createMCPTool
normalizes its candidates.
Adds the cases an audit found surviving mutation: a configured name that is a
bare but not delimiter-aligned suffix must not match, an empty candidate list
behaves as no list, and splitToolCallName still falls back to the oauth prefix
when a list is supplied but nothing in it matches.
* fix: keep resolved server names when a non-owner retains MCP tools
The shared-agent path keeps an agent's existing MCP tools verbatim but supplied
no mcpServerNames, so persistence re-derived them and reduced a configured
server like Google_mcp_Workspace to Workspace - which ServerConfigsDB then
treats as a DB server, granting the agent's viewers access to an unrelated one.
Carries the existing resolved names across instead, and clears the index on the
owner path where every MCP tool is removed.
* fix: preserve resolved MCP names for every tools update
extractMCPServerNames was reachable from any caller that writes tools without
mcpServerNames - the Action edit path does exactly that - so a configured
Google_mcp_Workspace was reindexed as Workspace and ServerConfigsDB granted
shared-agent viewers an unrelated DB server by that name.
updateAgent now rebuilds the index from the agent's own resolved names: one
carries forward while a retained tool still resolves to it, and only keys
matching none of them fall back to derivation. Callers are safe by default
rather than by remembering to pass the set.
normalizeServerName moves to librechat-data-provider so the client can match
its candidates against tool keys, which embed the normalized form; the icon map
is keyed the same way since it is looked up with a parsed server name.
* refactor: move MCP context resolution into packages/api
New backend logic belongs in the TypeScript workspace per CLAUDE.md, with /api
kept to a thin wrapper. resolveMCPServerContext now lives in
packages/api/src/mcp/context.ts and takes ensureConfigServers by injection,
since the registry accessor is still legacy-only; the /api function is reduced
to loading the request app config and translating failures into the empty
degrade it already promised.
* test: teach the MCP service mock about resolveMCPServerContext
The spec mocks @librechat/api with a hand-listed factory, so moving the
resolver into that package left it undefined and the wrapper degraded into its
own catch, returning empty config servers. The stub mirrors the real resolver
so these tests still cover what the wrapper owns - loading the request config
and degrading on failure - while the resolution logic is unit-tested in
packages/api.
* fix: only persist an authoritative MCP server index on update
Assigning the resolved set unconditionally pinned the index to [] whenever
nothing authoritative was available - a legacy agent holding MCP tools with no
stored mcpServerNames - which suppressed updateAgent's derivation and stripped
agent-scoped access to its DB-backed server.
The field is now supplied only when the result is authoritative: names were
resolved, or no MCP tool survives so the index genuinely is empty. The
retained-tools branch likewise leaves it unset when the agent has none stored.
---------
Co-authored-by: Jens Schumann <schumajs@gmail.com>
|
||
|---|---|---|
| .. | ||
| __tests__ | ||
| Agents | ||
| Artifacts | ||
| Config | ||
| Endpoints | ||
| Files | ||
| Runs | ||
| Skills | ||
| start | ||
| Threads | ||
| Tools | ||
| ActionService.js | ||
| ActionService.spec.js | ||
| AssistantService.js | ||
| AuthService.js | ||
| AuthService.spec.js | ||
| cleanup.js | ||
| createRunBody.js | ||
| GraphApiService.js | ||
| GraphApiService.spec.js | ||
| GraphTokenService.js | ||
| initializeMCPs.js | ||
| initializeMCPs.spec.js | ||
| initializeOAuthReconnectManager.js | ||
| MCP.js | ||
| MCP.spec.js | ||
| MCPRequestContext.js | ||
| OboPolicyService.js | ||
| OboTokenService.js | ||
| OboTokenService.spec.js | ||
| PermissionService.js | ||
| PermissionService.spec.js | ||
| PluginService.js | ||
| systemGrant.spec.js | ||
| ToolService.js | ||
| twoFactorService.js | ||