diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 817e9e837c..a089f2d9c1 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -9,6 +9,7 @@ const { refreshListAvatars, collectEdgeAgentIds, mergeAgentOcrConversion, + sanitizeModelParameters, MAX_AVATAR_REFRESH_AGENTS, collectToolResourceFileIds, convertOcrToContextInPlace, @@ -348,7 +349,10 @@ const createAgentHandler = async (req, res) => { const { tools = [], ...agentData } = removeNullishValues(validatedData); if (agentData.model_parameters && typeof agentData.model_parameters === 'object') { - agentData.model_parameters = removeNullishValues(agentData.model_parameters, true); + agentData.model_parameters = removeNullishValues( + sanitizeModelParameters(agentData.model_parameters), + true, + ); } const { id: userId, role: userRole } = req.user; @@ -604,7 +608,10 @@ const updateAgentHandler = async (req, res) => { const updateData = removeNullishValues(rest); if (updateData.model_parameters && typeof updateData.model_parameters === 'object') { - updateData.model_parameters = removeNullishValues(updateData.model_parameters, true); + updateData.model_parameters = removeNullishValues( + sanitizeModelParameters(updateData.model_parameters), + true, + ); } if (avatarField === null) { diff --git a/api/server/controllers/agents/v1.spec.js b/api/server/controllers/agents/v1.spec.js index 8111193101..76d9796b74 100644 --- a/api/server/controllers/agents/v1.spec.js +++ b/api/server/controllers/agents/v1.spec.js @@ -500,6 +500,38 @@ describe('Agent Controllers - Mass Assignment Protection', () => { expect(agentInDb.model_parameters.maxContextTokens).toBeUndefined(); }); + test('should drop non-numeric strings and coerce numeric strings in model_parameters', async () => { + // Regression test for #12920: a stray placeholder string ("System") persisted + // into max_tokens was forwarded to the provider, causing a 400 + const dataWithCorruptModelParams = { + provider: 'openai', + model: 'gpt-4', + name: 'Agent with Corrupt Model Params', + model_parameters: { + max_tokens: 'System', + maxContextTokens: '256000', + fileTokenLimit: 256000, + useResponsesApi: true, + }, + }; + + mockReq.body = dataWithCorruptModelParams; + + await createAgentHandler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(201); + + const createdAgent = mockRes.json.mock.calls[0][0]; + expect(createdAgent.model_parameters.max_tokens).toBeUndefined(); + expect(createdAgent.model_parameters.maxContextTokens).toBe(256000); + expect(createdAgent.model_parameters.fileTokenLimit).toBe(256000); + expect(createdAgent.model_parameters.useResponsesApi).toBe(true); + + const agentInDb = await Agent.findOne({ id: createdAgent.id }); + expect(agentInDb.model_parameters.max_tokens).toBeUndefined(); + expect(agentInDb.model_parameters.maxContextTokens).toBe(256000); + }); + test('should handle invalid avatar format', async () => { const dataWithInvalidAvatar = { provider: 'openai', @@ -697,6 +729,32 @@ describe('Agent Controllers - Mass Assignment Protection', () => { expect(agentInDb.name).toBe('Updated Agent'); }); + test('should sanitize corrupt numeric model_parameters on update', async () => { + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + name: 'Healed Agent', + model_parameters: { + max_tokens: 'System', + maxContextTokens: 256000, + temperature: '0.7', + }, + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + + const updatedAgent = mockRes.json.mock.calls[0][0]; + expect(updatedAgent.model_parameters.max_tokens).toBeUndefined(); + expect(updatedAgent.model_parameters.maxContextTokens).toBe(256000); + expect(updatedAgent.model_parameters.temperature).toBe(0.7); + + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(agentInDb.model_parameters.max_tokens).toBeUndefined(); + expect(agentInDb.model_parameters.maxContextTokens).toBe(256000); + }); + test('should reject update with unauthorized fields (mass assignment protection)', async () => { mockReq.user.id = existingAgentAuthorId.toString(); mockReq.params.id = existingAgentId; diff --git a/client/src/components/SidePanel/Parameters/DynamicInput.tsx b/client/src/components/SidePanel/Parameters/DynamicInput.tsx index cdc6f3a452..19d43cfaf3 100644 --- a/client/src/components/SidePanel/Parameters/DynamicInput.tsx +++ b/client/src/components/SidePanel/Parameters/DynamicInput.tsx @@ -1,4 +1,4 @@ -import { OptionTypes } from 'librechat-data-provider'; +import { OptionTypes, SettingTypes } from 'librechat-data-provider'; import { Label, Input, HoverCard, HoverCardTrigger } from '@librechat/client'; import type { DynamicSettingProps } from 'librechat-data-provider'; import { useLocalize, useDebouncedInput, useParameterEffects, TranslationKeys } from '~/hooks'; @@ -8,6 +8,8 @@ import OptionHover from './OptionHover'; import { ESide } from '~/common'; function DynamicInput({ + type, + range, label = '', settingKey, defaultValue, @@ -15,8 +17,6 @@ function DynamicInput({ columnSpan, setOption, optionType, - type, - range, placeholder = '', readonly = false, showDefault = false, @@ -45,7 +45,7 @@ function DynamicInput({ }); const handleInputChange = (e: React.ChangeEvent) => { - if (type === 'number') { + if (type === SettingTypes.Number) { // Integer params: strip thousands separators so "120,000" / "120.000" // become 120000 instead of being truncated to 120 downstream by parseInt. // Keep a leading minus for fields whose range permits negatives (e.g. @@ -58,7 +58,7 @@ function DynamicInput({ setInputValue(sanitized, sanitized !== '-'); return; } - setInputValue(e, !isNaN(Number(e.target.value))); + setInputValue(e, type === SettingTypes.String ? false : !isNaN(Number(e.target.value))); }; const placeholderText = placeholderCode diff --git a/client/src/components/SidePanel/Parameters/__tests__/DynamicInput.spec.tsx b/client/src/components/SidePanel/Parameters/__tests__/DynamicInput.spec.tsx new file mode 100644 index 0000000000..5cd348ab90 --- /dev/null +++ b/client/src/components/SidePanel/Parameters/__tests__/DynamicInput.spec.tsx @@ -0,0 +1,155 @@ +import React from 'react'; +import userEvent from '@testing-library/user-event'; +import { render, screen, act, fireEvent } from '@testing-library/react'; +import type { TSetOption, SettingRange } from 'librechat-data-provider'; +import DynamicInput from '../DynamicInput'; +import { ChatContext } from '~/Providers'; + +type ChatContextValue = React.ContextType; + +const chatContextValue = { preset: null } as unknown as ChatContextValue; + +function setup({ + type, + range, + settingKey, +}: { + type: 'number' | 'string'; + range?: SettingRange; + settingKey: string; +}) { + const commit = jest.fn(); + const setOption = jest.fn(() => commit) as unknown as TSetOption; + render( + + + , + ); + return { input: screen.getByRole('textbox'), commit }; +} + +describe('DynamicInput', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('strips typed non-numeric text for number settings', async () => { + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + const { input, commit } = setup({ type: 'number', settingKey: 'max_tokens' }); + + await user.type(input, 'System'); + + expect(input).toHaveValue(''); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(commit).toHaveBeenLastCalledWith(''); + }); + + it('strips pasted or autofilled non-numeric text for number settings', () => { + const { input, commit } = setup({ type: 'number', settingKey: 'max_tokens' }); + + fireEvent.change(input, { target: { value: 'System' } }); + + expect(input).toHaveValue(''); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(commit).toHaveBeenLastCalledWith(''); + }); + + it('commits numeric input as a number for number settings', async () => { + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + const { input, commit } = setup({ type: 'number', settingKey: 'max_tokens' }); + + await user.type(input, '4096'); + + expect(input).toHaveValue('4096'); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(commit).toHaveBeenLastCalledWith(4096); + }); + + it('strips thousands separators from pasted values', () => { + const { input, commit } = setup({ type: 'number', settingKey: 'maxContextTokens' }); + + fireEvent.change(input, { target: { value: '120,000' } }); + + expect(input).toHaveValue('120000'); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(commit).toHaveBeenLastCalledWith(120000); + }); + + it('allows typing negative numbers when the range permits (e.g. thinkingBudget -1)', async () => { + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + const { input, commit } = setup({ + type: 'number', + range: { min: -1, max: 24576, step: 1 }, + settingKey: 'thinkingBudget', + }); + + await user.type(input, '-1'); + + expect(input).toHaveValue('-1'); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(commit).toHaveBeenLastCalledWith(-1); + }); + + it('drops the minus sign when the range does not permit negatives', async () => { + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + const { input, commit } = setup({ type: 'number', settingKey: 'max_tokens' }); + + await user.type(input, '-1'); + + expect(input).toHaveValue('1'); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(commit).toHaveBeenLastCalledWith(1); + }); + + it('commits digit-only input as a string for string settings', async () => { + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + const { input, commit } = setup({ type: 'string', settingKey: 'modelLabel' }); + + await user.type(input, '123'); + + expect(input).toHaveValue('123'); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(commit).toHaveBeenLastCalledWith('123'); + }); + + it('preserves the numeric sniffing fallback when no type is provided', () => { + const commit = jest.fn(); + const setOption = jest.fn(() => commit) as unknown as TSetOption; + render( + + + , + ); + const input = screen.getByRole('textbox'); + + fireEvent.change(input, { target: { value: '42' } }); + act(() => { + jest.advanceTimersByTime(500); + }); + expect(commit).toHaveBeenLastCalledWith(42); + }); +}); diff --git a/packages/api/src/agents/index.ts b/packages/api/src/agents/index.ts index 28c70bb538..8865ef8762 100644 --- a/packages/api/src/agents/index.ts +++ b/packages/api/src/agents/index.ts @@ -14,6 +14,7 @@ export * from './legacy'; export * from './memory'; export * from './orphans'; export * from './migration'; +export * from './parameters'; export * from './openai'; export * from './transactions'; export * from './usage'; diff --git a/packages/api/src/agents/parameters.spec.ts b/packages/api/src/agents/parameters.spec.ts new file mode 100644 index 0000000000..423ca5168a --- /dev/null +++ b/packages/api/src/agents/parameters.spec.ts @@ -0,0 +1,90 @@ +import { sanitizeModelParameters } from './parameters'; + +describe('sanitizeModelParameters', () => { + it('drops non-numeric strings from numeric keys', () => { + const result = sanitizeModelParameters({ + max_tokens: 'System', + maxContextTokens: 256000, + fileTokenLimit: 256000, + }); + expect(result).toEqual({ maxContextTokens: 256000, fileTokenLimit: 256000 }); + }); + + it('coerces numeric strings to numbers', () => { + const result = sanitizeModelParameters({ + max_tokens: '4096', + temperature: ' 0.7 ', + maxOutputTokens: '8192', + }); + expect(result).toEqual({ max_tokens: 4096, temperature: 0.7, maxOutputTokens: 8192 }); + }); + + it('preserves explicit zero and negative values', () => { + const result = sanitizeModelParameters({ + max_tokens: 0, + frequency_penalty: -1.5, + presence_penalty: '-0.5', + }); + expect(result).toEqual({ max_tokens: 0, frequency_penalty: -1.5, presence_penalty: -0.5 }); + }); + + it('drops NaN, Infinity, empty strings, booleans, and objects from numeric keys', () => { + const result = sanitizeModelParameters({ + max_tokens: NaN, + maxTokens: Infinity, + maxOutputTokens: '', + max_output_tokens: ' ', + maxContextTokens: true, + max_context_tokens: { value: 4096 }, + topK: null, + topP: undefined, + }); + expect(result).toEqual({}); + }); + + it('passes non-numeric keys through untouched', () => { + const result = sanitizeModelParameters({ + model: 'gemma4:26b', + region: 'us-east-1', + useResponsesApi: true, + promptCache: false, + customSetting: 'value', + }); + expect(result).toEqual({ + model: 'gemma4:26b', + region: 'us-east-1', + useResponsesApi: true, + promptCache: false, + customSetting: 'value', + }); + }); + + it('handles all known numeric key variants', () => { + const keys = [ + 'temperature', + 'top_p', + 'topP', + 'top_k', + 'topK', + 'frequency_penalty', + 'frequencyPenalty', + 'presence_penalty', + 'presencePenalty', + 'max_tokens', + 'maxTokens', + 'max_output_tokens', + 'maxOutputTokens', + 'max_context_tokens', + 'maxContextTokens', + 'fileTokenLimit', + 'thinking_budget', + 'thinkingBudget', + ]; + const corrupt = Object.fromEntries(keys.map((key) => [key, 'System'])); + expect(sanitizeModelParameters(corrupt)).toEqual({}); + + const valid = Object.fromEntries(keys.map((key, i) => [key, `${i + 1}`])); + const expected = Object.fromEntries(keys.map((key, i) => [key, i + 1])); + expect(sanitizeModelParameters(valid)).toEqual(expected); + }); +}); diff --git a/packages/api/src/agents/parameters.ts b/packages/api/src/agents/parameters.ts new file mode 100644 index 0000000000..6179f91125 --- /dev/null +++ b/packages/api/src/agents/parameters.ts @@ -0,0 +1,55 @@ +const NUMERIC_PARAM_KEYS = new Set([ + 'temperature', + 'top_p', + 'topP', + 'top_k', + 'topK', + 'frequency_penalty', + 'frequencyPenalty', + 'presence_penalty', + 'presencePenalty', + 'max_tokens', + 'maxTokens', + 'max_output_tokens', + 'maxOutputTokens', + 'max_context_tokens', + 'maxContextTokens', + 'fileTokenLimit', + 'thinking_budget', + 'thinkingBudget', +]); + +function coerceFiniteNumber(value: unknown): number | undefined { + if (typeof value === 'number') { + return Number.isFinite(value) ? value : undefined; + } + if (typeof value !== 'string') { + return undefined; + } + const trimmed = value.trim(); + if (trimmed === '') { + return undefined; + } + const parsed = Number(trimmed); + return Number.isFinite(parsed) ? parsed : undefined; +} + +/** + * Coerces known numeric model parameter keys to finite numbers, dropping values + * that cannot be represented as one (e.g. a stray placeholder string captured by + * the UI). Explicit `0` and negative values are preserved; other keys pass through. + */ +export function sanitizeModelParameters(params: Record): Record { + const result: Record = {}; + for (const [key, value] of Object.entries(params)) { + if (!NUMERIC_PARAM_KEYS.has(key)) { + result[key] = value; + continue; + } + const coerced = coerceFiniteNumber(value); + if (coerced !== undefined) { + result[key] = coerced; + } + } + return result; +} diff --git a/packages/data-provider/src/parameterSettings.ts b/packages/data-provider/src/parameterSettings.ts index 44389cb323..4f495307fd 100644 --- a/packages/data-provider/src/parameterSettings.ts +++ b/packages/data-provider/src/parameterSettings.ts @@ -112,7 +112,7 @@ export const librechat = { labelCode: true, type: 'number', component: 'input', - placeholder: 'com_nav_theme_system', + placeholder: 'com_endpoint_default', placeholderCode: true, description: 'com_endpoint_context_info', descriptionCode: true, @@ -149,7 +149,7 @@ export const librechat = { labelCode: true, description: 'com_ui_file_token_limit_desc', descriptionCode: true, - placeholder: 'com_nav_theme_system', + placeholder: 'com_endpoint_default', placeholderCode: true, type: 'number', component: 'input', @@ -222,7 +222,7 @@ const openAIParams: Record = { component: 'input', description: 'com_endpoint_openai_max_tokens', descriptionCode: true, - placeholder: 'com_nav_theme_system', + placeholder: 'com_endpoint_default', placeholderCode: true, optionType: 'model', columnSpan: 2, @@ -350,7 +350,7 @@ const anthropic: Record = { component: 'input', description: 'com_endpoint_anthropic_maxoutputtokens', descriptionCode: true, - placeholder: 'com_nav_theme_system', + placeholder: 'com_endpoint_default', placeholderCode: true, range: { min: anthropicSettings.maxOutputTokens.min, @@ -539,7 +539,7 @@ const bedrock: Record = { component: 'input', description: 'com_endpoint_anthropic_maxoutputtokens', descriptionCode: true, - placeholder: 'com_nav_theme_system', + placeholder: 'com_endpoint_default', placeholderCode: true, optionType: 'model', columnSpan: 2, @@ -684,7 +684,7 @@ const google: Record = { component: 'input', description: 'com_endpoint_google_maxoutputtokens', descriptionCode: true, - placeholder: 'com_nav_theme_system', + placeholder: 'com_endpoint_default', placeholderCode: true, default: googleSettings.maxOutputTokens.default, range: {