From ed02fe40e07b5cbe86b935537bc7050222d93936 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 2 Apr 2026 20:38:46 -0400 Subject: [PATCH 01/11] =?UTF-8?q?=F0=9F=AA=86=20fix:=20Allow=20Nested=20`a?= =?UTF-8?q?ddParams`=20in=20Config=20Schema=20(#12526)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: allow nested addParams in config schema * Respect no-op task constraint Constraint: Task 2 explicitly forbids code changes Directive: Keep this worker branch code-identical to the assigned base for this task Confidence: high Scope-risk: narrow Tested: git status --short (clean) * fix: align addParams web_search validation with runtime * test: cover addParams edge cases * chore: ignore .codex directory --- .gitignore | 1 + .../specs/config-schemas.spec.ts | 171 +++++++++++++++++- packages/data-provider/src/config.ts | 30 ++- 3 files changed, 199 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index e302d15a46..d775e70a26 100644 --- a/.gitignore +++ b/.gitignore @@ -137,6 +137,7 @@ helm/**/.values.yaml # AI Assistants /.claude/ +/.codex/ /.cursor/ /.copilot/ /.aider/ diff --git a/packages/data-provider/specs/config-schemas.spec.ts b/packages/data-provider/specs/config-schemas.spec.ts index fabd35cec9..66013baadd 100644 --- a/packages/data-provider/specs/config-schemas.spec.ts +++ b/packages/data-provider/specs/config-schemas.spec.ts @@ -1,8 +1,9 @@ import { - endpointSchema, paramDefinitionSchema, agentsEndpointSchema, azureEndpointSchema, + endpointSchema, + configSchema, } from '../src/config'; import { tModelSpecPresetSchema, EModelEndpoint } from '../src/schemas'; @@ -222,6 +223,109 @@ describe('endpointSchema deprecated fields', () => { }); }); +describe('endpointSchema addParams validation', () => { + const validEndpoint = { + name: 'CustomEndpoint', + apiKey: 'test-key', + baseURL: 'https://api.example.com', + models: { default: ['model-1'] }, + }; + const nestedAddParams = { + provider: { + only: ['z-ai'], + quantizations: ['int4'], + }, + }; + + it('accepts nested addParams objects and arrays', () => { + const result = endpointSchema.safeParse({ + ...validEndpoint, + addParams: nestedAddParams, + }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.addParams).toEqual(nestedAddParams); + } + }); + + it('keeps configSchema validation intact with nested custom addParams', () => { + const result = configSchema.safeParse({ + version: '1.0.0', + endpoints: { + custom: [ + { + ...validEndpoint, + addParams: nestedAddParams, + }, + ], + }, + }); + expect(result.success).toBe(true); + }); + + it('accepts boolean web_search in addParams', () => { + const result = endpointSchema.safeParse({ + ...validEndpoint, + addParams: { + provider: { + only: ['z-ai'], + }, + web_search: true, + }, + }); + expect(result.success).toBe(true); + }); + + it('accepts scalar addParams values', () => { + const result = endpointSchema.safeParse({ + ...validEndpoint, + addParams: { + model: 'custom-model', + retries: 2, + metadata: null, + }, + }); + expect(result.success).toBe(true); + }); + + it('rejects non-boolean web_search objects in addParams', () => { + const result = endpointSchema.safeParse({ + ...validEndpoint, + addParams: { + provider: { + only: ['z-ai'], + }, + web_search: { + enabled: true, + }, + }, + }); + expect(result.success).toBe(false); + }); + + it('rejects configSchema entries with non-boolean web_search objects in custom addParams', () => { + const result = configSchema.safeParse({ + version: '1.0.0', + endpoints: { + custom: [ + { + ...validEndpoint, + addParams: { + provider: { + only: ['z-ai'], + }, + web_search: { + enabled: true, + }, + }, + }, + ], + }, + }); + expect(result.success).toBe(false); + }); +}); + describe('agentsEndpointSchema', () => { it('does not accept baseURL', () => { const result = agentsEndpointSchema.safeParse({ @@ -251,4 +355,69 @@ describe('azureEndpointSchema', () => { expect(result.data).not.toHaveProperty('plugins'); } }); + + it('accepts nested addParams in azure groups', () => { + const result = azureEndpointSchema.safeParse({ + groups: [ + { + group: 'test-group', + apiKey: 'test-key', + models: { 'gpt-4': true }, + addParams: { + provider: { + only: ['z-ai'], + }, + }, + }, + ], + }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.groups[0].addParams).toEqual({ + provider: { + only: ['z-ai'], + }, + }); + } + }); + + it('accepts boolean web_search in azure addParams', () => { + const result = azureEndpointSchema.safeParse({ + groups: [ + { + group: 'test-group', + apiKey: 'test-key', + models: { 'gpt-4': true }, + addParams: { + provider: { + only: ['z-ai'], + }, + web_search: false, + }, + }, + ], + }); + expect(result.success).toBe(true); + }); + + it('rejects non-boolean web_search objects in azure addParams', () => { + const result = azureEndpointSchema.safeParse({ + groups: [ + { + group: 'test-group', + apiKey: 'test-key', + models: { 'gpt-4': true }, + addParams: { + provider: { + only: ['z-ai'], + }, + web_search: { + enabled: true, + }, + }, + }, + ], + }); + expect(result.success).toBe(false); + }); }); diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index ae3f5b9560..2b512a84d7 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -115,13 +115,39 @@ export const modelConfigSchema = z export type TAzureModelConfig = z.infer; +const paramValueSchema: z.ZodType = z.lazy(() => + z.union([ + z.string(), + z.number(), + z.boolean(), + z.null(), + z.array(paramValueSchema), + z.record(z.string(), paramValueSchema), + ]), +); + +/** Validates addParams while keeping web_search aligned with current runtime boolean handling. */ +const addParamsSchema: z.ZodType> = z + .record(z.string(), paramValueSchema) + .superRefine((params, ctx) => { + if (params.web_search === undefined || typeof params.web_search === 'boolean') { + return; + } + + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['web_search'], + message: '`web_search` must be a boolean in addParams', + }); + }); + export const azureBaseSchema = z.object({ apiKey: z.string(), serverless: z.boolean().optional(), instanceName: z.string().optional(), deploymentName: z.string().optional(), assistants: z.boolean().optional(), - addParams: z.record(z.union([z.string(), z.number(), z.boolean(), z.null()])).optional(), + addParams: addParamsSchema.optional(), dropParams: z.array(z.string()).optional(), version: z.string().optional(), baseURL: z.string().optional(), @@ -361,7 +387,7 @@ export const endpointSchema = baseEndpointSchema.merge( iconURL: z.string().optional(), modelDisplayLabel: z.string().optional(), headers: z.record(z.string()).optional(), - addParams: z.record(z.union([z.string(), z.number(), z.boolean(), z.null()])).optional(), + addParams: addParamsSchema.optional(), dropParams: z.array(z.string()).optional(), customParams: z .object({ From fa4a43da212929d49cfbd22c1462c08bcd73e445 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 2 Apr 2026 21:03:44 -0400 Subject: [PATCH 02/11] =?UTF-8?q?=F0=9F=94=90=20fix:=20Strip=20`code=5Fcha?= =?UTF-8?q?llenge`=20from=20Admin=20OAuth=20requests=20before=20Passport?= =?UTF-8?q?=20(#12534)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿ” 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) --- api/server/routes/admin/auth.js | 54 +++--- packages/api/src/auth/adminPkce.spec.ts | 211 ++++++++++++++++++++++++ packages/api/src/auth/exchange.ts | 67 ++++++++ 3 files changed, 298 insertions(+), 34 deletions(-) create mode 100644 packages/api/src/auth/adminPkce.spec.ts diff --git a/api/server/routes/admin/auth.js b/api/server/routes/admin/auth.js index 72f23b7d52..07306ac4db 100644 --- a/api/server/routes/admin/auth.js +++ b/api/server/routes/admin/auth.js @@ -3,7 +3,12 @@ const passport = require('passport'); const crypto = require('node:crypto'); const { CacheKeys } = require('librechat-data-provider'); const { logger, SystemCapabilities } = require('@librechat/data-schemas'); -const { getAdminPanelUrl, exchangeAdminCode, createSetBalanceConfig } = require('@librechat/api'); +const { + getAdminPanelUrl, + exchangeAdminCode, + createSetBalanceConfig, + storeAndStripChallenge, +} = require('@librechat/api'); const { loginController } = require('~/server/controllers/auth/LoginController'); const { requireCapability } = require('~/server/middleware/roles/capabilities'); const { createOAuthHandler } = require('~/server/controllers/auth/oauth'); @@ -73,11 +78,6 @@ router.get('/oauth/openid/check', (req, res) => { res.status(200).json({ message: 'OpenID check successful' }); }); -/** PKCE challenge cache TTL: 5 minutes (enough for user to authenticate with IdP) */ -const PKCE_CHALLENGE_TTL = 5 * 60 * 1000; -/** Regex pattern for valid PKCE challenges: 64 hex characters (SHA-256 hex digest) */ -const PKCE_CHALLENGE_PATTERN = /^[a-f0-9]{64}$/; - /** * Generates a random hex state string for OAuth flows. * @returns {string} A 32-byte random hex string. @@ -86,27 +86,6 @@ function generateState() { return crypto.randomBytes(32).toString('hex'); } -/** - * Stores a PKCE challenge in cache keyed by state. - * @param {string} state - The OAuth state value. - * @param {string | undefined} codeChallenge - The PKCE code_challenge from query params. - * @param {string} provider - Provider name for logging. - * @returns {Promise} True if stored successfully or no challenge provided. - */ -async function storePkceChallenge(state, codeChallenge, provider) { - if (typeof codeChallenge !== 'string' || !PKCE_CHALLENGE_PATTERN.test(codeChallenge)) { - return true; - } - try { - const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); - await cache.set(`pkce:${state}`, codeChallenge, PKCE_CHALLENGE_TTL); - return true; - } catch (err) { - logger.error(`[admin/oauth/${provider}] Failed to store PKCE challenge:`, err); - return false; - } -} - /** * Middleware to retrieve PKCE challenge from cache using the OAuth state. * Reads state from req.oauthState (set by a preceding middleware). @@ -148,7 +127,8 @@ function retrievePkceChallenge(provider) { router.get('/oauth/openid', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'openid'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'openid'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/openid/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -185,7 +165,8 @@ router.get( router.get('/oauth/saml', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'saml'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'saml'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/saml/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -222,7 +203,8 @@ router.post( router.get('/oauth/google', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'google'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'google'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/google/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -260,7 +242,8 @@ router.get( router.get('/oauth/github', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'github'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'github'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/github/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -298,7 +281,8 @@ router.get( router.get('/oauth/discord', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'discord'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'discord'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/discord/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -336,7 +320,8 @@ router.get( router.get('/oauth/facebook', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'facebook'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'facebook'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/facebook/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, @@ -374,7 +359,8 @@ router.get( router.get('/oauth/apple', async (req, res, next) => { const state = generateState(); - const stored = await storePkceChallenge(state, req.query.code_challenge, 'apple'); + const cache = getLogStores(CacheKeys.ADMIN_OAUTH_EXCHANGE); + const stored = await storeAndStripChallenge(cache, req, state, 'apple'); if (!stored) { return res.redirect( `${getAdminPanelUrl()}/auth/apple/callback?error=pkce_store_failed&error_description=Failed+to+store+PKCE+challenge`, diff --git a/packages/api/src/auth/adminPkce.spec.ts b/packages/api/src/auth/adminPkce.spec.ts new file mode 100644 index 0000000000..906b7d400b --- /dev/null +++ b/packages/api/src/auth/adminPkce.spec.ts @@ -0,0 +1,211 @@ +import { Keyv } from 'keyv'; + +jest.mock( + '@librechat/data-schemas', + () => ({ + logger: { + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }, + }), + { virtual: true }, +); + +import { stripCodeChallenge, storeAndStripChallenge } from './exchange'; +import type { PkceStrippableRequest } from './exchange'; + +function makeReq(overrides: Partial = {}): PkceStrippableRequest { + return { query: {}, originalUrl: '', url: '', ...overrides }; +} + +describe('stripCodeChallenge', () => { + const challenge = 'a'.repeat(64); + + it('removes code_challenge from req.query and both URL strings (sole param)', () => { + const req = makeReq({ + query: { code_challenge: challenge }, + originalUrl: `/api/admin/oauth/openid?code_challenge=${challenge}`, + url: `/oauth/openid?code_challenge=${challenge}`, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/api/admin/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + it('preserves other params when code_challenge is last', () => { + const req = makeReq({ + query: { foo: 'bar', code_challenge: challenge }, + originalUrl: `/oauth/openid?foo=bar&code_challenge=${challenge}`, + url: `/oauth/openid?foo=bar&code_challenge=${challenge}`, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.query.foo).toBe('bar'); + expect(req.originalUrl).toBe('/oauth/openid?foo=bar'); + expect(req.url).toBe('/oauth/openid?foo=bar'); + }); + + it('preserves other params when code_challenge is first of multiple', () => { + const req = makeReq({ + query: { code_challenge: challenge, foo: 'bar' }, + originalUrl: `/oauth/openid?code_challenge=${challenge}&foo=bar`, + url: `/oauth/openid?code_challenge=${challenge}&foo=bar`, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid?foo=bar'); + expect(req.url).toBe('/oauth/openid?foo=bar'); + }); + + it('preserves other params when code_challenge is in the middle', () => { + const req = makeReq({ + query: { a: '1', code_challenge: challenge, b: '2' }, + originalUrl: `/oauth/openid?a=1&code_challenge=${challenge}&b=2`, + url: `/oauth/openid?a=1&code_challenge=${challenge}&b=2`, + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid?a=1&b=2'); + expect(req.url).toBe('/oauth/openid?a=1&b=2'); + }); + + it('handles empty code_challenge= value', () => { + const req = makeReq({ + query: { code_challenge: '' }, + originalUrl: '/oauth/openid?code_challenge=', + url: '/oauth/openid?code_challenge=', + }); + + stripCodeChallenge(req); + + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + it('is a no-op when no code_challenge is present', () => { + const req = makeReq({ + query: { foo: 'bar' }, + originalUrl: '/oauth/openid?foo=bar', + url: '/oauth/openid?foo=bar', + }); + + stripCodeChallenge(req); + + expect(req.query.foo).toBe('bar'); + expect(req.originalUrl).toBe('/oauth/openid?foo=bar'); + expect(req.url).toBe('/oauth/openid?foo=bar'); + }); + + it('is a no-op on a bare path with no query string', () => { + const req = makeReq({ + query: {}, + originalUrl: '/oauth/openid', + url: '/oauth/openid', + }); + + stripCodeChallenge(req); + + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); +}); + +describe('storeAndStripChallenge', () => { + const challenge = 'a'.repeat(64); + + it('stores valid challenge in cache and strips from request', async () => { + const cache = new Keyv(); + const setSpy = jest.spyOn(cache, 'set'); + const req = makeReq({ + query: { code_challenge: challenge }, + originalUrl: `/oauth/openid?code_challenge=${challenge}`, + url: `/oauth/openid?code_challenge=${challenge}`, + }); + + const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + expect(result).toBe(true); + expect(setSpy).toHaveBeenCalledWith(`pkce:test-state`, challenge, expect.any(Number)); + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + it('strips and returns true when no code_challenge is present', async () => { + const cache = new Keyv(); + const setSpy = jest.spyOn(cache, 'set'); + const req = makeReq({ + query: {}, + originalUrl: '/oauth/openid', + url: '/oauth/openid', + }); + + const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + expect(result).toBe(true); + expect(setSpy).not.toHaveBeenCalled(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + it('strips and returns true when code_challenge is invalid (not 64 hex)', async () => { + const cache = new Keyv(); + const setSpy = jest.spyOn(cache, 'set'); + const req = makeReq({ + query: { code_challenge: 'too-short' }, + originalUrl: '/oauth/openid?code_challenge=too-short', + url: '/oauth/openid?code_challenge=too-short', + }); + + const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + expect(result).toBe(true); + expect(setSpy).not.toHaveBeenCalled(); + expect(req.query.code_challenge).toBeUndefined(); + expect(req.originalUrl).toBe('/oauth/openid'); + expect(req.url).toBe('/oauth/openid'); + }); + + it('returns false and does not strip on cache failure', async () => { + const cache = new Keyv(); + jest.spyOn(cache, 'set').mockRejectedValueOnce(new Error('cache down')); + const req = makeReq({ + query: { code_challenge: challenge }, + originalUrl: `/oauth/openid?code_challenge=${challenge}`, + url: `/oauth/openid?code_challenge=${challenge}`, + }); + + const result = await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + expect(result).toBe(false); + expect(req.query.code_challenge).toBe(challenge); + expect(req.originalUrl).toBe(`/oauth/openid?code_challenge=${challenge}`); + expect(req.url).toBe(`/oauth/openid?code_challenge=${challenge}`); + }); + + it('reads code_challenge before stripping (ordering guarantee)', async () => { + const cache = new Keyv(); + const setSpy = jest.spyOn(cache, 'set'); + const req = makeReq({ + query: { code_challenge: challenge }, + originalUrl: `/oauth/openid?code_challenge=${challenge}`, + url: `/oauth/openid?code_challenge=${challenge}`, + }); + + await storeAndStripChallenge(cache, req, 'test-state', 'openid'); + + const storedValue = setSpy.mock.calls[0][1]; + expect(storedValue).toBe(challenge); + }); +}); diff --git a/packages/api/src/auth/exchange.ts b/packages/api/src/auth/exchange.ts index f304ac6532..931c251d55 100644 --- a/packages/api/src/auth/exchange.ts +++ b/packages/api/src/auth/exchange.ts @@ -168,6 +168,73 @@ export async function exchangeAdminCode( }; } +/** PKCE challenge cache TTL: 5 minutes (enough for user to authenticate with IdP) */ +export const PKCE_CHALLENGE_TTL = 5 * 60 * 1000; +/** Regex pattern for valid PKCE challenges: 64 hex characters (SHA-256 hex digest) */ +export const PKCE_CHALLENGE_PATTERN = /^[a-f0-9]{64}$/; + +/** Removes `code_challenge` from a single URL string, preserving other query params. */ +const stripChallengeFromUrl = (url: string): string => + url.replace(/\?code_challenge=[^&]*&/, '?').replace(/[?&]code_challenge=[^&]*/, ''); + +/** Minimal request shape needed by {@link stripCodeChallenge}. */ +export interface PkceStrippableRequest { + query: Record; + originalUrl: string; + url: string; +} + +/** + * Strips `code_challenge` from the request query and URL strings. + * + * openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` + * to distinguish an initial authorization request from an OAuth callback. + * The admin-panel-specific `code_challenge` query parameter would cause the + * strategy to misclassify the request as a callback and return 401. + * + * Applied defensively to all providers to ensure the admin-panel-private + * `code_challenge` parameter never reaches any Passport strategy. + */ +export function stripCodeChallenge(req: PkceStrippableRequest): void { + delete req.query.code_challenge; + req.originalUrl = stripChallengeFromUrl(req.originalUrl); + req.url = stripChallengeFromUrl(req.url); +} + +/** + * Stores the admin-panel PKCE challenge in cache, then strips `code_challenge` + * from the request so it doesn't interfere with the Passport strategy. + * + * Must be called before `passport.authenticate()` โ€” the two operations are + * logically atomic: read the challenge from the query, persist it, then remove + * the parameter from the request URL. + * @param cache - The Keyv cache instance for storing PKCE challenges. + * @param req - The Express request to read and mutate. + * @param state - The OAuth state value (cache key). + * @param provider - Provider name for logging. + * @returns True if stored (or no challenge provided); false on cache failure. + */ +export async function storeAndStripChallenge( + cache: Keyv, + req: PkceStrippableRequest, + state: string, + provider: string, +): Promise { + const { code_challenge: codeChallenge } = req.query; + if (typeof codeChallenge !== 'string' || !PKCE_CHALLENGE_PATTERN.test(codeChallenge)) { + stripCodeChallenge(req); + return true; + } + try { + await cache.set(`pkce:${state}`, codeChallenge, PKCE_CHALLENGE_TTL); + stripCodeChallenge(req); + return true; + } catch (err) { + logger.error(`[admin/oauth/${provider}] Failed to store PKCE challenge:`, err); + return false; + } +} + /** * Checks if the redirect URI is for the admin panel (cross-origin). * Uses proper URL parsing to compare origins, handling edge cases where From b4d97bd8881909c09542df70f47cbef9fee1c8da Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 2 Apr 2026 22:29:54 -0400 Subject: [PATCH 03/11] =?UTF-8?q?=F0=9F=97=9C=EF=B8=8F=20refactor:=20Elimi?= =?UTF-8?q?nate=20Unstable=20React=20Keys=20During=20SSE=20Lifecycle=20(#1?= =?UTF-8?q?2536)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * debug: add instrumentation to MessageIcon arePropsEqual + render cycle tests Temporary debug commit to identify which field triggers MessageIcon re-renders during message creation and streaming. arePropsEqual now logs 'icon_memo_diff' with the exact field name and prev/next values whenever it returns false. Filter browser console for 'icon_memo_diff' to see the trigger. Also adds render-level integration tests that simulate the message lifecycle (initial mount, streaming chunks, context updates) and assert render counts via logger spy. * perf: stabilize MultiMessage key to prevent unmount/remount during SSE lifecycle messageId changes 3 times during the SSE message lifecycle: 1. useChatFunctions creates initialResponse with client-generated UUID 2. createdHandler replaces it with userMessageId + '_' 3. finalHandler replaces it with server-assigned messageId Since MultiMessage used key={message.messageId}, each change caused React to destroy and recreate the entire message component subtree, unmounting MessageIcon and all memoized children. This produced visible icon/image flickering that no memo comparator could prevent. Switch to key={parentMessageId + '_' + siblingIdx}: - parentMessageId is stable from creation through final response - siblingIdx ensures sibling switches still get clean remounts - Eliminates 2 unnecessary unmount/remount cycles per message Add key stability tests verifying: - Current key={messageId} causes 3 mounts / 2 unmounts per lifecycle - Stable key causes 1 mount / 0 unmounts per lifecycle - Sibling switches still trigger clean remounts with stable key * perf: stabilize root MultiMessage key across new conversation lifecycle When a user sends their first message in a new conversation, conversationId transitions from null/'new' to the server-assigned UUID. MessagesView used key={conversationId} on the root MultiMessage, so this transition destroyed the entire message tree and rebuilt it from scratch โ€” causing all MessageIcons to unmount/remount (visible as image flickering). Use a ref-based stable key that captures the first real conversationId and only changes on genuine conversation switches (navigating to a different conversation), not on the nullโ†’UUID transition within the same conversation. * debug: add mount/unmount lifecycle tracking to MessageIcon Adds icon_lifecycle logs (MOUNT/UNMOUNT) and render count to distinguish between fresh mounts (memo comparator not called) and internal re-renders (hook bypassing memo). Enable: localStorage.setItem('DEBUG_LOGGING', 'icon_lifecycle,icon_data,icon_memo_diff') * debug: add key and root tracking to MultiMessage and MessagesView Logs multi_message_key (stableKey, messageId, parentMessageId, route) and messages_view_key (rootKey, conversationId) to trace which key changes trigger unmount/remount cycles. Enable: localStorage.setItem('DEBUG_LOGGING', 'icon_lifecycle,icon_data,icon_memo_diff,multi_message_key,messages_view_key') * perf: remove key from root MultiMessage to prevent tree destruction The ref-based stable key still changed during 'new' โ†’ real UUID transition, destroying the entire tree. The root MultiMessage is the sole child at its position, so React reuses the instance via positional reconciliation without any key. The messageId prop (conversationId) naturally resets Recoil siblingIdxFamily state on conversation switches. * perf: remove unstable keys from MultiMessage to prevent SSE lifecycle remounts Both messageId and parentMessageId change during the SSE lifecycle (client UUID โ†’ CREATED server ID โ†’ FINAL server ID), making neither viable as a stable React key. Each key change caused React to destroy and recreate the entire message component subtree, including all memoized children โ€” visible as icon/image flickering. Remove explicit keys entirely and rely on React's positional reconciliation. MultiMessage always renders exactly one child at the same position, so React reuses the component instance and updates props in place. The existing memo comparators on ContentRender/MessageRender handle field-level diffing correctly. Update tests to verify: key={messageId} causes 3 mounts/2 unmounts per lifecycle, while no key causes 1 mount/0 unmounts. * perf: remove unstable keys from child MultiMessage in message wrappers Message.tsx, MessageContent.tsx, and MessageParts.tsx each render a child MultiMessage with key={messageId} for the current message's children. Since messageId changes during the SSE lifecycle (CREATED event replaces the user message ID), the child MultiMessage gets destroyed and recreated, unmounting the entire agent response subtree including its MessageIcon. Remove these keys for the same reason as the parent MultiMessage: each child MultiMessage renders exactly one child at a fixed position, so positional reconciliation correctly reuses the instance. * chore: remove MultiMessage key tests โ€” they test React behavior, not our code The tests verified that key={messageId} causes remounts while no key doesn't, but this is React's own reconciliation behavior. No unit test can prevent someone from re-adding a key prop to JSX. The JSDoc comments on MultiMessage document the decision and rationale. --- .../src/components/Chat/Messages/Message.tsx | 1 - .../components/Chat/Messages/MessageIcon.tsx | 72 ++++--- .../components/Chat/Messages/MessageParts.tsx | 1 - .../components/Chat/Messages/MessagesView.tsx | 1 - .../components/Chat/Messages/MultiMessage.tsx | 61 +++--- .../__tests__/MessageIcon.render.test.tsx | 193 ++++++++++++++++++ .../components/Messages/MessageContent.tsx | 1 - 7 files changed, 264 insertions(+), 66 deletions(-) create mode 100644 client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx diff --git a/client/src/components/Chat/Messages/Message.tsx b/client/src/components/Chat/Messages/Message.tsx index 53aef812fc..fc2a79fca0 100644 --- a/client/src/components/Chat/Messages/Message.tsx +++ b/client/src/components/Chat/Messages/Message.tsx @@ -47,7 +47,6 @@ export default function Message(props: TMessageProps) { { - logger.log('icon_data', iconData, assistant, agent); + const renderCountRef = useRef(0); + renderCountRef.current += 1; + + useEffect(() => { + logger.log( + 'icon_lifecycle', + 'MOUNT', + iconData?.modelLabel, + `render #${renderCountRef.current}`, + ); + return () => { + logger.log('icon_lifecycle', 'UNMOUNT', iconData?.modelLabel); + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + logger.log( + 'icon_data', + `render #${renderCountRef.current}`, + iconData?.isCreatedByUser ? 'user' : iconData?.modelLabel, + iconData, + ); const { data: endpointsConfig } = useGetEndpointsQuery(); const agentName = agent?.name ?? ''; diff --git a/client/src/components/Chat/Messages/MessageParts.tsx b/client/src/components/Chat/Messages/MessageParts.tsx index 3d13fa6ae0..2d0e3d512b 100644 --- a/client/src/components/Chat/Messages/MessageParts.tsx +++ b/client/src/components/Chat/Messages/MessageParts.tsx @@ -179,7 +179,6 @@ export default function Message(props: TMessageProps) {
- ); + return ; } else if (message.content) { - return ( - - ); + return ; } - return ( - - ); + return ; } diff --git a/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx b/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx new file mode 100644 index 0000000000..1c7b3c1aec --- /dev/null +++ b/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx @@ -0,0 +1,193 @@ +import React from 'react'; +import { render } from '@testing-library/react'; +import { EModelEndpoint } from 'librechat-data-provider'; +import type { Agent } from 'librechat-data-provider'; +import type { TMessageIcon } from '~/common'; + +jest.mock('librechat-data-provider', () => ({ + ...jest.requireActual('librechat-data-provider'), + getEndpointField: jest.fn(() => ''), +})); +jest.mock('~/data-provider', () => ({ + useGetEndpointsQuery: jest.fn(() => ({ data: {} })), +})); + +// logger is a plain object with a real function โ€” not a jest.fn() โ€” +// so restoreMocks/clearMocks won't touch it. We spy on it per-test instead. +const logCalls: unknown[][] = []; +jest.mock('~/utils', () => ({ + getIconEndpoint: jest.fn(() => 'agents'), + logger: { + log: (...args: unknown[]) => { + logCalls.push(args); + }, + }, +})); +jest.mock('~/components/Endpoints/ConvoIconURL', () => { + const ConvoIconURL = (props: Record) => ( +
+ ); + ConvoIconURL.displayName = 'ConvoIconURL'; + return { __esModule: true, default: ConvoIconURL }; +}); +jest.mock('~/components/Endpoints/Icon', () => { + const Icon = (props: Record) => ( +
+ ); + Icon.displayName = 'Icon'; + return { __esModule: true, default: Icon }; +}); + +import MessageIcon from '../MessageIcon'; + +const makeAgent = (overrides?: Partial): Agent => + ({ + id: 'agent_123', + name: 'GitHub Agent', + avatar: { filepath: '/images/agent-avatar.png' }, + ...overrides, + }) as Agent; + +const baseIconData: TMessageIcon = { + endpoint: EModelEndpoint.agents, + model: 'agent_123', + iconURL: undefined, + modelLabel: 'GitHub Agent', + isCreatedByUser: false, +}; + +describe('MessageIcon render cycles', () => { + beforeEach(() => { + logCalls.length = 0; + }); + + it('renders once on initial mount', () => { + render(); + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + expect(iconDataCalls).toHaveLength(1); + }); + + it('does not re-render when parent re-renders with same field values but new object references', () => { + const agent = makeAgent(); + const { rerender } = render(); + logCalls.length = 0; + + // Simulate parent re-render: new iconData object (same field values), new agent object (same data) + rerender(); + + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + expect(iconDataCalls).toHaveLength(0); + }); + + it('does not re-render when agent object reference changes but name and avatar are the same', () => { + const agent1 = makeAgent(); + const { rerender } = render(); + logCalls.length = 0; + + // New agent object with different id but same display fields + const agent2 = makeAgent({ id: 'agent_456' }); + rerender(); + + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + expect(iconDataCalls).toHaveLength(0); + }); + + it('re-renders when agent avatar filepath changes', () => { + const agent1 = makeAgent(); + const { rerender } = render(); + logCalls.length = 0; + + const agent2 = makeAgent({ avatar: { filepath: '/images/new-avatar.png' } }); + rerender(); + + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + expect(iconDataCalls).toHaveLength(1); + }); + + it('re-renders when agent goes from undefined to defined (name changes from undefined to string)', () => { + const { rerender } = render(); + logCalls.length = 0; + + rerender(); + + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + expect(iconDataCalls).toHaveLength(1); + }); + + describe('simulates message lifecycle', () => { + it('renders exactly twice during new message + streaming start: initial render + modelLabel update', () => { + // Phase 1: Initial response message created by useChatFunctions + // model is set to agent_id, iconURL is undefined, modelLabel is '' or sender + const initialIconData: TMessageIcon = { + endpoint: EModelEndpoint.agents, + model: 'agent_123', + iconURL: undefined, + modelLabel: '', // Not yet resolved + isCreatedByUser: false, + }; + const agent = makeAgent(); + + const { rerender } = render(); + + // Phase 2: First streaming chunk arrives, messageLabel resolves to agent name + // This is a legitimate re-render โ€” modelLabel changed from '' to 'GitHub Agent' + const streamingIconData: TMessageIcon = { + ...initialIconData, + modelLabel: 'GitHub Agent', + }; + + rerender(); + + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + // Exactly 2: initial mount + modelLabel change + expect(iconDataCalls).toHaveLength(2); + }); + + it('does NOT re-render on subsequent streaming chunks (content changes, isSubmitting stays true)', () => { + const iconData: TMessageIcon = { + endpoint: EModelEndpoint.agents, + model: 'agent_123', + iconURL: undefined, + modelLabel: 'GitHub Agent', + isCreatedByUser: false, + }; + const agent = makeAgent(); + + const { rerender } = render(); + logCalls.length = 0; + + // Simulate multiple parent re-renders from streaming chunks + // Parent (ContentRender) re-renders because chatContext changed, + // but MessageIcon props are identical field-by-field + for (let i = 0; i < 5; i++) { + rerender(); + } + + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + expect(iconDataCalls).toHaveLength(0); + }); + + it('does NOT re-render when agentsMap context updates with same agent data', () => { + const iconData: TMessageIcon = { + endpoint: EModelEndpoint.agents, + model: 'agent_123', + iconURL: undefined, + modelLabel: 'GitHub Agent', + isCreatedByUser: false, + }; + + // First render with agent from original agentsMap + const agent1 = makeAgent(); + const { rerender } = render(); + logCalls.length = 0; + + // agentsMap refetched โ†’ new agent object, same display data + const agent2 = makeAgent(); + expect(agent1).not.toBe(agent2); // different reference + rerender(); + + const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); + expect(iconDataCalls).toHaveLength(0); + }); + }); +}); diff --git a/client/src/components/Messages/MessageContent.tsx b/client/src/components/Messages/MessageContent.tsx index 977e397022..67865ed397 100644 --- a/client/src/components/Messages/MessageContent.tsx +++ b/client/src/components/Messages/MessageContent.tsx @@ -48,7 +48,6 @@ export default function MessageContent(props: TMessageProps) {
Date: Fri, 3 Apr 2026 12:22:58 -0400 Subject: [PATCH 04/11] =?UTF-8?q?=F0=9F=A7=B9=20chore:=20Clean=20Up=20Conf?= =?UTF-8?q?ig=20Fields=20(#12537)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore: remove unused `interface.endpointsMenu` config field * chore: address review โ€” restore JSDoc UI-only example, add Zod strip test * chore: remove unused `interface.sidePanel` config field * chore: restrict fileStrategy/fileStrategies schema to valid storage backends * fix: use valid FileStorage value in AppService test * chore: address review โ€” version bump, exhaustiveness guard, JSDoc, configSchema test * chore: remove debug logger.log from MessageIcon render path * fix: rewrite MessageIcon render tests to use render counting instead of logger spying * chore: bump librechat-data-provider to 0.8.407 * chore: sync example YAML version to 1.3.7 --- .../components/Chat/Messages/MessageIcon.tsx | 53 +++--------- .../__tests__/MessageIcon.render.test.tsx | 80 +++++++----------- librechat.example.yaml | 4 +- packages/api/src/admin/config.handler.spec.ts | 38 ++++----- packages/api/src/admin/config.spec.ts | 4 +- packages/api/src/admin/config.ts | 2 +- packages/api/src/app/AppService.spec.ts | 10 +-- packages/api/src/app/checks.ts | 9 +-- packages/api/src/app/config.test.ts | 2 +- packages/api/src/app/service.spec.ts | 8 +- packages/data-provider/package.json | 2 +- .../specs/config-schemas.spec.ts | 81 +++++++++++++++++++ packages/data-provider/src/config.ts | 29 ++++--- packages/data-schemas/src/app/interface.ts | 3 - .../data-schemas/src/app/resolution.spec.ts | 44 +++++----- .../data-schemas/src/methods/config.spec.ts | 28 +++---- packages/data-schemas/src/types/app.ts | 4 +- 17 files changed, 211 insertions(+), 190 deletions(-) diff --git a/client/src/components/Chat/Messages/MessageIcon.tsx b/client/src/components/Chat/Messages/MessageIcon.tsx index 619d2f827d..1c4401c338 100644 --- a/client/src/components/Chat/Messages/MessageIcon.tsx +++ b/client/src/components/Chat/Messages/MessageIcon.tsx @@ -1,10 +1,10 @@ -import { useMemo, useEffect, useRef, memo } from 'react'; +import { useMemo, memo } from 'react'; import { getEndpointField } from 'librechat-data-provider'; import type { Assistant, Agent } from 'librechat-data-provider'; import type { TMessageIcon } from '~/common'; import ConvoIconURL from '~/components/Endpoints/ConvoIconURL'; import { useGetEndpointsQuery } from '~/data-provider'; -import { getIconEndpoint, logger } from '~/utils'; +import { getIconEndpoint } from '~/utils'; import Icon from '~/components/Endpoints/Icon'; type MessageIconProps = { @@ -19,25 +19,20 @@ type MessageIconProps = { * this component renders display properties only, not identity-derived content. */ export function arePropsEqual(prev: MessageIconProps, next: MessageIconProps): boolean { - const checks: [string, unknown, unknown][] = [ - ['iconData.endpoint', prev.iconData?.endpoint, next.iconData?.endpoint], - ['iconData.model', prev.iconData?.model, next.iconData?.model], - ['iconData.iconURL', prev.iconData?.iconURL, next.iconData?.iconURL], - ['iconData.modelLabel', prev.iconData?.modelLabel, next.iconData?.modelLabel], - ['iconData.isCreatedByUser', prev.iconData?.isCreatedByUser, next.iconData?.isCreatedByUser], - ['agent.name', prev.agent?.name, next.agent?.name], - ['agent.avatar.filepath', prev.agent?.avatar?.filepath, next.agent?.avatar?.filepath], - ['assistant.name', prev.assistant?.name, next.assistant?.name], - [ - 'assistant.metadata.avatar', - prev.assistant?.metadata?.avatar, - next.assistant?.metadata?.avatar, - ], + const checks: [unknown, unknown][] = [ + [prev.iconData?.endpoint, next.iconData?.endpoint], + [prev.iconData?.model, next.iconData?.model], + [prev.iconData?.iconURL, next.iconData?.iconURL], + [prev.iconData?.modelLabel, next.iconData?.modelLabel], + [prev.iconData?.isCreatedByUser, next.iconData?.isCreatedByUser], + [prev.agent?.name, next.agent?.name], + [prev.agent?.avatar?.filepath, next.agent?.avatar?.filepath], + [prev.assistant?.name, next.assistant?.name], + [prev.assistant?.metadata?.avatar, next.assistant?.metadata?.avatar], ]; - for (const [field, prevVal, nextVal] of checks) { + for (const [prevVal, nextVal] of checks) { if (prevVal !== nextVal) { - logger.log('icon_memo_diff', `field "${field}" changed:`, prevVal, 'โ†’', nextVal); return false; } } @@ -45,28 +40,6 @@ export function arePropsEqual(prev: MessageIconProps, next: MessageIconProps): b } const MessageIcon = memo(({ iconData, assistant, agent }: MessageIconProps) => { - const renderCountRef = useRef(0); - renderCountRef.current += 1; - - useEffect(() => { - logger.log( - 'icon_lifecycle', - 'MOUNT', - iconData?.modelLabel, - `render #${renderCountRef.current}`, - ); - return () => { - logger.log('icon_lifecycle', 'UNMOUNT', iconData?.modelLabel); - }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); - - logger.log( - 'icon_data', - `render #${renderCountRef.current}`, - iconData?.isCreatedByUser ? 'user' : iconData?.modelLabel, - iconData, - ); const { data: endpointsConfig } = useGetEndpointsQuery(); const agentName = agent?.name ?? ''; diff --git a/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx b/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx index 1c7b3c1aec..c19142494a 100644 --- a/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx +++ b/client/src/components/Chat/Messages/__tests__/MessageIcon.render.test.tsx @@ -11,29 +11,25 @@ jest.mock('librechat-data-provider', () => ({ jest.mock('~/data-provider', () => ({ useGetEndpointsQuery: jest.fn(() => ({ data: {} })), })); - -// logger is a plain object with a real function โ€” not a jest.fn() โ€” -// so restoreMocks/clearMocks won't touch it. We spy on it per-test instead. -const logCalls: unknown[][] = []; jest.mock('~/utils', () => ({ getIconEndpoint: jest.fn(() => 'agents'), - logger: { - log: (...args: unknown[]) => { - logCalls.push(args); - }, - }, })); + +const iconRenderCount = { current: 0 }; + jest.mock('~/components/Endpoints/ConvoIconURL', () => { - const ConvoIconURL = (props: Record) => ( -
- ); + const ConvoIconURL = (props: Record) => { + iconRenderCount.current += 1; + return
; + }; ConvoIconURL.displayName = 'ConvoIconURL'; return { __esModule: true, default: ConvoIconURL }; }); jest.mock('~/components/Endpoints/Icon', () => { - const Icon = (props: Record) => ( -
- ); + const Icon = (props: Record) => { + iconRenderCount.current += 1; + return
; + }; Icon.displayName = 'Icon'; return { __esModule: true, default: Icon }; }); @@ -58,79 +54,68 @@ const baseIconData: TMessageIcon = { describe('MessageIcon render cycles', () => { beforeEach(() => { - logCalls.length = 0; + iconRenderCount.current = 0; }); it('renders once on initial mount', () => { render(); - const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); - expect(iconDataCalls).toHaveLength(1); + expect(iconRenderCount.current).toBe(1); }); it('does not re-render when parent re-renders with same field values but new object references', () => { const agent = makeAgent(); const { rerender } = render(); - logCalls.length = 0; + iconRenderCount.current = 0; - // Simulate parent re-render: new iconData object (same field values), new agent object (same data) rerender(); - const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); - expect(iconDataCalls).toHaveLength(0); + expect(iconRenderCount.current).toBe(0); }); it('does not re-render when agent object reference changes but name and avatar are the same', () => { const agent1 = makeAgent(); const { rerender } = render(); - logCalls.length = 0; + iconRenderCount.current = 0; - // New agent object with different id but same display fields const agent2 = makeAgent({ id: 'agent_456' }); rerender(); - const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); - expect(iconDataCalls).toHaveLength(0); + expect(iconRenderCount.current).toBe(0); }); it('re-renders when agent avatar filepath changes', () => { const agent1 = makeAgent(); const { rerender } = render(); - logCalls.length = 0; + iconRenderCount.current = 0; const agent2 = makeAgent({ avatar: { filepath: '/images/new-avatar.png' } }); rerender(); - const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); - expect(iconDataCalls).toHaveLength(1); + expect(iconRenderCount.current).toBe(1); }); it('re-renders when agent goes from undefined to defined (name changes from undefined to string)', () => { const { rerender } = render(); - logCalls.length = 0; + iconRenderCount.current = 0; rerender(); - const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); - expect(iconDataCalls).toHaveLength(1); + expect(iconRenderCount.current).toBe(1); }); describe('simulates message lifecycle', () => { it('renders exactly twice during new message + streaming start: initial render + modelLabel update', () => { - // Phase 1: Initial response message created by useChatFunctions - // model is set to agent_id, iconURL is undefined, modelLabel is '' or sender const initialIconData: TMessageIcon = { endpoint: EModelEndpoint.agents, model: 'agent_123', iconURL: undefined, - modelLabel: '', // Not yet resolved + modelLabel: '', isCreatedByUser: false, }; const agent = makeAgent(); const { rerender } = render(); - // Phase 2: First streaming chunk arrives, messageLabel resolves to agent name - // This is a legitimate re-render โ€” modelLabel changed from '' to 'GitHub Agent' const streamingIconData: TMessageIcon = { ...initialIconData, modelLabel: 'GitHub Agent', @@ -138,9 +123,7 @@ describe('MessageIcon render cycles', () => { rerender(); - const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); - // Exactly 2: initial mount + modelLabel change - expect(iconDataCalls).toHaveLength(2); + expect(iconRenderCount.current).toBe(2); }); it('does NOT re-render on subsequent streaming chunks (content changes, isSubmitting stays true)', () => { @@ -154,17 +137,13 @@ describe('MessageIcon render cycles', () => { const agent = makeAgent(); const { rerender } = render(); - logCalls.length = 0; + iconRenderCount.current = 0; - // Simulate multiple parent re-renders from streaming chunks - // Parent (ContentRender) re-renders because chatContext changed, - // but MessageIcon props are identical field-by-field for (let i = 0; i < 5; i++) { rerender(); } - const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); - expect(iconDataCalls).toHaveLength(0); + expect(iconRenderCount.current).toBe(0); }); it('does NOT re-render when agentsMap context updates with same agent data', () => { @@ -176,18 +155,15 @@ describe('MessageIcon render cycles', () => { isCreatedByUser: false, }; - // First render with agent from original agentsMap const agent1 = makeAgent(); const { rerender } = render(); - logCalls.length = 0; + iconRenderCount.current = 0; - // agentsMap refetched โ†’ new agent object, same display data const agent2 = makeAgent(); - expect(agent1).not.toBe(agent2); // different reference + expect(agent1).not.toBe(agent2); rerender(); - const iconDataCalls = logCalls.filter((c) => c[0] === 'icon_data'); - expect(iconDataCalls).toHaveLength(0); + expect(iconRenderCount.current).toBe(0); }); }); }); diff --git a/librechat.example.yaml b/librechat.example.yaml index 03bb5f5bc2..92206c4b6e 100644 --- a/librechat.example.yaml +++ b/librechat.example.yaml @@ -2,7 +2,7 @@ # https://www.librechat.ai/docs/configuration/librechat_yaml # Configuration version (required) -version: 1.3.6 +version: 1.3.7 # Cache settings: Set to true to enable caching cache: true @@ -80,10 +80,8 @@ interface: By using the Website, you acknowledge that you have read these Terms of Service and agree to be bound by them. - endpointsMenu: true modelSelect: true parameters: true - sidePanel: true presets: true prompts: use: true diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index ad33f5f158..8f4e2002a5 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -54,7 +54,7 @@ function createHandlers(overrides = {}) { toggleConfigActive: jest.fn().mockResolvedValue({ _id: 'c1', isActive: false }), hasConfigCapability: jest.fn().mockResolvedValue(true), - getAppConfig: jest.fn().mockResolvedValue({ interface: { endpointsMenu: true } }), + getAppConfig: jest.fn().mockResolvedValue({ interface: { modelSelect: true } }), ...overrides, }; const handlers = createAdminConfigHandlers(deps); @@ -133,7 +133,7 @@ describe('createAdminConfigHandlers', () => { }); const req = mockReq({ params: { principalType: 'role', principalId: 'admin' }, - body: { overrides: { interface: { endpointsMenu: false } } }, + body: { overrides: { interface: { modelSelect: false } } }, }); const res = mockRes(); @@ -148,7 +148,7 @@ describe('createAdminConfigHandlers', () => { }); const req = mockReq({ params: { principalType: 'role', principalId: 'admin' }, - body: { overrides: { interface: { endpointsMenu: false } } }, + body: { overrides: { interface: { modelSelect: false } } }, }); const res = mockRes(); @@ -178,7 +178,7 @@ describe('createAdminConfigHandlers', () => { params: { principalType: 'role', principalId: 'admin' }, body: { overrides: { - interface: { endpointsMenu: false, prompts: false, agents: { use: false } }, + interface: { modelSelect: false, prompts: false, agents: { use: false } }, }, }, }); @@ -188,7 +188,7 @@ describe('createAdminConfigHandlers', () => { expect(res.statusCode).toBe(201); const savedOverrides = deps.upsertConfig.mock.calls[0][3]; - expect(savedOverrides.interface).toEqual({ endpointsMenu: false }); + expect(savedOverrides.interface).toEqual({ modelSelect: false }); }); it('preserves UI sub-keys in composite permission fields like mcpServers', async () => { @@ -263,17 +263,13 @@ describe('createAdminConfigHandlers', () => { const { handlers, deps } = createHandlers(); const req = mockReq({ params: { principalType: 'role', principalId: 'admin' }, - query: { fieldPath: 'interface.endpointsMenu' }, + query: { fieldPath: 'interface.modelSelect' }, }); const res = mockRes(); await handlers.deleteConfigField(req, res); - expect(deps.unsetConfigField).toHaveBeenCalledWith( - 'role', - 'admin', - 'interface.endpointsMenu', - ); + expect(deps.unsetConfigField).toHaveBeenCalledWith('role', 'admin', 'interface.modelSelect'); }); it('allows deleting mcpServers UI sub-key paths', async () => { @@ -343,18 +339,14 @@ describe('createAdminConfigHandlers', () => { const { handlers, deps } = createHandlers(); const req = mockReq({ params: { principalType: 'role', principalId: 'admin' }, - query: { fieldPath: 'interface.endpointsMenu' }, + query: { fieldPath: 'interface.modelSelect' }, }); const res = mockRes(); await handlers.deleteConfigField(req, res); expect(res.statusCode).toBe(200); - expect(deps.unsetConfigField).toHaveBeenCalledWith( - 'role', - 'admin', - 'interface.endpointsMenu', - ); + expect(deps.unsetConfigField).toHaveBeenCalledWith('role', 'admin', 'interface.modelSelect'); }); it('returns 400 when fieldPath query param is missing', async () => { @@ -407,7 +399,7 @@ describe('createAdminConfigHandlers', () => { params: { principalType: 'role', principalId: 'admin' }, body: { entries: [ - { fieldPath: 'interface.endpointsMenu', value: false }, + { fieldPath: 'interface.modelSelect', value: false }, { fieldPath: 'interface.prompts', value: false }, ], }, @@ -418,7 +410,7 @@ describe('createAdminConfigHandlers', () => { expect(res.statusCode).toBe(200); const patchedFields = deps.patchConfigFields.mock.calls[0][3]; - expect(patchedFields['interface.endpointsMenu']).toBe(false); + expect(patchedFields['interface.modelSelect']).toBe(false); expect(patchedFields['interface.prompts']).toBeUndefined(); }); @@ -632,21 +624,21 @@ describe('createAdminConfigHandlers', () => { name: 'upsertConfigOverrides', reqOverrides: { params: { principalType: 'role', principalId: 'admin' }, - body: { overrides: { interface: { endpointsMenu: false } } }, + body: { overrides: { interface: { modelSelect: false } } }, }, }, { name: 'patchConfigField', reqOverrides: { params: { principalType: 'role', principalId: 'admin' }, - body: { entries: [{ fieldPath: 'interface.endpointsMenu', value: false }] }, + body: { entries: [{ fieldPath: 'interface.modelSelect', value: false }] }, }, }, { name: 'deleteConfigField', reqOverrides: { params: { principalType: 'role', principalId: 'admin' }, - query: { fieldPath: 'interface.endpointsMenu' }, + query: { fieldPath: 'interface.modelSelect' }, }, }, { @@ -775,7 +767,7 @@ describe('createAdminConfigHandlers', () => { await handlers.getBaseConfig(req, res); expect(res.statusCode).toBe(200); - expect(res.body!.config).toEqual({ interface: { endpointsMenu: true } }); + expect(res.body!.config).toEqual({ interface: { modelSelect: true } }); }); }); }); diff --git a/packages/api/src/admin/config.spec.ts b/packages/api/src/admin/config.spec.ts index 499cfaa35b..3298cb5faa 100644 --- a/packages/api/src/admin/config.spec.ts +++ b/packages/api/src/admin/config.spec.ts @@ -2,7 +2,7 @@ import { isValidFieldPath, getTopLevelSection } from './config'; describe('isValidFieldPath', () => { it('accepts simple dot paths', () => { - expect(isValidFieldPath('interface.endpointsMenu')).toBe(true); + expect(isValidFieldPath('interface.modelSelect')).toBe(true); expect(isValidFieldPath('registration.socialLogins')).toBe(true); expect(isValidFieldPath('a')).toBe(true); expect(isValidFieldPath('a.b.c.d')).toBe(true); @@ -47,7 +47,7 @@ describe('isValidFieldPath', () => { describe('getTopLevelSection', () => { it('returns first segment of a dot path', () => { - expect(getTopLevelSection('interface.endpointsMenu')).toBe('interface'); + expect(getTopLevelSection('interface.modelSelect')).toBe('interface'); expect(getTopLevelSection('registration.socialLogins.github')).toBe('registration'); }); diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index 357096da9b..c1ce2cb13f 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -40,7 +40,7 @@ export function getTopLevelSection(fieldPath: string): string { * - `"interface.mcpServers.use"` โ†’ true (permission sub-key) * - `"interface.mcpServers.placeholder"` โ†’ false (UI-only sub-key) * - `"interface.peoplePicker.users"` โ†’ true (all peoplePicker sub-keys are permissions) - * - `"interface.endpointsMenu"` โ†’ false (UI-only field) + * - `"interface.modelSelect"` โ†’ false (UI-only field) */ function isInterfacePermissionPath(fieldPath: string): boolean { const parts = fieldPath.split('.'); diff --git a/packages/api/src/app/AppService.spec.ts b/packages/api/src/app/AppService.spec.ts index df607d612b..2c07460b84 100644 --- a/packages/api/src/app/AppService.spec.ts +++ b/packages/api/src/app/AppService.spec.ts @@ -85,7 +85,7 @@ describe('AppService', () => { it('should correctly assign process.env and initialize app config based on custom config', async () => { const config: Partial = { registration: { socialLogins: ['testLogin'] }, - fileStrategy: 'testStrategy' as FileSources, + fileStrategy: FileSources.s3, balance: { enabled: true, }, @@ -93,22 +93,20 @@ describe('AppService', () => { const result = await AppService({ config, systemTools: mockSystemTools }); - expect(process.env.CDN_PROVIDER).toEqual('testStrategy'); + expect(process.env.CDN_PROVIDER).toEqual('s3'); expect(result).toEqual( expect.objectContaining({ config: expect.objectContaining({ - fileStrategy: 'testStrategy', + fileStrategy: 's3', }), registration: expect.objectContaining({ socialLogins: ['testLogin'], }), - fileStrategy: 'testStrategy', + fileStrategy: 's3', interfaceConfig: expect.objectContaining({ - endpointsMenu: true, modelSelect: true, parameters: true, - sidePanel: true, presets: true, }), mcpConfig: null, diff --git a/packages/api/src/app/checks.ts b/packages/api/src/app/checks.ts index 66f5b620e6..50312267ac 100644 --- a/packages/api/src/app/checks.ts +++ b/packages/api/src/app/checks.ts @@ -192,16 +192,13 @@ export function checkInterfaceConfig(appConfig: AppConfig) { if (i === 0) i++; } - // warn about config.modelSpecs.enforce if true and if any of these, endpointsMenu, modelSelect, presets, or parameters are enabled, that enforcing model specs can conflict with these options. + // warn about config.modelSpecs.enforce if true and if any of these, modelSelect, presets, or parameters are enabled, that enforcing model specs can conflict with these options. if ( appConfig?.modelSpecs?.enforce && - (interfaceConfig?.endpointsMenu || - interfaceConfig?.modelSelect || - interfaceConfig?.presets || - interfaceConfig?.parameters) + (interfaceConfig?.modelSelect || interfaceConfig?.presets || interfaceConfig?.parameters) ) { logger.warn( - "Note: Enforcing model specs can conflict with the interface options: endpointsMenu, modelSelect, presets, and parameters. It's recommended to disable these options from the interface or disable enforcing model specs.", + "Note: Enforcing model specs can conflict with the interface options: modelSelect, presets, and parameters. It's recommended to disable these options from the interface or disable enforcing model specs.", ); if (i === 0) i++; } diff --git a/packages/api/src/app/config.test.ts b/packages/api/src/app/config.test.ts index 3e2ee6d143..a3e7401efd 100644 --- a/packages/api/src/app/config.test.ts +++ b/packages/api/src/app/config.test.ts @@ -10,7 +10,7 @@ const createTestAppConfig = (overrides: Partial = {}): AppConfig => { version: '1.0.0', cache: true, interface: { - endpointsMenu: true, + modelSelect: true, }, registration: { socialLogins: [], diff --git a/packages/api/src/app/service.spec.ts b/packages/api/src/app/service.spec.ts index 8d168095c7..e5e076c8eb 100644 --- a/packages/api/src/app/service.spec.ts +++ b/packages/api/src/app/service.spec.ts @@ -34,7 +34,7 @@ function createMockCache(namespace = 'app_config') { function createDeps(overrides = {}) { const cache = createMockCache(); - const baseConfig = { interfaceConfig: { endpointsMenu: true }, endpoints: ['openAI'] }; + const baseConfig = { interfaceConfig: { modelSelect: true }, endpoints: ['openAI'] }; return { loadBaseConfig: jest.fn().mockResolvedValue(baseConfig), @@ -79,7 +79,7 @@ describe('createAppConfigService', () => { getApplicableConfigs: jest .fn() .mockResolvedValue([ - { priority: 10, overrides: { interface: { endpointsMenu: false } }, isActive: true }, + { priority: 10, overrides: { interface: { modelSelect: false } }, isActive: true }, ]), }); const { getAppConfig } = createAppConfigService(deps); @@ -125,7 +125,7 @@ describe('createAppConfigService', () => { getApplicableConfigs: jest .fn() .mockResolvedValue([ - { priority: 10, overrides: { interface: { endpointsMenu: false } }, isActive: true }, + { priority: 10, overrides: { interface: { modelSelect: false } }, isActive: true }, ]), }); const { getAppConfig } = createAppConfigService(deps); @@ -133,7 +133,7 @@ describe('createAppConfigService', () => { const config = await getAppConfig({ role: 'ADMIN' }); const merged = config as TestConfig; - expect(merged.interfaceConfig?.endpointsMenu).toBe(false); + expect(merged.interfaceConfig?.modelSelect).toBe(false); expect(merged.endpoints).toEqual(['openAI']); }); diff --git a/packages/data-provider/package.json b/packages/data-provider/package.json index 67eebf9cad..725d780b09 100644 --- a/packages/data-provider/package.json +++ b/packages/data-provider/package.json @@ -1,6 +1,6 @@ { "name": "librechat-data-provider", - "version": "0.8.406", + "version": "0.8.407", "description": "data services for librechat apps", "main": "dist/index.js", "module": "dist/index.es.js", diff --git a/packages/data-provider/specs/config-schemas.spec.ts b/packages/data-provider/specs/config-schemas.spec.ts index 66013baadd..6e76bced06 100644 --- a/packages/data-provider/specs/config-schemas.spec.ts +++ b/packages/data-provider/specs/config-schemas.spec.ts @@ -4,8 +4,12 @@ import { azureEndpointSchema, endpointSchema, configSchema, + interfaceSchema, + fileStorageSchema, + fileStrategiesSchema, } from '../src/config'; import { tModelSpecPresetSchema, EModelEndpoint } from '../src/schemas'; +import { FileSources } from '../src/types/files'; describe('paramDefinitionSchema', () => { it('accepts a minimal definition with only key', () => { @@ -421,3 +425,80 @@ describe('azureEndpointSchema', () => { expect(result.success).toBe(false); }); }); + +describe('fileStorageSchema', () => { + const validStrategies = [ + FileSources.local, + FileSources.firebase, + FileSources.s3, + FileSources.azure_blob, + ]; + const invalidStrategies = [ + FileSources.openai, + FileSources.azure, + FileSources.vectordb, + FileSources.execute_code, + FileSources.mistral_ocr, + FileSources.azure_mistral_ocr, + FileSources.vertexai_mistral_ocr, + FileSources.text, + FileSources.document_parser, + ]; + + for (const strategy of validStrategies) { + it(`accepts storage strategy "${strategy}"`, () => { + expect(fileStorageSchema.safeParse(strategy).success).toBe(true); + }); + } + + for (const strategy of invalidStrategies) { + it(`rejects processing strategy "${strategy}"`, () => { + expect(fileStorageSchema.safeParse(strategy).success).toBe(false); + }); + } +}); + +describe('fileStrategiesSchema', () => { + it('accepts valid storage strategies for all sub-fields', () => { + const result = fileStrategiesSchema.safeParse({ + default: FileSources.s3, + avatar: FileSources.local, + image: FileSources.firebase, + document: FileSources.azure_blob, + }); + expect(result.success).toBe(true); + }); + + it('rejects processing strategies in sub-fields', () => { + const result = fileStrategiesSchema.safeParse({ + default: FileSources.vectordb, + }); + expect(result.success).toBe(false); + }); +}); + +describe('configSchema fileStrategy', () => { + it('rejects a processing strategy as fileStrategy', () => { + const result = configSchema.safeParse({ version: '1.3.7', fileStrategy: FileSources.vectordb }); + expect(result.success).toBe(false); + }); + + it('defaults fileStrategy to local when absent', () => { + const result = configSchema.safeParse({ version: '1.3.7' }); + expect(result.success).toBe(true); + expect(result.data?.fileStrategy).toBe(FileSources.local); + }); +}); + +describe('interfaceSchema', () => { + it('silently strips removed legacy fields', () => { + const result = interfaceSchema.parse({ + endpointsMenu: true, + sidePanel: true, + modelSelect: false, + }); + expect(result).not.toHaveProperty('endpointsMenu'); + expect(result).not.toHaveProperty('sidePanel'); + expect(result.modelSelect).toBe(false); + }); +}); diff --git a/packages/data-provider/src/config.ts b/packages/data-provider/src/config.ts index 2b512a84d7..ca40ec2c8c 100644 --- a/packages/data-provider/src/config.ts +++ b/packages/data-provider/src/config.ts @@ -62,14 +62,27 @@ export enum SettingsViews { advanced = 'advanced', } +/** Validates any FileSources value โ€” use for file metadata, DB records, and upload routing. */ export const fileSourceSchema = z.nativeEnum(FileSources); +/** Storage backend strategies only โ€” use for config fields that set where files are stored. */ +const FILE_STORAGE_BACKENDS = [ + FileSources.local, + FileSources.firebase, + FileSources.s3, + FileSources.azure_blob, +] as const satisfies ReadonlyArray; + +export const fileStorageSchema = z.enum(FILE_STORAGE_BACKENDS); + +export type FileStorage = z.infer; + export const fileStrategiesSchema = z .object({ - default: fileSourceSchema.optional(), - avatar: fileSourceSchema.optional(), - image: fileSourceSchema.optional(), - document: fileSourceSchema.optional(), + default: fileStorageSchema.optional(), + avatar: fileStorageSchema.optional(), + image: fileStorageSchema.optional(), + document: fileStorageSchema.optional(), }) .optional(); @@ -677,10 +690,8 @@ export const interfaceSchema = z termsOfService: termsOfServiceSchema.optional(), customWelcome: z.string().optional(), mcpServers: mcpServersSchema.optional(), - endpointsMenu: z.boolean().optional(), modelSelect: z.boolean().optional(), parameters: z.boolean().optional(), - sidePanel: z.boolean().optional(), multiConvo: z.boolean().optional(), bookmarks: z.boolean().optional(), memories: z.boolean().optional(), @@ -735,10 +746,8 @@ export const interfaceSchema = z .optional(), }) .default({ - endpointsMenu: true, modelSelect: true, parameters: true, - sidePanel: true, presets: true, multiConvo: true, bookmarks: true, @@ -1059,7 +1068,7 @@ export const configSchema = z.object({ .optional(), interface: interfaceSchema, turnstile: turnstileSchema.optional(), - fileStrategy: fileSourceSchema.default(FileSources.local), + fileStrategy: fileStorageSchema.default(FileSources.local), fileStrategies: fileStrategiesSchema, actions: z .object({ @@ -1833,7 +1842,7 @@ export enum Constants { /** Key for the app's version. */ VERSION = 'v0.8.4', /** Key for the Custom Config's version (librechat.yaml). */ - CONFIG_VERSION = '1.3.6', + CONFIG_VERSION = '1.3.7', /** Standard value for the first message's `parentMessageId` value, to indicate no parent exists. */ NO_PARENT = '00000000-0000-0000-0000-000000000000', /** Standard value to use whatever the submission prelim. `responseMessageId` is */ diff --git a/packages/data-schemas/src/app/interface.ts b/packages/data-schemas/src/app/interface.ts index 1701a22fad..3cd71cfb20 100644 --- a/packages/data-schemas/src/app/interface.ts +++ b/packages/data-schemas/src/app/interface.ts @@ -29,14 +29,11 @@ export async function loadDefaultInterface({ const loadedInterface: AppConfig['interfaceConfig'] = removeNullishValues({ // UI elements - use schema defaults - endpointsMenu: - interfaceConfig?.endpointsMenu ?? (hasModelSpecs ? false : defaults.endpointsMenu), modelSelect: interfaceConfig?.modelSelect ?? (hasModelSpecs ? includesAddedEndpoints : defaults.modelSelect), parameters: interfaceConfig?.parameters ?? (hasModelSpecs ? false : defaults.parameters), presets: interfaceConfig?.presets ?? (hasModelSpecs ? false : defaults.presets), - sidePanel: interfaceConfig?.sidePanel ?? defaults.sidePanel, privacyPolicy: interfaceConfig?.privacyPolicy ?? defaults.privacyPolicy, termsOfService: interfaceConfig?.termsOfService ?? defaults.termsOfService, mcpServers: interfaceConfig?.mcpServers ?? defaults.mcpServers, diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 991f6afb40..e241b7f9a4 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -16,7 +16,7 @@ function fakeConfig(overrides: Record, priority: number): IConf } const baseConfig = { - interfaceConfig: { endpointsMenu: true, sidePanel: true }, + interfaceConfig: { modelSelect: true, parameters: true }, registration: { enabled: true }, endpoints: ['openAI'], } as unknown as AppConfig; @@ -32,11 +32,11 @@ describe('mergeConfigOverrides', () => { }); it('deep merges interface UI fields into interfaceConfig', () => { - const configs = [fakeConfig({ interface: { endpointsMenu: false } }, 10)]; + const configs = [fakeConfig({ interface: { modelSelect: false } }, 10)]; const result = mergeConfigOverrides(baseConfig, configs) as unknown as Record; const iface = result.interfaceConfig as Record; - expect(iface.endpointsMenu).toBe(false); - expect(iface.sidePanel).toBe(true); + expect(iface.modelSelect).toBe(false); + expect(iface.parameters).toBe(true); }); it('sorts by priority โ€” higher priority wins', () => { @@ -58,16 +58,16 @@ describe('mergeConfigOverrides', () => { it('does not mutate the base config', () => { const original = JSON.parse(JSON.stringify(baseConfig)); - const configs = [fakeConfig({ interface: { endpointsMenu: false } }, 10)]; + const configs = [fakeConfig({ interface: { modelSelect: false } }, 10)]; mergeConfigOverrides(baseConfig, configs); expect(baseConfig).toEqual(original); }); it('handles null override values', () => { - const configs = [fakeConfig({ interface: { endpointsMenu: null } }, 10)]; + const configs = [fakeConfig({ interface: { modelSelect: null } }, 10)]; const result = mergeConfigOverrides(baseConfig, configs) as unknown as Record; const iface = result.interfaceConfig as Record; - expect(iface.endpointsMenu).toBeNull(); + expect(iface.modelSelect).toBeNull(); }); it('skips configs with no overrides object', () => { @@ -97,20 +97,20 @@ describe('mergeConfigOverrides', () => { it('merges three priority levels in order', () => { const configs = [ - fakeConfig({ interface: { endpointsMenu: false } }, 0), - fakeConfig({ interface: { endpointsMenu: true, sidePanel: false } }, 10), - fakeConfig({ interface: { sidePanel: true } }, 100), + fakeConfig({ interface: { modelSelect: false } }, 0), + fakeConfig({ interface: { modelSelect: true, parameters: false } }, 10), + fakeConfig({ interface: { parameters: true } }, 100), ]; const result = mergeConfigOverrides(baseConfig, configs) as unknown as Record; const iface = result.interfaceConfig as Record; - expect(iface.endpointsMenu).toBe(true); - expect(iface.sidePanel).toBe(true); + expect(iface.modelSelect).toBe(true); + expect(iface.parameters).toBe(true); }); it('remaps all renamed YAML keys (exhaustiveness check)', () => { const base = { mcpConfig: null, - interfaceConfig: { endpointsMenu: true }, + interfaceConfig: { modelSelect: true }, turnstileConfig: {}, } as unknown as AppConfig; @@ -118,7 +118,7 @@ describe('mergeConfigOverrides', () => { fakeConfig( { mcpServers: { srv: { url: 'http://mcp' } }, - interface: { endpointsMenu: false }, + interface: { modelSelect: false }, turnstile: { siteKey: 'key-123' }, }, 10, @@ -127,7 +127,7 @@ describe('mergeConfigOverrides', () => { const result = mergeConfigOverrides(base, configs) as unknown as Record; expect(result.mcpConfig).toEqual({ srv: { url: 'http://mcp' } }); - expect((result.interfaceConfig as Record).endpointsMenu).toBe(false); + expect((result.interfaceConfig as Record).modelSelect).toBe(false); expect((result.turnstileConfig as Record).siteKey).toBe('key-123'); expect(result.mcpServers).toBeUndefined(); @@ -137,14 +137,14 @@ describe('mergeConfigOverrides', () => { it('strips interface permission fields from overrides', () => { const base = { - interfaceConfig: { endpointsMenu: true, sidePanel: true }, + interfaceConfig: { modelSelect: true, parameters: true }, } as unknown as AppConfig; const configs = [ fakeConfig( { interface: { - endpointsMenu: false, + modelSelect: false, prompts: false, agents: { use: false }, marketplace: { use: false }, @@ -157,14 +157,14 @@ describe('mergeConfigOverrides', () => { const iface = result.interfaceConfig as Record; // UI field should be merged - expect(iface.endpointsMenu).toBe(false); + expect(iface.modelSelect).toBe(false); // Boolean permission fields should be stripped expect(iface.prompts).toBeUndefined(); // Object permission fields with only permission sub-keys should be stripped expect(iface.agents).toBeUndefined(); expect(iface.marketplace).toBeUndefined(); // Untouched base field preserved - expect(iface.sidePanel).toBe(true); + expect(iface.parameters).toBe(true); }); it('preserves UI sub-keys in composite permission fields like mcpServers', () => { @@ -220,7 +220,7 @@ describe('mergeConfigOverrides', () => { it('drops interface entirely when only permission fields are present', () => { const base = { - interfaceConfig: { endpointsMenu: true }, + interfaceConfig: { modelSelect: true }, } as unknown as AppConfig; const configs = [fakeConfig({ interface: { prompts: false, agents: false } }, 10)]; @@ -228,7 +228,7 @@ describe('mergeConfigOverrides', () => { const iface = result.interfaceConfig as Record; // Base should be unchanged - expect(iface.endpointsMenu).toBe(true); + expect(iface.modelSelect).toBe(true); expect(iface.prompts).toBeUndefined(); expect(iface.agents).toBeUndefined(); }); @@ -281,7 +281,7 @@ describe('INTERFACE_PERMISSION_FIELDS', () => { }); it('does not contain UI-only fields', () => { - const uiFields = ['endpointsMenu', 'modelSelect', 'parameters', 'presets', 'sidePanel']; + const uiFields = ['modelSelect', 'parameters', 'presets']; for (const field of uiFields) { expect(INTERFACE_PERMISSION_FIELDS.has(field)).toBe(false); } diff --git a/packages/data-schemas/src/methods/config.spec.ts b/packages/data-schemas/src/methods/config.spec.ts index 8bcf73a733..8ab6be35ee 100644 --- a/packages/data-schemas/src/methods/config.spec.ts +++ b/packages/data-schemas/src/methods/config.spec.ts @@ -32,7 +32,7 @@ describe('upsertConfig', () => { PrincipalType.ROLE, 'admin', PrincipalModel.ROLE, - { interface: { endpointsMenu: false } }, + { interface: { modelSelect: false } }, 10, ); @@ -49,7 +49,7 @@ describe('upsertConfig', () => { PrincipalType.ROLE, 'admin', PrincipalModel.ROLE, - { interface: { endpointsMenu: false } }, + { interface: { modelSelect: false } }, 10, ); @@ -57,7 +57,7 @@ describe('upsertConfig', () => { PrincipalType.ROLE, 'admin', PrincipalModel.ROLE, - { interface: { endpointsMenu: true } }, + { interface: { modelSelect: true } }, 10, ); @@ -70,7 +70,7 @@ describe('upsertConfig', () => { PrincipalType.ROLE, 'admin', PrincipalModel.ROLE, - { interface: { endpointsMenu: true } }, + { interface: { modelSelect: true } }, 10, ); @@ -78,7 +78,7 @@ describe('upsertConfig', () => { PrincipalType.ROLE, 'admin', PrincipalModel.ROLE, - { interface: { endpointsMenu: false } }, + { interface: { modelSelect: false } }, 10, ); @@ -240,7 +240,7 @@ describe('patchConfigFields', () => { PrincipalType.ROLE, 'admin', PrincipalModel.ROLE, - { interface: { endpointsMenu: true, sidePanel: true } }, + { interface: { modelSelect: true, parameters: true } }, 10, ); @@ -248,14 +248,14 @@ describe('patchConfigFields', () => { PrincipalType.ROLE, 'admin', PrincipalModel.ROLE, - { 'interface.endpointsMenu': false }, + { 'interface.modelSelect': false }, 10, ); const overrides = result!.overrides as Record; const iface = overrides.interface as Record; - expect(iface.endpointsMenu).toBe(false); - expect(iface.sidePanel).toBe(true); + expect(iface.modelSelect).toBe(false); + expect(iface.parameters).toBe(true); }); it('creates a config if none exists (upsert)', async () => { @@ -263,7 +263,7 @@ describe('patchConfigFields', () => { PrincipalType.ROLE, 'newrole', PrincipalModel.ROLE, - { 'interface.endpointsMenu': false }, + { 'interface.modelSelect': false }, 10, ); @@ -278,19 +278,19 @@ describe('unsetConfigField', () => { PrincipalType.ROLE, 'admin', PrincipalModel.ROLE, - { interface: { endpointsMenu: false, sidePanel: false } }, + { interface: { modelSelect: false, parameters: false } }, 10, ); const result = await methods.unsetConfigField( PrincipalType.ROLE, 'admin', - 'interface.endpointsMenu', + 'interface.modelSelect', ); const overrides = result!.overrides as Record; const iface = overrides.interface as Record; - expect(iface.endpointsMenu).toBeUndefined(); - expect(iface.sidePanel).toBe(false); + expect(iface.modelSelect).toBeUndefined(); + expect(iface.parameters).toBe(false); }); it('returns null for non-existent config', async () => { diff --git a/packages/data-schemas/src/types/app.ts b/packages/data-schemas/src/types/app.ts index 73d65611b0..5118fa9583 100644 --- a/packages/data-schemas/src/types/app.ts +++ b/packages/data-schemas/src/types/app.ts @@ -1,6 +1,6 @@ import type { TEndpoint, - FileSources, + FileStorage, TFileConfig, TAzureConfig, TCustomConfig, @@ -62,7 +62,7 @@ export interface AppConfig { /** Web search configuration */ webSearch?: TCustomConfig['webSearch']; /** File storage strategy ('local', 's3', 'firebase', 'azure_blob') */ - fileStrategy: FileSources.local | FileSources.s3 | FileSources.firebase | FileSources.azure_blob; + fileStrategy: FileStorage; /** File strategies configuration */ fileStrategies?: TCustomConfig['fileStrategies']; /** Registration configurations */ From 936936596b25db999c787e4b9ff683c100c2b6d6 Mon Sep 17 00:00:00 2001 From: Daniel Lew Date: Fri, 3 Apr 2026 11:24:59 -0500 Subject: [PATCH 05/11] =?UTF-8?q?=F0=9F=94=8D=20fix:=20only=20show=20Searc?= =?UTF-8?q?hbar=20if=20enabled=20(#12424)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The search bar was showing even if you have no search capability; now it respects the `enabled` field. --- client/src/components/UnifiedSidebar/ConversationsSection.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/components/UnifiedSidebar/ConversationsSection.tsx b/client/src/components/UnifiedSidebar/ConversationsSection.tsx index c02741d038..4cd487ad3d 100644 --- a/client/src/components/UnifiedSidebar/ConversationsSection.tsx +++ b/client/src/components/UnifiedSidebar/ConversationsSection.tsx @@ -120,7 +120,7 @@ const ConversationsSection = memo(() => { )} - + {search.enabled && }
{isSmallScreen && (
Date: Fri, 3 Apr 2026 10:24:11 -0700 Subject: [PATCH 06/11] =?UTF-8?q?=F0=9F=94=A8=20fix:=20Custom=20Role=20Per?= =?UTF-8?q?missions=20(#12528)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- api/server/routes/__tests__/roles.spec.js | 155 ++++++++++++++++++ api/server/routes/roles.js | 12 +- .../Sharing/PeoplePickerAdminSettings.tsx | 60 +++---- .../src/components/ui/AdminSettingsDialog.tsx | 59 +++---- client/src/data-provider/roles.ts | 12 ++ client/src/hooks/AuthContext.tsx | 20 ++- .../src/hooks/__tests__/AuthContext.spec.tsx | 136 ++++++++++++++- client/src/hooks/index.ts | 1 + client/src/hooks/useRoleSelector.ts | 64 ++++++++ packages/data-provider/src/api-endpoints.ts | 3 +- packages/data-provider/src/data-service.ts | 4 + packages/data-provider/src/keys.ts | 1 + packages/data-provider/src/roles.ts | 10 ++ packages/data-provider/src/types/queries.ts | 7 + 14 files changed, 462 insertions(+), 82 deletions(-) create mode 100644 api/server/routes/__tests__/roles.spec.js create mode 100644 client/src/hooks/useRoleSelector.ts diff --git a/api/server/routes/__tests__/roles.spec.js b/api/server/routes/__tests__/roles.spec.js new file mode 100644 index 0000000000..e78965d460 --- /dev/null +++ b/api/server/routes/__tests__/roles.spec.js @@ -0,0 +1,155 @@ +const express = require('express'); +const request = require('supertest'); +const { SystemRoles, roleDefaults } = require('librechat-data-provider'); + +const mockGetRoleByName = jest.fn(); +const mockHasCapability = jest.fn(); + +jest.mock('~/server/middleware', () => ({ + requireJwtAuth: (_req, _res, next) => next(), +})); + +jest.mock('~/server/middleware/roles/capabilities', () => ({ + hasCapability: (...args) => mockHasCapability(...args), + requireCapability: () => (_req, _res, next) => next(), +})); + +jest.mock('~/models', () => ({ + getRoleByName: (...args) => mockGetRoleByName(...args), + updateRoleByName: jest.fn(), +})); + +const rolesRouter = require('../roles'); + +function createApp(user) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.user = user; + next(); + }); + app.use('/api/roles', rolesRouter); + return app; +} + +const staffRole = { + name: 'STAFF', + permissions: { + PROMPTS: { USE: true, CREATE: false }, + }, +}; + +const userRole = roleDefaults[SystemRoles.USER]; +const adminRole = roleDefaults[SystemRoles.ADMIN]; + +beforeEach(() => { + jest.clearAllMocks(); + mockHasCapability.mockResolvedValue(false); + mockGetRoleByName.mockResolvedValue(null); +}); + +describe('GET /api/roles/:roleName โ€” isOwnRole authorization', () => { + it('allows a custom role user to fetch their own role', async () => { + mockGetRoleByName.mockResolvedValue(staffRole); + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + expect(res.body.name).toBe('STAFF'); + expect(mockGetRoleByName).toHaveBeenCalledWith('STAFF', '-_id -__v'); + }); + + it('returns 403 when a custom role user requests a different custom role', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/MANAGER'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('returns 403 when a custom role user requests ADMIN', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/ADMIN'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('allows USER to fetch the USER role (roleDefaults key)', async () => { + mockGetRoleByName.mockResolvedValue(userRole); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.USER}`); + + expect(res.status).toBe(200); + }); + + it('returns 403 when USER requests the ADMIN role', async () => { + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.ADMIN}`); + + expect(res.status).toBe(403); + }); + + it('allows ADMIN user to fetch their own ADMIN role via isOwnRole', async () => { + mockHasCapability.mockResolvedValue(false); + mockGetRoleByName.mockResolvedValue(adminRole); + const app = createApp({ id: 'u1', role: SystemRoles.ADMIN }); + + const res = await request(app).get(`/api/roles/${SystemRoles.ADMIN}`); + + expect(res.status).toBe(200); + }); + + it('allows any user with READ_ROLES capability to fetch any role', async () => { + mockHasCapability.mockResolvedValue(true); + mockGetRoleByName.mockResolvedValue(staffRole); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + expect(res.body.name).toBe('STAFF'); + }); + + it('returns 404 when the requested role does not exist', async () => { + mockGetRoleByName.mockResolvedValue(null); + const app = createApp({ id: 'u1', role: 'GHOST' }); + + const res = await request(app).get('/api/roles/GHOST'); + + expect(res.status).toBe(404); + }); + + it('returns 500 when getRoleByName throws', async () => { + mockGetRoleByName.mockRejectedValue(new Error('db error')); + const app = createApp({ id: 'u1', role: SystemRoles.USER }); + + const res = await request(app).get(`/api/roles/${SystemRoles.USER}`); + + expect(res.status).toBe(500); + }); + + it('returns 403 for prototype property names like constructor (no prototype pollution)', async () => { + const app = createApp({ id: 'u1', role: 'STAFF' }); + + const res = await request(app).get('/api/roles/constructor'); + + expect(res.status).toBe(403); + expect(mockGetRoleByName).not.toHaveBeenCalled(); + }); + + it('treats hasCapability failure as no capability (does not 500)', async () => { + mockHasCapability.mockRejectedValue(new Error('capability check failed')); + const app = createApp({ id: 'u1', role: 'STAFF' }); + mockGetRoleByName.mockResolvedValue(staffRole); + + const res = await request(app).get('/api/roles/STAFF'); + + expect(res.status).toBe(200); + }); +}); diff --git a/api/server/routes/roles.js b/api/server/routes/roles.js index 25ee47854d..456d144af1 100644 --- a/api/server/routes/roles.js +++ b/api/server/routes/roles.js @@ -71,9 +71,7 @@ const createPermissionUpdateHandler = (permissionKey) => { const config = permissionConfigs[permissionKey]; return async (req, res) => { - const { roleName: _r } = req.params; - // TODO: TEMP, use a better parsing for roleName - const roleName = _r.toUpperCase(); + const { roleName } = req.params; const updates = req.body; try { @@ -110,9 +108,7 @@ const createPermissionUpdateHandler = (permissionKey) => { * Get a specific role by name */ router.get('/:roleName', async (req, res) => { - const { roleName: _r } = req.params; - // TODO: TEMP, use a better parsing for roleName - const roleName = _r.toUpperCase(); + const { roleName } = req.params; try { let hasReadRoles = false; @@ -121,7 +117,9 @@ router.get('/:roleName', async (req, res) => { } catch (err) { logger.warn(`[GET /roles/:roleName] capability check failed: ${err.message}`); } - if (!hasReadRoles && (roleName === SystemRoles.ADMIN || !roleDefaults[roleName])) { + const isOwnRole = req.user?.role === roleName; + const isDefaultRole = Object.hasOwn(roleDefaults, roleName); + if (!hasReadRoles && !isOwnRole && (roleName === SystemRoles.ADMIN || !isDefaultRole)) { return res.status(403).send({ message: 'Unauthorized' }); } diff --git a/client/src/components/Sharing/PeoplePickerAdminSettings.tsx b/client/src/components/Sharing/PeoplePickerAdminSettings.tsx index 38d44ad311..5f334f3764 100644 --- a/client/src/components/Sharing/PeoplePickerAdminSettings.tsx +++ b/client/src/components/Sharing/PeoplePickerAdminSettings.tsx @@ -1,8 +1,8 @@ -import { useMemo, useEffect, useState } from 'react'; +import { useEffect, useState } from 'react'; import * as Ariakit from '@ariakit/react'; import { ShieldEllipsis } from 'lucide-react'; import { useForm, Controller } from 'react-hook-form'; -import { Permissions, SystemRoles, roleDefaults, PermissionTypes } from 'librechat-data-provider'; +import { Permissions, SystemRoles, PermissionTypes } from 'librechat-data-provider'; import { Button, Switch, @@ -15,7 +15,7 @@ import { } from '@librechat/client'; import type { Control, UseFormSetValue, UseFormGetValues } from 'react-hook-form'; import { useUpdatePeoplePickerPermissionsMutation } from '~/data-provider'; -import { useLocalize, useAuthContext } from '~/hooks'; +import { useLocalize, useAuthContext, useRoleSelector } from '~/hooks'; type FormValues = { [Permissions.VIEW_USERS]: boolean; @@ -70,7 +70,7 @@ const LabelController: React.FC = ({ const PeoplePickerAdminSettings = () => { const localize = useLocalize(); const { showToast } = useToastContext(); - const { user, roles } = useAuthContext(); + const { user } = useAuthContext(); const { mutate, isLoading } = useUpdatePeoplePickerPermissionsMutation({ onSuccess: () => { showToast({ status: 'success', message: localize('com_ui_saved') }); @@ -81,15 +81,14 @@ const PeoplePickerAdminSettings = () => { }); const [isRoleMenuOpen, setIsRoleMenuOpen] = useState(false); - const [selectedRole, setSelectedRole] = useState(SystemRoles.USER); - - const defaultValues = useMemo(() => { - const rolePerms = roles?.[selectedRole]?.permissions; - if (rolePerms) { - return rolePerms[PermissionTypes.PEOPLE_PICKER]; - } - return roleDefaults[selectedRole].permissions[PermissionTypes.PEOPLE_PICKER]; - }, [roles, selectedRole]); + const { + selectedRole, + isSelectedCustomRole, + isCustomRoleLoading, + isCustomRoleError, + defaultValues, + roleDropdownItems, + } = useRoleSelector(PermissionTypes.PEOPLE_PICKER); const { reset, @@ -100,17 +99,15 @@ const PeoplePickerAdminSettings = () => { formState: { isSubmitting }, } = useForm({ mode: 'onChange', - defaultValues, + defaultValues: defaultValues as FormValues, }); useEffect(() => { - const value = roles?.[selectedRole]?.permissions?.[PermissionTypes.PEOPLE_PICKER]; - if (value) { - reset(value); - } else { - reset(roleDefaults[selectedRole].permissions[PermissionTypes.PEOPLE_PICKER]); + if (isSelectedCustomRole && (isCustomRoleLoading || isCustomRoleError)) { + return; } - }, [roles, selectedRole, reset]); + reset(defaultValues as FormValues); + }, [isSelectedCustomRole, isCustomRoleLoading, isCustomRoleError, defaultValues, reset]); if (user?.role !== SystemRoles.ADMIN) { return null; @@ -138,21 +135,6 @@ const PeoplePickerAdminSettings = () => { mutate({ roleName: selectedRole, updates: data }); }; - const roleDropdownItems = [ - { - label: SystemRoles.USER, - onClick: () => { - setSelectedRole(SystemRoles.USER); - }, - }, - { - label: SystemRoles.ADMIN, - onClick: () => { - setSelectedRole(SystemRoles.ADMIN); - }, - }, - ]; - return ( @@ -179,7 +161,7 @@ const PeoplePickerAdminSettings = () => { isOpen={isRoleMenuOpen} setIsOpen={setIsRoleMenuOpen} trigger={ - + {selectedRole} } @@ -207,7 +189,11 @@ const PeoplePickerAdminSettings = () => {