Manual e2e on PR #12934: slides rendered correctly but didn't fill the
artifact panel — whitespace on either side. Two issues:
1. The scale was capped at `Math.min(1, available / SLIDE_W)`. On
panels wider than 960px, the cap clamped the scale to 1.0 and
slides rendered at native size with whitespace on the sides
instead of stretching.
2. The scale was computed against the constant `SLIDE_W = 960`, but
pptx-preview can emit slides whose `offsetWidth` differs from the
init param if the source PPTX has a non-16:9 layout. Per-slide
division of `available / nativeW` handles that case.
Fix: replace `computeScale()` with two helpers — `availableWidth()`
returns the panel content-box width and `scaleFor(nativeW)` returns
the per-slide scale. No upscale cap. The slide content is rendered
by pptx-preview against its 960×540 canvas using vector text /
canvas — scaling up to e.g. 1500px doesn't visibly degrade quality.
Tests: regression now also asserts:
- `availableWidth()` and `scaleFor()` exist by name
- The exact scale formula `availableWidth() / (nativeW || SLIDE_W)`
- Negative assertion that `Math.min(1, ...)` is NOT present, so a
future "let's add an upscale cap" rewrite can't silently
re-introduce the whitespace.
Manual e2e on PR #12934: regenerated PPTX showed "Preview unavailable"
in the iframe. Root cause: the MutationObserver I added in the
previous commit fired during pptx-preview's render and moved slides
out from under the library's references. pptx-preview's async
pipeline raised an unhandled rejection, the iframe's window-level
listener caught it, and the fallback message replaced the partial
render.
Fix: drop the MutationObserver. Apply the wrap+scale ONCE in a
`finalize` step that runs:
- On `previewer.preview().then` (the happy path)
- On the 8-second timeout safety net IF the container has children
(silent-failure path — pptx-preview emitted slides but never
resolved its outer promise)
To prevent the user from seeing an unscaled flash while pptx-preview
renders into the 960px-wide canvas, the container is set to
`visibility: hidden` at init and only revealed inside `finalize`
after wrap+scale completes.
Resize handling stays via `ResizeObserver` on `document.body`,
installed AFTER the wrap pass so it doesn't fire during the wrap
itself.
Tests: regression assertion now also locks in:
- `container.style.visibility = 'hidden' / 'visible'` (the flash-
prevention contract)
- Absence of MutationObserver (the bug we just removed — must NOT
creep back in via a future "let's scale during streaming" idea)
Manual e2e on PR #12934: pptx-preview rendered slides at their native
init dimensions (960×540 default). The artifact panel is much narrower
than that, so the iframe got a horizontal scrollbar and only a corner
of each slide showed at any time — the user had to drag-scroll across
each slide to read it.
Fix: keep pptx-preview's init at 960×540 so its internal layout math
stays correct, then post-process each rendered slide:
- Cache the slide's native width/height on its dataset BEFORE
applying any transform (so subsequent re-fits don't measure the
already-transformed box).
- Wrap the slide in `.lc-slide-wrap` with explicit width/height set
inline to the scaled dimensions; the wrap shrinks the layout space
the slide occupies.
- Apply `transform: scale(panel_width / 960)` to the slide itself
with `transform-origin: top left` so the rendered output shrinks
from the top-left corner into the wrap.
- Cap the scale at 1.0 so small slides don't upscale and get blurry.
Streaming + resize:
- `MutationObserver` watches the container for slide insertions so
streaming renders get scaled on arrival rather than waiting for
the entire `previewer.preview` promise to settle.
- `ResizeObserver` re-fits all wrapped slides when the iframe
resizes (panel drag, window resize).
Tests: new "bootstrap wraps + scales each slide" lock in the wrap
class, scale computation, observer setup, and native-size caching
so a future refactor can't silently re-introduce the overflow.
Two regressions surfaced after the textFormat security gate landed.
1. **Client** (`LogContent.test.tsx` "falls back to the legacy download
branch for an office file with no extracted text"):
When the security gate downgraded an office type without
`textFormat: 'html'` to PLAIN_TEXT, the lenient empty-text gate on
PLAIN_TEXT then accepted a missing `text` field and rendered a
half-empty panel card. The historical contract is "office type +
no text → legacy download UI"; the downgrade should only fire when
there's actual plain text that needs safe-escaping.
Fix in `detectArtifactTypeFromFile`: short-circuit to null when the
office type lands in the security-gate branch with no text. The
PLAIN_TEXT downgrade still fires for legacy attachments that DO
carry plain text.
2. **API** (`process.spec.js` + `process-traversal.spec.js`): the
`@librechat/api` mocks didn't expose `getExtractedTextFormat`, so
`processCodeOutput` called `undefined(...)` → TypeError → tests got
undefined results. Added the helper to both mocks with a faithful
default (returns 'text' for non-null extractor output, null
otherwise).
Tests: new regression in `artifacts.test.ts` pinning the empty-text
+ no-textFormat → null contract for all four office types
(.docx/.csv/.xlsx/.pptx), so a future refactor can't silently
re-introduce the half-empty card.
Per the plan-mode discussion: prove out a LibreOffice subprocess
pipeline as an alternative to the docx-preview / pptx-preview CDN
renderers. LibreOffice handles every office format Microsoft and
LibreOffice itself can open (DOCX, PPTX, XLSX, ODT, ODP, ODS, RTF,
many more), produces a PDF, and the host browser's built-in PDF
viewer renders it inside the Sandpack iframe via a `data:` URI.
No client-side JS dependency, no CDN dependency, true high
fidelity for any feature LibreOffice supports.
Off by default. Operators opt in by setting both:
- `OFFICE_PREVIEW_LIBREOFFICE=true`
- LibreOffice (`soffice` or `libreoffice`) on the server's `$PATH`
When either is missing, the dispatcher falls through to the
existing CDN/mammoth/slide-list pipeline so a misconfiguration
doesn't break previews.
Hardening (`packages/api/src/files/documents/libreoffice.ts`):
- Fresh subprocess per call with isolated temp dir, stripped env
(PATH/HOME/TMPDIR only), and `-env:UserInstallation` so concurrent
conversions can't collide on shared `~/.config/libreoffice` locks
- 30-second wall-time cap; SIGKILL on timeout
- 50 MB PDF output cap to bound disk pressure
- 512 KB output cap on the wrapped HTML so the SSE/cache contract
stays intact (base64 inflates ~33%, effective PDF cap ~380 KB)
- Macros disabled by default flags (`--norestore --invisible
--nodefault --nofirststartwizard --nolockcheck`)
- Tag-distinct `LibreOfficeUnavailableError` /
`LibreOfficeConversionError` so callers can swallow appropriately
Iframe wrapper (`buildPdfEmbedDocument`):
- Native browser PDF viewer via `<iframe src="data:application/pdf;
base64,...">` — works in Chrome, Edge, Safari, Firefox
- CSP locks the iframe to `default-src 'none'; frame-src data:;
connect-src 'none'; script-src 'unsafe-inline'` — no outbound
network, no eval, no external scripts
- `#view=FitH` for first-paint sizing
- 4-second heuristic timer that swaps to a "Preview unavailable"
fallback when the browser's PDF viewer is disabled (kiosk mode,
Brave Shields, etc.)
Wired into `wordDocToHtml` and `pptxToHtml` as the first branch —
returns null when disabled / unavailable / oversized so the existing
pipeline takes over. XLSX intentionally NOT routed through this
path: SheetJS's HTML output is already excellent for spreadsheets
(sortable, sticky headers) and PDF rendering of sheets is awkward.
Tests (`libreoffice.spec.ts`, 30 cases — 25 always run, 5 conditional
on the binary): env-gating parser semantics matching
`OFFICE_PREVIEW_DISABLE_CDN`, fallthrough contract (never throws,
returns null on any failure), CSP lock-down, fallback structure,
binary probe caching + missing-binary path, error tagging, and
integration tests that engage when `soffice`/`libreoffice` is on
PATH (DOCX→PDF, PPTX→PDF, output-cap fallthrough). Integration
tests skip cleanly on bare CI.
Codex P2 review on PR #12934: the CDN-rendered DOCX path always pulled
docx-preview + jszip from cdn.jsdelivr.net. Air-gapped or corporate-
filtered networks where jsdelivr is blocked would degrade to a static
"Preview unavailable" message even though the server already had a
local mammoth renderer that could produce readable output.
Now the dispatcher renders mammoth first and embeds the sanitized
output inside the CDN document as a hidden `#lc-fallback` block. The
iframe's existing `typeof docx === 'undefined'` check (which fires
when the CDN scripts can't load) un-hides the fallback so the user
sees a real preview. CDN-success path is unchanged: high-fidelity
docx-preview output owns the viewport, mammoth fallback stays hidden.
Two new safeguards in the dispatcher:
- Size budget: if base64(binary) + mammoth body + wrapper > 512 KB
(the `attachment.text` cache cap), drop to mammoth-only so a giant
document still renders. The `OFFICE_HTML_OUTPUT_CAP` constant
mirrors `MAX_TEXT_CACHE_BYTES` from extract.ts (separate constant
to avoid a circular import; pinned by a unit test).
- `lc-render` is hidden when fallback shows so the empty padded slot
doesn't sit above the mammoth content.
Tests: existing CDN-path tests updated for the new
`wordDocToHtmlViaCdn(buffer, mammothBody)` signature; new test for
the embedded fallback structure (`#lc-fallback`, mammoth body
content, "High-fidelity renderer unavailable" notice, render-slot
hide); new constant pin and per-fixture cap-respect assertion.
Codex P1 review on PR #12934: routing .docx/.csv/.xlsx/.xls/.ods/.pptx
into the office preview buckets assumed `attachment.text` was already
sanitized full-document HTML, but that guarantee only existed for the
new code-output extractor path. Existing stored attachments and other
non-code paths can still carry plain extracted text — `useArtifactProps`
would then inject that as `index.html` inside the Sandpack iframe.
Adds a `textFormat: 'html' | 'text' | null` trust flag persisted on
the file record by the code-output extractor, surfaced over the SSE
attachment payload and the TFile API type. The client's routing in
`detectArtifactTypeFromFile` requires `textFormat === 'html'` before
landing on an office HTML bucket; everything else (legacy attachments,
RAG-extracted plain text from `parseDocument`, explicitly-marked
'text' entries) falls back to the PLAIN_TEXT bucket where the
markdown viewer escapes content rather than executing it.
Tests: new `getExtractedTextFormat` helper has 14 cases covering all
office paths, legacy XLS MIME aliases, parseDocument fallthroughs,
and null-input. Client `artifacts.test.ts` adds three security-gate
tests proving downgrade behavior for missing/null/'text' textFormat,
plus a `fileToArtifact` test that legacy office attachments without
the flag end up in PLAIN_TEXT with their content escaped.
Manual e2e of the PPTX CDN renderer surfaced a black screen with
"Could not establish connection. Receiving end does not exist."
unhandled-rejection — characteristic of a Web Worker that couldn't
start.
Root cause: pptx-preview's bundled echarts dep spins up Web Workers
via blob: URLs for chart rendering. Our CSP had `default-src 'none'`
and no `worker-src`, so workers fell back to default → blocked. The
async failure deep inside echarts didn't surface through the outer
`previewer.preview()` promise, so my bootstrap's `.catch` never fired,
the loading state was removed, and the iframe sat with the body
background showing through (dark navy in dark mode = "black screen").
Three changes:
- Add `worker-src blob:` to the PPTX CSP. Allows blob:-only worker
creation without permitting arbitrary worker URLs.
- Bootstrap: window-level `unhandledrejection` and `error` listeners
so rejections from inside bundled-dep async pipelines surface as
the user-facing "Preview unavailable" fallback instead of going
silent.
- Bootstrap: 8-second timeout that checks `container.children.length`
— if the renderer hasn't appended anything visible by then, assume
silent failure and show the fallback.
Also wipe `container.innerHTML` when showing the fallback so a partial
render doesn't compete with the message.
DOCX wrapper unchanged: docx-preview doesn't use workers, so the
worker-src directive doesn't apply, and the existing fallback path
already covers its failure modes.
Tests
-----
- Existing PPTX CSP test now also asserts `worker-src blob:` is present.
- Existing fallback-message test extended to cover the new
unhandledrejection/error/timeout listeners.
packages/api/src/files: 467/467 ✅.
docx-preview defaults to rendering at the document's native page
width (8.5in for letter, 21cm for A4). In a wide artifact panel
that left whitespace on either side; in a narrow one it forced
horizontal scroll.
Two changes:
- Pass `ignoreWidth: true` to `docx.renderAsync` so the library skips
the document's pageSize width and uses its container's width.
- Defensive CSS overrides on `.docx-wrapper` and `.docx-wrapper > section.docx`
in case a future library version regresses on the option, plus
`padding: 0` on the wrapper to drop the page-edge whitespace
docx-preview otherwise reserves.
`renderHeaders`/`renderFooters`/etc. stay enabled — those still
appear in the rendered output, just inside a container that fills
the panel instead of a fixed-width "page."
Tests unchanged (100/100); manual e2e ahead of merge.
Mirrors the DOCX CDN architecture for PPTX: small files (≤350 KB
binary) embed as base64 and render via `pptx-preview` loaded from
jsdelivr inside the Sandpack iframe. Larger files and air-gapped
deployments fall back to the existing slide-list extraction.
Why
---
PPTX is the format where the gap between LibreChat's preview and
Claude.ai-style previews was most visible (slide-list of bullet
points vs. rendered slide layouts). LibreOffice → PDF → PDF.js is
still the eventual gold-standard answer for PPTX fidelity, but
client-side rendering inside the Sandpack iframe gets us a
meaningful intermediate step (~1.5/5 → ~3.5/5) without a sidecar.
What
----
- `pptx-preview@1.0.7` (ISC license, ~1.36 MB UMD bundle that
includes its echarts/lodash/uuid/jszip/tslib deps inline). Pinned
to a specific version on jsdelivr with SHA-384 SRI and
`crossorigin="anonymous"`.
- `buildPptxCdnDocument` mirrors the DOCX wrapper: same CSP locks
(`default-src 'none'`, `connect-src 'none'`, no eval, no base/form
tampering), same `id="lc-doc-data"` base64 slot, same fallback
message wiring (`typeof pptxPreview === 'undefined'` →
"Preview unavailable").
- New public `pptxToHtml(buffer)` dispatcher; `bufferToOfficeHtml`
switches its `'pptx'` case to call it. `pptxToSlideListHtml` stays
exported as the slide-list-only path (still hit by tests directly
and by the dispatcher fallback).
- `OFFICE_PREVIEW_DISABLE_CDN=true` env-var hatch applies to PPTX
too — air-gapped operators get the slide-list path. Same env-var
read at call time, same matrix of truthy values (`1` / `true` /
`yes` / case-insensitive / whitespace-trimmed).
- `_internal` re-exports moved to after the PPTX section since the
PPTX internals live further down in the file. Adds
`pptxToHtmlViaCdn`, `MAX_PPTX_CDN_BINARY_BYTES`,
`PPTX_PREVIEW_CDN`.
Honest caveats
--------------
- The 1.36 MB UMD bundle has `require("stream"/"events"/"buffer"/
"util")` references in its outer wrapper. Those are bundled-dep
artifacts (likely from `tslib` / Node-shim transforms) and don't
appear to execute on the browser code paths, but I haven't done
manual e2e on a wide range of decks. If a class of files turns up
that breaks rendering, the iframe-side fallback message catches it
and operators have `OFFICE_PREVIEW_DISABLE_CDN=true` as the bail.
- First-render CDN fetch is ~1.36 MB (browser-cached after).
- PPTX with embedded media easily exceeds the 350 KB binary cap;
those files take the slide-list path. Lifting the cap is a
follow-up (tied to the broader self-hosting work).
Tests
-----
11 new in two new describe blocks:
- `pptxToHtml dispatcher`: routing predicate (small → CDN, env-set
→ slide-list).
- `CDN-rendered path`: base64 round-trip, SRI integrity +
crossorigin, CSP locks (connect/eval/base/form), fallback message,
size-threshold lock at 350 KB.
- `OFFICE_PREVIEW_DISABLE_CDN escape hatch`: env-var matrix for
truthy values.
packages/api/src/files: 457/457 ✅ (was 446, +11).
Two follow-up review findings on PR #12934, both real.
P1 — Spreadsheet MIME aliases on client
----------------------------------------
Backend's `officeHtmlBucket` uses the broad `excelMimeTypes` regex from
`librechat-data-provider` (covers `application/x-ms-excel`,
`application/x-msexcel`, `application/msexcel`, `application/x-excel`,
`application/x-dos_ms_excel`, `application/xls`, `application/x-xls`,
plus the canonical sheet MIMEs). The client's exact-match
`MIME_TO_TOOL_ARTIFACT_TYPE` only had three of those, so an
extensionless XLS upload with a legacy MIME would have backend HTML
produced but the client would fail to route the artifact at all —
preview chip never registers.
Fix: import the same regex on the client and add it as a fallback in
`detectArtifactTypeFromFile` after the exact-match map miss. Stays in
lock-step with the backend automatically.
7 new test cases — one per legacy alias.
P2 — Hard CDN dependency on jsdelivr
-------------------------------------
Air-gapped / corporate-filtered networks where jsdelivr is unreachable
would see DOCX previews permanently degrade to "Preview unavailable"
because the iframe could never load the renderer scripts. Mammoth was
sitting right there on the server but the dispatcher always preferred
the CDN path for files under 350 KB.
Fix: `OFFICE_PREVIEW_DISABLE_CDN` env var. When truthy (`1`, `true`,
`yes`, case-insensitive, whitespace-trimmed), `wordDocToHtml`
short-circuits to the mammoth path regardless of file size. Operators
on filtered networks set the env var; default behavior is unchanged.
Read at function-call time (not module load) so jest can flip it in
`beforeEach` without `jest.resetModules()`. The cost is one property
access per render.
12 new test cases: env-unset uses CDN (default), all five truthy
forms force mammoth, five non-truthy forms (`false`/`0`/`no`/empty/
arbitrary string) leave CDN active.
Tests
-----
packages/api/src/files: 446/446 ✅ (was 434, +12 from env-var matrix).
client artifact suites: 235/235 ✅ (was 228, +7 from MIME aliases).
Switch the default DOCX render path from server-side mammoth → flat
HTML to client-side `docx-preview` loaded inside the Sandpack iframe.
Mammoth becomes the fallback for files above the cap.
Why
---
The Sandpack iframe is a real browser DOM. Server-side rendering
ceiling for DOCX→HTML is well below the source's visual fidelity —
mammoth strips cell shading, run colors, banners, and column layouts
because Word's layout model doesn't fit HTML's flow model. Pushing the
render into the iframe lifts that ceiling without paying the
server-side cost of jsdom or LibreOffice.
What
----
- New `wordDocToHtmlViaCdn(buffer)` builds a self-contained HTML doc
that embeds the binary as base64 and lets `docx-preview@0.3.7`
render it on load. CSS preserves dark/light mode handoff via
`prefers-color-scheme`. Bootstrap script falls back to a "preview
unavailable, please download" message if the CDN is unreachable or
the parse throws.
- `docx-preview` and its `jszip` peer dep are pinned to specific
versions on jsdelivr with SRI sha384 integrity hashes and
`crossorigin="anonymous"`. Refresh: re-fetch the file, run
`openssl dgst -sha384 -binary FILE | openssl base64 -A`.
- CSP locked down on the iframe: `default-src 'none'`, scripts only
from jsdelivr (no eval), `connect-src 'none'` so a parser bug in
docx-preview can't be turned into exfiltration of the embedded
document, `base-uri 'none'`, `form-action 'none'`. Defense in depth
on top of the Sandpack cross-origin sandbox.
- `wordDocToHtml` dispatches by size: ≤ 350 KB binary → CDN path
(high fidelity), larger → mammoth fallback (preserves the size cap
on `attachment.text`). 350 KB chosen so worst-case base64-inflated
output (~478 KB) plus wrapper overhead (~5 KB) fits under
MAX_TEXT_CACHE_BYTES (512 KB) with 40 KB headroom.
- Internal renderers exported as `_internal` for tests. Public API
unchanged — callers still go through `wordDocToHtml`.
PPTX intentionally NOT switched
-------------------------------
Surveyed the available client-side PPTX libraries:
- `pptx-preview@1.0.7` ships an ESM-only main entry plus a 1.36 MB
UMD that references `require("stream"/"events"/"buffer"/"util")` —
bundled for Node, not browser-clean. Could work but the runtime
references to undefined Node globals are a fragility risk worth
more validation than this PR can absorb.
- `pptxjs` is jQuery-era, requires four separate UMD scripts in a
specific order, less actively maintained.
- The honest answer for PPTX is the LibreOffice sidecar (DOCX/XLSX/
PPTX → PDF → PDF.js), which is the architecture every major
product (Google Drive, Claude.ai, ChatGPT) effectively uses and
the only path to ~5/5 fidelity for arbitrary user decks.
PPTX stays on the existing slide-list extraction for now. Open a
follow-up issue for the LibreOffice/Gotenberg sidecar.
Tests
-----
- 6 new in CDN-rendered describe block: wrapper structure, base64
round-trip, SRI integrity + crossorigin, CSP locks
(connect-src/eval/base-uri/form-action), fallback message wiring,
size-threshold lock.
- Adjusted 2 existing tests that asserted on mammoth-path artifacts
(literal document text in `<article class="lc-docx">`) — those
assertions move to the mammoth-fallback test that calls
`_internal.wordDocToHtmlViaMammoth` directly. Dispatcher tests now
assert CDN-path signatures instead.
packages/api files: 434/434 ✅, full unit suite 4473/4473 ✅.
Two UX improvements based on hands-on use of the office preview pipeline.
DOCX rendering — mammoth strips the navy banners, cell shading, and
column layouts that direct-formatted docs apply (python-docx-style
output is a common case). The flat `<p><strong>X</strong></p>` and
bare `<table><tr><td>` it emits looks washed out next to the source.
Three targeted compensations:
- Style map promotes `Title`, `Subtitle`, `Heading 1` thru `Heading 6`,
and `Quote` paragraphs to their semantic HTML equivalents (mammoth's
default only handles Heading 1-6, missing Title/Subtitle/Quote).
- Extra CSS scoped to `.lc-docx` gives the first table row sticky-
looking header styling regardless of `<thead>` (mammoth never emits
`<thead>`), adds zebra striping, and treats the python-docx
`<p><strong>X</strong></p>` section-heading idiom as a pseudo-h2 with
a thin accent left border so document structure survives the round
trip. Headings get a left accent or underline so they read as
headings instead of just bold paragraphs.
- Sanitizer's `allowedAttributes` opens `class` on the heading and
block tags the styleMap and CSS heuristics rely on. `<script>`,
event handlers, javascript: URLs, etc. are still stripped — the
existing security regression suite catches any drift.
Panel header — `Artifacts.tsx` showed a generic "Preview" pill for
preview-only artifacts. Single-tab Radio is a no-op; surfacing the
document filename there gives the user something useful in the chrome
without taking real estate. `displayFilename` handles the sandbox
dotfile suffix the upload pipeline applies.
Tests: html.spec.ts +1 (new CSS-emission lock), 71/71. Backend files
suite 428/428. Client 308/308.
Follow-up review nit on PR #12934. Jest's `it.each` substitutes `%s`
positionally, and the table rows were `[type, expected]` while the
description template read `'returns %s for type %s'` — outputting
"returns application/vnd.librechat.docx-preview for type true"
instead of the intended "type ... returns true".
Reorder the template to match the column order. Test runner output
now reads naturally: "type application/vnd.librechat.docx-preview
returns true". Pure cosmetic — runtime behavior unchanged.
Wraps up the 10-finding follow-up review. Two MAJOR + four MINOR + two
NIT addressed; one NIT skipped after verifying it was a misread of the
package.json structure.
MAJOR
- #1: Rewrite `renderOfficeHtml` JSDoc to document the HTML-or-null
contract explicitly. The pre-fix doc described a text-fallback path
that was the original XSS vector (commit b06f08a). A future
maintainer trusting the stale doc could reintroduce the fallback.
- #2: Replace byte-truncation of office HTML with a small "preview too
large" banner document. Cutting at a UTF-8 boundary lands mid-tag
(`<table><tr><td>con\n…[truncated]`) and ships malformed markup to
the iframe — unpredictable rendering, occasional broken layouts on
DOCX with embedded images / wide spreadsheets.
MINOR
- #4: Wrap `readSlidesFromZip`'s `zipfile.close()` in try/catch so a
close-time exception (mid-flight stream) doesn't replace the
original error. Mirrors the defensive pattern in zipSafety.ts.
- #5: Refactor PPTX extraction to use `yauzl.fromBuffer` directly,
eliminating the temp-file write/unlink the safety pre-flight already
proved unnecessary. Removes 4 unused imports (os, path, fs/promises,
randomUUID).
- #6: Extract `isPreviewOnlyArtifact(type)` to `client/src/utils/
artifacts.ts` so the membership check is unit-testable without
mounting the full Artifacts component (Recoil + Sandpack + media
query). 15 new test cases covering positive types, negative types,
null/undefined, and unknown strings.
NIT
- #3: Remove dead `stripColorStyles` / `COLOR_PROPERTY_PATTERN` —
unused (sanitizer's `allowedStyles` config handles color implicitly).
- #7: Remove dead `!_lc_csv_label` worksheet property write.
- #9: Remove no-op `exclusiveFilter: () => false` sanitize-html config.
- #10: Type-narrow `PREVIEW_ONLY_ARTIFACT_TYPES` to
`ReadonlySet<ToolArtifactType>` so the membership table is
compile-time checked against the enum.
SKIPPED
- #8: Reviewer flagged `sanitize-html` as duplicated in devDeps and
dependencies. The package has no `dependencies` section — only
`devDependencies` and `peerDependencies`. Existing convention
(mammoth, xlsx, yauzl, pdfjs-dist) is to appear in BOTH. Removing
the devDep entry would break local test runs.
Tests: packages/api 4406/4406, client artifacts 128/128.
Codex P1 review on #12934 caught that when `renderOfficeHtml` failed
(timeout, malformed file, zip-bomb rejection) for an office type, the
extractor fell through to `extractDocument` and returned plain text.
The client routes by extension/MIME to the office preview buckets and
feeds `attachment.text` straight into the Sandpack iframe's
`index.html`. A spreadsheet cell or document body containing the
literal string `<script>alert(1)</script>` would have been injected
as executable markup — direct XSS.
The contract for office types is now HTML-or-null with no text
fallback. Failed render returns null, the client's empty-text gate
keeps the artifact off the panel, and the file falls back to the
regular download UI (matching what PPTX already did). PDF and ODT
still go through `extractDocument` because the client routes them to
PLAIN_TEXT (which the markdown viewer escapes) or no artifact at all,
so plain text is safe there.
Test reshuffle:
- `document` describe block now uses ODT/PDF for the legacy
parseDocument-path tests (DOCX/XLSX/XLS/ODS bypass that path).
- New "does NOT call parseDocument for office HTML types" test locks
in the SEC contract for all four office HTML buckets.
- "falls back to ..." tests rewritten as "returns null when ..." with
explicit `parseDocumentCalls.length === 0` assertions to prove no
text leaks back to the client.
- New XSS regression test for the XLSX failure path.
- Mock parseDocument failure-name match relaxed to `includes()` so
ODT-named tests can use the same trigger.
Codex review on #12934 caught that the early `category === 'other'`
return short-circuited before `hasOfficeHtmlPath` was checked. The
classifier returns 'other' for inputs the new dispatcher can still
route — extensionless `application/csv` (CSV MIMEs aren't in the
classifier's text-MIME set and don't start with `text/`), and
extensionless office MIMEs with parameters like `application/vnd...
spreadsheetml.sheet; charset=binary` (the classifier's `isDocumentMime`
exact-matches these MIMEs without parameter normalization). Both would
route correctly through `officeHtmlBucket` but never reached it.
Move the office-HTML attempt above the 'other' early return, and drop
the `|| category === 'document' || category === 'pptx'` shortcut now
that `hasOfficeHtmlPath` covers the same surface (with parameter
normalization) and a wider one. ODT still routes through `extractDocument`
unchanged — `hasOfficeHtmlPath` returns false for it and the
`category === 'document'` branch below handles it.
Adds 3 regression tests:
- extensionless `application/csv` + category='other' → office HTML
- extensionless parameterized office MIME + category='other' → office HTML
- defense check: actual binary 'other' (image/jpeg) still returns null
without invoking the office producer
Two related Codex catches on PR #12934 — both about MIME-routing
inconsistencies between backend and client that would cause
extensionless CSV files to render as broken (raw text under an HTML
slot) or skip the artifact panel entirely.
P2 — backend MIME normalization:
`officeHtmlBucket` matched MIME strings exactly, so a real-world
`text/csv; charset=utf-8` Content-Type slipped through and the
backend returned raw CSV text. The client's `baseMime` helper
strips parameters before its own MIME lookup, so it routed the
same file to the SPREADSHEET bucket expecting an HTML body that
never arrived. Mirror the client's normalization on the backend
(strip everything from `;` onward, lowercase) before bucket
matching.
P3 — client legacy CSV MIME:
Backend's `CSV_MIME_PATTERN` accepts three variants (`text/csv`,
`application/csv`, `text/comma-separated-values`); the client's
`MIME_TO_TOOL_ARTIFACT_TYPE` only had the first two. An
extensionless file with `text/comma-separated-values` would have
backend HTML produced but the client would skip the artifact
panel entirely. Add the missing variant.
Tests:
- 9 new parameterized-MIME cases on backend covering charset/
boundary/case variants for every bucket.
- 1 new client routing case for `text/comma-separated-values`.
Addresses pre-existing availability vulnerability validated by
SEC review (Codex finding 275344c5...) and made worse by this PR's
HTML rendering path. A sub-1MiB compressed XLSX/DOCX/PPTX (highly
compressed run-of-zeros) inflates to 200+ MiB of XML when handed
to mammoth/xlsx — blocking the Node event loop for 10+ seconds and
spiking RSS to ~1 GiB. The existing 8s `withTimeout` wrapper uses
`Promise.race`, which can only return early; it cannot interrupt
synchronous parser CPU/RAM consumption. PoC ran an authenticated
execute_code call to OOM the API process.
Add `assertSafeZipSize(buffer)` — a yauzl-based pre-flight that
streams every entry with mid-inflate byte counting and bails on
either a per-entry or total decompressed-size cap. Mid-inflate
counting cannot be bypassed by falsifying the central directory's
`uncompressedSize` field (the technique the PoC used). Defaults:
25 MiB per entry, 100 MiB total — generous headroom for legitimate
image-heavy office files, well below the attack profile.
Hook the check into every path that hands a buffer to mammoth/xlsx
/yauzl:
- New HTML producers (`wordDocToHtml`, `excelSheetToHtml`,
`pptxToSlideListHtml`) — added by this PR
- Legacy RAG text extractors (`wordDocToText`, `excelSheetToText`
in `crud.ts`) — pre-existing path, also vulnerable
Errors propagate as a tag-distinct `ZipBombError` so callers can
distinguish a refused bomb from generic parse failures. The outer
`extractCodeArtifactText` swallows the error and returns null,
falling back to the regular download UI.
`.xls` (BIFF/CFB binary, not ZIP) is detected by magic bytes and
skipped — yauzl would reject it as malformed anyway.
Adds 15 tests:
- `zipSafety.spec.ts` (9): benign passes, per-entry cap, total cap,
ZipBombError type-tagging, malformed-zip distinction, directory-
entry handling, named-error surfacing, and the SEC-PoC pattern
(sub-1 MiB compressed → 50 MiB inflated rejected on default caps).
- `html.spec.ts` zip-bomb suite (5): each producer rejects a bomb;
dispatcher propagates correctly; legitimate fixtures still render.
- `extract.spec.ts` (1): outer extractor swallows ZipBombError and
returns null so the download UI fallback fires.
Codex review on #12934 caught that the predicate's if-chain interleaved
extension and MIME checks for each bucket — e.g. CSV's branch was
`ext === 'csv' || CSV_MIME_PATTERN.test(mimeType)`. A `deck.pptx`
shipped with `text/csv` (sandboxed tools sometimes ship generic MIMEs)
matched the CSV branch BEFORE the PPTX extension branch was reached,
so a binary PPTX would have been handed to `csvToHtml` to parse as
text — yielding garbage or a parse exception.
Restructure to a strict two-pass dispatch: an exhaustive extension
table first (one lookup, all known extensions), then MIME-only
fallback for extensionless / unknown-ext inputs. The doc comment's
"extension wins" claim is now actually enforced by the implementation.
Add 7 regression cases covering the conflicting-MIME footgun for each
bucket: deck.pptx + text/csv → pptx; workbook.xlsx + text/csv →
spreadsheet; legacy.xls + pptx-MIME → spreadsheet; report.docx +
text/csv → docx; data.csv + docx-MIME → csv; etc.
Codex review on #12934 caught that the office-render gate in
`extractCodeArtifactText` only fired when the extension was in
`OFFICE_HTML_EXTENSIONS` or the category was `document`/`pptx`. A tool
emitting `data` with `text/csv` (no extension) classifies as
`utf8-text`, so the gate was skipped and raw CSV text shipped to the
client — but the client routes by MIME to the SPREADSHEET bucket
expecting a full HTML document, so the panel rendered broken text.
Extract a shared `officeHtmlBucket(name, mime)` predicate from
`html.ts` (returns the bucket name or null). Both `bufferToOfficeHtml`
(the dispatcher) and the upstream gate in `extract.ts` now go through
this single source of truth, so they can never drift apart again. The
predicate already mirrors the dispatcher's extension/MIME logic
(extension wins; MIME is the fallback for extensionless inputs).
Adds:
- 14 cases for the new `officeHtmlBucket` predicate covering the
positive paths (each bucket via extension OR MIME) and the negative
paths (txt, py, json, jpg, pdf, zip, odt, plain noext).
- A direct regression test in `extract.spec.ts` for the Codex catch:
`data` with `text/csv` + utf8-text category routes through the
office HTML producer.
- Parameterized cases for extensionless DOCX/XLSX/XLS/ODS/PPTX files
identified by MIME alone.
Codex review on #12934 caught that `data:` lived in the global
`allowedSchemes`, which meant a smuggled `<a href="data:text/html,
<script>...</script>">` would survive sanitization. The Sandpack
iframe sandbox does not gate `target="_blank"` navigations, so a
click would open attacker-controlled HTML in a new tab.
Scope `data:` to `<img src>` only via `allowedSchemesByTag` (mammoth
inlines DOCX images as base64 `data:image/...` URIs — that path still
works). Add a regression suite (`sanitizeOfficeHtml security`) with
8 cases covering: <script> stripping, event-handler removal,
javascript:/data: rejection on anchors, data:image preservation in
<img>, http/https/mailto allowance, target=_blank rel=noopener
enforcement, and <iframe> stripping.
Render office files emitted by tools as interactive previews in the
artifact panel instead of raw extracted text. The backend produces a
sanitized HTML document via mammoth (DOCX), SheetJS (CSV/XLSX/XLS/ODS),
or yauzl-based slide extraction (PPTX) and ships it through the
existing SSE attachment payload; the client routes it through the
Sandpack `static` template's `index.html` slot — no new browser deps,
no client-side blob fetch, no React renderer components.
Two `dangerouslySetInnerHTML` sites rendered admin-supplied HTML
without sanitization:
- `Banner.tsx` rendered `banner.message` directly.
- `MCPConfigDialog.tsx` rendered each `customUserVars` description.
Wrap both with DOMPurify, allowing only the inline tags needed for
formatting (links, emphasis, line breaks). Hardens against compromised
admin or yaml supply-chain scenarios. Pattern matches the existing
`CustomUserVarsSection.tsx` and `Tooltip.tsx` sanitizer setup.
`/c/new?prompt=…&submit=true` previously auto-submitted the prompt
unconditionally. For deployments where users may receive crafted
links from external sources, an authenticated victim's click can
trigger an immediate, attacker-controlled prompt against a memory- or
tool-enabled model — providing a 1-click vector for prompt-injection
exfiltration via markdown image rendering.
Add `interface.autoSubmitFromUrl` (default `true` to preserve current
behavior). Operators handling sensitive memory/tool data can set it
to `false` so URL-supplied prompts only pre-fill the composer; the
user must press Send explicitly.
`resizeAvatar` previously called `node-fetch` on any string input with
no validation. When OIDC providers surface a user-controllable
`picture` claim, this could be used to make blind SSRF requests to
internal services on every social login.
Wrap the URL fetch with:
- An allowlist on the URL protocol (http/https only).
- The shared `createSSRFSafeAgents` utility, which blocks resolution to
private, loopback, and link-local IPs at TCP connect time
(TOCTOU-safe; works equally for hostname targets that DNS-resolve
privately and for IP-literal targets, since Node's `net.Socket`
always dispatches through the agent's `lookup` hook).
- `redirect: 'error'` so a public-IP redirect target cannot be used to
bypass the agent check on a subsequent hop.
- A 5-second total request budget (node-fetch v2's `timeout` covers
request initiation through full body receipt, bounding slow-loris
exposure rather than just the TCP connect).
- A 10 MB response cap (`size` option + `Content-Length` pre-check +
post-read length assertion) so a hostile payload cannot exhaust
memory before `sharp()` rejects it.
Fetch the canonicalized `parsed.href` rather than the raw input string
to eliminate any future parser-differential between `new URL()` and
the underlying fetch implementation.
Per-call agent construction is intentional: the avatar path runs once
per social login per user, so pooling adds complexity without a
measurable benefit. Documented inline.
Comprehensive test coverage in `avatar.spec.js`:
- Rejects malformed URLs, non-http(s) schemes (file://, data:,
javascript:).
- Asserts the happy-path canonicalization (`fetch` is called with
`parsed.href`) and the SSRF-safe agent factory routing
(https→httpsAgent, http→httpAgent).
- Rejects non-2xx HTTP status.
- Rejects an oversized Content-Length before reading the body, and
asserts `.buffer()` is never invoked in that case.
- Rejects an oversized body even when the server lies about / omits
Content-Length.
- Surfaces ESSRF, redirect, and `size` overflow errors thrown by the
fetch layer.
- Confirms Buffer inputs bypass the fetcher entirely.
* 🪟 feat: Add allowedAddresses Exemption List For SSRF-Guarded Targets
LibreChat already blocks SSRF-prone targets (private IPs, loopback,
link-local, .internal/.local TLDs) at every server-side fetch site
that consumes user-controllable URLs — custom-endpoint baseURLs, MCP
servers, OpenAPI Actions, and OAuth endpoints. The only existing
escape hatch is `allowedDomains`, but that flips the field into a
strict whitelist: adding `127.0.0.1` to permit a self-hosted Ollama
also blocks every public destination that isn't in the list.
Introduce `allowedAddresses` as the orthogonal primitive: a private-
IP-space exemption list. When a hostname or its resolved IP appears
in the list, the SSRF block is bypassed for that target. Public
destinations remain reachable. Operators can now run self-hosted
LLMs / MCP servers / Action endpoints on private addresses without
weakening the default-deny posture for everything else.
Schema additions in `packages/data-provider/src/config.ts`:
- `endpoints.allowedAddresses` (new — gates `validateEndpointURL`)
- `mcpSettings.allowedAddresses` (parallel to `allowedDomains`)
- `actions.allowedAddresses` (parallel to `allowedDomains`)
Core changes in `packages/api/src/auth/`:
- New `isAddressAllowed(hostnameOrIP, allowedAddresses)` — pure,
case-insensitive, bracket-stripped literal match.
- Threaded the list through `isSSRFTarget`, `resolveHostnameSSRF`,
`isDomainAllowedCore`, `isActionDomainAllowed`, `isMCPDomainAllowed`,
`isOAuthUrlAllowed`, and `validateEndpointURL`.
- Extended `createSSRFSafeAgents` and `createSSRFSafeUndiciConnect`
to accept the list, building an SSRF-safe DNS lookup that exempts
matching hostnames/IPs at TCP connect time (TOCTOU-safe).
Wiring:
- Custom and OpenAI endpoint initialize sites pass
`endpoints.allowedAddresses` to `validateEndpointURL`.
- `MCPServersRegistry` stores `allowedAddresses` and exposes it via
`getAllowedAddresses()`. The factory, connection class, manager,
`UserConnectionManager`, and `ConnectionsRepository` all thread
it through to the SSRF utilities.
- `MCPOAuthHandler.initiateOAuthFlow`, `refreshOAuthTokens`, and
`validateOAuthUrl` accept the list and consult it on every URL
validation along the OAuth chain.
- `ToolService`, `ActionService`, and the assistants/agents action
routes pass `actions.allowedAddresses` to `isActionDomainAllowed`
and to `createSSRFSafeAgents` for runtime action calls.
- `initializeMCPs.js` reads `mcpSettings.allowedAddresses` from the
app config and forwards it to the registry constructor.
Documentation:
- `librechat.example.yaml` shows the new field next to each existing
`allowedDomains` block, with a note clarifying that
`allowedAddresses` is an exemption list (not a whitelist).
Tests:
- Unit tests for `isAddressAllowed` covering literal IPs, hostnames,
IPv6 brackets, case insensitivity, and partial-match rejection.
- Exemption tests for every entry point: `isSSRFTarget`,
`resolveHostnameSSRF`, `validateEndpointURL`, `isActionDomainAllowed`,
`isMCPDomainAllowed`, `isOAuthUrlAllowed`.
- Existing tests updated to reflect the new optional parameter.
Default behavior is unchanged: omitted = empty list = no exemptions.
* 🩹 fix: Plumb allowedAddresses Through AppConfig endpoints Type
The initial PR added `endpoints.allowedAddresses` to the
data-provider config schema and consumed it in the endpoint
initialize sites, but the runtime `AppConfig.endpoints` shape in
`@librechat/data-schemas` was a hand-maintained subset that didn't
include the new field — so `tsc` rejected `appConfig.endpoints.allowedAddresses`.
Add the field to `AppConfig['endpoints']` in
`packages/data-schemas/src/types/app.ts` and forward it from the
loaded config in `packages/data-schemas/src/app/endpoints.ts` so the
runtime config carries the value.
Update `initializeMCPs.spec.js` to expect the third positional
argument (`allowedAddresses`) on the `createMCPServersRegistry` call.
* 🩹 fix: Enforce allowedDomains Before allowedAddresses In isOAuthUrlAllowed
The initial implementation checked the address exemption first, so a
URL whose hostname appeared in `allowedAddresses` would return true
even when the admin had configured `allowedDomains` as a strict bound
on OAuth endpoints. A malicious MCP server could advertise OAuth
metadata, token, or revocation URLs at any address the admin had
permitted for an unrelated reason (a self-hosted LLM at `127.0.0.1`,
for example) and pass validation, expanding SSRF reach beyond the
configured domain whitelist.
Reorder: when `allowedDomains` is set, treat it as authoritative —
return true only if the URL matches a domain entry, otherwise fall
through to false. The address exemption only applies when no
`allowedDomains` is configured (mirrors how the downstream SSRF check
in `validateOAuthUrl` consults `allowedAddresses`).
Add a regression test asserting that an `allowedAddresses` entry does
not broaden a configured `allowedDomains` list.
Reported by chatgpt-codex-connector on PR #12933.
* 🩹 fix: Forward allowedAddresses To Remaining OAuth Callers
Two `MCPOAuthHandler` callers still used the pre-feature signatures and
were silently dropping the new `allowedAddresses` argument:
- `api/server/routes/mcp.js` invoked `initiateOAuthFlow` with the old
5-argument shape, so OAuth flows initiated through the route handler
ignored the registry's `getAllowedAddresses()` and would reject any
metadata/authorization/token URL on a permitted private host.
- `api/server/controllers/UserController.js#maybeUninstallOAuthMCP`
invoked `revokeOAuthToken` without the address exemption, so
uninstalling an OAuth-backed MCP server on a permitted private host
would fail at the revocation step even though the rest of the MCP
connection path now permits it.
Both sites now read `allowedAddresses` from the registry alongside
`allowedDomains` and forward it. Reported by Copilot on PR #12933.
* 🩹 fix: Update Test Mocks And Assertions For OAuth allowedAddresses
The previous commit started passing `allowedAddresses` to
`MCPOAuthHandler.initiateOAuthFlow` from `api/server/routes/mcp.js`
and to `MCPOAuthHandler.revokeOAuthToken` from
`api/server/controllers/UserController.js`, but the corresponding
test files mocked the registry without `getAllowedAddresses` (causing
`TypeError`s) and asserted the old positional shape on
`toHaveBeenCalledWith`.
Update the mocks and assertions to match the new arity:
- `api/server/routes/__tests__/mcp.spec.js`: add
`getAllowedDomains`/`getAllowedAddresses` to the registry mock and
expect the additional positional args on `initiateOAuthFlow`.
- `api/server/controllers/__tests__/maybeUninstallOAuthMCP.spec.js`:
add a `getAllowedAddresses` mock alongside the existing
`getAllowedDomains` and seed it in `setupOAuthServerFound`.
- `api/server/controllers/__tests__/UserController.mcpOAuth.spec.js`:
add `getAllowedAddresses` to the registry mock and expect the
trailing `null` arg on the three `revokeOAuthToken` assertions.
* 🛡️ fix: Address Comprehensive Review — Scope allowedAddresses To Private IP Space
Major findings from the comprehensive PR review (severity → fix):
**CRITICAL — `validateOAuthUrl` SSRF fallback bypass.** When `allowedDomains`
is configured and a URL fails the whitelist, the SSRF fallback in
`validateOAuthUrl` was still passing `allowedAddresses` to `isSSRFTarget` /
`resolveHostnameSSRF`, letting a malicious MCP server advertise OAuth
endpoints at any address the admin had permitted for an unrelated reason.
Suppress `allowedAddresses` in the fallback when `allowedDomains` is active —
the address exemption is opt-in for the no-whitelist mode only.
**MAJOR — WebSocket transport SSRF check ignored exemptions.** The
`constructTransport` WebSocket branch called `resolveHostnameSSRF(wsHostname)`
without `this.allowedAddresses`, so a permitted private MCP server would
pass `isMCPDomainAllowed` but be blocked at transport creation. Forward
the exemption.
**Scope `allowedAddresses` to private IP space only (operator directive).**
The exemption list is for permitting private/internal targets; it must not
be a back-door to broaden trust to public destinations.
- Schema (`packages/data-provider/src/config.ts`): new
`allowedAddressesSchema` rejects URLs (`://`), paths/CIDR (`/`),
whitespace, and public IPv4/IPv6 literals at config-load time. Wired
into `endpoints`, `mcpSettings`, and `actions`.
- Runtime (`packages/api/src/auth/domain.ts`): `isAddressAllowed` now
drops public-IP candidates and public-IP entries on the match path —
defense in depth so a misconfigured runtime list never grants exemption.
- Hot path (`packages/api/src/auth/agent.ts`): `buildSSRFSafeLookup`
pre-normalizes the list into a `Set<string>` once at construction and
applies the same scoping filter, so the connect-time DNS lookup is an
O(1) Set membership check instead of a full re-iterate-and-normalize on
every outbound request.
**Test coverage for the connect-time and OAuth-fallback paths.**
- `agent.spec.ts`: new describe block exercising `buildSSRFSafeLookup` and
`createSSRFSafe*` with `allowedAddresses` — hostname-literal exemption,
resolved-IP exemption, public-IP scoping, URL/CIDR/whitespace rejection,
and the default no-list block.
- `handler.allowedAddresses.test.ts` (new): integration tests for
`validateOAuthUrl` — covers both the no-domains-set "permit private"
path and the strict-bound regression where `allowedAddresses` must NOT
bypass `allowedDomains`.
**Documentation & cleanup.**
- `connection.ts` redirect SSRF check: explicit comment that
`allowedAddresses` is intentionally NOT consulted for redirect targets
(server-controlled, must not inherit the admin's exemption).
- `MCPConnectionFactory.test.ts`: replaced an `eslint-disable` with a
proper `import { getTenantId } from '@librechat/data-schemas'`. The
disable was added to make a pre-existing `require()` quiet — the cleaner
fix is to use the existing top-level import.
Updated `MCPConnectionSSRF.test.ts` WebSocket SSRF assertions to match the
new two-argument call shape (`hostname, allowedAddresses`).
* 🩹 fix: Require Absolute URL Before allowedAddresses Trust Bypass In isOAuthUrlAllowed
`parseDomainSpec` is lenient — it silently prepends `https://` to
schemeless inputs so it can match patterns like bare `example.com`.
That leniency leaked into `isOAuthUrlAllowed`'s new
`allowedAddresses` short-circuit: a value like `10.0.0.5/oauth` (no
scheme) would parse successfully via the prepended default, hit the
address-exemption path, return `true`, and skip `validateOAuthUrl`'s
strict `new URL(url)` parse-or-throw — only to fail later in OAuth
discovery with a less clear runtime error.
Add a strict `new URL(url)` gate at the top of `isOAuthUrlAllowed`.
Schemeless inputs now fall through to `validateOAuthUrl`'s explicit
"Invalid OAuth <field>" rejection. Tests added in both
`auth/domain.spec.ts` (unit) and the OAuth handler integration spec
(end-to-end).
Reported by chatgpt-codex-connector (P2) on PR #12933.
* 🛡️ fix: Address Follow-Up Comprehensive Review — Schema Tests, Shared Normalization, host:port
Auditing the second comprehensive review:
**F1 MAJOR — schema validation untested.** `allowedAddressesSchema` had
zero coverage, so a regression in the three refinement stages or the
three wiring locations (`endpoints` / `mcpSettings` / `actions`) would
silently let invalid entries reach the runtime. Added a dedicated
`describe('allowedAddressesSchema')` block in `config.spec.ts` covering:
valid private IPs (v4 + v6, including the previously-missed 192.0.0.0/24
range), accepted hostnames, all rejection categories (URLs, CIDR, paths,
whitespace tabs/newlines, host:port, public IP literals), and full
`configSchema.parse()` integration at each of the three nesting points.
**F2 MINOR — `isPrivateIPv4Literal` divergence.** The schema reimpl in
`packages/data-provider` was discarding the `c` octet, so the
`192.0.0.0/24` (RFC 5736 IETF protocol assignments) range that the
authoritative `isPrivateIPv4` accepts was being rejected with a
misleading "public IP" error. Destructure `c` and add the missing range
check; covered by the new schema tests.
**F3 MINOR — DRY violation across `domain.ts` and `agent.ts`.** Both
files had independent normalization implementations with a subtle
whitespace-check divergence (`/\s/` vs `.includes(' ')`). Extracted the
shared logic into a new `packages/api/src/auth/allowedAddresses.ts`
module that both consumers import:
- `normalizeAddressEntry(entry)` — single-entry shape check
- `looksLikeHostPort(entry)` — host:port detector (used by F4)
- `normalizeAllowedAddressesSet(list)` — pre-normalized Set for the
connect-time hot path
- `isAddressInAllowedSet(candidate, set)` — membership check that
enforces private-IP scoping on the candidate
Both `isAddressAllowed` (preflight) and `buildSSRFSafeLookup` (connect)
now go through the same primitives; the whitespace divergence is gone.
To break the import cycle (`allowedAddresses` needs `isPrivateIP`,
`domain` previously owned it), extracted IP private-range detection
into a leaf `auth/ip.ts` module. `domain.ts` re-exports `isPrivateIP`
for backward compatibility with existing call sites.
**F4 MINOR — `host:port` silently misclassified.** Entries like
`localhost:8080` previously slipped through the URL/path guard, were
mis-detected as IPv6, failed `isPrivateIP`, and were silently dropped
with a misleading "public IP" schema error. Added an explicit
`looksLikeHostPort` check with a clear error: "allowedAddresses
entries must not include a port — list the bare hostname or IP only."
Bare `::1`, `[::1]`, and other valid IPv6 literals are intentionally
not matched (regex distinguishes by colon count and the bracketed
`[ipv6]:port` form).
**F5 MINOR — hostname-trust documentation gap.** Hostname entries
short-circuit `resolveHostnameSSRF` before any DNS lookup — that's a
deliberate design (admin trusts the name) but it means the exemption
follows whatever the name resolves to at runtime. Added an explicit
note in `librechat.example.yaml` for both `mcpSettings.allowedAddresses`
and `endpoints.allowedAddresses`: "a hostname entry trusts whatever IP
that name resolves to. Only list hostnames whose DNS you control.
Prefer literal IPs when you can."
**F6** (8 positional params) is flagged for follow-up; refactor to an
options object is a breaking-API change deferred to a separate PR.
**F7** (redirect/WebSocket asymmetry, NIT, conf 40) — skipping; the
existing inline comment is sufficient.
* 🧹 chore: Address Follow-Up NITs — Import Order And Mirror-Function Naming
Three NITs from the latest comprehensive review:
**NIT #1 (conf 85) — local import order.** AGENTS.md requires local
imports sorted longest-to-shortest. Both `domain.ts` and `agent.ts`
had `./ip` (shorter) before `./allowedAddresses` (longer). Swapped.
**NIT #2 (conf 60) — missing cross-reference.** The schema-side
`isHostPortShape` in `packages/data-provider/src/config.ts` had no
note pointing at the canonical runtime mirror. Added a JSDoc paragraph
explaining the mirror relationship and why a local copy exists (the
data-provider package can't import from `@librechat/api` without
creating a circular dependency).
**NIT #3 (conf 50) — naming inconsistency.** Renamed
`isHostPortShape` → `looksLikeHostPort` so the schema mirror matches
the runtime helper exactly. Kept as a separate function (not a shared
import) for the same circular-dependency reason; the matching name
makes it obvious they should stay in lockstep.
The Passport local strategy validation error logged the entire request
body (including the password) into error logs. Replace it with the
email only, matching the metadata shape used by sibling log calls in
the same function.
* 🔧 chore: Update dependencies in package-lock.json and package.json
- Bump version of @librechat/agents to 3.1.75-dev.0 in multiple package.json files.
- Upgrade various AWS SDK and Smithy dependencies to their latest versions in package-lock.json for improved stability and performance.
* 🔧 chore: Update AWS SDK and Smithy dependencies in package-lock.json
- Bump version of @aws-sdk/client-bedrock-runtime to 3.1041.0 and update related dependencies for improved performance and stability.
- Upgrade various AWS SDK and Smithy packages to their latest versions, ensuring compatibility and enhanced functionality.
* chore: Align LibreChat with agents LangChain upgrade
- Route LangChain imports through @librechat/agents facade exports
- Update @librechat/agents to 3.1.75-dev.1 and remove direct LangChain deps
- Normalize nullable agent model params and API key override typing
- Update Google thinking config typing for newer LangChain packages
- Refresh targeted audit-related dependency overrides
* chore: Add Jest types for API specs
* test: Fix LangChain upgrade CI specs
* test: Exercise agents env facade
* fix: Clean up TS preview diagnostics
* fix: Address Codex review feedback
Fixes #12912.\n\n- Clear stored MCP OAuth tokens and flow state on revoke cleanup-only paths.\n- Keep provider revocation best-effort when token and client metadata are available.\n- Add controller and function coverage for stale metadata, missing config, and cleanup failure paths.
* 🩹 fix: Sync ControlCombobox popover width with trigger after layout changes
The popover width was measured once on mount via offsetWidth. When the agent builder side panel opens after a page reload with the sidebar collapsed, the trigger button is initially measured during the layout transition (~26px) and never re-measured, leaving the agent select dropdown rendered at the far left with no options fully visible.
Use a ResizeObserver to keep buttonWidth in sync with the trigger's actual width whenever it resizes, then disconnect on unmount.
* test: cover ControlCombobox isCollapsed, no-ResizeObserver, and zero-width branches
Address review feedback:
- Use button.offsetWidth as the ResizeObserver fallback instead of
entry.contentRect.width to avoid a content-box vs border-box mismatch in
pre-2022 browsers that ship ResizeObserver without borderBoxSize.
- Add tests for the three previously-untested branches: isCollapsed=true
(no observation of the trigger), ResizeObserver unavailable (sync-only
measurement), and zero-width entries (state unchanged).
* test: lock the button.offsetWidth fallback against revert
Add a test that drives the ResizeObserver callback with borderBoxSize
absent and divergent contentRect.width vs offsetWidth (251 vs 275).
The fix would silently revert to entry.contentRect.width without this
test failing, so this pins the chosen fallback semantics.
---------
Co-authored-by: Danny Avila <danny@librechat.ai>
* 📥 fix: Use Endpoint-Aware Default Model on Imported Conversations
Claude conversations imported from claude.ai's data export display
"gpt-4o-mini" in the chat UI until the page is refreshed, and any
attempt to send a message before refreshing fails with "The model
'gpt-4o-mini' is not available for Anthropic."
Root cause: ImportBatchBuilder.finishConversation() unconditionally
defaulted the saved conversation's `model` field to
openAISettings.model.default, regardless of `this.endpoint`. Claude
exports don't carry a model name, so every imported Claude conversation
landed with endpoint=anthropic but model=gpt-4o-mini.
Fix: pick the default based on `this.endpoint` via a small lookup
(openAI -> gpt-4o-mini, anthropic -> claude-3-5-sonnet-latest), keeping
the existing OpenAI default as the fallback for unknown endpoints.
Fixes#12844
* 🪄 refactor: Resolve Import Default Model From `modelsConfig`
Replace the hardcoded per-endpoint default lookup added in the previous
commit with a runtime resolver that consults the same models config the
chat UI uses (`getModelsConfig` in ModelController -> `loadDefaultModels`
+ `loadConfigModels`). This way an imported conversation defaults to a
model the LibreChat instance has actually configured / discovered for
the endpoint, instead of a hardcoded constant that may not exist on this
deployment.
Resolution order:
1. First non-empty model in `modelsConfig[endpoint]`.
2. Per-endpoint hardcoded fallback (anthropic/openAI settings) if the
runtime config is empty for the endpoint or `getModelsConfig` throws.
3. `openAISettings.model.default` if even the per-endpoint fallback is
missing (unknown endpoint).
`importBatchBuilder.finishConversation` now accepts an optional
`defaultModel` argument; each importer resolves it once at the top via
`resolveImportDefaultModel({ endpoint, requestUserId, userRole })` and
threads it through. ChatGPT message-level model selection also falls
back to the resolved default before the hardcoded gpt-4o-mini.
* 🔌 fix: Follow 307/308 redirects in MCP streamable HTTP transport
Some MCP servers (e.g. Coda) return 308 Permanent Redirect to route
doc-scoped tool calls to a different endpoint path. The fetch wrapper
used `redirect: 'manual'` for SSRF protection, which silently dropped
these redirects and caused tool calls to fail with empty error bodies.
Follow 307/308 redirects (method-preserving per RFC 7538) up to a
depth of 5. SSRF safety is preserved because the same undici Agent
with its SSRF-safe connect function validates redirect targets.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* 🛡️ fix: Harden MCP 307/308 redirect handling against SSRF and credential leaks
- Validate every redirect target against `resolveHostnameSSRF` so allowlist
deployments (which disable connect-time SSRF protection) still block hops to
private/reserved IPs.
- Strip `Authorization`, `Cookie`, `mcp-session-id`, and any user-injected
headers when a 307/308 crosses an origin boundary, mirroring browser/Fetch
behavior so a redirecting MCP server can't exfiltrate credentials.
- Cancel the intermediate response body before each next hop so undici can
reuse pooled sockets rather than holding them until GC.
- Restructure redirect test helpers to be same-origin (matching real-world
Coda-style routing), drop dead setup code, fix the misleading "5 hops
successfully" test, and add coverage for SSRF-blocked redirects, cross-
origin credential stripping, and same-origin credential preservation.
* 🛡️ fix: Also strip `serverConfig.headers` on cross-origin MCP redirects
Previously only runtime `setRequestHeaders` keys were treated as secret on a
307/308 cross-origin hop. API keys baked into `serverConfig.headers` (passed
through `requestInit.headers` at transport construction time) survived
stripping, so a malicious MCP endpoint could exfiltrate them by returning a
cross-origin `Location`. Pass the configured header keys through to
`createFetchFunction` so both runtime and config secrets are stripped.
The cross-origin credential test now also configures `serverConfig.headers`
to lock in this behavior.
* 🧹 chore: Tighten MCP redirect-stripping coverage and helper duplication
- Add `proxy-authorization` to the cross-origin forbidden header set so a
forward-proxy credential header would also be stripped on a cross-origin
hop, matching the Fetch-spec list.
- Strengthen the cross-origin credential test with positive assertions that
benign protocol headers (`accept`, `content-type`) survive the hop, so a
regression that strips everything indiscriminately would now fail.
- Extract the duplicated MCP request handler / session-teardown logic from
three test helpers into shared `createMCPRequestHandler` and
`closeMCPSessions` utilities.
* 🛠️ fix: Handle `Request` inputs in MCP `customFetch` URL derivation
`customFetch` is typed to accept `UndiciRequestInfo` (`string | URL | Request`),
but `Request.prototype.toString()` returns `"[object Request]"`. The previous
implementation derived `originalOrigin` and the redirect base via
`url.toString()`, so a `Request` input would throw inside `new URL(...)` before
any network call — a regression even when no redirect was involved.
Add a `getRequestUrlString` helper that extracts the URL string for all three
shapes, track the URL string alongside the fetch input through the redirect
loop, and add parameterized tests that exercise `customFetch` with each shape.
* 🛠️ fix: Don't override `Request` input headers in MCP `buildFetchInit`
Previously `buildFetchInit` always set `headers` on the returned init —
even when neither `init.headers` nor runtime headers contributed anything.
Passing `headers: {}` to `undiciFetch` overrides the headers carried on a
`Request` input (auth tokens, MCP session, protocol negotiation), so
Request-based wrappers could fail authentication even without a redirect
in play. Skip the `headers` override entirely when there is nothing to
merge.
Adds a regression test that supplies `Authorization` and a custom header
on the `Request` itself and asserts both reach the target server.
* 🛠️ fix: Preserve `Request` method/body across MCP redirects + guard cross-origin strip
Two regressions surfaced by extending `customFetch` to accept `Request` inputs:
1. **307/308 method/body loss.** The redirect loop switches `url` to the
new `Location` string, but the original `Request`'s method and body
stayed bound to the (now-discarded) `Request` object. A redirected
POST silently became a GET with no payload — the exact behavior the
method-preserving codes are designed to prevent. Added a
`resolveFetchInput` helper that runs once at the top of `customFetch`,
extracts a `Request`'s method/body/headers into the shared init, and
buffers the body via `arrayBuffer()` so 307/308 retries can replay it.
2. **Cross-origin strip crashed on absent headers.** After the previous
fix that stopped `buildFetchInit` from setting `headers: {}`,
`currentInit.headers` could legitimately be `undefined`. The
cross-origin branch read it as a `Record` and called `Object.entries`
on `undefined`, throwing `TypeError`. Guard the branch on
`currentInit.headers != null` — when there are no headers there is
nothing to strip.
Adds two regression tests: a POST-with-body `Request` that 308-redirects
cross-origin (asserts both method and body survive) and a no-headers
cross-origin redirect (asserts the strip path no longer crashes).
* 🛠️ fix: Forward `Request.signal` through MCP `customFetch` normalization
`resolveFetchInput` was copying method/body/headers off a `Request` input
but dropping `Request.signal` on the floor, so a caller that wired an
`AbortController` onto the `Request` for cancellation/timeouts lost that
wiring as soon as we re-shaped the input into the `(string, init)` pair
used by the redirect loop. Subsequent aborts no longer reached the
in-flight fetch — a regression from the pre-PR code, which forwarded
the original `Request` directly to undici.
Forward the signal alongside method/body/headers, with explicit
`init.signal` still winning per Fetch-spec semantics. Regression test
aborts a controller before calling \`customFetch\` with the wired
`Request` and asserts the call rejects.
* 🧪 test: Pin URL.origin contract for protocol-downgrade redirect handling
Audit follow-up. The cross-origin strip path keys off
`targetUrl.origin !== originalOrigin`, and `URL.origin` is defined as
`scheme + "://" + host + ":" + port`, so a same-host `https → http`
redirect produces a different origin and trips the strip path through
the existing logic — no separate code path needed.
Pin that contract with a small unit test so a future change to URL
semantics (or a refactor that swaps in a different comparison) doesn't
silently regress protocol-downgrade stripping. Standing up a TLS
fixture (self-signed cert, undici skip-verify, etc.) just to re-prove
the URL spec is wasted complexity.
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Danny Avila <danny@librechat.ai>
* 🧹 chore: Strip code-execution boilerplate from tool output
The bash executor in `@librechat/agents` appends two kinds of noise to
every successful run:
1. Trailing `Note:` paragraphs — long behavioral hints repeating
rules already in the system prompt ("Files from previous executions
are automatically available...", "Files in 'Available files' are
inputs..."). Re-stating these on every tool call adds ~50 tokens of
waste per call, which compounds across long agent traces.
2. Per-file `| <annotation>` suffixes on every line of `Generated
files:` / `Available files (...):`. The two section headers already
convey the new-vs-known distinction; the per-file annotations are
redundant *and* phrased inconsistently ("downloaded by the user"
vs. "displayed to the user" vs. "known to the user").
Strip both in a small `cleanCodeToolOutput` helper invoked from
`packages/api/src/agents/handlers.ts` for every tool listed in
`CODE_EXECUTION_TOOLS`. Non-code-execution tools pass through
unchanged. The cleaning happens *after* tool resolution but *before*
downstream consumers (model context, SSE forwarding, persistence) see
the content, so subsequent model turns get the lean output.
* 🩹 fix: Polish code-execution attachment rendering
Three rough edges visible in code-interpreter conversations:
1. **Sandbox-internal `.dirkeep` placeholders leak as file chips.** The
bash executor creates `.dirkeep` inside any new directory so the
stateless container preserves the folder across executions. After
`sanitizeArtifactPath`'s `_` prefix and 6-hex collision suffix it
surfaces as `_.dirkeep-<hash>` — a 0-byte chip with no value to the
user, sometimes hiding the real artifact behind it. New
`isInternalSandboxArtifact` helper filters them out of every
routing path (`Attachment`, `AttachmentGroup`, `LogContent`).
2. **The `-<hash>` collision suffix is visible in chip labels.** The
suffix is collision-avoidance machinery; users only need to see the
canonical name. New `displayFilename` strips it for display while
leaving the on-disk `attachment.filename` untouched so downloads
resolve. Applied across `FileContainer`, `ToolArtifactCard`,
`ToolMermaidArtifact`, and `LogContent`'s text-attachment label
path.
3. **0-byte / placeholder files outrank real artifacts in render
order.** Bucket sort by salience (non-empty before empty) sinks
stragglers to the bottom. Stable sort preserves arrival order for
peers.
Added regression tests cover the new helpers, the dirkeep filter
across buckets, and the within-bucket salience ordering.
* 🩹 fix: Don't auto-open artifact panel on history navigation
Navigating to a previous conversation full of code-execution artifacts
would auto-open the side panel and focus the most-recent artifact —
the same code path that fires for fresh streaming artifacts. Users
expect that "auto-open" behavior only when an artifact arrives via
SSE, not when they revisit an old chat.
Two-part gate:
1. `ToolArtifactCard`'s focus effect captures `isSubmitting` at first
render via a ref. A card mounted *during* a stream means a new
artifact arrived → steal panel focus (legacy behavior). A card
mounted while `isSubmitting === false` is part of conversation
history → leave focus alone.
2. `Presentation`'s panel-render condition gains `currentArtifactId
!= null`. With (1) keeping `currentArtifactId` null on history
load, the panel stops rendering at all on navigation — even if
`artifactsVisibility` was left `true` by a prior conversation.
User clicks on a chip to re-open (the click handler is unchanged
and unconditional).
Test seeds `isSubmittingFamily(0)` per case: existing tests opt into
streaming (default `true`) so legacy auto-focus assertions still hold;
new tests for history-load opt into `streaming: false` and verify
no auto-focus + click-to-open still works.
* 🩹 fix: Force panel visible on streaming artifact arrival
The previous commit gated `setCurrentArtifactId` on `isSubmitting` but
left `artifactsVisibility` untouched. When a user had explicitly
closed the panel earlier in the session, a fresh SSE artifact would
set `currentArtifactId` (so the chip read "click to close") but
`Presentation`'s render condition still required `visibility === true`
— net effect: the card claimed to be open, the panel stayed hidden.
Streaming arrivals now also call `setVisible(true)`, which is the
explicit "auto-open when first created" behavior the user asked for.
History mounts (`isSubmitting === false`) still leave both focus and
visibility alone, so navigating to an old conversation does not
re-open the panel.
Two regression tests added: one asserts streaming flips visibility on
even when seeded false, the other asserts history mounts leave a
seeded-false visibility alone.
* 🧹 chore: Tighten code-execution attachment polish per audit feedback
Resolves the eight actionable findings from the comprehensive audit:
- Scope `displayFilename` out of `FileContainer`: opt-in via a new
`displayName` prop. User-uploaded chips (input area, persisted
message files) keep their raw filename, eliminating the false-positive
class where `report-abc123.pdf` was silently rewritten to `report.pdf`.
Code-execution artifact paths in `Attachment.tsx` explicitly compute
the de-suffixed name and pass it through.
- Tighten `TRAILING_NOTES_PATTERN` to anchor on the two known boilerplate
openings (`Files from previous executions`, `Files in "Available files"`),
so a user-authored `Note:` line preceded by a blank line in stdout no
longer gets eaten along with everything after it.
- `ToolMermaidArtifact`: compute `visibleFilename` once and reuse for
title, content, and the download `aria-label` (was using the raw
`attachment.filename` for the aria-label, creating a screen-reader
inconsistency).
- `ToolArtifactCard`: read `isSubmittingFamily(0)` once via a
non-subscribing `useRecoilCallback`, instead of subscribing for the
full lifetime to a value the ref only ever needs at first render.
- Extract `bySalience` and `byEntrySalience` comparators from
`attachmentTypes.ts`, replacing the ten duplicated sort lambdas in
`Attachment.tsx` and `LogContent.tsx`.
- Treat `attachmentSalience({ bytes: undefined })` as neutral (`0`)
rather than empty (`1`); only an explicit `bytes === 0` sinks. Stops
non-code-exec sources (web-search inline results, files where the
schema omits the byte count) from silently sinking past real content.
- Pin the click-history test to the panel-open button by name instead
of relying on `getByRole('button', { pressed: false })`, which
matched by DOM order.
- Add the missing blank line between adjacent `it(...)` blocks.
- Drop the verbose narrating comments in `FileContainer` along with the
removed `displayFilename` import.
Adds three regression tests for the new behavior (FileContainer raw
filename, artifact-context displayName flow, user-authored `Note:` line
preserved through cleanup) and updates the salience test for the new
neutral-undefined semantics.
* 🧹 chore: Drop redundant `@testing-library/jest-dom` import in FileContainer spec
`client/test/setupTests.js` already imports the matchers globally for every
Jest test in the client workspace, so the explicit import here was dead code.
Removing it brings the spec in line with the broader convention used by
`ArtifactRouting.test.tsx`, `LogContent.test.tsx`, and `attachmentTypes.test.ts`.
* 🛡️ fix: Narrow `.dirkeep`/`.gitkeep` filter to the sandbox-specific form
`isInternalSandboxArtifact` was filtering bare `.dirkeep` / `.gitkeep`
along with the post-sanitization form. Bare versions never originate
from the bash executor (the dotfile rewrite + disambiguator step in
`sanitizeArtifactPath` always produces `_.dirkeep-<6 hex>`), so the only
real-world source of a bare `.gitkeep` is project scaffolding the user
uploaded — silently hiding it from every attachment bucket meant the
file disappeared with no way to surface or download it.
Tightening to `^_\.(?:dirkeep|gitkeep)-[0-9a-f]{6}$` keeps the
sandbox-placeholder filter intact while letting user-uploaded markers
render normally. Tests inverted accordingly: bare forms now expected to
render; only the post-sanitization form is filtered.
* 🩹 fix: Address comprehensive-review findings on attachment helpers
Five findings from the latest pass:
- **MAJOR — `displayFilename` false-positive on extensionless 6-hex.**
The previous regex `/-[0-9a-f]{6}(?=\.[^.]+$|$)/` stripped any leaf
ending in `-XXXXXX` regardless of context, so a user-named
`build-a1b2c3` (script-emitted hash artifact, no extension) lost its
tail and rendered as `build`. Split into two narrower patterns:
`COLLISION_SUFFIX_BEFORE_EXT` only matches when followed by an
extension; `SANITIZED_DOTFILE_TRAILING_SUFFIX` only fires when the
leaf starts with `_.` AND ends with `-XXXXXX` — the unambiguous
fingerprint of `sanitizeArtifactPath`'s dotfile rewrite.
- **MINOR — `isInternalSandboxArtifact` filter too aggressive.**
`(file.bytes ?? 0) > 0` treated undefined bytes as zero, falling
through to the regex check. Tightened to `file.bytes !== 0`: only
an *explicit* zero counts as the empty-placeholder shape worth
hiding. Non-code-exec sources without `bytes` populated render
normally now.
- **MINOR — `getValue()` could throw on a degenerate atom state.**
Switched the snapshot read in `ToolArtifactCard` to
`valueMaybe() ?? false` so a transient error / loading state on the
upstream selector doesn't crash card mount. The `false` default is
the right history-fallback (don't auto-open if we can't classify).
- **NIT — `attachmentSalience` / `bySalience` over-broad signature.**
Removed the test-only `{ bytes?: number }` arm; functions now accept
`TAttachment` directly. The internal `bytes` read still goes through
a cast since not every TAttachment branch declares it. Tests updated
to use the existing `baseAttachment(...)` helper.
- **MINOR — Missing regression test for extensionless 6-hex.**
Added `'build-a1b2c3'` and `'out/blob-deadbe'` cases that pin the
preservation behavior, plus an `isInternalSandboxArtifact` test that
asserts undefined-bytes attachments are not filtered.
* 🩹 fix: Make code-file artifacts click-to-open only
Removes mount-time auto-open from `ToolArtifactCard`. Streaming
arrivals no longer hijack the panel — even a freshly-emitted SSE
artifact registers silently in `artifactsState` and waits for the
user to click. Combined with `Presentation`'s
`currentArtifactId != null` render gate, the panel stays closed
across history navigation, page reload, and SSE arrival.
Click is the only path that opens the panel. `handleOpen` is
unchanged: first click focuses + reveals, second click on the same
chip closes.
Dropped:
- `useRecoilCallback` snapshot read of `isSubmittingFamily(0)`
- `mountedDuringStreamRef` ref + lazy-init block
- The whole focus + visibility effect (was effect 3)
- `useRef` import (now unused)
Tests:
- `ArtifactRouting.test.tsx` rewritten to exercise the click path:
registers-on-mount-without-focus, click-to-open-then-close, multi-
card-no-auto-focus, click-when-visibility-was-false. The streaming
state is no longer seeded; both `renderWith` and `renderWithProbe`
collapsed back to plain `RecoilRoot`.
- `LogContent.test.tsx` flips its panel-routing assertions from
`pressed: true` (which asserted auto-focus) to `pressed: false`
with a chip-title check (which asserts the panel card rendered
but stayed unfocused).
* Revert "🩹 fix: Make code-file artifacts click-to-open only"
This reverts commit 6761531287.
* 🩹 fix: Exclude CODE bucket from streaming auto-open
Narrows the previous-commit revert: rich-preview artifacts (HTML,
React, Markdown, plain text) keep the legacy SSE auto-open UX, but
the CODE bucket (`.py`, `.js`, `.cpp`, `Dockerfile`, `Makefile`, …)
stays click-to-open even on streaming.
Source-code artifacts are typically supporting helpers the agent
emits alongside a richer deliverable (a Python script that builds
the actual `.html` output, for example). Auto-opening every
helper's panel each time it gets written would shove the panel
in front of the user every tool call. The user explicitly opens
a code chip when they want to inspect it.
Implementation:
- Focus+open effect skips early when `artifact.type === CODE`.
- `artifact.type` added to the dep array so the gate re-evaluates
if the type ever changes (it shouldn't, but the dep is honest).
- JSDoc updated to call out the carve-out.
Tests:
- New `does NOT auto-open a streaming CODE artifact (test.py is
click-to-open)` — seeds isSubmitting=true, mounts a `.py`,
asserts the artifact registers but currentArtifactId stays null.
- New `clicking a CODE artifact focuses it even though it skipped
auto-open` — confirms the click path still surfaces a `.py`.
- All 25 prior auto-open tests for HTML/React/Markdown/plain-text
buckets still pass unchanged: those types continue to auto-open
on streaming.
* 🧹 chore: Address two NITs from the audit-fix follow-up review
- **NIT #1 (conf 60)**: Add a test for the dotfile-with-extension
intersection (`_.config-abcdef.txt` → `.config.txt`). Both halves
of the path were tested separately — extension-anchored suffix
stripping and `_.` underscore restoration — but the combination
wasn't pinned. Adds `expect(displayFilename('_.config-abcdef.txt'))
.toBe('.config.txt')`.
- **NIT #2 (conf 25)**: Tighten the cast in `attachmentSalience` from
the anonymous `{ bytes?: number }` shape to the concrete
`TFile & TAttachmentMetadata` (the actual TAttachment branch that
declares `bytes`). Same runtime behavior; a future retype of
`TFile.bytes` will now surface here at compile time instead of
being silently papered over.
* 🩹 fix: Stop stripping `-<6 hex>` suffixes from non-dotfile filenames
Codex's repeated P2 was correct: the `COLLISION_SUFFIX_BEFORE_EXT`
regex stripped any `-<6 hex>` immediately before an extension
regardless of context. That collapsed legitimate user-named files
like `report-deadbe.csv` and `report-beef01.csv` onto the same chip
label `report.csv`, silently merging distinct files in the UI.
The structural truth: only the dotfile shape (`_.foo-XXXXXX`) carries
an unambiguous discriminator (the leading `_.` that
`sanitizeArtifactPath` adds when rewriting a leading dot). The
extension-only case (`name-<hash>.ext`) has no such discriminator —
we can't distinguish a sanitized `report 1.csv` (which became
`report_1-<hash>.csv`) from a user-named `report-deadbe.csv` from
the filename alone.
Recovering the non-dotfile case cleanly would require a backend
`wasSanitized` metadata flag we don't have. Without it, the safer
choice is to leave non-dotfile names alone — uglier when the file
*was* sanitized, but never collapses distinct files onto a shared
label.
Changes:
- Drop `COLLISION_SUFFIX_BEFORE_EXT`. Replace
`SANITIZED_DOTFILE_TRAILING_SUFFIX` with a unified
`SANITIZED_DOTFILE_PATTERN` that handles both extensionless and
with-extension dotfile shapes in one regex.
- Simplify `displayFilename` to a single match + reconstruct path.
- Update tests: drop the broad-stripping assertion
(`output-deadbe.csv` → `output.csv`), add explicit codex-regression
cases (`report-deadbe.csv` and `report-beef01.csv` preserve
unchanged), document the deliberate non-recovery for sanitized
non-dotfiles, update the AttachmentGroup→FileContainer integration
test to reflect the narrower stripping (non-dotfile `archive-deadbe.zip`
passes through; new dotfile `_.config-abcdef.zip` → `.config.zip`
exercises the recoverable path).
* 🩹 fix: Scope code-tool annotation stripping to file-list sections
Codex was right: the previous global `.replace` would mutate any line
ending in one of the three annotation phrases — even legitimate
stdout. A user script doing
`echo "foo | File is already downloaded by the user"` had its output
silently scrubbed before being fed back into model context.
New `FILE_SECTION_PATTERN` captures `Generated files:` /
`Available files (...)` blocks (header + lines starting with `- /`).
Annotation stripping now only runs *within* the captured file-list
section via a nested `.replace`, so:
- Inside the section: per-file `| <ann>` suffixes still get stripped
(line-per-file ≥ 4 files form, inline `, ` comma-separated ≤ 3
files form — both already covered by existing patterns).
- Outside the section: stdout, stderr, blank lines, the trailing
`Note:` paragraphs (handled by their own pattern), and any user
text that coincidentally contains an annotation phrase pass
through unchanged.
Tests:
- New `does NOT mutate stdout that legitimately contains an
annotation phrase outside a file-list section` pins the codex
regression: three coincidental phrases in stdout, no
`Generated files:` header, all three preserved verbatim.
- New `strips annotations inside a file-list section but preserves
identical phrases in stdout above it` covers the mixed case where
the same phrase appears in both stdout and a file listing —
stdout survives, listing gets cleaned, exactly one occurrence
remains.
- All 9 prior tests still pass (file-section stripping behavior
unchanged for both line-per-file and inline-comma layouts).
* fix(sse): treat responseCode===0 as transport failure, not server error
When a long-running model response (e.g. gpt-5.4 with web_search:true)
takes longer than the browser's idle connection timeout, the SSE transport
drops and sse.js fires an error event with responseCode=0 and e.data set
to the raw response buffer (non-JSON SSE text).
The previous guard `!responseCode` is truthy for both 0 (transport drop)
and undefined (genuine server-sent error event), so the client incorrectly
entered the server-error branch, tried to JSON.parse raw SSE text, logged
"Failed to parse server error", and showed the user a red error banner --
even though the backend continued processing and delivered the final answer
seconds later.
Fix 1 (client): change guard from `!responseCode` to `responseCode == null`
so that only undefined/null (no HTTP status at all) triggers the server-error
parse path. responseCode===0 now correctly falls through to the reconnect path.
Fix 2 (backend): after res.flushHeaders() the response is already committed
as SSE. The fallback branch that wrote res.status(404).json() was an HTTP/SSE
protocol violation. Replace with an SSE-conformant event:error frame + res.end().
* fix(sse): use onError helper on subscribe failure + add regression tests
Replace silent res.end() with onError('Failed to subscribe to stream')
so the client receives a parseable SSE error event instead of a stream
that closes with no signal. The previous res.end() left the UI stuck
in "submitting" state because no error/abort/final event ever fired.
Also adds two missing test cases for the responseCode guard change:
- responseCode === 0 with raw SSE buffer data must NOT call errorHandler
(transport failure should reconnect, not display garbage)
- responseCode == null with JSON error data MUST call errorHandler
(server-sent error events should still surface to the user)
---------
Co-authored-by: Danny Avila <danny@librechat.ai>
