Commit graph

3 commits

Author SHA1 Message Date
Danny Avila
8c073b4400
📄 feat: Auto-render Text-Based Code Execution Artifacts Inline (#12829)
* 📄 feat: Auto-render Text-Based Code Execution Artifacts Inline

Eagerly extract text content from non-image artifacts produced by code
execution tools and render it inline in the message instead of behind a
click-to-download file card. Reuses the SkillFiles binary-detection
helper and the existing parseDocument dispatcher so docx, xlsx, csv,
html, code, and other text-renderable formats land directly under the
tool call.

PPTX is intentionally classified but not yet extracted — follow-up.

* 🌐 chore: Remove unused com_download_expires locale key

Removed in en/translation.json so the detect-unused-i18n-keys CI check
passes. The only reference was a commented-out localize() call in
LogContent.tsx that was deleted in the previous commit.

* 🩹 fix: Address PR review on code artifact text extraction

- extract.ts: build the temp document path from a randomUUID and pass
  path.basename(name) as originalname so a malicious artifact name
  cannot escape os.tmpdir() (P1 traversal flagged by codex/Copilot).
- process.js: classify and extract using safeName, not the raw name —
  defense in depth alongside the temp-path fix.
- classify.ts: add a bare-name lookup so extensionless text artifacts
  (Makefile, Dockerfile, …) classify as utf8-text instead of falling
  through to other.
- Attachment.tsx: wire aria-expanded / aria-controls on the show-all
  toggle for screen reader support.
- LogContent.tsx: restore a download chip (LogLink) on inline-text
  attachments so users can still pull down the underlying file.
- Tests: cover extensionless filenames and the temp-path traversal
  invariant.

* 🩹 fix: Address comprehensive PR review on code artifact extraction

- extract.ts: walk back to a UTF-8 code-point boundary before truncating
  so cuts cannot land mid-multibyte and emit U+FFFD (CJK/emoji concern).
  truncate() now accepts the original buffer to skip a redundant encode.
- extract.ts: add an 8s timeout around parseDocument via Promise.race so
  a pathological docx/xlsx cannot stall the response path.
- process.js: always set `text` (string or null) on the file payload —
  createFile uses findOneAndUpdate with $set semantics, so omitting the
  field leaves a stale value behind when an artifact's content changes.
- Attachment.tsx: switch the show-all toggle from char-count threshold
  to a useLayoutEffect ref measurement on scrollHeight, and use
  overflow-hidden when collapsed (overflow-auto when expanded) so the
  collapsed box has a single clear interaction model.
- Attachment.tsx + LogContent.tsx: lift `isImageAttachment` /
  `isTextAttachment` into a shared attachmentTypes module. LogContent
  keeps its looser image check (no width/height required) because the
  legacy log surface receives attachments without dimensions.
- Tests: cover multi-byte boundary, the always-set-text contract on
  updates, and the new shared predicates.

* 🧪 test: Component test for TextAttachment + direct withTimeout coverage

- Attachment.tsx: re-order local imports longest-to-shortest per
  AGENTS.md (attachmentTypes ahead of FileContainer/Image).
- extract.ts: export withTimeout so it can be unit-tested directly
  (it's also used internally — exporting carries no runtime cost).
- extract.spec.ts: three small unit tests on withTimeout that cover
  resolve, propagated rejection, and timeout rejection paths with
  real timers.
- TextAttachment.test.tsx: ten cases for the new React component —
  text rendering in <pre>, download chip presence/absence, ref-based
  collapse measurement (with scrollHeight stubbed via prototype),
  aria-expanded toggle, fall-through to FileAttachment for missing
  and empty text, and AttachmentGroup routing.

* 🩹 fix: Canonicalize document MIME by extension before parseDocument

When the classifier puts a file on the document path via its extension
(.docx, .xlsx, …) but the buffer sniffer returned a generic value like
application/zip or application/octet-stream, we previously forwarded
that generic MIME to parseDocument, which dispatches strictly by MIME
and silently rejected it — exactly defeating the extension-first
classification this PR added.

extractDocument now remaps the MIME from the extension (falling back
to the original sniffed MIME if the extension is unrecognized, so files
that reached the document branch via MIME detection still work). Adds
a parameterized test across docx/xlsx/xls/ods/odt against zip/octet
sniffs to guard the regression.

* 🩹 fix: Reuse existing withTimeout from utils/promise

The previous commit's local withTimeout export collided with the
already-exported `withTimeout` from `~/utils/promise`, breaking the
@librechat/api tsc job (TS2308 ambiguous re-export).

Drops the duplicate, imports from `~/utils/promise`, and removes the
now-redundant unit tests (the helper has its own coverage in
utils/promise.spec.ts). The third argument shifts from a label to the
fully-formed timeout error message that the existing helper expects.

* 🧹 chore: TextAttachment test polish (NITs)

- Use the conventional `import Attachment, { AttachmentGroup }` form
  rather than `default as Attachment`.
- Save the original `scrollHeight` property descriptor and restore it
  in afterAll, so the prototype patch never leaks past this suite.
2026-04-26 02:04:00 -07:00
Danny Avila
39f5f83a8a
🔌 fix: Isolate Code-Server HTTP Agents to Prevent Socket Pool Contamination (#12311)
* 🔧 fix: Isolate HTTP agents for code-server axios requests

Prevents socket hang up after 5s on Node 19+ when code executor has
file attachments. follow-redirects (axios dep) leaks `socket.destroy`
as a timeout listener on TCP sockets; with Node 19+ defaulting to
keepAlive: true, tainted sockets re-enter the global pool and destroy
active node-fetch requests in CodeExecutor after the idle timeout.

Uses dedicated http/https agents with keepAlive: false for all axios
calls targeting CODE_BASEURL in crud.js and process.js.

Closes #12298

* ♻️ refactor: Extract code-server HTTP agents to shared module

- Move duplicated agent construction from crud.js and process.js into
  a shared agents.js module to eliminate DRY violation
- Switch process.js from raw `require('axios')` to `createAxiosInstance()`
  for proxy configuration parity with crud.js
- Fix import ordering in process.js (agent constants no longer split imports)
- Add 120s timeout to uploadCodeEnvFile (was the only code-server call
  without a timeout)

* ✅ test: Add regression tests for code-server socket isolation

- Add crud.spec.js covering getCodeOutputDownloadStream and
  uploadCodeEnvFile (agent options, timeout, URL, error handling)
- Add socket pool isolation tests to process.spec.js asserting
  keepAlive:false agents are forwarded to axios
- Update process.spec.js mocks for createAxiosInstance() migration

* ♻️ refactor: Move code-server agents to packages/api

Relocate agents.js from api/server/services/Files/Code/ to
packages/api/src/utils/code.ts per workspace conventions. Consumers
now import codeServerHttpAgent/codeServerHttpsAgent from @librechat/api.
2026-03-19 16:16:57 -04:00
Danny Avila
f67bbb2bc5
🧹 fix: Sanitize Artifact Filenames in Code Execution Output (#12222)
* fix: sanitize artifact filenames to prevent path traversal in code output

* test: Mock sanitizeFilename function in process.spec.js to return the original filename

- Added a mock implementation for the `sanitizeFilename` function in the `process.spec.js` test file to return the original filename, ensuring that tests can run without altering the filename during the testing process.

* fix: use path.relative for traversal check, sanitize all filenames, add security logging

- Replace startsWith with path.relative pattern in saveLocalBuffer, consistent
  with deleteLocalFile and getLocalFileStream in the same file
- Hoist sanitizeFilename call before the image/non-image branch so both code
  paths store the sanitized name in MongoDB
- Log a warning when sanitizeFilename mutates a filename (potential traversal)
- Log a specific warning when saveLocalBuffer throws a traversal error, so
  security events are distinguishable from generic network errors in the catch

* test: improve traversal test coverage and remove mock reimplementation

- Remove partial sanitizeFilename reimplementation from process-traversal tests;
  use controlled mock returns to verify processCodeOutput wiring instead
- Add test for image branch sanitization
- Use mkdtempSync for test isolation in crud-traversal to avoid parallel worker
  collisions
- Add prefix-collision bypass test case (../user10/evil vs user1 directory)

* fix: use path.relative in isValidPath to prevent prefix-collision bypass

Pre-existing startsWith check without path separator had the same class
of prefix-collision vulnerability fixed in saveLocalBuffer.
2026-03-14 03:09:26 -04:00