[pull] main from danny-avila:main - #250
Merged
Merged
Conversation
* fix: surface Bedrock content-filtered responses * refactor: extract model refusal metadata * test: preserve refusal helper in agent callback mock --------- Co-authored-by: aeyeopsdev <275853971+aeyeopsdev@users.noreply.github.com> Co-authored-by: Danny Avila <danny@librechat.ai>
* 🔒 fix: Assert the Tenant When Saving a Persisted Document `doc.save()` on an already-persisted document issues `update` filtered on `_id` alone. The tenant-isolation plugin stamps `tenantId` onto the payload but never asserts it in the predicate, so the write is scoped only by provenance — safe in the normal flow, since you can only fetch a document your tenant can see, but unguarded when the `_id` comes from an unscoped source: `runAsSystem`, a cached id, or a client-supplied id. Found by the driver probe added in the previous commit, which is also what proves the fix: its characterization test flips from asserting the predicate is absent to asserting it is present. `$where` is Mongoose's documented public hook for this — "additional properties to attach to the query when calling `save()` and `isNew` is false" — and its own sharding plugin uses it to attach a shard key the same way. It is absent from the TypeScript types, hence the cast. ## Why this cannot break existing writes - `isNew` documents return early; all three `transaction.ts` saves and the `pluginAuth.ts` save construct their document, so they are untouched. - A document that never carried a `tenantId` yields no predicate, so pre-tenancy rows and the global `Role` collection still save. - With no tenant context — every single-tenant deployment — the scope is unscoped and no predicate is ever added. - The remaining two call sites fetch and save inside the same request context, so the carried tenant matches by construction. `session.ts` reaches `save()` via `createSession`, which passes an `isNew` document. - A mismatch fails loudly with `DocumentNotFoundError` rather than silently no-op'ing, and optimistic concurrency, array writes and subdocument writes were each verified to still work alongside it. 80 suites / 2793 tests green. * 🧭 fix: Reset Tenant Save Predicates Across Scopes * 🛡️ fix: Close Persisted Tenant Save Bypasses
Co-authored-by: aeyeopsdev <275853971+aeyeopsdev@users.noreply.github.com>
* feat: harden early buffer recovery observability * fix: fence recovery to emitted frontier * fix: close recovery observability races * fix: require atomic recovery capabilities * fix: fail incomplete recovery attachments closed * fix: preserve legacy store contract * fix: correlate failed overflow persistence * fix: break recovery type dependency cycle * fix: reconcile distributed recovery lifecycle * fix: preserve content snapshot contract * fix: harden cross-replica recovery lifecycle * fix: lease recovery subscriber state * fix: close recovery lifecycle edge cases * fix: close recovery validation races * test: align subscriber lease cleanup * fix: harden subscriber lease lifecycle * fix: finalize subscriber lease handoffs * fix: reconcile recovery error replacements * fix: preserve durable recovery frontier * test: expect durable frontier gaps * fix: scope durable snapshot protocol * fix: trust durable terminal recovery payloads * fix: close overflow recovery rollout races * test: bound overflow race fixture memory * fix: bound overflow marker refresh * test: reduce recovery fixture memory * fix: fence overflow recovery admission * fix: close overflow recovery admission races * fix: scope overflow admission fencing * fix: preserve owner replay after remote admission * test: await pre-admission append fencing * test: reflect pre-admission durability
* 🛡️ fix: Harden Conversation Imports and Upload Handling * 🩹 fix: Address Import Hardening Review Findings * 🩹 fix: Map Clone Size Validation Errors * 🩹 fix: Chunk Import Cleanup Queries
* feat: Define Conversation Code Approval Constraints * style: Sort Code Approval Test Imports * fix: Enforce Explicit Code Environment Policy
`@lhci/cli@0.15.1` pins `lighthouse` to an exact `12.6.1`, so the Lighthouse
lane could never move off a release whose `puppeteer-core` still depends on
`extract-zip` — an advisory with no patched version. That pin, not Lighthouse
itself, was holding 8 of the repository's 16 `npm audit` findings open.
LHCI was only ever a subprocess wrapper here: `audit.ts` shelled out to
`@lhci/cli/src/cli.js` for `collect` and `assert`, and LHCI's own node runner
shells out to `lighthouse/cli/index.js` in turn. Call that CLI directly, run
the three navigations in a loop, and assert the median budgets in TypeScript
next to the assertions `load.spec.ts` already makes.
- `npm audit`: 16 findings (7 high, 1 moderate, 8 low) -> 6 low. The remainder
is the pre-existing `elliptic` chain under `vite-plugin-node-polyfills`,
which has no fixed version and is unrelated to this lane.
- Lighthouse 13 removed `largest-contentful-paint-element` and moved the LCP
node into `lcp-breakdown-insight`. Reading it through a `lcpElement` helper
keeps the "the transcript must be the LCP element" guard working and makes
the next rename fail loudly in one place.
- Reports move to `.lighthouse/` as `lhr-N.report.{json,html}`; cookie
redaction, the API timing table and the desktop/provided-throttling settings
are unchanged. Budgets are now printed as a table immediately before they are
asserted, so the workflow's 80-line failure comment always contains them.
- `CHROME_PATH` and `LIGHTHOUSE_CHROME_FLAGS` are documented: chrome-launcher
prefers the Windows Chrome under WSL, whose debugging port Linux cannot
reach.
* 🎯 fix: Land a Steer Applied Before the First Run Step on the Live Placeholder
An interrupt (steer + preempt) sent before the model has produced a token
is applied server-side at content index 0 and stamped with the response
id the server pre-allocated at job creation. The pane, however, still
renders the in-flight response under the `${userMessageId}_` placeholder
until the FIRST run step renames it, so the exact-id lookup missed, the
bounded next-frame retry ran out silently, and the interrupt vanished
from the live view until a refresh — while the spinner kept going.
`findResponseMessageIndex` now accepts the pane's OWN placeholder
identities as an explicit fallback (never a positional guess, per the
regenerate rule) and both the steer and activity-label resolvers
delegate to it. `useResumableSSE` passes the submission's placeholder id
and the padded user-message id, read at call time so the `created`
reassignment is honored. The rename copies placeholder content forward,
so the part rides into the renamed row.
* 🧭 fix: Carry an Early Steer Through the Rename for Regenerates and Edited Resubmissions
A regenerate seeds the renamed response from `submission.initialResponse`
rather than the store tail, and an edited resubmission seeds from that
object's content, so a steer landed on the placeholder only in the store
was dropped by the first run step's rename. `syncSubmissionPlaceholder`
keeps the submission the step handler receives in step with the
placeholder this pane mutated.
The steer also claims the server-local index space that run steps and
labels already shift past the retained edit prefix; it now shifts the
same way through one shared `editPrefixLength()` instead of landing
inside the kept content. Codex round 1 (P2) on #15692.
* 🔤 fix: Decode percent-encoded S3 keys so non-ASCII filenames are readable extractKeyFromS3Url returns `URL.pathname` as the S3 object key without decoding it. The AWS SDK percent-encodes the Key again when it signs the request, so a file stored under `Ársreikningur.pdf` is fetched as `%C3%81rsreikningur.pdf` and every read fails with NoSuchKey. ASCII keys are byte-identical either way, which is why this only appears for non-English filenames. Decode keys derived from a URL path in all three branches (path-style endpoint, bucket-in-path, virtual-hosted). A key passed in raw — not a URL — still returns untouched, and a malformed escape sequence falls back to the raw value with a warning rather than throwing. * 🔗 fix: Percent-encode CloudFront URL keys so both URL forms agree buildCloudFrontUrl interpolated the raw S3 key into the URL while SDK-generated S3 URLs carry an encoded one, so the two forms of `file.filepath` disagreed about what a `%` means. `assertS3FileName` permits `%`, so a key containing the literal text `report%20final.pdf` produced a CloudFront URL indistinguishable from one for a key containing a space — and decoding on extraction would then target the wrong object on read, re-sign, and delete. Encoding each path segment here (separators stay literal) makes both producers consistent, which is what lets extractKeyFromS3Url decode unconditionally. * style: sort imports in cloudfront/crud.ts (pre-existing drift) The changed-file import-sort gate flags this file; the drift predates this PR (the untouched upstream version fails the same check). Kept as its own commit so it does not obscure the fix. * 🔏 fix: Encode the CloudFront Invalidation Path Like the Viewer URL `buildCloudFrontUrl` now percent-encodes each key segment, so the cached viewer path for a key with a literal `%` or a non-ASCII character is the encoded form. `deleteFileFromCloudFront` still handed the raw key to `CreateInvalidationCommand`, so the invalidation no longer matched the cached object and deleted content stayed served until the entry expired. Both producers now share one `encodeKeyPath` helper. Also asserts that `getS3FileStream` sends the decoded key to `GetObjectCommand`, which is the call that actually failed with `NoSuchKey`, rather than only checking the extractor. Co-authored-by: dinershtein <228485+dinershtein@users.noreply.github.com> --------- Co-authored-by: Danny Avila <danny@librechat.ai> Co-authored-by: dinershtein <228485+dinershtein@users.noreply.github.com>
#15686) The DocumentDB compatibility guard flagged Mongoose per-document save-condition bag reads and writes (document.$where) in tenantIsolation.ts as the unsupported $where operator, leaving dev red on Tests: data-schemas. The guard now judges a dotted $where only where the syntax is unambiguous: a call in any form, or code assigned through any operator or wrapper, is an offense; a read or a non-literal assignment is not claimed, with filter.$where = predicate stated as the one declared limit and the method sweep and live cluster run as its backstop. Every other way of writing the operator remains an offense. unwrapExpression also peels angle-bracket assertions, closing a gap in pipeline-update detection.
#15695) S3 and CloudFront records have carried `storageKey` since #12987, yet six readers still handed `file.filepath` to `getDownloadStream` and re-derived the key by parsing a presigned or CDN URL, while four others had grown their own `storageKey || filepath` expression. One resolver now serves all ten: `resolveDownloadPath` returns the recorded key when present and the path otherwise, so records without a key (local, Firebase, Azure, code output) behave exactly as before, and the share route keeps its local-only query-string strip on top. `resolveStoredS3Key` reuses the same `StoredFileRef` type. Covered by unit cases for the resolver and an S3 case proving a record with a key streams correctly even when its stored URL no longer parses. Closes #15693
* fix: enforce FILE_SEARCH role permission server-side
The FILE_SEARCH role permission is stored, served by GET /api/roles/:name and
settable through the admin API, but nothing ever checks it.
PermissionTypes.FILE_SEARCH occurs zero times under api/server; the three
occurrences in @librechat/api are all in the interface-to-role sync, which
writes the permission rather than checking it.
Measured on v0.8.8-rc1 and confirmed unchanged in rc2: a user whose role has
FILE_SEARCH.USE = false can still upload a document with
tool_resource=file_search and gets 200 with "embedded": true. The same user
calling execute_code correctly gets 403.
Two gates, both mirroring toolAccessPermType in ~/server/controllers/tools.js:
* Upload (api/server/routes/files/files.js): a tool-resource-to-permission map
checked before any branching, so it also covers the assistants path. Returns
403 with the same log line as the RUN_CODE gate. execute_code is included
because it had the same hole: the tool CALL was gated, the upload was not.
* Tool loading (api/app/clients/tools/util/handleTools.js): checked outside the
lazy loader so a denied user never gets the tool equipped, rather than one
that fails when called. Fails closed if the permission check itself throws.
All four required imports were already present in that file, next to an
existing FILE_CITATIONS check.
Both changes live in /api (JavaScript) rather than /packages/api (TypeScript)
as CLAUDE.md prefers for new backend code: each mirrors an existing pattern in
exactly these files, and a second permission gate in another language and
package would be harder to keep in step with the first.
Tests: api/app/clients/tools/util/handleTools.fileSearchPermission.test.js
covers permit, deny, the log line, and the fail-closed path.
* fixup! fix: enforce FILE_SEARCH role permission server-side
* 🔐 fix: Gate `file_search` and `execute_code` on Role Permissions
The `FILE_SEARCH` gate added in the previous commit lives in
`handleTools.loadTools`, which `loadAgentTools` only reaches when
`definitionsOnly` is false. That default is false on the chat path
(`Endpoints/agents/initialize.js`) but true on the responses and
OpenAI-compatible controllers, so on those paths a denied user still got
the tool definition advertised to the model and their files primed by
`primeSearchFiles`, and the call failed later in `loadToolsForExecution` —
the "gets one that fails when called" outcome the gate set out to avoid.
Move the decision to the capability filters that both loaders already run,
mirroring how `canUseMCP` resolves the `MCP_SERVERS` permission ahead of the
synchronous filter. `AgentCapabilities` stays the instance-wide deployment
switch; the role grant is a second condition a tool has to clear. Because
`hasFileSearch`/`hasExecuteCode` derive from the filtered list, priming is
skipped for a denied user too.
`RUN_CODE` had the same hole: `toolAccessPermType` in
`~/server/controllers/tools.js` gates only `POST /tools/:toolId/call`, not
the agent run, so a role without `RUN_CODE.USE` could still execute code
through an agent. Both tools go through the same map.
Checks use `checkAccessWithRequestCache` and run only for tools whose
capability is already enabled, so a run costs at most one role read. A check
that throws denies the tool.
Also formats the test file added in the previous commit, which Prettier
rejects as-is.
* 🔐 fix: Close the Remaining `RUN_CODE` and `FILE_SEARCH` Bypasses
Codex review of the previous commit found four ways past the gates. All four
share a cause: the permission map was copied per call site, so every boundary
that was not copied into stayed open.
Moves the maps and the check into `packages/api/src/tools/rolePermissions.ts`,
per CLAUDE.md ("all new backend code must be TypeScript in `/packages/api`",
"keep `/api` changes to the absolute minimum"), and points the four `/api`
boundaries at it.
- `handleTools.loadTools` now gates every tool in `toolRolePermissions`, not
just `file_search`. It is the shared boundary the Assistants required-action
flow crosses via `processRequiredActions`, which never passes the agent
capability filter — so a legacy Assistant with a raw function named
`execute_code` ran arbitrary code for a role denied `RUN_CODE`.
- The check is now request-cached (`checkAccessWithRequestCache`). The previous
`checkAccess` call omitted `req`, so the agent path resolved the same grant
twice and issued a second serial `getRoleByName` during chat startup, against
the rule in AGENTS.md.
- `codeExecutionEnabled` is gated by the role result at all three loaders, not
only the `execute_code` entry in the filtered list. It drives tool
classification and the programmatic bash tool, so an agent pairing
`execute_code` with an MCP `code_execution` tool still advertised and
instantiated `run_tools_with_bash` for a denied role.
- The upload map covers `EToolResources.code_interpreter`, which the Assistants
builder posts instead of `execute_code`, and native `code_interpreter` /
`file_search` tools are dropped when an assistant is created or updated.
Those run inside the provider and reach neither loader, so configuration time
is the only place they can be gated.
Renames `handleTools.fileSearchPermission.test.js` to
`handleTools.rolePermissions.test.js` — it now covers both tools.
* 🔐 fix: Gate the Image Upload Route and the v1 Assistant Writers
Second Codex round found the same shape again: boundaries the map had not been
wired into. Adds two shared helpers next to the maps in
`packages/api/src/tools/rolePermissions.ts` so a new upload handler or assistant
writer gets the check by calling one function.
- `/files/images` accepts `tool_resource` and routes agent uploads to
`processAgentFileUpload` on its own. The Code Files UI sends images there, so
every image was a way around the upload boundary for a role denied
`RUN_CODE`. Both upload routes now call
`checkToolResourceUploadPermission`.
- That helper passes `req`, so the role read joins the request cache. The
previous `checkAccess` call omitted it, and an allowed Assistants upload goes
on to `processFileUpload` → `addResourceFileId` → `updateAssistant`, whose own
check does pass `req` — two serial lookups per upload, against AGENTS.md.
- `/assistants/v1` is still mounted and its `createAssistant` / `patchAssistant`
write native tools straight to the provider. Both now filter through
`resolveAssistantToolPermissions`, which v2 also uses in place of its local
copy.
Run-time enforcement for native assistant tools is deliberately not here: it
means overriding `body.tools` at `createRun` in both chat controllers, which
changes run semantics, and is better argued on its own.
* 🔐 fix: Gate Legacy `retrieval` and the Code-Environment File Tools
Third Codex round, same shape twice more.
- The v1 assistant builder submits `{ type: 'retrieval' }` where v2 submits
`file_search` (`AssistantPanel.tsx:203`). Only the v2 spelling was mapped, so
`resolveAssistantToolPermissions` treated legacy retrieval as ungated and both
v1 writers preserved it for a role denied `FILE_SEARCH`. Both spellings now
answer to the same grant.
- `codeEnvAvailable` in the agent initializer was capability-only, and
`initializeAgent` rebuilds `bash_tool`, `read_file` and the workspace file
tools from it — after the tool loader has already dropped `execute_code` for a
denied role. The bash gate stops command execution, but those handlers still
read, search, create and edit files in an attached code environment. The flag
now carries the role grant, and short-circuits before the role read when the
deployment has the capability off.
* 🔐 fix: Resolve Tool Grants Once Per Request and Pair Every Capability Gate
Four review rounds found thirteen issues, all one class: a boundary the
permission check had not been copied into. Rounds 2 and 3 each found gaps
created by the previous round's fix, and a manual sweep of tool constructors
and upload routes still missed the `codeEnvAvailable` family entirely — because
the thing worth enumerating is not "paths that reach a tool" but "reads of the
capability".
There are 28 such reads. This replaces per-site permission checks with one
resolution per request, pairs every read that is a gate, and pins the list so a
new one cannot be added silently.
- `resolveToolRoleGrants` resolves `RUN_CODE` and `FILE_SEARCH` together and
memoizes on the request. Both perf findings disappear by construction: gates
share one role read instead of each issuing its own.
- Newly paired gates: `codeEnvAvailable` in the OpenAI-compatible, Responses and
memory-agent initializers (`initializeAgent` rebuilds `bash_tool`, `read_file`
and the workspace file tools from it, and hands their handlers the code
environment); the capability checks in agent upload processing and in
agent-management upload purposes; and the library chat-completion service,
behind an optional `getRoleByName` dependency so embedders are not broken.
- The v1 Knowledge upload posts `assistant_id` with no `tool_resource`, so there
is no resource to authorize. It now reads the assistant's own native tools and
requires their grants, instead of treating a missing resource as unrestricted.
- The startup role read moved off the critical path into the existing
`Promise.all`, and the direct tool-call check passes `req` so the shared
loader gate reuses its result rather than repeating the lookup.
- `toolCapabilityGates.spec.js` pins every file that reads either capability
with a count and a reason. A new read fails the suite until it is classified,
so the next boundary announces itself instead of waiting for a reviewer.
* 🔐 fix: Abort Assistant Writes on Role-Lookup Failure, Authorize Legacy Uploads First
Round 5 found no missed boundary — the inventory held. All three findings are
about how the gates behave.
- Failing closed is right where a denial blocks the operation, and wrong where a
denial instead filters a payload the caller persists. A transient role-store
outage during an assistant create or patch was silently stripping
`code_interpreter` / `file_search` / `retrieval` and saving the result, turning
a momentary failure into permanent config loss. `checkToolRolePermission`
gains `throwOnError`, and `resolveAssistantToolPermissions` uses it: every one
of its callers either persists the filtered list or rejects the request, so a
lookup failure has to propagate.
- The legacy-assistant upload gate ran after `sanitizedUploadFn` had already
pushed the file to the provider, so a denied role left an untracked remote file
and got a 500 for what is a 403. Moved to the route, next to the tool-resource
gate, before any bytes are sent.
- `resendFiles` hydration derived its resource set from unfiltered `agent.tools`,
so a denied role still paid the thread walk and code-file reads for a tool
about to be dropped. The `execute_code` half now keys off
`effectiveCodeEnvAvailable`, which already carries the grant.
The `file_search` half of that last one needs the grant threaded into
`initializeAgent`, which is plumbing in the most delicate file here for a latency
win on an already-correct path — deliberately left for a follow-up.
* 🧪 fix: Declare `resolveToolRoleGrants` in the Upload Spec's Module Stub
`Tests: api (shard 2/3)` failed on `a0b38c3`: `process.spec.js` stubs
`@librechat/api` wholesale, so the `resolveToolRoleGrants` call added to the
upload gate resolved to undefined and threw. Five failures in
`processAgentFileUpload`, plus one cascading `processFileURL` assertion from the
same suite.
The gate was verified through its own spec and the routes above it, but not
through the spec of the file it lives in — `process.spec.js` was never run
locally. Every spec belonging to a file this PR touches now has been, and the
mongodb-backed ones that still cannot run here were checked statically for the
same stub shape: all but `process.integration.spec.js` spread `requireActual`,
and that one never reaches these paths.
* ⚡ fix: Skip the Role Lookup When Code Execution Is Disabled
The browser initializer already guards this; the OpenAI-compatible and Responses
controllers did not, so every request on those endpoints issued a role read whose
result the capability short-circuit then discarded. Both now start the lookup
only when the deployment has `execute_code` enabled, matching the third.
* ⚡ fix: Drop Three Redundant Reads on the Gated Paths
- The memory initializer resolved grants unconditionally, so a deployment
without `execute_code` paid a role read for a flag that could only be false.
Guarded like the three chat initializers.
- Agent-management upload purposes resolved grants before `checkCapability`, so
a purpose the deployment has switched off paid a role read on the way to being
rejected. Capability first, then the grant.
- The legacy-assistant preflight builds an OpenAI client to read the assistant's
tools, and `processFileUpload` then built a second one — re-reading the user's
key expiry and values. The preflight now hands its client on, and processing
builds one only when it wasn't given one.
---------
Co-authored-by: Paul <200737214+SSIG-IT@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )