LibreChat

mirror of https://github.com/danny-avila/LibreChat.git synced 2026-08-04 14:57:42 +00:00

Author	SHA1	Message	Date
Artyom Bogachenko	5683706af5	🔐 feat: OIDC Bearer Token Authentication for Remote Agent API (#12450 ) * Remote Agent Auth middleware * consider migration and update user * fix eslint errors * add scope validation * fix codex review errors * add filter for use: sig * add jwks-rsa deps * Fix remote agent OIDC auth review findings * Polish remote agent OIDC timeout coverage * Reject remote OIDC tokens without subject * Use tenant context for remote agent auth config * Harden remote agent OIDC scope handling * Polish remote agent OIDC cache and scope tests * Resolve remote agent auth review comments * Reuse OpenID email claim resolver for remote auth * Skip empty OpenID email fallback claims * Use pre-auth tenant context for remote auth config * Downgrade expected OIDC fallback logging * Require secure remote OIDC endpoints * Polish remote agent auth edge cases * Enforce unique balance records * Bind remote OpenID users to issuer * Fix issuer-scoped OpenID indexes * Avoid unique balance index requirement * Fix remote OpenID issuer normalization boundaries * Require issuer-bound OpenID lookups * Enforce tenant API key policy after auth * Fix remote auth tenant policy types * Normalize remote OIDC discovery issuer * Allow normalized remote OIDC issuer validation * Enforce resolved tenant OIDC policy * Polish OpenID issuer and scope validation --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-05-04 17:06:35 -04:00
Danny Avila	4cce88be42	🪟 feat: Add allowedAddresses Exemption List For SSRF-Guarded Targets (#12933 ) * 🪟 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.	2026-05-03 21:43:59 -04:00
Danny Avila	5b5e2b0286	🛡️ fix: Handle MCP Tool Cache Lookup Failures (#12910 ) * Handle MCP tool cache lookup failures * Harden MCP cached tool lookup * Cover full MCP tool cache outage * Guard MCP tool cache store lookup	2026-05-02 09:21:28 +09:00
Helge Wiethoff	61b9b1daa7	🩹 fix(SSE): Treat `responseCode === 0` as Transport Failure, Not Server Error (#12834 ) * 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>	2026-04-29 10:05:51 +09:00
Danny Avila	35bf04b26c	🧰 refactor: Unify code-execution tools (#12767 ) * 🛠️ feat: Add registerCodeExecutionTools helper Idempotently registers `bash_tool` + `read_file` in the run's tool registry and tool-definition list via a registry `.has()` dedupe. Sets up the single code-execution tool path shared by: - `initializeAgent` (when an agent has `execute_code` in its tools and the capability is enabled for the run) - `injectSkillCatalog` (when skills are active; unconditional read_file, bash_tool follows `codeEnvAvailable`) Both callers reach the helper in the same initialization sequence, so the second call becomes a no-op and exactly one copy of each tool reaches the LLM — no more double registration for agents that combine `execute_code` capability with active skills. Unit-tested on a fresh run, idempotence (second call, overlap with prior tooldefs, partial overlap), and the no-registry variant. * 🔀 refactor: Route injectSkillCatalog bash_tool + read_file through registerCodeExecutionTools The `skill` tool is still registered inline (it's skill-path-specific), but `bash_tool` + `read_file` now flow through the shared idempotent helper so a prior registration from the execute_code path doesn't produce a duplicate copy later in the same run. Behavior preserved: - `read_file` always registers when any active skill is in scope — manually-primed `disable-model-invocation: true` skills still need it to load `references/` from storage. - `bash_tool` follows `codeEnvAvailable` exactly as before. Adds a test pinning the cross-call dedupe: when `injectSkillCatalog` runs AFTER `registerCodeExecutionTools` has already seeded the registry + tool definitions with bash_tool/read_file, the resulting `toolDefinitions` still contains exactly one copy of each. 🪄 feat: Expand `execute_code` tool name into bash_tool + read_file at initialize-time When an agent's `tools` include `execute_code` and the `execute_code` capability is enabled for the run, `initializeAgent` now registers `bash_tool` + `read_file` via `registerCodeExecutionTools` before `injectSkillCatalog`. The legacy `execute_code` tool definition is no longer handed to the LLM — `execute_code` remains on the agent document as a capability-trigger marker, but the runtime expands it into the skill-flavored tool pair. Call ordering matters: the `execute_code` registration runs BEFORE `injectSkillCatalog`, so the skill path's own `registerCodeExecutionTools` call inside `injectSkillCatalog` becomes a no-op via the registry's `.has()` check. Exactly one copy of each tool reaches the LLM whether the agent has: - only `execute_code` (legacy path) - only skills - both No data migration needed — `agent.tools: ['execute_code']` stays in the DB unchanged; the expansion is a runtime operation. Three tests cover the matrix: execute_code + capability on → bash_tool + read_file registered; execute_code + capability off → neither registered; no execute_code + capability on → neither registered. * 🗑️ refactor: Drop CodeExecutionToolDefinition from the builtin registry Removes the legacy `execute_code` entry from `agentToolDefinitions` and the corresponding import. With the initialize-time expansion in place, nothing consults `getToolDefinition('execute_code')` for a tool schema any more — the capability gate still filters on the string `execute_code`, but the actual tool definitions the LLM sees come from `registerCodeExecutionTools` (i.e. `bash_tool` + `read_file`). `loadToolDefinitions` in `packages/api/src/tools/definitions.ts` silently drops `execute_code` when it no longer resolves in the registry — that's the expected path and is now covered by an updated test. No caller of `getToolDefinition('execute_code')` expects a non-undefined result after this change. * 🔌 refactor: Read CODE_API_KEY from env for primeCodeFiles + PTC Finishes the Phase 4 server-env-keyed rollout on the two remaining `loadAuthValues({ authFields: [EnvVar.CODE_API_KEY] })` sites in `ToolService.js`: - `primeCodeFiles` (user-attached file priming on execute_code agents) - Programmatic Tool Calling (`createProgrammaticToolCallingTool`) Both now read `process.env[EnvVar.CODE_API_KEY]` directly, matching `bash_tool`'s pattern. The per-user plugin-auth path is no longer consulted for code-env credentials anywhere in the hot path — the agents library owns the actual tool-call execution and also reads the env var internally. Priming still fires for existing user-file workflows so the legacy `toolContextMap[execute_code]` hint ("files available at /mnt/data/...") stays in the prompt; only the key lookup changed. * 🔧 fix: Type the pre-seeded dedupe-test tools as LCTool CI TypeScript type checks caught `{ parameters: {} }` in the new cross-call dedupe test: `LCTool.parameters` is a `JsonSchemaType`, not `{}`. Use `{ type: 'object', properties: {} }` and type the local registry Map through the parameter-derived shape so the pre-seeded values match what `toolRegistry.set` expects. * 🛡️ fix: Run execute_code expansion before GOOGLE_TOOL_CONFLICT gate Codex review caught a latent regression: the original Phase 8 placement ran `registerCodeExecutionTools` after `hasAgentTools` was computed, so an execute-code-only agent on Google/Vertex with provider-specific `options.tools` populated would no longer trip `GOOGLE_TOOL_CONFLICT` — the legacy `CodeExecutionToolDefinition` used to populate `toolDefinitions` before the guard, but after dropping it from the registry, `toolDefinitions` stayed empty until my expansion ran downstream of the guard. Mixed provider + agent tools would silently flow through to the LLM. Fix moves the `execute_code` expansion to BEFORE `hasAgentTools` computation. `bash_tool` + `read_file` now contribute to the check the same way the legacy `execute_code` def did. Covered by a new test that pins the Google+execute_code+provider-tools scenario — the `rejects.toThrow(/google_tool_conflict/)` path would have silently passed on the prior placement. * 🔗 fix: Thread codeEnvAvailable through handoff sub-agents Round-2 codex review caught the other half of the execute_code expansion gap: `discoverConnectedAgents` omitted `codeEnvAvailable` from its forwarded `initializeAgent` params, so handoff sub-agents with `agent.tools: ['execute_code']` lost the `bash_tool` + `read_file` registration (pre-Phase 8 the legacy `CodeExecutionToolDefinition` would have landed in their `toolDefinitions` via the registry). - Add `codeEnvAvailable?` to `DiscoverConnectedAgentsParams` and forward it verbatim on every sub-agent `initializeAgent` call. - Update the three JS call sites that construct the primary's `codeEnvAvailable` (`services/Endpoints/agents/initialize.js`, `controllers/agents/openai.js`, `controllers/agents/responses.js`) to pass the same flag into `discoverConnectedAgents` — one authoritative source per request. - Two regression tests in `discovery.spec.ts` pin the true/false passthrough so a future refactor that drops the param-forwarding surfaces immediately. Left intentionally unchanged: `packages/api/src/agents/openai/service.ts` (public API helper with no in-repo caller). External consumers of `createAgentChatCompletion` who want code execution should pass a `codeEnvAvailable`-aware `initializeAgent` via `deps` — documenting the full public-API surface is out of scope for this Phase 8 PR. * 🔗 fix: Thread codeEnvAvailable through addedConvo + memory-agent paths Round-3 codex review caught the last two production `initializeAgent` callers missing the Phase-8 capability flag: - `api/server/services/Endpoints/agents/addedConvo.js` (multi-convo parallel agent execution). Added `codeEnvAvailable` to `processAddedConvo`'s destructured params and forwarded it into the per-added-agent `initializeAgent` call. Caller in `api/server/services/Endpoints/agents/initialize.js` passes the same `codeEnvAvailable` it computed for the primary. - `api/server/controllers/agents/client.js` (`useMemory` — memory extraction agent). Computes its own `codeEnvAvailable` from `appConfig?.endpoints?.[EModelEndpoint.agents]?.capabilities` and forwards into `initializeAgent`. Memory agents rarely list `execute_code`, but if one does, pre-Phase 8 they got the legacy `execute_code` tool registered unconditionally — the passthrough restores parity. With this, every production caller of `initializeAgent` explicitly resolves the capability: main chat flow (primary + handoff), OpenAI chat completions (primary + handoff), Responses API (primary + handoff), added convo parallel agents, and memory agents. The one remaining caller, `packages/api/src/agents/openai/service.ts::createAgentChatCompletion`, is a public API helper with no in-repo consumer (external callers must pass a capability-aware `initializeAgent` via `deps`). * 🪤 fix: Remove duplicate appConfig declaration causing TDZ ReferenceError The Responses API controller had TWO `const appConfig = req.config;` bindings inside `createResponse`: one at the top of the function (added by the Phase 4 `bash_tool` decouple) and one inside the try block (added by the polish PR #12760). Because `const` is block-scoped with a temporal dead zone, the inner redeclaration put `appConfig` in TDZ for the entire try block, so any earlier reference inside the try — notably `appConfig?.endpoints?.[EModelEndpoint.agents]?.allowedProviders` at line 348 — threw `ReferenceError: Cannot access 'appConfig' before initialization`. The error was silently swallowed by the outer try/catch, leaving `recordCollectedUsage` unreached and the six `responses.unit.spec.js` token-usage tests failing. Removing the inner redeclaration fixes the six failing tests (verified: 11/11 pass locally post-fix, 0 regressions elsewhere). The outer function-scoped binding already provides `appConfig` to every downstream reference. * 🔗 fix: Thread codeEnvAvailable through the OpenAI chat-completion public API Round-4 codex review (legitimate on the type-safety angle, even though the runtime concern was already covered): the `createAgentChatCompletion` helper defines its own narrower `InitializeAgentParams` interface locally, and the type was missing `codeEnvAvailable`. External consumers who supply a capability-aware `deps.initializeAgent` couldn't route `codeEnvAvailable` through without a type-cast workaround. - Widen the local `InitializeAgentParams` interface to include `codeEnvAvailable?: boolean` (matches the real `packages/api/src/agents/initialize.ts` type). - Derive `codeEnvAvailable` inside `createAgentChatCompletion` from `deps.appConfig?.endpoints?.agents?.capabilities` (the same source the in-repo controllers use) and forward to `deps.initializeAgent`. Uses a string literal `'execute_code'` lookup so this file stays free of a `librechat-data-provider` import — keeping the dependency surface of the public helper minimal. With this, external consumers of `createAgentChatCompletion` who pass `appConfig` with the agents capabilities get `bash_tool` + `read_file` registration automatically; consumers who don't pass `appConfig` retain the existing "explicit opt-in" semantics (the flag stays `undefined`, expansion is skipped). * 🧹 chore: Review-driven polish — observability log, JSDoc DRY, test gaps, no-op allocation Addresses the comprehensive review of PR #12767: - Finding #1 (MINOR, observability): `initializeAgent` now emits a debug log when an agent lists `execute_code` in its tools but the runtime gate is off (`params.codeEnvAvailable` !== true). The event-driven `loadToolDefinitionsWrapper` path doesn't log capability-disabled warnings, so without this the tool silently vanishes from the LLM's definitions with zero trace. Operators debugging "why isn't code interpreter working?" now get a signal at the initialize layer. - Finding #5 (NIT, allocation): `registerCodeExecutionTools` now returns the input `toolDefinitions` array by reference on the no-op path (both tools already registered by a prior caller in the same run) instead of allocating a fresh spread array every time. The common dual-call scenario — `initializeAgent` then `injectSkillCatalog` — saves one O(n) copy per request. - Finding #4 (NIT, DRY): Collapsed the duplicated 6-line JSDoc comment in `openai.js`, `responses.js`, and `addedConvo.js` into either a one-line `@see DiscoverConnectedAgentsParams.codeEnvAvailable` pointer (the two JS call sites) or a compact 3-line block referring back to the canonical source (addedConvo's @param). - Finding #2 (MINOR, test gap): Added `api/server/services/Endpoints/agents/addedConvo.spec.js` with three cases covering `codeEnvAvailable=true`, `codeEnvAvailable=false`, and omitted (undefined) passthrough. A future refactor that drops the param from destructuring now surfaces here instead of silently regressing multi-convo parallel agents with `execute_code`. - Finding #3 (MINOR, test gap): Added `api/server/controllers/agents/__tests__/client.memory.spec.js` pinning the capability-flag derivation that `AgentClient::useMemory` uses — six cases covering present/absent/null/undefined config shapes plus an enum-literal pin (`'execute_code'` / `'agents'`). Catches enum renames or config-path shifts that would otherwise silently strip `bash_tool` + `read_file` from memory agents. Finding #7 (jest.mock scoping, confidence 40) left as-is: the reviewer's own risk assessment noted `buildToolSet` doesn't touch the mocked exports, and restructuring a file-level `jest.mock` to `jest.doMock` + dynamic `import()` introduces more complexity than the speculative risk justifies. The existing mock is scoped to the test file and contains the same stubs the adjacent `skills.test.ts` already uses. Finding #6 (PR description commit count) addressed out-of-band via PR description update. All existing tests pass, typecheck clean, lint clean across touched files. New tests: 9 cases across 2 new spec files. * 🧽 refactor: Replace hardcoded 'execute_code' string with AgentCapabilities enum in service.ts Follow-up review (conf 55) caught that `openai/service.ts`'s Phase 8 `codeEnvAvailable` derivation used the literal `'execute_code'` while every in-repo controller uses `AgentCapabilities.execute_code` from `librechat-data-provider`. The file deliberately uses local type interfaces to keep the public API helper's type surface small, but that pattern was never a ban on single-value imports from the data provider — `packages/api` already depends on it. Importing the enum value means a future rename of `AgentCapabilities.execute_code` propagates to this file automatically, matching the in-repo controllers' behavior. Other follow-up findings left as-is per the reviewer's own verdict: - #2 (memory spec mirrors the production expression rather than calling `AgentClient::useMemory` directly): reviewer flagged as "not blocking" / "design-philosophy observation." The test file's JSDoc already explicitly documents the tradeoff and pins the enum literals to catch the most likely drift vector. Standing up `AgentClient` + all its mocks for a one-line regression guard is disproportionate. - #3 (`addedConvo.spec.js` mock signature vs. underlying `loadAddedAgent` arity): reviewer's own confidence 25 noted the mock matches the wrapper's actual call pattern in the production file. Not a real gap. - #4 was self-retracted as a false alarm. * 🗑️ refactor: Fully deprecate CODE_API_KEY — remove all LibreChat-side references The code-execution sandbox no longer authenticates via a per-run `CODE_API_KEY` (frontend or backend). Auth moved server-side into the agents library / sandbox service, so LibreChat drops every reference: Backend plumbing: - `api/server/services/Files/Code/crud.js`: `getCodeOutputDownloadStream`, `uploadCodeEnvFile`, `batchUploadCodeEnvFiles` no longer accept `apiKey` or send the `X-API-Key` header. - `api/server/services/Files/Code/process.js`: `processCodeOutput`, `getSessionInfo`, `primeFiles` drop the `apiKey` param throughout. - `api/server/services/ToolService.js`: stop reading `process.env[EnvVar.CODE_API_KEY]` for `primeCodeFiles` and PTC; the agents library handles auth internally. Remove the now-dead `loadAuthValues` + `EnvVar` imports. Drop the misleading "LIBRECHAT_CODE_API_KEY" hint from the bash_tool error log. - `api/server/services/Files/process.js`: remove the `loadAuthValues` call around `uploadCodeEnvFile`. - `api/server/routes/files/files.js`: code-env file download no longer fetches a per-user key. - `api/server/controllers/tools.js`: `execute_code` is no longer a tool that needs verifyToolAuth with `[EnvVar.CODE_API_KEY]` — the endpoint always reports system-authenticated so the client skips the key-entry dialog. `processCodeOutput` called without `apiKey`. - `api/server/controllers/agents/callbacks.js`: `processCodeOutput` invoked without the loadAuthValues round trip, for both LegacyHandler and Responses-API handlers. - `api/app/clients/tools/util/handleTools.js`: `createCodeExecutionTool` called with just `user_id` + files. packages/api: - `packages/api/src/agents/skillFiles.ts`: `PrimeSkillFilesParams`, `PrimeInvokedSkillsDeps`, `primeSkillFiles`, `primeInvokedSkills` all drop the `apiKey` param; the gate is purely `codeEnvAvailable`. - `packages/api/src/agents/handlers.ts`: `handleSkillToolCall` drops the `process.env[EnvVar.CODE_API_KEY]` read; skill-file priming is now gated solely on `codeEnvAvailable`. `ToolExecuteOptions` signatures drop apiKey from `batchUploadCodeEnvFiles` and `getSessionInfo`. - `packages/api/src/agents/skillConfigurable.ts`: JSDoc no longer references the env var. - `packages/api/src/tools/classification.ts`: PTC creation no longer gated on `loadAuthValues`; `buildToolClassification` drops the `loadAuthValues` dep entirely (no LibreChat-side callers need it for this path anymore). - `packages/api/src/tools/definitions.ts`: `LoadToolDefinitionsDeps` drops the `loadAuthValues` field. Frontend: - Delete `client/src/hooks/Plugins/useAuthCodeTool.ts`, `useCodeApiKeyForm.ts`, and `client/src/components/SidePanel/Agents/Code/ApiKeyDialog.tsx` — the install/revoke dialogs for CODE_API_KEY are fully dead. - `BadgeRowContext.tsx`: drop `codeApiKeyForm` from the context type and provider. `codeInterpreter` toggle treated as always authenticated (sandbox auth is server-side). - `ToolsDropdown.tsx`, `ToolDialogs.tsx`, `CodeInterpreter.tsx`, `RunCode.tsx`, `SidePanel/Agents/Code/Action.tsx` +`Form.tsx`: all API-key dialog trigger refs, "Configure code interpreter" gear buttons, and auth-verification plumbing removed. The "Code Interpreter" toggle is now a plain `AgentCapabilities.execute_code` checkbox — no key-entry gate. - `client/src/locales/en/translation.json`: drop the three `com_ui_librechat_code_api` keys and `com_ui_add_code_interpreter_api_key`. Other locales are externally automated per CLAUDE.md. Config:* - `.env.example`: remove the `# LIBRECHAT_CODE_API_KEY=your-key` section and its header. Tests: - `crud.spec.js`: assertions flipped to pin "no X-API-Key header" and "no apiKey param". - `skillFiles.spec.ts`: removed env-var save/restore; tests now pin that the batch-upload path is gated solely on `codeEnvAvailable` and that no apiKey is threaded through. - `handlers.spec.ts`: same — just the `codeEnvAvailable` gate pins remain. - `classification.spec.ts`: remove the two tests that asserted `loadAuthValues` was (not) called for PTC. - `definitions.spec.ts`: drop every `loadAuthValues: mockLoadAuthValues` entry from the deps shape. - `process.spec.js`: strip the mock of `EnvVar.CODE_API_KEY`. Comment hygiene: - `tools.ts`, `initialize.ts`, `registry/definitions.ts`: shortened stale comment references to "legacy `execute_code` tool" without naming the retired env var. Tests verified: 678 packages/api tests pass, 836 backend api tests pass. Typecheck clean, lint clean. Only remaining CODE_API_KEY mentions in the code are two regression-guard assertions: - `crud.spec.js`: pins "no X-API-Key header" stays absent. - `skillConfigurable.spec.ts`: pins `configurable` never grows a `codeApiKey` field. * 🧹 chore: Remove the last two CODE_API_KEY name mentions in LibreChat Follow-up to the prior full deprecation commit: two tests still named the retired identifier in their regression-guard assertions. - `packages/api/src/agents/skillConfigurable.spec.ts`: drop the "does not inject a codeApiKey key" test. The `codeApiKey` field is gone from the production configurable shape, so an absence-assertion naming it re-introduces the retired identifier in code. - `api/server/services/Files/Code/crud.spec.js`: rename the "without an X-API-Key header" case back to "should request stream response from the correct URL" and drop the `expect(headers).not.toHaveProperty('X-API-Key')` assertion. The surrounding request-shape checks (URL, timeout, responseType) still pin the behavior; the explicit header-absence line was named-after the deprecated contract. Result: `grep -rn "CODE_API_KEY\\|codeApiKey\\|LIBRECHAT_CODE_API_KEY"` against the LibreChat source tree returns zero hits. The only remaining `X-API-Key` strings in this repo are on unrelated OpenAPI Action + MCP server auth configurations, where the string is user-facing config, not a LibreChat-owned identifier. Tests: 677 packages/api pass (2 pre-existing summarization e2e failures unrelated); 126 api-workspace controller/service tests pass. Typecheck and lint clean. * 🎯 fix: Narrow codeEnvAvailable to per-agent (admin cap AND agent.tools) Before this commit, `codeEnvAvailable` was computed in the three JS controllers as the admin-level capability flag only (`enabledCapabilities.has(AgentCapabilities.execute_code)`) and passed through `initializeAgent` → `injectSkillCatalog` / `primeInvokedSkills` / `enrichWithSkillConfigurable` unchanged. A skills-only agent whose `tools` array didn't include `execute_code` still got `bash_tool` registered (via `injectSkillCatalog`) and skill files re-primed to the sandbox on every turn — wrong, because the agent never opted in to code execution. Fix: `initializeAgent` now computes the per-agent effective value once as `params.codeEnvAvailable === true && agent.tools.includes(Tools.execute_code)`, reuses the same boolean for: 1. The `execute_code` → `bash_tool + read_file` expansion gate (previously already consulted `agent.tools`; now shares the single `effectiveCodeEnvAvailable` binding). 2. The `injectSkillCatalog` call (previously got the raw admin flag). 3. The returned `InitializedAgent.codeEnvAvailable` field (new, typed as required boolean). Controllers (initialize.js, openai.js, responses.js): store `primaryConfig.codeEnvAvailable` in `agentToolContexts.set(primaryId, ...)`, capture `config.codeEnvAvailable` in every handoff `onAgentInitialized` callback, and read it from the per-agent ctx inside the `toolExecuteOptions.loadTools` runtime closure. The hoisted `const codeEnvAvailable = enabledCapabilities.has(...)` locals in the two OpenAI-compat controllers are gone — they were shadowing the narrowed per-agent value. primeInvokedSkills: `handlePrimeInvokedSkills` in `services/Endpoints/agents/initialize.js` now uses `primaryConfig.codeEnvAvailable` (per-agent, narrowed) instead of the raw admin flag. A skills-only primary agent won't re-prime historical skill files to the sandbox even when the admin enabled the capability globally. Efficiency: one extra `&&` in `initializeAgent`. No runtime hot-path cost — the `includes()` scan on `agent.tools` was already happening for the `execute_code` expansion gate; it's now just bound to a local. Tool execution closures read `ctx.codeEnvAvailable === true` (property access + strict equality, O(1)). Ephemeral-agent note: per-agent narrowing is authoritative for both persisted and ephemeral flows. The ephemeral toggle (`ephemeralAgent.execute_code`) is reconciled into `agent.tools` upstream in `packages/api/src/agents/added.ts`, so `agent.tools.includes('execute_code')` is the single source of truth by the time `initializeAgent` runs. Tests: two new regression tests pin the narrowing contract: - `initialize.test.ts` — four-quadrant matrix on `InitializedAgent.codeEnvAvailable` (cap on × agent asks, cap on × doesn't ask, cap off × asks, neither). Catches future refactors that drop either half of the AND. - `skills.test.ts` — `injectSkillCatalog` with `codeEnvAvailable: false` against an active skill catalog must NOT register `bash_tool` even though it still registers `read_file` + `skill`. This is the state a skills-only agent gets post-narrowing. All 191 affected packages/api tests pass + 836 backend api tests pass. Typecheck clean, lint clean. * 🧽 refactor: Comprehensive-review polish — hoist tool defs, pin verifyToolAuth contract, doc appConfig Addresses the comprehensive review of Phase 8. Findings mapped: #1 (MINOR): `verifyToolAuth` unconditional auth for execute_code - Added doc comment explicitly stating the deployment contract (admin capability → reachable sandbox; no per-check health probe to keep UI-gate queries O(1)). - New `api/server/controllers/__tests__/tools.verifyToolAuth.spec.js` with 4 regression tests pinning the contract: 1. `authenticated: true` + `SYSTEM_DEFINED` for execute_code. 2. 404 for unknown tool IDs. 3. `loadAuthValues` is never consulted (catches a future revert that would resurface the per-user key-entry dialog). 4. Response `message` is never `USER_PROVIDED`. #2 (MINOR): `openai/service.ts` undocumented `appConfig` dependency - Expanded the `ChatCompletionDependencies.appConfig` JSDoc to spell out that omitting it silently disables code execution for agents with `execute_code` in their tools. External consumers of `createAgentChatCompletion` now have the contract documented at the type boundary. #5 (NIT): `registerCodeExecutionTools` re-allocates tool defs - Hoisted `READ_FILE_DEF` and `BASH_TOOL_DEF` to module-level `Object.freeze`d constants. The shapes derive entirely from static `@librechat/agents` exports, so a single frozen object per tool is safe to share across every agent init. Eliminates the ~4-property allocations on every call (including the common second-call no-op path). #6 (NIT): Verbose history-priming comment in initialize.js - Trimmed the 16-line `handlePrimeInvokedSkills` block to a 5-line summary with `@see InitializedAgent.codeEnvAvailable` pointer. The canonical narrowing explanation lives on the type; the controller comment is just the ACL-vs-capability rationale. Skipped: - #3 (memory spec tests a mirror function): reviewer self-dismissed as a design tradeoff; the enum-literal pin already catches the highest-risk drift vector. - #4 (cross-repo contract for `createCodeExecutionTool`): user will explicitly install the latest `@librechat/agents` dev version once the companion PR publishes, so the version pin will be authoritative. - #7 (migration/deprecation note for self-hosters): out of scope per user direction — release notes handle this. Tests verified: 679 packages/api + 840 backend api tests pass. Typecheck + lint clean. * 🔧 chore: Update @librechat/agents version to 3.1.68-dev.1 across package-lock and package.json files This commit updates the version of the `@librechat/agents` package from `3.1.68-dev.0` to `3.1.68-dev.1` in the `package-lock.json` and relevant `package.json` files. This change ensures consistency across the project and incorporates any updates or fixes from the new version.	2026-04-25 04:02:01 -04:00
Danny Avila	9225a279eb	🎚️ feat: Per-User Skill Active/Inactive Toggle with Ownership-Aware Defaults (#12692 ) * feat: per-user skill active/inactive toggle with ownership-aware defaults - Add `skillStates` map (Record<string, boolean>) to user schema for per-user active/inactive overrides on skills - Add `defaultActiveOnShare` to interface.skills config (default: false) so admins can control whether shared skills auto-activate - Add GET/POST /api/user/settings/skills/active endpoints with validation - Add React Query hooks with optimistic mutations for skill states - Add useSkillActiveState hook with ownership-aware resolution: owned skills default active, shared skills default inactive - Add toggle switch UI to SkillListItem and SkillDetail components - Filter inactive skills in injectSkillCatalog before agent injection - Add localization keys for active/inactive labels * fix: use Record instead of Map for IUser.skillStates Mongoose .lean() flattens Map to a plain object, causing type incompatibility with IUser in methods that return lean documents. * fix: address review findings for skill active states - Fail-closed when userId is absent: filter rejects all shared skills instead of passing them through unfiltered (Codex P1) - Validate Mongoose Map key characters (reject . and $) in controller to return 400 instead of a 500 from schema validation (Codex P2) - Block toggle while initial skill states query is loading to prevent overwriting server-side overrides with an empty snapshot (Codex P2) - Extract shared SkillToggle component, eliminating duplicate toggle markup in SkillListItem and SkillDetail (Finding #3) - Move skill state query/mutation hooks from Favorites.ts to Skills/queries.ts per feature-directory convention (Finding #4) - Fix hardcoded English aria-label in SkillListItem by passing the localized string from the parent SkillList (Finding #5) - Fix inline arrow in SkillList render loop: pass stable callback reference so SkillListItem memo() is not invalidated (Finding #1) - Extract toRecord() helper in controller to DRY the Map-to-Object conversion (Finding #6) - Remove Promise.resolve wrapping synchronous config read (Finding #8) - Remove unused TUpdateSkillStatesRequest type (Finding #12) * fix: forward tabIndex on SkillToggle to preserve list keyboard nav The original inline toggle had tabIndex={-1} so the row itself remained the sole tab target. The extraction into SkillToggle dropped this prop, making every list toggle a tab stop. Add an optional tabIndex prop and pass -1 from SkillListItem. * fix: plumb skillStates to all agent entry points, isolate toggle keydown - Add skillStates/defaultActiveOnShare loading to openai.js and responses.js controllers so shared-skill activation is respected across all agent entry points, not just initialize.js (Codex P1) - Stop keydown propagation on SkillToggle so Enter/Space does not bubble to the parent row's navigation handler (Codex P2) * fix: paginate catalog fetch and serialize toggle writes - Paginate listSkillsByAccess (up to 10 pages of 100) until the active catalog quota is filled, so inactive shared skills in recent positions do not starve active owned skills past the first page (Codex P1) - Extend listSkillsByAccess interface with cursor/has_more/after for catalog pagination - Serialize skill-state writes via a ref queue: one in-flight request at a time, with the latest desired state sent when the previous one settles. Prevents last-response-wins races where an older request overwrites newer toggles (Codex P2) * fix: share write queue across hook instances, block toggle on fetch error - Move the write queue from a per-instance useRef to a module-scoped object so every mount of useSkillActiveState (SkillList, SkillDetail, etc.) serializes against the same in-flight slot. Prior per-instance queues allowed two components to race full-map POSTs (Codex P1) - Extend the toggle guard beyond isLoading: also block when isError is true or data is undefined. Prevents a failed GET from seeding a toggle with an empty baseline that would wipe server-side overrides on the next successful POST (Codex P1) * fix: stale closure, orphan cleanup, and cap-error UX - Read toggle baseline from React Query cache via queryClient.getQueryData instead of the captured skillStates closure. The closure can be stale between onMutate's setQueryData and the next render, so rapid successive toggles would build on old state and drop earlier changes (Codex P1) - Surface the MAX_SKILL_STATES_EXCEEDED error code with a specific toast key (com_ui_skill_states_limit) so users understand the 200-cap rather than seeing a generic error - Prune orphaned entries (skillIds whose Skill doc no longer exists) on both GET and POST in SkillStatesController. Self-heals over time without needing cascade-delete hooks or a migration job. Uses one indexed Skill._id query per request * test: pin skill active-state precedence with unit tests Extract the active-state resolution logic from a closure inside injectSkillCatalog into an exported resolveSkillActive helper, then cover every branch of the precedence matrix: - Fails closed when userId is absent (even with defaultActiveOnShare=true) - Explicit override wins over ownership and config (both true and false) - Owned skills default to active when no override is set - Shared skills default to defaultActiveOnShare value - Undefined skillStates behaves identically to an empty object - defaultActiveOnShare defaults to false when omitted - Owned skills ignore defaultActiveOnShare entirely Closes Finding #2 from the pre-rebase comprehensive review. Mirrors the existing scopeSkillIds test style; injectSkillCatalog now calls resolveSkillActive instead of inlining the closure. * refactor: limit skill active toggle to detail header, drop label - Remove the per-row toggle from SkillListItem and the active-state plumbing (hook call, isSkillEnabled/onToggleEnabled/toggleAriaLabel props) from SkillList. The detail view is now the single place to change a skill's active state - Drop dim/muted styling for inactive skills in the sidebar: without a control there, the visual indication has nowhere to land - Resize SkillToggle to match neighbor buttons: outer h-9 container, h-6 w-11 track with size-5 knob, no label span. The 'Active' / 'Inactive' text that accompanied the detail-view toggle is removed - Remove the now-unused label prop and tabIndex prop (the tabIndex existed only for the list-row context) from SkillToggle. Drop the onKeyDown stopPropagation for the same reason - Remove now-orphaned com_ui_skill_active / com_ui_skill_inactive translation keys * style: shrink SkillToggle track to h-5 w-9 with size-4 knob Container stays at h-9 to match neighbor button heights. The toggle track itself drops from h-6 w-11 to h-5 w-9, with a size-4 knob travelling 1.125rem on activation. Visually lighter inside the row. * fix: remove redundant skillStates entries that match the resolved default When a toggle lands on the ownership/config default, delete the key from the map instead of persisting `{id: defaultValue}`. Without this, a user toggling a skill off and back on would leave `{id: true}` for an owned skill (whose default is already true), silently consuming a slot against the 200-entry cap. Repeated round-trip toggles could exhaust the quota with zero meaningful overrides (Codex P2). Preserves the exceptions-list invariant that the runtime-resolution design depends on. * fix: prune before enforcing skill-state cap; reject non-ObjectId keys Reorder the update controller so pruneOrphans runs before the 200-cap check. Without this, a user near the cap with some orphaned entries (skills deleted since their last GET) could send a payload that would pass after pruning but gets rejected by the raw-size check first. Add a sanity cap on raw payload size (2 * MAX_SKILL_STATES) so abusive inputs do not reach the DB query, and enforce the real cap on the pruned result instead. Harden pruneOrphans: the earlier early-return path could pass non-ObjectId keys through unchanged. Now only valid ObjectIds are returned, and the Skill-model-unavailable fallback filters by format. Also add isValidObjectIdString validation at the input boundary so malformed (but otherwise non-Mongo-unsafe) keys never reach persistence (Codex P2 x2). * fix: enforce active filter at execute time, prune revoked shares, scope queue per user P1: injectSkillCatalog now returns activeSkillIds (the filtered set that appears in the catalog). initializeAgent uses that set as the stored accessibleSkillIds on the initialized agent, so getSkillByName at runtime cannot resolve a deactivated skill — even if the LLM hallucinates a name or the user invokes by direct-invocation shorthand. Previously the executor authorized against the full ACL set, bypassing the active-state guarantee (Codex P1). P2: pruneOrphans now checks user access via findAccessibleResources in addition to skill existence. When a share is revoked, the user's skillStates entry for that skill had no cleanup path and silently consumed the 200-cap. Self-heals on both GET and POST. One extra ACL query per settings read/write; scoped to a single user so no N-user amplification (Codex P2). P2: the write queue moves from a single module-scoped object to a Map keyed by userId. Logout/login in the same tab can no longer flush the previous user's pending snapshot under the new session's auth. Each userId gets its own pending/inFlight slot; the in-flight request retains its original auth via the cookie already attached when sent, so the race window closes (Codex P2). * refactor: extract skillStates helpers to packages/api; add tests; polish Address the remaining valid findings from the comprehensive review: - Extract toRecord, loadSkillStates, validateSkillStatesPayload, and pruneOrphanSkillStates into packages/api/src/skills/skillStates.ts as TypeScript. The controller in /api shrinks to a ~90-line thin wrapper that builds live dependency adapters for Mongoose + the permission service (Review #2 DRY, #3 workspace boundary) - Replace the triplicated 12-line skillStates loading block in initialize.js, openai.js, and responses.js with a single call to loadSkillStates from @librechat/api. One helper, three sites - Swap console.error for the project logger in the controller (Review #7) - Remove the redundant INVALID_KEY_PATTERN regex: a valid ObjectId cannot contain . or $, so isValidObjectIdString already covers it (Review #11) - Parameterize the 200-cap error toast with {{0}} interpolation driven by the error response's `limit` field, so future changes to MAX_SKILL_STATES update the UI message automatically (Review #12) - Add 24 unit tests for the new skillStates helpers (toRecord, resolveDefaultActiveOnShare, loadSkillStates, validateSkillStates- Payload, pruneOrphanSkillStates) covering success paths, malformed input, cap boundaries, and parallel-query behavior (Review #4) - Add 10 tests for injectSkillCatalog pagination covering empty accessible set, missing listSkillsByAccess, single-page filter, owned-vs-shared defaults, explicit-override precedence, multi-page collection, MAX_CATALOG_PAGES safety cap, early termination on has_more=false, additional_instructions injection, and fail-closed without userId (Review #5) Total test count: 60 (was 26 on this surface). * fix: rename skillStates ValidationError to avoid barrel-export collision packages/api/src/types/error.ts already exports a ValidationError (MongooseError extension). Re-exporting a different shape from skills/skillStates.ts through the skills barrel caused TS2308 in CI because the root index re-exports both. Rename to SkillStatesValidationError to keep the exports disjoint. * refactor: tighten tests and absorb caller guard into loadSkillStates Address the followup review findings: - Add optional `accessibleSkillIds` param to loadSkillStates so the helper short-circuits to defaults when no skills are accessible. All three controllers drop the residual 7-line conditional wrapper in favor of a single destructured call (Review #2) - Remove the unreachable `typeof key !== 'string'` check from validateSkillStatesPayload: Object.entries always yields string keys per the JS spec (Review #3) - Replace the two `as unknown as` agent casts in the injectSkillCatalog tests with a `makeAgent()` factory typed directly as the function's parameter shape (Review #4) - Tighten the MAX_CATALOG_PAGES assertion from `toBeLessThanOrEqual(11)` to `toHaveBeenCalledTimes(10)` — the loop deterministically makes exactly 10 page fetches before hitting the cap (Review #1) - Rewrite the parallel-execution test for pruneOrphanSkillStates using deferred promises instead of microtask-order assertions. The test now inspects `toHaveBeenCalledTimes(1)` on both mocks after a single Promise.resolve() yield, pinning Promise.all usage without relying on push-order into a shared array (Review #5) - Evict stale writeQueue entries on user change via a module-scoped `lastSeenUserId` sentinel. When a different user's toggle is the first one after a logout/login, the previous user's queue entry is deleted. Keeps the Map bounded without adding hook-instance effect cleanup (Review #6) * fix(test): mock loadSkillStates in openai and responses controller specs The prior refactor replaced the inline 12-line skillStates loading block with a call to loadSkillStates from @librechat/api. Both controller spec files mock @librechat/api as a flat object, so any new named import from that package is undefined in the test env. Calling `await loadSkillStates(...)` threw before recordCollectedUsage ran, surfacing as "undefined is not iterable" on the test's array destructure of `mockRecordCollectedUsage.mock.calls[0]`. Add the missing mock to both spec files alongside the existing scopeSkillIds stub. * fix: abandon stale skillStates write queues on user switch Close the cross-session leak window where an in-flight flush loop still holds a reference to a previous user's queue: it could fire its next mutateAsync under the new session's auth cookies and persist the stale snapshot to the new user's document (Codex P1). Add an `abandoned` flag on `WriteQueue`. Three mechanisms cooperate: - `getWriteQueue` marks every non-active queue abandoned when the user differs from the last-seen identity (pre-existing eviction site, now more aggressive). - A `useEffect` on `userId` calls the same abandonment pass on every render with a new active identity, covering the window between logout/login and the new user's first toggle (when `getWriteQueue` would otherwise not fire). - The flush loop checks `!queue.abandoned` in its while condition so the second and later iterations exit without firing another `mutateAsync` after the session changes. The first iteration's in-flight request (already dispatched under the original user's cookies) still runs to completion or failure on its own — only the subsequent iterations, which are the dangerous ones, are blocked.	2026-04-25 04:02:00 -04:00
Marco Beretta	f6ee2ea0ee	📜 feat: Skills UI + Initial E2E CRUD / Sharing (#12580 ) * 🎨 feat: Skills UI — Create/Edit/Share/List with Conditional File Tree First-pass UI on top of the CRUD API scaffolding (#12613). Ships the full user-facing flow for inline, single-SKILL.md skills and leaves a clean drop-in for phase-2 multi-file support. - Create a skill from /skills/new with name (kebab-case, validated), description, and SKILL.md body — wired to the real `useCreateSkillMutation` and `TCreateSkill` payload. - List skills in a sidebar (SkillsSidePanel) via `useListSkillsQuery` with live search filtering. - Edit any skill the caller has EDIT permission on — `useUpdateSkillMutation` passes `expectedVersion` for optimistic concurrency and surfaces 409 conflicts as a warning toast + cache refetch. - Non-blocking `TSkillWarning[]` (e.g. "description too short") are shown inline above the form after a successful create/patch. - Read-only mode when the current user lacks EDIT — the form still renders but inputs are marked `readOnly` and the save/reset buttons are hidden. - Share via ACL using the existing `GenericGrantAccessDialog` — the `ShareSkill` button is gated on the SHARE permission. - Delete with confirmation, driven by `useDeleteSkillMutation({ id })`. - Conditional file tree: only rendered when `useListSkillFilesQuery` returns > 0 files. The tree groups flat `relativePath` strings into a nested view (no `react-arborist` dependency) and supports per-file deletion via `useDeleteSkillFileMutation`. Upload is intentionally deferred — the backend stubs it at 501 in phase 1. - New routes: `/skills`, `/skills/new`, `/skills/:skillId`. - Sidebar accordion (`SkillsAccordion` wrapping `SkillsSidePanel`) added to `useSideNavLinks` gated on `PermissionTypes.SKILLS` USE. The initial UI branch (#12580) shipped a lot of exploration code on top of a now-superseded placeholder backend. Kept as complementary: the `Skills/` component tree, translation keys, role descriptions, `PublicSharingToggle` SKILL mapping, `resources.ts` SKILL config, `useCanSharePublic` SKILL mapping, and `data-provider/roles.ts` `useUpdateSkillPermissionsMutation`. Deferred out of this first pass: - Skill favorites (`useSkillFavorites`, `getSkillFavorites` endpoint) — the backend route doesn't exist yet; saving for a follow-up. - AgentConfig `SkillSelectDialog` integration — the UI branch had this gated behind `false &&`; rolled back with the config. - `InvocationMode` / `CategorySelector` / `parseSkillMd` / tree-node mutations — not in the Anthropic skill spec and not in the CRUD API. - `react-arborist` dependency — replaced with a hand-rolled recursive tree built from flat `TSkillFile[]`. - 38 data-schemas skill model tests: pass - 25 api skill route tests: pass - 16 user-controller cleanup tests: pass * 🔐 feat: Default-On Skills in Interface Config and Role Seeder The skills accordion was registered in the side nav gated on `PermissionTypes.SKILLS` USE, but no one was actually seeding that permission on startup, so a fresh install had the USER role with zero skill permissions and the accordion never rendered. Fixes three gaps: 1. `interfaceSchema` in data-provider's `config.ts` had no `skills` field at all. Added it alongside the existing agents/prompts shape (boolean \| { use, create, share, public }) and a default of `{ use: true, create: true, share: false, public: false }`. 2. `loadDefaultInterface` in data-schemas passed every interface key through to the loaded config EXCEPT `skills`. Added the one-line passthrough so `appConfig.interfaceConfig.skills` is actually populated on boot. 3. `updateInterfacePermissions` in packages/api/src/app/permissions.ts seeds role permissions from the interface config on every restart. Added: - `SKILLS` case to `hasExplicitConfig` - `skillsDefaultUse/Create/Share/Public` extraction (mirrors prompts/agents) - `PermissionTypes.SKILLS` block in `allPermissions` that falls through config → roleDefaults → schema default, same pattern as AGENTS and PROMPTS - `SKILLS` entry in the share-backfill array so that pre-existing SKILL role docs missing SHARE/SHARE_PUBLIC get them filled on the next restart Test expectations updated: seven `expectedPermissionsFor(User\|Admin)` blocks in `permissions.spec.ts` now include SKILLS, matching the role-default values (USER: use+create true, share/public false; ADMIN: all true). Result: on a fresh install, a regular USER gets skill USE/CREATE and the "Skills" accordion shows up in the chat side panel without any yaml config. Admins can lock it down per role or per tenant via `interface.skills` in librechat.yaml. Tests: - 34 packages/api permissions.spec.ts: pass - 151 packages/api app tests: pass - 38 data-schemas skill.spec.ts: pass - 928 data-provider tests: pass - 25 api skills.test.js: pass * ♻️ fix: Resolve Skills UI Review Findings Addresses the 13 findings from the PR review against the prior commit. 1. canEdit consistency — extracted `useSkillPermissions(skill)` as the single source of truth for owner/admin/ACL gating. `SkillsView`, `SkillForm`, `ShareSkill` all consume it; `SkillFileTree`'s per-file delete button now honors admin + EDIT-bit permissions instead of just ownership. Unit tests cover owner, admin, editor-ACL, viewer-ACL, owner-ACL, loading, and undefined-skill cases. 2. Disabled submit buttons — create/edit form submit buttons now set native `disabled` (not just `aria-disabled`) during `isLoading`. `onSubmit` also guards with an early return when the mutation is still in-flight so a duplicate enter-key submit can't create two skills. 3. Wrong maxLength error message — description/name `maxLength` rules no longer re-use `com_ui_skill__required`. Added dedicated `com_ui_skill_name_too_long` and `com_ui_skill_description_too_long` keys with the literal limit interpolated (`{{0}}`). 4. Search debouncing* — `SkillsSidePanel` now threads the filter input through the existing `useDebounce` hook (250ms) so typing "skills" no longer fires six separate list queries. 5. Frontend test coverage — added: - `tree.test.ts` (9 tests) covering `buildTree` / `nodeKey` edge cases: empty input, single root file, multiple roots, nested folders, deeply-nested trees, lexicographic sort, empty paths, stable keys - `useSkillPermissions.test.ts` (7 tests) covering every precedence branch (owner / admin / EDIT / VIEW / owner-ACL / loading / undef) Form integration tests proved flaky against react-hook-form's async `isValid` with our jest-dom mock setup; deferred to a follow-up PR with a proper `@librechat/client` test harness. 6. Shared `SKILL_NAME_PATTERN` — promoted the regex plus the four length constants (`SKILL_NAME_MAX_LENGTH`, `SKILL_DESCRIPTION_MAX_LENGTH`, `SKILL_DESCRIPTION_SHORT_THRESHOLD`, `SKILL_DISPLAY_TITLE_MAX_LENGTH`, `SKILL_BODY_MAX_LENGTH`) out of `packages/data-schemas/src/methods/skill.ts` and into `packages/data-provider/src/types/skills.ts`. The data-schemas module now aliases the shared exports so the backend validator and the frontend form share one source of truth. Also fixed a latent bug: the client regex was stricter than the backend (`^[a-z0-9]+(?:-[a-z0-9]+)$` vs. the real `^[a-z0-9][a-z0-9-]$`), which would have rejected valid names like `foo--bar` client-side. 7. Removed hardcoded "Claude" — replaced `com_ui_skill_description_help` ("Claude uses this to...") with a new `com_ui_skill_create_subtitle` for the form header and `com_ui_skill_description_field_hint` ("This is what the model reads to decide...") for the inline hint. LibreChat is LLM-agnostic; the old copy misled GPT/Gemini users. 8. Lifted tree mutation hook — `useDeleteSkillFileMutation` is now instantiated once in `SkillFileTree` (not per `TreeRow`). A `TreeContext` provides `onDeleteFile` + `isDeleting` + `canEdit` to rows. A 60-node tree used to instantiate 60 mutation hooks; it now instantiates one. 9. List O(n) re-render — `SkillListItem` no longer reads `useParams()` directly. `SkillList` reads the active id once and passes `isActive` as a prop, so navigation only re-renders the two items whose `isActive` flipped (memo'd), not all N items. 10. Deduped help text — the field-level hint and form-level subtitle now use different translation keys with distinct copy instead of showing the same sentence twice on the same page. 11. Removed ineffective `useCallback` — `DeleteSkill.handleDelete`, `CreateSkillForm.onSubmit` / `.handleCancel`, `SkillForm.onSubmit`, and `SkillFileTree.handleDeleteFile` all wrapped closures around React Query `mutation` refs, whose identities change every render. Their dep arrays invalidated every render, making the memo a no-op with extra overhead. `SkillFileTree` now destructures the stable `mutate` function and inlines the arrow inside the memoized `contextValue` — one stable reference per deps change. 12. Import order — fixed shortest→longest package ordering and longest→shortest local ordering across all touched skill files per AGENTS.md. `react` always first where imported. 13. Memoization principle — documented the rule with inline comments: `memo` on components that appear in repeated contexts (`TreeRow`, `SkillListItem`) or as children of frequently-re-rendering parents (`ShareSkill` / `DeleteSkill` under `SkillForm`'s per-keystroke form-state updates). Removed `memo` from `SkillFileTree` since its parent `SkillDetailPanel` only re-renders on query-data changes. - 38 data-schemas skill.spec.ts - 34 packages/api permissions.spec.ts - 25 api skills.test.js - 16 client unit tests (9 buildTree + 7 useSkillPermissions) - All type-checks + eslint clean on touched files * 🧹 fix: Skills Duplication, Input Styling, Remove LLM-specific Copy Three UI fixes from an in-chat review pass: 1. Sidebar duplication — `SkillsView` was rendering its own `SkillsSidePanel` aside alongside the chat side panel's `SkillsAccordion`, so on `/skills` the user saw the skill list twice. Fixed by mirroring the `InlinePromptsView` pattern: the route content is now just the detail / create panel and the chat side panel is the sole list. Added `/skills → /skills/new` redirect and a `/skills/new` literal route so `useParams().skillId` is `undefined` for "new" (matches prompts). 2. Name Input styling — the big floating-label pattern used by prompts/agents for the primary name field was replaced with a conventional `<Label>` + `<Input>` above it, diverging from the rest of the app. Restored the prompts-style `text-2xl` input with the peer-focus animated label on both `CreateSkillForm` and `SkillForm`. Kept the conventional pattern for description and body since they're textareas. 3. Remove LLM-specific copy from skill translations — dropped `com_ui_skill_description_help` ("Claude uses this to...") and the transitional "This is what the model reads..." phrasing. Field hint is now a neutral "Be specific about when this skill should apply." and the create-page subtitle is a neutral "Author a new skill your agents can invoke." LibreChat is LLM-agnostic; baking product names into user-facing copy is wrong outside the `com_endpoint_anthropic_` keys where the setting actually only applies to Claude models. Side-effect: the `SkillDetailView` wrapper in `SkillsView` now only renders the file-tree aside when the skill has > 0 files — same conditional-tree behavior as before, just scoped to this route instead of also trying to also render a list sidebar. - 16 client skill tests still pass - Type-check + eslint clean on touched files 🎁 feat: Restore Skills UI from PR #12580 Brings back everything the original UI PR (#12580, commit da039917c) shipped that my earlier rebase dropped. Verbatim restores where possible; adapts the new hooks/types where the backend contract has shifted. Scoped-out / gated-off (now restored as inert UI scaffolding): - `hooks/useSkillFavorites.ts` + `utils/favoritesError.ts` + the `useGetSkillFavoritesQuery` / `useUpdateSkillFavoritesMutation` additions in `data-provider/Favorites.ts`. The backend route doesn't exist yet — the data-service functions resolve with empty arrays so the Star UI is a visual-only no-op until phase 2. - `dialogs/SkillSelectDialog.tsx` + the "Add Skills" section in `SidePanel/Agents/AgentConfig.tsx` (still gated behind the original `false &&`) + `skills?: string[]` on `AgentForm` / `Agent` / `AgentCreateParams` / `AgentUpdateParams` + the `skills: []` entry in `defaultAgentFormValues`. - `TUserFavorite.skillId` reserved on the shared favorites type. Concept-is-gone / deleted-types (restored as UI-only types + stubs): - `InvocationMode` enum and `TSkillNode`, `TSkillTreeResponse`, `TCreateSkillNodeRequest`, `TUpdateSkillNodeRequest` types in `packages/data-provider/src/types.ts`. UI-facing only; the backend flat `TSkillFile[]` contract is unchanged. - `TSkill.invocationMode?: InvocationMode` as an optional field. Forms read/write it in local state and deliberately drop it from the PATCH payload until the backend column lands. - `tree/SkillFileTree.tsx` (`react-arborist`-based), `SkillTreeNode.tsx`, `TreeToolbar.tsx`, `SkillFileEditor.tsx`, `SkillFilePreview.tsx` — full filesystem-style browser UI restored verbatim. - `data-provider/Skills/tree-queries.ts` + `tree-mutations.ts` hooks (`useGetSkillTreeQuery`, `useCreateSkillNodeMutation`, etc.). The `data-service` stubs them: `getSkillTree` returns `{ nodes: [] }`, `createSkillNode` / `updateSkillNode` / `updateSkillNodeContent` return synthetic node shapes, `deleteSkillNode` resolves void. Hooks compile and run; tree is empty until phase 2 wires a real backend. - `MutationKeys.createSkillNode` / `updateSkillNode` / `deleteSkillNode` / `updateSkillNodeContent` + `CreateSkillNodeBody` / `UpdateSkillNodeVariables` / `DeleteSkillNodeBody` / `UpdateSkillNodeContentVariables` types. - `QueryKeys.skillTree` / `skillNodeContent` / `skillFavorites` / `favorites` and the `skillTree()` endpoint helper. Scope-simplified (restored with minimal adaptation): - `display/SkillDetailHeader.tsx` + `display/SkillDetail.tsx`. Header now falls back to `InvocationMode.auto` when `skill.invocationMode` is undefined. - `forms/SkillContentEditor.tsx` — click-to-edit markdown preview toggle for the SKILL.md body field. Wired into both `CreateSkillForm` and `SkillForm` replacing the plain `<TextareaAutosize>`. (Needed `@ts-ignore` on `remarkPlugins` / `rehypePlugins` for the same `PluggableList` vs `Pluggable[]` shape drift `MarkdownLite.tsx` already works around.) - `forms/InvocationModePicker.tsx` + `forms/CategorySelector.tsx` — the auto/manual/both dropdown and the skill category selector. Wired into both forms inside a `FormProvider` so the Controller-based widgets can read `useFormContext`. `category` flows to the PATCH / POST payload as before; `invocationMode` is UI-only per the type note above. - `buttons/CreateSkillMenu.tsx` + `utils/parseSkillMd.ts` — dropdown with AI / Manual / Upload SKILL.md entries + the YAML frontmatter parser for the upload path. `CreateSkillForm.defaultValues` now accepts the parsed shape, so the upload → redirect → pre-populated form flow works again. - `buttons/AdminSettings.tsx` — admin permissions dialog. Uses the existing `useUpdateSkillPermissionsMutation` which was already wired. - `sidebar/FilterSkills.tsx` — restored filter + AdminSettings + CreateSkillMenu wrapper. `SkillsSidePanel.tsx` is back to the original `FilterSkills`-based layout. - `lists/SkillList.tsx` + `lists/SkillListItem.tsx` — restored verbatim. - `layouts/SkillsView.tsx` — restored the full tree + file editor + file preview layout. The chat side panel keeps its own accordion list; this view is the inline detail experience. - `hooks/Generic/useUnsavedChangesPrompt.ts` — route-leave guard hook. - `useGetSkillByIdQuery` is aliased to `useGetSkillQuery` so restored components (`SkillsView`, `SkillForm`) that import the old name resolve to the new hook. - `SkillSelectDialog` + `AgentConfig` coerce `skillsData?.skills` instead of `.data` (list response shape drift from the CRUD PR). - `CreateSkillForm` / `SkillForm` wrap their JSX in `FormProvider` so the restored `CategorySelector` and `SkillContentEditor` components — which read `useFormContext` — work inside the existing forms without another refactor. - `CreateSkillForm.defaultValues` prop accepts `Partial<Values> & { invocationMode?: unknown }` so the upload flow's `{ name, description, invocationMode }` shape passes through cleanly. - `SkillsView` route map gains `/skills/:skillId/edit` and `/skills/:skillId/file/:nodeId` so the tree-navigation URLs the original view produces actually resolve. - `client/package.json` gains `react-arborist@^3.4.3`. - ~60 translation keys the restored files reference — invocation labels, edit/create page titles, file editor chrome, tree toolbar tooltips, favorites, admin allow-settings, unknown-file-type, sr_public_skill, delete/rename _var variants — all added to `en/translation.json`. - Prompts-style floating-label name input — kept from my earlier commit so it matches the rest of the app (user reviewed and approved that styling). Hidden skill-body textarea is replaced by `SkillContentEditor` in both forms. - 38 data-schemas skill.spec.ts - 34 packages/api permissions.spec.ts - 25 api skills.test.js - 7 client useSkillPermissions.test.ts - Type-check: pre-existing error count (188) dropped to 120 because my restorations fixed some previously-broken field types. * chore: Update package-lock.json to include react-arborist and memoize-one * feat: Add support for react-arborist in Vite configuration This update introduces a new condition in the Vite configuration to handle the 'react-arborist' package, ensuring it is properly recognized during the build process. This change enhances compatibility with the recently added 'react-arborist' dependency in the project. * 🩹 fix: Hide InvocationMode, Fix SkillContentEditor Click-to-Edit 1. Hide InvocationModePicker from both CreateSkillForm and SkillForm. Component stays on disk for when the backend lands the column. 2. Fix "Click to edit" doing nothing on SkillContentEditor. The `onBlur={() => setIsEditing(false)}` on the TextareaAutosize was racing with `autoFocus` — React renders the textarea, autoFocus fires, then a layout/reconciliation blur fires immediately, bouncing back to preview mode before the user can interact. Removed onBlur; users toggle via the header button or Escape key. * 🎨 feat: Reader-First Skills UI — Match Claude.ai Layout Reworks the Skills UI from form-first to reader-first, matching Claude.ai's skill detail pattern. Default view is now read-only. Clicking a skill in the sidebar navigates to `/skills/:id` which renders `SkillDetail` — a clean content view with: - Skill name as the primary heading - Metadata row: "Added by" + "Last updated" (formatted date) - Description block - Rendered SKILL.md body in a bordered card with a source/rendered toggle (eye + code icons, matching Claude.ai's segmented control) No form fields, no save/cancel buttons. The user reads the skill first and takes action deliberately. Create is now a dialog. The `/skills/new` route is gone. `CreateSkillMenu` (the + dropdown in the sidebar) now opens `CreateSkillDialog` — a minimal modal with name, description, and instructions fields. Upload-from-file still works: parse → populate dialog → create. Matches Claude.ai's "Write skill instructions" modal. Edit is behind an action. The detail view shows an "Edit" button (permission-gated) that navigates to `/skills/:id/edit`, rendering the existing `SkillForm`. The edit route is preserved for direct linking. Navigation goes to detail, not edit. `SkillListItem` now navigates to `/skills/:id` (detail) instead of `/skills/:id/edit`. - `display/SkillMarkdownRenderer.tsx` — shared ReactMarkdown component extracted from `SkillContentEditor`. Same remark/rehype plugins, no form dependency. - `display/SkillDetail.tsx` — the reader-first view (replaces the old thin wrapper). - `dialogs/CreateSkillDialog.tsx` — OGDialog modal for skill creation. - `layouts/SkillsView.tsx` — gutted and rebuilt. Three states: no-skill (empty state), skillId (SkillDetail), skillId+edit (SkillForm). Removed full-page CreateSkillForm, removed TreeView. - `buttons/CreateSkillMenu.tsx` — opens dialog instead of navigating to `/skills/new`. Upload flow: parse → set dialog defaults → open. - `lists/SkillListItem.tsx` — navigate to detail, not edit. - `routes/index.tsx` — removed `/skills/new` and file/nodeId routes; `/skills` renders SkillsView directly (empty state). - `display/index.ts`, `dialogs/index.ts` — added new exports. - `locales/en/translation.json` — added ~10 new keys for metadata, toggle labels, dialog title, empty state. * 🩹 fix: SkillContentEditor click-to-edit z-index — button was z-0 behind rendered content * 🩹 fix: Align Edit button size with Share/Delete (size-9) * 🎨 feat: Claude.ai-Style Skill List Panel Rewrites the skills sidebar to match Claude.ai's panel layout: - Header: "Skills" title + search icon (toggles input) + add icon (opens CreateSkillDialog directly, no dropdown menu) - Collapsible "Skills" section with chevron toggle - Skill items: 24px icon badge (rounded square with ScrollText icon) + name only. No description text in the list — that lives in the detail view. Active item gets highlighted bg + bold font. - Removed AdminSettings button from sidebar header — admin config is accessible via the admin dashboard, not cluttering every user's skill list. - Removed FilterSkills wrapper (was Filter + AdminSettings + CreateSkillMenu). The search + create are now inline in the panel header. Files changed: - sidebar/SkillsSidePanel.tsx — full rewrite - sidebar/SkillsAccordion.tsx — simplified wrapper - lists/SkillList.tsx — collapsible section, no description - lists/SkillListItem.tsx — icon badge + name, memo'd * 🎨 fix: Align Skills UI Styling with Prompts Patterns Style alignment pass based on direct comparison with claude.ai and the existing prompts preview dialog. SkillsSidePanel search now replaces the title in the header row when toggled (search icon + input + X close), matching Claude.ai's pattern. Previously it pushed a separate input below the header, wasting vertical space. Close button clears the search term. Replaced `text-text-tertiary` with `text-text-secondary` across SkillDetail, SkillList, SkillForm, CreateSkillForm, CreateSkillDialog, SkillContentEditor. Tertiary was too dark / low contrast. SkillList section chevron label now reads "Personal skills" (matching Claude.ai) via the existing `com_ui_my_skills` key, instead of the generic "Skills" which duplicated the header. Aligned with `PromptDetailHeader` styling: - 48px round icon (ScrollText in bg-surface-secondary circle) - Name + public badge in the icon row - Metadata below the icon: User icon + author, Calendar icon + date (text-xs text-text-secondary with gap-3, matching prompts exactly) - Description uses the same label-above-text pattern as prompts - Content card uses `bg-transparent` border (not bg-surface-primary-alt) - Toggle buttons use size-5 icons and text-text-secondary for inactive Changed from `max-w-lg p-0` to `max-w-5xl` with the same max-height and padding pattern as the prompts PreviewPrompt dialog: `max-h-[80vh] p-1 sm:p-2 gap-3 sm:gap-4`. Close button now renders via default OGDialogContent behavior (removed showCloseButton=false). * 🩹 fix: SkillDetail fills parent height, tighter spacing (px-6 pb-6 gap-2) * 🩹 fix: Align Skills panel header padding (px-4) with list content below * 🩹 fix: Reduce Skills header top padding (pt-2) to align with sidebar icon strip * 🩹 fix: Tighten Skills header (py-2) and detail top (py-2) to align with sidebar icons and match edit view * 🩹 fix: Offset SidePanel Nav pt-2 with -mt-2 on SkillsAccordion so Skills header aligns with icon strip * 🛠️ fix: Increase Node memory limit for production build in package.json * 🩹 fix: Remove top padding from SkillDetail header row (py-2 → pb-2) * 🏗️ refactor: Move pt-2 from SidePanel/Nav wrapper to each panel Removed the global `pt-2` from `SidePanel/Nav.tsx` and pushed it into each panel's own top-level wrapper. This lets each panel own its vertical alignment independently — Skills can sit flush at the top to align with the sidebar icon strip, while other panels keep their original spacing. Panels updated with `pt-2`: - PromptsAccordion (via className on PromptSidePanel) - BookmarkPanel - FilesPanel - MemoryPanel - MCPBuilderPanel - AgentPanel (form wrapper) - AssistantPanel (form wrapper) - ParametersPanel (already had pt-2) SkillsAccordion: removed the -mt-2 hack, now naturally flush. * 🧹 fix: Align CreateSkillDialog field styling + remove 19 unused i18n keys Dialog fields: all three inputs now use consistent `rounded-xl border-border-medium px-3 py-2 text-sm` styling. Replaced the `<Input>` component with a plain `<input>` to avoid the component's built-in `rounded-lg border-border-light` overriding the dialog's border style. Labels use `font-medium` for consistency. Removed 19 unused translation keys from translation.json: com_ui_skill_body, com_ui_skill_body_placeholder, com_ui_skill_create_subtitle, com_ui_skill_file_delete_confirm, com_ui_skill_file_delete_error, com_ui_skill_file_deleted, com_ui_skill_files_empty, com_ui_skill_files_multi_hint, com_ui_skill_list, com_ui_skill_load_error, com_ui_skill_resize_file_tree, com_ui_skill_select_file, com_ui_skill_select_file_desc, com_ui_skills_load_error, com_ui_add_first_skill, com_ui_create_skill_page, com_ui_edit_skill_page, com_ui_save_skill, com_ui_no_skills_title * 🎁 feat: Upload Skill Dialog + Simplified Create Menu New `UploadSkillDialog` matching Claude.ai's upload modal: - Dashed drop zone with drag-and-drop support - Accepts .md, .zip, .skill files - Phase 1: processes .md files (parses YAML frontmatter → creates skill with body as the full file content) - Shows file requirements below the drop zone - On success: navigates to the new skill's detail view `CreateSkillMenu` now has two flat options (no sub-menu): - "Write skill instructions" → opens `CreateSkillDialog` - "Upload a skill" → opens `UploadSkillDialog` Removed the disabled "Create with AI" option and the old file input hidden-element approach. The sidebar `+` button now renders `CreateSkillMenu` directly instead of a standalone create dialog. - Removed 5 unused i18n keys (com_ui_skill_added_by, com_ui_skill_last_updated, com_ui_skills_add_first, com_ui_skills_filter_placeholder, com_ui_skills_new) - Tightened metadata gap in SkillDetail (mt-1 → mt-0.5) - Added 7 new upload-related i18n keys * 🔒 feat: Zip/Skill File Upload Support with Safety Limits Rewrites UploadSkillDialog to properly handle all three accepted file types: - `.md` — reads as text, parses YAML frontmatter, creates skill - `.zip` / `.skill` — reads as ArrayBuffer, extracts with JSZip, finds SKILL.md (at root or one level deep), parses its content, creates skill. Shows spinner during processing. Security guards against zip bombs: - MAX_ZIP_SIZE: 50MB compressed file limit - MAX_ENTRIES: 500 file limit inside the archive - Path traversal rejection: skips entries with `..` or leading `/` - SKILL.md search limited to depth ≤ 2 segments Added `jszip@^3.10.1` to client dependencies (already in the monorepo's node_modules from backend usage). The name is inferred from the zip filename if SKILL.md frontmatter doesn't have one (e.g. `skills-autofix.zip` → `skills-autofix`). * 🚀 feat: Backend Skill Import + Live File Upload Endpoints New endpoint that accepts a single multipart file (.md, .zip, .skill) and creates a skill with all its files in one request: - .md: parse YAML frontmatter → create skill with body - .zip / .skill: extract with JSZip, find SKILL.md (root or one level deep), create skill from its content, then persist every additional file via `upsertSkillFile` + local file storage strategy. Returns the created skill + an `_importSummary` with per-file results. Security: - 50MB compressed file size limit (multer) - 500 max entries in archive - 10MB per individual file - Path traversal rejection (no `..`, no absolute, validated charset) - File type filter: only .md/.zip/.skill accepted - Rate limited via existing `fileUploadIpLimiter` + `fileUploadUserLimiter` Handler lives in `packages/api/src/skills/import.ts` with injectable deps (`createSkill`, `upsertSkillFile`, `saveBuffer`) for testability. Replaced the 501 stub with a real handler: - Accepts multipart FormData with `file` + `relativePath` - Saves file via local storage strategy - Calls `upsertSkillFile` to persist the SkillFile record - Returns the upserted document - Rate limited, ACL-gated (EDIT permission required) - 10MB per file limit `UploadSkillDialog` now sends the file to `/api/skills/import` via `dataService.importSkill(formData)` — no more client-side JSZip. Removed `jszip` from client dependencies (only backend needs it). Added `importSkill()` in data-service + `importSkill()` endpoint builder in api-endpoints. Updated the file upload test from expecting 501 stub to expecting 400 "no file provided" (live validation). All 25 skill route tests pass. * 🔒 fix: Complete Import Handler — Validation, Ownership, Error Surfacing Fixes several gaps in the skill import flow: 1. Skill validation now runs and surfaces properly. The import handler calls the real `createSkill(CreateSkillInput)` which runs `validateSkillName`, `validateSkillDescription`, `validateSkillBody`. Validation errors (SKILL_VALIDATION_FAILED) are caught and returned as 400 with the issue messages. Duplicate-key errors return 409. Previously all errors were swallowed into a generic 500. 2. `authorName` is now populated. The `CreateSkillInput` requires `authorName` which was missing — resolved from `req.user.name ?? req.user.username ?? 'Unknown'`, matching the existing create handler. 3. SKILL_OWNER permission is granted after import. Calls `grantPermission` with `AccessRoleIds.SKILL_OWNER` so the uploader can edit/delete/share the imported skill. This was entirely missing — imported skills would have been ownerless. 4. `tenantId` propagated. Both the skill and each SkillFile record receive `req.user.tenantId` for multi-tenant deployments. 5. SkillFile records are created in the DB. Each non-SKILL.md file in the zip is saved to file storage via `saveBuffer` and recorded via `upsertSkillFile`, which validates the relativePath, infers the category from the path prefix, and atomically bumps the skill's `fileCount` and `version`. Import deps now include `grantPermission` from PermissionService, injected in `api/server/routes/skills.js`. * 🐛 fix: Import grant uses accessRoleId (not roleId) — fixes skill not appearing in list * 🎨 fix: Cache invalidation, file tree, frontmatter rendering Three fixes for the skill detail view: 1. Cache invalidation after import. UploadSkillDialog now calls `queryClient.invalidateQueries([QueryKeys.skills])` after a successful import so the sidebar list picks up the new skill without requiring a page refresh. 2. File tree in detail view. When a skill has `fileCount > 0`, the detail view now queries `useListSkillFilesQuery` and renders a file list below the body card — SKILL.md first, then folders and root files. Icons: Folder for directories, FileText for files. 3. Frontmatter stripped and rendered as metadata. YAML frontmatter (`---\nversion: 0.1.0\ntriggers: ...\n---`) is now parsed out of the body before markdown rendering. The `name` and `description` fields are skipped (already shown in the header). Remaining fields (version, triggers, dependencies, etc.) are displayed in a Claude.ai–style grid: label on the left, value on the right, above the rendered markdown content. Source view still shows the full raw body including frontmatter. * 🩹 fix: Always fetch skill files — fileCount may be stale in cached skill object * 🌳 feat: Inline File Tree in Sidebar Skill List Moves the file tree from the bottom of SkillDetail into the sidebar list, matching Claude.ai's pattern: - Multi-file skills show a chevron toggle on the right side of the skill list item - Clicking the chevron expands an inline file tree below the skill name: SKILL.md first, then folders (with folder icon + right chevron) and root files - File list is fetched lazily (only when expanded) via useListSkillFilesQuery - Clicking a file navigates to the skill detail view - Files section removed from SkillDetail — the sidebar is now the sole file tree location, keeping the detail panel clean SkillDetail cleaned up: removed groupFiles helper, file-related state, useListSkillFilesQuery import, FileText/Folder icon imports. * 🌲 feat: Virtualized inline file tree with react-vtree Replace hand-rolled recursive FolderRow/FileRow buttons with a proper virtualized FixedSizeTree from react-vtree for the sidebar skill list. Dynamic height tracks open folders; capped at 350px with smooth expand/collapse transitions. * chore: Remove no longer used SkillFileTree and SkillTreeNode components * chore: Update Vite config to replace 'react-arborist' with 'react-vtree' for module resolution * feat: Skill file content viewing with lazy DB caching - Add `skills` field to `fileStrategiesSchema` so operators can configure a dedicated storage backend for skill files. Falls back by type (image/document) when unset. - Fix hardcoded `FileSources.local` in skill save/import — now uses the resolved strategy via `getFileStrategy(req.config, { context })`. - Replace 501 download stub with real handler that streams from any storage backend and returns JSON `{ content, mimeType, isBinary }`. - Binary detection (null-byte + non-printable ratio on first 8 KB) flags files on first read so they're never re-fetched. - Text content ≤ 512 KB is cached in the SkillFile MongoDB document; subsequent reads skip storage entirely. - Clicking a skill row now expands inline files (not just chevron). - Clicking a file navigates to `?file=<path>` and renders content in a new SkillFileViewer (markdown, code, images, binary placeholder). * chore: Remove react-window and its type definitions from package.json and package-lock.json - Deleted `react-window` and `@types/react-window` dependencies from both `package.json` and `package-lock.json` to streamline the project and reduce unnecessary bloat. * fix: Build errors — remove endpoints import, fix Uint8Array cast - Replace `import { endpoints }` (not public) with inline URL in SkillFileViewer - Remove `as Uint8Array` cast in stream chunk handling - Extend getSkillFileByPath return type with content/isBinary to decouple from data-schemas build artifact resolution * chore: Remove 8 unused i18next keys com_ui_create_skill_ai, com_ui_create_skill_manual, com_ui_delete_folder_confirm_var, com_ui_delete_skill, com_ui_delete_skill_confirm_var, com_ui_delete_var, com_ui_rename_var, com_ui_skill_files * fix: Add configMiddleware to skills router, handle SKILL.md in viewer - Add configMiddleware to skills router so req.config is populated when getLocalFileStream (or any strategy) reads file paths. - Handle SKILL.md in download handler — serves skill.body directly from the Skill document instead of looking for a SkillFile record. - Clicking SKILL.md in sidebar tree now opens the file viewer (matching Claude.ai behavior: file view vs default detail view). * ci: Run unit tests on PRs to any branch Remove the branches filter from both test workflows so contributor PRs targeting feature branches (not just main/dev) get CI coverage. Path filters are kept so tests only run when relevant files change. * fix: Update skills route tests for download handler changes - Mock configMiddleware (sets req.config for file storage access) - Mock getStrategyFunctions and getFileStrategy (storage strategy deps) - Replace 501 stub test with SKILL.md content test + 404 test * fix: Auto-expand files, frontmatter parsing, select-none, prefetch - Auto-expand file tree when navigating directly to a skill URL - Prefetch files for the active skill (eliminates first-expand lag) - Fix frontmatter parser to handle multi-line YAML list values (triggers field was missing because it uses list syntax) - SkillFileViewer now parses frontmatter for .md files — shows structured grid + rendered body (matching SkillDetail's display) with source/rendered toggle - Add select-none to all sidebar skill and file tree buttons * refactor: Derive expanded state from isActive instead of useEffect Replace useEffect sync with deterministic derivation: expanded = hasFiles && (isActive \|\| !collapsed) Active skill is always open. collapsed is a manual toggle that only takes effect on non-active items. * fix: Remove empty space above body card — overlay view toggle Move the rendered/source toggle from a dedicated row (40px of empty space) to an absolute-positioned overlay in the card's top-right corner, matching Claude.ai's layout. * fix: Remove header bars from content editors — overlay action buttons Collapse the full-width header bars ("Skill Content", "Text") in SkillContentEditor, PromptTextCard, and PromptEditor. Action buttons (edit/save toggle, copy, variables) are now absolute-positioned in the card's top-right corner, reclaiming ~46px of vertical space. * fix: Spinner visibility in file viewer — use text-text-secondary * fix: Address review findings — security, correctness, code quality Codex P1: Use $unset instead of undefined to clear cached content and isBinary fields on file re-upload (Mongoose strips undefined). Codex P2: Match skill-file validation errors by error.code instead of error.message substring. F1: Zip bomb defense — track cumulative decompressed bytes (500 MB cap), check declared uncompressed size before buffering each entry. F2: Remove misleading "atomically" from import handler JSDoc. F3: Static import for isBinaryBuffer instead of dynamic import(). F4: Replace console.error with logger in upload handler. F6: Add multer error handler middleware to skills router. F7: Move React import to top of SkillDetail.tsx. F9: Fix variable shadowing (trimmed → item) in parseFrontmatter. F11: Replace JSON.parse(JSON.stringify()) with toJSON() for Mongoose document serialization. F12: Remove dead dynamic import('fs') fallback (memoryStorage always provides file.buffer). F13: Hoist MIME_MAP to module scope to avoid per-call allocation. F16: Share single multer.memoryStorage() instance. * fix: Follow-up review — close zip bomb gap, fix error handler F1: Add post-decompression cumulative byte check with break (the pre-decompression check relies on undocumented JSZip internals that may be absent; this closes the gap unconditionally). F2+F3: Multer error handler now forwards non-multer errors via next(err) instead of swallowing them. Also catches file filter rejections (plain Error, not MulterError) by message prefix. F4: Move isBinaryBuffer import to local imports section per CLAUDE.md import order rules. F5: Simplify dead toJSON branch — createSkill returns a POJO. * nit: Link filter error message to handler prefix check * feat: Accordion expansion + active file highlight in sidebar - Only one skill's file tree can be expanded at a time (accordion). Expansion state lifted from SkillListItem to SkillList. - Selected file gets bg-surface-active highlight in the tree. Skill row uses subtle style (no background) when a file is active, matching Claude.ai's pattern where the file — not the skill — carries the selection state. * style: Adjust margin for file tree in SkillListItem component - Reduced left margin from 10 to 5 for improved layout consistency in the file tree display. * fix: TS error on FileTreeNode, nested ternary, chevron collapse - Make style prop optional to match react-vtree's NodeComponentProps - Flatten nested ternary for skill row active styles - Skill row click expands (but doesn't collapse) files + navigates - Chevron click explicitly toggles collapse (matching Claude.ai where clicking the chevron is how you collapse files) * fix: Upload basePath, reject SKILL.md uploads, add skills permission route - Pass basePath: 'uploads' in per-file upload handler (was defaulting to 'images' path, inconsistent with the import flow). - Reject uploads targeting SKILL.md (reserved path — download handler special-cases it to return skill.body, making an uploaded file unreachable via the API). - Add skills entry to roles router permissionConfigs so PUT /api/roles/:roleName/skills actually reaches a handler instead of returning 404. * feat: Expand content area, move controls to header, reduce padding Default detail view: - Remove rounded-xl bordered card wrapper — content flows directly into the article, capitalizing on full screen width - Move eye/code toggle inline with the divider row - Reduce px-6/pb-6 to px-4/pb-4 File viewer: - Move eye/code toggle from card overlay to the header bar - Add copy-to-clipboard button for text files in the header bar - Remove rounded-xl bordered card wrapper for markdown content - Remove bordered pre wrapper for non-markdown text - Reduce px-6/py-4 to px-4/py-3 Both views maximize content space over decorative chrome. * fix: Stable header height, restore some padding - Fix layout shift in file viewer header: use fixed h-10 so the bar height stays constant whether the eye/code toggle renders (markdown) or not (plain text). - Bump content padding from px-4/py-3 back to px-5/py-4 in both views — the previous reduction was too aggressive. * fix: Grant rollback, path validation, error format, dead code cleanup F2: grantOwnership now rolls back (compensating delete) on failure, matching the create handler. Both markdown and zip import paths check the result and return 500 on grant failure. F4: Upload handler validates relativePath with regex + traversal check before calling downstream upsertSkillFile. F5: Document JSZip _data.uncompressedSize as best-effort; the post-decompression cumulative check is the real safety net. F10: Standardize all upload handler error responses to { error } (was { message }, inconsistent with handlers.ts). F13: Single-pass fileResults accumulation in import handler. F1-5: Remove dead uploadFileStubHandler (no route references it). Codex P2: Fix delete nav from /skills/new to /skills. F12: Use cn() in UploadSkillDialog instead of template literals. * perf: Stream-first binary detection + O(1) public skill check F1: Download handler now reads only the first 8 KB for binary detection. If binary, the stream is destroyed immediately without buffering the remaining file. Text files continue reading for caching. Eliminates buffering up to 10 MB per request for binary files under concurrent load. F7: Single-skill GET and PATCH now use hasPublicPermission (O(1) ACL lookup) instead of getPublicSkillIdSet (queries ALL public skill IDs). The list handler still uses the Set approach since it serializes multiple skills. serializeSkill/serializeSkillSummary now accept boolean \| Set for flexibility. * fix: Update test to match { error } response format * fix: Critical stream truncation bug, grantedBy, error format NF-1 (CRITICAL): Rewrite binary detection to single for-await loop. Breaking out of for-await-of destroys the stream via iterator.return(), so the previous two-loop approach silently truncated text files > 8KB. Now: one loop collects chunks, checks binary after 8KB accumulated, and either destroys+returns (binary) or continues reading (text). NF-2: Add grantedBy to import handler's grantPermission call and interface (was missing, inconsistent with create handler). NF-3: Standardize all import handler error responses from { message } to { error }, matching handlers.ts convention. Update client's UploadSkillDialog to read response.data.error accordingly. * fix: Prefer specific validation message over generic error field * fix: YAML quote stripping, saveBuffer null guard, dot segment rejection - Strip surrounding YAML quotes from frontmatter values so name: "my-skill" parses as my-skill (not "my-skill" with quotes that fails the name validator). - Guard resolveSkillStorage against backends with saveBuffer: null (e.g. OpenAI/vector strategies) — throws a descriptive error caught by the handler's try/catch instead of a TypeError. - Tighten upload path validation to reject . segments (e.g. docs/./a.md) matching the model-layer validator, preventing storage writes for paths the DB will reject. * fix: Orphan cleanup, stream errors, malformed zip, cache latency F1: Upload handler now deletes the stored blob if the subsequent DB upsert fails, preventing orphaned files on disk/cloud. F2: Multer error handler returns { error } (was { message }). F3: Wrap JSZip.loadAsync in try/catch — malformed zip returns 400 instead of falling through to 500. F4: Raw download stream gets an error handler — logs the error and destroys the response if headers were already sent. F8: Strip leading hyphens from inferred skill name so filenames like _my-skill.zip don't produce -my-skill (invalid name pattern). F9: Fire-and-forget all updateSkillFileContent cache writes so the response is sent immediately. Cache failures are logged but don't block or fail the read. * fix: Import orphan cleanup + Content-Disposition sanitization Finding A: Add deleteFile dep to ImportSkillDeps. The per-file loop in handleZip now cleans up stored blobs when upsertSkillFile fails, closing the second half of the F1 orphan fix (upload handler was already fixed). Finding B: Sanitize filename in Content-Disposition header for raw downloads — strip quotes, backslashes, and newlines to prevent header injection from user-uploaded filenames. * security: Prevent stored XSS via raw file downloads Non-image files served via ?raw=true now use Content-Disposition: attachment (force download) instead of inline. An uploaded .html or .svg file served inline from the LibreChat origin could execute scripts with access to the user's session — this closes that vector. Images stay inline (needed for <img> rendering in SkillFileViewer). X-Content-Type-Options: nosniff added to prevent MIME sniffing. * security: Block SVG XSS — allowlist safe raster MIME types for inline SVG (image/svg+xml) passed the startsWith('image/') check and was served inline, but SVG is a scriptable format — embedded <script> tags execute in the LibreChat origin. Replace the prefix match with a Set of safe raster-only MIME types (png, jpeg, gif, webp, avif, bmp). SVGs and any future scriptable image/* subtypes now get Content-Disposition: attachment (forced download). * fix: Cap JSON text response at 1MB, consistent md name inference F3: Text files > 1MB now return { isBinary: false } with no content field, forcing the client to use ?raw=true for download. Prevents buffering 10MB files into heap for JSON serialization. Frontend shows a download fallback when content is absent. F4: handleMarkdown now infers skill name from filename (same as handleZip) when frontmatter has no name, instead of rejecting with 400. Consistent behavior across import paths. F1 (reviewer concern): upsertSkillFile is NOT affected — it uses { new: false } for insert-vs-replace detection but does a follow-up findOne (lines 855-859) to return the post-upsert document. * fix: deleteFile arg shape, raw URL base path, hoist SAFE_INLINE_MIMES Codex P2: deleteFile expects { filepath } object, not a raw string. Both upload handler cleanup and import handler cleanup now pass { filepath } to match the strategy contract (deleteLocalFile, deleteFileFromS3 all expect a file object). Codex P2: Raw download URL in SkillFileViewer now uses apiBaseUrl prefix so subpath deployments (/chat, etc.) resolve correctly. NIT: Hoist SAFE_INLINE_MIMES Set to factory scope — was re-allocated per raw download request inside the if block. * fix: Remove inert cache write for large text files, localize aria-label N2: The { isBinary: false } cache write for text files > 1MB had no effect — subsequent requests still fell through to stream read since neither isBinary nor content provided a fast-path short-circuit. Removed the pointless DB updateOne per request. N4: Replace hardcoded "Back to skill" aria-label with localize(). * refactor: Extract shared parseFrontmatter, widen deleteFile type N3: Extract parseFrontmatter into Skills/utils/frontmatter.ts — single implementation shared by SkillDetail and SkillFileViewer. Accepts optional skipKeys set so callers control which frontmatter fields are excluded (SkillDetail skips name/description since they're shown in the header; other .md files show all fields). N5: Widen ImportSkillDeps.deleteFile file param from { filepath } to { filepath; [key: string]: unknown } to signal extensibility if strategies start accessing additional file properties. * fix: Advance i past list items for skipped keys, DRY parseSkillMd Finding A: parseFrontmatter now consumes multi-line YAML list items before checking skipKeys — prevents list lines from leaking into subsequent key parsing as spurious fields. Finding B: parseSkillMd now delegates to the shared parseFrontmatter instead of re-implementing the same frontmatter scanning loop. Reduces client-side parseFrontmatter implementations from 3 to 1. * fix: Call apiBaseUrl(), delete storage blob on file removal - apiBaseUrl is a function, not a string — call it in the template literal so raw download URLs resolve correctly. - deleteFileHandler now looks up the file record before deleting, then fire-and-forget deletes the storage blob via the strategy's deleteFile. Previously only the DB record was removed, leaving orphaned blobs in local/S3/Firebase/Azure storage. * fix: Clean up storage blobs when deleting an entire skill deleteHandler now lists all files for the skill before calling deleteSkill, then fire-and-forget deletes each blob via the storage strategy. Previously only per-file deletion cleaned up blobs — deleting a whole skill left all associated files orphaned in local/S3/Firebase/Azure storage. * refactor: useImportSkillMutation hook, fix TSkill[] unsafe cast - Create useImportSkillMutation in mutations.ts + ImportSkillOptions type. UploadSkillDialog now uses the mutation hook instead of calling dataService.importSkill directly with manual useState loading management. Eliminates unmounted-component state update risk and aligns with the React Query mutation pattern used by every other mutation in the codebase. - SkillSelectDialog: replace as unknown as TSkill[] with proper TSkillSummary typing. SkillCard props updated to TSkillSummary. The dialog only uses summary-level fields (name, description, category, author) — the cast was hiding a type mismatch. * fix: Use saved source for import cleanup, delete old blob on replace Codex P2: Import cleanup now uses file.source (the backend the file was actually saved to) instead of re-resolving from config. In mixed strategy setups, the previous approach could target the wrong backend. Codex P2: When re-uploading a file to an existing relativePath, the old blob is now deleted after successful upsert. Previously only the DB record was replaced, leaving the old storage object orphaned. * fix: Register PUT /:roleName/skills route in roles router * fix: Re-read skill after zip file processing for fresh metadata The import response was built from the skill object created before the file loop, but each upsertSkillFile bumps version and fileCount. Clients caching the stale response would get 409 conflicts on first edit and see incorrect file counts. Now re-reads the skill via getSkillById after the loop so the response reflects the current version, fileCount, and updatedAt. * fix: Size-check SKILL.md before decompression, don't gate on fileCount P1: SKILL.md was decompressed before any size accounting. A crafted archive could expand SKILL.md past 10MB before validation ran. Now checks declared size pre-decompression and actual size post, both against MAX_SINGLE_FILE_BYTES. P2: File list query was gated on cached fileCount which can be stale after mutations. Now fetches files for the active skill regardless of fileCount. hasFiles derived from fetched data with fileCount as fallback, so newly uploaded files appear without hard refresh. * fix: Move files declaration before hasFiles to avoid TDZ error * security: Stream-decompress zip entries with enforced byte cap Replace zipEntry.async('nodebuffer') (buffers entire entry before checking limits) with zipEntry.nodeStream('nodebuffer') piped through a byte counter that destroys the stream when the per-file or cumulative limit is exceeded. Previously, when JSZip's _data.uncompressedSize was absent (the common case), a high-ratio entry could allocate hundreds of MB before the post-decompression check caught it. Now decompression is aborted mid-stream at the exact byte threshold — no entry can exceed its limit regardless of compression ratio. * refactor: Reorganize access check for prompts in useSideNavLinks hook Moved the prompts access check to a new position in the code to improve readability and maintainability. This change ensures that the prompts link is added to the navigation only if the user has the appropriate access, without altering the existing functionality. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-25 04:02:00 -04:00
Danny Avila	963068b112	🧬 feat: Scaffold Skills CRUD with ACL Sharing and File Schema (#12613 ) * 🧬 feat: Scaffold Skills CRUD with ACL Sharing and File Schema Adds Skills as a new first-class resource modeled on Anthropic's Agent Skills, reusing the existing Prompt ACL stack for sharing. Lays the groundwork for multi-file skills (SkillFile schema + metadata routes) without wiring upload processing — single-file skills (inline SKILL.md body) work end-to-end, multi-file uploads are stubbed for phase 2. * 🔬 fix: Wire Skill Cleanup, AccessRole Enum, and Express 5 Path Params CI surfaced four follow-ups from the initial Skills scaffolding commit that local builds missed: - AccessRole's resourceType field had a hardcoded enum that didn't include `'skill'`, blocking SKILL_OWNER/EDITOR/VIEWER role creation in every test that hit the AccessRole model. - The seedDefaultRoles assertion in accessRole.spec.ts hard-listed the expected role IDs and needed the new SKILL_* entries. - deleteUserController had no cleanup for skills, and the deleteUserResourceCoverage guard test enforces every ResourceType has a documented handler — wired in db.deleteUserSkills(user._id) and added the entry to HANDLED_RESOURCE_TYPES. - Express 5's path-to-regexp v6 rejects the legacy `()` named-group glob syntax. The two skill file routes now use a plain `:relativePath` param; the client already encodeURIComponents the path, so a single param is sufficient and decoded server-side. 🪡 fix: Make Skill Name Uniqueness Application-Level Resolve three more CI failures from the Skills scaffolding PR: - Mongoose creates indexes asynchronously and mongodb-memory-server tests can race ahead of the unique (name, author, tenantId) index being built, so the duplicate-name uniqueness test was flaky. Added an explicit findOne pre-check inside createSkill that throws with code 11000 (mimicking the index violation), giving deterministic behavior. The unique index stays as the persistent guarantee. - The deleteUser.spec.js and UserController.spec.js suites mock the ~/models module directly and were missing deleteUserSkills, causing deleteUserController to throw and return 500 instead of 200. - Removed two doc-comment claims that the SKILL_NAME_MAX_LENGTH and SKILL_DESCRIPTION_MAX_LENGTH constants "match Anthropic's API". The values themselves are reasonable but the comments were misleading about who enforces them. * 🪢 fix: Address Code Review Findings on Skills Scaffolding Resolve all 15 findings from the comprehensive PR review: Critical: - Rollback the created skill when grantPermission throws so a transient ACL failure cannot leave an orphaned, inaccessible skill in the DB. - Fix infinite query cache corruption in useUpdateSkillMutation helpers. setQueriesData([QueryKeys.skills]) matches useSkillsInfiniteQuery's InfiniteData cache entries, which have { pages, pageParams } shape — spreading data.skills on those would throw. Added an isInfiniteSkillData guard and per-page transform so both flat and infinite caches update correctly. Major: - Fix TUpdateSkillContext type: the public type declared previousListData but onMutate actually returns previousListSnapshots (a [key, value] tuple array). Updated the type + added TSkillCacheEntry as a shared export from data-provider. - Add cancelQueries calls before optimistic update in onMutate so in-flight refetches cannot clobber the optimistic state. - Parallelize deleteUserSkills ACL removal via Promise.allSettled instead of a sequential await loop — O(1) round-trip vs O(n). - Stub mockDeleteUserSkills in stubDeletionMocks() and assert it's called with user.id in the deleteUser.spec.js happy-path test. - Add idResolver: getSkillById to the SKILL branch in accessPermissions.js so GET /api/permissions/skill/<missing-id> returns 404 instead of 403. Minor: - Reuse resolved skill from req.resourceAccess.resourceInfo in getHandler to eliminate a redundant getSkillById call per GET /api/skills/:id. - Reject PATCH /api/skills/:id requests whose body contains only expectedVersion — previously they silently bumped version with no changes, triggering spurious 409s for collaborators. - Make TSkill.frontmatter optional (wire type) and add serializeFrontmatter / serializeSourceMetadata helpers that return undefined for empty objects instead of casting incomplete data to SkillFrontmatter. - Standardize deleteUserSkills to accept string \| ObjectId and convert internally, matching deleteUserPrompts's signature; UserController now passes user.id consistently. - Replace bumpSkillVersionAndRecount (read-then-write, racy) with bumpSkillVersionAndAdjustFileCount using atomic $inc. upsertSkillFile pre-checks existence to distinguish insert (+1) from replace (0). - Add DELETE /api/skills/:id/files/:relativePath integration tests covering success, 404, and 403 paths. Nits: - Drop trivial resolveSkillId wrapper — pass getSkillById directly. - Remove dead staleTime: 1000 * 10 from useListSkillsQuery since all refetch triggers are already disabled. * 🧭 fix: Resolve Second Skills Review Pass — Cache, Gate, TOCTOU Address 13 of 14 findings from the second code review; reject #13 as misread of the AGENTS.md import-order rule (package types correctly precede local types regardless of length). Major: - Fix addSkillToCachedLists closure bug: a hoisted `prepended` flag was shared across every cache entry matched by setQueriesData, so concurrent flat + infinite caches would silently drop the prepend on whichever was processed second. Replaced the shared helper with three per-entry inline updaters that handle InfiniteData at the page level (page 0 only for prepend, all pages for replace/remove). - Tighten patchHandler's expectedVersion validation: NaN passes `typeof === 'number'` and would previously leak current skill state via a misleading 409. Now requires finite positive integer and returns 400 otherwise. - Guard decodeURIComponent in deleteFileHandler with try/catch — malformed percent encoding now returns 400 instead of 500. - Add PermissionTypes.SKILLS + skillPermissionsSchema + TSkillPermissions in data-provider; seed default SKILLS permissions for ADMIN (all true) and USER (use + create only); wire checkSkillAccess / checkSkillCreate via generateCheckAccess onto the skills router mirroring the prompts pattern. Skills route now enforces role-based capability gates alongside per-resource ACLs. Test suite adds a mocked getRoleByName returning permissive SKILLS. - Fix upsertSkillFile TOCTOU: replaced the pre-check + upsert pair with a single `findOneAndUpdate({ new: false, upsert: true })` call that atomically returns the pre-update doc (null ⇒ insert) so fileCount delta can't double-count on concurrent same-path uploads. Minor: - Add `sourceMetadata` to listSkillsByAccess .select() so summaries no longer silently drop the field for GitHub/Notion-synced skills. - Include `cursor` in useListSkillsQuery's query key so manual pagination doesn't alias across pages. - Clean up TSkillSummary to `Omit<TSkill, 'body' \| 'frontmatter'>` matching what serializeSkillSummary actually emits; drop the Omit-then-re-add noise. - Skip getPublicSkillIdSet in createHandler; a newly-created skill cannot have a PUBLIC ACL entry, so pass an empty set directly instead of paying a DB round-trip. - Trim SkillMethods public surface: drop internal helpers countSkillFiles / deleteSkillFilesBySkillId / getSkillFile from the return object; inline the file cascade into deleteSkill. - Use TSkillConflictResponse at the PATCH 409 call site instead of an inline ad-hoc object literal. - Drop the now-unused EXPECTED_VERSION_ERROR module constant. * 🧩 fix: Extend Role Schema + Types with SKILLS PermissionType CI type-check and unit test failures from the PermissionTypes.SKILLS addition surfaced three unrelated places that all hardcode the permission-type set: - IRole.permissions in data-schemas/types/role.ts enumerates every PermissionTypes key as an optional field. Adding SKILLS to the enum without updating the interface caused TS7053 'expression of type PermissionTypes can't be used to index type' errors in role.methods.spec.ts (lines 407-408, 477-478) because Object.values(PermissionTypes) now yielded a value the interface didn't cover. - schema/role.ts rolePermissionsSchema mirrors the interface at the Mongoose layer; also needed SKILLS added so the persisted role document can actually store skill permissions. - data-provider/roles.spec.ts has a guard test that every permission type carrying CREATE/SHARE/SHARE_PUBLIC must be explicitly "tracked" either in RESOURCE_PERMISSION_TYPES or in the PROMPTS/AGENTS/MEMORIES exemption list. Added SKILLS to the exemption list since skills follow the same default model as prompts/agents (USE + CREATE on for USER, SHARE / SHARE_PUBLIC off). All three are additive pass-throughs with no behavior change. * 🏷️ refactor: Introduce ISkillSummary for Narrow List Projection Follow-up NITs from the second review pass on the Skills PR: - Define ISkillSummary = Omit<ISkill, 'body' \| 'frontmatter'> and use it as the element type in ListSkillsByAccessResult. The list query's .select() intentionally omits body and frontmatter for payload size, but the previous type claimed both fields were present — a type lie that would mislead future readers even though serializeSkillSummary never touches those fields at runtime. handlers.ts's signature for serializeSkillSummary now accepts ISkillSummary too. - Document the intentional second-round-trip `findOne` in upsertSkillFile. Switching to `findOneAndUpdate({ new: false })` was required for TOCTOU-safe insert-vs-replace detection, which means the handler needs a follow-up query to return the post-upsert document. A comment now explains the tradeoff so future readers don't silently "optimize" it away. No behavior change. * 🌐 fix: Wire SKILL into SHARE_PUBLIC Resource Maps Address codex comment #1 — making a skill public was blocked on two hardcoded resource→permission-type maps that didn't know about SKILL: - api/server/middleware/checkSharePublicAccess.js's resourceToPermissionType map was missing ResourceType.SKILL, so PUT /api/permissions/skill/:id with { public: true } would fall through to the 400 "Unsupported resource type for public sharing" path even though PermissionTypes.SKILLS exists and ADMIN has SHARE_PUBLIC configured. Added the mapping. - client/src/hooks/Sharing/useCanSharePublic.ts has an identical client-side map used to gate the "Make Public" UI toggle. Without the SKILL mapping the hook returned false for everyone, so the toggle wouldn't render for skills once the sharing UI lands in phase 2. Added the mapping. Codex comment #2 (create/update cache writes inject skills into unrelated filtered lists) is invalid — it flags a pattern that mirrors useUpdatePromptGroup (which the PR description explicitly cites as the model) and is a deliberate optimistic-update tradeoff. Trying to match each cache key's embedded filter would couple the mutation callback to query-key internals, which is exactly what setQueriesData is designed to avoid. No change there. * 🧪 feat: Frontmatter Validation, Reserved-Name Fixes, Coaching Warnings Address the follow-up review notes on the Skills PR. This commit closes the gap between the wire-type promise and what the backend actually enforces, tightens the reserved-name rules, and adds a non-blocking coaching tier for validators. Frontmatter validation (new): - Add `validateSkillFrontmatter` in data-schemas/methods/skill.ts with strict mode — unknown keys are rejected so expanding the allowed set is an intentional code change. Known keys are type-checked against a `FrontmatterKind` table derived from Anthropic's Agent Skills spec (name, description, when-to-use, allowed-tools, arguments, argument-hint, user-invocable, disable-model-invocation, model, effort, context, agent, paths, shell, hooks, version, metadata). - `hooks` and `metadata` get a shallow JSON-safety check (max depth 4, max string 2000, max array 100) instead of a full schema, since their full shapes live outside this module. - Wired into BOTH createSkill AND updateSkill so the PATCH path can't smuggle invalid frontmatter past the validator. Validation warning tier (new): - Add optional `severity: 'error' \| 'warning'` to `ValidationIssue` (defaults to error). `partitionIssues` splits an issue list into blocking errors and non-blocking warnings. - `createSkill` / `updateSkill` filter on errors for the throw check and return warnings in a new `warnings: ValidationIssue[]` field on their result objects (`CreateSkillResult` / `UpdateSkillResult`). - `validateSkillDescription` now emits a `TOO_SHORT` warning for descriptions under 20 chars — the primary triggering field, so a little coaching goes a long way. - `createHandler` / `patchHandler` in packages/api surface the warnings via a new `attachWarnings` helper that decorates the serialized response with a `warnings?: TSkillWarning[]` field. - `TSkill` gains an optional `warnings?: TSkillWarning[]` field documented as "present on POST/PATCH, never on GET". Reserved-name filter (tightened): - Replace the substring match (`.includes('anthropic')`) with prefix matching on `anthropic-` and `claude-` plus exact-match rejection of CLI slash-command collisions (help, clear, compact, model, exit, quit, settings, plus the bare `anthropic` / `claude` words). Both the pure validator (`methods/skill.ts`) and the Mongoose schema validator (`schema/skill.ts`) updated in lockstep; comments on each reference the other to prevent drift. - `research-anthropic-helper` and `about-claude` are now allowed; `anthropic-helper`, `claude-bot`, and `settings` are still rejected. Documentation: - Add docstrings on `ISkill`, `schema/skill.ts`, and `TSkill` explaining the semantics of `name` (Claude-visible identifier, kebab-case, stable), `displayTitle` (UI-only cosmetic label, NOT sent to Claude), `description` (highest-leverage trigger field), and `source` / `sourceMetadata` (reserved for phase 2+ external sync). - Add a detailed consistency comment on `bumpSkillVersionAndAdjustFileCount` explaining that it runs as a separate MongoDB operation from upsertSkillFile/deleteSkillFile, so `fileCount` can drift if the second op fails — options listed, tradeoff documented, phase 1 risk window noted as closed because upload is still stubbed. Tests: - data-schemas skill.spec.ts: destructure `{ skill, warnings }` from createSkill at every call site; add a TOO_SHORT warning test, a frontmatter strict-mode test, reserved-prefix tests (including positive cases for substring names that should pass), CLI reserved word tests, and a full `validateSkillFrontmatter` describe block covering unknown keys, type mismatches, and deep-nesting rejection. - api/server/routes/skills.test.js: bump default test description above the 20-char threshold, add a warning-emission test, add reserved-prefix + reserved-CLI-word tests, add an unknown-frontmatter- key test asserting the 400 response carries `issues` with `UNKNOWN_KEY`. * 📦 fix: Export CreateSkillResult from data-schemas Methods Index `CreateSkillResult` was defined in `methods/skill.ts` and consumed by `packages/api/src/skills/handlers.ts` but never re-exported from the methods barrel, so the type-check job failed with TS2724 "'@librechat/data-schemas' has no exported member named 'CreateSkillResult'". Rollup's bundle-mode build picked up the type via its internal resolver, but the standalone `tsc --noEmit` type-check ran against the package's public entrypoint and couldn't see it. Added the type import + export alongside the existing `UpdateSkillResult` export, which fixes the CI type-check without any runtime change.	2026-04-25 04:01:59 -04:00
Dustin Healy	fc3189b718	🔐 fix: Restore Tenant Context in MCP OAuth Callback (#12782 ) * fix: restore tenant context in MCP OAuth callback for multi-tenant deployments The MCP OAuth callback is a cross-origin redirect from the OAuth provider. SameSite=Strict cookies (including the JWT) are not sent, leaving the callback with no tenant context. With TENANT_ISOLATION_STRICT=true, all DB writes fail. Stores tenantId in flow metadata at OAuth initiation time (when the user is authenticated), then restores it via tenantStorage.run in the callback, wrapping the entire post-validation body. * test: address review findings for tenant context tests - Assert tenantId flows through to initFlow in MCPConnectionFactory test - Add beforeEach to tenant context tests to reset mocks independently	2026-04-22 14:05:51 -07:00
Dustin Healy	55fc37ff49	🔐 fix: Add Tenant Context to Admin OAuth Callback Routes (#12579 ) * 🔐 fix: Add Tenant Context to Admin OAuth Callback Routes * fix: add tenant context to admin local login route	2026-04-08 20:04:15 -04:00
Danny Avila	d350c58633	🚫 fix: Hide Delete Account Button When ALLOW_ACCOUNT_DELETION Is Disabled (#12568 ) * fix: hide Delete Account button when ALLOW_ACCOUNT_DELETION is false * fix: add admin bypass, inline env read, and tests for allowAccountDeletion - Show delete button for admin users even when ALLOW_ACCOUNT_DELETION=false, matching the canDeleteAccount middleware's ACCESS_ADMIN bypass - Move env var read inline in buildSharedPayload() for per-request evaluation - Add 4 frontend tests for Account conditional rendering - Add 3 backend tests for allowAccountDeletion config field * fix: use server-side ACCESS_ADMIN capability check instead of frontend role check - Replace frontend SystemRoles.ADMIN check with server-side hasCapability() in the authenticated config route, matching canDeleteAccount middleware exactly - Admin bypass now evaluates ACCESS_ADMIN capability per-user in GET /api/config, so users with the grant (regardless of role) see the button, and admins without the grant do not - Add 3 authenticated backend tests: without capability, with capability, and skip-when-already-enabled - Simplify frontend to pure config check (no role logic) - Remove redundant jest-dom import; add inline env var comment * test: add missing toHaveBeenCalled assertion in ACCESS_ADMIN test	2026-04-07 23:51:23 -04:00
Denis Palnitsky	8ed0bcf5ca	♻️ fix: Reuse Existing MCP OAuth Client Registrations to Prevent `client_id` Mismatch (#11925 ) * fix: reuse existing OAuth client registrations to prevent client_id mismatch When using auto-discovered OAuth (DCR), LibreChat calls /register on every flow initiation, getting a new client_id each time. When concurrent connections or reconnections happen, the client_id used during /authorize differs from the one used during /token, causing the server to reject the exchange. Before registering a new client, check if a valid client registration already exists in the database and reuse it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Handle re-registration of OAuth clients when redirect_uri changes * Add undefined fields for logo_uri and tos_uri in OAuth metadata tests * test: add client registration reuse tests for horizontal scaling race condition Reproduces the client_id mismatch bug that occurs in multi-replica deployments where concurrent initiateOAuthFlow calls each register a new OAuth client. Tests verify that the findToken-based client reuse prevents re-registration. * fix: address review findings for client registration reuse - Fix empty redirect_uris bug: invert condition so missing/empty redirect_uris triggers re-registration instead of silent reuse - Revert undocumented config?.redirect_uri in auto-discovery path - Change DB error logging from debug to warn for operator visibility - Fix import order: move package type import to correct section - Remove redundant type cast and misleading JSDoc comment - Test file: remove dead imports, restore process.env.DOMAIN_SERVER, rename describe blocks, add empty redirect_uris edge case test, add concurrent reconnection test with pre-seeded token, scope documentation to reconnection stabilization * fix: resolve type check errors for OAuthClientInformation redirect_uris The SDK's OAuthClientInformation type lacks redirect_uris (only on OAuthClientInformationFull). Cast to the local OAuthClientInformation type in handler.ts when accessing deserialized client info from DB, and use intersection types in tests for clientInfo with redirect_uris. * fix: address follow-up review findings R1, R2, R3 - R1: Move `import type { TokenMethods }` to the type-imports section, before local types, per CLAUDE.md import order rules - R2: Add unit test for empty redirect_uris in handler.test.ts to verify the inverted condition triggers re-registration - R3: Use delete for process.env.DOMAIN_SERVER restoration when the original value was undefined to avoid coercion to string "undefined" * fix: clear stale client registration on OAuth flow failure When a stored client_id is no longer recognized by the OAuth server, the flow fails but the stale client stays in MongoDB, causing every retry to reuse the same invalid registration in an infinite loop. On OAuth failure, clear the stored client registration so the next attempt falls through to fresh Dynamic Client Registration. - Add MCPTokenStorage.deleteClientRegistration() for targeted cleanup - Call it from MCPConnectionFactory's OAuth failure path - Add integration test proving recovery from stale client reuse * fix: validate auth server identity and target cleanup to reused clients - Gate client reuse on authorization server identity: compare stored issuer against freshly discovered metadata before reusing, preventing wrong-client reuse when the MCP server switches auth providers - Add reusedStoredClient flag to MCPOAuthFlowMetadata so cleanup only runs when the failed flow actually reused a stored registration, not on unrelated failures (timeouts, user-denied consent, etc.) - Add cleanup in returnOnOAuth path: when a prior flow that reused a stored client is detected as failed, clear the stale registration before re-initiating - Add tests for issuer mismatch and reusedStoredClient flag assertions * fix: address minor review findings N3, N5, N6 - N3: Type deleteClientRegistration param as TokenMethods['deleteTokens'] instead of Promise<unknown> - N5: Elevate deletion failure logging from debug to warn for operator visibility when stale client cleanup fails - N6: Use getLogPrefix() instead of hardcoded log prefix to respect system-user privacy convention * fix: correct stale-client cleanup in both OAuth paths - Blocking path: remove result?.clientInfo guard that made cleanup unreachable (handleOAuthRequired returns null on failure, so result?.clientInfo was always false in the failure branch) - returnOnOAuth path: only clear stored client when the prior flow status is FAILED, not on COMPLETED or PENDING flows, to avoid deleting valid registrations during normal flow replacement * fix: remove redundant cast on clientMetadata clientMetadata is already typed as Record<string, unknown>; the as Record<string, unknown> cast was a no-op. * fix: thread reusedStoredClient through return type instead of re-reading flow state FlowStateManager.createFlow() deletes FAILED flow state before rejecting, so getFlowState() after handleOAuthRequired() returns null would find nothing — making the stale-client cleanup dead code. Fix: hoist reusedStoredClient flag from flowMetadata into a local variable, include it in handleOAuthRequired()'s return type (both success and catch paths), and use result.reusedStoredClient directly in the caller instead of a second getFlowState() round-trip. * fix: selective stale-client cleanup in returnOnOAuth path The returnOnOAuth cleanup was unreliable: it depended on reading FAILED flow state, but FlowStateManager.monitorFlow() deletes FAILED state before rejecting. Move cleanup into createFlow's catch handler where flowMetadata.reusedStoredClient is still in scope. Make cleanup selective in both paths: add isClientRejection() helper that only matches errors indicating the OAuth server rejected the client_id (invalid_client, unauthorized_client, client not found). Timeouts, user-cancelled flows, and other transient failures no longer wipe valid stored registrations. Thread the error from handleOAuthRequired() through the return type so the blocking path can also check isClientRejection(). * fix: tighten isClientRejection heuristic Narrow 'client_id' match to 'client_id mismatch' to avoid false-positive cleanup on unrelated errors that happen to mention client_id. * test: add isClientRejection tests and enforced client_id on test server - Add isClientRejection unit tests: invalid_client, unauthorized_client, client_id mismatch, client not found, unknown client, and negative cases (timeout, flow state not found, user denied, null, undefined) - Enhance OAuth test server with enforceClientId option: binds auth codes to the client_id that initiated /authorize, rejects token exchange with mismatched or unregistered client_id (401 invalid_client) - Add integration tests proving the test server correctly rejects stale client_ids and accepts matching ones at /token * fix: issuer validation, callback error propagation, and cleanup DRY - Issuer check: re-register when storedIssuer is absent or non-string instead of silently reusing. Narrows unknown type with typeof guard and inverts condition so missing issuer → fresh DCR (safer default). - OAuth callback route: call failFlow with the OAuth error when the authorization server redirects back with error= parameter, so the waiting flow receives the actual rejection instead of timing out. This lets isClientRejection match stale-client errors correctly. - Extract duplicated cleanup block to clearStaleClientIfRejected() private method, called from both returnOnOAuth and blocking paths. - Test fixes: add issuer to stored metadata in reuse tests, reset server to undefined in afterEach to prevent double-close. * fix: gate failFlow behind callback validation, propagate reusedStoredClient on join - OAuth callback: move failFlow call to after CSRF/session/active-flow validation so an attacker with only a leaked state parameter cannot force-fail a flow without passing the same integrity checks required for legitimate callbacks - PENDING join path: propagate reusedStoredClient from flow metadata into the return object so joiners can trigger stale-client cleanup if the joined flow later fails with a client rejection * fix: restore early oauthError/code redirects, gate only failFlow behind CSRF The previous restructuring moved oauthError and missing-code checks behind CSRF validation, breaking tests that expect those redirects without cookies. The redirect itself is harmless (just shows an error page). Only the failFlow call needs CSRF gating to prevent DoS. Restructure: oauthError check stays early (redirects immediately), but failFlow inside it runs the full CSRF/session/active-flow validation before marking the flow as FAILED. * fix: require deleteTokens for client reuse, add missing import in MCP.js Client registration reuse without cleanup capability creates a permanent failure loop: if the reused client is stale, the code detects the rejection but cannot clear the stored registration because deleteTokens is missing, so every retry reuses the same broken client_id. - MCPConnectionFactory: only pass findToken to initiateOAuthFlow when deleteTokens is also available, ensuring reuse is only enabled when recovery is possible - api/server/services/MCP.js: add deleteTokens to the tokenMethods object (was the only MCP call site missing it) * fix: set reusedStoredClient before createFlow in joined-flow path When joining a PENDING flow, reusedStoredClient was only set on the success return but not before the await. If createFlow throws (e.g. invalid_client during token exchange), the outer catch returns the local variable which was still false, skipping stale-client cleanup. * fix: require browser binding (CSRF/session) for failFlow on OAuth error hasActiveFlow only proves a PENDING flow exists, not that the caller is the same browser that initiated it. An attacker with a leaked state could force-fail the flow without any user binding. Require hasCsrf or hasSession before calling failFlow on the oauthError path. * fix: guard findToken with deleteTokens check in blocking OAuth path Match the returnOnOAuth path's defense-in-depth: only enable client registration reuse when deleteTokens is also available, ensuring cleanup is possible if the reused client turns out to be stale. * fix: address review findings — tests, types, normalization, docs - Add deleteTokens method to InMemoryTokenStore matching TokenMethods contract; update test call site from deleteToken to deleteTokens - Add MCPConnectionFactory test: returnOnOAuth flow fails with invalid_client → clearStaleClientIfRejected invoked automatically - Add mcp.spec.js tests: OAuth error with CSRF → failFlow called; OAuth error without cookies → failFlow NOT called (DoS prevention) - Add JSDoc to isClientRejection with RFC 6749 and vendor attribution - Add inline comment explaining findToken/deleteTokens coupling guard - Normalize issuer comparison: strip trailing slashes to prevent spurious re-registrations from URL formatting differences - Fix dead-code: use local reusedStoredClient variable in PENDING join return instead of re-reading flowMeta * fix: address final review nits N1-N4 - N1: Add session cookie failFlow test — validates the hasSession branch triggers failFlow on OAuth error callback - N2: Replace setTimeout(50) with setImmediate for microtask drain - N3: Add 'unknown client' attribution to isClientRejection JSDoc - N4: Remove dead getFlowState mock from failFlow tests --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-03 22:15:08 -04:00
Dustin Healy	261941c05f	🔨 fix: Custom Role Permissions (#12528 ) * fix: Resolve custom role permissions not loading in frontend Users assigned to custom roles (non-USER/ADMIN) had all permission checks fail because AuthContext only fetched system role permissions. The roles map keyed by USER/ADMIN never contained the custom role name, so useHasAccess returned false for every feature gate. - Fetch the user's custom role in AuthContext and include it in the roles map so useHasAccess can resolve permissions correctly - Use encodeURIComponent instead of toLowerCase for role name URLs to preserve custom role casing through the API roundtrip - Only uppercase system role names on the backend GET route; pass custom role names through as-is for exact DB lookup - Allow users to fetch their own assigned role without READ_ROLES capability * refactor: Normalize all role names to uppercase Custom role names were stored in original casing, causing case-sensitivity bugs across the stack — URL lowercasing, route uppercasing, and case-sensitive DB lookups all conflicted for mixed-case custom roles. Enforce uppercase normalization at every boundary: - createRoleByName trims and uppercases the name before storage - createRoleHandler uppercases before passing to createRoleByName - All admin route handlers (get, update, delete, members, permissions) uppercase the :name URL param before DB lookups - addRoleMemberHandler uppercases before setting user.role - Startup migration (normalizeRoleNames) finds non-uppercase custom roles, renames them, and updates affected user.role values with collision detection Legacy GET /api/roles/:roleName retains always-uppercase behavior. Tests updated to expect uppercase role names throughout. * fix: Use case-preserved role names with strict equality Remove uppercase normalization — custom role names are stored and compared exactly as the user sets them, with only trimming applied. USER and ADMIN remain reserved case-insensitively via isSystemRoleName. - Remove toUpperCase from createRoleByName, createRoleHandler, and all admin route handlers (get, update, delete, members, permissions) - Remove toUpperCase from legacy GET and PUT routes in roles.js; the frontend now sends exact casing via encodeURIComponent - Remove normalizeRoleNames startup migration - Revert test expectations to original casing * fix: Format useMemo dependency array for Prettier * feat: Add custom role support to admin settings + review fixes - Add backend tests for isOwnRole authorization gate on GET /api/roles/:roleName - Add frontend tests for custom role detection and fetching in AuthContext - Fix transient null permission flash by only spreading custom role once loaded - Add isSystemRoleName helper to data-provider for case-insensitive system role detection - Use sentinel value in useGetRole to avoid ghost cache entry from empty string - Add useListRoles hook and listRoles data service for fetching all roles - Update AdminSettingsDialog and PeoplePickerAdminSettings to dynamically list custom roles in the role dropdown, with proper fallback defaults * fix: Address review findings for custom role permissions - Add assertions to AuthContext test verifying custom role in roles map - Fix empty array bypassing nullish coalescing fallback in role dropdowns - Add null/undefined guard to isSystemRoleName helper - Memoize role dropdown items to avoid unnecessary re-renders - Apply sentinel pattern to useGetRole in admin settings for consistency - Mark ListRolesResponse description as required to match schema * fix: Prevent prototype pollution in role authorization gate - Replace roleDefaults[roleName] with Object.hasOwn to prevent prototype chain bypass for names like constructor or __proto__ - Add dedicated rolesList query key to avoid cache collision when a custom role is named 'list' - Add regression test for prototype property name authorization * fix: Resolve Prettier formatting and unused variable lint errors * fix: Address review findings for custom role permissions - Add ADMIN self-read test documenting isOwnRole bypass behavior - Guard save button while custom role data loads to prevent data loss - Extract useRoleSelector hook eliminating ~55 lines of duplication - Unify defaultValues/useEffect permission resolution (fixes inconsistency) - Make ListRolesResponse.description and _id optional to match schema - Fix vacuous test assertions to verify sentinel calls exist - Only fetch userRole when user.role === USER (avoid unnecessary requests) - Remove redundant empty string guard in custom role detection * fix: Revert USER role fetch restriction to preserve admin settings Admins need the USER role loaded in AuthContext.roles so the admin settings dialog shows persisted USER permissions instead of defaults. * fix: Remove unused useEffect import from useRoleSelector * fix: Clean up useRoleSelector hook - Use existing isCustom variable instead of re-calling isSystemRoleName - Remove unused roles and availableRoleNames from return object * fix: Address review findings for custom role permissions - Use Set-based isSystemRoleName to auto-expand with future SystemRoles - Add isCustomRoleError handling: guard useEffect reset and disable Save - Remove resolvePermissions from hook return; use defaultValues in useEffect to eliminate redundant computation and stale-closure reset race - Rename customRoleName to userRoleName in AuthContext for clarity * fix: Request server-max roles for admin dropdown listRoles now passes limit=200 (the server's MAX_PAGE_LIMIT) so the admin role selector shows all roles instead of silently truncating at the default page size of 50. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-03 13:24:11 -04:00
Danny Avila	fa4a43da21	🔐 fix: Strip `code_challenge` from Admin OAuth requests before Passport (#12534 ) * 🔐 fix: Strip code_challenge from admin OAuth requests before Passport openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. * 🔐 fix: Strip code_challenge from admin OAuth requests before Passport openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. - Fix regex to handle `code_challenge` in any query position without producing malformed URLs, and handle empty `code_challenge=` values (`[^&]` vs `[^&]+`) - Combine `storePkceChallenge` + `stripCodeChallenge` into a single `storeAndStripChallenge` helper to enforce read-store-strip ordering - Apply defensively to all 7 admin OAuth providers - Add 12 unit tests covering stripCodeChallenge and storeAndStripChallenge refactor: Extract PKCE helpers to utility file, harden tests - Move stripCodeChallenge and storeAndStripChallenge to api/server/utils/adminPkce.js — eliminates _test production export and avoids loading the full auth.js module tree in tests - Add missing req.originalUrl/req.url assertions to invalid-challenge and no-challenge test branches (regression blind spots) - Hoist cache reference to module scope in tests (was redundantly re-acquired from mock factory on every beforeEach) * chore: Address review NITs — imports, exports, naming, assertions - Fix import order in auth.js (longest-to-shortest per CLAUDE.md) - Remove unused PKCE_CHALLENGE_TTL/PKCE_CHALLENGE_PATTERN exports - Hoist strip arrow to module-scope stripChallengeFromUrl - Rename auth.test.js → auth.spec.js (project convention) - Tighten cache-failure test: toBe instead of toContain, add req.url * refactor: Move PKCE helpers to packages/api with dependency injection Move stripCodeChallenge and storeAndStripChallenge from api/server/utils into packages/api/src/auth/exchange.ts alongside the existing PKCE verification logic. Cache is now injected as a Keyv parameter, matching the dependency-injection pattern used throughout packages/api/. - Add PkceStrippableRequest interface for minimal req typing - auth.js imports storeAndStripChallenge from @librechat/api - Delete api/server/utils/adminPkce.js - Move tests to packages/api/src/auth/adminPkce.spec.ts (TypeScript, real Keyv instances, no getLogStores mock needed)	2026-04-02 21:03:44 -04:00
Dustin Healy	2451bf54cf	🛡️ fix: Restrict System Grants to Role Principals (#12491 ) * 🛡️ fix: restrict system grants to role principals only Narrows GrantPrincipalType to PrincipalType.ROLE, rejecting GROUP and USER with 400. Removes grant cascade cleanup from group/user deletion handlers and their route wiring since only roles can hold grants. * 🛡️ fix: address review findings for grants roles-only restriction Add missing GROUP rejection test for revokeGrant (symmetric with getPrincipalGrants and assignGrant coverage), add extensibility comment to GrantPrincipalType, and document the checkRoleExists guard.	2026-03-31 19:25:14 -04:00
Danny Avila	2e706ebcb3	⚖️ refactor: Split Config Route into Unauthenticated and Authenticated Paths (#12490 ) * refactor: split /api/config into unauthenticated and authenticated response paths - Replace preAuthTenantMiddleware with optionalJwtAuth on the /api/config route so the handler can detect whether the request is authenticated - When unauthenticated: call getAppConfig({ baseOnly: true }) for zero DB queries, return only login-relevant fields (social logins, turnstile, privacy policy / terms of service from interface config) - When authenticated: call getAppConfig({ role, userId, tenantId }) to resolve per-user DB overrides (USER + ROLE + GROUP + PUBLIC principals), return full payload including modelSpecs, balance, webSearch, etc. - Extract buildSharedPayload() and addWebSearchConfig() helpers to avoid duplication between the two code paths - Fixes per-user balance overrides not appearing in the frontend because userId was never passed to getAppConfig (follow-up to #12474) * test: rewrite config route tests for unauthenticated vs authenticated paths - Replace the previously-skipped supertest tests with proper mocked tests - Cover unauthenticated path: baseOnly config call, minimal payload, interface subset (privacyPolicy/termsOfService only), exclusion of authenticated-only fields - Cover authenticated path: getAppConfig called with userId, full payload including modelSpecs/balance/webSearch, per-user balance override merging * fix: address review findings — restore multi-tenant support, improve tests - Chain preAuthTenantMiddleware back before optionalJwtAuth on /api/config so unauthenticated requests in multi-tenant deployments still get tenant-scoped config via X-Tenant-Id header (Finding #1) - Use getAppConfig({ tenantId }) instead of getAppConfig({ baseOnly: true }) when a tenant context is present; fall back to baseOnly for single-tenant - Fix @type annotation: unauthenticated payload is Partial<TStartupConfig> - Refactor addWebSearchConfig into pure buildWebSearchConfig that returns a value instead of mutating the payload argument - Hoist isBirthday() to module level - Remove inline narration comments - Assert tenantId propagation in tests, including getTenantId fallback and user.tenantId preference - Add error-path tests for both unauthenticated and authenticated branches - Expand afterEach env var cleanup for proper test isolation * test: fix mock isolation and add tenant-scoped response test - Replace jest.clearAllMocks() with jest.resetAllMocks() so mockReturnValue implementations don't leak between tests - Add test verifying tenant-scoped socialLogins and turnstile are correctly mapped in the unauthenticated response * fix: add optionalJwtAuth to /api/config in experimental.js Without this middleware, req.user is never populated in the experimental cluster entrypoint, so authenticated users always receive the minimal unauthenticated config payload.	2026-03-31 19:22:51 -04:00
Dustin Healy	3d1b883e9d	👨‍👨‍👦‍👦 feat: Admin Users API Endpoints (#12446 ) * feat: add admin user management endpoints Add /api/admin/users with list, search, and delete handlers gated by ACCESS_ADMIN + READ_USERS/MANAGE_USERS system grants. Handler factory in packages/api uses findUsers, countUsers, and deleteUserById from data-schemas. * fix: address convention violations in admin users handlers * fix: add pagination, self-deletion guard, and DB-level search limit - listUsers now uses parsePagination + countUsers for proper pagination matching the roles/groups pattern - findUsers extended with optional limit/offset options - deleteUser returns 403 when caller tries to delete own account - searchUsers passes limit to DB query instead of fetching all and slicing in JS - Fix import ordering per CLAUDE.md, complete logger mock - Replace fabricated date fallback with undefined * fix: deterministic sort, null-safe pagination, consistent search filter - Add sort option to findUsers; listUsers sorts by createdAt desc for deterministic pagination - Use != null guards for offset/limit to handle zero values correctly - Remove username from search filter since it is not in the projection or AdminUserSearchResult response type * fix: last-admin deletion guard and search query max-length - Prevent deleting the last admin user (look up target role, count admins, reject with 400 if count <= 1) - Cap search query at 200 characters to prevent regex DoS - Add tests for both guards * fix: include missing capability name in 403 Forbidden response * fix: cascade user deletion cleanup, search username, parallel capability checks - Cascade Config, AclEntry, and SystemGrant cleanup on user deletion (matching the pattern in roles/groups handlers) - Add username to admin search $or filter for parity with searchUsers - Parallelize READ_* capability checks in listAllGrants with Promise.all * fix: TOCTOU safety net, capability info leak, DRY/style cleanup, data-layer tests - Add post-delete admin recount with CRITICAL log if race leaves 0 admins - Revert capability name from 403 response to server-side log only - Document thin deleteUserById limitation (full cascade is a future task) - DRY: extract query.trim() to local variable in searchUsersHandler - Add username to search projection, response type, and AdminUserSearchResult - Functional filter/map in grants.ts parallel capability check - Consistent null guards and limit>0 guard in findUsers options - Fallback for empty result.message on delete response - Fix mockUser() to generate unique _id per call - Break long destructuring across multiple lines - Assert countUsers filter and non-admin skip in delete tests - Add data-layer tests for findUsers limit, offset, sort, and pagination * chore: comment out admin delete user endpoint (out of scope) * fix: cast USER principalId to ObjectId for ACL entry cleanup ACL entries store USER principalId as ObjectId (via grantPermission casting), but deleteAclEntries is a raw deleteMany that passes the filter through. Passing a string won't match stored ObjectIds, leaving orphaned entries. * chore: comment out unused requireManageUsers alongside disabled delete route * fix: add missing logger.warn mock in capabilities test * fix: harden admin users handlers — type safety, response consistency, test coverage - Unify response shape: AdminUserSearchResult.userId → id, add AdminUserListItem type - Fix unsafe req.query type assertion in searchUsersHandler (typeof guards) - Anchor search regex with ^ for prefix matching (enables index usage) - Add total/capped to search response for truncation signaling - Add parseInt radix, remove redundant new Date() wrap - Add tests: countUsers throw, countUsers call args, array query param, capped flag * fix: scope deleteGrantsForPrincipal to tenant, deterministic search sort, align test mocks - Add tenantId option to AdminUsersDeps.deleteGrantsForPrincipal and pass req.user.tenantId at the call site, matching the pattern already used by the roles and groups handlers - Add sort: { name: 1 } to searchUsersHandler for deterministic results - Align test mock deleteUserById messages with production output ('User was deleted successfully.') - Make capped-results test explicitly set limit: '20' instead of relying on the implicit default * test: add tenantId propagation test for deleteGrantsForPrincipal Add tenantId to createReqRes user type and test that a non-undefined tenantId is threaded through to deleteGrantsForPrincipal. * test: remove redundant deleteUserById override in tenantId test --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-03-30 23:06:50 -04:00
Danny Avila	4f37e8adb9	🔐 feat: Admin Auth Support for SAML and Social OAuth Providers (#12472 ) * refactor: Add existingUsersOnly support to social and SAML auth callbacks - Add `existingUsersOnly` option to the `socialLogin` handler factory to reject unknown users instead of creating new accounts - Refactor SAML strategy callback into `createSamlCallback(existingUsersOnly)` factory function, mirroring the OpenID `createOpenIDCallback` pattern - Extract shared SAML config into `getBaseSamlConfig()` helper - Register `samlAdmin` passport strategy with `existingUsersOnly: true` and admin-specific callback URL, called automatically from `setupSaml()` * feat: Register admin OAuth strategy variants for all social providers - Add admin strategy exports to Google, GitHub, Discord, Facebook, and Apple strategy files with admin callback URLs and existingUsersOnly - Extract provider configs into reusable helpers to avoid duplication between regular and admin strategy constructors - Re-export all admin strategy factories from strategies/index.js - Register admin passport strategies (googleAdmin, githubAdmin, etc.) alongside regular ones in socialLogins.js when env vars are present * feat: Add admin auth routes for SAML and social OAuth providers - Add initiation and callback routes for SAML, Google, GitHub, Discord, Facebook, and Apple to the admin auth router - Each provider follows the exchange code + PKCE pattern established by OpenID admin auth: store PKCE challenge on initiation, retrieve on callback, generate exchange code for the admin panel - SAML and Apple use POST callbacks with state extracted from req.body.RelayState and req.body.state respectively - Extract storePkceChallenge(), retrievePkceChallenge(), and generateState() helpers; refactor existing OpenID routes to use them - All callback chains enforce requireAdminAccess, setBalanceConfig, checkDomainAllowed, and the shared createOAuthHandler - No changes needed to the generic POST /oauth/exchange endpoint * fix: Update SAML strategy test to handle dual strategy registration setupSaml() now registers both 'saml' and 'samlAdmin' strategies, causing the SamlStrategy mock to be called twice. The verifyCallback variable was getting overwritten with the admin callback (which has existingUsersOnly: true), making all new-user tests fail. Fix: capture only the first callback per setupSaml() call and reset between tests. * fix: Address review findings for admin OAuth strategy changes - Fix existingUsersOnly rejection in socialLogin.js to use cb(null, false, { message }) instead of cb(error), ensuring passport's failureRedirect fires correctly for admin flows - Consolidate duplicate require() calls in strategies/index.js by destructuring admin exports from the already-imported default export - Pass pre-parsed baseConfig to setupSamlAdmin() to avoid redundant certificate file I/O at startup - Extract getGoogleConfig() helper in googleStrategy.js for consistency with all other provider strategy files - Replace randomState() (openid-client) with generateState() (crypto) in the OpenID admin route for consistency with all other providers, and remove the now-unused openid-client import * Reorder import statements in auth.js	2026-03-30 22:49:44 -04:00
Danny Avila	2bf0f892d6	🛡️ fix: Add Origin Binding to Admin OAuth Exchange Codes (#12469 ) * fix(auth): add origin binding to admin OAuth exchange codes Bind admin OAuth exchange codes to the admin panel's origin at generation time and validate the origin on redemption. This prevents an intercepted code (via referrer leakage, logs, or network capture) from being redeemed by a different origin within the 30-second TTL. - Store the admin panel origin alongside the exchange code in cache - Extract the request origin (from Origin/Referer headers) on the exchange endpoint and pass it for validation - Reject code redemption when the request origin does not match the stored origin (code is still consumed to prevent replay) - Backward compatible: codes without a stored origin are accepted * fix(auth): add PKCE proof-of-possession to admin OAuth exchange codes Add a PKCE-like code_challenge/code_verifier flow to the admin OAuth exchange so that intercepting the exchange code alone is insufficient to redeem it. The admin panel generates a code_verifier (stored in its HttpOnly session cookie) and sends sha256(verifier) as code_challenge through the OAuth initiation URL. LibreChat stores the challenge keyed by OAuth state and attaches it to the exchange code. On redemption, the admin panel sends the verifier and LibreChat verifies the hash match. - Add verifyCodeChallenge() helper using SHA-256 - Store code_challenge in ADMIN_OAUTH_EXCHANGE cache (pkce: prefix, 5min TTL) - Capture OAuth state in callback middleware before passport processes it - Accept code_verifier in exchange endpoint body - Backward compatible: no challenge stored → PKCE check skipped * fix(auth): harden PKCE and origin binding in admin OAuth exchange - Await cache.set for PKCE challenge storage with error handling - Use crypto.timingSafeEqual for PKCE hash comparison - Drop case-insensitive flag from hex validation regexes - Add code_verifier length validation (max 512 chars) - Normalize Origin header via URL parsing in resolveRequestOrigin - Add test for undefined requestOrigin rejection - Clarify JSDoc: hex-encoded SHA-256, not RFC 7636 S256 * fix(auth): fail closed on PKCE callback cache errors, clean up origin/buffer handling - Callback middleware now redirects to error URL on cache.get failure instead of silently continuing without PKCE challenge - resolveRequestOrigin returns undefined (not raw header) on parse failure - Remove dead try/catch around Buffer.from which never throws for string input * chore(auth): remove narration comments, scope eslint-disable to lines * chore(auth): narrow query.state to string, remove narration comments in exchange.ts * fix(auth): address review findings — warn on missing PKCE challenge, validate verifier length, deduplicate URL parse - Log warning when OAuth state is present but no PKCE challenge found - Add minimum length check (>= 1) on code_verifier input validation - Update POST /oauth/exchange JSDoc to document code_verifier param - Deduplicate new URL(redirectUri) parse in createOAuthHandler - Restore intent comment on pre-delete pattern in exchangeAdminCode * test(auth): replace mock cache with real Keyv, remove all as-any casts - Use real Keyv in-memory store instead of hand-rolled Map mock - Replace jest.fn mocks with jest.spyOn on real Keyv instance - Remove redundant store.has() assertion, use cache.get() instead - Eliminate all eslint-disable and as-any suppressions - User fixture no longer needs any cast (Keyv accepts plain objects) * fix(auth): add IUser type cast for test fixture to satisfy tsc	2026-03-30 16:54:00 -04:00
Danny Avila	fda72ac621	🏗️ refactor: Remove Redundant Caching, Migrate Config Services to TypeScript (#12466 ) * ♻️ refactor: Remove redundant scopedCacheKey caching, support user-provided key model fetching Remove redundant cache layers that used `scopedCacheKey()` (tenant-only scoping) on top of `getAppConfig()` which already caches per-principal (role+user+tenant). This caused config overrides for different principals within the same tenant to be invisible due to stale cached data. Changes: - Add `requireJwtAuth` to `/api/endpoints` route for proper user context - Remove ENDPOINT_CONFIG, STARTUP_CONFIG, PLUGINS, TOOLS, and MODELS_CONFIG cache layers — all derive from `getAppConfig()` with cheap computation - Enhance MODEL_QUERIES cache: hash(baseURL+apiKey) keys, 2-minute TTL, caching centralized in `fetchModels()` base function - Support fetching models with user-provided API keys in `loadConfigModels` via `getUserKeyValues` lookup (no caching for user keys) - Update all affected tests Closes #1028 * ♻️ refactor: Migrate config services to TypeScript in packages/api Move core config logic from CJS /api wrappers to typed TypeScript in packages/api using dependency injection factories: - `createEndpointsConfigService` — endpoint config merging + checkCapability - `createLoadConfigModels` — custom endpoint model loading with user key support - `createMCPToolCacheService` — MCP tool cache operations (update, merge, cache) /api files become thin wrappers that wire dependencies (getAppConfig, loadDefaultEndpointsConfig, getUserKeyValues, getCachedTools, etc.) into the typed factories. Also moves existing `endpoints/config.ts` → `endpoints/config/providers.ts` to accommodate the new `config/` directory structure. * 🔄 fix: Invalidate models query when user API key is set or revoked Without this, users had to refresh the page after entering their API key to see the updated model list fetched with their credentials. - Invalidate QueryKeys.models in useUpdateUserKeysMutation onSuccess - Invalidate QueryKeys.models in useRevokeUserKeyMutation onSuccess - Invalidate QueryKeys.models in useRevokeAllUserKeysMutation onSuccess * 🗺️ fix: Remap YAML-level override keys to AppConfig equivalents in mergeConfigOverrides Config overrides stored in the DB use YAML-level keys (TCustomConfig), but they're merged into the already-processed AppConfig where some fields have been renamed by AppService. This caused mcpServers overrides to land on a nonexistent key instead of mcpConfig, so config-override MCP servers never appeared in the UI. - Add OVERRIDE_KEY_MAP to remap mcpServers→mcpConfig, interface→interfaceConfig - Apply remapping before deep merge in mergeConfigOverrides - Add test for YAML-level key remapping behavior - Update existing tests to use AppConfig field names in assertions * 🧪 test: Update service.spec to use AppConfig field names after override key remapping * 🛡️ fix: Address code review findings — reliability, types, tests, and performance - Pass tenant context (getTenantId) in importers.js getEndpointsConfig call - Add 5 tests for user-provided API key model fetching (key found, no key, DB error, missing userId, apiKey-only with fixed baseURL) - Distinguish NO_USER_KEY (debug) from infrastructure errors (warn) in catch - Switch fetchPromisesMap from Promise.all to Promise.allSettled so one failing provider doesn't kill the entire model config - Parallelize getUserKeyValues DB lookups via batched Promise.allSettled instead of sequential awaits in the loop - Hoist standardCache instance in fetchModels to avoid double instantiation - Replace Record<string, unknown> types with Partial<TConfig>-based types; remove as unknown as T double-cast in endpoints config - Narrow Bedrock availableRegions to typed destructure - Narrow version field from string\|number\|undefined to string\|undefined - Fix import ordering in mcp/tools.ts and config/models.ts per AGENTS.md - Add JSDoc to getModelsConfig alias clarifying caching semantics * fix: Guard against null getCachedTools in mergeAppTools * 🔍 fix: Address follow-up review — deduplicate extractEnvVariable, fix error discrimination, add log-level tests - Deduplicate extractEnvVariable calls: resolve apiKey/baseURL once, reuse for both the entry and isUserProvided checks (Finding A) - Move ResolvedEndpoint interface from function closure to module scope (Finding B) - Replace fragile msg.includes('NO_USER_KEY') with ErrorTypes.NO_USER_KEY enum check against actual error message format (Finding C). Also handle ErrorTypes.INVALID_USER_KEY as an expected "no key" case. - Add test asserting logger.warn is called for infra errors (not debug) - Add test asserting logger.debug is called for NO_USER_KEY errors (not warn) * fix: Preserve numeric assistants version via String() coercion * 🐛 fix: Address secondary review — Ollama cache bypass, cache tests, type safety - Fix Ollama success path bypassing cache write in fetchModels (CRITICAL): store result before returning so Ollama models benefit from 2-minute TTL - Add 4 fetchModels cache behavior tests: cache write with TTL, cache hit short-circuits HTTP, skipCache bypasses read+write, empty results not cached - Type-safe OVERRIDE_KEY_MAP: Partial<Record<keyof TCustomConfig, keyof AppConfig>> so compiler catches future field rename mismatches - Fix import ordering in config/models.ts (package types longest→shortest) - Rename ToolCacheDeps → MCPToolCacheDeps for naming consistency - Expand getModelsConfig JSDoc to explain caching granularity * fix: Narrow OVERRIDE_KEY_MAP index to satisfy strict tsconfig * 🧩 fix: Add allowedProviders to TConfig, remove Record<string, unknown> from PartialEndpointEntry The agents endpoint config includes allowedProviders (used by the frontend AgentPanel to filter available providers), but it was missing from TConfig. This forced PartialEndpointEntry to use & Record<string, unknown> as an escape hatch, violating AGENTS.md type policy. - Add allowedProviders?: (string \| EModelEndpoint)[] to TConfig - Remove Record<string, unknown> from PartialEndpointEntry — now just Partial<TConfig> * 🛡️ fix: Isolate Ollama cache write from fetch try-catch, add Ollama cache tests - Separate Ollama fetch and cache write into distinct scopes so a cache failure (e.g., Redis down) doesn't misattribute the error as an Ollama API failure and fall through to the OpenAI-compatible path (Issue A) - Add 2 Ollama-specific cache tests: models written with TTL on fetch, cached models returned without hitting server (Issue B) - Replace hardcoded 120000 with Time.TWO_MINUTES constant in cache TTL test assertion (Issue C) - Fix OVERRIDE_KEY_MAP JSDoc to accurately describe runtime vs compile-time type enforcement (Issue D) - Add global beforeEach for cache mock reset to prevent cross-test leakage * 🧪 fix: Address third review — DI consistency, cache key width, MCP tests - Inject loadCustomEndpointsConfig via EndpointsConfigDeps with default fallback, matching loadDefaultEndpointsConfig DI pattern (Finding 3) - Widen modelsCacheKey from 64-bit (.slice(0,16)) to 128-bit (.slice(0,32)) for collision-sensitive cross-credential cache key (Finding 4) - Add fetchModels.mockReset() in loadConfigModels.spec beforeEach to prevent mock implementation leaks across tests (Finding 5) - Add 11 unit tests for createMCPToolCacheService covering all three functions: null/empty input, successful ops, error propagation, cold-cache merge (Finding 2) - Simplify getModelsConfig JSDoc to @see reference (Finding 10) * ♻️ refactor: Address remaining follow-ups from reviews OVERRIDE_KEY_MAP completeness: - Add missing turnstile→turnstileConfig mapping - Add exhaustiveness test verifying all three renamed keys are remapped and original YAML keys don't leak through Import role context: - Pass userRole through importConversations job → importLibreChatConvo so role-based endpoint overrides are honored during conversation import - Update convos.js route to include req.user.role in the job payload createEndpointsConfigService unit tests: - Add 8 tests covering: default+custom merge, Azure/AzureAssistants/ Anthropic Vertex/Bedrock config enrichment, assistants version coercion, agents allowedProviders, req.config bypass Plugins/tools efficiency: - Use Set for includedTools/filteredTools lookups (O(1) vs O(n) per plugin) - Combine auth check + filter into single pass (eliminates intermediate array) - Pre-compute toolDefKeys Set for O(1) tool definition lookups * fix: Scope model query cache by user when userIdQuery is enabled * fix: Skip model cache for userIdQuery endpoints, fix endpoints test types - When userIdQuery is true, skip caching entirely (like user_provided keys) to avoid cross-user model list leakage without duplicating cache data - Fix AgentCapabilities type error in endpoints.spec.ts — use enum values and appConfig() helper for partial mock typing * 🐛 fix: Restore filteredTools+includedTools composition, add checkCapability tests - Fix filteredTools regression: whitelist and blacklist are now applied independently (two flat guards), matching original behavior where includedTools=['a','b'] + filteredTools=['b'] produces ['a'] (Finding A) - Fix Set spread in toolkit loop: pre-compute toolDefKeysList array once alongside the Set, reuse for .some() without per-plugin allocation (Finding B) - Add 2 filteredTools tests: blacklist-only path and combined whitelist+blacklist composition (Finding C) - Add 3 checkCapability tests: capability present, capability absent, fallback to defaultAgentCapabilities for non-agents endpoints (Finding D) * 🔑 fix: Include config-override MCP servers in filterAuthorizedTools Config-override MCP servers (defined via admin config overrides for roles/groups) were rejected by filterAuthorizedTools because it called getAllServerConfigs(userId) without the configServers parameter. Only YAML and DB-backed user servers were included in the access check. - Add configServers parameter to filterAuthorizedTools - Resolve config servers via resolveConfigServers(req) at all 4 callsites (create, update, duplicate, revert) using parallel Promise.all - Pass configServers through to getAllServerConfigs(userId, configServers) so the registry merges config-source servers into the access check - Update filterAuthorizedTools.spec.js mock for resolveConfigServers * fix: Skip model cache for userIdQuery endpoints, fix endpoints test types For user-provided key endpoints (userProvide: true), skip the full model list re-fetch during message validation — the user already selected from a list we served them, and re-fetching with skipCache:true on every message send is both slow and fragile (5s provider timeout = rejected model). Instead, validate the model string format only: - Must be a string, max 256 chars - Must match [a-zA-Z0-9][a-zA-Z0-9_.:\-/@+ ]* (covers all known provider model ID formats while rejecting injection attempts) System-configured endpoints still get full model list validation as before. * 🧪 test: Add regression tests for filterAuthorizedTools configServers and validateModel filterAuthorizedTools: - Add test verifying configServers is passed to getAllServerConfigs and config-override server tools are allowed through - Guard resolveConfigServers in createAgentHandler to only run when MCP tools are present (skip for tool-free agent creates) validateModel (12 new tests): - Format validation: missing model, non-string, length overflow, leading special char, script injection, standard model ID acceptance - userProvide early-return: next() called immediately, getModelsConfig not invoked (regression guard for the exact bug this fixes) - System endpoint list validation: reject unknown model, accept known model, handle null/missing models config Also fix unnecessary backslash escape in MODEL_PATTERN regex. * 🧹 fix: Remove space from MODEL_PATTERN, trim input, clean up nits - Remove space character from MODEL_PATTERN regex — no real model ID uses spaces; prevents spurious violation logs from whitespace artifacts - Add model.trim() before validation to handle accidental whitespace - Remove redundant filterUniquePlugins call on already-deduplicated output - Add comment documenting intentional whitelist+blacklist composition - Add getUserKeyValues.mockReset() in loadConfigModels.spec beforeEach - Remove narrating JSDoc from getModelsConfig one-liner - Add 2 tests: trim whitespace handling, reject spaces in model ID * fix: Match startup tool loader semantics — includedTools takes precedence over filteredTools The startup tool loader (loadAndFormatTools) explicitly ignores filteredTools when includedTools is set, with a warning log. The PluginController was applying both independently, creating inconsistent behavior where the same config produced different results at startup vs plugin listing time. Restored mutually exclusive semantics: when includedTools is non-empty, filteredTools is not evaluated. * 🧹 chore: Simplify validateModel flow, note auth requirement on endpoints route - Separate missing-model from invalid-model checks cleanly: type+presence guard first, then trim+format guard (reviewer NIT) - Add route comment noting auth is required for role/tenant scoping * fix: Write trimmed model back to req.body.model for downstream consumers	2026-03-30 16:49:48 -04:00
Dustin Healy	a4a17ac771	⛩️ feat: Admin Grants API Endpoints (#12438 ) * feat: add System Grants handler factory with tests Handler factory with 4 endpoints: getEffectiveCapabilities (expanded capability set for authenticated user), getPrincipalGrants (list grants for a specific principal), assignGrant, and revokeGrant. Write ops dynamically check MANAGE_ROLES/GROUPS/USERS based on target principal type. 31 unit tests covering happy paths, validation, 403, and errors. * feat: wire System Grants REST routes Mount /api/admin/grants with requireJwtAuth + ACCESS_ADMIN gate. Add barrel export for createAdminGrantsHandlers and AdminGrantsDeps. * fix: cascade grant cleanup on role deletion Add deleteGrantsForPrincipal to AdminRolesDeps and call it in deleteRoleHandler via Promise.allSettled after successful deletion, matching the groups cleanup pattern. 3 tests added for cleanup call, skip on 404, and resilience to cleanup failure. * fix: simplify cascade grant cleanup on role deletion Replace Promise.allSettled wrapper with a direct try/catch for the single deleteGrantsForPrincipal call. * fix: harden grant handlers with auth, validation, types, and RESTful revoke - Add per-handler auth checks (401) and granular capability gates (READ_* for getPrincipalGrants, possession check for assignGrant) - Extract validatePrincipal helper; rewrite validateGrantBody to use direct type checks instead of unsafe `as string` casts - Align DI types with data layer (ResolvedPrincipal.principalType widened to string, getUserPrincipals role made optional) - Switch revoke route from DELETE body to RESTful URL params - Return 201 for assignGrant to match roles/groups create convention - Handle null grantCapability return with 500 - Add comprehensive test coverage for new auth/validation paths * fix: deduplicate ResolvedPrincipal, typed body, defensive auth checks - Remove duplicate ResolvedPrincipal from capabilities.ts; import the canonical export from grants.ts - Replace Record<string, unknown> with explicit GrantRequestBody interface - Add defensive 403 when READ_CAPABILITY_BY_TYPE lookup misses - Document revoke asymmetry (no possession check) with JSDoc - Use _id only in resolveUser (avoid Mongoose virtual reliance) - Improve null-grant error message - Complete logger mock in tests * refactor: move ResolvedPrincipal to shared types to fix circular dep Extract ResolvedPrincipal from admin/grants.ts to types/principal.ts so middleware/capabilities.ts imports from shared types rather than depending upward on the admin handler layer. * chore: remove dead re-export, align logger mocks across admin tests - Remove unused ResolvedPrincipal re-export from grants.ts (canonical source is types/principal.ts) - Align logger mocks in roles.spec.ts and groups.spec.ts to include all log levels (error, warn, info, debug) matching grants.spec.ts * fix: cascade Config and AclEntry cleanup on role deletion Add deleteConfig and deleteAclEntries to role deletion cascade, matching the group deletion pattern. Previously only grants were cleaned up, leaving orphaned config overrides and ACL entries. * perf: single-query batch for getEffectiveCapabilities Add getCapabilitiesForPrincipals (plural) to the data layer — a single $or query across all principals instead of N+1 parallel queries. Wire it into the grants handler so getEffectiveCapabilities hits the DB once regardless of how many principals the user has. * fix: defer SystemCapabilities access to factory call time Move all SystemCapabilities usage (VALID_CAPABILITIES, MANAGE_CAPABILITY_BY_TYPE, READ_CAPABILITY_BY_TYPE) inside the createAdminGrantsHandlers factory. External test suites that mock @librechat/data-schemas without providing SystemCapabilities crashed at import time when grants.ts was loaded transitively. * test: add data-layer and handler test coverage for review findings - Add 6 mongodb-memory-server tests for getCapabilitiesForPrincipals: multi-principal batch, empty array, filtering, tenant scoping - Add handler test: all principals filtered (only PUBLIC) - Add handler test: granting an implied capability succeeds - Add handler test: all cascade cleanup operations fail simultaneously - Document platform-scope-only tenantId behavior in JSDoc * fix: resolveUser fallback to user.id, early-return empty principals - Match capabilities middleware pattern: _id?.toString() ?? user.id to handle JWT-deserialized users without Mongoose _id - Move empty-array guard before principals.map() to skip unnecessary normalizePrincipalId calls - Add comment explaining VALID_PRINCIPAL_TYPES module-scope asymmetry * refactor: derive VALID_PRINCIPAL_TYPES from capability maps Make MANAGE_CAPABILITY_BY_TYPE and READ_CAPABILITY_BY_TYPE non-Partial Records over a shared GrantPrincipalType union, then derive VALID_PRINCIPAL_TYPES from the map keys. This makes divergence between the three data structures structurally impossible. * feat: add GET /api/admin/grants list-all-grants endpoint Add listAllGrants data-layer method and handler so the admin panel can fetch all grants in a single request instead of fanning out N+M calls per role and group. Response is filtered to only include grants for principal types the caller has read access to. * fix: update principalType to use GrantPrincipalType for consistency in grants handling - Refactor principalType in createAdminGrantsHandlers to use GrantPrincipalType instead of PrincipalType for better type accuracy. - Ensure type consistency across the grants handling logic in the API. * fix: address admin grants review findings — tenantId propagation, capability validation, pagination, and test coverage Propagate tenantId through all grant operations for multi-tenancy support. Extract isValidCapability to accept full SystemCapability union (base, section, assign) and reuse it in both Mongoose schema validation and handler input checks. Replace listAllGrants with paginated listGrants + countGrants. Filter PUBLIC principals from getCapabilitiesForPrincipals queries. Export getCachedPrincipals from ALS store for fast-path principal resolution. Move DELETE capability param to query string to avoid colon-in-URL issues. Remove dead code and add comprehensive handler and data-layer test coverage. * refactor: harden admin grants — FilterQuery types, auth-first ordering, DELETE path param, isValidCapability tests Replace Record<string, unknown> with FilterQuery<ISystemGrant> across all data-layer query filters. Refactor buildTenantFilter to a pure tenantCondition function that returns a composable FilterQuery fragment, eliminating the $or collision between tenant and principal queries. Move auth check before input validation in getPrincipalGrantsHandler, assignGrantHandler, and revokeGrantHandler to avoid leaking valid type names to unauthenticated callers. Switch DELETE route from query param back to path param (/:capability) with encodeURIComponent per project conventions. Add compound index for listGrants sort. Type VALID_PRINCIPAL_TYPES as Set<GrantPrincipalType>. Remove unused GetCachedPrincipalsFn type export. Add dedicated isValidCapability unit tests and revokeGrant idempotency test. * refactor: batch capability checks in listGrantsHandler via getHeldCapabilities Replace 3 parallel hasCapabilityForPrincipals DB calls with a single getHeldCapabilities query that returns the subset of capabilities any principal holds. Also: defensive limit(0) clamp, parallelized assignGrant auth checks, principalId type-vs-required error split, tenantCondition hoisted to factory top, JSDoc on cascade deps, DELETE route encoding note. * fix: normalize principalId and filter undefined in getHeldCapabilities Add normalizePrincipalId + null guard to getHeldCapabilities, matching the contract of getCapabilitiesForPrincipals. Simplify allCaps build with flatMap, add no-tenantId cross-check and undefined-principalId test cases. * refactor: use concrete types in GrantRequestBody, rename encoding test Replace unknown fields with explicit string types in GrantRequestBody, matching the established pattern in roles/groups/config handlers. Rename misleading 'encoded' test to 'with colons' since Express auto-decodes req.params. * fix: support hierarchical parent capabilities in possession checks hasCapabilityForPrincipals and getHeldCapabilities now resolve parent base capabilities for section/assignment grants. An admin holding manage:configs can now grant manage:configs:<section> and transitively read:configs:<section>. Fixes anti-escalation 403 blocking config capability delegation. * perf: use getHeldCapabilities in assignGrant to halve DB round-trips assignGrantHandler was making two parallel hasCapabilityForPrincipals calls to check manage + capability possession. getHeldCapabilities was introduced in this PR specifically for this pattern. Replace with a single batched call. Update corresponding spec assertions. * fix: validate role existence before granting capabilities Grants for non-existent role names were silently persisted, creating orphaned grants that could surprise-activate if a role with that name was later created. Add optional checkRoleExists dep to assignGrant and wire it to getRoleByName in the route file. * refactor: tighten principalType typing and use grantCapability in tests Narrow getCapabilitiesForPrincipals parameter from string to PrincipalType, removing the redundant cast. Replace direct SystemGrant.create() calls in getCapabilitiesForPrincipals tests with methods.grantCapability() to honor the schema's normalization invariant. Add getHeldCapabilities extended capability tests. * test: rename misleading cascade cleanup test name The test only injects failure into deleteGrantsForPrincipal, not all cascade operations. Rename from 'cascade cleanup fails' to 'grant cleanup fails' to match the actual scope. * fix: reorder role check after permission guard, add tenantId to index Move checkRoleExists after the getHeldCapabilities permission check so that a sub-MANAGE_ROLES admin cannot probe role name existence via 400 vs 403 response codes. Add tenantId to the { principalType, capability } index so listGrants queries in multi-tenant deployments can use a covering index instead of post-scanning for tenant condition. Add missing test for checkRoleExists throwing. * fix: scope deleteGrantsForPrincipal to tenant on role deletion deleteGrantsForPrincipal previously filtered only on principalType + principalId, deleting grants across all tenants. Since the role schema supports multi-tenancy (compound unique index on name + tenantId), two tenants can share a role name like 'editor'. Deleting that role in one tenant would wipe grants for identically-named roles in other tenants. Add optional tenantId parameter to deleteGrantsForPrincipal. When provided, scopes the delete to that tenant plus platform-level grants. Propagate req.user.tenantId through the role deletion cascade. * fix: scope grant cleanup to tenant on group deletion Same cross-tenant gap as the role deletion path: deleteGroupHandler called deleteGrantsForPrincipal without tenantId, so deleting a group would wipe its grants across all tenants. Extract req.user.tenantId and pass it through. * test: add HTTP integration test for admin grants routes Supertest-based test with real MongoMemoryServer exercising the full Express wiring: route registration, injected auth middleware, handler DI deps, and real DB round-trips. Covers GET /, GET /effective, POST / + DELETE / lifecycle, role existence validation, and 401 for unauthenticated callers. Also documents the expandImplications scope: the /effective endpoint returns base-level capabilities only; section-level resolution is handled at authorization check time by getParentCapabilities. * fix: use exact tenant match in deleteGrantsForPrincipal, normalize principalId, harden API CRITICAL: deleteGrantsForPrincipal was using tenantCondition (a read-query helper) for deleteMany, which includes the { tenantId: { $exists: false } } arm. This silently destroyed platform-level grants when a tenant-scoped role/group deletion occurred. Replace with exact { tenantId } match for deletes so platform-level grants survive tenant-scoped cascade cleanup. Refactor deleteGrantsForPrincipal signature from fragile positional overload (sessionOrTenantId union + maybeSession) to a clean options object: { tenantId?, session? }. Update all callers and test assertions. Add normalizePrincipalId to hasCapabilityForPrincipals to match the pattern already used by getHeldCapabilities — prevents string/ObjectId type mismatch on USER/GROUP principal queries. Also: export GrantPrincipalType from barrel, add upper-bound cap to listGrants, document GROUP/USER existence check trade-off, add integration tests for tenant-isolation property of deleteGrantsForPrincipal. * fix: forward tenantId to getUserPrincipals in resolvePrincipals resolvePrincipals had tenantId available from the caller but only forwarded it to getCachedPrincipals (cache lookup). The DB fallback via getUserPrincipals omitted it. While the Group schema's applyTenantIsolation Mongoose plugin handles scoping via AsyncLocalStorage in HTTP request context, explicitly passing tenantId makes the contract visible and prevents silent cross-tenant group resolution if called outside request context. * fix: remove unused import and add assertion to 401 integration test Remove unused SystemCapabilities import flagged by ESLint. Add explicit body assertion to the 401 test so it has a jest expect() call. * chore: hoist grant limit constants to scope, remove dead isolateModules Move GRANTS_DEFAULT_LIMIT / GRANTS_MAX_LIMIT from inside listGrants function body to createSystemGrantMethods scope so they are evaluated once at module load. Remove dead jest.isolateModules + jest.doMock block in integration test — the ~/models mock was never exercised since handlers are built with explicit DI deps. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-03-30 16:49:23 -04:00
Danny Avila	877c2efc85	🏗️ feat: bulkWrite isolation, pre-auth context, strict-mode fixes (#12445 ) * fix: wrap seedDatabase() in runAsSystem() for strict tenant mode seedDatabase() was called without tenant context at startup, causing every Mongoose operation inside it to throw when TENANT_ISOLATION_STRICT=true. Wrapping in runAsSystem() gives it the SYSTEM_TENANT_ID sentinel so the isolation plugin skips filtering, matching the pattern already used for performStartupChecks and updateInterfacePermissions. * fix: chain tenantContextMiddleware in optionalJwtAuth optionalJwtAuth populated req.user but never established ALS tenant context, unlike requireJwtAuth which chains tenantContextMiddleware after successful auth. Authenticated users hitting routes with optionalJwtAuth (e.g. /api/banner) had no tenant isolation. * feat: tenant-safe bulkWrite wrapper and call-site migration Mongoose's bulkWrite() does not trigger schema-level middleware hooks, so the applyTenantIsolation plugin cannot intercept it. This adds a tenantSafeBulkWrite() utility that injects the current ALS tenant context into every operation's filter/document before delegating to native bulkWrite. Migrates all 8 runtime bulkWrite call sites: - agentCategory (seedCategories, ensureDefaultCategories) - conversation (bulkSaveConvos) - message (bulkSaveMessages) - file (batchUpdateFiles) - conversationTag (updateTagsForConversation, bulkIncrementTagCounts) - aclEntry (bulkWriteAclEntries) systemGrant.seedSystemGrants is intentionally not migrated — it uses explicit tenantId: { $exists: false } filters and is exempt from the isolation plugin. * feat: pre-auth tenant middleware and tenant-scoped config cache Adds preAuthTenantMiddleware that reads X-Tenant-Id from the request header and wraps downstream in tenantStorage ALS context. Wired onto /oauth, /api/auth, /api/config, and /api/share — unauthenticated routes that need tenant scoping before JWT auth runs. The /api/config cache key is now tenant-scoped (STARTUP_CONFIG:${tenantId}) so multi-tenant deployments serve the correct login page config per tenant. The middleware is intentionally minimal — no subdomain parsing, no OIDC claim extraction. The private fork's reverse proxy or auth gateway sets the header. * feat: accept optional tenantId in updateInterfacePermissions When tenantId is provided, the function re-enters inside tenantStorage.run({ tenantId }) so all downstream Mongoose queries target that tenant's roles instead of the system context. This lets the private fork's tenant provisioning flow call updateInterfacePermissions per-tenant after creating tenant-scoped ADMIN/USER roles. * fix: tenant-filter $lookup in getPromptGroup aggregation The $lookup stage in getPromptGroup() queried the prompts collection without tenant filtering. While the outer PromptGroup aggregate is protected by the tenantIsolation plugin's pre('aggregate') hook, $lookup runs as an internal MongoDB operation that bypasses Mongoose hooks entirely. Converts from simple field-based $lookup to pipeline-based $lookup with an explicit tenantId match when tenant context is active. * fix: replace field-level unique indexes with tenant-scoped compounds Field-level unique:true creates a globally-unique single-field index in MongoDB, which would cause insert failures across tenants sharing the same ID values. - agent.id: removed field-level unique, added { id, tenantId } compound - convo.conversationId: removed field-level unique (compound at line 50 already exists: { conversationId, user, tenantId }) - message.messageId: removed field-level unique (compound at line 165 already exists: { messageId, user, tenantId }) - preset.presetId: removed field-level unique, added { presetId, tenantId } compound * fix: scope MODELS_CONFIG, ENDPOINT_CONFIG, PLUGINS, TOOLS caches by tenant These caches store per-tenant configuration (available models, endpoint settings, plugin availability, tool definitions) but were using global cache keys. In multi-tenant mode, one tenant's cached config would be served to all tenants. Appends :${tenantId} to cache keys when tenant context is active. Falls back to the unscoped key when no tenant context exists (backward compatible for single-tenant OSS deployments). Covers all read, write, and delete sites: - ModelController.js: get/set MODELS_CONFIG - PluginController.js: get/set PLUGINS, get/set TOOLS - getEndpointsConfig.js: get/set/delete ENDPOINT_CONFIG - app.js: delete ENDPOINT_CONFIG (clearEndpointConfigCache) - mcp.js: delete TOOLS (updateMCPTools, mergeAppTools) - importers.js: get ENDPOINT_CONFIG * fix: add getTenantId to PluginController spec mock The data-schemas mock was missing getTenantId, causing all PluginController tests to throw when the controller calls getTenantId() for tenant-scoped cache keys. * fix: address review findings — migration, strict-mode, DRY, types Addresses all CRITICAL, MAJOR, and MINOR review findings: F1 (CRITICAL): Add agents, conversations, messages, presets to SUPERSEDED_INDEXES in tenantIndexes.ts so dropSupersededTenantIndexes() drops the old single-field unique indexes that block multi-tenant inserts. F2 (CRITICAL): Unknown bulkWrite op types now throw in strict mode instead of silently passing through without tenant injection. F3 (MAJOR): Replace wildcard export with named export for tenantSafeBulkWrite, hiding _resetBulkWriteStrictCache from the public package API. F5 (MAJOR): Restore AnyBulkWriteOperation<IAclEntry>[] typing on bulkWriteAclEntries — the unparameterized wrapper accepts parameterized ops as a subtype. F7 (MAJOR): Fix config.js tenant precedence — JWT-derived req.user.tenantId now takes priority over the X-Tenant-Id header for authenticated requests. F8 (MINOR): Extract scopedCacheKey() helper into tenantContext.ts and replace all 11 inline occurrences across 7 files. F9 (MINOR): Use simple localField/foreignField $lookup for the non-tenant getPromptGroup path (more efficient index seeks). F12 (NIT): Remove redundant BulkOp type alias. F13 (NIT): Remove debug log that leaked raw tenantId. * fix: add new superseded indexes to tenantIndexes test fixture The test creates old indexes to verify the migration drops them. Missing fixture entries for agents.id_1, conversations.conversationId_1, messages.messageId_1, and presets.presetId_1 caused the count assertion to fail (expected 22, got 18). * fix: restore logger.warn for unknown bulk op types in non-strict mode * fix: block SYSTEM_TENANT_ID sentinel from external header input CRITICAL: preAuthTenantMiddleware accepted any string as X-Tenant-Id, including '__SYSTEM__'. The tenantIsolation plugin treats SYSTEM_TENANT_ID as an explicit bypass — skipping ALL query filters. A client sending X-Tenant-Id: __SYSTEM__ to pre-auth routes (/api/share, /api/config, /api/auth, /oauth) would execute Mongoose operations without tenant isolation. Fixes: - preAuthTenantMiddleware rejects SYSTEM_TENANT_ID in header - scopedCacheKey returns the base key (not key:__SYSTEM__) in system context, preventing stale cache entries during runAsSystem() - updateInterfacePermissions guards tenantId against SYSTEM_TENANT_ID - $lookup pipeline separates $expr join from constant tenantId match for better index utilization - Regression test for sentinel rejection in preAuthTenant.spec.ts - Remove redundant getTenantId() call in config.js * test: add missing deleteMany/replaceOne coverage, fix vacuous ALS assertions bulkWrite spec: - deleteMany: verifies tenant-scoped deletion leaves other tenants untouched - replaceOne: verifies tenantId injected into both filter and replacement - replaceOne overwrite: verifies a conflicting tenantId in the replacement document is overwritten by the ALS tenant (defense-in-depth) - empty ops array: verifies graceful handling preAuthTenant spec: - All negative-case tests now use the capturedNext pattern to verify getTenantId() inside the middleware's execution context, not the test runner's outer frame (which was always undefined regardless) * feat: tenant-isolate MESSAGES cache, FLOWS cache, and GenerationJobManager MESSAGES cache (streamAudio.js): - Cache key now uses scopedCacheKey(messageId) to prefix with tenantId, preventing cross-tenant message content reads during TTS streaming. FLOWS cache (FlowStateManager): - getFlowKey() now generates ${type}:${tenantId}:${flowId} when tenant context is active, isolating OAuth flow state per tenant. GenerationJobManager: - tenantId added to SerializableJobData and GenerationJobMetadata - createJob() captures the current ALS tenant context (excluding SYSTEM_TENANT_ID) and stores it in job metadata - SSE subscription endpoint validates job.metadata.tenantId matches req.user.tenantId, blocking cross-tenant stream access - Both InMemoryJobStore and RedisJobStore updated to accept tenantId * fix: add getTenantId and SYSTEM_TENANT_ID to MCP OAuth test mocks FlowStateManager.getFlowKey() now calls getTenantId() for tenant-scoped flow keys. The 4 MCP OAuth test files mock @librechat/data-schemas without these exports, causing TypeError at runtime. * fix: correct import ordering per AGENTS.md conventions Package imports sorted shortest to longest line length, local imports sorted longest to shortest — fixes ordering violations introduced by our new imports across 8 files. * fix: deserialize tenantId in RedisJobStore — cross-tenant SSE guard was no-op in Redis mode serializeJob() writes tenantId to the Redis hash via Object.entries, but deserializeJob() manually enumerates fields and omitted tenantId. Every getJob() from Redis returned tenantId: undefined, causing the SSE route's cross-tenant guard to short-circuit (undefined && ... → false). * test: SSE tenant guard, FlowStateManager key consistency, ALS scope docs SSE stream tenant tests (streamTenant.spec.js): - Cross-tenant user accessing another tenant's stream → 403 - Same-tenant user accessing own stream → allowed - OSS mode (no tenantId on job) → tenant check skipped FlowStateManager tenant tests (manager.tenant.spec.ts): - completeFlow finds flow created under same tenant context - completeFlow does NOT find flow under different tenant context - Unscoped flows are separate from tenant-scoped flows Documentation: - JSDoc on getFlowKey documenting ALS context consistency requirement - Comment on streamAudio.js scopedCacheKey capture site * fix: SSE stream tests hang on success path, remove internal fork references The success-path tests entered the SSE streaming code which never closes, causing timeout. Mock subscribe() to end the response immediately. Restructured assertions to verify non-403/non-404. Removed "private fork" and "OSS" references from code and test descriptions — replaced with "deployment layer", "multi-tenant deployments", and "single-tenant mode". * fix: address review findings — test rigor, tenant ID validation, docs F1: SSE stream tests now mock subscribe() with correct signature (streamId, writeEvent, onDone, onError) and assert 200 status, verifying the tenant guard actually allows through same-tenant users. F2: completeFlow logs the attempted key and ALS tenantId when flow is not found, so reverse proxy misconfiguration (missing X-Tenant-Id on OAuth callback) produces an actionable warning. F3/F10: preAuthTenantMiddleware validates tenant ID format — rejects colons, special characters, and values exceeding 128 chars. Trims whitespace. Prevents cache key collisions via crafted headers. F4: Documented cache invalidation scope limitation in clearEndpointConfigCache — only the calling tenant's key is cleared; other tenants expire via TTL. F7: getFlowKey JSDoc now lists all 8 methods requiring consistent ALS context. F8: Added dedicated scopedCacheKey unit tests — base key without context, base key in system context, scoped key with tenant, no ALS leakage across scope boundaries. * fix: revert flow key tenant scoping, fix SSE test timing FlowStateManager: Reverts tenant-scoped flow keys. OAuth callbacks arrive without tenant ALS context (provider redirects don't carry X-Tenant-Id), so completeFlow/failFlow would never find flows created under tenant context. Flow IDs are random UUIDs with no collision risk, and flow data is ephemeral (TTL-bounded). SSE tests: Use process.nextTick for onDone callback so Express response headers are flushed before res.write/res.end are called. * fix: restore getTenantId import for completeFlow diagnostic log * fix: correct completeFlow warning message, add missing flow test The warning referenced X-Tenant-Id header consistency which was only relevant when flow keys were tenant-scoped (since reverted). Updated to list actual causes: TTL expiry, missing flow, or routing to a different instance without shared Keyv storage. Removed the getTenantId() call and import — no longer needed since flow keys are unscoped. Added test for the !flowState branch in completeFlow — verifies return false and logger.warn on nonexistent flow ID. * fix: add explicit return type to recursive updateInterfacePermissions The recursive call (tenantId branch calls itself without tenantId) causes TypeScript to infer circular return type 'any'. Adding explicit Promise<void> satisfies the rollup typescript plugin. * fix: update MCPOAuthRaceCondition test to match new completeFlow warning * fix: clearEndpointConfigCache deletes both scoped and unscoped keys Unauthenticated /api/endpoints requests populate the unscoped ENDPOINT_CONFIG key. Admin config mutations clear only the tenant-scoped key, leaving the unscoped entry stale indefinitely. Now deletes both when in tenant context. * fix: tenant guard on abort/status endpoints, warn logs, test coverage F1: Add tenant guard to /chat/status/:conversationId and /chat/abort matching the existing guard on /chat/stream/:streamId. The status endpoint exposes aggregatedContent (AI response text) which requires tenant-level access control. F2: preAuthTenantMiddleware now logs warn for rejected __SYSTEM__ sentinel and malformed tenant IDs, providing observability for bypass probing attempts. F3: Abort fallback path (getActiveJobIdsForUser) now has tenant check after resolving the job. F4: Test for strict mode + SYSTEM_TENANT_ID — verifies runAsSystem bypasses tenantSafeBulkWrite without throwing in strict mode. F5: Test for job with tenantId + user without tenantId → 403. F10: Regex uses idiomatic hyphen-at-start form. F11: Test descriptions changed from "rejects" to "ignores" since middleware calls next() (not 4xx). Also fixes MCPOAuthRaceCondition test assertion to match updated completeFlow warning message. * fix: test coverage for logger.warn, status/abort guards, consistency A: preAuthTenant spec now mocks logger and asserts warn calls for __SYSTEM__ sentinel, malformed characters, and oversized headers. B: streamTenant spec expanded with status and abort endpoint tests — cross-tenant status returns 403, same-tenant returns 200 with body, cross-tenant abort returns 403. C: Abort endpoint uses req.user.tenantId (not req.user?.tenantId) matching stream/status pattern — requireJwtAuth guarantees req.user. D: Malformed header warning now includes ip in log metadata, matching the sentinel warning for consistent SOC correlation. * fix: assert ip field in malformed header warn tests * fix: parallelize cache deletes, document tenant guard, fix import order - clearEndpointConfigCache uses Promise.all for independent cache deletes instead of sequential awaits - SSE stream tenant guard has inline comment explaining backward-compat behavior for untenanted legacy jobs - conversation.ts local imports reordered longest-to-shortest per AGENTS.md * fix: tenant-qualify userJobs keys, document tenant guard backward-compat Job store userJobs keys now include tenantId when available: - Redis: stream:user:{tenantId:userId}:jobs (falls back to stream:user:{userId}:jobs when no tenant) - InMemory: composite key tenantId:userId in userJobMap getActiveJobIdsByUser/getActiveJobIdsForUser accept optional tenantId parameter, threaded through from req.user.tenantId at all call sites (/chat/active and /chat/abort fallback). Added inline comments on all three SSE tenant guards explaining the backward-compat design: untenanted legacy jobs remain accessible when the userId check passes. * fix: parallelize cache deletes, document tenant guard, fix import order Fix InMemoryJobStore.getActiveJobIdsByUser empty-set cleanup to use the tenant-qualified userKey instead of bare userId — prevents orphaned empty Sets accumulating in userJobMap for multi-tenant users. Document cross-tenant staleness in clearEndpointConfigCache JSDoc — other tenants' scoped keys expire via TTL, not active invalidation. * fix: cleanup userJobMap leak, startup warning, DRY tenant guard, docs F1: InMemoryJobStore.cleanup() now removes entries from userJobMap before calling deleteJob, preventing orphaned empty Sets from accumulating with tenant-qualified composite keys. F2: Startup warning when TENANT_ISOLATION_STRICT is active — reminds operators to configure reverse proxy to control X-Tenant-Id header. F3: mergeAppTools JSDoc documents that tenant-scoped TOOLS keys are not actively invalidated (matching clearEndpointConfigCache pattern). F5: Abort handler getActiveJobIdsForUser call uses req.user.tenantId (not req.user?.tenantId) — consistent with stream/status handlers. F6: updateInterfacePermissions JSDoc clarifies SYSTEM_TENANT_ID behavior — falls through to caller's ALS context. F7: Extracted hasTenantMismatch() helper, replacing three identical inline tenant guard blocks across stream/status/abort endpoints. F9: scopedCacheKey JSDoc documents both passthrough cases (no context and SYSTEM_TENANT_ID context). * fix: clean userJobMap in evictOldest — same leak as cleanup()	2026-03-28 16:43:50 -04:00
Danny Avila	935288f841	🏗️ feat: 3-Tier MCP Server Architecture with Config-Source Lazy Init (#12435 ) * feat: add MCPServerSource type, tenantMcpPolicy schema, and source-based dbSourced wiring - Add `tenantMcpPolicy` to `mcpSettings` in YAML config schema with `enabled`, `maxServersPerTenant`, `allowedTransports`, and `allowedDomains` - Add `MCPServerSource` type ('yaml' \| 'config' \| 'user') and `source` field to `ParsedServerConfig` - Change `dbSourced` determination from `!!config.dbId` to `config.source === 'user'` across MCPManager, ConnectionsRepository, UserConnectionManager, and MCPServerInspector - Set `source: 'user'` on all DB-sourced servers in ServerConfigsDB * feat: three-layer MCPServersRegistry with config cache and lazy init - Add `configCacheRepo` as third repository layer between YAML cache and DB for admin-defined config-source MCP servers - Implement `ensureConfigServers()` that identifies config-override servers from resolved `getAppConfig()` mcpConfig, lazily inspects them, and caches parsed configs with `source: 'config'` - Add `lazyInitConfigServer()` with timeout, stub-on-failure, and concurrent-init deduplication via `pendingConfigInits` map - Extend `getAllServerConfigs()` with optional `configServers` param for three-way merge: YAML → Config → User - Add `getServerConfig()` lookup through config cache layer - Add `invalidateConfigCache()` for clearing config-source inspection results on admin config mutations - Tag `source: 'yaml'` on CACHE-stored servers and `source: 'user'` on DB-stored servers in `addServer()` and `addServerStub()` * feat: wire tenant context into MCP controllers, services, and cache invalidation - Resolve config-source servers via `getAppConfig({ role, tenantId })` in `getMCPTools()` and `getMCPServersList()` controllers - Pass `ensureConfigServers()` results through `getAllServerConfigs()` for three-way merge of YAML + Config + User servers - Add tenant/role context to `getMCPSetupData()` and connection status routes via `getTenantId()` from ALS - Add `clearMcpConfigCache()` to `invalidateConfigCaches()` so admin config mutations trigger re-inspection of config-source MCP servers * feat: enforce tenantMcpPolicy on admin config mcpServers mutations - Add `validateMcpServerPolicy()` helper that checks mcpServers against operator-defined `tenantMcpPolicy` (enabled, maxServersPerTenant, allowedTransports, allowedDomains) - Wire validation into `upsertConfigOverrides` and `patchConfigField` handlers — rejects with 403 when policy is violated - Infer transport type from config shape (command → stdio, url protocol → websocket/sse, type field → streamable-http) - Validate server domains against policy allowlist when configured * revert: remove tenantMcpPolicy schema and enforcement The existing admin config CRUD routes already provide the mechanism for granular MCP server prepopulation (groups, roles, users). The tenantMcpPolicy gating adds unnecessary complexity that can be revisited if needed in the future. - Remove tenantMcpPolicy from mcpSettings Zod schema - Remove validateMcpServerPolicy helper and TenantMcpPolicy interface - Remove policy enforcement from upsertConfigOverrides and patchConfigField handlers * test: update test assertions for source field and config-server wiring - Use objectContaining in MCPServersRegistry reset test to account for new source: 'yaml' field on CACHE-stored configs - Add getTenantId and ensureConfigServers mocks to MCP route tests - Add getAppConfig mock to route test Config service mock - Update getMCPSetupData assertion to expect second options argument - Update getAllServerConfigs assertions for new configServers parameter * fix: disconnect active connections when config-source servers are evicted When admin config overrides change and config-source MCP servers are removed, the invalidation now proactively disconnects active connections for evicted servers instead of leaving them lingering until timeout. - Return evicted server names from invalidateConfigCache() - Disconnect app-level connections for evicted servers in clearMcpConfigCache() via MCPManager.appConnections.disconnect() * fix: address code review findings (CRITICAL, MAJOR, MINOR) CRITICAL fixes: - Scope configCacheRepo keys by config content hash to prevent cross-tenant cache poisoning when two tenants define the same server name with different configurations - Change dbSourced checks from `source === 'user'` to `source !== 'yaml' && source !== 'config'` so undefined source (pre-upgrade cached configs) fails closed to restricted mode MAJOR fixes: - Derive OAuth servers from already-computed mcpConfig instead of calling getOAuthServers() separately — config-source OAuth servers are now properly detected - Add parseInt radix (10) and NaN guard with fallback to 30_000 for CONFIG_SERVER_INIT_TIMEOUT_MS - Add CONFIG_CACHE_NAMESPACE to aggregate-key branch in ServerConfigsCacheFactory to avoid SCAN-based Redis stalls - Remove `if (role \|\| tenantId)` guard in getMCPSetupData — config servers now always resolve regardless of tenant context MINOR fixes: - Extract resolveAllMcpConfigs() helper in mcp controller to eliminate 3x copy-pasted config resolution boilerplate - Distinguish "not initialized" from real errors in clearMcpConfigCache — log actual failures instead of swallowing - Remove narrative inline comments per style guide - Remove dead try/catch inside Promise.allSettled in ensureConfigServers (inner method never throws) - Memoize YAML server names to avoid repeated cacheConfigsRepo.getAll() calls per request Test updates: - Add ensureConfigServers mock to registry test fixtures - Update getMCPSetupData assertions for inline OAuth derivation * fix: address code review findings (CRITICAL, MAJOR, MINOR) CRITICAL fixes: - Break circular dependency: move CONFIG_CACHE_NAMESPACE from MCPServersRegistry to ServerConfigsCacheFactory - Fix dbSourced fail-closed: use source field when present, fall back to legacy dbId check when absent (backward-compatible with pre-upgrade cached configs that lack source field) MAJOR fixes: - Add CONFIG_CACHE_NAMESPACE to aggregate-key set in ServerConfigsCacheFactory to avoid SCAN-based Redis stalls - Add comprehensive test suite (ensureConfigServers.test.ts, 18 tests) covering lazy init, stub-on-failure, cross-tenant isolation via config hash keys, concurrent deduplication, merge order, and cache invalidation MINOR fixes: - Update MCPServerInspector test assertion for dbSourced change * fix: restore getServerConfig lookup for config-source servers (NEW-1) Add configNameToKey map that indexes server name → hash-based cache key for O(1) lookup by name in getServerConfig. This restores the config cache layer that was dropped when hash-based keys were introduced. Without this fix, config-source servers appeared in tool listings (via getAllServerConfigs) but getServerConfig returned undefined, breaking all connection and tool call paths. - Populate configNameToKey in ensureSingleConfigServer - Clear configNameToKey in invalidateConfigCache and reset - Clear stale read-through cache entries after lazy init - Remove dead code in invalidateConfigCache (config.title, key parsing) - Add getServerConfig tests for config-source server lookup * fix: eliminate configNameToKey race via caller-provided configServers param Replace the process-global configNameToKey map (last-writer-wins under concurrent multi-tenant load) with a configServers parameter on getServerConfig. Callers pass the pre-resolved config servers map directly — no shared mutable state, no cross-tenant race. - Add optional configServers param to getServerConfig; when provided, returns matching config directly without any global lookup - Remove configNameToKey map entirely (was the source of the race) - Extract server names from cache keys via lastIndexOf in invalidateConfigCache (safe for names containing colons) - Use mcpConfig[serverName] directly in getMCPTools instead of a redundant getServerConfig call - Add cross-tenant isolation test for getServerConfig * fix: populate read-through cache after config server lazy init After lazyInitConfigServer succeeds, write the parsed config to readThroughCache keyed by serverName so that getServerConfig calls from ConnectionsRepository, UserConnectionManager, and MCPManager.callTool find the config without needing configServers. Without this, config-source servers appeared in tool listings but every connection attempt and tool call returned undefined. * fix: user-scoped getServerConfig fallback to server-only cache key When getServerConfig is called with a userId (e.g., from callTool or UserConnectionManager), the cache key is serverName::userId. Config-source servers are cached under the server-only key (no userId). Add a fallback so user-scoped lookups find config-source servers in the read-through cache. * fix: configCacheRepo fallback, isUserSourced DRY, cross-process race CRITICAL: Add findInConfigCache fallback in getServerConfig so config-source servers remain reachable after readThroughCache TTL expires (5s). Without this, every tool call after 5s returned undefined for config-source servers. MAJOR: Extract isUserSourced() helper to mcp/utils.ts and replace all 5 inline dbSourced ternary expressions (MCPManager x2, ConnectionsRepository, UserConnectionManager, MCPServerInspector). MAJOR: Fix cross-process Redis race in lazyInitConfigServer — when configCacheRepo.add throws (key exists from another process), fall back to reading the existing entry instead of returning undefined. MINOR: Parallelize invalidateConfigCache awaits with Promise.all. Remove redundant .catch(() => {}) inside Promise.allSettled. Tighten dedup test assertion to toBe(1). Add TTL-expiry tests for getServerConfig (with and without userId). * feat: thread configServers through getAppToolFunctions and formatInstructionsForContext Add optional configServers parameter to getAppToolFunctions, getInstructions, and formatInstructionsForContext so config-source server tools and instructions are visible to agent initialization and context injection paths. Existing callers (boot-time init, tests) pass no argument and continue to work unchanged. Agent runtime paths can now thread resolved config servers from request context. * fix: stale failure stubs retry after 5 min, upsert for cross-process races - Add CONFIG_STUB_RETRY_MS (5 min) — stale failure stubs are retried instead of permanently disabling config-source servers after transient errors (DNS outage, cold-start race) - Extract upsertConfigCache() helper that tries add then falls back to update, preventing cross-process Redis races where a second instance's successful inspection result was discarded - Add test for stale-stub retry after CONFIG_STUB_RETRY_MS * fix: stamp updatedAt on failure stubs, null-guard callTool config, test cleanup - Add updatedAt: Date.now() to failure stubs in lazyInitConfigServer so CONFIG_STUB_RETRY_MS (5 min) window works correctly — without it, stubs were always considered stale (updatedAt ?? 0 → epoch → always expired) - Add null guard for rawConfig in MCPManager.callTool before passing to preProcessGraphTokens — prevents unsafe `as` cast on undefined - Log double-failure in upsertConfigCache instead of silently swallowing - Replace module-scope Date.now monkey-patch with jest.useFakeTimers / jest.setSystemTime / jest.useRealTimers in ensureConfigServers tests * fix: server-only readThrough fallback only returns truthy values Prevents a cached undefined from a prior no-userId lookup from short-circuiting the DB query on a subsequent userId-scoped lookup. * fix: remove findInConfigCache to eliminate cross-tenant config leakage The findInConfigCache prefix scan (serverName:) could return any tenant's config after readThrough TTL expires, violating tenant isolation. Config-source servers are now ONLY resolvable through: 1. The configServers param (callers with tenant context from ALS) 2. The readThrough cache (populated by ensureSingleConfigServer, 5s TTL, repopulated on every HTTP request via resolveAllMcpConfigs) Connection/tool-call paths without tenant context rely exclusively on the readThrough cache. If it expires before the next HTTP request repopulates it, the server is not found — which is correct because there is no tenant context to determine which config to return. - Remove findInConfigCache method and its call in getServerConfig - Update server-only readThrough fallback to only return truthy values (prevents cached undefined from short-circuiting user-scoped DB lookup) - Update tests to document tenant isolation behavior after cache expiry style: fix import order per AGENTS.md conventions Sort package imports shortest-to-longest, local imports longest-to-shortest across MCPServersRegistry, ConnectionsRepository, MCPManager, UserConnectionManager, and MCPServerInspector. * fix: eliminate cross-tenant readThrough contamination and TTL-expiry tool failures Thread pre-resolved serverConfig from tool creation context into callTool, removing dependency on the readThrough cache for config-source servers. This fixes two issues: - Cross-tenant contamination: the readThrough cache key was unscoped (just serverName), so concurrent multi-tenant requests for same-named servers would overwrite each other's entries - TTL expiry: tool calls happening >5s after config resolution would fail with "Configuration not found" because the readThrough entry had expired Changes: - Add optional serverConfig param to MCPManager.callTool — uses provided config directly, falling back to getServerConfig lookup for YAML/user servers - Thread serverConfig from createMCPTool through createToolInstance closure to callTool - Remove readThrough write from ensureSingleConfigServer — config-source servers are only accessible via configServers param (tenant-scoped) - Remove server-only readThrough fallback from getServerConfig - Increase config cache hash from 8 to 16 hex chars (64-bit) - Add isUserSourced boundary tests for all source/dbId combinations - Fix double Object.keys call in getMCPTools controller - Update test assertions for new getServerConfig behavior * fix: cache base configs for config-server users; narrow upsertConfigCache error handling - Refactor getAllServerConfigs to separate base config fetch (YAML + DB) from config-server layering. Base configs are cached via readThroughCacheAll regardless of whether configServers is provided, eliminating uncached MongoDB queries per request for config-server users - Narrow upsertConfigCache catch to duplicate-key errors only; infrastructure errors (Redis timeouts, network failures) now propagate instead of being silently swallowed, preventing inspection storms during outages * fix: restore correct merge order and document upsert error matching - Restore YAML → Config → User DB precedence in getAllServerConfigs (user DB servers have highest precedence, matching the JSDoc contract) - Add source comment on upsertConfigCache duplicate-key detection linking to the two cache implementations that define the error message * feat: complete config-source server support across all execution paths Wire configServers through the entire agent execution pipeline so config-source MCP servers are fully functional — not just visible in listings but executable in agent sessions. - Thread configServers into handleTools.js agent tool pipeline: resolve config servers from tenant context before MCP tool iteration, pass to getServerConfig, createMCPTools, and createMCPTool - Thread configServers into agent instructions pipeline: applyContextToAgent → getMCPInstructionsForServers → formatInstructionsForContext, resolved in client.js before agent context application - Add configServers param to createMCPTool and createMCPTools for reconnect path fallback - Add source field to redactServerSecrets allowlist for client UI differentiation of server tiers - Narrow invalidateConfigCache to only clear readThroughCacheAll (merged results), preserving YAML individual-server readThrough entries - Update context.spec.ts assertions for new configServers parameter * fix: add missing mocks for config-source server dependencies in client.test.js Mock getMCPServersRegistry, getAppConfig, and getTenantId that were added to client.js but not reflected in the test file's jest.mock declarations. * fix: update formatInstructionsForContext assertions for configServers param The test assertions expected formatInstructionsForContext to be called with only the server names array, but it now receives configServers as a second argument after the config-source server feature wiring. * fix: move configServers resolution before MCP tool loop to avoid TDZ configServers was declared with `let` after the first tool loop but referenced inside it via getServerConfig(), causing a ReferenceError temporal dead zone. Move declaration and resolution before the loop, using tools.some(mcpToolPattern) to gate the async resolution. * fix: address review findings — cache bypass, discoverServerTools gap, DRY - #2: getAllServerConfigs now always uses getBaseServerConfigs (cached via readThroughCacheAll) instead of bypassing it when configServers is present. Extracts user-DB entries from cached base by diffing against YAML keys to maintain YAML → Config → User DB merge order without extra MongoDB calls. - #3: Add configServers param to ToolDiscoveryOptions and thread it through discoverServerTools → getServerConfig so config-source servers are discoverable during OAuth reconnection flows. - #6: Replace inline import() type annotations in context.ts with proper import type { ParsedServerConfig } per AGENTS.md conventions. - #7: Extract resolveConfigServers(req) helper in MCP.js and use it from handleTools.js and client.js, eliminating the duplicated 6-line config resolution pattern. - #10: Restore removed "why" comment explaining getLoaded() vs getAll() choice in getMCPSetupData — documents non-obvious correctness constraint. - #11: Fix incomplete JSDoc param type on resolveAllMcpConfigs. * fix: consolidate imports, reorder constants, fix YAML-DB merge edge case - Merge duplicate @librechat/data-schemas requires in MCP.js into one - Move resolveConfigServers after module-level constants - Fix getAllServerConfigs edge case where user-DB entry overriding a YAML entry with the same name was excluded from userDbConfigs; now uses reference equality check to detect DB-overwritten YAML keys * fix: replace fragile string-match error detection with proper upsert method Add upsert() to IServerConfigsRepositoryInterface and all implementations (InMemory, Redis, RedisAggregateKey, DB). This eliminates the brittle error message string match ('already exists in cache') in upsertConfigCache that was the only thing preventing cross-process init races from silently discarding inspection results. Each implementation handles add-or-update atomically: - InMemory: direct Map.set() - Redis: direct cache.set() - RedisAggregateKey: read-modify-write under write lock - DB: delegates to update() (DB servers use explicit add() with ACL setup) * fix: wire configServers through remaining HTTP endpoints - getMCPServerById: use resolveAllMcpConfigs instead of bare getServerConfig - reinitialize route: resolve configServers before getServerConfig - auth-values route: resolve configServers before getServerConfig - getOAuthHeaders: accept configServers param, thread from callers - Update mcp.spec.js tests to mock getAllServerConfigs for GET by name * fix: thread serverConfig through getConnection for config-source servers Config-source servers exist only in configCacheRepo, not in YAML cache or DB. When callTool → getConnection → getUserConnection → getServerConfig runs without configServers, it returns undefined and throws. Fix by threading the pre-resolved serverConfig (providedConfig) from callTool through getConnection → getUserConnection → createUserConnectionInternal, using it as a fallback before the registry lookup. * fix: thread configServers through reinit, reconnect, and tool definition paths Wire configServers through every remaining call chain that creates or reconnects MCP server connections: - reinitMCPServer: accepts serverConfig and configServers, uses them for getServerConfig fallback, getConnection, and discoverServerTools - reconnectServer: accepts and passes configServers to reinitMCPServer - createMCPTools/createMCPTool: pass configServers to reconnectServer - ToolService.loadToolDefinitionsWrapper: resolves configServers from req, passes to both reinitMCPServer call sites - reinitialize route: passes serverConfig and configServers to reinitMCPServer * fix: address review findings — simplify merge, harden error paths, fix log labels - Simplify getAllServerConfigs merge: replace fragile reference-equality loop with direct spread { ...yamlConfigs, ...configServers, ...base } - Guard upsertConfigCache in lazyInitConfigServer catch block so cache failures don't mask the original inspection error - Deduplicate getYamlServerNames cold-start with promise dedup pattern - Remove dead `if (!mcpConfig)` guard in getMCPSetupData - Fix hardcoded "App server" in ServerConfigsCacheRedisAggregateKey error messages — now uses this.namespace for correct Config/App labeling - Remove misleading OAuth callback comment about readThrough cache - Move resolveConfigServers after module-level constants in MCP.js * fix: clear rejected yamlServerNames promise, fix config-source reinspect, fix reset log label - Clear yamlServerNamesPromise on rejection so transient cache errors don't permanently prevent ensureConfigServers from working - Skip reinspectServer for config-source servers (source: 'config') in reinitMCPServer — they lack a CACHE/DB storage location; retry is handled by CONFIG_STUB_RETRY_MS in ensureConfigServers - Use source field instead of dbId for storageLocation derivation - Fix remaining hardcoded "App" in reset() leaderCheck message * fix: persist oauthHeaders in flow state for config-source OAuth servers The OAuth callback route has no JWT auth context and cannot resolve config-source server configs. Previously, getOAuthHeaders would silently return {} for config-source servers, dropping custom token exchange headers. Now oauthHeaders are persisted in MCPOAuthFlowMetadata during flow initiation (which has auth context), and the callback reads them from the stored flow state with a fallback to the registry lookup for YAML/user-DB servers. * fix: update tests for getMCPSetupData null guard removal and ToolService mock - MCP.spec.js: update test to expect graceful handling of null mcpConfig instead of a throw (getAllServerConfigs always returns an object) - MCP.js: add defensive \|\| {} for Object.entries(mcpConfig) in case of null from test mocks - ToolService.spec.js: add missing mock for ~/server/services/MCP (resolveConfigServers) * fix: address review findings — DRY, naming, logging, dead code, defensive guards - #1: Simplify getAllServerConfigs to single getBaseServerConfigs call, eliminating redundant double-fetch of cacheConfigsRepo.getAll() - #2: Add warning log when oauthHeaders absent from OAuth callback flow state - #3: Extract resolveAllMcpConfigs to MCP.js service layer; controller imports shared helper instead of reimplementing - #4: Rename _serverConfig/_provider to capturedServerConfig/capturedProvider in createToolInstance — these are actively used, not unused - #5: Log rejected results from ensureConfigServers Promise.allSettled so cache errors are visible instead of silently dropped - #6: Remove dead 'MCP config not found' error handlers from routes - #7: Document circular-dependency reason for dynamic require in clearMcpConfigCache - #8: Remove logger.error from withTimeout to prevent double-logging timeouts - #10: Add explicit userId guard in ServerConfigsDB.upsert with clear error message - #12: Use spread instead of mutation in addServer for immutability consistency - Add upsert mock to ensureConfigServers.test.ts DB mock - Update route tests for resolveAllMcpConfigs import change * fix: restore correct merge priority, use immutable spread, fix test mock - getAllServerConfigs: { ...configServers, ...base } so userDB wins over configServers, matching documented "User DB (highest)" priority - lazyInitConfigServer: use immutable spread instead of direct mutation for parsedConfig.source, consistent with addServer fix - Fix test to mock getAllServerConfigs as {} instead of null, remove unnecessary \|\| {} defensive guard in getMCPSetupData * fix: error handling, stable hashing, flatten nesting, remove dead param - Wrap resolveConfigServers/resolveAllMcpConfigs in try/catch with graceful {} fallback so transient DB/cache errors don't crash tool pipeline - Sort keys in configCacheKey JSON.stringify for deterministic hashing regardless of object property insertion order - Flatten clearMcpConfigCache from 3 nested try-catch to early returns; document that user connections are cleaned up lazily (accepted tradeoff) - Remove dead configServers param from getAppToolFunctions (never passed) - Add security rationale comment for source field in redactServerSecrets * fix: use recursive key-sorting replacer in configCacheKey to prevent cross-tenant cache collision The array replacer in JSON.stringify acts as a property allowlist at every nesting depth, silently dropping nested keys like headers['X-API-Key'], oauth.client_secret, etc. Two configs with different nested values but identical top-level structure produced the same hash, causing cross-tenant cache hits and potential credential contamination. Switch to a function replacer that recursively sorts keys at all depths without dropping any properties. Also document the known gap in getOAuthServers: config-source OAuth servers are not covered by auto-reconnection or uninstall cleanup because callers lack request context. * fix: move clearMcpConfigCache to packages/api to eliminate circular dependency The function only depends on MCPServersRegistry and MCPManager, both of which live in packages/api. Import it directly from @librechat/api in the CJS layer instead of using dynamic require('~/config'). * chore: imports/fields ordering * fix: address review findings — error handling, targeted lookup, test gaps - Narrow resolveAllMcpConfigs catch to only wrap ensureConfigServers so getAppConfig/getAllServerConfigs failures propagate instead of masking infrastructure errors as empty server lists. - Use targeted getServerConfig in getMCPServerById instead of fetching all server configs for a single-server lookup. - Forward configServers to inner createMCPTool calls so reconnect path works for config-source servers. - Update getAllServerConfigs JSDoc to document disjoint-key design. - Add OAuth callback oauthHeaders fallback tests (flow state present vs registry fallback). - Add resolveConfigServers/resolveAllMcpConfigs unit tests covering happy path and error propagation. * fix: add getOAuthReconnectionManager mock to OAuth callback tests * chore: imports ordering	2026-03-28 10:36:43 -04:00
Dustin Healy	5972a21479	🪪 feat: Admin Roles API Endpoints (#12400 ) * feat: add createRole and deleteRole methods to role * feat: add admin roles handler factory and Express routes * fix: address convention violations in admin roles handlers * fix: rename createRole/deleteRole to avoid AccessRole name collision The existing accessRole.ts already exports createRole/deleteRole for the AccessRole model. In createMethods index.ts, these are spread after roleMethods, overwriting them. Renamed our Role methods to createRoleByName/deleteRoleByName to match the existing pattern (getRoleByName, updateRoleByName) and avoid the collision. * feat: add description field to Role model - Add description to IRole, CreateRoleRequest, UpdateRoleRequest types - Add description field to Mongoose roleSchema (default: '') - Wire description through createRoleHandler and updateRoleHandler - Include description in listRoles select clause so it appears in list * fix: address Copilot review findings in admin roles handlers * test: add unit tests for admin roles and groups handlers * test: add data-layer tests for createRoleByName, deleteRoleByName, listUsersByRole * fix: allow system role updates when name is unchanged The updateRoleHandler guard rejected any request where body.name matched a system role, even when the name was not being changed. This blocked editing a system role's description. Compare against the URL param to only reject actual renames to reserved names. * fix: address external review findings for admin roles - Block renaming system roles (ADMIN/USER) and add user migration on rename - Add input validation: name max-length, trim on update, duplicate name check - Replace fragile String.includes error matching with prefix-based classification - Catch MongoDB 11000 duplicate key in createRoleByName - Add pagination (limit/offset/total) to getRoleMembersHandler - Reverse delete order in deleteRoleByName — reassign users before deletion - Add role existence check in removeRoleMember; drop unused createdAt select - Add Array.isArray guard for permissions input; use consistent ?? coalescing - Fix import ordering per AGENTS.md conventions - Type-cast mongoose.models.User as Model<IUser> for proper TS inference - Add comprehensive tests: rename guards, pagination, validation, 500 paths * fix: address re-review findings for admin roles - Gate deleteRoleByName on existence check — skip user reassignment and cache invalidation when role doesn't exist (fixes test mismatch) - Reverse rename order: migrate users before renaming role so a migration failure leaves the system in a consistent state - Add .sort({ _id: 1 }) to listUsersByRole for deterministic pagination - Import shared AdminMember type from data-schemas instead of local copy; make joinedAt optional since neither groups nor roles populate it - Change IRole.description from optional to required to match schema default - Add data-layer tests for updateUsersByRole and countUsersByRole - Add handler test verifying users-first rename ordering and migration failure safety * fix: add rollback on rename failure and update PR description - Roll back user migration if updateRoleByName returns null during a rename (race: role deleted between existence check and update) - Add test verifying rollback calls updateUsersByRole in reverse - Update PR #12400 description to reflect current test counts (56 handler tests, 40 data-layer tests) and safety features * fix: rollback on rename throw, description validation, delete/DRY cleanup - Hoist isRename/trimmedName above try block so catch can roll back user migration when updateRoleByName throws (not just returns null) - Add description type + max-length (2000) validation in create and update, consistent with groups handler - Remove redundant getRoleByName existence check in deleteRoleHandler — use deleteRoleByName return value directly - Skip no-op name write when body.name equals current name (use isRename) - Extract getUserModel() accessor to DRY repeated Model<IUser> casts - Use name.trim() consistently in createRoleByName error messages - Add tests: rename-throw rollback, description validation (create+update), update delete test mocks to match simplified handler * fix: guard spurious rollback, harden createRole error path, validate before DB calls - Add migrationRan flag to prevent rollback of user migration that never ran - Return generic message on 500 in createRoleHandler, specific only for 409 - Move description validation before DB queries in updateRoleHandler - Return existing role early when update body has no changes - Wrap cache.set in createRoleByName with try/catch to prevent masking DB success - Add JSDoc on 11000 catch explaining compound unique index - Add tests: spurious rollback guard, empty update body, description validation ordering, listUsersByRole pagination * fix: validate permissions in create, RoleConflictError, rollback safety, cache consistency - Add permissions type/array validation in createRoleHandler - Introduce RoleConflictError class replacing fragile string-prefix matching - Wrap rollback in !role null path with try/catch for correct 404 response - Wrap deleteRoleByName cache.set in try/catch matching createRoleByName - Narrow updateRoleHandler body type to { name?, description? } - Add tests: non-string description in create, rollback failure logging, permissions array rejection, description max-length assertion fix * feat: prevent removing the last admin user Add guard in removeRoleMember that checks countUsersByRole before demoting an ADMIN user, returning 400 if they are the last one. * fix: move interleaved export below imports, add await to countUsersByRole * fix: paginate listRoles, null-guard permissions handler, fix export ordering - Add limit/offset/total pagination to listRoles matching the groups pattern - Add countRoles data-layer method - Omit permissions from listRoles select (getRole returns full document) - Null-guard re-fetched role in updateRolePermissionsHandler - Move interleaved export below all imports in methods/index.ts * fix: address review findings — race safety, validation DRY, type accuracy, test coverage - Add post-write admin count verification in removeRoleMember to prevent zero-admin race condition (TOCTOU → rollback if count hits 0) - Make IRole.description optional; backfill in initializeRoles for pre-existing roles that lack the field (.lean() bypasses defaults) - Extract parsePagination, validateNameParam, validateRoleName, and validateDescription helpers to eliminate duplicated validation - Add validateNameParam guard to all 7 handlers reading req.params.name - Catch 11000 in updateRoleByName and surface as 409 via RoleConflictError - Add idempotent skip in addRoleMember when user already has target role - Verify updateRolePermissions test asserts response body - Add data-layer tests: listRoles sort/pagination/projection, countRoles, and createRoleByName 11000 duplicate key race * fix: defensive rollback in removeRoleMember, type/style cleanup, test coverage - Wrap removeRoleMember post-write admin rollback in try/catch so a transient DB failure cannot leave the system with zero administrators - Replace double `as unknown[] as IRole[]` cast with `.lean<IRole[]>()` - Type parsePagination param explicitly; extract DEFAULT/MAX page constants - Preserve original error cause in updateRoleByName re-throw - Add test for rollback failure path in removeRoleMember (returns 400) - Add test for pre-existing roles missing description field (.lean()) * chore: bump @librechat/data-schemas to 0.0.47 * fix: stale cache on rename, extract renameRole helper, shared pagination, cleanup - Fix updateRoleByName cache bug: invalidate old key and populate new key when updates.name differs from roleName (prevents stale cache after rename) - Extract renameRole helper to eliminate mutable outer-scope state flags (isRename, trimmedName, migrationRan) in updateRoleHandler - Unify system-role protection to 403 for both rename-from and rename-to - Extract parsePagination to shared admin/pagination.ts; use in both roles.ts and groups.ts - Extract name.trim() to local const in createRoleByName (was called 5×) - Remove redundant findOne pre-check in deleteRoleByName - Replace getUserModel closure with local const declarations - Remove redundant description ?? '' in createRoleHandler (schema default) - Add doc comment on updateRolePermissionsHandler noting cache dependency - Add data-layer tests for cache rename behavior (old key null, new key set) * fix: harden role guards, add User.role index, validate names, improve tests - Add index on User.role field for efficient member queries at scale - Replace fragile SystemRoles key lookup with value-based Set check (6 sites) - Elevate rename rollback failure logging to CRITICAL (matches removeRoleMember) - Guard removeRoleMember against non-ADMIN system roles (403 for USER) - Fix parsePagination limit=0 gotcha: use parseInt + NaN check instead of \|\| - Add control character and reserved path segment validation to role names - Simplify validateRoleName: remove redundant casts and dead conditions - Add JSDoc to deleteRoleByName documenting non-atomic window - Split mixed value+type import in methods/index.ts per AGENTS.md - Add 9 new tests: permissions assertion, combined rename+desc, createRole with permissions, pagination edge cases, control char/reserved name rejection, system role removeRoleMember guard * fix: exact-case reserved name check, consistent validation, cleaner createRole - Remove .toLowerCase() from reserved name check so only exact matches (members, permissions) are rejected, not legitimate names like "Members" - Extract trimmed const in validateRoleName for consistent validation - Add control char check to validateNameParam for parity with body validation - Build createRole roleData conditionally to avoid passing description: undefined - Expand deleteRoleByName JSDoc documenting self-healing design and no-op trade-off * fix: scope rename rollback to only migrated users, prevent cross-role corruption Capture user IDs before forward migration so the rollback path only reverts users this request actually moved. Previously the rollback called updateUsersByRole(newName, currentName) which would sweep all users with the new role — including any independently assigned by a concurrent admin request — causing silent cross-role data corruption. Adds findUserIdsByRole and updateUsersRoleByIds to the data layer. Extracts rollbackMigratedUsers helper to deduplicate rollback sites. * fix: guard last admin in addRoleMember to prevent zero-admin lockout Since each user has exactly one role, addRoleMember implicitly removes the user from their current role. Without a guard, reassigning the sole admin to a non-admin role leaves zero admins and locks out admin management. Adds the same countUsersByRole check used in removeRoleMember. * fix: wire findUserIdsByRole and updateUsersRoleByIds into roles route The scoped rollback deps added in `c89b5db` were missing from the route DI wiring, causing renameRole to call undefined and return a 500. * fix: post-write admin guard in addRoleMember, compound role index, review cleanup - Add post-write admin count check + rollback to addRoleMember to match removeRoleMember's two-phase TOCTOU protection (prevents zero-admin via concurrent requests) - Replace single-field User.role index with compound { role: 1, tenantId: 1 } to align with existing multi-tenant index pattern (email, OAuth IDs) - Narrow listRoles dep return type to RoleListItem (projected fields only) - Refactor validateDescription to early-return style per AGENTS.md - Remove redundant double .lean() in updateRoleByName - Document rename snapshot race window in renameRole JSDoc - Document cache null-set behavior in deleteRoleByName - Add routing-coupling comment on RESERVED_ROLE_NAMES - Add test for addRoleMember post-write rollback * fix: review cleanup — system-role guard, type safety, JSDoc accuracy, tests - Add system-role guard to addRoleMember: block direct assignment to non-ADMIN system roles (403), symmetric with removeRoleMember - Fix RESERVED_ROLE_NAMES comment: explain semantic URL ambiguity, not a routing conflict (Express resolves single vs multi-segment correctly) - Replace _id: unknown with Types.ObjectId \| string per AGENTS.md - Narrow listRoles data-layer return type to Pick<IRole, 'name' \| 'description'> to match the actual .select() projection - Move updateRoleHandler param check inside try/catch for consistency - Include user IDs in all CRITICAL rollback failure logs for operator recovery - Clarify deleteRoleByName JSDoc: replace "self-healing" with "idempotent", document that recovery requires caller retry - Add tests: system-role guard, promote non-admin to ADMIN, findUserIdsByRole throw prevents migration * fix: include _id in listRoles return type to match RoleListItem Pick<IRole, 'name' \| 'description'> omits _id, making it incompatible with the handler dep's RoleListItem which requires _id. * fix: case-insensitive system role guard, reject null permissions, check updateUser result - System role name checks now use case-insensitive comparison via toUpperCase() — prevents creating 'admin' or 'user' which would collide with the legacy roles route that uppercases params - Reject permissions: null in createRole (typeof null === 'object' was bypassing the validation) - Check updateUser return in addRoleMember — return 404 if the user was deleted between the findUser and updateUser calls * fix: check updateUser return in removeRoleMember for concurrent delete safety --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-03-27 15:44:47 -04:00
Dustin Healy	2e3d66cfe2	👥 feat: Admin Groups API Endpoints (#12387 ) * feat: add listGroups and deleteGroup methods to userGroup * feat: add admin groups handler factory and Express routes * fix: address convention violations in admin groups handlers * fix: address Copilot review findings in admin groups handlers - Escape regex in listGroups to prevent injection/ReDoS - Validate ObjectId format in all handlers accepting id/userId params - Replace N+1 findUser loop with batched findUsers query - Remove unused findGroupsByMemberId from dep interface - Map Mongoose ValidationError to 400 in create/update handlers - Validate name in updateGroupHandler (reject empty/whitespace) - Handle null updateGroupById result (race condition) - Tighten error message matching in add/remove member handlers * test: add unit tests for admin groups handlers * fix: address code review findings for admin groups Atomic delete/update handlers (single DB trip), pass through idOnTheSource, add removeMemberById for non-ObjectId members, deduplicate member results, fix error message exposure, add hard cap/sort to listGroups, replace GroupListFilter with Pick of GroupFilterOptions, validate memberIds as array, trim name in update, fix import order, and improve test hygiene with fresh IDs per test. * fix: cascade cleanup, pagination, and test coverage for admin groups Add deleteGrantsForPrincipal to systemGrant data layer and wire cascade cleanup (Config, AclEntry, SystemGrant) into deleteGroupHandler. Add limit/offset pagination to getGroupMembers. Guard empty PATCH bodies with 400. Remove dead type guard and unnecessary type cast. Add 11 new tests covering cascade delete, idempotent member removal, empty update, search filter, 500 error paths, and pagination. * fix: harden admin groups with cascade resilience, type safety, and fallback removal Wrap cascade cleanup in inner try/catch so partial failure logs but still returns 200 (group is already deleted). Replace Record<string, unknown> on deleteAclEntries with proper typed filter. Log warning for unmapped user ObjectIds in createGroup memberIds. Add removeMemberById fallback when removeUserFromGroup throws User not found for ObjectId-format userId. Extract VALID_GROUP_SOURCES constant. Add 3 new tests (60 total). * refactor: add countGroups, pagination, and projection type to data layer Extract buildGroupQuery helper, add countGroups method, support limit/offset/skip in listGroups, standardize session handling to .session(session ?? null), and tighten projection parameter from Record<string, unknown> to Record<string, 0 \| 1>. * fix: cascade resilience, pagination, validation, and error clarity for admin groups - Use Promise.allSettled for cascade cleanup so all steps run even if one fails; log individual rejections - Echo deleted group id in delete response - Add countGroups dep and wire limit/offset pagination for listGroups - Deduplicate memberIds before computing total in getGroupMembers - Use { memberIds: 1 } projection in getGroupMembers - Cap memberIds at 500 entries in createGroup - Reject search queries exceeding 200 characters - Clarify addGroupMember error for non-ObjectId userId - Document deleted-user fallback limitation in removeGroupMember * test: extend handler and DB-layer test coverage for admin groups Handler tests: projection assertion, dedup total, memberIds cap, search max length, non-ObjectId memberIds passthrough, cascade partial failure resilience, dedup scenarios, echo id in delete response. DB-layer tests: listGroups sort/filter/pagination, countGroups, deleteGroup, removeMemberById, deleteGrantsForPrincipal. * fix: cast group principalId to ObjectId for ACL entry cleanup deleteAclEntries is a thin deleteMany wrapper with no type casting, but grantPermission stores group principalId as ObjectId. Passing the raw string from req.params would leave orphaned ACL entries on group deletion. * refactor: remove redundant pagination clamping from DB listGroups Handler already clamps limit/offset at the API boundary. The DB method is a general-purpose building block and should not re-validate. * fix: add source and name validation, import order, and test coverage for admin groups - Validate source against VALID_GROUP_SOURCES in createGroupHandler - Cap name at 500 characters in both create and update handlers - Document total as upper bound in getGroupMembers response - Document ObjectId requirement for deleteAclEntries in cascade - Fix import ordering in test file (local value after type imports) - Add tests for updateGroup with description, email, avatar fields - Add tests for invalid source and name max-length in both handlers * fix: add field length caps, flatten nested try/catch, and fix logger level in admin groups Add max-length validation for description, email, avatar, and idOnTheSource in create/update handlers. Extract removeObjectIdMember helper to flatten nested try/catch per never-nesting convention. Downgrade unmapped-memberIds log from error to warn. Fix type import ordering and add missing await in removeMemberById for consistency.	2026-03-26 17:36:18 -04:00
Danny Avila	9f6d8c6e93	🧵 feat: ALS Context Middleware, Tenant Threading, and Config Cache Invalidation (#12407 ) * feat: add tenant context middleware for ALS-based isolation Introduces tenantContextMiddleware that propagates req.user.tenantId into AsyncLocalStorage, activating the Mongoose applyTenantIsolation plugin for all downstream DB queries within a request. - Strict mode (TENANT_ISOLATION_STRICT=true) returns 403 if no tenantId - Non-strict mode passes through for backward compatibility - No-op for unauthenticated requests - Includes 6 unit tests covering all paths * feat: register tenant middleware and wrap startup/auth in runAsSystem() - Register tenantContextMiddleware in Express app after capability middleware - Wrap server startup initialization in runAsSystem() for strict mode compat - Wrap auth strategy getAppConfig() calls in runAsSystem() since they run before user context is established (LDAP, SAML, OpenID, social login, AuthService) * feat: thread tenantId through all getAppConfig callers Pass tenantId from req.user to getAppConfig() across all callers that have request context, ensuring correct per-tenant cache key resolution. Also fixes getBaseConfig admin endpoint to scope to requesting admin's tenant instead of returning the unscoped base config. Files updated: - Controllers: UserController, PluginController - Middleware: checkDomainAllowed, balance - Routes: config - Services: loadConfigModels, loadDefaultModels, getEndpointsConfig, MCP - Audio services: TTSService, STTService, getVoices, getCustomConfigSpeech - Admin: getBaseConfig endpoint * feat: add config cache invalidation on admin mutations - Add clearOverrideCache(tenantId?) to flush per-principal override caches by enumerating Keyv store keys matching _OVERRIDE_: prefix - Add invalidateConfigCaches() helper that clears base config, override caches, tool caches, and endpoint config cache in one call - Wire invalidation into all 5 admin config mutation handlers (upsert, patch, delete field, delete overrides, toggle active) - Add strict mode warning when __default__ tenant fallback is used - Add 3 new tests for clearOverrideCache (all/scoped/base-preserving) * chore: update getUserPrincipals comment to reflect ALS-based tenant filtering The TODO(#12091) about missing tenantId filtering is resolved by the tenant context middleware + applyTenantIsolation Mongoose plugin. Group queries are now automatically scoped by tenantId via ALS. * fix: replace runAsSystem with baseOnly for pre-tenant code paths App configs are tenant-owned — runAsSystem() would bypass tenant isolation and return cross-tenant DB overrides. Instead, add baseOnly option to getAppConfig() that returns YAML-derived config only, with zero DB queries. All startup code, auth strategies, and MCP initialization now use getAppConfig({ baseOnly: true }) to get the YAML config without touching the Config collection. * fix: address PR review findings — middleware ordering, types, cache safety - Chain tenantContextMiddleware inside requireJwtAuth after passport auth instead of global app.use() where req.user is always undefined (Finding 1) - Remove global tenantContextMiddleware registration from index.js - Update BalanceMiddlewareOptions to include tenantId, remove redundant cast (Finding 4) - Add warning log when clearOverrideCache cannot enumerate keys on Redis (Finding 3) - Use startsWith instead of includes for cache key filtering (Finding 12) - Use generator loop instead of Array.from for key enumeration (Finding 3) - Selective barrel export — exclude _resetTenantMiddlewareStrictCache (Finding 5) - Move isMainThread check to module level, remove per-request check (Finding 9) - Move mid-file require to top of app.js (Finding 8) - Parallelize invalidateConfigCaches with Promise.all (Finding 10) - Remove clearOverrideCache from public app.js exports (internal only) - Strengthen getUserPrincipals comment re: ALS dependency (Finding 2) * fix: restore runAsSystem for startup DB ops, consolidate require, clarify baseOnly - Restore runAsSystem() around performStartupChecks, updateInterfacePermissions, initializeMCPs, and initializeOAuthReconnectManager — these make Mongoose queries that need system context in strict tenant mode (NEW-3) - Consolidate duplicate require('@librechat/api') in requireJwtAuth.js (NEW-1) - Document that baseOnly ignores role/userId/tenantId in JSDoc (NEW-2) * test: add requireJwtAuth tenant chaining + invalidateConfigCaches tests - requireJwtAuth: 5 tests verifying ALS tenant context is set after passport auth, isolated between concurrent requests, and not set when user has no tenantId (Finding 6) - invalidateConfigCaches: 4 tests verifying all four caches are cleared, tenantId is threaded through, partial failure is handled gracefully, and operations run in parallel via Promise.all (Finding 11) * fix: address Copilot review — passport errors, namespaced cache keys, /base scoping - Forward passport errors in requireJwtAuth before entering tenant middleware — prevents silent auth failures from reaching handlers (P1) - Account for Keyv namespace prefix in clearOverrideCache — stored keys are namespaced as "APP_CONFIG:_OVERRIDE_:..." not "_OVERRIDE_:...", so override caches were never actually matched/cleared (P2) - Remove role from getBaseConfig — /base should return tenant-scoped base config, not role-merged config that drifts per admin role (P2) - Return tenantStorage.run() for cleaner async semantics - Update mock cache in service.spec.ts to simulate Keyv namespacing * fix: address second review — cache safety, code quality, test reliability - Decouple cache invalidation from mutation response: fire-and-forget with logging so DB mutation success is not masked by cache failures - Extract clearEndpointConfigCache helper from inline IIFE - Move isMainThread check to lazy once-per-process guard (no import side effect) - Memoize process.env read in overrideCacheKey to avoid per-request env lookups and log flooding in strict mode - Remove flaky timer-based parallelism assertion, use structural check - Merge orphaned double JSDoc block on getUserPrincipals - Fix stale [getAppConfig] log prefix → [ensureBaseConfig] - Fix import order in tenant.spec.ts (package types before local values) - Replace "Finding 1" reference with self-contained description - Use real tenantStorage primitives in requireJwtAuth spec mock * fix: move JSDoc to correct function after clearEndpointConfigCache extraction * refactor: remove Redis SCAN from clearOverrideCache, rely on TTL expiry Redis SCAN causes 60s+ stalls under concurrent load (see #12410). APP_CONFIG defaults to FORCED_IN_MEMORY_CACHE_NAMESPACES, so the in-memory store.keys() path handles the standard case. When APP_CONFIG is Redis-backed, overrides expire naturally via overrideCacheTtl (60s default) — an acceptable window for admin config mutations. * fix: remove return from tenantStorage.run to satisfy void middleware signature * fix: address second review — cache safety, code quality, test reliability - Switch invalidateConfigCaches from Promise.all to Promise.allSettled so partial failures are logged individually instead of producing one undifferentiated error (Finding 3) - Gate overrideCacheKey strict-mode warning behind a once-per-process flag to prevent log flooding under load (Finding 4) - Add test for passport error forwarding in requireJwtAuth — the if (err) { return next(err) } branch now has coverage (Finding 5) - Add test for real partial failure in invalidateConfigCaches where clearAppConfigCache rejects (not just the swallowed endpoint error) * chore: reorder imports in index.js and app.js for consistency - Moved logger and runAsSystem imports to maintain a consistent import order across files. - Improved code readability by ensuring related imports are grouped together.	2026-03-26 17:35:00 -04:00
Danny Avila	4b6d68b3b5	🎛️ feat: DB-Backed Per-Principal Config System (#12354 ) * ✨ feat: Add Config schema, model, and methods for role-based DB config overrides Add the database foundation for principal-based configuration overrides (user, group, role) in data-schemas. Includes schema with tenantId and tenant isolation, CRUD methods, and barrel exports. * 🔧 fix: Add shebang and enforce LF line endings for git hooks The pre-commit hook was missing #!/bin/sh, and core.autocrlf=true was converting it to CRLF, both causing "Exec format error" on Windows. Add .gitattributes to force LF for .husky/* and .sh files. ✨ feat: Add admin config API routes with section-level capability checks Add /api/admin/config endpoints for managing per-principal config overrides (user, group, role). Handlers in @librechat/api use DI pattern with section-level hasConfigCapability checks for granular access control. Supports full overrides replacement, per-field PATCH via dot-paths, field deletion, toggle active, and listing. * 🐛 fix: Move deleteConfigField fieldPath from URL param to request body The path-to-regexp wildcard syntax (:fieldPath()) is not supported by the version used in Express. Send fieldPath in the DELETE request body instead, which also avoids URL-encoding issues with dotted paths. ✨ feat: Wire config resolution into getAppConfig with override caching Add mergeConfigOverrides utility in data-schemas for deep-merging DB config overrides into base AppConfig by priority order. Update getAppConfig to query DB for applicable configs when role/userId is provided, with short-TTL caching and a hasAnyConfigs feature flag for zero-cost when no DB configs exist. Also: add unique compound index on Config schema, pass userId from config middleware, and signal config changes from admin API handlers. * 🔄 refactor: Extract getAppConfig logic into packages/api as TS service Move override resolution, caching strategy, and signalConfigChange from api/server/services/Config/app.js into packages/api/src/app/appConfigService.ts using the DI factory pattern (createAppConfigService). The JS file becomes a thin wiring layer injecting loadBaseConfig, cache, and DB dependencies. * 🧹 chore: Rename configResolution.ts to resolution.ts * ✨ feat: Move admin types & capabilities to librechat-data-provider Move SystemCapabilities, CapabilityImplications, and utility functions (hasImpliedCapability, expandImplications) from data-schemas to data-provider so they are available to external consumers like the admin panel without a data-schemas dependency. Add API-friendly admin types: TAdminConfig, TAdminSystemGrant, TAdminAuditLogEntry, TAdminGroup, TAdminMember, TAdminUserSearchResult, TCapabilityCategory, and CAPABILITY_CATEGORIES. data-schemas re-exports these from data-provider and extends with config-schema-derived types (ConfigSection, SystemCapability union). Bump version to 0.8.500. * feat: Add JSON-serializable admin config API response types to data-schemas Add AdminConfig, AdminConfigListResponse, AdminConfigResponse, and AdminConfigDeleteResponse types so both LibreChat API handlers and the admin panel can share the same response contract. Bump version to 0.0.41. * refactor: Move admin capabilities & types from data-provider to data-schemas SystemCapabilities, CapabilityImplications, utility functions, CAPABILITY_CATEGORIES, and admin API response types should not be in data-provider as it gets compiled into the frontend bundle, exposing the capability surface. Moved everything to data-schemas (server-only). All consumers already import from @librechat/data-schemas, so no import changes needed elsewhere. Consolidated duplicate AdminConfig type (was in both config.ts and admin.ts). * chore: Bump @librechat/data-schemas to 0.0.42 * refactor: Reorganize admin capabilities into admin/ and types/admin.ts Split systemCapabilities.ts following data-schemas conventions: - Types (BaseSystemCapability, SystemCapability, AdminConfig, etc.) → src/types/admin.ts - Runtime code (SystemCapabilities, CapabilityImplications, utilities) → src/admin/capabilities.ts Revert data-provider version to 0.8.401 (no longer modified). * chore: Fix import ordering, rename appConfigService to service - Rename app/appConfigService.ts → app/service.ts (directory provides context) - Fix import order in admin/config.ts, types/admin.ts, types/config.ts - Add naming convention to AGENTS.md * feat: Add DB base config support (role/__base__) - Add BASE_CONFIG_PRINCIPAL_ID constant for reserved base config doc - getApplicableConfigs always includes __base__ in queries - getAppConfig queries DB even without role/userId when DB configs exist - Bump @librechat/data-schemas to 0.0.43 * fix: Address PR review issues for admin config - Add listAllConfigs method; listConfigs endpoint returns all active configs instead of only __base__ - Normalize principalId to string in all config methods to prevent ObjectId vs string mismatch on user/group lookups - Block __proto__ and all dunder-prefixed segments in field path validation to prevent prototype pollution - Fix configVersion off-by-one: default to 0, guard pre('save') with !isNew, use $inc on findOneAndUpdate - Remove unused getApplicableConfigs from admin handler deps * fix: Enable tree-shaking for data-schemas, bump packages - Switch data-schemas Rollup output to preserveModules so each source file becomes its own chunk; consumers (admin panel) can now import just the modules they need without pulling in winston/mongoose/etc. - Add sideEffects: false to data-schemas package.json - Bump data-schemas to 0.0.44, data-provider to 0.8.402 * feat: add capabilities subpath export to data-schemas Adds `@librechat/data-schemas/capabilities` subpath export so browser consumers can import BASE_CONFIG_PRINCIPAL_ID and capability constants without pulling in Node.js-only modules (winston, async_hooks, etc.). Bump version to 0.0.45. * fix: include dist/ in data-provider npm package Add explicit files field so npm includes dist/types/ in the published package. Without this, the root .gitignore exclusion of dist/ causes npm to omit type declarations, breaking TypeScript consumers. * chore: bump librechat-data-provider to 0.8.403 * feat: add GET /api/admin/config/base for raw AppConfig Returns the full AppConfig (YAML + DB base merged) so the admin panel can display actual config field values and structure. The startup config endpoint (/api/config) returns TStartupConfig which is a different shape meant for the frontend app. * chore: imports order * fix: address code review findings for admin config Critical: - Fix clearAppConfigCache: was deleting from wrong cache store (CONFIG_STORE instead of APP_CONFIG), now clears BASE and HAS_DB_CONFIGS keys - Eliminate race condition: patchConfigField and deleteConfigField now use atomic MongoDB $set/$unset with dot-path notation instead of read-modify-write cycles, removing the lost-update bug entirely - Add patchConfigFields and unsetConfigField atomic DB methods Major: - Reorder cache check before principal resolution in getAppConfig so getUserPrincipals DB query only fires on cache miss - Replace '' as ConfigSection with typed BROAD_CONFIG_ACCESS constant - Parallelize capability checks with Promise.all instead of sequential awaits in for loops - Use loose equality (== null) for cache miss check to handle both null and undefined returns from cache implementations - Set HAS_DB_CONFIGS_KEY to true on successful config fetch Minor: - Remove dead pre('save') hook from config schema (all writes use findOneAndUpdate which bypasses document hooks) - Consolidate duplicate type imports in resolution.ts - Remove dead deepGet/deepSet/deepUnset functions (replaced by atomic ops) - Add .sort({ priority: 1 }) to getApplicableConfigs query - Rename _impliedBy to impliedByMap * fix: self-referencing BROAD_CONFIG_ACCESS constant * fix: replace type-cast sentinel with proper null parameter Update hasConfigCapability to accept ConfigSection \| null where null means broad access check (MANAGE_CONFIGS or READ_CONFIGS only). Removes the '' as ConfigSection type lie from admin config handlers. * fix: remaining review findings + add tests - listAllConfigs accepts optional { isActive } filter so admin listing can show inactive configs (#9) - Standardize session application to .session(session ?? null) across all config DB methods (#15) - Export isValidFieldPath and getTopLevelSection for testability - Add 38 tests across 3 spec files: - config.spec.ts (api): path validation, prototype pollution rejection - resolution.spec.ts: deep merge, priority ordering, array replacement - config.spec.ts (data-schemas): full CRUD, ObjectId normalization, atomic $set/$unset, configVersion increment, toggle, __base__ query * fix: address second code review findings - Fix cross-user cache contamination: overrideCacheKey now handles userId-without-role case with its own cache key (#1) - Add broad capability check before DB lookup in getConfig to prevent config existence enumeration (#2/#3) - Move deleteConfigField fieldPath from request body to query parameter for proxy/load balancer compatibility (#5) - Derive BaseSystemCapability from SystemCapabilities const instead of manual string union (#6) - Return 201 on upsert creation, 200 on update (#11) - Remove inline narration comments per AGENTS.md (#12) - Type overrides as Partial<TCustomConfig> in DB methods and handler deps (#13) - Replace double as-unknown-as casts in resolution.ts with generic deepMerge<T> (#14) - Make override cache TTL injectable via AppConfigServiceDeps (#16) - Add exhaustive never check in principalModel switch (#17) * fix: remaining review findings — tests, rename, semantics - Rename signalConfigChange → markConfigsDirty with JSDoc documenting the stale-window tradeoff and overrideCacheTtl knob - Fix DEFAULT_OVERRIDE_CACHE_TTL naming convention - Add createAppConfigService tests (14 cases): cache behavior, feature flag, cross-user key isolation, fallback on error, markConfigsDirty - Add admin handler integration tests (13 cases): auth ordering, 201/200 on create/update, fieldPath from query param, markConfigsDirty calls, capability checks * fix: global flag corruption + empty overrides auth bypass - Remove HAS_DB_CONFIGS_KEY=false optimization: a scoped query returning no configs does not mean no configs exist globally. Setting the flag false from a per-principal query short-circuited all subsequent users. - Add broad manage capability check before section checks in upsertConfigOverrides: empty overrides {} no longer bypasses auth. * test: add regression and invariant tests for config system Regression tests: - Bug 1: User A's empty result does not short-circuit User B's overrides - Bug 2: Empty overrides {} returns 403 without MANAGE_CONFIGS Invariant tests (applied across ALL handlers): - All 5 mutation handlers call markConfigsDirty on success - All 5 mutation handlers return 401 without auth - All 5 mutation handlers return 403 without capability - All 3 read handlers return 403 without capability * fix: third review pass — all findings addressed Service (service.ts): - Restore HAS_DB_CONFIGS=false for base-only queries (no role/userId) so deployments with zero DB configs skip DB queries (#1) - Resolve cache once at factory init instead of per-invocation (#8) - Use BASE_CONFIG_PRINCIPAL_ID constant in overrideCacheKey (#10) - Add JSDoc to clearAppConfigCache documenting stale-window (#4) - Fix log message to not say "from YAML" (#14) Admin handlers (config.ts): - Use configVersion===1 for 201 vs 200, eliminating TOCTOU race (#2) - Add Array.isArray guard on overrides body (#5) - Import CapabilityUser from capabilities.ts, remove duplicate (#6) - Replace as-unknown-as cast with targeted type assertion (#7) - Add MAX_PATCH_ENTRIES=100 cap on entries array (#15) - Reorder deleteConfigField to validate principalType first (#12) - Export CapabilityUser from middleware/capabilities.ts DB methods (config.ts): - Remove isActive:true from patchConfigFields to prevent silent reactivation of disabled configs (#3) Schema (config.ts): - Change principalId from Schema.Types.Mixed to String (#11) Tests: - Add patchConfigField unsafe fieldPath rejection test (#9) - Add base-only HAS_DB_CONFIGS=false test (#1) - Update 201/200 tests to use configVersion instead of findConfig (#2) * fix: add read handler 401 invariant tests + document flag behavior - Add invariant: all 3 read handlers return 401 without auth - Document on markConfigsDirty that HAS_DB_CONFIGS stays true after all configs are deleted until clearAppConfigCache or restart * fix: remove HAS_DB_CONFIGS false optimization entirely getApplicableConfigs([]) only queries for __base__, not all configs. A deployment with role/group configs but no __base__ doc gets the flag poisoned to false by a base-only query, silently ignoring all scoped overrides. The optimization is not safe without a comprehensive Config.exists() check, which adds its own DB cost. Removed entirely. The flag is now write-once-true (set when configs are found or by markConfigsDirty) and only cleared by clearAppConfigCache/restart. * chore: reorder import statements in app.js for clarity * refactor: remove HAS_DB_CONFIGS_KEY machinery entirely The three-state flag (false/null/true) was the source of multiple bugs across review rounds. Every attempt to safely set it to false was defeated by getApplicableConfigs querying only a subset of principals. Removed: HAS_DB_CONFIGS_KEY constant, all reads/writes of the flag, markConfigsDirty (now a no-op concept), notifyChange wrapper, and all tests that seeded false manually. The per-user/role TTL cache (overrideCacheTtl, default 60s) is the sole caching mechanism. On cache miss, getApplicableConfigs queries the DB. This is one indexed query per user per TTL window — acceptable for the config override use case. * docs: rewrite admin panel remaining work with current state * perf: cache empty override results to avoid repeated DB queries When getApplicableConfigs returns no configs for a principal, cache baseConfig under their override key with TTL. Without this, every user with no per-principal overrides hits MongoDB on every request after the 60s cache window expires. * fix: add tenantId to cache keys + reject PUBLIC principal type - Include tenantId in override cache keys to prevent cross-tenant config contamination. Single-tenant deployments (tenantId undefined) use '_' as placeholder — no behavior change for them. - Reject PrincipalType.PUBLIC in admin config validation — PUBLIC has no PrincipalModel and is never resolved by getApplicableConfigs, so config docs for it would be dead data. - Config middleware passes req.user.tenantId to getAppConfig. * fix: fourth review pass findings DB methods (config.ts): - findConfigByPrincipal accepts { includeInactive } option so admin GET can retrieve inactive configs (#5) - upsertConfig catches E11000 duplicate key on concurrent upserts and retries without upsert flag (#2) - unsetConfigField no longer filters isActive:true, consistent with patchConfigFields (#11) - Typed filter objects replace Record<string, unknown> (#12) Admin handlers (config.ts): - patchConfigField: serial broad capability check before Promise.all to pre-warm ALS principal cache, preventing N parallel DB calls (#3) - isValidFieldPath rejects leading/trailing dots and consecutive dots (#7) - Duplicate fieldPaths in patch entries return 400 (#8) - DEFAULT_PRIORITY named constant replaces hardcoded 10 (#14) - Admin getConfig and patchConfigField pass includeInactive to findConfigByPrincipal (#5) - Route import uses barrel instead of direct file path (#13) Resolution (resolution.ts): - deepMerge has MAX_MERGE_DEPTH=10 guard to prevent stack overflow from crafted deeply nested configs (#4) * fix: final review cleanup - Remove ADMIN_PANEL_REMAINING.md (local dev notes with Windows paths) - Add empty-result caching regression test - Add tenantId to AdminConfigDeps.getAppConfig type - Restore exhaustive never check in principalModel switch - Standardize toggleConfigActive session handling to options pattern * fix: validate priority in patchConfigField handler Add the same non-negative number validation for priority that upsertConfigOverrides already has. Without this, invalid priority values could be stored via PATCH and corrupt merge ordering. * chore: remove planning doc from PR * fix: correct stale cache key strings in service tests * fix: clean up service tests and harden tenant sentinel - Remove no-op cache delete lines from regression tests - Change no-tenant sentinel from '_' to '__default__' to avoid collision with a real tenant ID when multi-tenancy is enabled - Remove unused CONFIG_STORE from AppConfigServiceDeps * chore: bump @librechat/data-schemas to 0.0.46 * fix: block prototype-poisoning keys in deepMerge Skip __proto__, constructor, and prototype keys during config merge to prevent prototype pollution via PUT /api/admin/config overrides.	2026-03-25 19:39:29 -04:00
Marco Beretta	ccd049d8ce	📁 refactor: Prompts UI (#11570 ) * style: enhance prompts UI with new components and improved structure; add CreatePromptButton and AutoSendPrompt; refactor GroupSidePanel and PromptsAccordion * refactor(Prompts): move button components to buttons/ subdirectory * refactor(Prompts): move dialog components to dialogs/ subdirectory * refactor(Prompts): move display components to display/ subdirectory * refactor(Prompts): move editor components to editor/ subdirectory * refactor(Prompts): move field components to fields/ subdirectory * refactor(Prompts): move form components to forms/ subdirectory * refactor(Prompts): move layout components to layouts/ subdirectory * refactor(Prompts): move list components to lists/ subdirectory * refactor(Prompts): move sidebar components to sidebar/ subdirectory * refactor(Prompts): move utility components to utils/ subdirectory * refactor(Prompts): update main exports and external imports * refactor(Prompts): fix class name typo in AutoSendPrompt * refactor(Prompts): reorganize exports and imports order across components * refactor(Prompts): reorder exports for better organization and clarity * refactor(Buttons): enhance prompts accessibility with aria-labels and update translations * refactor(AdminSettings): reorganize imports and improve form structure for clarity * refactor(Dialogs): reorganize imports for consistency and clarity across DeleteVersion, SharePrompt, and VariableDialog components * refactor(Dialogs): enhance prompts accessibility with aria-labels * refactor(Display): enhance prompt components and accessibility features * refactor(.gitignore): add Playwright MCP directory * refactor(Preview): enhance prompt components, improve layout, and add accessibility features * refactor(Prompts): enhance variable handling, improve accessibility, and update UI components * refactor(Prompts): enhance loading state handling and improve accessibility in PromptName component * refactor(Prompts): streamline special variable handling, improve icon management, and enhance UI components * refactor(Prompts): update AdvancedSwitch component to use Radio for mode selection, enhance PromptName with tooltips, and improve layout in PromptForm * refactor(Prompts): enhance VersionCard and VersionBadge components for improved UI and accessibility, update loading state handling in VersionsPanel * refactor(Prompts): improve layout and styling of VersionCard component for better visual alignment and clarity * refactor(DeleteVersion): update text color for confirmation prompt in DeleteConfirmDialog * refactor(Prompts): add configurations for always make production and auto-send prompts, update localization strings for clarity * refactor(Prompts): enhance layout and styling in CategorySelector, CreatePromptForm, and List components for improved responsiveness and clarity * refactor(Prompts): enhance PromptDetailHeader and ChatGroupItem components, add shared prompt indication, and remove unused PromptMetadata component * refactor(Prompts): implement prompt group usage tracking, update sorting logic, and enhance related components * fix(Prompts): security, performance, and pagination fixes - Fix cursor pagination skipping/duplicating items by including numberOfGenerations in cursor condition to match sort order - Close NoSQL injection vector via otherFilters rest spread in GET /all, GET /groups, and buildPromptGroupFilter - Validate groupId as ObjectId before passing to query (GET /) - Add prompt body validation in addPromptToGroup (type + text) - Return 404 instead of 500 for missing group in POST /use - Combine data + count into single $facet aggregation - Add compound index {numberOfGenerations, updatedAt, _id} - Add index on prompt.author for deleteUserPrompts - Update useRecordPromptUsage to refresh client caches - Replace console.error with logger.error * refactor(PromptForm): remove console warning for unselected prompt in VersionsPanel * refactor(Prompts): improve error handling for groupId and streamline usage tracking * refactor(.gitignore): add CLAUDE.md to ignore list * refactor(Prompts): streamline prompt components by removing unused variables and enhancing props structure * refactor(Prompts): fix sort stability, keyboard handling, and remove dead code Add _id tiebreaker to prompt group sort pipelines for deterministic pagination ordering. Prevent default browser scroll on Space key in PromptEditor preview mode. Remove unused blurTimeoutRef and its onMutate callback from DashGroupItem. * refactor(Prompts): enhance groupId validation and improve prompt group aggregation handling * fix: aria-hidden, API fixes, accessibility improvements * fix: ACL author filter, mobile guard, semantic HTML, and add useFocusTrap hook - Remove author filter from patchPromptGroup so ACL-granted editors can update prompt groups (aligns with deletePromptGroupController) - Add missing group guard to mobile HeaderActions in PromptForm - Replace div with article in DashGroupItem, remove redundant stopPropagation and onClick on outer container - Add useFocusTrap hook for keyboard focus management - Add numberOfGenerations to default projection - Deduplicate ObjectId validation, remove console.warn, fix aria-labelledby, localize search announcements * refactor(Prompts): adjust UI and improve a11y * refactor(Prompts): reorder imports for consistency and clarity * refactor(Prompts): implement updateFieldsInPlace for efficient data updates and add related tests * refactor(Prompts): reorder imports to include updateFieldsInPlace for better organization * refactor(Prompts): enhance DashGroupItem with toast notifications for prompt updates and add click-to-edit functionality in PromptEditor * style: use self-closing TooltipAnchor in CreatePromptButton Replace ></TooltipAnchor> with /> for consistency with the rest of the Prompts directory. * fix(i18n): replace placeholder text for com_ui_global_group translation key The value was left as 'something needs to go here. was empty' which would be visible to users as an aria-label in DashGroupItem. * fix(DashGroupItem): sync rename input with group.name on external changes nameInputValue was initialized via useState(group.name) but never synced when group.name changed from a background refetch. Added useEffect that updates the input when the dialog is closed. * perf(useFocusTrap): store onEscape in ref to avoid listener churn onEscape was in the useEffect dependency array, causing the keydown listener to be torn down and re-attached on every render when callers passed an inline function. Now stored in a ref so the effect only re-runs when active or containerRef changes. * fix(a11y): replace role=button div with layered button overlay in ListCard The card used role='button' on a div that contained nested Button elements — an invalid ARIA pattern. Replaced with a hidden button at z-0 for the card action while child interactive elements sit at z-10, eliminating nested interactive element violations. * fix(PromptForm): reset selectionIndex on route change, guard auto-save, and fix a11y - Reset selectionIndex to 0 and isEditing to false when promptId changes, preventing out-of-bounds index when navigating between groups with different version counts. - Track selectedPrompt in a ref so the auto-save effect doesn't fire against a stale prompt when the selection changed mid-edit. - Stabilize useFocusTrap onEscape via useCallback to avoid unnecessary listener re-attachment. - Conditionally render mobile overlay instead of always-present button with aria-hidden/pointer-events toggling. * refactor: extract isValidObjectIdString to shared utility in data-schemas The same regex helper was duplicated in api/server/routes/prompts.js and packages/data-schemas/src/methods/prompt.ts. Moved to packages/data-schemas/src/utils/objectId.ts and imported from both consumers. Also removed a duplicate router.use block introduced during the extraction. * perf(updateFieldsInPlace): replace JSON deep clone with targeted spread Instead of JSON.parse(JSON.stringify(data)) which serializes the entire paginated data structure, use targeted immutable spreads that only copy the affected page and collection array. Returns the original data reference unchanged when the item is not found. * perf(VariablesDropdown): memoize items array and stabilize handleAddVariable The items array containing JSX elements was rebuilt on every render. Wrapped in useMemo keyed on usedVariables and localize. Also wrapped handleAddVariable in useCallback and memoized usedCount to avoid redundant array filtering. * perf(DashGroupItem): stabilize mutation callbacks via refs handleSaveRename and handleDelete had updateGroup/deleteGroup mutation objects in their useCallback dependency arrays. Since mutation objects are new references each render, the callbacks were recreated every render, defeating memoization. Now store mutation objects in refs and call via ref.current in the callbacks. * fix(security): validate groupId in incrementPromptGroupUsage The data-schema method passed the groupId string directly to findByIdAndUpdate without validation. If called from a different entrypoint without the route-level check, Mongoose would throw a CastError. Now validates with isValidObjectIdString before the DB call and throws a clean 'Invalid groupId' error. * fix(security): add rate limiter to prompt usage tracking endpoint POST /groups/:groupId/use had no rate limiting — a user could spam it to inflate numberOfGenerations, which controls sort order for all users. Added promptUsageLimiter (30 req/user/min) following the same pattern as toolCallLimiter. Also handle 'Invalid groupId' error from the data layer in the route error handler. * fix(updateFieldsInPlace): guard against undefined identifier value If updatedItem[identifierField] is null/undefined, findIndex could match unintended items where that field is also undefined. Added early return when the identifier value is nullish. * fix(a11y): use React useId for stable unique IDs in ListCard aria-describedby/id values were derived from prompt name which can contain spaces and special characters, producing invalid HTML IDs and potential collisions. Now uses React.useId() for guaranteed unique, valid IDs per component instance. * fix: Align prompts panel styling with other sidebar panels and fix test - Match FilterPrompts first row to Memory/Bookmark pattern (items-center gap-2) - Remove items-stretch override from PromptsAccordion - Add missing promptUsageLimiter mock to prompts route test * fix: Address code review findings for prompts refactor PR - Fix #5: Gate DeletePrompt in HeaderActions behind canDelete permission - Fix #8: BackToChat navigates to last conversation instead of /c/new - Fix #7: Restore useLiveAnnouncer for screen reader feedback on delete/rename - Fix #1: Use isPublic (set by API) instead of deprecated projectIds for globe icon - Fix #4: Optimistic cache update in useRecordPromptUsage instead of full invalidation - Fix #6: Add migration to drop superseded { createdAt, updatedAt } compound index - Fix #9: Single-pass reduce in PromptVariables instead of triple filter - Fix #10: Rename PromptLabelsForm internal component to avoid collision with PromptForm - Fix #14: Remove redundant aria-label from aria-hidden Checkbox in AutoSendPrompt * fix: Align prompts panel filter row element sizes with other panels - Override Dropdown trigger to size-9 (36px) to match FilterInput height - Set CreatePromptButton to size-9 shrink-0 bg-transparent matching Memory/Bookmark panel button pattern * fix(prompts): Shared Prompts filter ignores direct shares, only returns PUBLIC Folds fix from PR #11882 into the refactored codebase. Bug A: filterAccessibleIdsBySharedLogic now accepts ownedPromptGroupIds: - MY_PROMPTS: accessible intersect owned - SHARED_PROMPTS: (accessible union public) minus owned - ALL: accessible union public (deduplicated) Legacy fallback preserved when ownedPromptGroupIds is omitted. Bug B: getPromptGroup uses $lookup aggregation to populate productionPrompt, fixing empty text on direct URL navigation to shared prompts. Also adds getOwnedPromptGroupIds to data-schemas methods and passes it from both /all and /groups route handlers. * fix: Add missing canDelete to mobile HeaderActions, remove dead instanceProjectId prop - Pass canDelete to mobile HeaderActions row (was only on desktop) - Remove instanceProjectId prop from ChatGroupItem and DashGroupItem since global check now uses group.isPublic - Remove useGetStartupConfig from List.tsx (no longer needed) * fix: Use runtime ObjectId instead of type-only Types.ObjectId, fix i18next interpolation - getPromptGroup and getOwnedPromptGroupIds were using Types.ObjectId (imported as type-only), which is erased at compile time. Use the runtime ObjectId from mongoose.Types (already destructured at line 20). This fixes the 404s in PATCH /groups/:groupId tests. - Fix com_ui_prompt_deleted_group translation to use {{0}} (i18next double-brace syntax) instead of {0}. * chore: Fix translation key ordering, add sideEffects: false to data-provider - Reorder new translation keys to maintain alphabetical order: com_ui_click_to_edit, com_ui_labels, com_ui_live, com_ui_prompt_delete_confirm, com_ui_prompt_deleted_group, com_ui_prompt_details, com_ui_prompt_renamed, com_ui_prompt_update_error, com_ui_prompt_variables_list - Add "sideEffects": false to librechat-data-provider package.json to enable tree-shaking of unused exports (types, constants, pure functions) * fix: Reduce prompts panel spacing, align memory toggle with checkbox pattern - Remove unnecessary wrapper div around AutoSendPrompt in PromptsAccordion, reducing vertical space between the toggle and the first prompt item - Replace Memory panel's Switch toggle with Checkbox+Button pattern matching the prompts panel's AutoSendPrompt for visual consistency * fix: Reduce gap between AutoSendPrompt and first prompt item Change ChatGroupItem margin from my-2 to mb-2 to eliminate the doubled spacing (gap-2 from parent + top margin from first item). Restore wrapper div around AutoSendPrompt for right-alignment. * fix: Restore prompt name on empty save, remove dead bodyProps from checkGlobalPromptShare - PromptName: reset newName to name when save is cancelled due to empty or unchanged input, preventing blank title in read mode - checkGlobalPromptShare: remove dead bodyProps config — Permissions.SHARE was not in the permissions array so the bodyProps rule was never evaluated. Per-resource share checks are handled by canAccessPromptGroupResource. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-03-22 16:56:22 -04:00
Danny Avila	b5c097e5c7	⚗️ feat: Agent Context Compaction/Summarization (#12287 ) * chore: imports/types Add summarization config and package-level summarize handler contracts Register summarize handlers across server controller paths Port cursor dual-read/dual-write summary support and UI status handling Selectively merge cursor branch files for BaseClient summary content block detection (last-summary-wins), dual-write persistence, summary block unit tests, and on_summarize_status SSE event handling with started/completed/failed branches. Co-authored-by: Cursor <cursoragent@cursor.com> refactor: type safety feat: add localization for summarization status messages refactor: optimize summary block detection in BaseClient Updated the logic for identifying existing summary content blocks to use a reverse loop for improved efficiency. Added a new test case to ensure the last summary content block is updated correctly when multiple summary blocks exist. chore: add runName to chainOptions in AgentClient refactor: streamline summarization configuration and handler integration Removed the deprecated summarizeNotConfigured function and replaced it with a more flexible createSummarizeFn. Updated the summarization handler setup across various controllers to utilize the new function, enhancing error handling and configuration resolution. Improved overall code clarity and maintainability by consolidating summarization logic. feat(summarization): add staged chunk-and-merge fallback feat(usage): track summarization usage separately from messages feat(summarization): resolve prompt from config in runtime fix(endpoints): use @librechat/api provider config loader refactor(agents): import getProviderConfig from @librechat/api chore: code order feat(app-config): auto-enable summarization when configured feat: summarization config refactor(summarization): streamline persist summary handling and enhance configuration validation Removed the deprecated createDeferredPersistSummary function and integrated a new createPersistSummary function for MongoDB persistence. Updated summarization handlers across various controllers to utilize the new persistence method. Enhanced validation for summarization configuration to ensure provider, model, and prompt are properly set, improving error handling and overall robustness. refactor(summarization): update event handling and remove legacy summarize handlers Replaced the deprecated summarization handlers with new event-driven handlers for summarization start and completion across multiple controllers. This change enhances the clarity of the summarization process and improves the integration of summarization events in the application. Additionally, removed unused summarization functions and streamlined the configuration loading process. refactor(summarization): standardize event names in handlers Updated event names in the summarization handlers to use constants from GraphEvents for consistency and clarity. This change improves maintainability and reduces the risk of errors related to string literals in event handling. feat(summarization): enhance usage tracking for summarization events Added logic to track summarization usage in multiple controllers by checking the current node type. If the node indicates a summarization task, the usage type is set accordingly. This change improves the granularity of usage data collected during summarization processes. feat(summarization): integrate SummarizationConfig into AppSummarizationConfig type Enhanced the AppSummarizationConfig type by extending it with the SummarizationConfig type from librechat-data-provider. This change improves type safety and consistency in the summarization configuration structure. test: add end-to-end tests for summarization functionality Introduced a comprehensive suite of end-to-end tests for the summarization feature, covering the full LibreChat pipeline from message creation to summarization. This includes a new setup file for environment configuration and a Jest configuration specifically for E2E tests. The tests utilize real API keys and ensure proper integration with the summarization process, enhancing overall test coverage and reliability. refactor(summarization): include initial summary in formatAgentMessages output Updated the formatAgentMessages function to return an initial summary alongside messages and index token count map. This change is reflected in multiple controllers and the corresponding tests, enhancing the summarization process by providing additional context for each agent's response. refactor: move hydrateMissingIndexTokenCounts to tokenMap utility Extracted the hydrateMissingIndexTokenCounts function from the AgentClient and related tests into a new tokenMap utility file. This change improves code organization and reusability, allowing for better management of token counting logic across the application. refactor(summarization): standardize step event handling and improve summary rendering Refactored the step event handling in the useStepHandler and related components to utilize constants for event names, enhancing consistency and maintainability. Additionally, improved the rendering logic in the Summary component to conditionally display the summary text based on its availability, providing a better user experience during the summarization process. feat(summarization): introduce baseContextTokens and reserveTokensRatio for improved context management Added baseContextTokens to the InitializedAgent type to calculate the context budget based on agentMaxContextNum and maxOutputTokensNum. Implemented reserveTokensRatio in the createRun function to allow configurable context token management. Updated related tests to validate these changes and ensure proper functionality. feat(summarization): add minReserveTokens, context pruning, and overflow recovery configurations Introduced new configuration options for summarization, including minReserveTokens, context pruning settings, and overflow recovery parameters. Updated the createRun function to accommodate these new options and added a comprehensive test suite to validate their functionality and integration within the summarization process. feat(summarization): add updatePrompt and reserveTokensRatio to summarization configuration Introduced an updatePrompt field for updating existing summaries with new messages, enhancing the flexibility of the summarization process. Additionally, added reserveTokensRatio to the configuration schema, allowing for improved management of token allocation during summarization. Updated related tests to validate these new features. feat(logging): add on_agent_log event handler for structured logging Implemented an on_agent_log event handler in both the agents' callbacks and responses to facilitate structured logging of agent activities. This enhancement allows for better tracking and debugging of agent interactions by logging messages with associated metadata. Updated the summarization process to ensure proper handling of log events. fix: remove duplicate IBalanceUpdate interface declaration perf(usage): single-pass partition of collectedUsage Replace two Array.filter() passes with a single for-of loop that partitions message vs. summarization usages in one iteration. fix(BaseClient): shallow-copy message content before mutating and preserve string content Avoid mutating the original message.content array in-place when appending a summary block. Also convert string content to a text content part instead of silently discarding it. fix(ui): fix Part.tsx indentation and useStepHandler summarize-complete handling - Fix SUMMARY else-if branch indentation in Part.tsx to match chain level - Guard ON_SUMMARIZE_COMPLETE with didFinalize flag to avoid unnecessary re-renders when no summarizing parts exist - Protect against undefined completeData.summary instead of unsafe spread fix(agents): use strict enabled check for summarization handlers Change summarizationConfig?.enabled !== false to === true so handlers are not registered when summarizationConfig is undefined. chore: fix initializeClient JSDoc and move DEFAULT_RESERVE_RATIO to module scope refactor(Summary): align collapse/expand behavior with Reasoning component - Single render path instead of separate streaming vs completed branches - Use useMessageContext for isSubmitting/isLatestMessage awareness so the "Summarizing..." label only shows during active streaming - Default to collapsed (matching Reasoning), user toggles to expand - Add proper aria attributes (aria-hidden, role, aria-controls, contentId) - Hide copy button while actively streaming feat(summarization): default to self-summarize using agent's own provider/model When no summarization config is provided (neither in librechat.yaml nor on the agent), automatically enable summarization using the agent's own provider and model. The agents package already provides default prompts, so no prompt configuration is needed. Also removes the dead resolveSummarizationLLMConfig in summarize.ts (and its spec) — run.ts buildAgentContext is the single source of truth for summarization config resolution. Removes the duplicate RuntimeSummarizationConfig local type in favor of the canonical SummarizationConfig from data-provider. chore: schema and type cleanup for summarization - Add trigger field to summarizationAgentOverrideSchema so per-agent trigger overrides in librechat.yaml are not silently stripped by Zod - Remove unused SummarizationStatus type from runs.ts - Make AppSummarizationConfig.enabled non-optional to reflect the invariant that loadSummarizationConfig always sets it refactor(responses): extract duplicated on_agent_log handler refactor(run): use agents package types for summarization config Import SummarizationConfig, ContextPruningConfig, and OverflowRecoveryConfig from @librechat/agents and use them to type-check the translation layer in buildAgentContext. This ensures the config object passed to the agent graph matches what it expects. - Use `satisfies AgentSummarizationConfig` on the config object - Cast contextPruningConfig and overflowRecoveryConfig to agents types - Properly narrow trigger fields from DeepPartial to required shape feat(config): add maxToolResultChars to base endpoint schema Add maxToolResultChars to baseEndpointSchema so it can be configured on any endpoint in librechat.yaml. Resolved during agent initialization using getProviderConfig's endpoint resolution: custom endpoint config takes precedence, then the provider-specific endpoint config, then the shared `all` config. Passed through to the agents package ToolNode, which uses it to cap tool result length before it enters the context window. When not configured, the agents package computes a sensible default from maxContextTokens. fix(summarization): forward agent model_parameters in self-summarize default When no explicit summarization config exists, the self-summarize default now forwards the agent's model_parameters as the summarization parameters. This ensures provider-specific settings (e.g. Bedrock region, credentials, endpoint host) are available when the agents package constructs the summarization LLM. fix(agents): register summarization handlers by default Change the enabled gate from === true to !== false so handlers register when no explicit summarization config exists. This aligns with the self-summarize default where summarization is always on unless explicitly disabled via enabled: false. refactor(summarization): let agents package inherit clientOptions for self-summarize Remove model_parameters forwarding from the self-summarize default. The agents package now reuses the agent's own clientOptions when the summarization provider matches the agent's provider, inheriting all provider-specific settings (region, credentials, proxy, etc.) automatically. refactor(summarization): use MessageContentComplex[] for summary content Unify summary content to always use MessageContentComplex[] arrays, matching the pattern used by on_message_delta. No more string \| array unions — content is always an array of typed blocks ({ type: 'text', text: '...' } for text, { type: 'reasoning_content', ... } for reasoning). Agents package: - SummaryContentBlock.content: MessageContentComplex[] (was string) - tokenCount now optional (not sent on deltas) - Removed reasoning field — reasoning is now a content block type - streamAndCollect normalizes all chunks to content block arrays - Delta events pass content blocks directly LibreChat: - SummaryContentPart.content: Agents.MessageContentComplex[] - Updated Part.tsx, Summary.tsx, useStepHandler.ts, BaseClient.js - Summary.tsx derives display text from content blocks via useMemo - Aggregator uses simple array spread refactor(summarization): enhance summary handling and text extraction - Updated BaseClient.js to improve summary text extraction, accommodating both legacy and new content formats. - Modified summarization logic to ensure consistent handling of summary content across different message formats. - Adjusted test cases in summarization.e2e.spec.js to utilize the new summary text extraction method. - Refined SSE useStepHandler to initialize summary content as an array. - Updated configuration schema by removing unused minReserveTokens field. - Cleaned up SummaryContentPart type by removing rangeHash property. These changes streamline the summarization process and ensure compatibility with various content structures. refactor(summarization): streamline usage tracking and logging - Removed direct checks for summarization nodes in ModelEndHandler and replaced them with a dedicated markSummarizationUsage function for better readability and maintainability. - Updated OpenAIChatCompletionController and responses handlers to utilize the new markSummarizationUsage function for setting usage types. - Enhanced logging functionality by ensuring the logger correctly handles different log levels. - Introduced a new useCopyToClipboard hook in the Summary component to encapsulate clipboard copy logic, improving code reusability and clarity. These changes improve the overall structure and efficiency of the summarization handling and logging processes. refactor(summarization): update summary content block documentation - Removed outdated comment regarding the last summary content block in BaseClient.js. - Added a new comment to clarify the purpose of the findSummaryContentBlock method, ensuring consistency in documentation. These changes enhance code clarity and maintainability by providing accurate descriptions of the summarization logic. refactor(summarization): update summary content structure in tests - Modified the summarization content structure in e2e tests to use an array format for text, aligning with recent changes in summary handling. - Updated test descriptions to clarify the behavior of context token calculations, ensuring consistency and clarity in the tests. These changes enhance the accuracy and maintainability of the summarization tests by reflecting the updated content structure. refactor(summarization): remove legacy E2E test setup and configuration - Deleted the e2e-setup.js and jest.e2e.config.js files, which contained legacy configurations for E2E tests using real API keys. - Introduced a new summarization.e2e.ts file that implements comprehensive E2E backend integration tests for the summarization process, utilizing real AI providers and tracking summaries throughout the run. These changes streamline the testing framework by consolidating E2E tests into a single, more robust file while removing outdated configurations. refactor(summarization): enhance E2E tests and error handling - Added a cleanup step to force exit after all tests to manage Redis connections. - Updated the summarization model to 'claude-haiku-4-5-20251001' for consistency across tests. - Improved error handling in the processStream function to capture and return processing errors. - Enhanced logging for cross-run tests and tight context scenarios to provide better insights into test execution. These changes improve the reliability and clarity of the E2E tests for the summarization process. refactor(summarization): enhance test coverage for maxContextTokens behavior - Updated run-summarization.test.ts to include a new test case ensuring that maxContextTokens does not exceed user-defined limits, even when calculated ratios suggest otherwise. - Modified summarization.e2e.ts to replace legacy UsageMetadata type with a more appropriate type for collectedUsage, improving type safety and clarity in the test setup. These changes improve the robustness of the summarization tests by validating context token constraints and refining type definitions. feat(summarization): add comprehensive E2E tests for summarization process - Introduced a new summarization.e2e.test.ts file that implements extensive end-to-end integration tests for the summarization pipeline, covering the full flow from LibreChat to agents. - The tests utilize real AI providers and include functionality to track summaries during and between runs. - Added necessary cleanup steps to manage Redis connections post-tests and ensure proper exit. These changes enhance the testing framework by providing robust coverage for the summarization process, ensuring reliability and performance under real-world conditions. fix(service): import logger from winston configuration - Removed the import statement for logger from '@librechat/data-schemas' and replaced it with an import from '~/config/winston'. - This change ensures that the logger is correctly sourced from the updated configuration, improving consistency in logging practices across the application. refactor(summary): simplify Summary component and enhance token display - Removed the unused `meta` prop from the `SummaryButton` component to streamline its interface. - Updated the token display logic to use a localized string for better internationalization support. - Adjusted the rendering of the `meta` information to improve its visibility within the `Summary` component. These changes enhance the clarity and usability of the Summary component while ensuring better localization practices. feat(summarization): add maxInputTokens configuration for summarization - Introduced a new `maxInputTokens` property in the summarization configuration schema to control the amount of conversation context sent to the summarizer, with a default value of 10000. - Updated the `createRun` function to utilize the new `maxInputTokens` setting, allowing for more flexible summarization based on agent context. These changes enhance the summarization capabilities by providing better control over input token limits, improving the overall summarization process. refactor(summarization): simplify maxInputTokens logic in createRun function - Updated the logic for the `maxInputTokens` property in the `createRun` function to directly use the agent's base context tokens when the resolved summarization configuration does not specify a value. - This change streamlines the configuration process and enhances clarity in how input token limits are determined for summarization. These modifications improve the maintainability of the summarization configuration by reducing complexity in the token calculation logic. feat(summary): enhance Summary component to display meta information - Updated the SummaryContent component to accept an optional `meta` prop, allowing for additional contextual information to be displayed above the main content. - Adjusted the rendering logic in the Summary component to utilize the new `meta` prop, improving the visibility of supplementary details. These changes enhance the user experience by providing more context within the Summary component, making it clearer and more informative. refactor(summarization): standardize reserveRatio configuration in summarization logic - Replaced instances of `reserveTokensRatio` with `reserveRatio` in the `createRun` function and related tests to unify the terminology across the codebase. - Updated the summarization configuration schema to reflect this change, ensuring consistency in how the reserve ratio is defined and utilized. - Removed the per-agent override logic for summarization configuration, simplifying the overall structure and enhancing clarity. These modifications improve the maintainability and readability of the summarization logic by standardizing the configuration parameters. * fix: circular dependency of `~/models` * chore: update logging scope in agent log handlers Changed log scope from `[agentus:${data.scope}]` to `[agents:${data.scope}]` in both the callbacks and responses controllers to ensure consistent logging format across the application. * feat: calibration ratio * refactor(tests): update summarizationConfig tests to reflect changes in enabled property Modified tests to check for the new `summarizationEnabled` property instead of the deprecated `enabled` field in the summarization configuration. This change ensures that the tests accurately validate the current configuration structure and behavior of the agents. * feat(tests): add markSummarizationUsage mock for improved test coverage Introduced a mock for the markSummarizationUsage function in the responses unit tests to enhance the testing of summarization usage tracking. This addition supports better validation of summarization-related functionalities and ensures comprehensive test coverage for the agents' response handling. * refactor(tests): simplify event handler setup in createResponse tests Removed redundant mock implementations for event handlers in the createResponse unit tests, streamlining the setup process. This change enhances test clarity and maintainability while ensuring that the tests continue to validate the correct behavior of usage tracking during on_chat_model_end events. * refactor(agents): move calibration ratio capture to finally block Reorganized the logic for capturing the calibration ratio in the AgentClient class to ensure it is executed in the finally block. This change guarantees that the ratio is captured even if the run is aborted, enhancing the reliability of the response message persistence. Removed redundant code and improved clarity in the handling of context metadata. * refactor(agents): streamline bulk write logic in recordCollectedUsage function Removed redundant bulk write operations and consolidated document handling in the recordCollectedUsage function. The logic now combines all documents into a single bulk write operation, improving efficiency and reducing error handling complexity. Updated logging to provide consistent error messages for bulk write failures. * refactor(agents): enhance summarization configuration resolution in createRun function Streamlined the summarization configuration logic by introducing a base configuration and allowing for overrides from agent-specific settings. This change improves clarity and maintainability, ensuring that the summarization configuration is consistently applied while retaining flexibility for customization. Updated the handling of summarization parameters to ensure proper integration with the agent's model and provider settings. * refactor(agents): remove unused tokenCountMap and streamline calibration ratio handling Eliminated the unused tokenCountMap variable from the AgentClient class to enhance code clarity. Additionally, streamlined the logic for capturing the calibration ratio by using optional chaining and a fallback value, ensuring that context metadata is consistently defined. This change improves maintainability and reduces potential confusion in the codebase. * refactor(agents): extract agent log handler for improved clarity and reusability Refactored the agent log handling logic by extracting it into a dedicated function, `agentLogHandler`, enhancing code clarity and reusability across different modules. Updated the event handlers in both the OpenAI and responses controllers to utilize the new handler, ensuring consistent logging behavior throughout the application. * test: add summarization event tests for useStepHandler Implemented a series of tests for the summarization events in the useStepHandler hook. The tests cover scenarios for ON_SUMMARIZE_START, ON_SUMMARIZE_DELTA, and ON_SUMMARIZE_COMPLETE events, ensuring proper handling of summarization logic, including message accumulation and finalization. This addition enhances test coverage and validates the correct behavior of the summarization process within the application. * refactor(config): update summarizationTriggerSchema to use enum for type validation Changed the type of the `type` field in the summarizationTriggerSchema from a string to an enum with a single value 'token_count'. This modification enhances type safety and ensures that only valid types are accepted in the configuration, improving overall clarity and maintainability of the schema. * test(usage): add bulk write tests for message and summarization usage Implemented tests for the bulk write functionality in the recordCollectedUsage function, covering scenarios for combined message and summarization usage, summarization-only usage, and message-only usage. These tests ensure correct document handling and token rollup calculations, enhancing test coverage and validating the behavior of the usage tracking logic. * refactor(Chat): enhance clipboard copy functionality and type definitions in Summary component Updated the Summary component to improve the clipboard copy functionality by handling clipboard permission errors. Refactored type definitions for SummaryProps to use a more specific type, enhancing type safety. Adjusted the SummaryButton and FloatingSummaryBar components to accept isCopied and onCopy props, promoting better separation of concerns and reusability. * chore(translations): remove unused "Expand Summary" key from English translations Deleted the "Expand Summary" key from the English translation file to streamline the localization resources and improve clarity in the user interface. This change helps maintain an organized and efficient translation structure. * refactor: adjust token counting for Claude model to account for API discrepancies Implemented a correction factor for token counting when using the Claude model, addressing discrepancies between Anthropic's API and local tokenizer results. This change ensures accurate token counts by applying a scaling factor, improving the reliability of token-related functionalities. * refactor(agents): implement token count adjustment for Claude model messages Added a method to adjust token counts for messages processed by the Claude model, applying a correction factor to align with API expectations. This enhancement improves the accuracy of token counting, ensuring reliable functionality when interacting with the Claude model. * refactor(agents): token counting for media content in messages Introduced a new method to estimate token costs for image and document blocks in messages, improving the accuracy of token counting. This enhancement ensures that media content is properly accounted for, particularly for the Claude model, by integrating additional token estimation logic for various content types. Updated the token counting function to utilize this new method, enhancing overall reliability and functionality. * chore: fix missing import * fix(agents): clamp baseContextTokens and document reserve ratio change Prevent negative baseContextTokens when maxOutputTokens exceeds the context window (misconfigured models). Document the 10%→5% default reserve ratio reduction introduced alongside summarization. * fix(agents): include media tokens in hydrated token counts Add estimateMediaTokensForMessage to createTokenCounter so the hydration path (used by hydrateMissingIndexTokenCounts) matches the precomputed path in AgentClient.getTokenCountForMessage. Without this, messages containing images or documents were systematically undercounted during hydration, risking context window overflow. Add 34 unit tests covering all block-type branches of estimateMediaTokensForMessage. * fix(agents): include summarization output tokens in usage return value The returned output_tokens from recordCollectedUsage now reflects all billed LLM calls (message + summarization). Previously, summarization completions were billed but excluded from the returned metadata, causing a discrepancy between what users were charged and what the response message reported. * fix(tests): replace process.exit with proper Redis cleanup in e2e test The summarization E2E test used process.exit(0) to work around a Redis connection opened at import time, which killed the Jest runner and bypassed teardown. Use ioredisClient.quit() and keyvRedisClient.disconnect() for graceful cleanup instead. * fix(tests): update getConvo imports in OpenAI and response tests Refactor test files to import getConvo from the main models module instead of the Conversation submodule. This change ensures consistency across tests and simplifies the import structure, enhancing maintainability. * fix(clients): improve summary text validation in BaseClient Refactor the summary extraction logic to ensure that only non-empty summary texts are considered valid. This change enhances the robustness of the message processing by utilizing a dedicated method for summary text retrieval, improving overall reliability. * fix(config): replace z.any() with explicit union in summarization schema Model parameters (temperature, top_p, etc.) are constrained to primitive types rather than the policy-violating z.any(). * refactor(agents): deduplicate CLAUDE_TOKEN_CORRECTION constant Export from the TS source in packages/api and import in the JS client, eliminating the static class property that could drift out of sync. * refactor(agents): eliminate duplicate selfProvider in buildAgentContext selfProvider and provider were derived from the same expression with different type casts. Consolidated to a single provider variable. * refactor(agents): extract shared SSE handlers and restrict log levels - buildSummarizationHandlers() factory replaces triplicated handler blocks across responses.js and openai.js - agentLogHandlerObj exported from callbacks.js for consistent reuse - agentLogHandler restricted to an allowlist of safe log levels (debug, info, warn, error) instead of accepting arbitrary strings * fix(SSE): batch summarize deltas, add exhaustiveness check, conditional error announcement - ON_SUMMARIZE_DELTA coalesces rapid-fire renders via requestAnimationFrame instead of calling setMessages per chunk - Exhaustive never-check on TStepEvent catches unhandled variants at compile time when new StepEvents are added - ON_SUMMARIZE_COMPLETE error announcement only fires when a summary part was actually present and removed * feat(agents): persist instruction overhead in contextMeta and seed across runs Extend contextMeta with instructionOverhead and toolCount so the provider-observed instruction overhead is persisted on the response message and seeded into the pruner on subsequent runs. This enables the pruner to use a calibrated budget from the first call instead of waiting for a provider observation, preventing the ratio collapse caused by local tokenizer overestimating tool schema tokens. The seeded overhead is only used when encoding and tool count match between runs, ensuring stale values from different configurations are discarded. * test(agents): enhance OpenAI test mocks for summarization handlers Updated the OpenAI test suite to include additional mock implementations for summarization handlers, including buildSummarizationHandlers, markSummarizationUsage, and agentLogHandlerObj. This improves test coverage and ensures consistent behavior during testing. * fix(agents): address review findings for summarization v2 Cancel rAF on unmount to prevent stale Recoil writes from dead component context. Clear orphaned summarizing:true parts when ON_SUMMARIZE_COMPLETE arrives without a summary payload. Add null guard and safe spread to agentLogHandler. Handle Anthropic-format base64 image/* documents in estimateMediaTokensForMessage. Use role="region" for expandable summary content. Add .describe() to contextMeta Zod fields. Extract duplicate usage loop into helper. * refactor: simplify contextMeta to calibrationRatio + encoding only Remove instructionOverhead and toolCount from cross-run persistence — instruction tokens change too frequently between runs (prompt edits, tool changes) for a persisted seed to be reliable. The intra-run calibration in the pruner still self-corrects via provider observations. contextMeta now stores only the tokenizer-bias ratio and encoding, which are stable across instruction changes. * test(SSE): enhance useStepHandler tests for ON_SUMMARIZE_COMPLETE behavior Updated the test for ON_SUMMARIZE_COMPLETE to clarify that it finalizes the existing part with summarizing set to false when the summary is undefined. Added assertions to verify the correct behavior of message updates and the state of summary parts. * refactor(BaseClient): remove handleContextStrategy and truncateToolCallOutputs functions Eliminated the handleContextStrategy method from BaseClient to streamline message handling. Also removed the truncateToolCallOutputs function from the prompts module, simplifying the codebase and improving maintainability. * refactor: add AGENT_DEBUG_LOGGING option and refactor token count handling in BaseClient Introduced AGENT_DEBUG_LOGGING to .env.example for enhanced debugging capabilities. Refactored token count handling in BaseClient by removing the handleTokenCountMap method and simplifying token count updates. Updated AgentClient to log detailed token count recalculations and adjustments, improving traceability during message processing. * chore: update dependencies in package-lock.json and package.json files Bumped versions of several dependencies, including @librechat/agents to ^3.1.62 and various AWS SDK packages to their latest versions. This ensures compatibility and incorporates the latest features and fixes. * chore: imports order * refactor: extract summarization config resolution from buildAgentContext * refactor: rename and simplify summarization configuration shaping function * refactor: replace AgentClient token counting methods with single-pass pure utility Extract getTokenCount() and getTokenCountForMessage() from AgentClient into countFormattedMessageTokens(), a pure function in packages/api that handles text, tool_call, image, and document content types in one loop. - Decompose estimateMediaTokensForMessage into block-level helpers (estimateImageDataTokens, estimateImageBlockTokens, estimateDocumentBlockTokens) shared by both estimateMediaTokensForMessage and the new single-pass function - Remove redundant per-call getEncoding() resolution (closure captures once) - Remove deprecated gpt-3.5-turbo-0301 model branching - Drop this.getTokenCount guard from BaseClient.sendMessage * refactor: streamline token counting in createTokenCounter function Simplified the createTokenCounter function by removing the media token estimation and directly calculating the token count. This change enhances clarity and performance by consolidating the token counting logic into a single pass, while maintaining compatibility with Claude's token correction. * refactor: simplify summarization configuration types Removed the AppSummarizationConfig type and directly used SummarizationConfig in the AppConfig interface. This change streamlines the type definitions and enhances consistency across the codebase. * chore: import order * fix: summarization event handling in useStepHandler - Cancel pending summarizeDeltaRaf in clearStepMaps to prevent stale frames firing after map reset or component unmount - Move announcePolite('summarize_completed') inside the didFinalize guard so screen readers only announce when finalization actually occurs - Remove dead cleanup closure returned from stepHandler useCallback body that was never invoked by any caller * fix: estimate tokens for non-PDF/non-image base64 document blocks Previously estimateDocumentBlockTokens returned 0 for unrecognized MIME types (e.g. text/plain, application/json), silently underestimating context budget. Fall back to character-based heuristic or countTokens. * refactor: return cloned usage from markSummarizationUsage Avoid mutating LangChain's internal usage_metadata object by returning a shallow clone with the usage_type tag. Update all call sites in callbacks, openai, and responses controllers to use the returned value. * refactor: consolidate debug logging loops in buildMessages Merge the two sequential O(n) debug-logging passes over orderedMessages into a single pass inside the map callback where all data is available. * refactor: narrow SummaryContentPart.content type Replace broad Agents.MessageContentComplex[] with the specific Array<{ type: ContentTypes.TEXT; text: string }> that all producers and consumers already use, improving compile-time safety. * refactor: use single output array in recordCollectedUsage Have processUsageGroup append to a shared array instead of returning separate arrays that are spread into a third, reducing allocations. * refactor: use for...in in hydrateMissingIndexTokenCounts Replace Object.entries with for...in to avoid allocating an intermediate tuple array during token map hydration.	2026-03-21 14:28:56 -04:00
Danny Avila	67db0c1cb3	🗑️ chore: Remove Action Test Suite and Update Mock Implementations (#12268 ) - Deleted the Action test suite located in `api/models/Action.spec.js` to streamline the codebase. - Updated various test files to reflect changes in model mocks, consolidating mock implementations for user-related actions and enhancing clarity. - Improved consistency in test setups by aligning with the latest model updates and removing redundant mock definitions.	2026-03-21 14:28:55 -04:00
Danny Avila	dd72b7b17e	🔄 chore: Consolidate agent model imports across middleware and tests from rebase - Updated imports for `createAgent` and `getAgent` to streamline access from a unified `~/models` path. - Enhanced test files to reflect the new import structure, ensuring consistency and maintainability across the codebase. - Improved clarity by removing redundant imports and aligning with the latest model updates.	2026-03-21 14:28:55 -04:00
Atef Bellaaj	a0fed6173c	🗂️ refactor: Migrate S3 Storage to TypeScript in packages/api (#11947 ) * Migrate S3 storage module with unit and integration tests - Migrate S3 CRUD and image operations to packages/api/src/storage/s3/ - Add S3ImageService class with dependency injection - Add unit tests using aws-sdk-client-mock - Add integration tests with real s3 bucket (condition presence of AWS_TEST_BUCKET_NAME) * AI Review Findings Fixes * chore: tests and refactor S3 storage types - Added mock implementations for the 'sharp' library in various test files to improve image processing testing. - Updated type references in S3 storage files from MongoFile to TFile for consistency and type safety. - Refactored S3 CRUD operations to ensure proper handling of file types and improve code clarity. - Enhanced integration tests to validate S3 file operations and error handling more effectively. * chore: rename test file * Remove duplicate import of refreshS3Url * chore: imports order * fix: remove duplicate imports for S3 URL handling in UserController * fix: remove duplicate import of refreshS3FileUrls in files.js * test: Add mock implementations for 'sharp' and '@librechat/api' in UserController tests - Introduced mock functions for the 'sharp' library to facilitate image processing tests, including metadata retrieval and buffer conversion. - Enhanced mocking for '@librechat/api' to ensure consistent behavior in tests, particularly for the needsRefresh and getNewS3URL functions. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-03-21 14:28:55 -04:00
Danny Avila	9e0592a236	📜 feat: Implement System Grants for Capability-Based Authorization (#11896 ) * feat: Implement System Grants for Role-Based Capabilities - Added a new `systemGrant` model and associated methods to manage role-based capabilities within the application. - Introduced middleware functions `hasCapability` and `requireCapability` to check user permissions based on their roles. - Updated the database seeding process to include system grants for the ADMIN role, ensuring all necessary capabilities are assigned on startup. - Enhanced type definitions and schemas to support the new system grant functionality, improving overall type safety and clarity in the codebase. * test: Add unit tests for capabilities middleware and system grant methods - Introduced comprehensive unit tests for the capabilities middleware, including `hasCapability` and `requireCapability`, ensuring proper permission checks based on user roles. - Added tests for the `SystemGrant` methods, verifying the seeding of system grants, capability granting, and revocation processes. - Enhanced test coverage for edge cases, including idempotency of grant operations and handling of unexpected errors in middleware. - Utilized mocks for database interactions to isolate tests and improve reliability. * refactor: Transition to Capability-Based Access Control - Replaced role-based access checks with capability-based checks across various middleware and routes, enhancing permission management. - Introduced `hasCapability` and `requireCapability` functions to streamline capability verification for user actions. - Updated relevant routes and middleware to utilize the new capability system, ensuring consistent permission enforcement. - Enhanced type definitions and added tests for the new capability functions, improving overall code reliability and maintainability. * test: Enhance capability-based access tests for ADMIN role - Updated tests to reflect the new capability-based access control, specifically for the ADMIN role. - Modified test descriptions to clarify that users with the MANAGE_AGENTS capability can bypass permission checks. - Seeded capabilities for the ADMIN role in multiple test files to ensure consistent permission checks across different routes and middleware. - Improved overall test coverage for capability verification, ensuring robust permission management. * test: Update capability tests for MCP server access - Renamed test to reflect the correct capability for bypassing permission checks, changing from MANAGE_AGENTS to MANAGE_MCP_SERVERS. - Updated seeding of capabilities for the ADMIN role to align with the new capability structure. - Ensured consistency in capability definitions across tests and middleware for improved permission management. * feat: Add hasConfigCapability for enhanced config access control - Introduced `hasConfigCapability` function to check user permissions for managing or reading specific config sections. - Updated middleware to export the new capability function, ensuring consistent access control across the application. - Enhanced unit tests to cover various scenarios for the new capability, improving overall test coverage and reliability. * fix: Update tenantId filter in createSystemGrantMethods - Added a condition to set tenantId filter to { $exists: false } when tenantId is null, ensuring proper handling of cases where tenantId is not provided. - This change improves the robustness of the system grant methods by explicitly managing the absence of tenantId in the filter logic. * fix: account deletion capability check - Updated the `canDeleteAccount` middleware to ensure that the `hasManageUsers` capability check only occurs if a user is present, preventing potential errors when the user object is undefined. - This change improves the robustness of the account deletion logic by ensuring proper handling of user permissions. * refactor: Optimize seeding of system grants for ADMIN role - Replaced sequential capability granting with parallel execution using Promise.all in the seedSystemGrants function. - This change improves performance and efficiency during the initialization of system grants, ensuring all capabilities are granted concurrently. * refactor: Simplify systemGrantSchema index definition - Removed the sparse option from the unique index on principalType, principalId, capability, and tenantId in the systemGrantSchema. - This change streamlines the index definition, potentially improving query performance and clarity in the schema design. * refactor: Reorganize role capability check in roles route - Moved the capability check for reading roles to occur after parsing the roleName, improving code clarity and structure. - This change ensures that the authorization logic is consistently applied before fetching role details, enhancing overall permission management. * refactor: Remove unused ISystemGrant interface from systemCapabilities.ts - Deleted the ISystemGrant interface as it was no longer needed, streamlining the code and improving clarity. - This change helps reduce clutter in the file and focuses on relevant capabilities for the system. * refactor: Migrate SystemCapabilities to data-schemas - Replaced imports of SystemCapabilities from 'librechat-data-provider' with imports from '@librechat/data-schemas' across multiple files. - This change centralizes the management of system capabilities, improving code organization and maintainability. * refactor: Update account deletion middleware and capability checks - Modified the `canDeleteAccount` middleware to ensure that the account deletion permission is only granted to users with the `MANAGE_USERS` capability, improving security and clarity in permission management. - Enhanced error logging for unauthorized account deletion attempts, providing better insights into permission issues. - Updated the `capabilities.ts` file to ensure consistent handling of user authentication checks, improving robustness in capability verification. - Refined type definitions in `systemGrant.ts` and `systemGrantMethods.ts` to utilize the `PrincipalType` enum, enhancing type safety and code clarity. * refactor: Extract principal ID normalization into a separate function - Introduced `normalizePrincipalId` function to streamline the normalization of principal IDs based on their type, enhancing code clarity and reusability. - Updated references in `createSystemGrantMethods` to utilize the new normalization function, improving maintainability and reducing code duplication. * test: Add unit tests for principalId normalization in systemGrant - Introduced tests for the `grantCapability`, `revokeCapability`, and `getCapabilitiesForPrincipal` methods to verify correct handling of principalId normalization between string and ObjectId formats. - Enhanced the `capabilities.ts` middleware to utilize the `PrincipalType` enum for improved type safety. - Added a new utility function `normalizePrincipalId` to streamline principal ID normalization logic, ensuring consistent behavior across the application. * feat: Introduce capability implications and enhance system grant methods - Added `CapabilityImplications` to define relationships between broader and implied capabilities, allowing for more intuitive permission checks. - Updated `createSystemGrantMethods` to expand capability queries to include implied capabilities, improving authorization logic. - Enhanced `systemGrantSchema` to include an `expiresAt` field for future TTL enforcement of grants, and added validation to ensure `tenantId` is not set to null. - Documented authorization requirements for prompt group and prompt deletion methods to clarify access control expectations. * test: Add unit tests for canDeleteAccount middleware - Introduced unit tests for the `canDeleteAccount` middleware to verify account deletion permissions based on user roles and capabilities. - Covered scenarios for both allowed and blocked account deletions, including checks for ADMIN users with the `MANAGE_USERS` capability and handling of undefined user cases. - Enhanced test structure to ensure clarity and maintainability of permission checks in the middleware. * fix: Add principalType enum validation to SystemGrant schema Without enum validation, any string value was accepted for principalType and silently stored. Invalid documents would never match capability queries, creating phantom grants impossible to diagnose without raw DB inspection. All other ACL models in the codebase validate this field. * fix: Replace seedSystemGrants Promise.all with bulkWrite for concurrency safety When two server instances start simultaneously (K8s rolling deploy, PM2 cluster), both call seedSystemGrants. With Promise.all + findOneAndUpdate upsert, both instances may attempt to insert the same documents, causing E11000 duplicate key errors that crash server startup. bulkWrite with ordered:false handles concurrent upserts gracefully and reduces 17 individual round trips to a single network call. The returned documents (previously discarded) are no longer fetched. * perf: Add AsyncLocalStorage per-request cache for capability checks Every hasCapability call previously required 2 DB round trips (getUserPrincipals + SystemGrant.exists) — replacing what were O(1) string comparisons. Routes like patchPromptGroup triggered this twice, and hasConfigCapability's fallback path resolved principals twice. This adds a per-request AsyncLocalStorage cache that: - Caches resolved principals (same for all checks within one request) - Caches capability check results (same user+cap = same answer) - Automatically scoped to request lifetime (no stale grants) - Falls through to DB when no store exists (background jobs, tests) - Requires no signature changes to hasCapability The capabilityContextMiddleware is registered at the app level before all routes, initializing a fresh store per request. * fix: Add error handling for inline hasCapability calls canDeleteAccount, fetchAssistants, and validateAuthor all call hasCapability without try-catch. These were previously O(1) string comparisons that could never throw. Now they hit the database and can fail on connection timeout or transient errors. Wrap each call in try-catch, defaulting to deny (false) on error. This ensures a DB hiccup returns a clean 403 instead of an unhandled 500 with a stack trace. * test: Add canDeleteAccount DB-error resilience test Tests that hasCapability rejection (e.g., DB timeout) results in a clean 403 rather than an unhandled exception. Validates the error handling added in the previous commit. * refactor: Use barrel import for hasCapability in validateAuthor Import from ~/server/middleware barrel instead of directly from ~/server/middleware/roles/capabilities for consistency with other non-middleware consumers. Files within the middleware barrel itself must continue using direct imports to avoid circular requires. * refactor: Remove misleading pre('save') hook from SystemGrant schema The pre('save') hook normalized principalId for USER/GROUP principals, but the primary write path (grantCapability) uses findOneAndUpdate — which does not trigger save hooks. The normalization was already handled explicitly in grantCapability itself. The hook created a false impression of schema-level enforcement that only covered save()/create() paths. Replace with a comment documenting that all writes must go through grantCapability. * feat: Add READ_ASSISTANTS capability to complete manage/read pair Every other managed resource had a paired READ_X / MANAGE_X capability except assistants. This adds READ_ASSISTANTS and registers the MANAGE_ASSISTANTS → READ_ASSISTANTS implication in CapabilityImplications, enabling future read-only assistant visibility grants. * chore: Reorder systemGrant methods for clarity Moved hasCapabilityForPrincipals to a more logical position in the returned object of createSystemGrantMethods, improving code readability. This change also maintains the inclusion of seedSystemGrants in the export, ensuring all necessary methods are available. * fix: Wrap seedSystemGrants in try-catch to avoid blocking startup Seeding capabilities is idempotent and will succeed on the next restart. A transient DB error during seeding should not prevent the server from starting — log the error and continue. * refactor: Improve capability check efficiency and add audit logging Move hasCapability calls after cheap early-exits in validateAuthor and fetchAssistants so the DB check only runs when its result matters. Add logger.debug on every capability bypass grant across all 7 call sites for auditability, and log errors in catch blocks instead of silently swallowing them. * test: Add integration tests for AsyncLocalStorage capability caching Exercises the full vertical — ALS context, generateCapabilityCheck, real getUserPrincipals, real hasCapabilityForPrincipals, real MongoDB via MongoMemoryServer. Covers per-request caching, cross-context isolation, concurrent request isolation, negative caching, capability implications, tenant scoping, group-based grants, and requireCapability middleware. * test: Add systemGrant data-layer and ALS edge-case integration tests systemGrant.spec.ts (51 tests): Full integration tests for all systemGrant methods against real MongoDB — grant/revoke lifecycle, principalId normalization (string→ObjectId for USER/GROUP, string for ROLE), capability implications (both directions), tenant scoping, schema validation (null tenantId, invalid enum, required fields, unique compound index). capabilities.integration.spec.ts (27 tests): Adds ALS edge cases — missing context degrades gracefully with no caching (background jobs, child processes), nested middleware creates independent inner context, optional-chaining safety when store is undefined, mid-request grant changes are invisible due to result caching, requireCapability works without ALS, and interleaved concurrent contexts maintain isolation. * fix: Add worker thread guards to capability ALS usage Detect when hasCapability or capabilityContextMiddleware is called from a worker thread (where ALS context does not propagate from the parent). hasCapability logs a warn-once per factory instance; the middleware logs an error since mounting Express middleware in a worker is likely a misconfiguration. Both continue to function correctly — the guard is observability, not a hard block. * fix: Include tenantId in ALS principal cache key for tenant isolation The principal cache key was user.id:user.role, which would reuse cached principals across tenants for the same user within a request. When getUserPrincipals gains tenant-scoped group resolution, principals from tenant-a would incorrectly serve tenant-b checks. Changed to user.id:user.role:user.tenantId to prevent cross-tenant cache hits. Adds integration test proving separate principal lookups per tenantId. * test: Remove redundant mocked capabilities.spec.js The JS wrapper test (7 tests, all mocked) is a strict subset of capabilities.integration.spec.ts (28 tests, real MongoDB). Every scenario it covered — hasCapability true/false, tenantId passthrough, requireCapability 403/500, error handling — is tested with higher fidelity in the integration suite. * test: Replace mocked canDeleteAccount tests with real MongoDB integration Remove hasCapability mock — tests now exercise the full capability chain against real MongoDB (getUserPrincipals, hasCapabilityForPrincipals, SystemGrant collection). Only mocks remaining are logger and cache. Adds new coverage: admin role without grant is blocked, user-level grant bypasses deletion restriction, null user handling. * test: Add comprehensive tests for ACL entry management and user group methods Introduces new tests for `deleteAclEntries`, `bulkWriteAclEntries`, and `findPublicResourceIds` in `aclEntry.spec.ts`, ensuring proper functionality for deleting and bulk managing ACL entries. Additionally, enhances `userGroup.spec.ts` with tests for finding groups by ID and name pattern, including external ID matching and source filtering. These changes improve coverage and validate the integrity of ACL and user group operations against real MongoDB interactions. * refactor: Update capability checks and logging for better clarity and error handling Replaced `MANAGE_USERS` with `ACCESS_ADMIN` in the `canDeleteAccount` middleware and related tests to align with updated permission structure. Enhanced logging in various middleware functions to use `logger.warn` for capability check failures, providing clearer error messages. Additionally, refactored capability checks in the `patchPromptGroup` and `validateAuthor` functions to improve readability and maintainability. This commit also includes adjustments to the `systemGrant` methods to implement retry logic for transient failures during capability seeding, ensuring robustness in the face of database errors. * refactor: Enhance logging and retry logic in seedSystemGrants method Updated the logging format in the seedSystemGrants method to include error messages for better clarity. Improved the retry mechanism by explicitly mocking multiple failures in tests, ensuring robust error handling during transient database issues. Additionally, refined imports in the systemGrant schema for better type management. * refactor: Consolidate imports in canDeleteAccount middleware Merged logger and SystemCapabilities imports from the data-schemas module into a single line for improved readability and maintainability of the code. This change streamlines the import statements in the canDeleteAccount middleware. * test: Enhance systemGrant tests for error handling and capability validation Added tests to the systemGrant methods to handle various error scenarios, including E11000 race conditions, invalid ObjectId strings for USER and GROUP principals, and invalid capability strings. These enhancements improve the robustness of the capability granting and revoking logic, ensuring proper error propagation and validation of inputs. * fix: Wrap hasCapability calls in deny-by-default try-catch at remaining sites canAccessResource, files.js, and roles.js all had hasCapability inside outer try-catch blocks that returned 500 on DB failure instead of falling through to the regular ACL check. This contradicts the deny-by-default pattern used everywhere else. Also removes raw error.message from the roles.js 500 response to prevent internal host/connection info leaking to clients. * fix: Normalize user ID in canDeleteAccount before passing to hasCapability requireCapability normalizes req.user.id via _id?.toString() fallback, but canDeleteAccount passed raw req.user directly. If req.user.id is absent (some auth layers only populate _id), getUserPrincipals received undefined, silently returning empty principals and blocking the bypass. * fix: Harden systemGrant schema and type safety - Reject empty string tenantId in schema validator (was only blocking null; empty string silently orphaned documents) - Fix reverseImplications to use BaseSystemCapability[] instead of string[], preserving the narrow discriminated type - Document READ_ASSISTANTS as reserved/unenforced * test: Use fake timers for seedSystemGrants retry tests and add tenantId validation - Switch retry tests to jest.useFakeTimers() to eliminate 3+ seconds of real setTimeout delays per test run - Add regression test for empty-string tenantId rejection * docs: Add TODO(#12091) comments for tenant-scoped capability gaps In multi-tenant mode, platform-level grants (no tenantId) won't match tenant-scoped queries, breaking admin access. getUserPrincipals also returns cross-tenant group memberships. Both need fixes in #12091.	2026-03-21 14:28:54 -04:00
Danny Avila	8ba2bde5c1	📦 refactor: Consolidate DB models, encapsulating Mongoose usage in `data-schemas` (#11830 ) * chore: move database model methods to /packages/data-schemas * chore: add TypeScript ESLint rule to warn on unused variables * refactor: model imports to streamline access - Consolidated model imports across various files to improve code organization and reduce redundancy. - Updated imports for models such as Assistant, Message, Conversation, and others to a unified import path. - Adjusted middleware and service files to reflect the new import structure, ensuring functionality remains intact. - Enhanced test files to align with the new import paths, maintaining test coverage and integrity. * chore: migrate database models to packages/data-schemas and refactor all direct Mongoose Model usage outside of data-schemas * test: update agent model mocks in unit tests - Added `getAgent` mock to `client.test.js` to enhance test coverage for agent-related functionality. - Removed redundant `getAgent` and `getAgents` mocks from `openai.spec.js` and `responses.unit.spec.js` to streamline test setup and reduce duplication. - Ensured consistency in agent mock implementations across test files. * fix: update types in data-schemas * refactor: enhance type definitions in transaction and spending methods - Updated type definitions in `checkBalance.ts` to use specific request and response types. - Refined `spendTokens.ts` to utilize a new `SpendTxData` interface for better clarity and type safety. - Improved transaction handling in `transaction.ts` by introducing `TransactionResult` and `TxData` interfaces, ensuring consistent data structures across methods. - Adjusted unit tests in `transaction.spec.ts` to accommodate new type definitions and enhance robustness. * refactor: streamline model imports and enhance code organization - Consolidated model imports across various controllers and services to a unified import path, improving code clarity and reducing redundancy. - Updated multiple files to reflect the new import structure, ensuring all functionalities remain intact. - Enhanced overall code organization by removing duplicate import statements and optimizing the usage of model methods. * feat: implement loadAddedAgent and refactor agent loading logic - Introduced `loadAddedAgent` function to handle loading agents from added conversations, supporting multi-convo parallel execution. - Created a new `load.ts` file to encapsulate agent loading functionalities, including `loadEphemeralAgent` and `loadAgent`. - Updated the `index.ts` file to export the new `load` module instead of the deprecated `loadAgent`. - Enhanced type definitions and improved error handling in the agent loading process. - Adjusted unit tests to reflect changes in the agent loading structure and ensure comprehensive coverage. * refactor: enhance balance handling with new update interface - Introduced `IBalanceUpdate` interface to streamline balance update operations across the codebase. - Updated `upsertBalanceFields` method signatures in `balance.ts`, `transaction.ts`, and related tests to utilize the new interface for improved type safety. - Adjusted type imports in `balance.spec.ts` to include `IBalanceUpdate`, ensuring consistency in balance management functionalities. - Enhanced overall code clarity and maintainability by refining type definitions related to balance operations. * feat: add unit tests for loadAgent functionality and enhance agent loading logic - Introduced comprehensive unit tests for the `loadAgent` function, covering various scenarios including null and empty agent IDs, loading of ephemeral agents, and permission checks. - Enhanced the `initializeClient` function by moving `getConvoFiles` to the correct position in the database method exports, ensuring proper functionality. - Improved test coverage for agent loading, including handling of non-existent agents and user permissions. * chore: reorder memory method exports for consistency - Moved `deleteAllUserMemories` to the correct position in the exported memory methods, ensuring a consistent and logical order of method exports in `memory.ts`.	2026-03-21 14:28:53 -04:00
Danny Avila	58f128bee7	🗑️ chore: Remove Deprecated Project Model and Associated Fields (#11773 ) * chore: remove projects and projectIds usage * chore: empty line linting * chore: remove isCollaborative property across agent models and related tests - Removed the isCollaborative property from agent models, controllers, and tests, as it is deprecated in favor of ACL permissions. - Updated related validation schemas and data provider types to reflect this change. - Ensured all references to isCollaborative were stripped from the codebase to maintain consistency and clarity.	2026-03-21 14:28:53 -04:00
Danny Avila	365a0dc0f6	🩺 refactor: Surface Descriptive OCR Error Messages to Client (#12344 ) * fix: pass along error message when OCR fails Right now, if OCR fails, it just says "Error processing file" which isn't very helpful. The `error.message` does has helpful information in it, but our filter wasn't including the right case to pass it along. Now it does! * fix: extract shared upload error filter, apply to images route The 'Unable to extract text from' error was only allowlisted in the files route but not the images route, which also calls processAgentFileUpload. Extract the duplicated error filter logic into a shared resolveUploadErrorMessage utility in packages/api so both routes stay in sync. --------- Co-authored-by: Dan Lew <daniel@mightyacorn.com>	2026-03-20 17:10:25 -04:00
Danny Avila	9a64791e3e	🪢 fix: Action Domain Encoding Collision for HTTPS URLs (#12271 ) * fix: strip protocol from domain before encoding in `domainParser` All https:// (and http://) domains produced the same 10-char base64 prefix due to ENCODED_DOMAIN_LENGTH truncation, causing tool name collisions for agents with multiple actions. Strip the protocol before encoding so the base64 key is derived from the hostname. Add `legacyDomainEncode` to preserve the old encoding logic for backward-compatible matching of existing stored actions. * fix: backward-compatible tool matching in ToolService Update `getActionToolDefinitions` to match stored tools against both new and legacy domain encodings. Update `loadActionToolsForExecution` to resolve model-called tool names via a `normalizedToDomain` map that includes both encoding variants, with legacy fallback for request builder lookup. * fix: action route save/delete domain encoding issues Save routes now remove old tools matching either new or legacy domain encoding, preventing stale entries when an action's encoding changes on update. Delete routes no longer re-encode the already-encoded domain extracted from the stored actions array, which was producing incorrect keys and leaving orphaned tools. * test: comprehensive coverage for action domain encoding Rewrite ActionService tests to cover real matching patterns used by ToolService and action routes. Tests verify encode/decode round-trips, protocol stripping, backward-compatible tool name matching at both definition and execution phases, save-route cleanup of old/new encodings, delete-route domain extraction, and the collision fix for multi-action agents. * fix: add legacy domain compat to all execution paths, make legacyDomainEncode sync CRITICAL: processRequiredActions (assistants path) was not updated with legacy domain matching — existing assistants with https:// domain actions would silently fail post-deployment because domainMap only had new encoding. MAJOR: loadAgentTools definitionsOnly=false path had the same issue. Both now use a normalizedToDomain map with legacy+new entries and extract function names via the matched key (not the canonical domain). Also: make legacyDomainEncode synchronous (no async operations), store legacyNormalized in processedActionSets to eliminate recomputation in the per-tool fallback, and hoist domainSeparatorRegex to module level. * refactor: clarify domain variable naming and tool-filter helpers in action routes Rename shadowed 'domain' to 'encodedDomain' to separate raw URL from encoded key in both agent and assistant save routes. Rename shouldRemoveTool to shouldRemoveAgentTool / shouldRemoveAssistantTool to make the distinct data-shape guards explicit. Remove await on now-synchronous legacyDomainEncode. * test: expand coverage for all review findings - Add validateAndUpdateTool tests (protocol-stripping match logic) - Restore unicode domain encode/decode/round-trip tests - Add processRequiredActions matching pattern tests (assistants path) - Add legacy guard skip test for short bare hostnames - Add pre-normalized Set test for definition-phase optimization - Fix corrupt-cache test to assert typeof instead of toBeDefined - Verify legacyDomainEncode is synchronous (not a Promise) - Remove all await on legacyDomainEncode (now sync) 58 tests, up from 44. * fix: address follow-up review findings A-E A: Fix stale JSDoc @returns {Promise<string>} on now-synchronous legacyDomainEncode — changed to @returns {string}. B: Rename normalizedToDomain to domainLookupMap in processRequiredActions and loadAgentTools where keys are raw encoded domains (not normalized), avoiding confusion with loadActionToolsForExecution where keys ARE normalized. C: Pre-normalize actionToolNames into a Set<string> in getActionToolDefinitions, replacing O(signatures × tools) per-check .some() + .replace() with O(1) Set.has() lookups. D: Remove stripProtocol from ActionService exports — it is a one-line internal helper. Spec tests for it removed; behavior is fully covered by domainParser protocol-stripping tests. E: Fix pre-existing bug where processRequiredActions re-loaded action sets on every missing-tool iteration. The guard !actionSets.length always re-triggered because actionSets was reassigned to a plain object (whose .length is undefined). Replaced with a null-check on a dedicated actionSetsData variable. * fix: strip path and query from domain URLs in stripProtocol URLs like 'https://api.example.com/v1/endpoint?foo=bar' previously retained the path after protocol stripping, contaminating the encoded domain key. Now strips everything after the first '/' following the host, using string indexing instead of URL parsing to avoid punycode normalization of unicode hostnames. Closes Copilot review comments 1, 2, and 5.	2026-03-17 01:38:51 -04:00
Danny Avila	0c27ad2d55	🛡️ refactor: Scope Action Mutations by Parent Resource Ownership (#12237 ) * 🛡️ fix: Scope action mutations by parent resource ownership Prevent cross-tenant action overwrites by validating that an existing action's agent_id/assistant_id matches the URL parameter before allowing updates or deletes. Without this, a user with EDIT access on their own agent could reference a foreign action_id to hijack another agent's action record. * 🛡️ fix: Harden action ownership checks and scope write filters - Remove && short-circuit that bypassed the guard when agent_id or assistant_id was falsy (e.g. assistant-owned actions have no agent_id, so the check was skipped entirely on the agents route). - Include agent_id / assistant_id in the updateAction and deleteAction query filters so the DB write itself enforces ownership atomically. - Log a warning when deleteAction returns null (silent no-op from data-integrity mismatch). * 📝 docs: Update Action model JSDoc to reflect scoped query params * ✅ test: Add Action ownership scoping tests Cover update, delete, and cross-type protection scenarios using MongoMemoryServer to verify that scoped query filters (agent_id, assistant_id) prevent cross-tenant overwrites and deletions at the database level. * 🛡️ fix: Scope updateAction filter in agent duplication handler * 🐛 fix: Use action metadata domain instead of action_id when duplicating agent actions The duplicate handler was splitting `action.action_id` by `actionDelimiter` to extract the domain, but `action_id` is a bare nanoid that doesn't contain the delimiter. This produced malformed entries in the duplicated agent's actions array (nanoid_action_newNanoid instead of domain_action_newNanoid). The domain is available on `action.metadata.domain`. * ✅ test: Add integration tests for agent duplication action handling Uses MongoMemoryServer with real Agent and Action models to verify: - Duplicated actions use metadata.domain (not action_id) for the agent actions array entries - Sensitive metadata fields are stripped from duplicated actions - Original action documents are not modified	2026-03-15 10:19:29 -04:00
Danny Avila	7bc793b18d	🌊 fix: Prevent Buffered Event Duplication on SSE Resume Connections (#12225 ) * fix: skipBufferReplay for job resume connections - Introduced a new option `skipBufferReplay` in the `subscribe` method of `GenerationJobManagerClass` to prevent duplication of events when resuming a connection. - Updated the logic to conditionally skip replaying buffered events if a sync event has already been sent, enhancing the efficiency of event handling during reconnections. - Added integration tests to verify the correct behavior of the new option, ensuring that no buffered events are replayed when `skipBufferReplay` is true, while still allowing for normal replay behavior when false. * refactor: Update GenerationJobManager to handle sync events more efficiently - Modified the `subscribe` method to utilize a new `skipBufferReplay` option, allowing for the prevention of duplicate events during resume connections. - Enhanced the logic in the `chat/stream` route to conditionally skip replaying buffered events if a sync event has already been sent, improving event handling efficiency. - Updated integration tests to verify the correct behavior of the new option, ensuring that no buffered events are replayed when `skipBufferReplay` is true, while maintaining normal replay behavior when false. * test: Enhance GenerationJobManager integration tests for Redis mode - Updated integration tests to conditionally run based on the USE_REDIS environment variable, allowing for better control over Redis-related tests. - Refactored test descriptions to utilize a dynamic `describeRedis` function, improving clarity and organization of tests related to Redis functionality. - Removed redundant checks for Redis availability within individual tests, streamlining the test logic and enhancing readability. * fix: sync handler state for new messages on resume The sync event's else branch (new response message) was missing resetContentHandler() and syncStepMessage() calls, leaving stale handler state that caused subsequent deltas to build on partial content instead of the synced aggregatedContent. * feat: atomic subscribeWithResume to close resume event gap Replaces separate getResumeState() + subscribe() calls with a single subscribeWithResume() that atomically drains earlyEventBuffer between the resume snapshot and the subscribe. In in-memory mode, drained events are returned as pendingEvents for the client to replay after sync. In Redis mode, pendingEvents is empty since chunks are already persisted. The route handler now uses the atomic method for resume connections and extracted shared SSE write helpers to reduce duplication. The client replays any pendingEvents through the existing step/content handlers after applying aggregatedContent from the sync payload. * fix: only capture gap events in subscribeWithResume, not pre-snapshot buffer The previous implementation drained the entire earlyEventBuffer into pendingEvents, but pre-snapshot events are already reflected in aggregatedContent. Replaying them re-introduced the duplication bug through a different vector. Now records buffer length before getResumeState() and slices from that index, so only events arriving during the async gap are returned as pendingEvents. Also: - Handle pendingEvents when resumeState is null (replay directly) - Hoist duplicate test helpers to shared scope - Remove redundant writableEnded guard in onDone	2026-03-14 10:54:26 -04:00
Danny Avila	35a35dc2e9	📏 refactor: Add File Size Limits to Conversation Imports (#12221 ) * fix: add file size limits to conversation import multer instance * fix: address review findings for conversation import file size limits * fix: use local jest.mock for data-schemas instead of global moduleNameMapper The global @librechat/data-schemas mock in jest.config.js only provided logger, breaking all tests that depend on createModels from the same package. Replace with a virtual jest.mock scoped to the import spec file. * fix: move import to top of file, pre-compute upload middleware, assert logger.warn in tests * refactor: move resolveImportMaxFileSize to packages/api New backend logic belongs in packages/api as TypeScript. Delete the api/server/utils/import/limits.js wrapper and import directly from @librechat/api in convos.js and importConversations.js. Resolver unit tests move to packages/api; the api/ spec retains only multer behavior tests. * chore: rename importLimits to import * fix: stale type reference and mock isolation in import tests Update typeof import path from '../importLimits' to '../import' after the rename. Clear mockLogger.warn in beforeEach to prevent cross-test accumulation. * fix: add resolveImportMaxFileSize to @librechat/api mock in convos.spec.js * fix: resolve jest.mock hoisting issue in import tests jest.mock factories are hoisted above const declarations, so the mockLogger reference was undefined at factory evaluation time. Use a direct import of the mocked logger module instead. * fix: remove virtual flag from data-schemas mock for CI compatibility virtual: true prevents the mock from intercepting the real module in CI where @librechat/data-schemas is built, causing import.ts to use the real logger while the test asserts against the mock.	2026-03-14 03:06:29 -04:00
Danny Avila	c6982dc180	🛡️ fix: Agent Permission Check on Image Upload Route (#12219 ) * fix: add agent permission check to image upload route * refactor: remove unused SystemRoles import and format test file for clarity * fix: address review findings for image upload agent permission check * refactor: move agent upload auth logic to TypeScript in packages/api Extract pure authorization logic from agentPermCheck.js into checkAgentUploadAuth() in packages/api/src/files/agentUploadAuth.ts. The function returns a structured result ({ allowed, status, error }) instead of writing HTTP responses directly, eliminating the dual responsibility and confusing sentinel return value. The JS wrapper in /api is now a thin adapter that translates the result to HTTP. * test: rewrite image upload permission tests as integration tests Replace mock-heavy images-agent-perm.spec.js with integration tests using MongoMemoryServer, real models, and real PermissionService. Follows the established pattern in files.agents.test.js. Moves test to sibling location (images.agents.test.js) matching backend convention. Adds temp file cleanup assertions on 403/404 responses and covers message_file exemption paths (boolean true, string "true", false). * fix: widen AgentUploadAuthDeps types to accept ObjectId from Mongoose The injected getAgent returns Mongoose documents where _id and author are Types.ObjectId at runtime, not string. Widen the DI interface to accept string \| Types.ObjectId for _id, author, and resourceId so the contract accurately reflects real callers. * chore: move agent upload auth into files/agents/ subdirectory * refactor: delete agentPermCheck.js wrapper, move verifyAgentUploadPermission to packages/api The /api-only dependencies (getAgent, checkPermission) are now passed as object-field params from the route call sites. Both images.js and files.js import verifyAgentUploadPermission from @librechat/api and inject the deps directly, eliminating the intermediate JS wrapper. * style: fix import type ordering in agent upload auth * fix: prevent token TTL race in MCPTokenStorage.storeTokens When expires_in is provided, use it directly instead of round-tripping through Date arithmetic. The previous code computed accessTokenExpiry as a Date, then after an async encryptV2 call, recomputed expiresIn by subtracting Date.now(). On loaded CI runners the elapsed time caused Math.floor to truncate to 0, triggering the 1-year fallback and making the token appear permanently valid — so refresh never fired.	2026-03-14 02:57:56 -04:00
Danny Avila	71a3b48504	🔑 fix: Require OTP Verification for 2FA Re-Enrollment and Backup Code Regeneration (#12223 ) * fix: require OTP verification for 2FA re-enrollment and backup code regeneration * fix: require OTP verification for account deletion when 2FA is enabled * refactor: Improve code formatting and readability in TwoFactorController and UserController - Reformatted code in TwoFactorController and UserController for better readability by aligning parameters and breaking long lines. - Updated test cases in deleteUser.spec.js and TwoFactorController.spec.js to enhance clarity by formatting object parameters consistently. * refactor: Consolidate OTP and backup code verification logic in TwoFactorController and UserController - Introduced a new `verifyOTPOrBackupCode` function to streamline the verification process for TOTP tokens and backup codes across multiple controllers. - Updated the `enable2FA`, `disable2FA`, and `deleteUserController` methods to utilize the new verification function, enhancing code reusability and readability. - Adjusted related tests to reflect the changes in verification logic, ensuring consistent behavior across different scenarios. - Improved error handling and response messages for verification failures, providing clearer feedback to users. * chore: linting * refactor: Update BackupCodesItem component to enhance OTP verification logic - Consolidated OTP input handling by moving the 2FA verification UI logic to a more consistent location within the component. - Improved the state management for OTP readiness, ensuring the regenerate button is only enabled when the OTP is ready. - Cleaned up imports by removing redundant type imports, enhancing code clarity and maintainability. * chore: lint * fix: stage 2FA re-enrollment in pending fields to prevent disarmament window enable2FA now writes to pendingTotpSecret/pendingBackupCodes instead of overwriting the live fields. confirm2FA performs the atomic swap only after the new TOTP code is verified. If the user abandons mid-flow, their existing 2FA remains active and intact.	2026-03-14 01:51:31 -04:00
Danny Avila	189cdf581d	🔐 fix: Add User Filter to Message Deletion (#12220 ) * fix: add user filter to message deletion to prevent IDOR * refactor: streamline DELETE request syntax in messages-delete test - Simplified the DELETE request syntax in the messages-delete.spec.js test file by combining multiple lines into a single line for improved readability. This change enhances the clarity of the test code without altering its functionality. * fix: address review findings for message deletion IDOR fix * fix: add user filter to message deletion in conversation tests - Included a user filter in the message deletion test to ensure proper handling of user-specific deletions, enhancing the accuracy of the test case and preventing potential IDOR vulnerabilities. * chore: lint	2026-03-13 23:42:37 -04:00
Danny Avila	ca79a03135	🚦 fix: Add Rate Limiting to Conversation Duplicate Endpoint (#12218 ) * fix: add rate limiting to conversation duplicate endpoint * chore: linter * fix: address review findings for conversation duplicate rate limiting * refactor: streamline test mocks for conversation routes - Consolidated mock implementations into a dedicated `convos-route-mocks.js` file to enhance maintainability and readability of test files. - Updated tests in `convos-duplicate-ratelimit.spec.js` and `convos.spec.js` to utilize the new mock structure, improving clarity and reducing redundancy. - Enhanced the `duplicateConversation` function to accept an optional title parameter for better flexibility in conversation duplication. * chore: rename files	2026-03-13 23:40:44 -04:00
Danny Avila	fa9e1b228a	🪪 fix: MCP API Responses and OAuth Validation (#12217 ) * 🔒 fix: Validate MCP Configs in Server Responses * 🔒 fix: Enhance OAuth URL Validation in MCPOAuthHandler - Introduced validation for OAuth URLs to ensure they do not target private or internal addresses, enhancing security against SSRF attacks. - Updated the OAuth flow to validate both authorization and token URLs before use, ensuring compliance with security standards. - Refactored redirect URI handling to streamline the OAuth client registration process. - Added comprehensive error handling for invalid URLs, improving robustness in OAuth interactions. * 🔒 feat: Implement Permission Checks for MCP Server Management - Added permission checkers for MCP server usage and creation, enhancing access control. - Updated routes for reinitializing MCP servers and retrieving authentication values to include these permission checks, ensuring only authorized users can access these functionalities. - Refactored existing permission logic to improve clarity and maintainability. * 🔒 fix: Enhance MCP Server Response Validation and Redaction - Updated MCP route tests to use `toMatchObject` for better validation of server response structures, ensuring consistency in expected properties. - Refactored the `redactServerSecrets` function to streamline the removal of sensitive information, ensuring that user-sourced API keys are properly redacted while retaining their source. - Improved OAuth security tests to validate rejection of private URLs across multiple endpoints, enhancing protection against SSRF vulnerabilities. - Added comprehensive tests for the `redactServerSecrets` function to ensure proper handling of various server configurations, reinforcing security measures. * chore: eslint * 🔒 fix: Enhance OAuth Server URL Validation in MCPOAuthHandler - Added validation for discovered authorization server URLs to ensure they meet security standards. - Improved logging to provide clearer insights when an authorization server is found from resource metadata. - Refactored the handling of authorization server URLs to enhance robustness against potential security vulnerabilities. * 🔒 test: Bypass SSRF validation for MCP OAuth Flow tests - Mocked SSRF validation functions to allow tests to use real local HTTP servers, facilitating more accurate testing of the MCP OAuth flow. - Updated test setup to ensure compatibility with the new mocking strategy, enhancing the reliability of the tests. * 🔒 fix: Add Validation for OAuth Metadata Endpoints in MCPOAuthHandler - Implemented checks for the presence and validity of registration and token endpoints in the OAuth metadata, enhancing security by ensuring that these URLs are properly validated before use. - Improved error handling and logging to provide better insights during the OAuth metadata processing, reinforcing the robustness of the OAuth flow. * 🔒 refactor: Simplify MCP Auth Values Endpoint Logic - Removed redundant permission checks for accessing the MCP server resource in the auth-values endpoint, streamlining the request handling process. - Consolidated error handling and response structure for improved clarity and maintainability. - Enhanced logging for better insights during the authentication value checks, reinforcing the robustness of the endpoint. * 🔒 test: Refactor LeaderElection Integration Tests for Improved Cleanup - Moved Redis key cleanup to the beforeEach hook to ensure a clean state before each test. - Enhanced afterEach logic to handle instance resignations and Redis key deletion more robustly, improving test reliability and maintainability.	2026-03-13 23:18:56 -04:00
Danny Avila	f32907cd36	🔏 fix: MCP Server URL Schema Validation (#12204 ) * fix: MCP server configuration validation and schema - Added tests to reject URLs containing environment variable references for SSE, streamable-http, and websocket types in the MCP routes. - Introduced a new schema in the data provider to ensure user input URLs do not resolve environment variables, enhancing security against potential leaks. - Updated existing MCP server user input schema to utilize the new validation logic, ensuring consistent handling of user-supplied URLs across the application. * fix: MCP URL validation to reject env variable references - Updated tests to ensure that URLs for SSE, streamable-http, and websocket types containing environment variable patterns are rejected, improving security against potential leaks. - Refactored the MCP server user input schema to enforce stricter validation rules, preventing the resolution of environment variables in user-supplied URLs. - Introduced new test cases for various URL types to validate the rejection logic, ensuring consistent handling across the application. * test: Enhance MCPServerUserInputSchema tests for environment variable handling - Introduced new test cases to validate the prevention of environment variable exfiltration through user input URLs in the MCPServerUserInputSchema. - Updated existing tests to confirm that URLs containing environment variable patterns are correctly resolved or rejected, improving security against potential leaks. - Refactored test structure to better organize environment variable handling scenarios, ensuring comprehensive coverage of edge cases.	2026-03-12 23:19:31 -04:00
Danny Avila	fcb344da47	🛂 fix: MCP OAuth Race Conditions, CSRF Fallback, and Token Expiry Handling (#12171 ) * fix: Implement race conditions in MCP OAuth flow - Added connection mutex to coalesce concurrent `getUserConnection` calls, preventing multiple simultaneous attempts. - Enhanced flow state management to retry once when a flow state is missing, improving resilience against race conditions. - Introduced `ReauthenticationRequiredError` for better error handling when access tokens are expired or missing. - Updated tests to cover new race condition scenarios and ensure proper handling of OAuth flows. * fix: Stale PENDING flow detection and OAuth URL re-issuance PENDING flows in handleOAuthRequired now check createdAt age — flows older than 2 minutes are treated as stale and replaced instead of joined. Fixes the case where a leftover PENDING flow from a previous session blocks new OAuth initiation. authorizationUrl is now stored in MCPOAuthFlowMetadata so that when a second caller joins an active PENDING flow (e.g., the SSE-emitting path in ToolService), it can re-issue the URL to the user via oauthStart. * fix: CSRF fallback via active PENDING flow in OAuth callback When the OAuth callback arrives without CSRF or session cookies (common in the chat/SSE flow where cookies can't be set on streaming responses), fall back to validating that a PENDING flow exists for the flowId. This is safe because the flow was created server-side after JWT authentication and the authorization code is PKCE-protected. * test: Extract shared OAuth test server helpers Move MockKeyv, getFreePort, trackSockets, and createOAuthMCPServer into a shared helpers/oauthTestServer module. Enhance the test server with refresh token support, token rotation, metadata discovery, and dynamic client registration endpoints. Add InMemoryTokenStore for token storage tests. Refactor MCPOAuthRaceCondition.test.ts to import from shared helpers. * test: Add comprehensive MCP OAuth test modules MCPOAuthTokenStorage — 21 tests for storeTokens/getTokens with InMemoryTokenStore: encrypt/decrypt round-trips, expiry calculation, refresh callback wiring, ReauthenticationRequiredError paths. MCPOAuthFlow — 10 tests against real HTTP server: token refresh with stored client info, refresh token rotation, metadata discovery, dynamic client registration, full store/retrieve/expire/refresh lifecycle. MCPOAuthConnectionEvents — 5 tests for MCPConnection OAuth event cycle with real OAuth-gated MCP server: oauthRequired emission on 401, oauthHandled reconnection, oauthFailed rejection, token expiry detection. MCPOAuthTokenExpiry — 12 tests for the token expiry edge case: refresh success/failure paths, ReauthenticationRequiredError, PENDING flow CSRF fallback, authorizationUrl metadata storage, full re-auth cycle after refresh failure, concurrent expired token coalescing, stale PENDING flow detection. * test: Enhance MCP OAuth connection tests with cooldown reset Added a `beforeEach` hook to clear the cooldown for `MCPConnection` before each test, ensuring a clean state. Updated the race condition handling in the tests to properly clear the timeout, improving reliability in the event data retrieval process. * refactor: PENDING flow management and state recovery in MCP OAuth - Introduced a constant `PENDING_STALE_MS` to define the age threshold for PENDING flows, improving the handling of stale flows. - Updated the logic in `MCPConnectionFactory` and `FlowStateManager` to check the age of PENDING flows before joining or reusing them. - Modified the `completeFlow` method to return false when the flow state is deleted, ensuring graceful handling of race conditions. - Enhanced tests to validate the new behavior and ensure robustness against state recovery issues. * refactor: MCP OAuth flow management and testing - Updated the `completeFlow` method to log warnings when a tool flow state is not found during completion, improving error handling. - Introduced a new `normalizeExpiresAt` function to standardize expiration timestamp handling across the application. - Refactored token expiration checks in `MCPConnectionFactory` to utilize the new normalization function, ensuring consistent behavior. - Added a comprehensive test suite for OAuth callback CSRF fallback logic, validating the handling of PENDING flows and their staleness. - Enhanced existing tests to cover new expiration normalization logic and ensure robust flow state management. * test: Add CSRF fallback tests for active PENDING flows in MCP OAuth - Introduced new tests to validate CSRF fallback behavior when a fresh PENDING flow exists without cookies, ensuring successful OAuth callback handling. - Added scenarios to reject requests when no PENDING flow exists, when only a COMPLETED flow is present, and when a PENDING flow is stale, enhancing the robustness of flow state management. - Improved overall test coverage for OAuth callback logic, reinforcing the handling of CSRF validation failures. * chore: imports order * refactor: Update UserConnectionManager to conditionally manage pending connections - Modified the logic in `UserConnectionManager` to only set pending connections if `forceNew` is false, preventing unnecessary overwrites. - Adjusted the cleanup process to ensure pending connections are only deleted when not forced, enhancing connection management efficiency. * refactor: MCP OAuth flow state management - Introduced a new method `storeStateMapping` in `MCPOAuthHandler` to securely map the OAuth state parameter to the flow ID, improving callback resolution and security against forgery. - Updated the OAuth initiation and callback handling in `mcp.js` to utilize the new state mapping functionality, ensuring robust flow management. - Refactored `MCPConnectionFactory` to store state mappings during flow initialization, enhancing the integrity of the OAuth process. - Adjusted comments to clarify the purpose of state parameters in authorization URLs, reinforcing code readability. * refactor: MCPConnection with OAuth recovery handling - Added `oauthRecovery` flag to manage OAuth recovery state during connection attempts. - Introduced `decrementCycleCount` method to reduce the circuit breaker's cycle count upon successful reconnection after OAuth recovery. - Updated connection logic to reset the `oauthRecovery` flag after handling OAuth, improving state management and connection reliability. * chore: Add debug logging for OAuth recovery cycle count decrement - Introduced a debug log statement in the `MCPConnection` class to track the decrement of the cycle count after a successful reconnection during OAuth recovery. - This enhancement improves observability and aids in troubleshooting connection issues related to OAuth recovery. * test: Add OAuth recovery cycle management tests - Introduced new tests for the OAuth recovery cycle in `MCPConnection`, validating the decrement of cycle counts after successful reconnections. - Added scenarios to ensure that the cycle count is not decremented on OAuth failures, enhancing the robustness of connection management. - Improved test coverage for OAuth reconnect scenarios, ensuring reliable behavior under various conditions. * feat: Implement circuit breaker configuration in MCP - Added circuit breaker settings to `.env.example` for max cycles, cycle window, and cooldown duration. - Refactored `MCPConnection` to utilize the new configuration values from `mcpConfig`, enhancing circuit breaker management. - Improved code maintainability by centralizing circuit breaker parameters in the configuration file. * refactor: Update decrementCycleCount method for circuit breaker management - Changed the visibility of the `decrementCycleCount` method in `MCPConnection` from private to public static, allowing it to be called with a server name parameter. - Updated calls to `decrementCycleCount` in `MCPConnectionFactory` to use the new static method, improving clarity and consistency in circuit breaker management during connection failures and OAuth recovery. - Enhanced the handling of circuit breaker state by ensuring the method checks for the existence of the circuit breaker before decrementing the cycle count. * refactor: cycle count decrement on tool listing failure - Added a call to `MCPConnection.decrementCycleCount` in the `MCPConnectionFactory` to handle cases where unauthenticated tool listing fails, improving circuit breaker management. - This change ensures that the cycle count is decremented appropriately, maintaining the integrity of the connection recovery process. * refactor: Update circuit breaker configuration and logic - Enhanced circuit breaker settings in `.env.example` to include new parameters for failed rounds and backoff strategies. - Refactored `MCPConnection` to utilize the updated configuration values from `mcpConfig`, improving circuit breaker management. - Updated tests to reflect changes in circuit breaker logic, ensuring accurate validation of connection behavior under rapid reconnect scenarios. * feat: Implement state mapping deletion in MCP flow management - Added a new method `deleteStateMapping` in `MCPOAuthHandler` to remove orphaned state mappings when a flow is replaced, preventing old authorization URLs from resolving after a flow restart. - Updated `MCPConnectionFactory` to call `deleteStateMapping` during flow cleanup, ensuring proper management of OAuth states. - Enhanced test coverage for state mapping functionality to validate the new deletion logic.	2026-03-10 21:15:01 -04:00
Lionel Ringenbach	6d0938be64	🔒 refactor: Set `ALLOW_SHARED_LINKS_PUBLIC` to `false` by Default (#12100 ) * fix: default ALLOW_SHARED_LINKS_PUBLIC to false for security Shared links were publicly accessible by default when ALLOW_SHARED_LINKS_PUBLIC was not explicitly set, which could lead to unintentional data exposure. Users may assume their authentication settings protect shared links when they do not. This changes the default behavior so shared links require JWT authentication unless ALLOW_SHARED_LINKS_PUBLIC is explicitly set to true. * Document ALLOW_SHARED_LINKS_PUBLIC in .env.example Add comment explaining ALLOW_SHARED_LINKS_PUBLIC setting. --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Danny Avila <danacordially@gmail.com>	2026-03-06 19:05:56 -05:00
Peter Nancarrow	14bcab60b3	🧬 feat: Allow Agent Editors to Duplicate Agents (#12041 ) * feat: allow editors to duplicate agents * fix: Update permissions for duplicating agents and enhance visibility in AgentFooter - Changed required permission for duplicating agents from VIEW to EDIT in the API route. - Updated AgentFooter component to display the duplicate button for admins and users with EDIT permission, improving access control. - Added tests to ensure the duplicate button visibility logic works correctly based on user roles and permissions. * test: Update AgentFooter tests to reflect permission changes - Adjusted tests in AgentFooter.spec.tsx to verify UI behavior based on user permissions. - Updated expectations for the visibility of the grant access dialog and duplicate button, ensuring they align with the new permission logic. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-03-03 20:45:02 -05:00
Danny Avila	b8c31e7314	🔱 chore: Harden API Routes Against IDOR and DoS Attacks (#11760 ) * 🔧 feat: Update user key handling in keys route and add comprehensive tests - Enhanced the PUT /api/keys route to destructure request body for better clarity and maintainability. - Introduced a new test suite for keys route, covering key update, deletion, and retrieval functionalities, ensuring robust validation and IDOR prevention. - Added tests to verify handling of extraneous fields and missing optional parameters in requests. * 🔧 fix: Enhance conversation deletion route with parameter validation - Updated the DELETE /api/convos route to handle cases where the request body is empty or the 'arg' parameter is null/undefined, returning a 400 status with an appropriate error message for DoS prevention. - Added corresponding tests to ensure proper validation and error handling for these scenarios, enhancing the robustness of the API. * 🔧 fix: Improve request body validation in keys and convos routes - Updated the DELETE /api/convos and PUT /api/keys routes to validate the request body, returning a 400 status for null or invalid bodies to enhance security and prevent potential DoS attacks. - Added corresponding tests to ensure proper error handling for these scenarios, improving the robustness of the API.	2026-02-12 18:08:24 -05:00

1 2 3 4 5 ...

533 commits