mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-07-10 16:23:44 +00:00
🧮 fix: Prevent String Corruption of Numeric Agent Model Parameters (#14119)
* 🧮 fix: Prevent String Corruption of Numeric Agent Model Parameters * 🧮 fix: Support Partial Numeric Input and Cover Parameter Aliases
This commit is contained in:
parent
1f7b38e972
commit
2d4ef52c22
8 changed files with 379 additions and 13 deletions
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<HTMLInputElement>) => {
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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<typeof ChatContext>;
|
||||
|
||||
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(
|
||||
<ChatContext.Provider value={chatContextValue}>
|
||||
<DynamicInput
|
||||
settingKey={settingKey}
|
||||
type={type}
|
||||
range={range}
|
||||
setOption={setOption}
|
||||
conversation={{}}
|
||||
/>
|
||||
</ChatContext.Provider>,
|
||||
);
|
||||
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(
|
||||
<ChatContext.Provider value={chatContextValue}>
|
||||
<DynamicInput settingKey="custom" setOption={setOption} conversation={{}} />
|
||||
</ChatContext.Provider>,
|
||||
);
|
||||
const input = screen.getByRole('textbox');
|
||||
|
||||
fireEvent.change(input, { target: { value: '42' } });
|
||||
act(() => {
|
||||
jest.advanceTimersByTime(500);
|
||||
});
|
||||
expect(commit).toHaveBeenLastCalledWith(42);
|
||||
});
|
||||
});
|
||||
|
|
@ -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';
|
||||
|
|
|
|||
90
packages/api/src/agents/parameters.spec.ts
Normal file
90
packages/api/src/agents/parameters.spec.ts
Normal file
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
55
packages/api/src/agents/parameters.ts
Normal file
55
packages/api/src/agents/parameters.ts
Normal file
|
|
@ -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<string, unknown>): Record<string, unknown> {
|
||||
const result: Record<string, unknown> = {};
|
||||
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;
|
||||
}
|
||||
|
|
@ -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<string, SettingDefinition> = {
|
|||
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<string, SettingDefinition> = {
|
|||
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<string, SettingDefinition> = {
|
|||
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<string, SettingDefinition> = {
|
|||
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: {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue