From 20129484cfe7d5bcf28a83279a3db30c07cc81f4 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:14:04 +0200 Subject: [PATCH 01/20] fix: validate principal config overrides against configSchema Reject admin config writes whose fields fail configSchema, and drop invalid fields from stored overrides at merge time so the base value from librechat.yaml survives. --- .../config-override-validation.spec.ts | 205 ++++++++++++++++++ packages/api/src/admin/config.handler.spec.ts | 95 ++++++++ packages/api/src/admin/config.ts | 21 +- packages/api/src/app/service.spec.ts | 4 +- packages/data-provider/src/index.ts | 1 + packages/data-provider/src/overrides.spec.ts | 73 +++++++ packages/data-provider/src/overrides.ts | 199 +++++++++++++++++ .../data-schemas/src/app/resolution.spec.ts | 71 +++++- packages/data-schemas/src/app/resolution.ts | 53 ++++- 9 files changed, 713 insertions(+), 9 deletions(-) create mode 100644 e2e/specs/mock/scenarios/config-override-validation.spec.ts create mode 100644 packages/data-provider/src/overrides.spec.ts create mode 100644 packages/data-provider/src/overrides.ts diff --git a/e2e/specs/mock/scenarios/config-override-validation.spec.ts b/e2e/specs/mock/scenarios/config-override-validation.spec.ts new file mode 100644 index 00000000000..cc1eceeff8d --- /dev/null +++ b/e2e/specs/mock/scenarios/config-override-validation.spec.ts @@ -0,0 +1,205 @@ +import { expect, test } from '@playwright/test'; +import type { APIRequestContext } from '@playwright/test'; +import { getPrimaryE2EUser, getSecondaryE2EUser } from '../../../setup/users.mock'; +import { withMongo } from '../db'; + +/** + * Principal config overrides are checked against `configSchema`: an invalid field is + * rejected when written, and one already stored is ignored when merged, so the + * `librechat.yaml` value survives. The primary user (first registered, ADMIN) writes + * overrides for the secondary user, whose own `/api/config` shows the merged result. + * `interface.contextCost` is `true` in e2e/config/librechat.e2e.yaml. + */ + +type Session = { headers: Record; userId: string }; +type InterfaceConfig = { contextCost?: unknown; customWelcome?: string }; + +async function login( + request: APIRequestContext, + user: { email: string; password: string }, +): Promise { + const res = await request.post('/api/auth/login', { + data: { email: user.email, password: user.password }, + }); + expect(res.ok()).toBeTruthy(); + const { token, user: body } = (await res.json()) as { + token: string; + user: { id?: string; _id?: string }; + }; + const userId = body.id ?? body._id; + expect(token).toBeTruthy(); + expect(userId).toBeTruthy(); + return { headers: { Authorization: `Bearer ${token}` }, userId: userId as string }; +} + +async function sessions(request: APIRequestContext): Promise<{ admin: Session; target: Session }> { + const admin = await login(request, getPrimaryE2EUser()); + const target = await login(request, getSecondaryE2EUser()); + return { admin, target }; +} + +function configPath(userId: string): string { + return `/api/admin/config/user/${userId}`; +} + +async function readInterface( + request: APIRequestContext, + session: Session, +): Promise { + const res = await request.get('/api/config', { headers: session.headers }); + expect(res.ok()).toBeTruthy(); + const body = (await res.json()) as { interface?: InterfaceConfig }; + return body.interface ?? {}; +} + +async function storedOverrides( + request: APIRequestContext, + admin: Session, + userId: string, +): Promise | null> { + const res = await request.get(configPath(userId), { headers: admin.headers }); + if (res.status() === 404) { + return null; + } + expect(res.ok()).toBeTruthy(); + const body = (await res.json()) as { config?: { overrides?: Record } }; + return body.config?.overrides ?? {}; +} + +async function clearOverrides( + request: APIRequestContext, + admin: Session, + userId: string, +): Promise { + const res = await request.delete(configPath(userId), { headers: admin.headers }); + expect([200, 204, 404]).toContain(res.status()); +} + +test.describe('Principal config override validation', () => { + test.describe.configure({ mode: 'serial' }); + + test('an invalid field in a whole-document write is rejected and nothing is stored @scenario:config-override-invalid-put-rejected', async ({ + request, + }) => { + const { admin, target } = await sessions(request); + await clearOverrides(request, admin, target.userId); + try { + const res = await request.put(configPath(target.userId), { + headers: admin.headers, + data: { overrides: { interface: { contextCost: 'yes', customWelcome: 'rejected' } } }, + }); + expect(res.status()).toBe(400); + const body = (await res.json()) as { error: string; issues: Array<{ path: string }> }; + expect(body.error).toContain('interface.contextCost'); + expect(body.issues.map((issue) => issue.path)).toEqual(['interface.contextCost']); + + expect(await storedOverrides(request, admin, target.userId)).toBeNull(); + const iface = await readInterface(request, target); + expect(iface.contextCost).toBe(true); + expect(iface.customWelcome).not.toBe('rejected'); + } finally { + await clearOverrides(request, admin, target.userId); + } + }); + + test('an invalid field in a field patch is rejected and the stored override is unchanged @scenario:config-override-invalid-patch-rejected', async ({ + request, + }) => { + const { admin, target } = await sessions(request); + await clearOverrides(request, admin, target.userId); + try { + const seeded = await request.put(configPath(target.userId), { + headers: admin.headers, + data: { overrides: { interface: { customWelcome: 'kept' } } }, + }); + expect(seeded.ok()).toBeTruthy(); + + const res = await request.patch(`${configPath(target.userId)}/fields`, { + headers: admin.headers, + data: { + entries: [ + { fieldPath: 'interface.customWelcome', value: 'replaced' }, + { fieldPath: 'interface.contextCost', value: 'yes' }, + ], + }, + }); + expect(res.status()).toBe(400); + const body = (await res.json()) as { issues: Array<{ path: string }> }; + expect(body.issues.map((issue) => issue.path)).toEqual(['interface.contextCost']); + + expect(await storedOverrides(request, admin, target.userId)).toEqual({ + interface: { customWelcome: 'kept' }, + }); + } finally { + await clearOverrides(request, admin, target.userId); + } + }); + + test('a valid partial section override is accepted and merged over the base @scenario:config-override-valid-partial-accepted', async ({ + request, + }) => { + const { admin, target } = await sessions(request); + await clearOverrides(request, admin, target.userId); + const marker = `welcome-${Date.now()}`; + try { + const res = await request.put(configPath(target.userId), { + headers: admin.headers, + data: { overrides: { interface: { customWelcome: marker } } }, + }); + expect(res.ok()).toBeTruthy(); + + await expect + .poll(async () => (await readInterface(request, target)).customWelcome, { + timeout: 30000, + intervals: [500, 1000, 2000], + }) + .toBe(marker); + expect((await readInterface(request, target)).contextCost).toBe(true); + } finally { + await clearOverrides(request, admin, target.userId); + } + }); + + test('an invalid override already stored leaves the base value in place @scenario:config-override-stored-invalid-keeps-base', async ({ + request, + }) => { + const { admin, target } = await sessions(request); + await clearOverrides(request, admin, target.userId); + const marker = `stored-${Date.now()}`; + try { + const seeded = await request.put(configPath(target.userId), { + headers: admin.headers, + data: { overrides: { interface: { customWelcome: 'seed' } } }, + }); + expect(seeded.ok()).toBeTruthy(); + + /** Written straight to the collection, as a document stored before write validation. */ + const written = await withMongo((db) => + db + .collection('configs') + .updateOne( + { principalType: 'user', principalId: target.userId }, + { $set: { 'overrides.interface.contextCost': 'yes' } }, + ), + ); + expect(written.matchedCount).toBe(1); + + /** A valid patch on the same document invalidates the merged-config cache. */ + const patched = await request.patch(`${configPath(target.userId)}/fields`, { + headers: admin.headers, + data: { entries: [{ fieldPath: 'interface.customWelcome', value: marker }] }, + }); + expect(patched.ok()).toBeTruthy(); + + await expect + .poll(async () => (await readInterface(request, target)).customWelcome, { + timeout: 30000, + intervals: [500, 1000, 2000], + }) + .toBe(marker); + expect((await readInterface(request, target)).contextCost).toBe(true); + } finally { + await clearOverrides(request, admin, target.userId); + } + }); +}); diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index 11c16c4e93f..dcc7612a90e 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -2434,4 +2434,99 @@ describe('createAdminConfigHandlers', () => { } }); }); + + describe('override validation against configSchema', () => { + it('rejects a whole-document write with an invalid field and stores nothing', async () => { + const { handlers, deps } = createHandlers(); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { + overrides: { registration: { oauthStateTtlMs: 5 }, interface: { modelSelect: false } }, + }, + }); + const res = mockRes(); + + await handlers.upsertConfigOverrides(req, res); + + expect(res.statusCode).toBe(400); + expect(res.body?.error).toContain('registration.oauthStateTtlMs'); + expect(res.body?.issues).toEqual([ + expect.objectContaining({ path: 'registration.oauthStateTtlMs' }), + ]); + expect(deps.upsertConfig).not.toHaveBeenCalled(); + }); + + it('accepts a partial section whose provided fields are valid', async () => { + const { handlers, deps } = createHandlers(); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { + overrides: { + registration: { oauthStateTtlMs: 120_000 }, + mcpServers: { github: { timeout: 5000 } }, + endpoints: { custom: [{ name: 'groq', apiKey: 'sk-test' }] }, + }, + }, + }); + const res = mockRes(); + + await handlers.upsertConfigOverrides(req, res); + + expect(res.statusCode).toBe(201); + expect(deps.upsertConfig).toHaveBeenCalledTimes(1); + }); + + it('rejects a field patch with an invalid value and writes no entry', async () => { + const { handlers, deps } = createHandlers(); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { + entries: [ + { fieldPath: 'interface.customWelcome', value: 'hi' }, + { fieldPath: 'registration.oauthStateTtlMs', value: 'soon' }, + ], + }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(400); + expect(res.body?.issues).toEqual([ + expect.objectContaining({ path: 'registration.oauthStateTtlMs' }), + ]); + expect(deps.patchConfigFields).not.toHaveBeenCalled(); + }); + + it('validates an object patch as a partial of the addressed section', async () => { + const { handlers, deps } = createHandlers(); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { entries: [{ fieldPath: 'interface.schedules', value: { maxPerUser: 'x' } }] }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(400); + expect(res.body?.issues).toEqual([ + expect.objectContaining({ path: 'interface.schedules.maxPerUser' }), + ]); + expect(deps.patchConfigFields).not.toHaveBeenCalled(); + }); + + it('accepts a secret field cleared with a non-string value', async () => { + const { handlers, deps } = createHandlers(); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { entries: [{ fieldPath: 'ocr.apiKey', value: null }] }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(200); + expect(deps.patchConfigFields).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index ef30bbad4e8..291f3ce493a 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -10,9 +10,10 @@ import { hasProcessMCPServerConfig, isProcessMCPServerConfig, isProcessMCPServerField, + getConfigOverrideIssues, } from 'librechat-data-provider'; import type { AppConfig, ConfigSection, IConfig, SystemCapability } from '@librechat/data-schemas'; -import type { TCustomConfig } from 'librechat-data-provider'; +import type { ConfigOverrideIssue, TCustomConfig } from 'librechat-data-provider'; import type { Types, ClientSession } from 'mongoose'; import type { Response } from 'express'; import type { CapabilityUser } from '~/middleware/capabilities'; @@ -411,6 +412,14 @@ function redactAppConfigForResponse(appConfig: AppConfig): AppConfig { return safeConfig; } +function invalidOverrideResponse(res: Response, issues: ConfigOverrideIssue[]): Response { + const [first] = issues; + return res.status(400).json({ + error: `Invalid config value at ${first.path}: ${first.message}`, + issues, + }); +} + function preservePatchedConfigSecretFields( fields: Record, existingOverrides?: unknown, @@ -715,6 +724,10 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { } const encryptedOverrides = encryptConfigSecrets(filteredOverrides); + const overrideIssues = getConfigOverrideIssues(encryptedOverrides); + if (overrideIssues.length > 0) { + return invalidOverrideResponse(res, overrideIssues); + } const needsExistingSecrets = getConfigSecretSections().some((section) => isConfigSecretPreservablePatch( section, @@ -911,6 +924,12 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { ? await findConfigByPrincipal(principalType, principalId, { includeInactive: true }) : null; const encryptedFields = encryptConfigSecretFields(fields); + const fieldIssues = Object.entries(encryptedFields).flatMap(([fieldPath, value]) => + getConfigOverrideIssues(value, fieldPath), + ); + if (fieldIssues.length > 0) { + return invalidOverrideResponse(res, fieldIssues); + } const preservedFields = preservePatchedConfigSecretFields( encryptedFields, existing?.overrides, diff --git a/packages/api/src/app/service.spec.ts b/packages/api/src/app/service.spec.ts index 16f9306daed..ee813ed8d21 100644 --- a/packages/api/src/app/service.spec.ts +++ b/packages/api/src/app/service.spec.ts @@ -613,7 +613,7 @@ describe('createAppConfigService', () => { getApplicableConfigs: jest.fn().mockResolvedValue([ { priority: 10, - overrides: { endpoints: ['untrusted-override'] }, + overrides: { interface: { modelSelect: false } }, isActive: true, }, ]), @@ -625,7 +625,7 @@ describe('createAppConfigService', () => { expect(config).toEqual( expect.objectContaining({ - endpoints: ['untrusted-override'], + interfaceConfig: { modelSelect: false }, }), ); }); diff --git a/packages/data-provider/src/index.ts b/packages/data-provider/src/index.ts index 92cb7a6cc86..4a3a99afacc 100644 --- a/packages/data-provider/src/index.ts +++ b/packages/data-provider/src/index.ts @@ -3,6 +3,7 @@ export * from './azure'; export * from './bedrock'; export * from './balance'; export * from './config'; +export * from './overrides'; export * from './footer'; export * from './langchain'; export * from './filters'; diff --git a/packages/data-provider/src/overrides.spec.ts b/packages/data-provider/src/overrides.spec.ts new file mode 100644 index 00000000000..9fbfd733eef --- /dev/null +++ b/packages/data-provider/src/overrides.spec.ts @@ -0,0 +1,73 @@ +import { getConfigOverrideIssues } from './overrides'; + +describe('getConfigOverrideIssues', () => { + it('accepts a partial section whose provided fields are valid', () => { + expect( + getConfigOverrideIssues({ + registration: { allowedDomains: ['a.com'] }, + interface: { schedules: { maxPerUser: 2 } }, + mcpServers: { github: { timeout: 5000 } }, + }), + ).toEqual([]); + }); + + it('reports each invalid field by its dot-path', () => { + expect( + getConfigOverrideIssues({ + registration: { oauthStateTtlMs: 5 }, + balance: { enabled: 'yes' }, + mcpServers: { github: { timeout: 'x' } }, + }).map((issue) => issue.path), + ).toEqual(['registration.oauthStateTtlMs', 'balance.enabled', 'mcpServers.github.timeout']); + }); + + it('checks a field path against the schema it addresses', () => { + expect(getConfigOverrideIssues(120_000, 'registration.oauthStateTtlMs')).toEqual([]); + expect(getConfigOverrideIssues(5, 'registration.oauthStateTtlMs')).toEqual([ + expect.objectContaining({ path: 'registration.oauthStateTtlMs' }), + ]); + expect(getConfigOverrideIssues('bad', 'endpoints.custom.0.models')).toEqual([ + expect.objectContaining({ path: 'endpoints.custom.0.models' }), + ]); + }); + + it('matches either form of a union field and reports the shape that matched', () => { + expect(getConfigOverrideIssues(false, 'interface.schedules')).toEqual([]); + expect(getConfigOverrideIssues({ minIntervalMinutes: 5 }, 'interface.schedules')).toEqual([]); + expect( + getConfigOverrideIssues({ maxPerUser: 'x' }, 'interface.schedules').map( + (issue) => issue.path, + ), + ).toEqual(['interface.schedules.maxPerUser']); + expect(getConfigOverrideIssues('x', 'interface.schedules')).toHaveLength(1); + }); + + it('validates merged-by-name array items partially and replaced arrays in full', () => { + expect( + getConfigOverrideIssues({ endpoints: { custom: [{ name: 'groq', baseURL: 'https://a' }] } }), + ).toEqual([]); + expect( + getConfigOverrideIssues({ endpoints: { custom: [{ name: 'groq', models: 5 }] } }).map( + (issue) => issue.path, + ), + ).toEqual(['endpoints.custom.0.models']); + expect( + getConfigOverrideIssues({ modelSpecs: { list: [{ name: 'x' }] } }).map((issue) => issue.path), + ).toEqual(['modelSpecs.list']); + }); + + it('accepts fields the schema does not define and stored secret shapes', () => { + expect(getConfigOverrideIssues({ unknownSection: 5 })).toEqual([]); + expect(getConfigOverrideIssues(3, 'unknown.path')).toEqual([]); + expect( + getConfigOverrideIssues({ + endpoints: { custom: [{ name: 'x', apiKey: 'v3:enc', apiKeyPreview: 'sk-...' }] }, + ocr: { apiKey: '' }, + }), + ).toEqual([]); + }); + + it('rejects null, which would otherwise replace the base value', () => { + expect(getConfigOverrideIssues({ interface: { contextCost: null } })).toHaveLength(1); + }); +}); diff --git a/packages/data-provider/src/overrides.ts b/packages/data-provider/src/overrides.ts new file mode 100644 index 00000000000..4b8968af396 --- /dev/null +++ b/packages/data-provider/src/overrides.ts @@ -0,0 +1,199 @@ +import { ZodFirstPartyTypeKind } from 'zod'; +import type { ZodTypeAny } from 'zod'; +import { configSchema } from './config'; + +export type ConfigOverrideIssue = { + /** Dot-path of the rejected override field, in YAML (`TCustomConfig`) keys. */ + path: string; + message: string; +}; + +type PlainObject = { [key: string]: unknown }; + +/** + * Override arrays merged item-by-item over the base (by `name`) rather than replaced, + * so each item may carry only the fields it changes. + */ +const PARTIAL_ARRAY_PATHS = new Set(['endpoints.custom']); +const MAX_DEPTH = 32; + +function isPlainObject(value: unknown): value is PlainObject { + return value != null && typeof value === 'object' && !Array.isArray(value); +} + +function joinPath(path: string, key: string): string { + return path ? `${path}.${key}` : key; +} + +/** Strips wrappers that do not change which fields an object accepts. */ +function unwrap(schema: ZodTypeAny): ZodTypeAny { + let current = schema; + for (let depth = 0; depth < MAX_DEPTH; depth++) { + const def = current._def; + switch (def.typeName) { + case ZodFirstPartyTypeKind.ZodOptional: + case ZodFirstPartyTypeKind.ZodNullable: + case ZodFirstPartyTypeKind.ZodDefault: + case ZodFirstPartyTypeKind.ZodCatch: + case ZodFirstPartyTypeKind.ZodReadonly: + current = def.innerType; + break; + case ZodFirstPartyTypeKind.ZodEffects: + current = def.schema; + break; + case ZodFirstPartyTypeKind.ZodBranded: + current = def.type; + break; + case ZodFirstPartyTypeKind.ZodPipeline: + current = def.in; + break; + case ZodFirstPartyTypeKind.ZodLazy: + current = def.getter(); + break; + default: + return current; + } + } + return current; +} + +function checkLeaf(schema: ZodTypeAny, value: unknown, path: string): ConfigOverrideIssue[] { + const result = schema.safeParse(value); + if (result.success) { + return []; + } + const [issue] = result.error.issues; + const detail = + issue.path.length > 0 ? `${issue.path.join('.')}: ${issue.message}` : issue.message; + return [{ path, message: detail }]; +} + +/** + * Validates an override value the way it is applied: plain objects are deep-merged over + * the base config, so each provided field is checked against its own schema and absent + * fields are left to the base. Keys the schema does not define are not checked, and + * object-level refinements are skipped because they judge the merged object, not the patch. + */ +function checkPartial( + schema: ZodTypeAny, + value: unknown, + path: string, + depth: number, +): ConfigOverrideIssue[] { + const inner = unwrap(schema); + const def = inner._def; + if (depth >= MAX_DEPTH) { + return []; + } + + if ( + def.typeName === ZodFirstPartyTypeKind.ZodArray && + Array.isArray(value) && + PARTIAL_ARRAY_PATHS.has(path) + ) { + return value.flatMap((item, index) => + checkPartial(def.type, item, joinPath(path, String(index)), depth + 1), + ); + } + + if (!isPlainObject(value)) { + return checkLeaf(schema, value, path); + } + + switch (def.typeName) { + case ZodFirstPartyTypeKind.ZodObject: { + const shape = def.shape() as Record; + return Object.entries(value).flatMap(([key, fieldValue]) => { + const fieldSchema = Object.prototype.hasOwnProperty.call(shape, key) + ? shape[key] + : undefined; + return fieldSchema + ? checkPartial(fieldSchema, fieldValue, joinPath(path, key), depth + 1) + : []; + }); + } + case ZodFirstPartyTypeKind.ZodRecord: + return Object.entries(value).flatMap(([key, fieldValue]) => + checkPartial(def.valueType, fieldValue, joinPath(path, key), depth + 1), + ); + case ZodFirstPartyTypeKind.ZodIntersection: + return [ + ...checkPartial(def.left, value, path, depth + 1), + ...checkPartial(def.right, value, path, depth + 1), + ]; + case ZodFirstPartyTypeKind.ZodUnion: + case ZodFirstPartyTypeKind.ZodDiscriminatedUnion: + return checkOptions(def.options as ZodTypeAny[], value, path, depth); + default: + return checkLeaf(schema, value, path); + } +} + +/** A value matching any option is valid; otherwise report the closest option's issues. */ +function checkOptions( + options: ZodTypeAny[], + value: unknown, + path: string, + depth: number, +): ConfigOverrideIssue[] { + let closest: ConfigOverrideIssue[] | undefined; + let closestRank = Infinity; + for (const option of options) { + const issues = checkPartial(option, value, path, depth + 1); + if (issues.length === 0) { + return issues; + } + /** Ties go to an option whose shape matched, i.e. one that reported a nested field. */ + const rank = issues.length * 2 + (issues.every((issue) => issue.path === path) ? 1 : 0); + if (rank < closestRank) { + closest = issues; + closestRank = rank; + } + } + return closest ?? []; +} + +/** Every schema a dot-path can address; a union contributes each of its options. */ +function resolveSchemas(schema: ZodTypeAny, segments: string[]): ZodTypeAny[] { + if (segments.length === 0) { + return [schema]; + } + const [segment, ...rest] = segments; + const inner = unwrap(schema); + const def = inner._def; + switch (def.typeName) { + case ZodFirstPartyTypeKind.ZodObject: { + const shape = def.shape() as Record; + return Object.prototype.hasOwnProperty.call(shape, segment) + ? resolveSchemas(shape[segment], rest) + : []; + } + case ZodFirstPartyTypeKind.ZodRecord: + return resolveSchemas(def.valueType, rest); + case ZodFirstPartyTypeKind.ZodArray: + return /^\d+$/.test(segment) ? resolveSchemas(def.type, rest) : []; + case ZodFirstPartyTypeKind.ZodIntersection: + return [...resolveSchemas(def.left, segments), ...resolveSchemas(def.right, segments)]; + case ZodFirstPartyTypeKind.ZodUnion: + case ZodFirstPartyTypeKind.ZodDiscriminatedUnion: + return (def.options as ZodTypeAny[]).flatMap((option) => resolveSchemas(option, segments)); + default: + return []; + } +} + +/** + * Checks a principal config override against `configSchema` before it is stored or merged. + * + * `fieldPath` addresses where `value` is written (empty for a whole overrides document). + * Returns one issue per rejected field; an empty list means every field `configSchema` + * defines is valid. Paths the schema does not define are accepted unchanged. + */ +export function getConfigOverrideIssues(value: unknown, fieldPath = ''): ConfigOverrideIssue[] { + const segments = fieldPath ? fieldPath.split('.') : []; + const schemas = resolveSchemas(configSchema, segments); + if (schemas.length === 0) { + return []; + } + return checkOptions(schemas, value, fieldPath, 0); +} diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 594f87b6799..45f2c0ab7d6 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -235,9 +235,12 @@ describe('mergeConfigOverrides', () => { }); it('replaces plain arrays (no merge key) instead of concatenating', () => { - const configs = [fakeConfig({ endpoints: ['anthropic', 'google'] }, 10)]; - const result = mergeConfigOverrides(baseConfig, configs) as unknown as Record; - expect(result.endpoints).toEqual(['anthropic', 'google']); + const base = { registration: { allowedDomains: ['base.com'] } } as unknown as AppConfig; + const configs = [fakeConfig({ registration: { allowedDomains: ['a.com', 'b.com'] } }, 10)]; + const result = mergeConfigOverrides(base, configs) as unknown as { + registration: { allowedDomains: string[] }; + }; + expect(result.registration.allowedDomains).toEqual(['a.com', 'b.com']); }); it('merges endpoints.custom arrays by name instead of replacing', () => { @@ -422,11 +425,11 @@ describe('mergeConfigOverrides', () => { expect(baseConfig).toEqual(original); }); - it('handles null override values', () => { + it('keeps the base value under a null override the schema does not allow', () => { const configs = [fakeConfig({ interface: { modelSelect: null } }, 10)]; const result = mergeConfigOverrides(baseConfig, configs) as unknown as Record; const iface = result.interfaceConfig as Record; - expect(iface.modelSelect).toBeNull(); + expect(iface.modelSelect).toBe(true); }); it('skips configs with no overrides object', () => { @@ -880,6 +883,64 @@ describe('mergeConfigOverrides', () => { }); }); +describe('mergeConfigOverrides: invalid stored overrides', () => { + const base = { + interfaceConfig: { contextCost: true, customWelcome: 'base' }, + registration: { oauthStateTtlMs: 600_000, allowedDomains: ['base.com'] }, + endpoints: { + custom: [{ name: 'groq', baseURL: 'https://base', apiKey: 'k', models: { default: ['m'] } }], + }, + } as unknown as AppConfig; + + it('keeps the base value when a stored override field fails the schema', () => { + const merged = mergeConfigOverrides(base, [ + fakeConfig( + { + interface: { contextCost: 'yes', customWelcome: 'override' }, + registration: { oauthStateTtlMs: 5, allowedDomains: ['override.com'] }, + }, + 10, + ), + ]) as unknown as Record>; + + expect(merged.interfaceConfig).toEqual({ contextCost: true, customWelcome: 'override' }); + expect(merged.registration).toEqual({ + oauthStateTtlMs: 600_000, + allowedDomains: ['override.com'], + }); + }); + + it('drops only the invalid field of a merged array item', () => { + const merged = mergeConfigOverrides(base, [ + fakeConfig( + { endpoints: { custom: [{ name: 'groq', baseURL: 'https://o', models: 5 }] } }, + 10, + ), + ]) as unknown as { endpoints: { custom: Array> } }; + + expect(merged.endpoints.custom).toEqual([ + { name: 'groq', baseURL: 'https://o', apiKey: 'k', models: { default: ['m'] } }, + ]); + }); + + it('lets a lower-priority valid override survive a higher-priority invalid one', () => { + const merged = mergeConfigOverrides(base, [ + fakeConfig({ interface: { contextCost: false } }, 10), + fakeConfig({ interface: { contextCost: 'no' } }, 20, undefined, 'other'), + ]) as unknown as { interfaceConfig: Record }; + + expect(merged.interfaceConfig.contextCost).toBe(false); + }); + + it('leaves fields the schema does not define untouched', () => { + const merged = mergeConfigOverrides(base, [ + fakeConfig({ registration: { enabled: false } }, 10), + ]) as unknown as { registration: Record }; + + expect(merged.registration.enabled).toBe(false); + }); +}); + describe('INTERFACE_PERMISSION_FIELDS', () => { it('contains all expected permission fields', () => { const expected = [ diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 26b7a5b5bb8..7a17f0053e1 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -5,10 +5,12 @@ import { RUNTIME_CONFIG_INTERFACE_FIELDS, PERMISSION_SUB_KEYS, isProcessMCPServerConfig, + getConfigOverrideIssues, } from 'librechat-data-provider'; import type { TCustomConfig } from 'librechat-data-provider'; import type { AppConfig, IConfig } from '~/types'; import { BASE_CONFIG_PRINCIPAL_ID } from '~/admin/capabilities'; +import logger from '~/config/winston'; type AnyObject = { [key: string]: unknown }; @@ -238,6 +240,55 @@ function deepMerge(target: T, source: AnyObject, depth = 0, return result as T; } +function omitPath(target: unknown, segments: string[]): unknown { + const [segment, ...rest] = segments; + if (Array.isArray(target)) { + const index = Number(segment); + if (!Number.isInteger(index) || index < 0 || index >= target.length) { + return target; + } + const next = [...target]; + if (rest.length === 0) { + next.splice(index, 1); + } else { + next[index] = omitPath(next[index], rest); + } + return next; + } + if (target == null || typeof target !== 'object' || !(segment in target)) { + return target; + } + const next = { ...(target as AnyObject) }; + if (rest.length === 0) { + delete next[segment]; + } else { + next[segment] = omitPath(next[segment], rest); + } + return next; +} + +/** + * Drops override fields that fail `configSchema`, so an invalid stored value (written + * before write-time validation, or by an older server) leaves the base value in place + * instead of replacing it. + */ +function stripInvalidOverrides(config: IConfig): AnyObject { + const overrides = config.overrides as AnyObject; + const issues = getConfigOverrideIssues(overrides); + if (issues.length === 0) { + return overrides; + } + let stripped: unknown = overrides; + for (let index = issues.length - 1; index >= 0; index--) { + const { path, message } = issues[index]; + logger.warn( + `[mergeConfigOverrides] Ignoring invalid override "${path}" for ${config.principalType}/${config.principalId}: ${message}`, + ); + stripped = omitPath(stripped, path.split('.')); + } + return stripped as AnyObject; +} + function filterMCPServerOverrides(value: unknown, current: unknown): AnyObject { if (value == null || typeof value !== 'object' || Array.isArray(value)) { return {}; @@ -305,7 +356,7 @@ export function mergeConfigOverrides(baseConfig: AppConfig, configs: IConfig[]): if (config.overrides && typeof config.overrides === 'object') { const remapped: AnyObject = {}; - for (const [key, value] of Object.entries(config.overrides)) { + for (const [key, value] of Object.entries(stripInvalidOverrides(config))) { if ( BASE_ONLY_OVERRIDE_SECTIONS.has(key) || (!isBasePrincipal && BASE_PRINCIPAL_OVERRIDE_SECTIONS.has(key)) From 5860930e285c933399211fa002224c6fefcd1064 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:39:43 +0200 Subject: [PATCH 02/20] fix: require the merge key on partial custom endpoint overrides --- packages/data-provider/src/overrides.spec.ts | 12 +++++++++ packages/data-provider/src/overrides.ts | 25 +++++++++++-------- .../data-schemas/src/app/resolution.spec.ts | 11 ++++++++ 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/packages/data-provider/src/overrides.spec.ts b/packages/data-provider/src/overrides.spec.ts index 9fbfd733eef..895fe14746f 100644 --- a/packages/data-provider/src/overrides.spec.ts +++ b/packages/data-provider/src/overrides.spec.ts @@ -56,6 +56,18 @@ describe('getConfigOverrideIssues', () => { ).toEqual(['modelSpecs.list']); }); + it('requires the merge key on every merged-by-name array item', () => { + expect( + getConfigOverrideIssues({ + endpoints: { custom: [{ baseURL: 'https://a' }, { name: '', models: 5 }] }, + }), + ).toEqual([ + { path: 'endpoints.custom.0', message: 'name: Required' }, + { path: 'endpoints.custom.1', message: 'name: Required' }, + ]); + expect(getConfigOverrideIssues({ baseURL: 'https://a' }, 'endpoints.custom.0')).toEqual([]); + }); + it('accepts fields the schema does not define and stored secret shapes', () => { expect(getConfigOverrideIssues({ unknownSection: 5 })).toEqual([]); expect(getConfigOverrideIssues(3, 'unknown.path')).toEqual([]); diff --git a/packages/data-provider/src/overrides.ts b/packages/data-provider/src/overrides.ts index 4b8968af396..04b218ea582 100644 --- a/packages/data-provider/src/overrides.ts +++ b/packages/data-provider/src/overrides.ts @@ -11,10 +11,10 @@ export type ConfigOverrideIssue = { type PlainObject = { [key: string]: unknown }; /** - * Override arrays merged item-by-item over the base (by `name`) rather than replaced, - * so each item may carry only the fields it changes. + * Override arrays merged item-by-item over the base by a key field rather than replaced, + * so each item may carry only the fields it changes, but must carry its key. */ -const PARTIAL_ARRAY_PATHS = new Set(['endpoints.custom']); +const PARTIAL_ARRAY_KEYS: Record = { 'endpoints.custom': 'name' }; const MAX_DEPTH = 32; function isPlainObject(value: unknown): value is PlainObject { @@ -86,14 +86,17 @@ function checkPartial( return []; } - if ( - def.typeName === ZodFirstPartyTypeKind.ZodArray && - Array.isArray(value) && - PARTIAL_ARRAY_PATHS.has(path) - ) { - return value.flatMap((item, index) => - checkPartial(def.type, item, joinPath(path, String(index)), depth + 1), - ); + const keyField = Object.prototype.hasOwnProperty.call(PARTIAL_ARRAY_KEYS, path) + ? PARTIAL_ARRAY_KEYS[path] + : undefined; + if (def.typeName === ZodFirstPartyTypeKind.ZodArray && Array.isArray(value) && keyField) { + return value.flatMap((item, index) => { + const itemPath = joinPath(path, String(index)); + if (isPlainObject(item) && (typeof item[keyField] !== 'string' || item[keyField] === '')) { + return [{ path: itemPath, message: `${keyField}: Required` }]; + } + return checkPartial(def.type, item, itemPath, depth + 1); + }); } if (!isPlainObject(value)) { diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 45f2c0ab7d6..dd81eb837ff 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -923,6 +923,17 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { ]); }); + it('drops a merged array item that has no merge key', () => { + const merged = mergeConfigOverrides({} as AppConfig, [ + fakeConfig( + { endpoints: { custom: [{ baseURL: 'https://keyless' }, { name: 'kept', baseURL: 'x' }] } }, + 10, + ), + ]) as unknown as { endpoints: { custom: Array> } }; + + expect(merged.endpoints.custom).toEqual([{ name: 'kept', baseURL: 'x' }]); + }); + it('lets a lower-priority valid override survive a higher-priority invalid one', () => { const merged = mergeConfigOverrides(base, [ fakeConfig({ interface: { contextCost: false } }, 10), From 6f6035214a31554b7b735a94bff9e500219ec24c Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 10:56:58 +0200 Subject: [PATCH 03/20] test: register a dedicated user for config override scenarios --- .../config-override-validation.spec.ts | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/e2e/specs/mock/scenarios/config-override-validation.spec.ts b/e2e/specs/mock/scenarios/config-override-validation.spec.ts index cc1eceeff8d..c6a83da187a 100644 --- a/e2e/specs/mock/scenarios/config-override-validation.spec.ts +++ b/e2e/specs/mock/scenarios/config-override-validation.spec.ts @@ -1,13 +1,13 @@ import { expect, test } from '@playwright/test'; import type { APIRequestContext } from '@playwright/test'; -import { getPrimaryE2EUser, getSecondaryE2EUser } from '../../../setup/users.mock'; +import { getPrimaryE2EUser } from '../../../setup/users.mock'; import { withMongo } from '../db'; /** * Principal config overrides are checked against `configSchema`: an invalid field is * rejected when written, and one already stored is ignored when merged, so the * `librechat.yaml` value survives. The primary user (first registered, ADMIN) writes - * overrides for the secondary user, whose own `/api/config` shows the merged result. + * overrides for a user this file registers, whose own `/api/config` shows the merged result. * `interface.contextCost` is `true` in e2e/config/librechat.e2e.yaml. */ @@ -32,9 +32,16 @@ async function login( return { headers: { Authorization: `Bearer ${token}` }, userId: userId as string }; } +/** A user owned by this file, so no other spec's cleanup can remove it mid-run. */ +const targetUser = { + email: `config-override-${Date.now()}-${Math.random().toString(36).slice(2, 8)}@example.com`, + name: 'Config Override Target', + password: 'securepassword789', +}; + async function sessions(request: APIRequestContext): Promise<{ admin: Session; target: Session }> { const admin = await login(request, getPrimaryE2EUser()); - const target = await login(request, getSecondaryE2EUser()); + const target = await login(request, targetUser); return { admin, target }; } @@ -78,6 +85,24 @@ async function clearOverrides( test.describe('Principal config override validation', () => { test.describe.configure({ mode: 'serial' }); + test.beforeAll(async ({ request }) => { + const res = await request.post('/api/auth/register', { + data: { ...targetUser, confirm_password: targetUser.password }, + }); + expect(res.ok()).toBeTruthy(); + }); + + test.afterAll(async () => { + await withMongo(async (db) => { + const user = await db.collection('users').findOne({ email: targetUser.email }); + if (!user) { + return; + } + await db.collection('configs').deleteMany({ principalId: user._id.toString() }); + await db.collection('users').deleteOne({ _id: user._id }); + }); + }); + test('an invalid field in a whole-document write is rejected and nothing is stored @scenario:config-override-invalid-put-rejected', async ({ request, }) => { From f12c9d0ae2496ecc35557bc7fea3fdb0ae1272af Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 11:15:52 +0200 Subject: [PATCH 04/20] test: log in once per worker in config override scenarios --- .../scenarios/config-override-validation.spec.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/e2e/specs/mock/scenarios/config-override-validation.spec.ts b/e2e/specs/mock/scenarios/config-override-validation.spec.ts index c6a83da187a..4a193fa0eb5 100644 --- a/e2e/specs/mock/scenarios/config-override-validation.spec.ts +++ b/e2e/specs/mock/scenarios/config-override-validation.spec.ts @@ -39,10 +39,16 @@ const targetUser = { password: 'securepassword789', }; +let cachedSessions: { admin: Session; target: Session } | undefined; + +/** Logs in once per worker: the harness allows 20 logins per window across all specs. */ async function sessions(request: APIRequestContext): Promise<{ admin: Session; target: Session }> { - const admin = await login(request, getPrimaryE2EUser()); - const target = await login(request, targetUser); - return { admin, target }; + if (!cachedSessions) { + const admin = await login(request, getPrimaryE2EUser()); + const target = await login(request, targetUser); + cachedSessions = { admin, target }; + } + return cachedSessions; } function configPath(userId: string): string { From acd864125abfae40f90a8d566ee3a00a1b7101aa Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 11:40:28 +0200 Subject: [PATCH 05/20] fix: strip invalid overrides under dotted record keys --- packages/data-provider/src/overrides.spec.ts | 21 +++++++++- packages/data-provider/src/overrides.ts | 42 ++++++++++--------- .../data-schemas/src/app/resolution.spec.ts | 13 ++++++ packages/data-schemas/src/app/resolution.ts | 4 +- 4 files changed, 57 insertions(+), 23 deletions(-) diff --git a/packages/data-provider/src/overrides.spec.ts b/packages/data-provider/src/overrides.spec.ts index 895fe14746f..ae1be5f8828 100644 --- a/packages/data-provider/src/overrides.spec.ts +++ b/packages/data-provider/src/overrides.spec.ts @@ -62,12 +62,29 @@ describe('getConfigOverrideIssues', () => { endpoints: { custom: [{ baseURL: 'https://a' }, { name: '', models: 5 }] }, }), ).toEqual([ - { path: 'endpoints.custom.0', message: 'name: Required' }, - { path: 'endpoints.custom.1', message: 'name: Required' }, + { + path: 'endpoints.custom.0', + segments: ['endpoints', 'custom', '0'], + message: 'name: Required', + }, + { + path: 'endpoints.custom.1', + segments: ['endpoints', 'custom', '1'], + message: 'name: Required', + }, ]); expect(getConfigOverrideIssues({ baseURL: 'https://a' }, 'endpoints.custom.0')).toEqual([]); }); + it('keeps a record key that contains a dot as one segment', () => { + expect(getConfigOverrideIssues({ mcpServers: { 'team.prod': { timeout: 'x' } } })).toEqual([ + expect.objectContaining({ + path: 'mcpServers.team.prod.timeout', + segments: ['mcpServers', 'team.prod', 'timeout'], + }), + ]); + }); + it('accepts fields the schema does not define and stored secret shapes', () => { expect(getConfigOverrideIssues({ unknownSection: 5 })).toEqual([]); expect(getConfigOverrideIssues(3, 'unknown.path')).toEqual([]); diff --git a/packages/data-provider/src/overrides.ts b/packages/data-provider/src/overrides.ts index 04b218ea582..3ef6b65339e 100644 --- a/packages/data-provider/src/overrides.ts +++ b/packages/data-provider/src/overrides.ts @@ -5,6 +5,8 @@ import { configSchema } from './config'; export type ConfigOverrideIssue = { /** Dot-path of the rejected override field, in YAML (`TCustomConfig`) keys. */ path: string; + /** The same location as keys, unambiguous when a record key itself contains a dot. */ + segments: string[]; message: string; }; @@ -21,8 +23,8 @@ function isPlainObject(value: unknown): value is PlainObject { return value != null && typeof value === 'object' && !Array.isArray(value); } -function joinPath(path: string, key: string): string { - return path ? `${path}.${key}` : key; +function toIssue(segments: string[], message: string): ConfigOverrideIssue { + return { path: segments.join('.'), segments, message }; } /** Strips wrappers that do not change which fields an object accepts. */ @@ -57,7 +59,7 @@ function unwrap(schema: ZodTypeAny): ZodTypeAny { return current; } -function checkLeaf(schema: ZodTypeAny, value: unknown, path: string): ConfigOverrideIssue[] { +function checkLeaf(schema: ZodTypeAny, value: unknown, segments: string[]): ConfigOverrideIssue[] { const result = schema.safeParse(value); if (result.success) { return []; @@ -65,7 +67,7 @@ function checkLeaf(schema: ZodTypeAny, value: unknown, path: string): ConfigOver const [issue] = result.error.issues; const detail = issue.path.length > 0 ? `${issue.path.join('.')}: ${issue.message}` : issue.message; - return [{ path, message: detail }]; + return [toIssue(segments, detail)]; } /** @@ -77,7 +79,7 @@ function checkLeaf(schema: ZodTypeAny, value: unknown, path: string): ConfigOver function checkPartial( schema: ZodTypeAny, value: unknown, - path: string, + segments: string[], depth: number, ): ConfigOverrideIssue[] { const inner = unwrap(schema); @@ -86,21 +88,22 @@ function checkPartial( return []; } + const path = segments.join('.'); const keyField = Object.prototype.hasOwnProperty.call(PARTIAL_ARRAY_KEYS, path) ? PARTIAL_ARRAY_KEYS[path] : undefined; if (def.typeName === ZodFirstPartyTypeKind.ZodArray && Array.isArray(value) && keyField) { return value.flatMap((item, index) => { - const itemPath = joinPath(path, String(index)); + const itemSegments = [...segments, String(index)]; if (isPlainObject(item) && (typeof item[keyField] !== 'string' || item[keyField] === '')) { - return [{ path: itemPath, message: `${keyField}: Required` }]; + return [toIssue(itemSegments, `${keyField}: Required`)]; } - return checkPartial(def.type, item, itemPath, depth + 1); + return checkPartial(def.type, item, itemSegments, depth + 1); }); } if (!isPlainObject(value)) { - return checkLeaf(schema, value, path); + return checkLeaf(schema, value, segments); } switch (def.typeName) { @@ -111,24 +114,24 @@ function checkPartial( ? shape[key] : undefined; return fieldSchema - ? checkPartial(fieldSchema, fieldValue, joinPath(path, key), depth + 1) + ? checkPartial(fieldSchema, fieldValue, [...segments, key], depth + 1) : []; }); } case ZodFirstPartyTypeKind.ZodRecord: return Object.entries(value).flatMap(([key, fieldValue]) => - checkPartial(def.valueType, fieldValue, joinPath(path, key), depth + 1), + checkPartial(def.valueType, fieldValue, [...segments, key], depth + 1), ); case ZodFirstPartyTypeKind.ZodIntersection: return [ - ...checkPartial(def.left, value, path, depth + 1), - ...checkPartial(def.right, value, path, depth + 1), + ...checkPartial(def.left, value, segments, depth + 1), + ...checkPartial(def.right, value, segments, depth + 1), ]; case ZodFirstPartyTypeKind.ZodUnion: case ZodFirstPartyTypeKind.ZodDiscriminatedUnion: - return checkOptions(def.options as ZodTypeAny[], value, path, depth); + return checkOptions(def.options as ZodTypeAny[], value, segments, depth); default: - return checkLeaf(schema, value, path); + return checkLeaf(schema, value, segments); } } @@ -136,18 +139,19 @@ function checkPartial( function checkOptions( options: ZodTypeAny[], value: unknown, - path: string, + segments: string[], depth: number, ): ConfigOverrideIssue[] { let closest: ConfigOverrideIssue[] | undefined; let closestRank = Infinity; for (const option of options) { - const issues = checkPartial(option, value, path, depth + 1); + const issues = checkPartial(option, value, segments, depth + 1); if (issues.length === 0) { return issues; } /** Ties go to an option whose shape matched, i.e. one that reported a nested field. */ - const rank = issues.length * 2 + (issues.every((issue) => issue.path === path) ? 1 : 0); + const matched = issues.some((issue) => issue.segments.length > segments.length); + const rank = issues.length * 2 + (matched ? 0 : 1); if (rank < closestRank) { closest = issues; closestRank = rank; @@ -198,5 +202,5 @@ export function getConfigOverrideIssues(value: unknown, fieldPath = ''): ConfigO if (schemas.length === 0) { return []; } - return checkOptions(schemas, value, fieldPath, 0); + return checkOptions(schemas, value, segments, 0); } diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index dd81eb837ff..5ed23108f4e 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -934,6 +934,19 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { expect(merged.endpoints.custom).toEqual([{ name: 'kept', baseURL: 'x' }]); }); + it('strips an invalid field under a record key that contains a dot', () => { + const merged = mergeConfigOverrides( + { mcpConfig: { 'team.prod': { url: 'https://mcp', timeout: 1000 } } } as unknown as AppConfig, + [fakeConfig({ mcpServers: { 'team.prod': { timeout: 'x', initTimeout: 500 } } }, 10)], + ) as unknown as { mcpConfig: Record> }; + + expect(merged.mcpConfig['team.prod']).toEqual({ + url: 'https://mcp', + timeout: 1000, + initTimeout: 500, + }); + }); + it('lets a lower-priority valid override survive a higher-priority invalid one', () => { const merged = mergeConfigOverrides(base, [ fakeConfig({ interface: { contextCost: false } }, 10), diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 7a17f0053e1..edfdaefed87 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -280,11 +280,11 @@ function stripInvalidOverrides(config: IConfig): AnyObject { } let stripped: unknown = overrides; for (let index = issues.length - 1; index >= 0; index--) { - const { path, message } = issues[index]; + const { path, segments, message } = issues[index]; logger.warn( `[mergeConfigOverrides] Ignoring invalid override "${path}" for ${config.principalType}/${config.principalId}: ${message}`, ); - stripped = omitPath(stripped, path.split('.')); + stripped = omitPath(stripped, segments); } return stripped as AnyObject; } From 1a676b4a2481fc38de8da88e2fc08856c83ea11c Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 13:16:59 +0200 Subject: [PATCH 06/20] fix: ignore a stored overrides document that is not an object --- packages/data-schemas/src/app/resolution.spec.ts | 9 +++++++++ packages/data-schemas/src/app/resolution.ts | 6 ++++++ 2 files changed, 15 insertions(+) diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 5ed23108f4e..03301f2e02c 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -947,6 +947,15 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { }); }); + it('ignores a stored overrides document that is not an object', () => { + const merged = mergeConfigOverrides(base, [ + fakeConfig(['stray'] as unknown as Record, 10), + ]) as unknown as Record; + + expect(merged).toEqual(base); + expect(merged).not.toHaveProperty('0'); + }); + it('lets a lower-priority valid override survive a higher-priority invalid one', () => { const merged = mergeConfigOverrides(base, [ fakeConfig({ interface: { contextCost: false } }, 10), diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index edfdaefed87..7ebd6fd76ac 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -278,6 +278,12 @@ function stripInvalidOverrides(config: IConfig): AnyObject { if (issues.length === 0) { return overrides; } + if (issues.some((issue) => issue.segments.length === 0)) { + logger.warn( + `[mergeConfigOverrides] Ignoring malformed overrides document for ${config.principalType}/${config.principalId}: ${issues[0].message}`, + ); + return {}; + } let stripped: unknown = overrides; for (let index = issues.length - 1; index >= 0; index--) { const { path, segments, message } = issues[index]; From 7c369ee316cd5302f82025aa6eb248f3f7368709 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 13:43:14 +0200 Subject: [PATCH 07/20] fix: reject indexed patches into merged-by-name override arrays --- packages/api/src/admin/config.handler.spec.ts | 15 +++++++++++++ packages/data-provider/src/overrides.spec.ts | 11 +++++++--- packages/data-provider/src/overrides.ts | 21 +++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index dcc7612a90e..a8676731998 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -2515,6 +2515,21 @@ describe('createAdminConfigHandlers', () => { expect(deps.patchConfigFields).not.toHaveBeenCalled(); }); + it('rejects a patch that addresses a custom endpoint by index', async () => { + const { handlers, deps } = createHandlers(); + const req = mockReq({ + params: { principalType: 'role', principalId: 'admin' }, + body: { entries: [{ fieldPath: 'endpoints.custom.0.models', value: { default: ['m'] } }] }, + }); + const res = mockRes(); + + await handlers.patchConfigField(req, res); + + expect(res.statusCode).toBe(400); + expect(res.body?.error).toContain('endpoints.custom'); + expect(deps.patchConfigFields).not.toHaveBeenCalled(); + }); + it('accepts a secret field cleared with a non-string value', async () => { const { handlers, deps } = createHandlers(); const req = mockReq({ diff --git a/packages/data-provider/src/overrides.spec.ts b/packages/data-provider/src/overrides.spec.ts index ae1be5f8828..8ab767dca7d 100644 --- a/packages/data-provider/src/overrides.spec.ts +++ b/packages/data-provider/src/overrides.spec.ts @@ -26,8 +26,9 @@ describe('getConfigOverrideIssues', () => { expect(getConfigOverrideIssues(5, 'registration.oauthStateTtlMs')).toEqual([ expect.objectContaining({ path: 'registration.oauthStateTtlMs' }), ]); - expect(getConfigOverrideIssues('bad', 'endpoints.custom.0.models')).toEqual([ - expect.objectContaining({ path: 'endpoints.custom.0.models' }), + expect(getConfigOverrideIssues('bad', 'modelSpecs.list.0.name')).toEqual([]); + expect(getConfigOverrideIssues(5, 'modelSpecs.list.0.name')).toEqual([ + expect.objectContaining({ path: 'modelSpecs.list.0.name' }), ]); }); @@ -73,7 +74,11 @@ describe('getConfigOverrideIssues', () => { message: 'name: Required', }, ]); - expect(getConfigOverrideIssues({ baseURL: 'https://a' }, 'endpoints.custom.0')).toEqual([]); + expect(getConfigOverrideIssues({ name: 'groq' }, 'endpoints.custom')).toHaveLength(1); + expect( + getConfigOverrideIssues(['m'], 'endpoints.custom.0.models').map((issue) => issue.path), + ).toEqual(['endpoints.custom.0.models']); + expect(getConfigOverrideIssues({ name: 'groq' }, 'endpoints.custom.0')).toHaveLength(1); }); it('keeps a record key that contains a dot as one segment', () => { diff --git a/packages/data-provider/src/overrides.ts b/packages/data-provider/src/overrides.ts index 3ef6b65339e..e559493a5c5 100644 --- a/packages/data-provider/src/overrides.ts +++ b/packages/data-provider/src/overrides.ts @@ -189,6 +189,23 @@ function resolveSchemas(schema: ZodTypeAny, segments: string[]): ZodTypeAny[] { } } +/** + * An item of a merged-by-key array is found by its key, not its index, so a path into one + * item would store a keyless item that the merge then drops. + */ +function getIndexedItemIssue(segments: string[]): ConfigOverrideIssue | undefined { + for (let end = 1; end < segments.length; end++) { + const arrayPath = segments.slice(0, end).join('.'); + if (Object.prototype.hasOwnProperty.call(PARTIAL_ARRAY_KEYS, arrayPath)) { + return toIssue( + segments, + `Write the whole ${arrayPath} array; its items are merged by ${PARTIAL_ARRAY_KEYS[arrayPath]}`, + ); + } + } + return undefined; +} + /** * Checks a principal config override against `configSchema` before it is stored or merged. * @@ -198,6 +215,10 @@ function resolveSchemas(schema: ZodTypeAny, segments: string[]): ZodTypeAny[] { */ export function getConfigOverrideIssues(value: unknown, fieldPath = ''): ConfigOverrideIssue[] { const segments = fieldPath ? fieldPath.split('.') : []; + const indexedItemIssue = getIndexedItemIssue(segments); + if (indexedItemIssue) { + return [indexedItemIssue]; + } const schemas = resolveSchemas(configSchema, segments); if (schemas.length === 0) { return []; From 81aaf90549ac6d5e29298dcff7669817dee52ed0 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:25:21 +0200 Subject: [PATCH 08/20] fix: match union options by shape, apply supplied-value refinements, reject paths past scalars --- packages/data-provider/src/overrides.spec.ts | 48 ++++ packages/data-provider/src/overrides.ts | 211 +++++++++++++++--- .../data-schemas/src/app/resolution.spec.ts | 20 ++ 3 files changed, 246 insertions(+), 33 deletions(-) diff --git a/packages/data-provider/src/overrides.spec.ts b/packages/data-provider/src/overrides.spec.ts index 8ab767dca7d..7dde64081ad 100644 --- a/packages/data-provider/src/overrides.spec.ts +++ b/packages/data-provider/src/overrides.spec.ts @@ -43,6 +43,54 @@ describe('getConfigOverrideIssues', () => { expect(getConfigOverrideIssues('x', 'interface.schedules')).toHaveLength(1); }); + it('judges an object by the union options that define its keys', () => { + expect( + getConfigOverrideIssues({ memory: { agent: { id: 5 } } }).map((issue) => issue.path), + ).toEqual(['memory.agent.id']); + expect(getConfigOverrideIssues({ memory: { agent: { id: 'agent_1' } } })).toEqual([]); + expect( + getConfigOverrideIssues({ memory: { agent: { provider: 'openAI', model: 'gpt' } } }), + ).toEqual([]); + }); + + it('applies refinements that judge the values an override supplies', () => { + expect( + getConfigOverrideIssues({ + messageFilter: { + pii: { customPatterns: [{ id: 'p1', label: 'Bad', regex: '(unclosed' }] }, + }, + }).map((issue) => issue.path), + ).toEqual(['messageFilter.pii.customPatterns.0']); + expect( + getConfigOverrideIssues({ + messageFilter: { pii: { customPatterns: [{ id: 'p1', label: 'Ok', regex: '\\d+' }] } }, + }), + ).toEqual([]); + expect( + getConfigOverrideIssues( + { web_search: 'yes', temperature: 1 }, + 'endpoints.azureOpenAI.groups.0.addParams', + ).map((issue) => issue.path), + ).toEqual(['endpoints.azureOpenAI.groups.0.addParams.web_search']); + }); + + it('leaves refinements relating fields to the merged result', () => { + expect( + getConfigOverrideIssues({ + cloudfront: { domain: 'https://cdn.example.com', requireSignedAccess: true }, + }), + ).toEqual([]); + expect(getConfigOverrideIssues({ cloudfront: { invalidateOnDelete: true } })).toEqual([]); + }); + + it('rejects a field path that continues past a field holding a value', () => { + expect( + getConfigOverrideIssues(true, 'interface.contextCost.foo').map((issue) => issue.path), + ).toEqual(['interface.contextCost.foo']); + expect(getConfigOverrideIssues(1, 'registration.oauthStateTtlMs.foo')).toHaveLength(1); + expect(getConfigOverrideIssues(1, 'registration.unknownField.foo')).toEqual([]); + }); + it('validates merged-by-name array items partially and replaced arrays in full', () => { expect( getConfigOverrideIssues({ endpoints: { custom: [{ name: 'groq', baseURL: 'https://a' }] } }), diff --git a/packages/data-provider/src/overrides.ts b/packages/data-provider/src/overrides.ts index e559493a5c5..da00e09f9e0 100644 --- a/packages/data-provider/src/overrides.ts +++ b/packages/data-provider/src/overrides.ts @@ -27,34 +27,38 @@ function toIssue(segments: string[], message: string): ConfigOverrideIssue { return { path: segments.join('.'), segments, message }; } +/** One wrapper that does not change which fields an object accepts, or the schema itself. */ +function unwrapOnce(schema: ZodTypeAny): ZodTypeAny { + const def = schema._def; + switch (def.typeName) { + case ZodFirstPartyTypeKind.ZodOptional: + case ZodFirstPartyTypeKind.ZodNullable: + case ZodFirstPartyTypeKind.ZodDefault: + case ZodFirstPartyTypeKind.ZodCatch: + case ZodFirstPartyTypeKind.ZodReadonly: + return def.innerType; + case ZodFirstPartyTypeKind.ZodEffects: + return def.schema; + case ZodFirstPartyTypeKind.ZodBranded: + return def.type; + case ZodFirstPartyTypeKind.ZodPipeline: + return def.in; + case ZodFirstPartyTypeKind.ZodLazy: + return def.getter(); + default: + return schema; + } +} + /** Strips wrappers that do not change which fields an object accepts. */ function unwrap(schema: ZodTypeAny): ZodTypeAny { let current = schema; for (let depth = 0; depth < MAX_DEPTH; depth++) { - const def = current._def; - switch (def.typeName) { - case ZodFirstPartyTypeKind.ZodOptional: - case ZodFirstPartyTypeKind.ZodNullable: - case ZodFirstPartyTypeKind.ZodDefault: - case ZodFirstPartyTypeKind.ZodCatch: - case ZodFirstPartyTypeKind.ZodReadonly: - current = def.innerType; - break; - case ZodFirstPartyTypeKind.ZodEffects: - current = def.schema; - break; - case ZodFirstPartyTypeKind.ZodBranded: - current = def.type; - break; - case ZodFirstPartyTypeKind.ZodPipeline: - current = def.in; - break; - case ZodFirstPartyTypeKind.ZodLazy: - current = def.getter(); - break; - default: - return current; + const next = unwrapOnce(current); + if (next === current) { + return current; } + current = next; } return current; } @@ -70,6 +74,80 @@ function checkLeaf(schema: ZodTypeAny, value: unknown, segments: string[]): Conf return [toIssue(segments, detail)]; } +function hasRefinement(schema: ZodTypeAny): boolean { + let current = schema; + for (let depth = 0; depth < MAX_DEPTH; depth++) { + const def = current._def; + if (def.typeName === ZodFirstPartyTypeKind.ZodEffects && def.effect.type === 'refinement') { + return true; + } + const next = unwrapOnce(current); + if (next === current) { + return false; + } + current = next; + } + return false; +} + +/** + * Whether a refinement issue judges only what the patch supplies. A record entry stands + * alone. In an object, parsing fills defaults for absent fields, so an issue on a scalar + * field may come from a field the base provides; an issue inside a supplied field, or on + * a supplied array (which replaces the base whole), judges the patch's own value. + */ +function judgesSuppliedValue( + inner: ZodTypeAny, + value: PlainObject, + path: Array, +): boolean { + const [head] = path; + if (path.length === 0 || !Object.prototype.hasOwnProperty.call(value, head)) { + return false; + } + const def = inner._def; + if (def.typeName !== ZodFirstPartyTypeKind.ZodObject || path.length > 1) { + return true; + } + const fieldSchema = (def.shape() as Record)[String(head)]; + return ( + fieldSchema != null && unwrap(fieldSchema)._def.typeName === ZodFirstPartyTypeKind.ZodArray + ); +} + +/** + * Applies an object's refinements to a patch of it, keeping only the issues that judge + * what the patch supplies. A rule relating fields reports on the one it finds missing, + * which the base may provide, and a rule on the whole object judges the merged result, + * so both are left out rather than rejecting a valid partial write. + */ +function checkRefinements( + schema: ZodTypeAny, + value: PlainObject, + segments: string[], +): ConfigOverrideIssue[] { + if (!hasRefinement(schema)) { + return []; + } + const result = schema.safeParse(value); + if (result.success) { + return []; + } + const inner = unwrap(schema); + return result.error.issues + .filter((issue) => issue.code === 'custom' && judgesSuppliedValue(inner, value, issue.path)) + .map((issue) => { + const path = issue.path.map(String); + const itemEnd = path.findIndex((segment) => /^\d+$/.test(segment)) + 1; + if (itemEnd === 0 || itemEnd === path.length) { + return toIssue([...segments, ...path], issue.message); + } + /** An array item is removed whole, never left without the field that failed. */ + const detail = `${path.slice(itemEnd).join('.')}: ${issue.message}`; + return toIssue([...segments, ...path.slice(0, itemEnd)], detail); + }); +} + /** * Validates an override value the way it is applied: plain objects are deep-merged over * the base config, so each provided field is checked against its own schema and absent @@ -109,7 +187,7 @@ function checkPartial( switch (def.typeName) { case ZodFirstPartyTypeKind.ZodObject: { const shape = def.shape() as Record; - return Object.entries(value).flatMap(([key, fieldValue]) => { + const issues = Object.entries(value).flatMap(([key, fieldValue]) => { const fieldSchema = Object.prototype.hasOwnProperty.call(shape, key) ? shape[key] : undefined; @@ -117,11 +195,14 @@ function checkPartial( ? checkPartial(fieldSchema, fieldValue, [...segments, key], depth + 1) : []; }); + return issues.length > 0 ? issues : checkRefinements(schema, value, segments); } - case ZodFirstPartyTypeKind.ZodRecord: - return Object.entries(value).flatMap(([key, fieldValue]) => + case ZodFirstPartyTypeKind.ZodRecord: { + const issues = Object.entries(value).flatMap(([key, fieldValue]) => checkPartial(def.valueType, fieldValue, [...segments, key], depth + 1), ); + return issues.length > 0 ? issues : checkRefinements(schema, value, segments); + } case ZodFirstPartyTypeKind.ZodIntersection: return [ ...checkPartial(def.left, value, segments, depth + 1), @@ -135,6 +216,49 @@ function checkPartial( } } +/** How many of an object value's keys a schema defines; records define every key. */ +function countKnownKeys(schema: ZodTypeAny, value: PlainObject, depth: number): number { + const def = unwrap(schema)._def; + if (depth >= MAX_DEPTH) { + return 0; + } + switch (def.typeName) { + case ZodFirstPartyTypeKind.ZodObject: { + const shape = def.shape() as Record; + return Object.keys(value).filter((key) => Object.prototype.hasOwnProperty.call(shape, key)) + .length; + } + case ZodFirstPartyTypeKind.ZodRecord: + return Object.keys(value).length; + case ZodFirstPartyTypeKind.ZodIntersection: + return Math.max( + countKnownKeys(def.left, value, depth + 1), + countKnownKeys(def.right, value, depth + 1), + ); + case ZodFirstPartyTypeKind.ZodUnion: + case ZodFirstPartyTypeKind.ZodDiscriminatedUnion: + return Math.max( + 0, + ...(def.options as ZodTypeAny[]).map((option) => countKnownKeys(option, value, depth + 1)), + ); + default: + return 0; + } +} + +/** + * An object value is judged only by the options that define the most of its keys, so an + * option that ignores every supplied key cannot accept it by reporting nothing. + */ +function getCandidateOptions(options: ZodTypeAny[], value: unknown, depth: number): ZodTypeAny[] { + if (!isPlainObject(value)) { + return options; + } + const counts = options.map((option) => countKnownKeys(option, value, depth)); + const best = Math.max(0, ...counts); + return best === 0 ? options : options.filter((_, index) => counts[index] === best); +} + /** A value matching any option is valid; otherwise report the closest option's issues. */ function checkOptions( options: ZodTypeAny[], @@ -144,7 +268,7 @@ function checkOptions( ): ConfigOverrideIssue[] { let closest: ConfigOverrideIssue[] | undefined; let closestRank = Infinity; - for (const option of options) { + for (const option of getCandidateOptions(options, value, depth)) { const issues = checkPartial(option, value, segments, depth + 1); if (issues.length === 0) { return issues; @@ -160,8 +284,18 @@ function checkOptions( return closest ?? []; } -/** Every schema a dot-path can address; a union contributes each of its options. */ -function resolveSchemas(schema: ZodTypeAny, segments: string[]): ZodTypeAny[] { +/** Merges alternative resolutions: unreachable only when every alternative is. */ +function combineResolutions(results: Array): ZodTypeAny[] | null { + const reachable = results.filter((result): result is ZodTypeAny[] => result !== null); + return reachable.length === 0 ? null : reachable.flat(); +} + +/** + * Every schema a dot-path can address; a union contributes each of its options. An empty + * list means the path leaves the schema through an undefined key, and `null` means it + * continues past a field that holds a value rather than fields. + */ +function resolveSchemas(schema: ZodTypeAny, segments: string[]): ZodTypeAny[] | null { if (segments.length === 0) { return [schema]; } @@ -178,14 +312,22 @@ function resolveSchemas(schema: ZodTypeAny, segments: string[]): ZodTypeAny[] { case ZodFirstPartyTypeKind.ZodRecord: return resolveSchemas(def.valueType, rest); case ZodFirstPartyTypeKind.ZodArray: - return /^\d+$/.test(segment) ? resolveSchemas(def.type, rest) : []; + return /^\d+$/.test(segment) ? resolveSchemas(def.type, rest) : null; case ZodFirstPartyTypeKind.ZodIntersection: - return [...resolveSchemas(def.left, segments), ...resolveSchemas(def.right, segments)]; + return combineResolutions([ + resolveSchemas(def.left, segments), + resolveSchemas(def.right, segments), + ]); case ZodFirstPartyTypeKind.ZodUnion: case ZodFirstPartyTypeKind.ZodDiscriminatedUnion: - return (def.options as ZodTypeAny[]).flatMap((option) => resolveSchemas(option, segments)); - default: + return combineResolutions( + (def.options as ZodTypeAny[]).map((option) => resolveSchemas(option, segments)), + ); + case ZodFirstPartyTypeKind.ZodAny: + case ZodFirstPartyTypeKind.ZodUnknown: return []; + default: + return null; } } @@ -220,6 +362,9 @@ export function getConfigOverrideIssues(value: unknown, fieldPath = ''): ConfigO return [indexedItemIssue]; } const schemas = resolveSchemas(configSchema, segments); + if (schemas === null) { + return [toIssue(segments, 'Path continues past a field that is not an object')]; + } if (schemas.length === 0) { return []; } diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 03301f2e02c..b900cbfc1d3 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -956,6 +956,26 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { expect(merged).not.toHaveProperty('0'); }); + it('drops a replaced-array item that fails a refinement, keeping its valid siblings', () => { + const merged = mergeConfigOverrides({} as AppConfig, [ + fakeConfig( + { + messageFilter: { + pii: { + customPatterns: [ + { id: 'bad', label: 'Bad', regex: '(unclosed' }, + { id: 'ok', label: 'Ok', regex: 'x+' }, + ], + }, + }, + }, + 10, + ), + ]) as unknown as { messageFilter: { pii: { customPatterns: Array<{ id: string }> } } }; + + expect(merged.messageFilter.pii.customPatterns.map((pattern) => pattern.id)).toEqual(['ok']); + }); + it('lets a lower-priority valid override survive a higher-priority invalid one', () => { const merged = mergeConfigOverrides(base, [ fakeConfig({ interface: { contextCost: false } }, 10), From 7daf7e3d84f432a0ddbb932ab57fbdb691512eaa Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:58:21 +0200 Subject: [PATCH 09/20] refactor: validate config overrides on the merged result instead of the patch --- packages/api/src/admin/config.ts | 37 +- .../src/endpoints/config/endpoints.spec.ts | 2 +- packages/data-provider/src/index.ts | 1 - packages/data-provider/src/overrides.spec.ts | 155 -------- packages/data-provider/src/overrides.ts | 372 ------------------ .../data-schemas/src/app/resolution.spec.ts | 176 ++++++++- packages/data-schemas/src/app/resolution.ts | 269 +++++++++++-- 7 files changed, 447 insertions(+), 565 deletions(-) delete mode 100644 packages/data-provider/src/overrides.spec.ts delete mode 100644 packages/data-provider/src/overrides.ts diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index 291f3ce493a..45b1362b397 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -1,4 +1,9 @@ -import { logger, BASE_CONFIG_PRINCIPAL_ID } from '@librechat/data-schemas'; +import { + logger, + getConfigFieldIssues, + getConfigOverrideIssues, + BASE_CONFIG_PRINCIPAL_ID, +} from '@librechat/data-schemas'; import { BASE_PRINCIPAL_CONFIG_SECTIONS, BASE_ONLY_CONFIG_SECTIONS, @@ -10,10 +15,15 @@ import { hasProcessMCPServerConfig, isProcessMCPServerConfig, isProcessMCPServerField, - getConfigOverrideIssues, } from 'librechat-data-provider'; -import type { AppConfig, ConfigSection, IConfig, SystemCapability } from '@librechat/data-schemas'; -import type { ConfigOverrideIssue, TCustomConfig } from 'librechat-data-provider'; +import type { + AppConfig, + ConfigSection, + IConfig, + SystemCapability, + ConfigOverrideIssue, +} from '@librechat/data-schemas'; +import type { TCustomConfig } from 'librechat-data-provider'; import type { Types, ClientSession } from 'mongoose'; import type { Response } from 'express'; import type { CapabilityUser } from '~/middleware/capabilities'; @@ -471,6 +481,15 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { invalidateConfigCaches, } = deps; + /** The deployment's `librechat.yaml` config, which overrides are validated on top of. */ + async function getBaseYamlConfig(tenantId?: string): Promise> { + if (!getAppConfig) { + return {}; + } + const appConfig = await getAppConfig({ tenantId, baseOnly: true }); + return appConfig?.config ?? {}; + } + /** * GET / — List all active config overrides. */ @@ -724,7 +743,10 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { } const encryptedOverrides = encryptConfigSecrets(filteredOverrides); - const overrideIssues = getConfigOverrideIssues(encryptedOverrides); + const overrideIssues = getConfigOverrideIssues( + encryptedOverrides, + await getBaseYamlConfig(user.tenantId), + ); if (overrideIssues.length > 0) { return invalidOverrideResponse(res, overrideIssues); } @@ -924,8 +946,9 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { ? await findConfigByPrincipal(principalType, principalId, { includeInactive: true }) : null; const encryptedFields = encryptConfigSecretFields(fields); - const fieldIssues = Object.entries(encryptedFields).flatMap(([fieldPath, value]) => - getConfigOverrideIssues(value, fieldPath), + const fieldIssues = getConfigFieldIssues( + encryptedFields, + await getBaseYamlConfig(user.tenantId), ); if (fieldIssues.length > 0) { return invalidOverrideResponse(res, fieldIssues); diff --git a/packages/api/src/endpoints/config/endpoints.spec.ts b/packages/api/src/endpoints/config/endpoints.spec.ts index bee644dfd02..2c0446ad024 100644 --- a/packages/api/src/endpoints/config/endpoints.spec.ts +++ b/packages/api/src/endpoints/config/endpoints.spec.ts @@ -453,7 +453,7 @@ describe('createEndpointsConfigService', () => { name: 'FOO', apiKey: '${FOO_KEY}', baseURL: '${FOO_URL}', - models: { fetch: true }, + models: { default: ['foo-model'], fetch: true }, }, ], }, diff --git a/packages/data-provider/src/index.ts b/packages/data-provider/src/index.ts index 4a3a99afacc..92cb7a6cc86 100644 --- a/packages/data-provider/src/index.ts +++ b/packages/data-provider/src/index.ts @@ -3,7 +3,6 @@ export * from './azure'; export * from './bedrock'; export * from './balance'; export * from './config'; -export * from './overrides'; export * from './footer'; export * from './langchain'; export * from './filters'; diff --git a/packages/data-provider/src/overrides.spec.ts b/packages/data-provider/src/overrides.spec.ts deleted file mode 100644 index 7dde64081ad..00000000000 --- a/packages/data-provider/src/overrides.spec.ts +++ /dev/null @@ -1,155 +0,0 @@ -import { getConfigOverrideIssues } from './overrides'; - -describe('getConfigOverrideIssues', () => { - it('accepts a partial section whose provided fields are valid', () => { - expect( - getConfigOverrideIssues({ - registration: { allowedDomains: ['a.com'] }, - interface: { schedules: { maxPerUser: 2 } }, - mcpServers: { github: { timeout: 5000 } }, - }), - ).toEqual([]); - }); - - it('reports each invalid field by its dot-path', () => { - expect( - getConfigOverrideIssues({ - registration: { oauthStateTtlMs: 5 }, - balance: { enabled: 'yes' }, - mcpServers: { github: { timeout: 'x' } }, - }).map((issue) => issue.path), - ).toEqual(['registration.oauthStateTtlMs', 'balance.enabled', 'mcpServers.github.timeout']); - }); - - it('checks a field path against the schema it addresses', () => { - expect(getConfigOverrideIssues(120_000, 'registration.oauthStateTtlMs')).toEqual([]); - expect(getConfigOverrideIssues(5, 'registration.oauthStateTtlMs')).toEqual([ - expect.objectContaining({ path: 'registration.oauthStateTtlMs' }), - ]); - expect(getConfigOverrideIssues('bad', 'modelSpecs.list.0.name')).toEqual([]); - expect(getConfigOverrideIssues(5, 'modelSpecs.list.0.name')).toEqual([ - expect.objectContaining({ path: 'modelSpecs.list.0.name' }), - ]); - }); - - it('matches either form of a union field and reports the shape that matched', () => { - expect(getConfigOverrideIssues(false, 'interface.schedules')).toEqual([]); - expect(getConfigOverrideIssues({ minIntervalMinutes: 5 }, 'interface.schedules')).toEqual([]); - expect( - getConfigOverrideIssues({ maxPerUser: 'x' }, 'interface.schedules').map( - (issue) => issue.path, - ), - ).toEqual(['interface.schedules.maxPerUser']); - expect(getConfigOverrideIssues('x', 'interface.schedules')).toHaveLength(1); - }); - - it('judges an object by the union options that define its keys', () => { - expect( - getConfigOverrideIssues({ memory: { agent: { id: 5 } } }).map((issue) => issue.path), - ).toEqual(['memory.agent.id']); - expect(getConfigOverrideIssues({ memory: { agent: { id: 'agent_1' } } })).toEqual([]); - expect( - getConfigOverrideIssues({ memory: { agent: { provider: 'openAI', model: 'gpt' } } }), - ).toEqual([]); - }); - - it('applies refinements that judge the values an override supplies', () => { - expect( - getConfigOverrideIssues({ - messageFilter: { - pii: { customPatterns: [{ id: 'p1', label: 'Bad', regex: '(unclosed' }] }, - }, - }).map((issue) => issue.path), - ).toEqual(['messageFilter.pii.customPatterns.0']); - expect( - getConfigOverrideIssues({ - messageFilter: { pii: { customPatterns: [{ id: 'p1', label: 'Ok', regex: '\\d+' }] } }, - }), - ).toEqual([]); - expect( - getConfigOverrideIssues( - { web_search: 'yes', temperature: 1 }, - 'endpoints.azureOpenAI.groups.0.addParams', - ).map((issue) => issue.path), - ).toEqual(['endpoints.azureOpenAI.groups.0.addParams.web_search']); - }); - - it('leaves refinements relating fields to the merged result', () => { - expect( - getConfigOverrideIssues({ - cloudfront: { domain: 'https://cdn.example.com', requireSignedAccess: true }, - }), - ).toEqual([]); - expect(getConfigOverrideIssues({ cloudfront: { invalidateOnDelete: true } })).toEqual([]); - }); - - it('rejects a field path that continues past a field holding a value', () => { - expect( - getConfigOverrideIssues(true, 'interface.contextCost.foo').map((issue) => issue.path), - ).toEqual(['interface.contextCost.foo']); - expect(getConfigOverrideIssues(1, 'registration.oauthStateTtlMs.foo')).toHaveLength(1); - expect(getConfigOverrideIssues(1, 'registration.unknownField.foo')).toEqual([]); - }); - - it('validates merged-by-name array items partially and replaced arrays in full', () => { - expect( - getConfigOverrideIssues({ endpoints: { custom: [{ name: 'groq', baseURL: 'https://a' }] } }), - ).toEqual([]); - expect( - getConfigOverrideIssues({ endpoints: { custom: [{ name: 'groq', models: 5 }] } }).map( - (issue) => issue.path, - ), - ).toEqual(['endpoints.custom.0.models']); - expect( - getConfigOverrideIssues({ modelSpecs: { list: [{ name: 'x' }] } }).map((issue) => issue.path), - ).toEqual(['modelSpecs.list']); - }); - - it('requires the merge key on every merged-by-name array item', () => { - expect( - getConfigOverrideIssues({ - endpoints: { custom: [{ baseURL: 'https://a' }, { name: '', models: 5 }] }, - }), - ).toEqual([ - { - path: 'endpoints.custom.0', - segments: ['endpoints', 'custom', '0'], - message: 'name: Required', - }, - { - path: 'endpoints.custom.1', - segments: ['endpoints', 'custom', '1'], - message: 'name: Required', - }, - ]); - expect(getConfigOverrideIssues({ name: 'groq' }, 'endpoints.custom')).toHaveLength(1); - expect( - getConfigOverrideIssues(['m'], 'endpoints.custom.0.models').map((issue) => issue.path), - ).toEqual(['endpoints.custom.0.models']); - expect(getConfigOverrideIssues({ name: 'groq' }, 'endpoints.custom.0')).toHaveLength(1); - }); - - it('keeps a record key that contains a dot as one segment', () => { - expect(getConfigOverrideIssues({ mcpServers: { 'team.prod': { timeout: 'x' } } })).toEqual([ - expect.objectContaining({ - path: 'mcpServers.team.prod.timeout', - segments: ['mcpServers', 'team.prod', 'timeout'], - }), - ]); - }); - - it('accepts fields the schema does not define and stored secret shapes', () => { - expect(getConfigOverrideIssues({ unknownSection: 5 })).toEqual([]); - expect(getConfigOverrideIssues(3, 'unknown.path')).toEqual([]); - expect( - getConfigOverrideIssues({ - endpoints: { custom: [{ name: 'x', apiKey: 'v3:enc', apiKeyPreview: 'sk-...' }] }, - ocr: { apiKey: '' }, - }), - ).toEqual([]); - }); - - it('rejects null, which would otherwise replace the base value', () => { - expect(getConfigOverrideIssues({ interface: { contextCost: null } })).toHaveLength(1); - }); -}); diff --git a/packages/data-provider/src/overrides.ts b/packages/data-provider/src/overrides.ts deleted file mode 100644 index da00e09f9e0..00000000000 --- a/packages/data-provider/src/overrides.ts +++ /dev/null @@ -1,372 +0,0 @@ -import { ZodFirstPartyTypeKind } from 'zod'; -import type { ZodTypeAny } from 'zod'; -import { configSchema } from './config'; - -export type ConfigOverrideIssue = { - /** Dot-path of the rejected override field, in YAML (`TCustomConfig`) keys. */ - path: string; - /** The same location as keys, unambiguous when a record key itself contains a dot. */ - segments: string[]; - message: string; -}; - -type PlainObject = { [key: string]: unknown }; - -/** - * Override arrays merged item-by-item over the base by a key field rather than replaced, - * so each item may carry only the fields it changes, but must carry its key. - */ -const PARTIAL_ARRAY_KEYS: Record = { 'endpoints.custom': 'name' }; -const MAX_DEPTH = 32; - -function isPlainObject(value: unknown): value is PlainObject { - return value != null && typeof value === 'object' && !Array.isArray(value); -} - -function toIssue(segments: string[], message: string): ConfigOverrideIssue { - return { path: segments.join('.'), segments, message }; -} - -/** One wrapper that does not change which fields an object accepts, or the schema itself. */ -function unwrapOnce(schema: ZodTypeAny): ZodTypeAny { - const def = schema._def; - switch (def.typeName) { - case ZodFirstPartyTypeKind.ZodOptional: - case ZodFirstPartyTypeKind.ZodNullable: - case ZodFirstPartyTypeKind.ZodDefault: - case ZodFirstPartyTypeKind.ZodCatch: - case ZodFirstPartyTypeKind.ZodReadonly: - return def.innerType; - case ZodFirstPartyTypeKind.ZodEffects: - return def.schema; - case ZodFirstPartyTypeKind.ZodBranded: - return def.type; - case ZodFirstPartyTypeKind.ZodPipeline: - return def.in; - case ZodFirstPartyTypeKind.ZodLazy: - return def.getter(); - default: - return schema; - } -} - -/** Strips wrappers that do not change which fields an object accepts. */ -function unwrap(schema: ZodTypeAny): ZodTypeAny { - let current = schema; - for (let depth = 0; depth < MAX_DEPTH; depth++) { - const next = unwrapOnce(current); - if (next === current) { - return current; - } - current = next; - } - return current; -} - -function checkLeaf(schema: ZodTypeAny, value: unknown, segments: string[]): ConfigOverrideIssue[] { - const result = schema.safeParse(value); - if (result.success) { - return []; - } - const [issue] = result.error.issues; - const detail = - issue.path.length > 0 ? `${issue.path.join('.')}: ${issue.message}` : issue.message; - return [toIssue(segments, detail)]; -} - -function hasRefinement(schema: ZodTypeAny): boolean { - let current = schema; - for (let depth = 0; depth < MAX_DEPTH; depth++) { - const def = current._def; - if (def.typeName === ZodFirstPartyTypeKind.ZodEffects && def.effect.type === 'refinement') { - return true; - } - const next = unwrapOnce(current); - if (next === current) { - return false; - } - current = next; - } - return false; -} - -/** - * Whether a refinement issue judges only what the patch supplies. A record entry stands - * alone. In an object, parsing fills defaults for absent fields, so an issue on a scalar - * field may come from a field the base provides; an issue inside a supplied field, or on - * a supplied array (which replaces the base whole), judges the patch's own value. - */ -function judgesSuppliedValue( - inner: ZodTypeAny, - value: PlainObject, - path: Array, -): boolean { - const [head] = path; - if (path.length === 0 || !Object.prototype.hasOwnProperty.call(value, head)) { - return false; - } - const def = inner._def; - if (def.typeName !== ZodFirstPartyTypeKind.ZodObject || path.length > 1) { - return true; - } - const fieldSchema = (def.shape() as Record)[String(head)]; - return ( - fieldSchema != null && unwrap(fieldSchema)._def.typeName === ZodFirstPartyTypeKind.ZodArray - ); -} - -/** - * Applies an object's refinements to a patch of it, keeping only the issues that judge - * what the patch supplies. A rule relating fields reports on the one it finds missing, - * which the base may provide, and a rule on the whole object judges the merged result, - * so both are left out rather than rejecting a valid partial write. - */ -function checkRefinements( - schema: ZodTypeAny, - value: PlainObject, - segments: string[], -): ConfigOverrideIssue[] { - if (!hasRefinement(schema)) { - return []; - } - const result = schema.safeParse(value); - if (result.success) { - return []; - } - const inner = unwrap(schema); - return result.error.issues - .filter((issue) => issue.code === 'custom' && judgesSuppliedValue(inner, value, issue.path)) - .map((issue) => { - const path = issue.path.map(String); - const itemEnd = path.findIndex((segment) => /^\d+$/.test(segment)) + 1; - if (itemEnd === 0 || itemEnd === path.length) { - return toIssue([...segments, ...path], issue.message); - } - /** An array item is removed whole, never left without the field that failed. */ - const detail = `${path.slice(itemEnd).join('.')}: ${issue.message}`; - return toIssue([...segments, ...path.slice(0, itemEnd)], detail); - }); -} - -/** - * Validates an override value the way it is applied: plain objects are deep-merged over - * the base config, so each provided field is checked against its own schema and absent - * fields are left to the base. Keys the schema does not define are not checked, and - * object-level refinements are skipped because they judge the merged object, not the patch. - */ -function checkPartial( - schema: ZodTypeAny, - value: unknown, - segments: string[], - depth: number, -): ConfigOverrideIssue[] { - const inner = unwrap(schema); - const def = inner._def; - if (depth >= MAX_DEPTH) { - return []; - } - - const path = segments.join('.'); - const keyField = Object.prototype.hasOwnProperty.call(PARTIAL_ARRAY_KEYS, path) - ? PARTIAL_ARRAY_KEYS[path] - : undefined; - if (def.typeName === ZodFirstPartyTypeKind.ZodArray && Array.isArray(value) && keyField) { - return value.flatMap((item, index) => { - const itemSegments = [...segments, String(index)]; - if (isPlainObject(item) && (typeof item[keyField] !== 'string' || item[keyField] === '')) { - return [toIssue(itemSegments, `${keyField}: Required`)]; - } - return checkPartial(def.type, item, itemSegments, depth + 1); - }); - } - - if (!isPlainObject(value)) { - return checkLeaf(schema, value, segments); - } - - switch (def.typeName) { - case ZodFirstPartyTypeKind.ZodObject: { - const shape = def.shape() as Record; - const issues = Object.entries(value).flatMap(([key, fieldValue]) => { - const fieldSchema = Object.prototype.hasOwnProperty.call(shape, key) - ? shape[key] - : undefined; - return fieldSchema - ? checkPartial(fieldSchema, fieldValue, [...segments, key], depth + 1) - : []; - }); - return issues.length > 0 ? issues : checkRefinements(schema, value, segments); - } - case ZodFirstPartyTypeKind.ZodRecord: { - const issues = Object.entries(value).flatMap(([key, fieldValue]) => - checkPartial(def.valueType, fieldValue, [...segments, key], depth + 1), - ); - return issues.length > 0 ? issues : checkRefinements(schema, value, segments); - } - case ZodFirstPartyTypeKind.ZodIntersection: - return [ - ...checkPartial(def.left, value, segments, depth + 1), - ...checkPartial(def.right, value, segments, depth + 1), - ]; - case ZodFirstPartyTypeKind.ZodUnion: - case ZodFirstPartyTypeKind.ZodDiscriminatedUnion: - return checkOptions(def.options as ZodTypeAny[], value, segments, depth); - default: - return checkLeaf(schema, value, segments); - } -} - -/** How many of an object value's keys a schema defines; records define every key. */ -function countKnownKeys(schema: ZodTypeAny, value: PlainObject, depth: number): number { - const def = unwrap(schema)._def; - if (depth >= MAX_DEPTH) { - return 0; - } - switch (def.typeName) { - case ZodFirstPartyTypeKind.ZodObject: { - const shape = def.shape() as Record; - return Object.keys(value).filter((key) => Object.prototype.hasOwnProperty.call(shape, key)) - .length; - } - case ZodFirstPartyTypeKind.ZodRecord: - return Object.keys(value).length; - case ZodFirstPartyTypeKind.ZodIntersection: - return Math.max( - countKnownKeys(def.left, value, depth + 1), - countKnownKeys(def.right, value, depth + 1), - ); - case ZodFirstPartyTypeKind.ZodUnion: - case ZodFirstPartyTypeKind.ZodDiscriminatedUnion: - return Math.max( - 0, - ...(def.options as ZodTypeAny[]).map((option) => countKnownKeys(option, value, depth + 1)), - ); - default: - return 0; - } -} - -/** - * An object value is judged only by the options that define the most of its keys, so an - * option that ignores every supplied key cannot accept it by reporting nothing. - */ -function getCandidateOptions(options: ZodTypeAny[], value: unknown, depth: number): ZodTypeAny[] { - if (!isPlainObject(value)) { - return options; - } - const counts = options.map((option) => countKnownKeys(option, value, depth)); - const best = Math.max(0, ...counts); - return best === 0 ? options : options.filter((_, index) => counts[index] === best); -} - -/** A value matching any option is valid; otherwise report the closest option's issues. */ -function checkOptions( - options: ZodTypeAny[], - value: unknown, - segments: string[], - depth: number, -): ConfigOverrideIssue[] { - let closest: ConfigOverrideIssue[] | undefined; - let closestRank = Infinity; - for (const option of getCandidateOptions(options, value, depth)) { - const issues = checkPartial(option, value, segments, depth + 1); - if (issues.length === 0) { - return issues; - } - /** Ties go to an option whose shape matched, i.e. one that reported a nested field. */ - const matched = issues.some((issue) => issue.segments.length > segments.length); - const rank = issues.length * 2 + (matched ? 0 : 1); - if (rank < closestRank) { - closest = issues; - closestRank = rank; - } - } - return closest ?? []; -} - -/** Merges alternative resolutions: unreachable only when every alternative is. */ -function combineResolutions(results: Array): ZodTypeAny[] | null { - const reachable = results.filter((result): result is ZodTypeAny[] => result !== null); - return reachable.length === 0 ? null : reachable.flat(); -} - -/** - * Every schema a dot-path can address; a union contributes each of its options. An empty - * list means the path leaves the schema through an undefined key, and `null` means it - * continues past a field that holds a value rather than fields. - */ -function resolveSchemas(schema: ZodTypeAny, segments: string[]): ZodTypeAny[] | null { - if (segments.length === 0) { - return [schema]; - } - const [segment, ...rest] = segments; - const inner = unwrap(schema); - const def = inner._def; - switch (def.typeName) { - case ZodFirstPartyTypeKind.ZodObject: { - const shape = def.shape() as Record; - return Object.prototype.hasOwnProperty.call(shape, segment) - ? resolveSchemas(shape[segment], rest) - : []; - } - case ZodFirstPartyTypeKind.ZodRecord: - return resolveSchemas(def.valueType, rest); - case ZodFirstPartyTypeKind.ZodArray: - return /^\d+$/.test(segment) ? resolveSchemas(def.type, rest) : null; - case ZodFirstPartyTypeKind.ZodIntersection: - return combineResolutions([ - resolveSchemas(def.left, segments), - resolveSchemas(def.right, segments), - ]); - case ZodFirstPartyTypeKind.ZodUnion: - case ZodFirstPartyTypeKind.ZodDiscriminatedUnion: - return combineResolutions( - (def.options as ZodTypeAny[]).map((option) => resolveSchemas(option, segments)), - ); - case ZodFirstPartyTypeKind.ZodAny: - case ZodFirstPartyTypeKind.ZodUnknown: - return []; - default: - return null; - } -} - -/** - * An item of a merged-by-key array is found by its key, not its index, so a path into one - * item would store a keyless item that the merge then drops. - */ -function getIndexedItemIssue(segments: string[]): ConfigOverrideIssue | undefined { - for (let end = 1; end < segments.length; end++) { - const arrayPath = segments.slice(0, end).join('.'); - if (Object.prototype.hasOwnProperty.call(PARTIAL_ARRAY_KEYS, arrayPath)) { - return toIssue( - segments, - `Write the whole ${arrayPath} array; its items are merged by ${PARTIAL_ARRAY_KEYS[arrayPath]}`, - ); - } - } - return undefined; -} - -/** - * Checks a principal config override against `configSchema` before it is stored or merged. - * - * `fieldPath` addresses where `value` is written (empty for a whole overrides document). - * Returns one issue per rejected field; an empty list means every field `configSchema` - * defines is valid. Paths the schema does not define are accepted unchanged. - */ -export function getConfigOverrideIssues(value: unknown, fieldPath = ''): ConfigOverrideIssue[] { - const segments = fieldPath ? fieldPath.split('.') : []; - const indexedItemIssue = getIndexedItemIssue(segments); - if (indexedItemIssue) { - return [indexedItemIssue]; - } - const schemas = resolveSchemas(configSchema, segments); - if (schemas === null) { - return [toIssue(segments, 'Path continues past a field that is not an object')]; - } - if (schemas.length === 0) { - return []; - } - return checkOptions(schemas, value, segments, 0); -} diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index b900cbfc1d3..f39e0155637 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -1,7 +1,8 @@ import { INTERFACE_PERMISSION_FIELDS, PermissionTypes } from 'librechat-data-provider'; +import type { TCustomConfig } from 'librechat-data-provider'; import type { AppConfig, IConfig } from '~/types'; +import { getConfigFieldIssues, getConfigOverrideIssues, mergeConfigOverrides } from './resolution'; import { BASE_CONFIG_PRINCIPAL_ID } from '~/admin/capabilities'; -import { mergeConfigOverrides } from './resolution'; function fakeConfig( overrides: Record, @@ -936,7 +937,10 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { it('strips an invalid field under a record key that contains a dot', () => { const merged = mergeConfigOverrides( - { mcpConfig: { 'team.prod': { url: 'https://mcp', timeout: 1000 } } } as unknown as AppConfig, + { + config: { mcpServers: { 'team.prod': { url: 'https://mcp', timeout: 1000 } } }, + mcpConfig: { 'team.prod': { url: 'https://mcp', timeout: 1000 } }, + } as unknown as AppConfig, [fakeConfig({ mcpServers: { 'team.prod': { timeout: 'x', initTimeout: 500 } } }, 10)], ) as unknown as { mcpConfig: Record> }; @@ -1028,3 +1032,171 @@ describe('INTERFACE_PERMISSION_FIELDS', () => { } }); }); + +describe('getConfigOverrideIssues', () => { + const paths = (issues: Array<{ path: string }>) => issues.map((issue) => issue.path); + + it('accepts a partial section whose supplied fields are valid', () => { + expect( + getConfigOverrideIssues({ + registration: { allowedDomains: ['a.com'] }, + interface: { schedules: { maxPerUser: 2 } }, + mcpServers: { github: { timeout: 5000 } }, + }), + ).toEqual([]); + }); + + it('reports each invalid supplied field by its dot-path', () => { + expect( + paths( + getConfigOverrideIssues({ + registration: { oauthStateTtlMs: 5 }, + balance: { enabled: 'yes' }, + interface: { contextCost: null }, + }), + ), + ).toEqual(['registration.oauthStateTtlMs', 'balance.enabled', 'interface.contextCost']); + }); + + it('judges a union by the branch the value was written for', () => { + expect(paths(getConfigOverrideIssues({ memory: { agent: { id: 5 } } }))).toEqual([ + 'memory.agent.id', + ]); + expect( + paths(getConfigOverrideIssues({ memory: { agent: { id: 5, provider: 'openAI' } } })), + ).toEqual(['memory.agent.id']); + expect(getConfigOverrideIssues({ memory: { agent: { id: 'agent_1' } } })).toEqual([]); + expect( + paths(getConfigOverrideIssues({ interface: { schedules: { maxPerUser: 'x' } } })), + ).toEqual(['interface.schedules.maxPerUser']); + }); + + it('applies refinements to the values an override supplies', () => { + expect( + paths( + getConfigOverrideIssues({ + messageFilter: { + pii: { customPatterns: [{ id: 'p1', label: 'Bad', regex: '(unclosed' }] }, + }, + }), + ), + ).toEqual(['messageFilter.pii.customPatterns.0.regex']); + expect( + paths( + getConfigOverrideIssues({ + cloudfront: { + domain: 'https://cdn.example.com', + imageSigning: 'none', + requireSignedAccess: true, + }, + }), + ), + ).toEqual(['cloudfront.requireSignedAccess']); + }); + + it('leaves a write that relies on the base for related or required fields to the merge', () => { + const base = { + cloudfront: { + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieDomain: '.example.com', + }, + } as Partial; + expect(getConfigOverrideIssues({ cloudfront: { requireSignedAccess: true } }, base)).toEqual( + [], + ); + expect(getConfigOverrideIssues({ endpoints: { azureOpenAI: { assistants: true } } })).toEqual( + [], + ); + expect( + paths( + getConfigOverrideIssues( + { endpoints: { azureOpenAI: { assistants: true } } }, + {}, + { requireComplete: true }, + ), + ), + ).toEqual(['endpoints.azureOpenAI']); + }); + + it('requires the merge key on custom endpoint items and maps merged items back by it', () => { + expect(getConfigOverrideIssues({ endpoints: { custom: [{ baseURL: 'https://a' }] } })).toEqual([ + { + path: 'endpoints.custom.0', + segments: ['endpoints', 'custom', '0'], + message: 'name: Required', + }, + ]); + const base = { + endpoints: { + custom: [ + { name: 'a', apiKey: 'k', baseURL: 'https://a', models: { default: ['m'] } }, + { name: 'b', apiKey: 'k', baseURL: 'https://b', models: { default: ['m'] } }, + ], + }, + } as Partial; + expect( + paths(getConfigOverrideIssues({ endpoints: { custom: [{ name: 'b', models: 5 }] } }, base)), + ).toEqual(['endpoints.custom.0.models']); + }); + + it('keeps a record key that contains a dot as one segment', () => { + expect( + getConfigOverrideIssues({ mcpServers: { 'team.prod': { timeout: 'x' } } }, { + mcpServers: { 'team.prod': { url: 'https://mcp' } }, + } as Partial), + ).toEqual([expect.objectContaining({ segments: ['mcpServers', 'team.prod', 'timeout'] })]); + }); + + it('accepts keys the schema does not define and stored secret shapes', () => { + expect( + getConfigOverrideIssues({ unknownSection: 5, registration: { enabled: false } }), + ).toEqual([]); + expect( + getConfigOverrideIssues({ + endpoints: { + custom: [ + { + name: 'x', + apiKey: 'v3:enc', + apiKeyPreview: 'sk-...', + baseURL: 'https://x', + models: { default: ['m'] }, + }, + ], + }, + ocr: { apiKey: '' }, + }), + ).toEqual([]); + }); + + it('rejects an overrides document that is not an object', () => { + expect(getConfigOverrideIssues(['stray'])).toEqual([ + { path: '', segments: [], message: 'Overrides must be an object' }, + ]); + }); +}); + +describe('getConfigFieldIssues', () => { + it('checks each written path the way the stored override would hold it', () => { + expect(getConfigFieldIssues({ 'registration.oauthStateTtlMs': 120_000 })).toEqual([]); + expect( + getConfigFieldIssues({ 'registration.oauthStateTtlMs': 5 }).map((issue) => issue.path), + ).toEqual(['registration.oauthStateTtlMs']); + }); + + it('rejects a path past a field that holds a value', () => { + expect( + getConfigFieldIssues({ 'interface.contextCost.foo': true }).map((issue) => issue.path), + ).toEqual(['interface.contextCost']); + expect(getConfigFieldIssues({ 'registration.unknownField.foo': 1 })).toEqual([]); + }); + + it('rejects an indexed write into a merged-by-name array', () => { + expect( + getConfigFieldIssues({ 'endpoints.custom.0.models': { default: ['m'] } }).map( + (issue) => issue.path, + ), + ).toEqual(['endpoints.custom']); + }); +}); diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 7ebd6fd76ac..3ba8b96361e 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -5,7 +5,7 @@ import { RUNTIME_CONFIG_INTERFACE_FIELDS, PERMISSION_SUB_KEYS, isProcessMCPServerConfig, - getConfigOverrideIssues, + configSchema, } from 'librechat-data-provider'; import type { TCustomConfig } from 'librechat-data-provider'; import type { AppConfig, IConfig } from '~/types'; @@ -15,6 +15,7 @@ import logger from '~/config/winston'; type AnyObject = { [key: string]: unknown }; const MAX_MERGE_DEPTH = 10; +const MAX_STRIP_PASSES = 4; const UNSAFE_KEYS = new Set(['__proto__', 'constructor', 'prototype']); /** Filters are a fail-closed security boundary even during mixed-package rollouts. */ const BASE_ONLY_OVERRIDE_SECTIONS = new Set(['filters', ...BASE_ONLY_CONFIG_SECTIONS]); @@ -240,6 +241,212 @@ function deepMerge(target: T, source: AnyObject, depth = 0, return result as T; } +export type ConfigOverrideIssue = { + /** Dot-path of the override node the issue is attributed to, in YAML (`TCustomConfig`) keys. */ + path: string; + /** The same location as keys, unambiguous when a record key itself contains a dot. */ + segments: string[]; + message: string; +}; + +type IssuePath = Array; + +function isPlainObject(value: unknown): value is AnyObject { + return value != null && typeof value === 'object' && !Array.isArray(value); +} + +function hasOwn(target: object, key: string): boolean { + return Object.prototype.hasOwnProperty.call(target, key); +} + +function toIssue(segments: string[], message: string): ConfigOverrideIssue { + return { path: segments.join('.'), segments, message }; +} + +/** + * The override node an issue in the merged config belongs to: the deepest node the + * overrides supply on the issue's path. Items of merged-by-key arrays are matched by their + * key, since the merged index differs from the override's. `undefined` means the overrides + * did not supply anything on the path, so the issue is the base's own. + */ +function attributeIssue( + overrides: AnyObject, + merged: AnyObject, + issuePath: IssuePath, +): string[] | undefined { + const segments: string[] = []; + let node: unknown = overrides; + let mergedNode: unknown = merged; + for (const part of issuePath) { + const key = String(part); + if (Array.isArray(node)) { + const arrayPath = segments.join('.'); + const keyField = hasOwn(ARRAY_MERGE_KEYS, arrayPath) + ? ARRAY_MERGE_KEYS[arrayPath] + : undefined; + const mergedItem = Array.isArray(mergedNode) ? mergedNode[Number(key)] : undefined; + const index = keyField + ? node.findIndex( + (item) => + isPlainObject(item) && + isPlainObject(mergedItem) && + item[keyField] === mergedItem[keyField], + ) + : Number(key); + if (keyField && index < 0) { + return undefined; + } + if (!Number.isInteger(index) || index < 0 || index >= node.length) { + break; + } + segments.push(String(index)); + node = node[index]; + mergedNode = mergedItem; + continue; + } + if (!isPlainObject(node) || !hasOwn(node, key)) { + break; + } + segments.push(key); + node = node[key]; + mergedNode = isPlainObject(mergedNode) ? mergedNode[key] : undefined; + } + return segments.length > 0 ? segments : undefined; +} + +/** Items of a merged-by-key array need their key: the merge drops an item without one. */ +function getKeylessItemIssues(overrides: AnyObject): ConfigOverrideIssue[] { + return Object.entries(ARRAY_MERGE_KEYS).flatMap(([arrayPath, keyField]) => { + const segments = arrayPath.split('.'); + let node: unknown = overrides; + for (const segment of segments) { + node = isPlainObject(node) && hasOwn(node, segment) ? node[segment] : undefined; + } + if (!Array.isArray(node)) { + return []; + } + return node.flatMap((item, index) => + isPlainObject(item) && (typeof item[keyField] !== 'string' || item[keyField] === '') + ? [toIssue([...segments, String(index)], `${keyField}: Required`)] + : [], + ); + }); +} + +type SchemaIssue = NonNullable< + ReturnType['error'] +>['issues'][number]; + +/** + * A union reports one issue at its own path; the branch whose failures are fewest, then + * deepest, is the shape the value was written for, so its issues locate the actual fault. + */ +function expandUnionIssues(issues: SchemaIssue[], depth = 0): SchemaIssue[] { + return issues.flatMap((issue) => { + if (issue.code !== 'invalid_union' || depth >= MAX_MERGE_DEPTH) { + return [issue]; + } + const branches = issue.unionErrors.map((error) => error.issues); + const closest = branches.reduce((best, branch) => { + if (!best || branch.length < best.length) { + return branch; + } + const depthOf = (list: SchemaIssue[]) => Math.max(0, ...list.map((i) => i.path.length)); + return branch.length === best.length && depthOf(branch) > depthOf(best) ? branch : best; + }, undefined); + return closest && closest.length > 0 ? expandUnionIssues(closest, depth + 1) : [issue]; + }); +} + +export interface ConfigOverrideCheckOptions { + /** + * Whether an issue the overrides caused only by leaving something out counts: a required + * field or a related field that another override layer may still supply. Off for a + * single write, which is judged only on the values it supplies; on at merge, where the + * accumulated config is final. + */ + requireComplete?: boolean; +} + +/** + * Checks config overrides the way they apply: merged over `base` (YAML-shaped), each + * section they touch parsed with its `configSchema` schema, and every failure attributed + * to the override node that caused it. Unions, refinements, required fields and defaults + * are therefore judged on the merged result, not on the patch alone. Keys the schema does + * not define are accepted unchanged. + */ +export function getConfigOverrideIssues( + overrides: unknown, + base: Partial = {}, + options: ConfigOverrideCheckOptions = {}, +): ConfigOverrideIssue[] { + if (!isPlainObject(overrides)) { + return [toIssue([], 'Overrides must be an object')]; + } + const merged = deepMerge(base as AnyObject, overrides); + const issues = getKeylessItemIssues(overrides); + const seen = new Set(issues.map((issue) => issue.path)); + const shape = configSchema.shape; + for (const section of Object.keys(overrides)) { + if (!hasOwn(shape, section)) { + continue; + } + const result = shape[section as keyof typeof shape].safeParse(merged[section]); + if (result.success) { + continue; + } + for (const issue of expandUnionIssues(result.error.issues)) { + if (issue.code === 'unrecognized_keys') { + continue; + } + const issuePath: IssuePath = [section, ...issue.path]; + const segments = attributeIssue(overrides, merged, issuePath); + if (!segments || (!options.requireComplete && segments.length < issuePath.length)) { + continue; + } + const path = segments.join('.'); + if (seen.has(path)) { + continue; + } + seen.add(path); + const detail = issuePath.length > segments.length ? `${issuePath.join('.')}: ` : ''; + issues.push(toIssue(segments, `${detail}${issue.message}`)); + } + } + return issues; +} + +/** Sets a dot-path the way a Mongo `$set` on `overrides.` does, without mutating. */ +function setPath(target: unknown, segments: string[], value: unknown): unknown { + if (segments.length === 0) { + return value; + } + const [segment, ...rest] = segments; + if (Array.isArray(target) && /^\d+$/.test(segment)) { + const next = [...target]; + next[Number(segment)] = setPath(next[Number(segment)], rest, value); + return next; + } + const next: AnyObject = isPlainObject(target) ? { ...target } : {}; + next[segment] = setPath(next[segment], rest, value); + return next; +} + +/** + * Checks dot-path field writes over the base, building the override the way a Mongo + * `$set` on each `overrides.` would. + */ +export function getConfigFieldIssues( + fields: Record, + base: Partial = {}, +): ConfigOverrideIssue[] { + const candidate = Object.entries(fields).reduce( + (current, [fieldPath, value]) => setPath(current, fieldPath.split('.'), value), + {}, + ); + return getConfigOverrideIssues(candidate, base); +} + function omitPath(target: unknown, segments: string[]): unknown { const [segment, ...rest] = segments; if (Array.isArray(target)) { @@ -255,10 +462,10 @@ function omitPath(target: unknown, segments: string[]): unknown { } return next; } - if (target == null || typeof target !== 'object' || !(segment in target)) { + if (!isPlainObject(target) || !hasOwn(target, segment)) { return target; } - const next = { ...(target as AnyObject) }; + const next = { ...target }; if (rest.length === 0) { delete next[segment]; } else { @@ -268,31 +475,33 @@ function omitPath(target: unknown, segments: string[]): unknown { } /** - * Drops override fields that fail `configSchema`, so an invalid stored value (written - * before write-time validation, or by an older server) leaves the base value in place - * instead of replacing it. + * Drops the override nodes that make the merged config fail `configSchema`, so an invalid + * stored value (written before write-time validation, by an older server, or one that only + * fails once layered over other overrides) leaves the value beneath it in place. */ -function stripInvalidOverrides(config: IConfig): AnyObject { - const overrides = config.overrides as AnyObject; - const issues = getConfigOverrideIssues(overrides); - if (issues.length === 0) { - return overrides; - } - if (issues.some((issue) => issue.segments.length === 0)) { - logger.warn( - `[mergeConfigOverrides] Ignoring malformed overrides document for ${config.principalType}/${config.principalId}: ${issues[0].message}`, - ); - return {}; - } - let stripped: unknown = overrides; - for (let index = issues.length - 1; index >= 0; index--) { - const { path, segments, message } = issues[index]; - logger.warn( - `[mergeConfigOverrides] Ignoring invalid override "${path}" for ${config.principalType}/${config.principalId}: ${message}`, - ); - stripped = omitPath(stripped, segments); +function stripInvalidOverrides(config: IConfig, base: Partial): AnyObject { + const principal = `${config.principalType}/${config.principalId}`; + let stripped: unknown = config.overrides; + /** Removing a field can leave its parent incomplete, so check again until nothing fails. */ + for (let pass = 0; pass < MAX_STRIP_PASSES; pass++) { + const issues = getConfigOverrideIssues(stripped, base, { requireComplete: true }); + if (issues.length === 0) { + return stripped as AnyObject; + } + if (issues.some((issue) => issue.segments.length === 0)) { + logger.warn(`[mergeConfigOverrides] Ignoring malformed overrides document for ${principal}`); + return {}; + } + for (let index = issues.length - 1; index >= 0; index--) { + const { path, segments, message } = issues[index]; + logger.warn( + `[mergeConfigOverrides] Ignoring invalid override "${path}" for ${principal}: ${message}`, + ); + stripped = omitPath(stripped, segments); + } } - return stripped as AnyObject; + logger.warn(`[mergeConfigOverrides] Ignoring overrides for ${principal}: still invalid`); + return {}; } function filterMCPServerOverrides(value: unknown, current: unknown): AnyObject { @@ -346,6 +555,8 @@ export function mergeConfigOverrides(baseConfig: AppConfig, configs: IConfig[]): const sorted = [...configs].sort((a, b) => a.priority - b.priority); let merged = { ...baseConfig }; + /** The YAML-shaped config the next override lands on, for validating it in place. */ + let raw: Partial = baseConfig.config ?? {}; for (const config of sorted) { const isBasePrincipal = config.principalId?.toString() === BASE_CONFIG_PRINCIPAL_ID; if (Array.isArray(config.tombstones)) { @@ -356,19 +567,22 @@ export function mergeConfigOverrides(baseConfig: AppConfig, configs: IConfig[]): (isBasePrincipal || !BASE_PRINCIPAL_OVERRIDE_SECTIONS.has(path.split('.')[0])) ) { merged = deleteConfigPath(merged, remapOverridePath(path)); + raw = deletePath(raw as AnyObject, path) as Partial; } } } if (config.overrides && typeof config.overrides === 'object') { const remapped: AnyObject = {}; - for (const [key, value] of Object.entries(stripInvalidOverrides(config))) { + const applied: AnyObject = {}; + for (const [key, value] of Object.entries(stripInvalidOverrides(config, raw))) { if ( BASE_ONLY_OVERRIDE_SECTIONS.has(key) || (!isBasePrincipal && BASE_PRINCIPAL_OVERRIDE_SECTIONS.has(key)) ) { continue; } + applied[key] = value; const mappedKey = OVERRIDE_KEY_MAP[key as keyof typeof OVERRIDE_KEY_MAP] ?? key; if (mappedKey === 'mcpConfig') { remapped[mappedKey] = filterMCPServerOverrides( @@ -416,6 +630,7 @@ export function mergeConfigOverrides(baseConfig: AppConfig, configs: IConfig[]): } } merged = deepMerge(merged, remapped); + raw = deepMerge(raw as AnyObject, applied) as Partial; } } From 7dd0f95a1d15f7ba7122249878b592d87da452a4 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:59:19 +0200 Subject: [PATCH 10/20] test: cover record refinements on written config fields --- .../data-schemas/src/app/resolution.spec.ts | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index f39e0155637..64b22e8a86f 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -1185,6 +1185,40 @@ describe('getConfigFieldIssues', () => { ).toEqual(['registration.oauthStateTtlMs']); }); + it('applies a record refinement to a single written key', () => { + const base = { + endpoints: { + azureOpenAI: { + groups: [ + { + group: 'g', + apiKey: 'k', + instanceName: 'i', + version: '2024-02-01', + models: { 'gpt-4o': { deploymentName: 'gpt-4o' } }, + }, + ], + }, + }, + } as Partial; + expect( + getConfigFieldIssues( + { 'endpoints.azureOpenAI.groups': base.endpoints?.azureOpenAI?.groups }, + base, + ), + ).toEqual([]); + expect( + getConfigFieldIssues( + { + 'endpoints.azureOpenAI.groups': [ + { ...base.endpoints?.azureOpenAI?.groups?.[0], addParams: { web_search: 'yes' } }, + ], + }, + base, + ).map((issue) => issue.path), + ).toEqual(['endpoints.azureOpenAI.groups.0.addParams.web_search']); + }); + it('rejects a path past a field that holds a value', () => { expect( getConfigFieldIssues({ 'interface.contextCost.foo': true }).map((issue) => issue.path), From 24eff6a4fad57bf022ea376c64e48288b7a6926d Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 15:05:53 +0200 Subject: [PATCH 11/20] fix: validate config field writes on top of the stored override --- packages/api/src/admin/config.handler.spec.ts | 3 +- packages/api/src/admin/config.ts | 9 +++--- .../data-schemas/src/app/resolution.spec.ts | 31 +++++++++++++++++++ packages/data-schemas/src/app/resolution.ts | 18 ++++++++--- 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index a8676731998..fcc3e30da4f 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -1455,7 +1455,8 @@ describe('createAdminConfigHandlers', () => { expect(res.statusCode).toBe(200); const [, , , , priorityArg] = deps.patchConfigFields.mock.calls[0]; expect(priorityArg).toBe(999); - expect(deps.findConfigByPrincipal).not.toHaveBeenCalled(); + /** Read once to validate the write on top of the stored fields, not for its priority. */ + expect(deps.findConfigByPrincipal).toHaveBeenCalledTimes(1); }); it('preserves priority 0 when broad caller supplies it', async () => { diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index 45b1362b397..00aaea3eecb 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -946,10 +946,11 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { ? await findConfigByPrincipal(principalType, principalId, { includeInactive: true }) : null; const encryptedFields = encryptConfigSecretFields(fields); - const fieldIssues = getConfigFieldIssues( - encryptedFields, - await getBaseYamlConfig(user.tenantId), - ); + const [stored, baseYaml] = await Promise.all([ + existing ?? findConfigByPrincipal(principalType, principalId, { includeInactive: true }), + getBaseYamlConfig(user.tenantId), + ]); + const fieldIssues = getConfigFieldIssues(encryptedFields, baseYaml, stored?.overrides); if (fieldIssues.length > 0) { return invalidOverrideResponse(res, fieldIssues); } diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 64b22e8a86f..50d9ec88ca0 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -1219,6 +1219,37 @@ describe('getConfigFieldIssues', () => { ).toEqual(['endpoints.azureOpenAI.groups.0.addParams.web_search']); }); + it('judges a write together with the fields the principal already overrides', () => { + const stored = { + cloudfront: { + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieDomain: '.example.com', + }, + }; + expect(getConfigFieldIssues({ 'cloudfront.requireSignedAccess': true }, {}, stored)).toEqual( + [], + ); + expect( + getConfigFieldIssues( + { 'cloudfront.requireSignedAccess': true }, + {}, + { + cloudfront: { ...stored.cloudfront, imageSigning: 'none' }, + }, + ).map((issue) => issue.path), + ).toEqual(['cloudfront.requireSignedAccess']); + expect( + getConfigFieldIssues( + { 'interface.customWelcome': 'hi' }, + {}, + { + interface: { contextCost: 'yes' }, + }, + ), + ).toEqual([]); + }); + it('rejects a path past a field that holds a value', () => { expect( getConfigFieldIssues({ 'interface.contextCost.foo': true }).map((issue) => issue.path), diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 3ba8b96361e..4dc4e888b68 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -432,19 +432,29 @@ function setPath(target: unknown, segments: string[], value: unknown): unknown { return next; } +function isRelatedPath(a: string[], b: string[]): boolean { + const length = Math.min(a.length, b.length); + return a.slice(0, length).every((segment, index) => segment === b[index]); +} + /** - * Checks dot-path field writes over the base, building the override the way a Mongo - * `$set` on each `overrides.` would. + * Checks dot-path field writes on top of the principal's stored overrides, building the + * result the way a Mongo `$set` on each `overrides.` would. Only issues on or around + * the written paths are reported, so an older stored field does not block an unrelated write. */ export function getConfigFieldIssues( fields: Record, base: Partial = {}, + stored?: unknown, ): ConfigOverrideIssue[] { + const written = Object.keys(fields).map((fieldPath) => fieldPath.split('.')); const candidate = Object.entries(fields).reduce( (current, [fieldPath, value]) => setPath(current, fieldPath.split('.'), value), - {}, + isPlainObject(stored) ? stored : {}, + ); + return getConfigOverrideIssues(candidate, base).filter((issue) => + written.some((segments) => isRelatedPath(issue.segments, segments)), ); - return getConfigOverrideIssues(candidate, base); } function omitPath(target: unknown, segments: string[]): unknown { From da6fa25f075d866a76e40afe55ecf5ffa901499d Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 15:28:21 +0200 Subject: [PATCH 12/20] fix: attribute id-addressed issues to their item and keep valid sections after repairs --- packages/api/src/admin/config.ts | 15 ++--- .../data-schemas/src/app/resolution.spec.ts | 51 ++++++++++++++++ packages/data-schemas/src/app/resolution.ts | 60 +++++++++++++++---- 3 files changed, 102 insertions(+), 24 deletions(-) diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index 00aaea3eecb..1cd195b9d4c 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -938,19 +938,12 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { } const requestedPriority = hasBroadManage ? priority : undefined; - const hasObjectValuedSecretPatch = Object.entries(fields).some(([fieldPath, value]) => - isConfigSecretPreservablePatch(fieldPath, value), - ); - const existing = - requestedPriority == null || hasObjectValuedSecretPatch - ? await findConfigByPrincipal(principalType, principalId, { includeInactive: true }) - : null; - const encryptedFields = encryptConfigSecretFields(fields); - const [stored, baseYaml] = await Promise.all([ - existing ?? findConfigByPrincipal(principalType, principalId, { includeInactive: true }), + const [existing, baseYaml] = await Promise.all([ + findConfigByPrincipal(principalType, principalId, { includeInactive: true }), getBaseYamlConfig(user.tenantId), ]); - const fieldIssues = getConfigFieldIssues(encryptedFields, baseYaml, stored?.overrides); + const encryptedFields = encryptConfigSecretFields(fields); + const fieldIssues = getConfigFieldIssues(encryptedFields, baseYaml, existing?.overrides); if (fieldIssues.length > 0) { return invalidOverrideResponse(res, fieldIssues); } diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 50d9ec88ca0..1e9a23cd0f4 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -980,6 +980,25 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { expect(merged.messageFilter.pii.customPatterns.map((pattern) => pattern.id)).toEqual(['ok']); }); + it('keeps valid sections when a cascade of removals outlasts the first passes', () => { + const merged = mergeConfigOverrides(baseConfig, [ + fakeConfig( + { + interface: { customWelcome: 'kept' }, + endpoints: { + azureOpenAI: { + groups: [{ group: 'g', apiKey: 'k', instanceName: 'i', version: 'v', models: 5 }], + }, + }, + }, + 10, + ), + ]) as unknown as { interfaceConfig: Record; endpoints: unknown }; + + expect(merged.interfaceConfig.customWelcome).toBe('kept'); + expect(merged.endpoints).toEqual(baseConfig.endpoints); + }); + it('lets a lower-priority valid override survive a higher-priority invalid one', () => { const merged = mergeConfigOverrides(base, [ fakeConfig({ interface: { contextCost: false } }, 10), @@ -1177,6 +1196,38 @@ describe('getConfigOverrideIssues', () => { }); }); +describe('getConfigOverrideIssues: items addressed by id', () => { + it('attributes a refinement that names an array item by its id to that item', () => { + const environment = (id: string, extra: Record = {}) => ({ + id, + name: id, + type: 'managed', + baseURL: 'https://code.example.com', + ...extra, + }); + const issues = getConfigOverrideIssues({ + endpoints: { + agents: { + statefulCodeSessions: { + allowedEnvironments: ['user'], + environments: [ + environment('worker-a'), + environment('worker-b', { pairing: { workerId: 'w1', tokenEnv: 'TOKEN' } }), + ], + }, + }, + }, + }); + + expect(issues).toContainEqual( + expect.objectContaining({ + path: 'endpoints.agents.statefulCodeSessions.environments.1.pairing', + message: 'Only attached code environments may configure pairing', + }), + ); + }); +}); + describe('getConfigFieldIssues', () => { it('checks each written path the way the stored override would hold it', () => { expect(getConfigFieldIssues({ 'registration.oauthStateTtlMs': 120_000 })).toEqual([]); diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 4dc4e888b68..20bd1f5267a 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -15,7 +15,7 @@ import logger from '~/config/winston'; type AnyObject = { [key: string]: unknown }; const MAX_MERGE_DEPTH = 10; -const MAX_STRIP_PASSES = 4; +const MAX_STRIP_PASSES = 16; const UNSAFE_KEYS = new Set(['__proto__', 'constructor', 'prototype']); /** Filters are a fail-closed security boundary even during mixed-package rollouts. */ const BASE_ONLY_OVERRIDE_SECTIONS = new Set(['filters', ...BASE_ONLY_CONFIG_SECTIONS]); @@ -263,6 +263,29 @@ function toIssue(segments: string[], message: string): ConfigOverrideIssue { return { path: segments.join('.'), segments, message }; } +/** + * The override item an issue path segment names: by the merge key for merged-by-key arrays + * (the merged index differs from the override's), by index, or by `id` for refinements that + * address an item by its identifier. + */ +function findItemIndex( + node: unknown[], + key: string, + keyField: string | undefined, + mergedItem: unknown, +): number { + if (keyField) { + return node.findIndex( + (item) => + isPlainObject(item) && isPlainObject(mergedItem) && item[keyField] === mergedItem[keyField], + ); + } + if (/^\d+$/.test(key)) { + return Number(key); + } + return node.findIndex((item) => isPlainObject(item) && item.id === key); +} + /** * The override node an issue in the merged config belongs to: the deepest node the * overrides supply on the issue's path. Items of merged-by-key arrays are matched by their @@ -285,14 +308,7 @@ function attributeIssue( ? ARRAY_MERGE_KEYS[arrayPath] : undefined; const mergedItem = Array.isArray(mergedNode) ? mergedNode[Number(key)] : undefined; - const index = keyField - ? node.findIndex( - (item) => - isPlainObject(item) && - isPlainObject(mergedItem) && - item[keyField] === mergedItem[keyField], - ) - : Number(key); + const index = findItemIndex(node, key, keyField, mergedItem); if (keyField && index < 0) { return undefined; } @@ -301,7 +317,9 @@ function attributeIssue( } segments.push(String(index)); node = node[index]; - mergedNode = mergedItem; + mergedNode = Array.isArray(mergedNode) + ? mergedNode[keyField ? Number(key) : index] + : undefined; continue; } if (!isPlainObject(node) || !hasOwn(node, key)) { @@ -492,7 +510,10 @@ function omitPath(target: unknown, segments: string[]): unknown { function stripInvalidOverrides(config: IConfig, base: Partial): AnyObject { const principal = `${config.principalType}/${config.principalId}`; let stripped: unknown = config.overrides; - /** Removing a field can leave its parent incomplete, so check again until nothing fails. */ + /** + * Removing a field can leave its parent incomplete, so check again while removals make + * progress. If they stop, only the sections that still fail are dropped. + */ for (let pass = 0; pass < MAX_STRIP_PASSES; pass++) { const issues = getConfigOverrideIssues(stripped, base, { requireComplete: true }); if (issues.length === 0) { @@ -502,6 +523,7 @@ function stripInvalidOverrides(config: IConfig, base: Partial): A logger.warn(`[mergeConfigOverrides] Ignoring malformed overrides document for ${principal}`); return {}; } + const before = stripped; for (let index = issues.length - 1; index >= 0; index--) { const { path, segments, message } = issues[index]; logger.warn( @@ -509,9 +531,21 @@ function stripInvalidOverrides(config: IConfig, base: Partial): A ); stripped = omitPath(stripped, segments); } + if (stripped === before) { + break; + } } - logger.warn(`[mergeConfigOverrides] Ignoring overrides for ${principal}: still invalid`); - return {}; + const failing = new Set( + getConfigOverrideIssues(stripped, base, { requireComplete: true }).map( + (issue) => issue.segments[0], + ), + ); + logger.warn( + `[mergeConfigOverrides] Ignoring still-invalid sections ${[...failing].join(', ')} for ${principal}`, + ); + return Object.fromEntries( + Object.entries(stripped as AnyObject).filter(([section]) => !failing.has(section)), + ); } function filterMCPServerOverrides(value: unknown, current: unknown): AnyObject { From 9b03b2502a6925eef584346e2754312180ee6cbb Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 15:56:29 +0200 Subject: [PATCH 13/20] fix: judge field writes by what they change, over the principal's tombstoned base --- packages/api/src/admin/config.ts | 2 +- .../data-schemas/src/app/resolution.spec.ts | 68 ++++++++++++++++--- packages/data-schemas/src/app/resolution.ts | 28 ++++++-- packages/data-schemas/src/methods/config.ts | 3 +- 4 files changed, 81 insertions(+), 20 deletions(-) diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index 1cd195b9d4c..f6c3ed25132 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -943,7 +943,7 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { getBaseYamlConfig(user.tenantId), ]); const encryptedFields = encryptConfigSecretFields(fields); - const fieldIssues = getConfigFieldIssues(encryptedFields, baseYaml, existing?.overrides); + const fieldIssues = getConfigFieldIssues(encryptedFields, baseYaml, existing); if (fieldIssues.length > 0) { return invalidOverrideResponse(res, fieldIssues); } diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 1e9a23cd0f4..4d57b5769e7 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -1271,22 +1271,26 @@ describe('getConfigFieldIssues', () => { }); it('judges a write together with the fields the principal already overrides', () => { - const stored = { - cloudfront: { - domain: 'https://cdn.example.com', - imageSigning: 'cookies', - cookieDomain: '.example.com', - }, + const cloudfront = { + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieDomain: '.example.com', }; - expect(getConfigFieldIssues({ 'cloudfront.requireSignedAccess': true }, {}, stored)).toEqual( - [], - ); expect( getConfigFieldIssues( { 'cloudfront.requireSignedAccess': true }, {}, { - cloudfront: { ...stored.cloudfront, imageSigning: 'none' }, + overrides: { cloudfront }, + }, + ), + ).toEqual([]); + expect( + getConfigFieldIssues( + { 'cloudfront.requireSignedAccess': true }, + {}, + { + overrides: { cloudfront: { ...cloudfront, imageSigning: 'none' } }, }, ).map((issue) => issue.path), ).toEqual(['cloudfront.requireSignedAccess']); @@ -1295,12 +1299,54 @@ describe('getConfigFieldIssues', () => { { 'interface.customWelcome': 'hi' }, {}, { - interface: { contextCost: 'yes' }, + overrides: { interface: { contextCost: 'yes' } }, }, ), ).toEqual([]); }); + it('reports a stored related field the write makes invalid', () => { + expect( + getConfigFieldIssues( + { 'cloudfront.imageSigning': 'none' }, + {}, + { + overrides: { + cloudfront: { + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieDomain: '.example.com', + requireSignedAccess: true, + }, + }, + }, + ).map((issue) => issue.path), + ).toEqual(['cloudfront.requireSignedAccess']); + }); + + it('validates on a base with the remaining tombstones of the principal applied', () => { + const base = { + cloudfront: { + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieDomain: '.example.com', + }, + } as Partial; + expect(getConfigFieldIssues({ 'cloudfront.requireSignedAccess': true }, base)).toEqual([]); + expect( + getConfigFieldIssues({ 'cloudfront.requireSignedAccess': true }, base, { + overrides: {}, + tombstones: ['cloudfront.imageSigning'], + }).map((issue) => issue.path), + ).toEqual(['cloudfront.requireSignedAccess']); + expect( + getConfigFieldIssues({ 'cloudfront.imageSigning': 'cookies' }, base, { + overrides: { cloudfront: { requireSignedAccess: true } }, + tombstones: ['cloudfront.imageSigning'], + }), + ).toEqual([]); + }); + it('rejects a path past a field that holds a value', () => { expect( getConfigFieldIssues({ 'interface.contextCost.foo': true }).map((issue) => issue.path), diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 20bd1f5267a..61223c0eeef 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -10,6 +10,7 @@ import { import type { TCustomConfig } from 'librechat-data-provider'; import type { AppConfig, IConfig } from '~/types'; import { BASE_CONFIG_PRINCIPAL_ID } from '~/admin/capabilities'; +import { getTombstonePathsToClear } from '~/methods/config'; import logger from '~/config/winston'; type AnyObject = { [key: string]: unknown }; @@ -456,22 +457,35 @@ function isRelatedPath(a: string[], b: string[]): boolean { } /** - * Checks dot-path field writes on top of the principal's stored overrides, building the - * result the way a Mongo `$set` on each `overrides.` would. Only issues on or around - * the written paths are reported, so an older stored field does not block an unrelated write. + * Checks dot-path field writes on top of the principal's stored config, building the + * result the way `patchConfigFields` stores it: a Mongo `$set` on each `overrides.`, + * clearing the tombstones those paths clear. The principal's remaining tombstones are + * applied to the base. An issue is reported when it touches a written path, or when the + * write introduced it elsewhere (a related field the write made invalid); issues the stored + * config already had are left to merge time so they do not block an unrelated write. */ export function getConfigFieldIssues( fields: Record, base: Partial = {}, - stored?: unknown, + stored?: { overrides?: unknown; tombstones?: unknown[] } | null, ): ConfigOverrideIssue[] { const written = Object.keys(fields).map((fieldPath) => fieldPath.split('.')); + const cleared = new Set(Object.keys(fields).flatMap(getTombstonePathsToClear)); + const effectiveBase = (stored?.tombstones ?? []) + .filter((path): path is string => typeof path === 'string' && !cleared.has(path)) + .reduce((current, path) => deletePath(current, path), base as AnyObject); + const storedOverrides = isPlainObject(stored?.overrides) ? stored.overrides : {}; const candidate = Object.entries(fields).reduce( (current, [fieldPath, value]) => setPath(current, fieldPath.split('.'), value), - isPlainObject(stored) ? stored : {}, + storedOverrides, ); - return getConfigOverrideIssues(candidate, base).filter((issue) => - written.some((segments) => isRelatedPath(issue.segments, segments)), + const existing = new Set( + getConfigOverrideIssues(storedOverrides, effectiveBase).map((issue) => issue.path), + ); + return getConfigOverrideIssues(candidate, effectiveBase).filter( + (issue) => + !existing.has(issue.path) || + written.some((segments) => isRelatedPath(issue.segments, segments)), ); } diff --git a/packages/data-schemas/src/methods/config.ts b/packages/data-schemas/src/methods/config.ts index 16f94efd9ea..d3051506ab7 100644 --- a/packages/data-schemas/src/methods/config.ts +++ b/packages/data-schemas/src/methods/config.ts @@ -6,7 +6,8 @@ import type { IConfig } from '~/types'; import { BASE_CONFIG_PRINCIPAL_ID } from '~/admin/capabilities'; import { escapeRegExp } from '~/utils/string'; -function getTombstonePathsToClear(fieldPath: string): string[] { +/** Tombstones a field write clears: the written path and its ancestors below the section. */ +export function getTombstonePathsToClear(fieldPath: string): string[] { const parts = fieldPath.split('.'); if (parts.length <= 1) { return [fieldPath]; From 0b92eb38b0e014eaf047dcbf6dba6905ad06ae78 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 16:20:37 +0200 Subject: [PATCH 14/20] fix: validate whole-document config writes on the principal's tombstoned base --- packages/api/src/admin/config.handler.spec.ts | 29 +++++++++++++++++++ packages/api/src/admin/config.ts | 28 +++++++++++------- .../data-schemas/src/app/resolution.spec.ts | 26 ++++++++++++++++- packages/data-schemas/src/app/resolution.ts | 18 ++++++++++-- 4 files changed, 86 insertions(+), 15 deletions(-) diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index fcc3e30da4f..f018b9fb1b0 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -2457,6 +2457,35 @@ describe('createAdminConfigHandlers', () => { expect(deps.upsertConfig).not.toHaveBeenCalled(); }); + it('validates a whole-document write on the base its retained tombstones leave', async () => { + const base = { + config: { + cloudfront: { domain: 'https://cdn.example.com', imageSigning: 'cookies' }, + }, + }; + const body = { overrides: { cloudfront: { requireSignedAccess: true } } }; + const params = { principalType: 'role', principalId: 'admin' }; + + const kept = createHandlers({ getAppConfig: jest.fn().mockResolvedValue(base) }); + const keptRes = mockRes(); + await kept.handlers.upsertConfigOverrides(mockReq({ params, body }), keptRes); + expect(keptRes.statusCode).toBe(201); + + const tombstoned = createHandlers({ + getAppConfig: jest.fn().mockResolvedValue(base), + findConfigByPrincipal: jest + .fn() + .mockResolvedValue({ overrides: {}, tombstones: ['cloudfront.imageSigning'] }), + }); + const tombstonedRes = mockRes(); + await tombstoned.handlers.upsertConfigOverrides(mockReq({ params, body }), tombstonedRes); + expect(tombstonedRes.statusCode).toBe(400); + expect(tombstonedRes.body?.issues).toEqual([ + expect.objectContaining({ path: 'cloudfront.requireSignedAccess' }), + ]); + expect(tombstoned.deps.upsertConfig).not.toHaveBeenCalled(); + }); + it('accepts a partial section whose provided fields are valid', async () => { const { handlers, deps } = createHandlers(); const req = mockReq({ diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index f6c3ed25132..170ed985b25 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -1,6 +1,7 @@ import { logger, getConfigFieldIssues, + applyConfigTombstones, getConfigOverrideIssues, BASE_CONFIG_PRINCIPAL_ID, } from '@librechat/data-schemas'; @@ -743,13 +744,6 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { } const encryptedOverrides = encryptConfigSecrets(filteredOverrides); - const overrideIssues = getConfigOverrideIssues( - encryptedOverrides, - await getBaseYamlConfig(user.tenantId), - ); - if (overrideIssues.length > 0) { - return invalidOverrideResponse(res, overrideIssues); - } const needsExistingSecrets = getConfigSecretSections().some((section) => isConfigSecretPreservablePatch( section, @@ -759,10 +753,22 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps): { const needsProtectedBaseSections = principalId === BASE_CONFIG_PRINCIPAL_ID && (overrideSections.length > 0 || priority != null); - const existingConfig = - needsExistingSecrets || needsProtectedBaseSections - ? await findConfigByPrincipal(principalType, principalId, { includeInactive: true }) - : null; + const needsExisting = needsExistingSecrets || needsProtectedBaseSections; + const [stored, baseYaml] = await Promise.all([ + overrideSections.length > 0 || needsExisting + ? findConfigByPrincipal(principalType, principalId, { includeInactive: true }) + : null, + overrideSections.length > 0 ? getBaseYamlConfig(user.tenantId) : {}, + ]); + /** A full replace keeps the principal's tombstones, so they shape the base it lands on. */ + const overrideIssues = getConfigOverrideIssues( + encryptedOverrides, + applyConfigTombstones(baseYaml, stored?.tombstones), + ); + if (overrideIssues.length > 0) { + return invalidOverrideResponse(res, overrideIssues); + } + const existingConfig = needsExisting ? stored : null; const preservedOverrides = preserveConfigSecrets( encryptedOverrides, existingConfig?.overrides, diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 4d57b5769e7..a9185e5c2c8 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -1,7 +1,12 @@ import { INTERFACE_PERMISSION_FIELDS, PermissionTypes } from 'librechat-data-provider'; import type { TCustomConfig } from 'librechat-data-provider'; import type { AppConfig, IConfig } from '~/types'; -import { getConfigFieldIssues, getConfigOverrideIssues, mergeConfigOverrides } from './resolution'; +import { + mergeConfigOverrides, + getConfigFieldIssues, + applyConfigTombstones, + getConfigOverrideIssues, +} from './resolution'; import { BASE_CONFIG_PRINCIPAL_ID } from '~/admin/capabilities'; function fakeConfig( @@ -1228,6 +1233,25 @@ describe('getConfigOverrideIssues: items addressed by id', () => { }); }); +describe('applyConfigTombstones', () => { + it('removes tombstoned base paths except the ones a write clears', () => { + const base = { + cloudfront: { domain: 'https://cdn.example.com', imageSigning: 'cookies' }, + } as Partial; + const tombstones = ['cloudfront.imageSigning']; + expect(applyConfigTombstones(base, tombstones)).toEqual({ + cloudfront: { domain: 'https://cdn.example.com' }, + }); + expect(applyConfigTombstones(base, tombstones, new Set(tombstones))).toEqual(base); + expect( + getConfigOverrideIssues( + { cloudfront: { requireSignedAccess: true } }, + applyConfigTombstones(base, tombstones), + ).map((issue) => issue.path), + ).toEqual(['cloudfront.requireSignedAccess']); + }); +}); + describe('getConfigFieldIssues', () => { it('checks each written path the way the stored override would hold it', () => { expect(getConfigFieldIssues({ 'registration.oauthStateTtlMs': 120_000 })).toEqual([]); diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 61223c0eeef..7570c0264e2 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -456,6 +456,20 @@ function isRelatedPath(a: string[], b: string[]): boolean { return a.slice(0, length).every((segment, index) => segment === b[index]); } +/** + * The base a principal's override lands on: `base` without the paths the principal + * tombstones, except those in `cleared` (tombstones the pending write removes). + */ +export function applyConfigTombstones( + base: Partial, + tombstones: unknown[] | undefined, + cleared: Set = new Set(), +): Partial { + return (tombstones ?? []) + .filter((path): path is string => typeof path === 'string' && !cleared.has(path)) + .reduce((current, path) => deletePath(current, path), base as AnyObject); +} + /** * Checks dot-path field writes on top of the principal's stored config, building the * result the way `patchConfigFields` stores it: a Mongo `$set` on each `overrides.`, @@ -471,9 +485,7 @@ export function getConfigFieldIssues( ): ConfigOverrideIssue[] { const written = Object.keys(fields).map((fieldPath) => fieldPath.split('.')); const cleared = new Set(Object.keys(fields).flatMap(getTombstonePathsToClear)); - const effectiveBase = (stored?.tombstones ?? []) - .filter((path): path is string => typeof path === 'string' && !cleared.has(path)) - .reduce((current, path) => deletePath(current, path), base as AnyObject); + const effectiveBase = applyConfigTombstones(base, stored?.tombstones, cleared); const storedOverrides = isPlainObject(stored?.overrides) ? stored.overrides : {}; const candidate = Object.entries(fields).reduce( (current, [fieldPath, value]) => setPath(current, fieldPath.split('.'), value), From fcfeaa722c1781b4804b8ca6f4e4ddb7d508e4ab Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 16:37:25 +0200 Subject: [PATCH 15/20] fix: report config override issues as stable codes instead of schema messages --- .../config-override-validation.spec.ts | 9 +++++--- packages/api/src/admin/config.handler.spec.ts | 11 +++++----- packages/api/src/admin/config.ts | 7 +++--- .../data-schemas/src/app/resolution.spec.ts | 6 ++--- packages/data-schemas/src/app/resolution.ts | 22 +++++++++++-------- 5 files changed, 32 insertions(+), 23 deletions(-) diff --git a/e2e/specs/mock/scenarios/config-override-validation.spec.ts b/e2e/specs/mock/scenarios/config-override-validation.spec.ts index 4a193fa0eb5..75611712f6e 100644 --- a/e2e/specs/mock/scenarios/config-override-validation.spec.ts +++ b/e2e/specs/mock/scenarios/config-override-validation.spec.ts @@ -120,9 +120,12 @@ test.describe('Principal config override validation', () => { data: { overrides: { interface: { contextCost: 'yes', customWelcome: 'rejected' } } }, }); expect(res.status()).toBe(400); - const body = (await res.json()) as { error: string; issues: Array<{ path: string }> }; - expect(body.error).toContain('interface.contextCost'); - expect(body.issues.map((issue) => issue.path)).toEqual(['interface.contextCost']); + const body = (await res.json()) as { + code: string; + issues: Array<{ path: string; code: string }>; + }; + expect(body.code).toBe('CONFIG_OVERRIDE_INVALID'); + expect(body.issues).toEqual([{ path: 'interface.contextCost', code: 'invalid_type' }]); expect(await storedOverrides(request, admin, target.userId)).toBeNull(); const iface = await readInterface(request, target); diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index f018b9fb1b0..c40d02a7377 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -2450,10 +2450,11 @@ describe('createAdminConfigHandlers', () => { await handlers.upsertConfigOverrides(req, res); expect(res.statusCode).toBe(400); - expect(res.body?.error).toContain('registration.oauthStateTtlMs'); - expect(res.body?.issues).toEqual([ - expect.objectContaining({ path: 'registration.oauthStateTtlMs' }), - ]); + expect(res.body).toEqual({ + error: 'Invalid config override', + code: 'CONFIG_OVERRIDE_INVALID', + issues: [{ path: 'registration.oauthStateTtlMs', code: 'too_small' }], + }); expect(deps.upsertConfig).not.toHaveBeenCalled(); }); @@ -2556,7 +2557,7 @@ describe('createAdminConfigHandlers', () => { await handlers.patchConfigField(req, res); expect(res.statusCode).toBe(400); - expect(res.body?.error).toContain('endpoints.custom'); + expect(res.body?.issues).toEqual([{ path: 'endpoints.custom', code: 'invalid_type' }]); expect(deps.patchConfigFields).not.toHaveBeenCalled(); }); diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index 170ed985b25..ddeaffe0e16 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -423,11 +423,12 @@ function redactAppConfigForResponse(appConfig: AppConfig): AppConfig { return safeConfig; } +/** Reports only the paths and stable codes: schema messages can echo the submitted values. */ function invalidOverrideResponse(res: Response, issues: ConfigOverrideIssue[]): Response { - const [first] = issues; return res.status(400).json({ - error: `Invalid config value at ${first.path}: ${first.message}`, - issues, + error: 'Invalid config override', + code: 'CONFIG_OVERRIDE_INVALID', + issues: issues.map(({ path, code }) => ({ path, code })), }); } diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index a9185e5c2c8..9b8193785df 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -1148,7 +1148,7 @@ describe('getConfigOverrideIssues', () => { { path: 'endpoints.custom.0', segments: ['endpoints', 'custom', '0'], - message: 'name: Required', + code: 'missing_merge_key', }, ]); const base = { @@ -1196,7 +1196,7 @@ describe('getConfigOverrideIssues', () => { it('rejects an overrides document that is not an object', () => { expect(getConfigOverrideIssues(['stray'])).toEqual([ - { path: '', segments: [], message: 'Overrides must be an object' }, + { path: '', segments: [], code: 'invalid_document' }, ]); }); }); @@ -1227,7 +1227,7 @@ describe('getConfigOverrideIssues: items addressed by id', () => { expect(issues).toContainEqual( expect.objectContaining({ path: 'endpoints.agents.statefulCodeSessions.environments.1.pairing', - message: 'Only attached code environments may configure pairing', + code: 'custom', }), ); }); diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 7570c0264e2..be541342c86 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -247,7 +247,12 @@ export type ConfigOverrideIssue = { path: string; /** The same location as keys, unambiguous when a record key itself contains a dot. */ segments: string[]; - message: string; + /** + * A stable, machine-readable reason: a zod issue code (`invalid_type`, `custom`, ...), + * `missing_merge_key`, or `invalid_document`. Schema messages are not carried because + * they can echo the submitted values. + */ + code: string; }; type IssuePath = Array; @@ -260,8 +265,8 @@ function hasOwn(target: object, key: string): boolean { return Object.prototype.hasOwnProperty.call(target, key); } -function toIssue(segments: string[], message: string): ConfigOverrideIssue { - return { path: segments.join('.'), segments, message }; +function toIssue(segments: string[], code: string): ConfigOverrideIssue { + return { path: segments.join('.'), segments, code }; } /** @@ -346,7 +351,7 @@ function getKeylessItemIssues(overrides: AnyObject): ConfigOverrideIssue[] { } return node.flatMap((item, index) => isPlainObject(item) && (typeof item[keyField] !== 'string' || item[keyField] === '') - ? [toIssue([...segments, String(index)], `${keyField}: Required`)] + ? [toIssue([...segments, String(index)], 'missing_merge_key')] : [], ); }); @@ -400,7 +405,7 @@ export function getConfigOverrideIssues( options: ConfigOverrideCheckOptions = {}, ): ConfigOverrideIssue[] { if (!isPlainObject(overrides)) { - return [toIssue([], 'Overrides must be an object')]; + return [toIssue([], 'invalid_document')]; } const merged = deepMerge(base as AnyObject, overrides); const issues = getKeylessItemIssues(overrides); @@ -428,8 +433,7 @@ export function getConfigOverrideIssues( continue; } seen.add(path); - const detail = issuePath.length > segments.length ? `${issuePath.join('.')}: ` : ''; - issues.push(toIssue(segments, `${detail}${issue.message}`)); + issues.push(toIssue(segments, issue.code)); } } return issues; @@ -551,9 +555,9 @@ function stripInvalidOverrides(config: IConfig, base: Partial): A } const before = stripped; for (let index = issues.length - 1; index >= 0; index--) { - const { path, segments, message } = issues[index]; + const { path, segments, code } = issues[index]; logger.warn( - `[mergeConfigOverrides] Ignoring invalid override "${path}" for ${principal}: ${message}`, + `[mergeConfigOverrides] Ignoring invalid override "${path}" for ${principal} (${code})`, ); stripped = omitPath(stripped, segments); } From 37c4f9a492120128332638f47414170f0a63be7b Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:46:29 +0200 Subject: [PATCH 16/20] fix: reject indexed writes into keyed arrays and keep filtered MCP servers out of validation state --- .../data-schemas/src/app/resolution.spec.ts | 35 +++++++++++++++++++ packages/data-schemas/src/app/resolution.ts | 20 ++++++++++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 9b8193785df..07fb1223c6e 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -889,6 +889,17 @@ describe('mergeConfigOverrides', () => { }); }); +describe('mergeConfigOverrides: filtered MCP servers', () => { + it('does not let a filtered process-backed server complete a later partial', () => { + const merged = mergeConfigOverrides({} as AppConfig, [ + fakeConfig({ mcpServers: { injected: { type: 'stdio', command: 'node', args: ['x'] } } }, 10), + fakeConfig({ mcpServers: { injected: { title: 'Injected' } } }, 20, undefined, 'other'), + ]) as unknown as { mcpConfig?: Record }; + + expect(merged.mcpConfig?.injected).toBeUndefined(); + }); +}); + describe('mergeConfigOverrides: invalid stored overrides', () => { const base = { interfaceConfig: { contextCost: true, customWelcome: 'base' }, @@ -1385,4 +1396,28 @@ describe('getConfigFieldIssues', () => { ), ).toEqual(['endpoints.custom']); }); + + it('rejects an indexed write into a stored merged-by-name array', () => { + const base = { + endpoints: { + custom: [{ name: 'a', apiKey: 'k', baseURL: 'https://a', models: { default: ['m'] } }], + }, + } as unknown as Partial; + const stored = { overrides: { endpoints: { custom: [{ name: 'a', baseURL: 'https://o' }] } } }; + + expect(getConfigFieldIssues({ 'endpoints.custom.0.name': 'b' }, base, stored)).toEqual([ + { + path: 'endpoints.custom', + segments: ['endpoints', 'custom'], + code: 'indexed_merge_key_write', + }, + ]); + expect( + getConfigFieldIssues( + { 'endpoints.custom': [{ name: 'a', baseURL: 'https://p' }] }, + base, + stored, + ), + ).toEqual([]); + }); }); diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index be541342c86..796a0a86404 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -249,7 +249,7 @@ export type ConfigOverrideIssue = { segments: string[]; /** * A stable, machine-readable reason: a zod issue code (`invalid_type`, `custom`, ...), - * `missing_merge_key`, or `invalid_document`. Schema messages are not carried because + * `missing_merge_key`, `indexed_merge_key_write`, or `invalid_document`. Schema messages are not carried because * they can echo the submitted values. */ code: string; @@ -495,6 +495,22 @@ export function getConfigFieldIssues( (current, [fieldPath, value]) => setPath(current, fieldPath.split('.'), value), storedOverrides, ); + /** + * An item of a merged-by-key array is identified by its key, not its position: an indexed + * write can rename the stored item, which the runtime merge then treats as a new one. + */ + const indexed = Object.keys(ARRAY_MERGE_KEYS).flatMap((arrayPath) => { + const arraySegments = arrayPath.split('.'); + return written.some( + (segments) => + segments.length > arraySegments.length && isRelatedPath(segments, arraySegments), + ) + ? [toIssue(arraySegments, 'indexed_merge_key_write')] + : []; + }); + if (indexed.length > 0) { + return indexed; + } const existing = new Set( getConfigOverrideIssues(storedOverrides, effectiveBase).map((issue) => issue.path), ); @@ -663,6 +679,8 @@ export function mergeConfigOverrides(baseConfig: AppConfig, configs: IConfig[]): value, (merged as unknown as AnyObject)[mappedKey], ); + /** A server the filter removed must not complete a later layer's partial of it. */ + applied[key] = remapped[mappedKey]; } else if ( key === 'interface' && value != null && From 24109293d5ca6438d4421b14196bd2e6dbb29fb2 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 19:00:13 +0200 Subject: [PATCH 17/20] fix: judge each override layer only on the values it supplies --- .../config-override-validation.spec.ts | 35 +++++++++++ packages/api/src/admin/config.handler.spec.ts | 4 +- .../data-schemas/src/app/resolution.spec.ts | 43 ++++++++++---- packages/data-schemas/src/app/resolution.ts | 59 +++++++++++-------- 4 files changed, 104 insertions(+), 37 deletions(-) diff --git a/e2e/specs/mock/scenarios/config-override-validation.spec.ts b/e2e/specs/mock/scenarios/config-override-validation.spec.ts index 75611712f6e..07be48b6767 100644 --- a/e2e/specs/mock/scenarios/config-override-validation.spec.ts +++ b/e2e/specs/mock/scenarios/config-override-validation.spec.ts @@ -236,4 +236,39 @@ test.describe('Principal config override validation', () => { await clearOverrides(request, admin, target.userId); } }); + + test('a lower-priority override completed by a higher-priority one keeps both @scenario:config-override-layers-complete-each-other', async ({ + request, + }) => { + const { admin, target } = await sessions(request); + const rolePath = '/api/admin/config/role/USER'; + await clearOverrides(request, admin, target.userId); + try { + /** The role layer leaves out the required `siteKey`, which the user layer supplies. */ + const role = await request.put(rolePath, { + headers: admin.headers, + data: { priority: 10, overrides: { turnstile: { options: { size: 'compact' } } } }, + }); + expect(role.ok()).toBeTruthy(); + const user = await request.put(configPath(target.userId), { + headers: admin.headers, + data: { priority: 20, overrides: { turnstile: { siteKey: 'layered-site-key' } } }, + }); + expect(user.ok()).toBeTruthy(); + + await expect + .poll( + async () => { + const res = await request.get('/api/config', { headers: target.headers }); + expect(res.ok()).toBeTruthy(); + return ((await res.json()) as { turnstile?: unknown }).turnstile; + }, + { timeout: 30000, intervals: [500, 1000, 2000] }, + ) + .toEqual({ siteKey: 'layered-site-key', options: { size: 'compact' } }); + } finally { + await request.delete(rolePath, { headers: admin.headers }); + await clearOverrides(request, admin, target.userId); + } + }); }); diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index c40d02a7377..b2168470b72 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -2557,7 +2557,9 @@ describe('createAdminConfigHandlers', () => { await handlers.patchConfigField(req, res); expect(res.statusCode).toBe(400); - expect(res.body?.issues).toEqual([{ path: 'endpoints.custom', code: 'invalid_type' }]); + expect(res.body?.issues).toEqual([ + { path: 'endpoints.custom', code: 'indexed_merge_key_write' }, + ]); expect(deps.patchConfigFields).not.toHaveBeenCalled(); }); diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 07fb1223c6e..5a35b01161a 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -890,13 +890,13 @@ describe('mergeConfigOverrides', () => { }); describe('mergeConfigOverrides: filtered MCP servers', () => { - it('does not let a filtered process-backed server complete a later partial', () => { + it('does not let a filtered process-backed server shape a later partial', () => { const merged = mergeConfigOverrides({} as AppConfig, [ fakeConfig({ mcpServers: { injected: { type: 'stdio', command: 'node', args: ['x'] } } }, 10), fakeConfig({ mcpServers: { injected: { title: 'Injected' } } }, 20, undefined, 'other'), ]) as unknown as { mcpConfig?: Record }; - expect(merged.mcpConfig?.injected).toBeUndefined(); + expect(merged.mcpConfig?.injected).toEqual({ title: 'Injected' }); }); }); @@ -996,7 +996,7 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { expect(merged.messageFilter.pii.customPatterns.map((pattern) => pattern.id)).toEqual(['ok']); }); - it('keeps valid sections when a cascade of removals outlasts the first passes', () => { + it('drops a replaced array item left incomplete by a repair and keeps other sections', () => { const merged = mergeConfigOverrides(baseConfig, [ fakeConfig( { @@ -1009,12 +1009,38 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { }, 10, ), - ]) as unknown as { interfaceConfig: Record; endpoints: unknown }; + ]) as unknown as { + interfaceConfig: Record; + endpoints: unknown; + }; expect(merged.interfaceConfig.customWelcome).toBe('kept'); expect(merged.endpoints).toEqual(baseConfig.endpoints); }); + it('keeps a lower layer that relies on a higher layer for a required field', () => { + const merged = mergeConfigOverrides({} as AppConfig, [ + fakeConfig( + { + cloudfront: { + imageSigning: 'cookies', + cookieDomain: '.example.com', + requireSignedAccess: true, + }, + }, + 10, + ), + fakeConfig({ cloudfront: { domain: 'https://cdn.example.com' } }, 20, undefined, 'other'), + ]) as unknown as { cloudfront: Record }; + + expect(merged.cloudfront).toEqual({ + imageSigning: 'cookies', + cookieDomain: '.example.com', + requireSignedAccess: true, + domain: 'https://cdn.example.com', + }); + }); + it('lets a lower-priority valid override survive a higher-priority invalid one', () => { const merged = mergeConfigOverrides(base, [ fakeConfig({ interface: { contextCost: false } }, 10), @@ -1143,15 +1169,6 @@ describe('getConfigOverrideIssues', () => { expect(getConfigOverrideIssues({ endpoints: { azureOpenAI: { assistants: true } } })).toEqual( [], ); - expect( - paths( - getConfigOverrideIssues( - { endpoints: { azureOpenAI: { assistants: true } } }, - {}, - { requireComplete: true }, - ), - ), - ).toEqual(['endpoints.azureOpenAI']); }); it('requires the merge key on custom endpoint items and maps merged items back by it', () => { diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 796a0a86404..61ae3ddca77 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -338,6 +338,24 @@ function attributeIssue( return segments.length > 0 ? segments : undefined; } +/** + * Whether the override node at `segments` is final: at or inside an array the merge replaces + * (any array not merged by key), so no other layer can supply what it leaves out. + */ +function isReplacedArrayNode(overrides: AnyObject, segments: string[]): boolean { + let node: unknown = overrides; + for (let index = 0; index < segments.length; index++) { + node = Array.isArray(node) + ? node[Number(segments[index])] + : (node as AnyObject)[segments[index]]; + const path = segments.slice(0, index + 1).join('.'); + if (Array.isArray(node) && !hasOwn(ARRAY_MERGE_KEYS, path)) { + return true; + } + } + return false; +} + /** Items of a merged-by-key array need their key: the merge drops an item without one. */ function getKeylessItemIssues(overrides: AnyObject): ConfigOverrideIssue[] { return Object.entries(ARRAY_MERGE_KEYS).flatMap(([arrayPath, keyField]) => { @@ -382,27 +400,17 @@ function expandUnionIssues(issues: SchemaIssue[], depth = 0): SchemaIssue[] { }); } -export interface ConfigOverrideCheckOptions { - /** - * Whether an issue the overrides caused only by leaving something out counts: a required - * field or a related field that another override layer may still supply. Off for a - * single write, which is judged only on the values it supplies; on at merge, where the - * accumulated config is final. - */ - requireComplete?: boolean; -} - /** * Checks config overrides the way they apply: merged over `base` (YAML-shaped), each * section they touch parsed with its `configSchema` schema, and every failure attributed * to the override node that caused it. Unions, refinements, required fields and defaults - * are therefore judged on the merged result, not on the patch alone. Keys the schema does - * not define are accepted unchanged. + * are therefore judged on the merged result, not on the patch alone. An issue caused only + * by leaving something out (a required or related field another layer may supply) is not + * reported, except inside an array the merge replaces, where nothing can supply it. Keys the schema does not define are accepted unchanged. */ export function getConfigOverrideIssues( overrides: unknown, base: Partial = {}, - options: ConfigOverrideCheckOptions = {}, ): ConfigOverrideIssue[] { if (!isPlainObject(overrides)) { return [toIssue([], 'invalid_document')]; @@ -425,7 +433,10 @@ export function getConfigOverrideIssues( } const issuePath: IssuePath = [section, ...issue.path]; const segments = attributeIssue(overrides, merged, issuePath); - if (!segments || (!options.requireComplete && segments.length < issuePath.length)) { + if ( + !segments || + (segments.length < issuePath.length && !isReplacedArrayNode(overrides, segments)) + ) { continue; } const path = segments.join('.'); @@ -540,10 +551,13 @@ function omitPath(target: unknown, segments: string[]): unknown { return target; } const next = { ...target }; - if (rest.length === 0) { + const child = rest.length === 0 ? undefined : omitPath(next[segment], rest); + /** A node its repairs emptied supplies nothing, so the value beneath it stays instead. */ + const emptied = child != null && typeof child === 'object' && Object.keys(child).length === 0; + if (rest.length === 0 || emptied) { delete next[segment]; } else { - next[segment] = omitPath(next[segment], rest); + next[segment] = child; } return next; } @@ -551,17 +565,18 @@ function omitPath(target: unknown, segments: string[]): unknown { /** * Drops the override nodes that make the merged config fail `configSchema`, so an invalid * stored value (written before write-time validation, by an older server, or one that only - * fails once layered over other overrides) leaves the value beneath it in place. + * fails once layered over other overrides) leaves the value beneath it in place. A layer + * is not judged on what it leaves out, since a higher-priority layer may supply it. */ function stripInvalidOverrides(config: IConfig, base: Partial): AnyObject { const principal = `${config.principalType}/${config.principalId}`; let stripped: unknown = config.overrides; /** - * Removing a field can leave its parent incomplete, so check again while removals make - * progress. If they stop, only the sections that still fail are dropped. + * Removing a field can make a related field it supplies fail, so check again while + * removals make progress. If they stop, only the sections that still fail are dropped. */ for (let pass = 0; pass < MAX_STRIP_PASSES; pass++) { - const issues = getConfigOverrideIssues(stripped, base, { requireComplete: true }); + const issues = getConfigOverrideIssues(stripped, base); if (issues.length === 0) { return stripped as AnyObject; } @@ -582,9 +597,7 @@ function stripInvalidOverrides(config: IConfig, base: Partial): A } } const failing = new Set( - getConfigOverrideIssues(stripped, base, { requireComplete: true }).map( - (issue) => issue.segments[0], - ), + getConfigOverrideIssues(stripped, base).map((issue) => issue.segments[0]), ); logger.warn( `[mergeConfigOverrides] Ignoring still-invalid sections ${[...failing].join(', ')} for ${principal}`, From 4ef74cc939b7e36256b9687d2ae6679f5b8fa545 Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 19:20:46 +0200 Subject: [PATCH 18/20] fix: reject merged-by-key array items that are not objects --- packages/data-schemas/src/app/resolution.spec.ts | 8 ++++++++ packages/data-schemas/src/app/resolution.ts | 7 +++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 5a35b01161a..c3e490afcb9 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -940,6 +940,14 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { ]); }); + it('drops a merged array item that is not an object', () => { + const merged = mergeConfigOverrides({} as AppConfig, [ + fakeConfig({ endpoints: { custom: [null, 'bad', { name: 'kept', baseURL: 'x' }] } }, 10), + ]) as unknown as { endpoints: { custom: unknown[] } }; + + expect(merged.endpoints.custom).toEqual([{ name: 'kept', baseURL: 'x' }]); + }); + it('drops a merged array item that has no merge key', () => { const merged = mergeConfigOverrides({} as AppConfig, [ fakeConfig( diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 61ae3ddca77..d764859f2ef 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -356,7 +356,10 @@ function isReplacedArrayNode(overrides: AnyObject, segments: string[]): boolean return false; } -/** Items of a merged-by-key array need their key: the merge drops an item without one. */ +/** + * Items of a merged-by-key array must be objects with their key: the merge drops any other + * item, or keeps it as is when nothing lies beneath the array. + */ function getKeylessItemIssues(overrides: AnyObject): ConfigOverrideIssue[] { return Object.entries(ARRAY_MERGE_KEYS).flatMap(([arrayPath, keyField]) => { const segments = arrayPath.split('.'); @@ -368,7 +371,7 @@ function getKeylessItemIssues(overrides: AnyObject): ConfigOverrideIssue[] { return []; } return node.flatMap((item, index) => - isPlainObject(item) && (typeof item[keyField] !== 'string' || item[keyField] === '') + !isPlainObject(item) || typeof item[keyField] !== 'string' || item[keyField] === '' ? [toIssue([...segments, String(index)], 'missing_merge_key')] : [], ); From 09e89478164067731d7372f35545a64b0f819f4f Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 19:54:22 +0200 Subject: [PATCH 19/20] fix: report repeated merge keys on the items the keyed merge discards --- .../data-schemas/src/app/resolution.spec.ts | 24 +++++++++++ packages/data-schemas/src/app/resolution.ts | 43 ++++++++++++------- 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index c3e490afcb9..0553b353955 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -940,6 +940,30 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { ]); }); + it('reports an earlier merged array item that repeats a merge key', () => { + const custom = [ + { name: 'x', models: { default: ['m'] } }, + { name: 'x', baseURL: 5 }, + ]; + expect(getConfigOverrideIssues({ endpoints: { custom } })).toEqual([ + { + path: 'endpoints.custom.0', + segments: ['endpoints', 'custom', '0'], + code: 'duplicate_merge_key', + }, + { + path: 'endpoints.custom.1.baseURL', + segments: ['endpoints', 'custom', '1', 'baseURL'], + code: 'invalid_type', + }, + ]); + + const merged = mergeConfigOverrides({} as AppConfig, [ + fakeConfig({ endpoints: { custom } }, 10), + ]) as unknown as { endpoints: { custom: unknown[] } }; + expect(merged.endpoints.custom).toEqual([{ name: 'x' }]); + }); + it('drops a merged array item that is not an object', () => { const merged = mergeConfigOverrides({} as AppConfig, [ fakeConfig({ endpoints: { custom: [null, 'bad', { name: 'kept', baseURL: 'x' }] } }, 10), diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index d764859f2ef..4cfcb34e524 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -249,7 +249,7 @@ export type ConfigOverrideIssue = { segments: string[]; /** * A stable, machine-readable reason: a zod issue code (`invalid_type`, `custom`, ...), - * `missing_merge_key`, `indexed_merge_key_write`, or `invalid_document`. Schema messages are not carried because + * `missing_merge_key`, `duplicate_merge_key`, `indexed_merge_key_write`, or `invalid_document`. Schema messages are not carried because * they can echo the submitted values. */ code: string; @@ -271,7 +271,7 @@ function toIssue(segments: string[], code: string): ConfigOverrideIssue { /** * The override item an issue path segment names: by the merge key for merged-by-key arrays - * (the merged index differs from the override's), by index, or by `id` for refinements that + * (the merged index differs from the override's; the last item with a key is the one merged), by index, or by `id` for refinements that * address an item by its identifier. */ function findItemIndex( @@ -281,10 +281,17 @@ function findItemIndex( mergedItem: unknown, ): number { if (keyField) { - return node.findIndex( - (item) => - isPlainObject(item) && isPlainObject(mergedItem) && item[keyField] === mergedItem[keyField], - ); + for (let index = node.length - 1; index >= 0; index--) { + const item = node[index]; + if ( + isPlainObject(item) && + isPlainObject(mergedItem) && + item[keyField] === mergedItem[keyField] + ) { + return index; + } + } + return -1; } if (/^\d+$/.test(key)) { return Number(key); @@ -357,10 +364,11 @@ function isReplacedArrayNode(overrides: AnyObject, segments: string[]): boolean } /** - * Items of a merged-by-key array must be objects with their key: the merge drops any other - * item, or keeps it as is when nothing lies beneath the array. + * Items of a merged-by-key array must be objects with their own key: the merge drops any + * other item (or keeps it as is when nothing lies beneath the array). A repeated key is + * reported on the earlier items: the last one is what the merge keeps. */ -function getKeylessItemIssues(overrides: AnyObject): ConfigOverrideIssue[] { +function getMergeKeyIssues(overrides: AnyObject): ConfigOverrideIssue[] { return Object.entries(ARRAY_MERGE_KEYS).flatMap(([arrayPath, keyField]) => { const segments = arrayPath.split('.'); let node: unknown = overrides; @@ -370,11 +378,16 @@ function getKeylessItemIssues(overrides: AnyObject): ConfigOverrideIssue[] { if (!Array.isArray(node)) { return []; } - return node.flatMap((item, index) => - !isPlainObject(item) || typeof item[keyField] !== 'string' || item[keyField] === '' - ? [toIssue([...segments, String(index)], 'missing_merge_key')] - : [], - ); + const lastIndex = new Map(); + node.forEach((item, index) => isPlainObject(item) && lastIndex.set(item[keyField], index)); + return node.flatMap((item, index) => { + if (!isPlainObject(item) || typeof item[keyField] !== 'string' || item[keyField] === '') { + return [toIssue([...segments, String(index)], 'missing_merge_key')]; + } + return lastIndex.get(item[keyField]) === index + ? [] + : [toIssue([...segments, String(index)], 'duplicate_merge_key')]; + }); }); } @@ -419,7 +432,7 @@ export function getConfigOverrideIssues( return [toIssue([], 'invalid_document')]; } const merged = deepMerge(base as AnyObject, overrides); - const issues = getKeylessItemIssues(overrides); + const issues = getMergeKeyIssues(overrides); const seen = new Set(issues.map((issue) => issue.path)); const shape = configSchema.shape; for (const section of Object.keys(overrides)) { From 9f73ce4e5fd4b7551692163bfb332d2199d2869f Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Mon, 28 Sep 2026 20:40:55 +0200 Subject: [PATCH 20/20] fix: reject keys a union option drops when another option defines them --- .../data-schemas/src/app/resolution.spec.ts | 30 ++++++ packages/data-schemas/src/app/resolution.ts | 99 ++++++++++++++++++- 2 files changed, 124 insertions(+), 5 deletions(-) diff --git a/packages/data-schemas/src/app/resolution.spec.ts b/packages/data-schemas/src/app/resolution.spec.ts index 0553b353955..533ff4b12f3 100644 --- a/packages/data-schemas/src/app/resolution.spec.ts +++ b/packages/data-schemas/src/app/resolution.spec.ts @@ -964,6 +964,17 @@ describe('mergeConfigOverrides: invalid stored overrides', () => { expect(merged.endpoints.custom).toEqual([{ name: 'x' }]); }); + it('drops a key the accepted union option does not define, keeping the rest', () => { + const merged = mergeConfigOverrides({} as AppConfig, [ + fakeConfig( + { memory: { agent: { enabled: true, id: 5, provider: 'openAI', model: 'gpt-4o' } } }, + 10, + ), + ]) as unknown as { memory: { agent: Record } }; + + expect(merged.memory.agent).toEqual({ enabled: true, provider: 'openAI', model: 'gpt-4o' }); + }); + it('drops a merged array item that is not an object', () => { const merged = mergeConfigOverrides({} as AppConfig, [ fakeConfig({ endpoints: { custom: [null, 'bad', { name: 'kept', baseURL: 'x' }] } }, 10), @@ -1203,6 +1214,25 @@ describe('getConfigOverrideIssues', () => { ); }); + it('reports a key another union option defines when the accepted option drops it', () => { + const agent = { enabled: true, id: 5, provider: 'openAI', model: 'gpt-4o' }; + expect(getConfigOverrideIssues({ memory: { agent } })).toEqual([ + { path: 'memory.agent.id', segments: ['memory', 'agent', 'id'], code: 'union_dropped_key' }, + ]); + expect( + getConfigOverrideIssues({ memory: { agent: { provider: 'openAI', model: 'gpt-4o' } } }), + ).toEqual([]); + expect( + getConfigOverrideIssues({ memory: { agent: { id: 'a', provider: 'openAI', unknown: 1 } } }), + ).toEqual([ + { + path: 'memory.agent.provider', + segments: ['memory', 'agent', 'provider'], + code: 'union_dropped_key', + }, + ]); + }); + it('requires the merge key on custom endpoint items and maps merged items back by it', () => { expect(getConfigOverrideIssues({ endpoints: { custom: [{ baseURL: 'https://a' }] } })).toEqual([ { diff --git a/packages/data-schemas/src/app/resolution.ts b/packages/data-schemas/src/app/resolution.ts index 4cfcb34e524..1bd85676a66 100644 --- a/packages/data-schemas/src/app/resolution.ts +++ b/packages/data-schemas/src/app/resolution.ts @@ -249,7 +249,7 @@ export type ConfigOverrideIssue = { segments: string[]; /** * A stable, machine-readable reason: a zod issue code (`invalid_type`, `custom`, ...), - * `missing_merge_key`, `duplicate_merge_key`, `indexed_merge_key_write`, or `invalid_document`. Schema messages are not carried because + * `missing_merge_key`, `duplicate_merge_key`, `indexed_merge_key_write`, `union_dropped_key`, or `invalid_document`. Schema messages are not carried because * they can echo the submitted values. */ code: string; @@ -416,6 +416,92 @@ function expandUnionIssues(issues: SchemaIssue[], depth = 0): SchemaIssue[] { }); } +/** The parts of a zod schema the stripped-key walk reads, without depending on zod. */ +type SchemaNode = { + _def: { + typeName?: string; + innerType?: SchemaNode; + schema?: SchemaNode; + type?: SchemaNode; + valueType?: SchemaNode; + options?: SchemaNode[] | Map; + }; + shape?: Record; + safeParse: (value: unknown) => { success: boolean }; +}; + +/** The schema beneath optional, nullable, default and refinement wrappers. */ +function unwrapSchema(schema: SchemaNode): SchemaNode { + let node = schema; + for (let depth = 0; depth < MAX_MERGE_DEPTH; depth++) { + const inner = node._def.innerType ?? node._def.schema; + if (!inner) { + break; + } + node = inner; + } + return node; +} + +/** + * Keys a union dropped: an object parses with the first union option that accepts it, and + * that option strips keys it does not define, so a key another option defines (and would + * type-check) is kept raw in the stored override but never validated. Keys no option defines + * are unknown keys and stay accepted. + */ +function findUnionDroppedKeys( + schema: SchemaNode, + value: unknown, + path: IssuePath, + depth = 0, +): IssuePath[] { + if (depth >= MAX_MERGE_DEPTH) { + return []; + } + const node = unwrapSchema(schema); + const { typeName } = node._def; + if (typeName === 'ZodObject' && node.shape && isPlainObject(value)) { + const shape = node.shape; + return Object.keys(value) + .filter((key) => hasOwn(shape, key)) + .flatMap((key) => findUnionDroppedKeys(shape[key], value[key], [...path, key], depth + 1)); + } + if (typeName === 'ZodArray' && node._def.type && Array.isArray(value)) { + const item = node._def.type; + return value.flatMap((entry, index) => + findUnionDroppedKeys(item, entry, [...path, index], depth + 1), + ); + } + if (typeName === 'ZodRecord' && node._def.valueType && isPlainObject(value)) { + const entrySchema = node._def.valueType; + return Object.entries(value).flatMap(([key, entry]) => + findUnionDroppedKeys(entrySchema, entry, [...path, key], depth + 1), + ); + } + if ( + (typeName !== 'ZodUnion' && typeName !== 'ZodDiscriminatedUnion') || + !node._def.options || + !isPlainObject(value) + ) { + return []; + } + const options = [...node._def.options.values()]; + const accepted = options.find((option) => option.safeParse(value).success); + if (!accepted) { + return []; + } + const acceptedShape = unwrapSchema(accepted).shape; + const defined = new Set( + options.flatMap((option) => Object.keys(unwrapSchema(option).shape ?? {})), + ); + const dropped = acceptedShape + ? Object.keys(value) + .filter((key) => !hasOwn(acceptedShape, key) && defined.has(key)) + .map((key) => [...path, key]) + : []; + return [...dropped, ...findUnionDroppedKeys(accepted, value, path, depth + 1)]; +} + /** * Checks config overrides the way they apply: merged over `base` (YAML-shaped), each * section they touch parsed with its `configSchema` schema, and every failure attributed @@ -439,11 +525,14 @@ export function getConfigOverrideIssues( if (!hasOwn(shape, section)) { continue; } + const schema = shape[section as keyof typeof shape] as unknown as SchemaNode; const result = shape[section as keyof typeof shape].safeParse(merged[section]); - if (result.success) { - continue; - } - for (const issue of expandUnionIssues(result.error.issues)) { + const dropped = findUnionDroppedKeys(schema, merged[section], []).map((path) => ({ + code: 'union_dropped_key', + path, + })); + const schemaIssues = result.success ? [] : expandUnionIssues(result.error.issues); + for (const issue of [...dropped, ...schemaIssues]) { if (issue.code === 'unrecognized_keys') { continue; }