* fix(api): align message search with page size
Pass a positive, capped page size to MeiliSearch and use the same conversation limit so requested message search pages are not silently truncated.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(data-schemas): expand shared-link search results
Request the MeiliSearch result ceiling using only conversationId so cursor pagination is not limited to the default 20 hits or burdened with unused fields.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* 🚧 fix: Narrow Chat MCP Selections to What the Picker May Offer
The chat MCP picker offers a server only when the operator has not hidden it
(`chatMenu: false`) and the user reaches it directly rather than through an
agent (`consumeOnly`). Nothing enforced either condition on the request, so a
stale client, a replayed body, or a hand-written one could name any server and
get its tools — `consumeOnly` in particular is a permission the registry derives
per user, so ignoring it let a user reach in plain chat a server they had only
been granted through an agent.
Narrow the picker's selection in the two ephemeral loaders, against the same
predicate the client's `selectableServers` uses. The resolver is injected, so
`packages/api` stays free of the registry and the loaders keep their existing
`deps` shape.
Provenance is why this lives in the loaders rather than at the tool-resolution
chokepoint: only here is a server name known to have come from the picker.
Servers a model spec pins are the operator's own choice and are added after the
narrowing, so pinning a chat-hidden server to a spec keeps working.
`chatMenu` only ever comes from the config tier, so the overlay settles it with
no lookup at all. Lookups for the rest run concurrently and are cached per
server and user. A name the registry cannot resolve — request-tier, declared on
the body and never registered — is kept, as is one whose lookup fails: this
narrows an already-authenticated selection and is not the authorization
boundary.
* 📝 docs: Say What chatMenu Now Bounds
The schema still described `chatMenu` as dropdown visibility alone, which is
misleading now that it also bars a server from the chat selection a request
carries. Spell out both halves, and what it deliberately does not restrict:
agents, and servers a model spec pins.
* 🛡️ fix: Close Four Gaps in the Chat MCP Narrowing
Dedupe before resolving. The selection is request-supplied and the loaders used
to build a `Set` immediately; mapping the raw array first let one body repeat a
name enough times to launch a resolver — registry, cache, and ACL reads, with no
single-flight — per occurrence.
Resolve names through the tool-loading aliases. A configured `private server` is
addressable as `private_server` downstream, so an exact-key lookup let the
normalized spelling walk past a `chatMenu: false` check and still load tools.
Names are returned as sent; only the decision is canonicalized.
Let the request overlay settle `chatMenu`. The config tier is where the flag
lives and is what the picker itself was built from, so an overlay that re-enables
a server now outranks the durable config the registry returns, which was silently
dropping a valid selection. `consumeOnly` still comes from the registry.
Narrow the request body, not just the loader's copy. `applyContextToAgent` reads
`req.body.ephemeralAgent.mcp` directly and prefers it over the agent's tools, so
a hidden server's `serverInstructions` reached the model context even after its
tools were dropped.
* 🧭 fix: Ask the Registry Which MCP Servers the Picker May Offer
Three rounds of review found the same mistake in three disguises: this narrowing
re-derived the registry's tier precedence instead of asking for it. A config
entry was treated as authoritative over a user-DB or process-backed server of the
same name; the alias map was built from config names only, so a DB server named
`private_server` was misrouted to a config `private server`; and the overlay
short-circuit ran before any of that could be consulted.
Resolve the accessible set through `resolveAllMcpConfigs` instead — the same call
behind `GET /api/mcp/servers`, so what the request may use and what the picker
was offered come from one place and cannot drift. Precedence, cross-tier name
collisions, and the config overlay are the registry's job again, and the
selectability test is a single flag check over what it returns.
The result is smaller: no overlay branch, no per-server lookups, one catalog read
regardless of how long the selection is, and aliases built over the whole
accessible set so an exact name always outranks another server's normalized form.
Publish the servers actually in play back onto the request body, spec pins
included. `applyContextToAgent` prefers that list over the agent's tools, so
narrowing to the picker selection alone had started dropping a spec-pinned
server's `serverInstructions` while suppressing a hidden one's.
* 🔑 fix: Decode Action OAuth Credentials After Decryption
Action client credentials are URL-encoded before encryption, but the OAuth
token exchange decrypted them with `decryptV2` alone, sending values like
`abc%40domain.com` to the provider and causing 401s for secrets containing
`@`, `+`, `=`, `/` or `:`. Decoding falls back to the raw decrypted value so
legacy secrets stored before 299cabd6e (which contain an unescaped `%`) still work.
Fixes#14636
* 🔑 fix: Share Action Credential Codec Between Read Paths
The encode-before-encrypt convention had two readers with different
behaviour. `ActionService.decryptSensitiveValue` decoded unconditionally,
so a credential stored before encoding was introduced (299cabd6e, March
2025) containing a stray `%` threw `URIError` and failed every read of the
action, not just the token exchange.
Extract the pair into `packages/api/src/actions/crypto.ts` as the single
implementation and consume it from both `ActionService` and the OAuth token
exchange, so the two paths cannot drift again.
Tests cover the codec against real crypto: reserved characters and literal
percent escapes round-trip, pre-encoding values are returned unchanged, and
a pre-encoding value with a stray `%` no longer throws.
A version marker on newly encrypted values was prototyped and dropped: it
changes the stored format for every action credential while fixing nothing,
since values written by the encoding convention already decode correctly
and pre-encoding values stay unmarked either way.
---------
Co-authored-by: Danny Avila <danny@librechat.ai>
* 🖼️ fix: Stop Reporting a Failed Sandbox Image Read as an Unreadable Format
`read_file` on a `/mnt/data` image collapsed every failure — rate limit,
missing path, truncated transfer, transport error — into the binary-file
hint, whose image branch reads as a permanent capability limit ("is an
image file and cannot be read as text"). The model stopped retrying, and
the operator had no way to tell which failure had actually occurred.
The failure that surfaced it: the reader windows the bytes through
`/exec` stdout, one call per window, against the Code API's per-user
execution limiter (20 requests / 30s in the reference deployment). At a
fixed 32KB window a 122KB chart cost 4 calls and a 1MB image cost 33 —
more than a whole limiter window, so anything over ~640KB could never
complete. A turn that generates several charts and reads them back
exhausts the budget partway through and reports the 429s as unreadable
images.
- `handlers.ts`: failed reads name the real cause and the affordance that
matches it — rate-limited waits and retries, a missing path lists the
directory, a truncated transfer retries. The "cannot be read as text"
phrasing is now reserved for bytes that are not a supported image. An
image past the inline budget says to downscale and read the smaller
copy rather than pointing at `bash_tool`, which cannot show an image.
- `process.js`: the window is derived from the runner's stdout budget
(48,960B vs 32,768B, so a 122KB chart is 3 calls and 1MB is 22), halves
and re-reads the same offset when a runner truncates anyway (remembered
per Code API, with the env remedy logged), waits out a rate limit it
can afford instead of discarding the bytes already pulled, and stops at
a 24-call ceiling so no read stalls a turn across limiter windows.
- `utils/code.ts`: shared `getCodeApiRetryAfterMs` (header, then the
`retry_after_seconds` body); `skillFiles.ts` drops its private copy.
* ♻️ refactor: Move the Sandbox Image Reader Into TypeScript
Addresses the review round on #15453.
- P1: windowing, window sizing, the in-sandbox reader script, response
parsing and byte assembly move to `packages/api/src/files/code/image.ts`,
and the Code API rate-limit policy to `packages/api/src/utils/code.ts`.
`process.js` keeps only the `/exec` transport it owns. Both new modules
are tested against their real implementations (19 + 13 cases), including
the round-trip ceiling, the changed-file guard and the shell-injection
guard on the reader script.
- P2: a configured stdout budget below the old 8KB window floor produced a
window the runner could never emit, and narrowing was pinned to the same
floor, so no retry could succeed. The window now aligns to the base64
group size instead, and the round-trip ceiling bounds what a small budget
costs.
- P2: `.env.example` documented a 32,768-byte default that no longer holds
and never mentioned the stdout budget. It now describes
`LIBRECHAT_CODE_SANDBOX_OUTPUT_MAX_SIZE` as the setting to tune, with the
chunk override as the escape hatch.
- P3: `process.spec.js` no longer reimplements the retry parser in its
module mock. Behaviour is tested where it lives; the mock is a declared
passthrough and the spec covers what the transport still owns — session
forwarding, the profile header, parser hand-off and error logging.
- P3: `getCodeApiRetryAfterMs` reads the response body through `in`
narrowing rather than an `as` assertion.
- P3: the window-sizing JSDoc no longer claims a multiple of 3 in the one
case that is not — an explicit `LIBRECHAT_CODE_IMAGE_CHUNK_BYTES`.
* 🎯 fix: Size Sandbox Image Reads to What the Limiter Can Deliver
Addresses review round 2 on #15453.
- Narrowing now halves the caller's own failing window instead of the
shared learned value, and the learned value only moves down to the
narrowest size any read needed. Reads run concurrently (one per tool
call), so two reads failing at the same size were compounding into a
window two steps smaller than the runner would have accepted, and
persisting it for every later read.
- Narrowing continues until a runner accepts a window or a floor below
which no runner could serve an image at all. Two halvings stopped at
12,240 bytes — still ~16KB of base64 — so an 8-10KB stdout cap that was
not configured up front stayed unreachable.
- The round-trip ceiling is now one limiter window (20), and a read that
cannot finish within it says so from the FIRST response rather than
spending the whole budget rediscovering it: the first window reveals
both the file size and what a call really delivers. A near-limit image
no longer drains the turn's executions before failing, which is what
made the wait budget look too small.
- The round-trip message names the size this deployment can actually
inline, so the model has a downscale target instead of a guess.
- `python3: not found` from the sandbox reader was matching the generic
"not found" in `isSandboxMissingFileError` and being reported as a
missing image path, sending the model to `ls /mnt/data` while hiding a
runner dependency. Image reads now classify only path-specific ENOENT
forms as a missing file.
* 📐 fix: Size a Truncated Read's Retry From the Cap It Revealed
Addresses review round 3 on #15453.
A ~1KB sandbox stdout cap is a real runner default, and the 1,536-byte
narrowing floor sat above it: discovery stopped one step short and
declared every image unreadable, even though small ones fit comfortably
in the 576-byte window the configured path already derives.
Narrowing now runs until no smaller window exists. It also no longer has
to grope its way down: a truncated response emitted exactly as much as
the runner allows, so its length measures the cap this deployment never
declared, and the retry is derived from it directly — one round-trip
instead of the eight blind halvings it takes to fall from ~49KB to ~1KB.
Blind halving remains the fallback and bounds the result either way, so
every retry is strictly smaller than the window that just failed.
* 🗂️ fix: Resolve Code Interpreter Destination Collisions
Codeapi mounts each `/exec` input at a destination derived from its `name`
and rejects the whole request when two entries land on one. LibreChat never
enforced that: `primeFiles` sent `name: file.filename` verbatim, and the
graph seed deduped by `(storage_session_id, id)` — identity, not destination.
Two conversation files sharing a filename therefore killed code execution for
the rest of the conversation. The rejected request never reaches the sandbox,
so no result comes back to collapse the pair, and every later turn re-primes
both and fails identically. Only code-generated outputs are covered by the
`(filename, conversationId, context, tenantId)` partial unique index; uploads
carry `context: message_attachment`, so one conversation can hold several
records sharing a filename — two uploads of `image.png`, or an upload whose
name a later execution wrote back in place.
`packages/api/src/files/code/destinations.ts` now owns the name → destination
mapping for every contributor to an `/exec` file list, mirroring codeapi's
conflict rule including directory-prefix conflicts:
- `primeFiles` claims per pushed file, disambiguating with a `-2`, `-3`
counter and echoing the resolved path into the tool context, so the model
is told where each file actually mounts. Claim order comes from immutable
`createdAt`, not `getFiles`'s `updatedAt` sort, which usage accounting and
re-upload both bump — order-dependent destinations would silently repoint
a path an earlier turn had already handed the model. Newest wins the bare
name, matching `ToolNode.updateCodeSession` and giving an in-place rewrite
precedence over the original it superseded.
- `seedCodeFilesIntoSessions` drops a later file whose destination is already
claimed. Renaming there would invent a path nothing told the model about,
and a conflict at that layer means one logical file arrived twice under
different storage pointers.
Fixes#15443
* 🧹 chore: Tolerate Holes in the Destination Priority Sort
`primeFiles` sorts the raw `getFiles` result before its own per-entry guard
runs, so the comparator has to survive a null row rather than throw ahead of
the loop that was already defending against one.
Also tightens the tool-context assertion in the upload-vs-output case, which
was passing on a trailing space rather than on the path.
* 🔁 fix: Address Codex Review on Destination Assignment
Four findings from the review of `b0f61d0`.
**Identity-derived suffixes instead of a collision counter.** A counter is
assigned from whatever is in the set, so a third file arriving under the
literal name a displaced file had been given pushed that file along, and code
the model wrote in an earlier turn would silently start reading the newcomer.
The fallback now hashes the file's own `file_id`, so a displaced file keeps
its destination for the life of the conversation and only the winner of the
bare name can change. A trailing counter still guards a hash collision so the
search terminates on any input.
**Rank by last content write, not record creation.** `processCodeOutput`
claims one row per `(filename, conversationId)` and rewrites it in place, so
a repeatedly written output keeps its original `createdAt` while recording the
latest writer in `metadata.sourceDispatchedAt`. Ranking on `createdAt` alone
handed the bare path to an upload the output had since been rewritten over.
Recency is now `max(sourceDispatchedAt, createdAt)` — still never `updatedAt`,
which usage accounting bumps without any content changing.
**Advertise a displaced generated output.** Generated files are left out of
the tool context because the model already knows what it wrote — which stops
being true once a newer same-named file takes the bare path and the output
mounts somewhere the model has never seen. Displaced outputs are now listed
with the name they were written as, and displaced uploads name their original
too.
**Rank conversation files above an agent's private ones.** Each agent claims
destinations over its own candidate set — the conversation's files plus its
own — so a private file taking a bare name from a shared file in one agent but
not in another left two agents advertising different paths for the same file
into one shared mount namespace. Shared files now rank first, so the majority
that every agent sees resolves identically. The private-versus-private case
still drops at the merge and is documented there; closing it needs one
assignment across contributors, made before any agent renders its context.
* 📌 test: Pin Alias Ownership on an In-Place Rewrite
Codex round 2 flagged that the identity alias still falls back to a counter
when the alias itself is literally claimed, so a displaced file can move after
all. The mechanism is real and my JSDoc overstated the guarantee, which this
corrects.
The behaviour stays as it is. The realistic way a file arrives under another
file's alias is the model rewriting the displaced file in place: it was told
`<stem>-<hash>.ext`, wrote back to it, and `processCodeOutput` registered an
output under that literal name. Keeping the alias with the newest holder is
what makes the model's familiar path resolve to its own edit; reserving it
against the newcomer would hand that path back to the bytes it replaced. The
test pins that ordering so a later change cannot invert it silently.
* 🔐 fix: Redirect OAuth Browser Navigations Instead of Rendering JSON
The /oauth/* routes are reached by top-level browser navigation, so a
middleware that rejects one with a JSON body makes the browser render
that body as the page. Hitting the login rate limit during a social
sign-in dropped the user on a blank page showing:
{"message": "Too many login attempts, please try again after 5 minutes."}
Mark those requests with markOAuthNavigation and have the two middlewares
that can reject them answer with a redirect to the login page carrying an
error code the client localizes. The login limiter stays a single instance
so the attempt budget is still shared with POST /api/auth/login, and that
route keeps its JSON 429 for the SPA.
* 🧪 fix: Address PR Review Bot Findings
chatgpt-codex-connector:
- Split the inline `type TranslationKeys` specifier out of the value
import, per the standalone `import type` convention in CLAUDE.md. This
also clears the Static checks import-sort failure.
- Cover the new OAuth error branches in Login.spec.tsx: each mapped
`?error=` code raises its localized toast and the query is cleaned,
and unmapped codes including prototype members raise nothing.
* 🧹 fix: Strip LangChain Attribution From User-Facing Provider Errors
LangChain classifies provider failures by mutating `error.message` with a
docs URL, and that message is forwarded verbatim into the error content part
we persist, so users saw `docs.langchain.com/...` credited for a provider's
own 429. The client only intercepted `MODEL_NOT_FOUND`, leaving every other
code to render raw.
Resolve the classification server-side instead: codes with localized copy
become the typed payload the client already knows how to localize, and every
other message keeps the provider's wording with the docs URL removed before
it reaches the database. The untouched error still reaches the logs through
`getSafeErrorMetadata`.
The client keeps a code-driven lookup for messages persisted before this, and
gains `com_error_model_rate_limit` alongside the existing not-found copy. The
OpenAI-compatible and Responses controllers now share one provider-error
helper rather than three copies of it, so the strip applies on those surfaces
too.
* 🛡️ fix: Harden Error Sanitization For Non-String Messages And Legacy Text
Codex review follow-up.
`stripLangChainTroubleshootingUrl` assumed a string, so an error-like object
with a truthy non-string `message` threw `.replace is not a function` inside
the error handler that was meant to report it — the template interpolation it
replaced had coerced such values safely. It now coerces at the boundary, as
does the code lookup.
Legacy messages whose LangChain code has no localized copy still rendered the
docs URL, which is exactly the historical case the client-side path exists to
clean up. The rendered fallback is now stripped too.
Both helpers move to `librechat-data-provider`, which is shared by the client
and the backend, so one implementation serves both instead of a second regex
drifting in the component.
* 🖼️ fix: Apply an Agent's Configured Image Detail
The Agent Builder exposes an Image Detail slider and saves it to
`model_parameters.imageDetail`, but nothing ever read it back: `encodeAndFormat`
resolves the level from `req.body.imageDetail`, which the agents route never
populates, so every image was encoded as `detail: "auto"`. Providers that reject
`auto` — MiniMax's OpenAI-compatible API among them — fail every image upload
through an agent, and the setting that would fix it has no effect.
The root cause is a classification error rather than a missing argument.
`extractLibreChatParams` splits LibreChat's own settings out of an agent's model
parameters, and its key set is derived from the `librechat` group in
`parameterSettings`. `imageDetail` sat in `baseDefinitions` instead, so it was
neither extracted for LibreChat's use nor stripped from the options handed to the
provider.
It is now grouped with `resendFiles` and the other LibreChat-level parameters,
which gives it the same path those already take: extracted during agent
initialization, carried on the initialized agent, passed to the client, and read
by `encodeAndFormat`. `AgentClient.getSaveOptions` already expected
`options.imageDetail` — nothing had ever populated it.
The request body still wins where it is set, so the classic endpoints keep
resolving detail from conversation settings exactly as before.
Fixes#15359
* 🔤 fix: Type Image Detail as Its Enum, Not a Bare String
`TConversation.imageDetail` is the `ImageDetail` enum, so the extractor's return
type and the initialized agent's field are now declared as that enum rather than
widened to `string`, and the tests pass enum members instead of raw literals.
Caught by CI's type check, which I had run before adding the tests and not after —
jest compiles the specs through babel, so the suite stayed green over a type error.
* fix: recover missing MCP marketplace catalogs
* fix: make MCP catalog recovery passive
* test: type MCP catalog recovery fixtures
* fix: bound and back off passive MCP catalog recovery
Passive recovery runs inline on `GET /api/mcp/tools` and its results are
request-local by design, so every list request re-dialed the same cold
servers with the default connection timeout. Three limits keep that cost
proportional to what recovery can actually recover:
- Cap the discovery timeout at 5s instead of inheriting the connection
default (`initTimeout ?? 30s`); a server configured to connect faster
keeps its own shorter limit.
- Skip a server the config tier already marked `inspectionFailed`, leaving
it to that tier's retry window rather than re-dialing it per request.
- Skip a server whose declared `customUserVars` are unset, matching the
gate `reinitMCPServer` applies for issue #10969 — connecting without them
fails auth, so the attempt is spent for nothing.
Servers that still fail discovery enter a one-minute per-process cooldown,
which is what stops an unreachable server from being re-dialed by every
subsequent list request. A server that recovers clears its own entry, and
expired entries are swept at most once per window so the map stays bounded.
Skipped servers render exactly as they did before recovery existed: present
in the catalog with an empty tool list.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B
* fix: bound passive MCP recovery by deadline, key cooldowns by config
Both follow-ups address the same mistake: recovery expressed its own
request-level constraints in terms borrowed from other layers.
`connectionTimeout` bounds one connection attempt, and
`MCPConnectionFactory.discoverToolsInternal` spends it twice — once on the
authenticated connection, then again in `attemptUnauthenticatedToolListing`
— so capping it bounded no total this layer could reason about. Recovery now
enforces its own wall-clock deadline per server with `withTimeout`, which
holds however many attempts the factory makes; `connectionTimeout` is left to
do only its own job, still honouring a shorter operator `initTimeout`. An
attempt abandoned by the deadline disposes its own connection when it
settles, and `Promise.race` keeps a handler on it, so a late rejection is
not unhandled.
A per-request budget now caps total recovery regardless of server count.
A server is dialed only if the remaining budget can fund a full deadline;
never dialing one is not evidence against it, so a skipped server records no
cooldown and a later request reaches it once those ahead are cached or
cooling down.
Cooldown identity now includes the publication generation — the same
effective-config identity the tool caches fence on — instead of just user and
server name. Correcting a server's URL or transport keys a new entry, so the
refetch the client issues on update is no longer skipped for up to a minute
by the previous configuration's failure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B
* refactor: keep passive MCP recovery stateless and bounded by its own work
Reverts the cooldown, request budget and deadline race added in 257d5cf and
fc3e3c9, and keeps only the three stateless limits.
The tool cache refuses unfenced writes (`tools.ts`), and a discovery
connection owns no publication generation and is disposed, so a recovered
catalog cannot be retained by design. Those commits responded by building a
cache-shaped memory in front of it — per-process failure state, a scheduling
budget, an identity, an eviction sweep — and each round of review found
another way that hand-rolled cache differed from a real one: wrong identity
for configuration, wrong identity for credentials, no fairness across
requests, and a limiter slot released while its network operation was still
running. None of that machinery was asked for; all of it was compensation for
a result the architecture does not allow keeping.
Recovery is now stateless. It skips only what configuration alone proves
pointless — a server the config tier already marked `inspectionFailed`, and
one whose declared `customUserVars` are unset — and bounds the work itself
rather than racing it, so a limiter slot is held for exactly as long as its
network operation runs and the concurrency limit of three is real.
The attempt timeout is not a compromise: recovery exists for a server that is
reachable and authorized but whose catalog cache expired, and such a server
answers tools/list well inside 1.5s. Anything slower cannot be rescued here,
so failing fast costs nothing. The factory spends that value per attempt, so
a server's ceiling is it times the attempts made; the constant documents that
rather than hiding it behind a number tuned to today's attempt count.
Consequences that were bugs are now gone by construction: every cold server
is attempted on every request, so none is starved by those ahead of it, and
correcting a server's configuration or credentials takes effect on the next
refetch instead of waiting out a stale cooldown.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B
* fix: correct the inspection-failure skip and bound catalog fan-out
Three fixes that belong to this layer; a fourth issue does not, and is
described below.
The `inspectionFailed` skip was too broad. `MCPServersInitializer` stores a
YAML server that was unreachable at startup via `addServerStub`, which stamps
`source: 'yaml'`, and only config-tier entries get the timed retry in
`ensureSingleConfigServer`. Skipping every failed stub therefore hid a
recoverable server from the marketplace permanently — the exact state this
recovery exists to escape. It now defers only `source === 'config'`, matching
what `reinitMCPServer` already does.
Plugin auth is read only when some cold server actually declares
`customUserVars`, and only for those servers. The common unauthenticated case
no longer pays a MongoDB round trip whose result nothing can consume.
Snapshot refreshes are now bounded by the same limiter as discovery. They are
not local reads: both connection paths reach `fetchOrderedToolsSnapshot` and
issue a real `tools/list`, so a cache reset across many servers previously
burst unbounded outbound requests while discovery was capped at three.
Not fixed here, because it cannot be: `connectionTimeout` does not bound
discovery. It covers `connection.connect()` only, and `fetchToolsSnapshot`
then applies its own `TOOLS_LIST_TIMEOUT_MS` (30s) to `tools/list`, so a
server that connects fast and stalls while listing still holds its slot for
that window. The factory also does not cancel a timed-out connect before
starting the unauthenticated fallback. Bounding this end to end needs a
deadline threaded through `MCPConnectionFactory` into both `connect()` and
`fetchToolsSnapshot()`, which is a change to shared connection machinery
rather than to this caller. The constant's comment now states what it does
and does not bound instead of implying an end-to-end guarantee.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B
* fix: thread live-session OBO context into passive catalog discovery
The merge of #15334 sources OBO tokens from the live OpenID session via
request-boundary closures. Passive catalog recovery is a discovery call
site too; without these options an OBO server whose stored token went
stale fails recovery — the exact cold-catalog class this PR fixes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xr1Dabvdn1mzzyYgpJgU5B
---------
Co-authored-by: Claude <noreply@anthropic.com>
FluxAPI._call() only dispatched to generateFinetunedImage() when action
was explicitly "generate_finetuned". If a model set endpoint to
/v1/flux-pro-finetuned or /v1/flux-pro-1.1-ultra-finetuned while leaving
action at its default ("generate"), the request went through the plain
generate payload path instead, which never forwards finetune_id,
finetune_strength, or guidance. The finetune was silently dropped and a
finetuned endpoint was called without its required finetune_id.
Route by endpoint as well as action, and share the finetuned-endpoint
list between the dispatcher and generateFinetunedImage's own validation.
Fixes#15335
* 🧊 fix: Inline-refresh OpenID session tokens at MCP OBO call time
Resolves the walk-away failure mode where MCP tool calls using OBO auth
fail with "No valid OpenID access token is available for OBO exchange"
after a user idles past their access-token lifetime. The strategy-time
snapshot on `user.federatedTokens` could expire mid-stream before
`resolveOboToken` ran, while `req.session.openidTokens` carried a still-
valid (or refreshable) token that nothing read.
- New OpenIDSessionRefresh service: per-user single-flighted closure that
reads `req.session.openidTokens` at OBO time and inline-refreshes via
`openid-client.refreshTokenGrant` when expired (30s skew), persisting
via `req.session.save()`. No cookie writes (headers already flushed).
- `resolveOboToken` gains a required UpstreamTokenProvider parameter
(typed as `() => Promise<OIDCTokens | null>`, reusing the shared shape
from @librechat/data-schemas). Compile-time guarantee that every call
site is updated.
- New `session_refresh_failed` OboTokenResolutionReason distinguishes
"session expired and IdP rejected refresh" from "no upstream token
ever existed."
- `req` threaded through createMCPTool/createMCPTools/createToolInstance
to construct the closure with captured request, plus fail-closed
guards in MCPConnectionFactory.getOboTokens and MCPManager.callTool
when the closure isn't plumbed.
- Startup warning in MCPServersInitializer when OBO is configured but
OPENID_REUSE_TOKENS is unset (the strategy populating
user.federatedTokens is only registered under reuse, so OBO would
fail every call without it).
Tests: 16 new in OpenIDSessionRefresh.spec.js; obo.spec.ts extended
for the new param + error reason; wiring smoke tests in MCPManager,
MCPConnectionFactory, MCPServersInitializer, and MCP.spec.js.
* 🛡️ fix: Harden OBO inline-refresh against token type and session edge cases
- Token-preference asymmetry: live-token reuse and expires_at derivation
now strictly gate on the access_token, not the id_token. Added a
required `tokenPreference` parameter on isLiveSessionTokenStillValid,
buildOIDCTokensFromSession, and createOpenIDSessionTokenProvider
so every call site is explicit. Dropped the bogus id_token-exp
fallback in performIdpRefresh — id_token TTL is governed by IdP
session policy and would mark a short-lived access_token reusable
past its real lifetime.
- Missing req in /reinitialize route: the manual reconnect
endpoint now forwards req into reinitMCPServer, so OBO servers can
build a session-aware upstream-token closure instead of failing with
missing_upstream_token.
- Single-flight key collisions: composed key as
tenantId:openidIssuer:openidId:sessionId via getSingleFlightKey.
Concurrent calls in the same session still coalesce; separate sessions
never share an in-flight refresh, preventing refresh-token rotation
from breaking sibling sessions and preventing cross-tenant token
crossover when distinct users share an IdP sub.
- Opaque access token reuse): persist accessTokenExpiresAt
(unix seconds, from tokenset.expires_in) on each refresh AND on initial
login / SPA refresh in setOpenIDAuthTokens. New getAccessTokenExp
helper falls back to it when the access token isn't a JWT, avoiding
redundant inline refreshes for Microsoft Graph and Auth0 default
audiences.
- Log hygiene: the single-flight key (containing sessionId,
openidId, openidIssuer, tenantId) is now SHA-256-hashed in the
"Joining in-flight refresh" debug log. Preserves cross-line correlation
via a 12-char prefix without leaking credential or PII material.
Documented req.session.openidTokens shape contract via JSDoc typedef so
the new accessTokenExpiresAt field has a discoverable home alongside the
existing accessToken/idToken/refreshToken/expiresAt/lastRefreshedAt.
Tests: OpenIDSessionRefresh.spec.js up to 30 passing (added coverage for
opaque-token reuse, JWT-access-token-exp fallback, no-id_token-fallback
regression, cross-session no-coalesce, persistence on refresh, and a
guard against stale accessTokenExpiresAt carryover). AuthService.spec.js
adds two cases covering accessTokenExpiresAt persistence on login.
mcp.spec.js (route) gains a regression test asserting req flows into
reinitMCPServer.
* 🔍 fix: Detect OBO-only MCP admin config overrides
Admin Config overlays for YAML-defined MCP servers compare only
ADMIN_CONFIGURABLE_FIELDS to decide whether to lazy-init a config-tier override.
The OBO config field was added after that fingerprint list, so an override that
only added or changed `obo` was treated as unchanged YAML and skipped.
Include `obo` in the admin-configurable field list and add a regression test for
an OBO-only override.
* 🔊 fix: Mock MCP OAuth timeout in SDK integration test
MCPConnectionFactory.attemptToConnect reads mcpConfig.OAUTH_HANDLING_TIMEOUT
when building the OAuth connection timeout. The SDK OAuth integration test
mocked mcpConfig without that field, which made the timeout calculation produce
NaN and caused the test to fail before the OAuth refresh/start path completed.
Add OAUTH_HANDLING_TIMEOUT to the test mock.
* ♻️ refactor: Pass OBO upstream-token closure into MCP instead of req
Build the OpenID upstream-token provider at the request boundary and thread
only the closure through MCP handling, so the MCP service layer no longer
receives the raw Express request. The closure still reads/refreshes the live
session at tool-call time, preserving the walk-away recovery.
- Drop `req`/`capturedReq` from createMCPTools, createMCPTool, reconnectServer,
createToolInstance, and reinitMCPServer; forward `upstreamTokenProvider`
instead. Closure is constructed in loadTools, loadToolDefinitionsWrapper, and
the reinitialize route, where req/res are in scope.
- OBO: fall back to user.federatedTokens when the provider yields no live
session, so OIDC remote-agent calls (verified bearer, no session) still work.
- Inline refresh: mirror a rotated refresh token to the refreshToken cookie via
a shared setRefreshTokenCookie helper, guarded by !res.headersSent (no-op on
the streaming path; session copy stays authoritative).
- Single-flight: hydrate a joining request's own session from the resolved
tokens so a later OBO call doesn't replay a rotated-away refresh token.
Addresses owner feedback and three review findings.
* 🔒 fix: Recover OIDC refresh-token rotation after SSE OBO refresh
When an inline OBO refresh rotates the OpenID refresh token after SSE headers
have already been sent, the browser refreshToken cookie cannot be updated. Store
a short-lived encrypted bridge from the stale cookie token to the rotated token
so /api/auth/refresh can recover after express-session loss.
Use the signed openid_user_id cookie to load user context for bridge validation,
retry only on invalid_grant, and delete the bridge only after the bridged refresh
succeeds.
* 🔨 fix: hydrate joined OIDC refresh sessions with stable refresh tokens
Update single-flight OIDC refresh joiners whenever refreshed access token
state changes, even if the IdP keeps the refresh token unchanged.
This prevents joined requests from retaining stale accessToken or
accessTokenExpiresAt values and redundantly refreshing later in the same run.
* 🌉 Persist OIDC refresh-token recovery bridges in MongoDB
Store SSE OBO refresh-token recovery bridges in MongoDB instead of
process-local memory so /api/auth/refresh can recover after worker
restarts or cross-worker routing.
Derive bridge expiry from REFRESH_TOKEN_EXPIRY so the recovery window
matches the stale refreshToken cookie it repairs, and delete bridges
after successful recovery.
* 🤝 Coordinate OIDC inline refreshes across workers
Add a short-lived Mongo-backed refresh-flight record so concurrent
OBO refreshes for the same OpenID session do not redeem the same
rotating refresh token on different workers.
The winning worker performs the IdP refresh and stores an encrypted
result; joiners wait for that result, hydrate their request session,
and return without calling the IdP.
* ⚓ Keep OpenID marker cookies aligned on inline refresh
Refresh token_provider and openid_user_id with the same expiry as the
rotated refreshToken cookie when an inline OBO refresh can still write
headers.
Share the marker-cookie writer with the normal OpenID auth refresh path
so the fallback /api/auth/refresh branch continues to recognize valid
OpenID refresh tokens after session expiry.
* 🔑 fix: include refresh token in OIDC local refresh flight key
Key the process-local OIDC refresh coalescing by the current session
refresh token, matching the Mongo-backed flight key. This prevents a
request with a newly rotated token from joining an older pending refresh
and inheriting its failure/result.
* 🌉 fix: store OIDC refresh bridge without cookie response
Treat missing or non-cookie responses like headers-sent streaming
responses during inline OIDC refresh. When the IdP rotates the refresh
token and cookies cannot be written, persist a recovery bridge so a later
/auth/refresh can recover after session expiry.
* 🫙 fix: preserve stale OIDC cookie bridge key
Track the refresh token last written to the browser cookie separately
from the current session refresh token. When inline OIDC refreshes rotate
tokens without a writable response, keep bridging from the browser-stale
token directly to the latest session token.
* 🙌 fix: keep OIDC bridge recovery success on cleanup failure
Make refresh-token bridge cleanup best-effort after a bridged OIDC
refresh succeeds. A transient delete failure now logs a warning but does
not convert the already-refreshed session and cookies into a 403 response.
* 📦 test: Exclude RefreshTokenBridge from tenant-isolation coverage
Add RefreshTokenBridge to the tenant-isolation coverage allowlist because
refresh bridge lookups run during unauthenticated OpenID refresh recovery.
The controller first recovers user context from the signed OpenID marker
cookie, then the bridge methods apply explicit user and tenant filters.
Ambient tenant isolation would bind this recovery path to request-local
tenant context that is not available at the point the stale cookie is being
resolved
* ⚡ Fix OpenID refresh flight retry and marker hydration
Allow failed OpenID refresh flights to be reclaimed immediately instead of pinning transient errors.
Preserve the browser refresh-token marker when joined refreshes hydrate session tokens from a shared flight result.
Stabilize AuthService tests by isolating mocked module imports from prior suites.
* 🛠️ fix: centralize OBO identity scoping
Add shared auth identity helpers for app user ids, OpenID subjects,
tenant ids, and normalized OpenID issuers.
Thread a non-placeholder-visible OBO identity context from the real
request user through MCP connection, tool-call, reinit, and refresh
paths. Keep tenantId and openidIssuer out of createSafeUser so MCP
user placeholders do not expose those fields.
Scope OBO token cache and in-flight exchange keys by tenant, issuer,
OpenID subject, scopes, and a SHA-256 hash of the upstream assertion.
This prevents cross-tenant/cross-issuer collisions and avoids reusing
tokens minted from stale rotated assertions.
Use the shared identity helpers for OpenID refresh-flight keys and
refresh-token bridge recovery records so related OBO refresh paths share
the same identity normalization rules.
The helper is intended for auth-boundary and credential-cache code, not
as a blanket replacement for ordinary app user id ownership checks.
* 🛠️ fix: preserve OIDC refresh-token sync on save failures
Sync OpenID refresh-token cookie/bridge state before persisting the
session so a transient session-store failure cannot lose an IdP-rotated
refresh token.
Also trigger sync when the session refresh token differs from the
browser refresh-token marker, not only when the current grant rotates
the token. This lets later writable refreshes repair stale browser
cookies left behind by SSE refreshes.
Route refresh bridge identity through the shared identity helper with
the threaded OBO identity context, falling back to request/user context
when needed.
Add regression coverage for session-save failures, stale browser cookie
repair, non-writable bridge storage, and shared-helper identity fallback.
* 🛠️ fix: keep OIDC refresh bridge during recovery grace
After successful bridged refresh recovery, re-store the stale-cookie bridge
with a short grace TTL instead of deleting it immediately. This lets parallel
/api/auth/refresh requests that already sent the stale browser cookie recover
before they can observe the first response's Set-Cookie.
Retarget the bridge to the refresh token returned by the bridged retry so
B-to-C refresh-token rotation remains recoverable. The grace TTL is parsed with
math() and defaults to 60s, which shrinks the replay window from the original
REFRESH_TOKEN_EXPIRY bridge lifetime to the short recovery grace period.
Remove the now-unused explicit bridge delete path from the service and
data-schemas method surface. Add regression coverage for grace re-store,
identity symmetry, retry failure behavior, and same-key upsert replacement.
* 🛠️ fix: fail closed on OBO MCP user identity mismatch
Add an OBO-specific guard before MCP tool execution that requires the
effective invocation user and captured request user to both have ids and
to match. This prevents OBO tool calls from falling back to a separate
configurable.user_id identity after request-bound OBO context has already
been captured.
Keep the existing user id fallback behavior for non-OBO MCP calls.
Tests cover mismatched OBO users, missing user ids, and the matching-user
path ignoring a conflicting configurable.user_id.
* 🛠️ fix: Guard OpenID bridge retry user identity
Extract the shared OpenID refresh/user-resolution flow in AuthController
so the normal refresh path and bridge-recovery retry use the same grant,
claims, issuer, user lookup, and diagnostic logging code.
Preserve the existing path-specific behavior: the normal path still owns
migration updates and 401 login redirects, while the bridge retry still
falls through to the existing 403 invalid-token response.
Add a bridge-recovery guard that rejects retry results whose resolved
user id differs from the signed openid_user_id cookie before issuing
tokens or re-storing the grace bridge. Cover both the successful
matching-user recovery and the mismatched-user rejection.
* 🛠️ fix: type-safety polish on OBO data layer
Replace refresh token bridge query/update Record<string, unknown> usage
with typed Mongoose FilterQuery and UpdateQuery definitions.
Harden OpenID marker cookie JWT expiry handling by converting refresh
expiry milliseconds to integer seconds and rejecting invalid or
non-positive durations.
Add focused CSRF tests for fractional refresh expiry values and invalid
expiry configuration.
* 🛠️ fix: Bind OpenID session tokens to authenticated identity
Stamp OpenID session token state with the LibreChat user id, OpenID subject,
tenant id, and normalized issuer when tokens are stored.
Fail closed before OBO inline token reuse/refresh when the session token
identity does not match the current authenticated identity, preventing a stale
or mixed Express session from supplying another user's upstream assertion.
Also validate the normal /api/auth/refresh session-token reuse shortcut against
the signed marker-cookie user before returning cached session tokens.
Note: sessions created before this change carry no identity stamp and are
treated as a mismatch. This is self-healing — the reuse path forces a full IdP
refresh (which re-stamps the session) and the OBO path throws, surfacing as a
one-time re-authentication for active OBO users at deploy time. The session
re-stamps within one session lifetime (SESSION_EXPIRY, default 15 min).
* 🛠️ fix: Recover OpenID refresh token drift
Prefer the browser refresh-token cookie when it differs from the
server-side OpenID session state, and force a real IdP refresh in that
case instead of reusing stale session tokens.
Store a short-lived refresh-token bridge when inline OBO refresh writes
a rotated browser cookie but session persistence fails, so follow-up
refreshes can still recover from the old token.
Keep the bridge grace TTL centralized in RefreshTokenBridge so both
recovery paths use the same env-backed value.
Note: drift is measured against the last-synced browserRefreshToken
marker, so the SSE path (intentionally stale cookie, authoritative
session) does not false-positive. Sessions predating the marker have no
browserRefreshToken; for those, drift falls back to comparing the cookie
against the session refresh token and prefers the cookie on difference.
This is the same self-healing pre-change-session window as the identity
binding fix and re-syncs within one session lifetime.
Tests cover cookie/session drift selection, reusable-session bypass on
drift, bridge storage after session-save failure, and the shared bridge
constant wiring.
* 🛠️ fix: Harden OBO token caching and expiry handling
Reject malformed OBO grant responses before writing them to the exchanged-token cache so a missing access_token cannot poison the cache.
Store absolute expires_at values with cached OBO tokens and ignore legacy cache entries without usable expiry metadata. This keeps cached-token freshness based on the token’s real
remaining lifetime instead of reusing the original relative expires_in on cache hits.
Move OBO expiry normalization and skew helpers into packages/api and use them from both the JS exchange service and the TS MCP resolver. Apply a 30-second safety margin with a one-
second floor for short-lived tokens, covered by direct helper tests and caller-level regression tests.
Tests:
- packages/api: npm run build
- packages/api: npx jest src/mcp/oauth/expiry.spec.ts src/mcp/oauth/obo.spec.ts
- api: npx jest server/services/OboTokenService.spec.js
* 🛠️ fix: Harden OBO refresh-token bridge lookup and indexing
Reuse getValidOpenIDReuseUserId for the bridge-recovery user lookup in
refreshController instead of re-verifying openid_user_id inline. The shared
helper enforces the JWT_REFRESH_SECRET presence check and a strict
typeof payload.id === 'string' guard, rejecting tokens whose id claim is
present but not a string (e.g. a numeric id) that the inline check accepted.
Fail closed on issuer mismatch in getRefreshTokenBridge. Both the stored and
the expected issuer are now normalized and compared for equality, so a bridge
is recovered only when both sides agree (both absent, or both present and
equal after normalization). Previously the check was skipped whenever the
stored issuer was absent, allowing recovery across mismatched issuer context.
Drop the unused {oldRefreshTokenHash, userId, tenantId, openidIssuer} index
and the openidIssuer field on RefreshTokenBridgeQuery. The data-layer filter
only queries the 3-field {oldRefreshTokenHash, userId, tenantId} index; the
issuer is verified in application code, not the query. Hoist the repeated
model accessor into getRefreshTokenBridgeModel.
Note: issuer is now load-bearing for recovery. A bridge stored with an issuer
recovers only when the lookup supplies a matching issuer; the recovery lookup
reads user.openidIssuer via AUTH_REFRESH_USER_PROJECTION (an exclusion
projection that retains the field). If a user's persisted openidIssuer is
empty while the stored bridge has one, recovery fails closed (falls through to
normal re-authentication) until the bridge TTLs out — no security regression.
Tests cover invalid signed-cookie payloads bypassing the bridge, both
asymmetric issuer-presence cases, issuer normalization before comparison, and
an index-alignment assertion guarding against re-adding the dropped index.
* 🛠️ fix: Degrade OBO discovery on token resolution failures
Catch expected OboTokenResolutionError failures during MCP tool discovery and
fall back to unauthenticated tool listing instead of aborting discovery. This
keeps discovery aligned with the existing unauthenticated listing behavior while
preserving unexpected errors as real failures.
Also correct OBO tool-call freshness comment and tighten the OBO trust-check
permissions type to the existing role permission shape.
Tests:
- npx jest src/mcp/__tests__/MCPConnectionFactory.test.ts --runInBand --coverage=false
- npx jest src/mcp/oauth/obo.spec.ts --runInBand --coverage=false
* 🛠️ fix: tighten OBO tool-call errors, bridge logging, and flight typing
Move resolveToolCallUserId inside the tool-call try/catch so an OBO
identity mismatch surfaces with serverName/toolName context and the
standard tool-call-failed message instead of an opaque bare Error.
Raise the refresh-token bridge lookup failure log from debug to warn so
transient infrastructure failures on the unauthenticated /api/auth/refresh
path are observable, and guard the message access against non-Error values.
Replace the unknown+cast in isDuplicateKeyError with a hasErrorCode type
predicate so the duplicate-key check reads error.code without an assertion.
Preserve real math/isEnabled in the MCPConnectionFactory test mock (mock
only processMCPEnv) so mcpConfig timeouts no longer resolve to NaN, fixing
the TimeoutNaNWarning that masked slow OAuth retry behavior.
* 🧪 fix: Restore the Flight Uniqueness Index and Buffer the Graph Cache TTL
Two CI failures on the merge, both in suites this environment cannot run
(their MongoDB binary download is blocked).
`GraphApiService.spec.js` still asserted the unbuffered TTL. Graph tokens
route through the same `getTokenCacheTtlMs` as the OBO and openidStrategy
caches, so the entry now expires 30s before the credential does.
`openidRefreshFlight.spec.ts` dropped the database between tests, which takes
the indexes with it, and Mongoose builds them only once when the model is
compiled. Whether the unique `key` index survived into a test was a race with
that one-time build. Without it a second `create` inserts instead of raising a
duplicate-key error, so every worker believes it won the flight — the
mutual exclusion the file exists to prove. Indexes are now rebuilt after each
drop, which also makes the reclaim and complete cases reach those paths for
the right reason.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* fix: address OBO review findings
* 🔐 fix: Install Bridge Indexes and Carry OBO Through Assistant Recovery
Two findings from the Codex pass on d08c82f0d.
The refresh-token bridge relied on Mongoose auto-indexing for both of its
indexes, and `MONGO_AUTO_INDEX=false` is a supported deployment setting. A
bridge holds an encrypted refresh token and the TTL index is the only thing
that ever deletes one, so under that setting they would accumulate for the
life of the collection while concurrent upserts lost the compound uniqueness
the filter assumes. Installed before the first write, matching the flight
methods and the session and schedule methods before them.
`recoverServerTools`, the assistant create/update path that reruns
`reinitMCPServer` when a referenced server's catalog and connection snapshot
are both missing, was the last reinit site not carrying the upstream-token
closure. For an OBO server the factory rejects the connection outright, so the
assistant write failed with unavailable MCP definitions. It now builds the
provider at that request boundary like the other entry points; assistant
writes have no `res`, so a rotation there falls back to the recovery bridge.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* fix: harden OBO refresh coordination
* ⏳ fix: Keep Elapsed Expiries Elapsed and Revoke the Superseded Session
Two of the five findings from the Codex pass on fcdc15885 — the two that are
defects in code this branch introduced rather than design questions about the
bridge.
`getSkewedTokenExpiresAtMs` floored every result at a second in the future,
including an expiry the provider had already declared elapsed. An exchange
answering `expires_in: 0` or a past `expires_at` was handed to the MCP
connection stamped valid for another second, which only moves the failure
downstream. The floor now applies to a lifetime that is still live, which is
what it was for; an elapsed one stays elapsed so the caller rejects it. Same
for the cache TTL, which falls back to the elapsed-credential floor.
Bridge recovery left the stale token's durable Session behind. Only the token
it recovered through was passed as `existingRefreshToken`, so that one's
session was replaced while the token the browser actually presented kept its
record until its original expiry. That record, with the marker cookie still
bound to it, is what authorizes local image access for OpenID users — so a
copy of the stale cookie outlived the rotation it had lost. Revoked
explicitly on successful recovery.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* fix: close OBO refresh review findings
* 🎟️ fix: Carry the Bridged Token Through a Non-Rotating Recovery
Bridge recovery passes the browser's stale token as `existingRefreshToken` so
the durable Session naming it is the record replaced. That also makes the
stale token the fallback the installed session and the refresh cookie use when
a tokenset carries no `refresh_token` of its own, which holds only while the
recovery grant rotates.
An IdP that answers that grant without rotating sends the browser back to the
very token the bridge exists to retire: `storeOpenIDSession` installs and
deletes the same stale record in one call, and the cookie is rewritten to a
token the IdP already rejected — a sign-out on the next refresh. The grace
bridge one line above already guards this with `|| bridgedRefreshToken`; the
resolved tokenset now does the same, so leader and followers alike publish the
recovered token.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* 🚫 fix: Reject an OBO Exchange That Returns an Expired Credential
Preserving an elapsed expiry through the skew helper only helps if something
acts on it, and nothing did: `MCPManager.callTool` checks the access token and
nothing else before setting the Authorization header, so a credential the IdP
declared spent still went downstream to fail there. It is rejected at the
exchange now, where the reason is known, and retryably — the exchange itself
worked, so a fresh grant can succeed.
Completes the elapsed-expiry change in cb26a6f7d, which made the stamp honest
without giving anyone a reason to look at it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* fix: close OpenID refresh review findings
* fix: coordinate OpenID refresh entry points
* chore: sort OpenID flight imports
* fix: fence OpenID refreshes during logout
* fix: close OpenID logout publication races
* fix: narrow completed refresh flight
* test: cover bridge cleanup failure after ownership loss
The compensating delete in storeRefreshTokenBridgeWithLease swallows its own
failure so the lease error stays the one the caller sees. Nothing asserted
that, so removing the inner catch left every suite green while callers began
receiving the cleanup error instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* fix: compensate OpenID bridges only on proven ownership loss
The post-write lease assertion deletes the bridge it just published when it
throws, but it threw for two different reasons: a coordination record that is
no longer ours, and a coordination read that simply failed. Treating the second
as the first destroys the only mapping from the token the browser still holds
to the one the IdP already rotated to, so a transient Mongo error on the
headers-already-sent path signed the user out.
Tag the ownership error where the lease raises it and compensate only for that,
preserving the bridge whenever ownership is merely undetermined. A preserved
bridge stays behind the logout revocation fence, so the safe default costs
nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* fix: reject spent OpenID refresh results
Two ways a refresh could report success while handing back a credential
nothing can use.
An inline refresh carries the previous id_token forward when the IdP omits
one on rotation, so tokenset.id_token is not necessarily freshly issued.
setOpenIDAuthTokens applied its freshness guard only to the session copy and
took tokenset.id_token unconditionally, so /refresh returned an expired
bearer even though the grant produced a usable access token. Skip it only
when it is provably expired: an id_token whose expiry cannot be read stays
preferred, since access_token may be opaque or scoped to another audience.
normalizeExpiresIn preserves a zero or negative lifetime rather than
discarding it, so a grant declaring an already-spent access token still
published, rotating the refresh token and returning a token every freshness
check rejects. Each OBO call then repeated the grant. Reject an elapsed
lifetime before publishing; an unknown lifetime still publishes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* fix: harden OpenID refresh publication
* fix: keep identity and results intact through OpenID refresh cleanup
Two follow-ons from the last round's fixes.
Stripping an expired carried-forward id_token from the refresh result removed
the only identity material a rotation without id_token leaves behind. The
result is rebuilt by buildOIDCTokensFromSession, so it carries no provider
claims() either, and getTokenClaims accepts only those two — bridge recovery
failed with "no usable identity claims" before setOpenIDAuthTokens could hand
back the fresh access token. The stripped token now travels in a
non-enumerable marker, alongside the existing browser and predecessor markers,
which identity resolution reads and the authentication response never sees.
The lease drained a pending renewal by awaiting it in finally, so a transient
coordination failure there threw from finally and replaced the operation's
result. The refresh had already settled and published, so the caller saw a
failure on credentials that had rotated. Proven ownership loss is recorded on
ownershipLost and checked before the return, so the drain has nothing to add
but noise; it now absorbs and logs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F
* refactor: fence OpenID recovery publication
* fix: close OpenID publication transaction
* refactor: make OpenID publication transactional
* fix: satisfy OpenID publication type checks
* fix: fence OpenID session publication
* fix: authorize OpenID refresh publication
* fix: bind OpenID replay generations
* fix: fence OpenID response generations
* fix: authorize OpenID token delivery
* fix: linearize OpenID publication delivery
* chore: sort OpenID refresh flight imports
---------
Co-authored-by: J.C. Bartle <jcbartle@users.noreply.github.com>
Co-authored-by: jbartle <jbartle@rand.org>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: jcbartle <7274202+jcbartle@users.noreply.github.com>
* 🕰️ fix: Guard `expires_in` So a Token Response Cannot Outlive Its Credential
RFC 6749 §5.1 makes `expires_in` only RECOMMENDED, so a token response may legally omit it.
Four sites derived a lifetime from the raw field, where `undefined * 1000` is `NaN`.
`NaN` is not a short TTL, it is no TTL. `@keyv/redis` writes the key without `PX` because
`NaN` is falsy, so the entry is stored in Redis with no expiration at all; the in-memory
backend embeds `expires: NaN` and every check compares with `>`, always false against `NaN`.
The namespace default does not stand in either, since Keyv applies it with `??=` and `NaN` is
neither `null` nor `undefined`. The exchanged access token was therefore cached permanently at
`openidStrategy.js` and `GraphApiService.js`, and once it genuinely expired the poisoned entry
kept being served with no path to eviction.
The same omission is sharper in `ActionService.js`, where `new Date(NaN).toISOString()` throws
`RangeError: Invalid time value`. Both call sites are inside a `try`, so the failure surfaces as
a generic "Failed to authenticate OAuth tool" that names nothing, and the refresh site falls
through to `requestLogin()` on every attempt, looping with no exit.
The rule had six hand-written homes and three of them were wrong, so it now has one. A new
`packages/api/src/oauth/expiry.ts` normalizes `expires_in` to a positive finite number of
seconds or nothing, and exposes the two shapes callers actually need: a cache TTL that falls
back rather than returning `NaN`, and an absolute expiry that is absent rather than Invalid.
The four unguarded sites adopt it, and the two ad-hoc guards in `openidStrategy.js` and
`OboTokenService.js` are consolidated onto it.
`createHandleOAuthToken` is folded in as well. Its guard already handled `null` and unparseable
strings but admitted `NaN`, since `typeof NaN === 'number'` satisfied its first branch.
The `mcp/oauth` sites are deliberately left alone: `tokens.ts` guards on truthiness and carries
richer logic that reads a JWT access token's own expiry when the response omits one, and the
file is being reworked in #13901.
Closes#15318Closes#15319
* 🕰️ fix: Address `expires_in` Guard Review Round 1
Preserve an explicitly elapsed lifetime instead of collapsing it into "unknown". `expires_in: 0`
is the provider stating the credential is already dead, which is information; treating it as
absent handed it the one-hour fallback in `createHandleOAuthToken` and dropped the expiry
entirely in `ActionService`, so a credential declared expired could be used and retained for up
to an hour. Both sites preserved that value before this branch, so the collapse was a regression
introduced here.
`normalizeExpiresIn` now returns any finite number, positive or not, and reports `undefined` only
for a lifetime that is genuinely unusable. `getTokenExpiresAt` therefore yields a past timestamp
for an elapsed lifetime, so callers refresh rather than guess.
Cache TTLs cannot pass such a value through raw: Keyv reads a TTL of exactly `0` as "no expiry",
turning a dead credential into the immortal entry this module exists to prevent. `getTokenCacheTtlMs`
floors an elapsed lifetime at one millisecond, which expires immediately without ever writing an
entry that outlives its credential.
Parse numeric strings with `Number` rather than `parseInt`, which truncates a complete value such
as `"3.6e3"` to `3` and would expire an hour-long credential after three seconds, re-exchanging
against the identity provider on every request. An empty or blank string is rejected rather than
read as zero, since `Number('')` is `0`.
* 🕰️ fix: Bound `expires_in` to Lifetimes a Date Can Represent
Parsing the complete numeric string last round made an overflow reachable that `parseInt` had
been masking. `parseInt('1e13', 10)` was `1`; `Number('1e13')` is `1e13`, and `1e13` seconds is
1e16 ms, past the ECMAScript time value range of ±8.64e15. Every derived timestamp was therefore
an Invalid Date whose `toISOString()` throws `RangeError: Invalid time value` — the exact failure
this branch exists to remove, reintroduced by its own fix. The token model derives the same way
at `packages/data-schemas/src/methods/token.ts:19`, so storage and authentication would fail with it.
A lifetime is now reported as unusable unless it can still produce a valid `Date`. The bound is
the time value range halved, leaving room for the `Date.now()` every derived timestamp adds. At
roughly 137,000 years it rejects nothing a provider could mean: a one-year refresh token and even
a hundred-year lifetime still pass through untouched, while `1e13`, `Number.MAX_SAFE_INTEGER` and
`1e300` take the caller's fallback instead of poisoning a timestamp.
The invariant tests now carry the overflow shapes rather than a fixed list of small ones, since a
guard that only sees the inputs its author imagined is how the previous round's regression got in.
The openidJwt reuse strategy substituted the raw incoming Authorization bearer whenever it
found no stored access token in the session or cookies. Clearing this strategy's audience
check does not make a token an access token: an OIDC ID token is minted for the client id
and satisfies the very same check, so the fallback could store an ID token as
federatedTokens.access_token. That value is used verbatim as the On-Behalf-Of assertion, and
Entra rejects an ID token there with AADSTS240002. It is easy to reach whenever the OpenID
session store is not persistent, since express-session falls back to MemoryStore and every
restart wipes the stored access tokens for active sessions.
Deleting the fallback outright would break the case it legitimately serves, so reuse is now
gated on the bearer being identifiable as an access token. Detection rests on the two signals
that separate the token types by specification rather than by provider convention: an RFC 9068
`at+jwt` header type, defined for access tokens alone, and an `aud` that omits the OIDC client
id, which OIDC Core section 2 requires every ID token to carry. A scp/scope claim is only a
supporting signal, applying once the audience has already ruled out an ID token, because
providers add claims freely in both directions -- Keycloak has emitted nonce and auth_time in
genuine access tokens and maps scope into its ID tokens. at_hash and c_hash veto regardless,
since they exist only to bind an ID token to its companion access token or code.
An unrecognised token is left unset rather than guessed at, so isOpenIDTokenValid fails closed
and the OBO path raises its actionable error instead of attempting a Graph exchange with a
token of the wrong type. Reuse now requires either an at+jwt provider or OPENID_AUDIENCE
naming a resource distinct from the client id; a bearer audienced only to the client id cannot
be told apart from an ID token and fails closed. That applies solely on the degraded path where
nothing was stored to begin with.
Separately, processSingleValue gated every credential placeholder on isOpenIDTokenValid, which
reports on the access token alone, so leaving access_token unset made a header configured with
only {{LIBRECHAT_OPENID_ID_TOKEN}} raise re-authentication despite a present, current ID token.
Validity now depends on the credential actually requested: an access-token-specific pattern
gates the raise, and the ID token resolves through processOpenIDPlaceholders, which already
validates its own expiry. processOpenIDPlaceholders takes a fields allowlist so that path
resolves the ID token alone and identity metadata keeps its literal-then-strip behaviour.
* 🤖 feat: make event actor HITL durable
* 🤖 fix: break event actor outcome cycle
* 🤖 fix: close durable actor terminal races
* fix: harden durable event actor recovery proofs
* fix: close event actor resume publication races
