Skip to content

👛 fix: Drop Unread Credentials From MCP Domain Validation - #16035

Merged
danny-avila merged 2 commits into
danny-avila:devfrom
SpectralOne:fix/resolve-live-openid-bearer
Sep 17, 2026
Merged

danny-avila merged 2 commits into
danny-avila:devfrom
SpectralOne:fix/resolve-live-openid-bearer

Conversation

@SpectralOne

@SpectralOne SpectralOne commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

With a YAML-configured streamable-http MCP server whose Authorization header uses {{LIBRECHAT_OPENID_TOKEN}}, agent MCP tool loading fails whenever the session's OpenID access token is expired, even though the session still holds a valid refresh token and direct OpenID bearer recovery (#15704) is active for that server.

The early domain gate isEarlyDomainAllowed (api/server/services/MCP.js) runs processMCPEnv over the whole config with the request-time req.user snapshot, which openIdJwtStrategy populates from req.session.openidTokens without refreshing. processSingleValue (packages/api/src/utils/env.ts) then throws OpenIDReauthRequiredError because that snapshot token is stale, before the connection path's resolveDirectOpenIDBearerConfig hook can refresh it. loadTools (api/app/clients/tools/util/handleTools.js) catches and logs the error, the tool is never registered, and the agent run fails with Tool "<name>_mcp_<server>" not found.

The gate decides from config.url alone. The credential it demanded is one it never inspects.

How it works

buildMCPDomainValidationConfig (packages/api/src/mcp/domainValidation.ts) narrows the config to the fields the decision actually reads, so resolving it needs no live credential:

   const validationConfig = processMCPEnv({
     user,
     body: requestBody,
     dbSourced: isUserSourced(serverConfig),
-    options: serverConfig,
+    options: buildMCPDomainValidationConfig(serverConfig),
     customUserVars: getServerCustomUserVars(userMCPAuthMap, serverName),
   });

apiKey, args, env, headers, oauth and oauth_headers are dropped. url is deliberately kept, so a credential placeholder there still fails closed, and so do the {{LIBRECHAT_USER_*}} placeholders a URL may depend on. source and dbId are kept because they decide which placeholders processMCPEnv resolves at all, as is the presence or absence of url that isMCPDomainAllowed fails closed on.

isEarlyDomainAllowed(serverConfig)
  buildMCPDomainValidationConfig(serverConfig)   # copy: url, source, dbId, ...
    processMCPEnv(copy)                          # no credential to resolve, no reauth throw
      isMCPDomainAllowed(resolved.url, ...)
  ...(serverConfig itself is untouched and continues to the connection path,
     where resolveDirectOpenIDBearerConfig refreshes the bearer it owns)

The argument is never mutated, so direct-bearer recovery keeps the placeholder it knows how to refresh.

An earlier revision of this PR instead resolved the live bearer for the gate. That worked, but it spent an upstreamTokenProvider call per gate evaluation, and createMCPTools re-runs the gate for every discovered tool, so a 30-tool server paid roughly 1 + N session round trips per request even when the snapshot was healthy. It also only covered configs whose Authorization header carries the placeholder, since that is what isDirectOpenIDBearerRecoveryEnabled requires; an X-Auth-Token or an admin apiKey holding the same placeholder still failed the gate. Dropping the unread fields needs no IdP work and covers every credential-bearing field. hasDurableMCPAuthorization in the same file already builds its validation copy this way.

Change Type

  • Bug fix (non-breaking change which fixes an issue)

Testing

Hard to reproduce on the fly, so this is covered by unit tests. Three in api/server/services/MCP.spec.js:

  • createMCPTool loads the tool when the gate meets an expired snapshot token, calls no upstreamTokenProvider, and passes the domain check a config with no headers while the original keeps its placeholder.
  • The gate still fails closed with OpenIDReauthRequiredError when the URL carries an unresolvable OpenID credential.
  • The same load behavior on the batch path, createMCPTools.

Ten in packages/api/src/mcp/domainValidation.spec.ts: each credential-bearing field in turn (headers, oauth_headers, env, args, oauth, apiKey), the URL fail-closed case, non-mutation of the argument, and {{LIBRECHAT_USER_ID}} still resolving in a URL that an allowlist then accepts or rejects.

Both new /api cases fail against the pre-fix service.

Note on behavior

For a server that is not in direct-bearer-recovery mode but carries an OpenID credential placeholder in a header, re-authentication now surfaces from the connection path rather than from this gate. Same outcome for the user, raised slightly later and with an actionable message, instead of the tool silently disappearing from the agent's toolset.

Checklist

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • I have commented in any complex areas of my code
  • My changes do not introduce new warnings
  • I have written tests demonstrating that my changes are effective or that my feature works
  • Local unit tests pass with my changes

SpectralOne and others added 2 commits September 17, 2026 00:51
The early domain gate resolves the whole server config through
`processMCPEnv`, but decides from the URL alone. A credential placeholder in
any other field therefore raised `OpenIDReauthRequiredError` from a stale
request-time OpenID snapshot, before the connection path could refresh that
bearer, and the tool was dropped from the agent's toolset.

`buildMCPDomainValidationConfig` narrows the config to what the decision reads,
so the gate needs no live credential. A URL placeholder still fails closed, and
the argument is never mutated, so direct-bearer recovery keeps the placeholder
it knows how to refresh.

Replaces the per-tool `upstreamTokenProvider` call the gate would otherwise
make (1 + N per request per server) and covers every credential-bearing field,
not only `Authorization`.

Co-authored-by: Artyom Bogachenko <SpectralOne@users.noreply.github.com>
@lia-by-librechat

Copy link
Copy Markdown
Contributor

Pushed 2fbb5e818f290faea9215d29eeb656cba739cdca to this branch (on top of 3e64c75e6), keeping the diagnosis and the tests but changing the mechanism.

What the head carries

The diagnosis here is right: isEarlyDomainAllowed resolves the whole config through processMCPEnv, processSingleValue raises OpenIDReauthRequiredError on a stale request-time snapshot, and the throw beats resolveDirectOpenIDBearerConfig on the connection path. What this head changes is what the gate does about it.

Instead of refreshing the bearer for validation, the gate now drops the fields it never reads. isMCPDomainAllowed decides from config.url alone, so buildMCPDomainValidationConfig (packages/api/src/mcp/domainValidation.ts) removes apiKey, args, env, headers, oauth and oauth_headers before resolution. url stays in, so a credential placeholder there still fails closed. The argument is not mutated, so direct-bearer recovery keeps the placeholder it refreshes.

Three reasons for the swap:

  1. No IdP work for a URL decision. resolveEarlyValidationConfig called upstreamTokenProvider whenever a provider existed and direct-bearer was on, not only when the snapshot was stale — and createMCPTools re-runs the gate per discovered tool, so a 30-tool server paid 1 + N provider calls per request, each running assertOpenIDSessionIdentityMatch and the publication-flight assertion before the reuse fast path.
  2. Covers the whole class. isDirectOpenIDBearerRecoveryEnabled requires the Authorization header, so X-Auth-Token: {{LIBRECHAT_OPENID_ACCESS_TOKEN}} — and an admin apiKey carrying the same placeholder — still failed the gate. Both are now covered, and both have tests.
  3. Existing precedent in the same file. hasDurableMCPAuthorization already builds its validation copy with args/env/headers/oauth_headers: undefined for exactly this reason.

Per CLAUDE.md the decision lives in /packages/api with its own unit tests; MCP.js keeps one call inside isEarlyDomainAllowed, so no future call site can reintroduce the bug by forgetting to wrap. resolveDirectOpenIDBearerConfig is no longer imported there.

Tests — your three cases kept their shape, with the assertions following the new mechanism: the gate no longer calls upstreamTokenProvider, the validation config carries no headers, and the original config keeps its placeholder. The fail-closed case moved to a URL-borne credential, which is where failing closed still matters. Plus 10 cases in packages/api/src/mcp/domainValidation.spec.ts covering each credential-bearing field, non-mutation, and URL user-placeholder resolution.

Checks on this head

check result
api jest server/services/MCP.spec.js 89 passed
packages/api jest domainValidation.spec.ts, openid.spec.ts 46 passed
packages/api jest src/utils/env.spec.ts 142 passed
api jest ToolService.spec.js passed
the two new /api cases with MCP.js reverted both fail, as intended
packages/api npx tsc --noEmit 48 errors, byte-identical to the untouched base (stale sibling dist in my sandbox); none in the touched files
eslint, prettier, sort-imports on all 5 files clean

Not run: four mongo-backed suites (mcp.servers, routes/mcp, grants, messages-delete) — mongodb-memory-server cannot start in this sandbox, and they fail identically on the untouched base. CI covers them.

Thanks for the diagnosis and the repro tests — the write-up made the ordering bug easy to confirm.

@lia-by-librechat lia-by-librechat Bot changed the title fix: Resolve live OpenID bearer before the MCP early domain gate 👛 fix: Drop Unread Credentials From MCP Domain Validation Sep 17, 2026
@lia-by-librechat

Copy link
Copy Markdown
Contributor

Retitled and rewrote the description to match 2fbb5e818f290faea9215d29eeb656cba739cdca, since both still described the bearer-refresh mechanism this branch no longer uses. The diagnosis in the summary is unchanged and still yours; the "How it works" section now covers the field-narrowing, and records why the refresh approach was dropped so the history is not lost. Title went from "fix: Resolve live OpenID bearer before the MCP early domain gate" to "👛 fix: Drop Unread Credentials From MCP Domain Validation".

CI is green on this head: 23 checks pass, including Integration Tests (MongoDB and Redis), TypeScript type checks, Static checks and lighthouse. That covers the four mongo-backed suites I could not run locally.

@danny-avila
danny-avila merged commit 12d7890 into danny-avila:dev Sep 17, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants