From 54511b2b8c6c689420200bbb377ded0432d42565 Mon Sep 17 00:00:00 2001 From: Mihidum <55163074+mihidumh@users.noreply.github.com> Date: Sun, 6 Sep 2026 12:34:06 +1000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=A0=20fix:=20Return=20400=20for=20Inva?= =?UTF-8?q?lid=20Memory=20Keys=20(#14618)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: return 400 for invalid memory key format instead of 500 Co-Authored-By: Claude Fable 5 * fix: trim PATCH rename key to match POST normalization Co-Authored-By: Claude Fable 5 * fix: return 400 when PATCH rename key is not a string A non-string `key` in the PATCH body (e.g. {"key": 123}) previously fell back to the URL key and updated the memory with a 200, hiding the invalid field. Reject any defined non-string key with 400 before normalization. Co-Authored-By: Claude Fable 5 * fix: share memory key validation between schema and routes * fix: reject blank memory rename keys * test: use real data-provider exports in memory route suite * fix: share memory key validation with ID updates --------- Co-authored-by: Claude Fable 5 Co-authored-by: Marco Beretta <81851188+berry-13@users.noreply.github.com> --- api/server/routes/__tests__/memories.spec.js | 165 +++++++++++++++++++ api/server/routes/memories.js | 20 ++- api/server/routes/memories.test.js | 13 -- packages/api/src/memory/handlers.ts | 6 +- packages/data-schemas/src/schema/index.ts | 2 +- packages/data-schemas/src/schema/memory.ts | 8 +- 6 files changed, 196 insertions(+), 18 deletions(-) create mode 100644 api/server/routes/__tests__/memories.spec.js diff --git a/api/server/routes/__tests__/memories.spec.js b/api/server/routes/__tests__/memories.spec.js new file mode 100644 index 0000000000..f6f193734f --- /dev/null +++ b/api/server/routes/__tests__/memories.spec.js @@ -0,0 +1,165 @@ +const express = require('express'); +const request = require('supertest'); + +jest.mock('@librechat/api', () => ({ + ...jest.requireActual('@librechat/api'), + Tokenizer: { + getTokenCount: jest.fn().mockReturnValue(5), + }, + generateCheckAccess: jest.fn(() => (req, res, next) => next()), +})); + +jest.mock('librechat-data-provider', () => ({ + ...jest.requireActual('librechat-data-provider'), +})); + +jest.mock('~/models', () => ({ + getAllUserMemories: jest.fn(), + getUserMemories: jest.fn(), + toggleUserMemories: jest.fn(), + getRoleByName: jest.fn(), + createMemory: jest.fn(), + deleteMemory: jest.fn(), + setMemory: jest.fn(), + getAgents: jest.fn(), +})); + +jest.mock('~/server/services/PermissionService', () => ({ + findAccessibleResources: jest.fn().mockResolvedValue([]), +})); + +jest.mock('~/server/middleware', () => ({ + requireJwtAuth: (req, res, next) => next(), + configMiddleware: (req, res, next) => next(), +})); + +const { createMemory, getUserMemories, setMemory } = require('~/models'); + +describe('memories routes', () => { + let app; + + beforeAll(() => { + const memoriesRouter = require('../memories'); + + app = express(); + app.use(express.json()); + app.use((req, res, next) => { + req.user = { id: 'user-1', role: 'USER' }; + next(); + }); + app.use('/api/memories', memoriesRouter); + }); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('POST /', () => { + it('returns 400 when the key contains invalid characters', async () => { + const response = await request(app) + .post('/api/memories') + .send({ key: 'My Key!', value: 'some value' }); + + expect(response.status).toBe(400); + expect(response.body.error).toContain('lowercase letters and underscores'); + expect(createMemory).not.toHaveBeenCalled(); + }); + + it('returns 400 when the key contains uppercase letters', async () => { + const response = await request(app) + .post('/api/memories') + .send({ key: 'myKey', value: 'some value' }); + + expect(response.status).toBe(400); + expect(response.body.error).toContain('lowercase letters and underscores'); + expect(createMemory).not.toHaveBeenCalled(); + }); + + it('creates a memory when the key is valid', async () => { + getUserMemories + .mockResolvedValueOnce([]) + .mockResolvedValueOnce([{ key: 'my_key', value: 'some value', tokenCount: 5 }]); + createMemory.mockResolvedValue({ ok: true }); + + const response = await request(app) + .post('/api/memories') + .send({ key: 'my_key', value: 'some value' }); + + expect(response.status).toBe(201); + expect(response.body.created).toBe(true); + expect(createMemory).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user-1', key: 'my_key' }), + ); + }); + }); + + describe('PATCH /:key', () => { + it('returns 400 when renaming to a key with invalid characters', async () => { + const response = await request(app) + .patch('/api/memories/my_key') + .send({ key: 'New Key', value: 'updated value' }); + + expect(response.status).toBe(400); + expect(response.body.error).toContain('lowercase letters and underscores'); + expect(createMemory).not.toHaveBeenCalled(); + }); + + it('returns 400 when the key is not a string', async () => { + const response = await request(app) + .patch('/api/memories/my_key') + .send({ key: 123, value: 'updated value' }); + + expect(response.status).toBe(400); + expect(response.body.error).toBe('Key must be a string.'); + expect(setMemory).not.toHaveBeenCalled(); + expect(createMemory).not.toHaveBeenCalled(); + }); + + it('rejects a whitespace-only rename instead of updating the existing memory', async () => { + getUserMemories.mockResolvedValue([{ key: 'my_key', value: 'old value', tokenCount: 5 }]); + setMemory.mockResolvedValue({ ok: true }); + + const response = await request(app) + .patch('/api/memories/my_key') + .send({ key: ' \t\n ', value: 'updated value' }); + + expect(response.status).toBe(400); + expect(setMemory).not.toHaveBeenCalled(); + expect(createMemory).not.toHaveBeenCalled(); + }); + + it('trims the new key before validating and renaming', async () => { + const { deleteMemory } = require('~/models'); + getUserMemories + .mockResolvedValueOnce([{ key: 'my_key', value: 'old value', tokenCount: 5 }]) + .mockResolvedValueOnce([{ key: 'new_key', value: 'updated value', tokenCount: 5 }]); + createMemory.mockResolvedValue({ ok: true }); + deleteMemory.mockResolvedValue({ ok: true }); + + const response = await request(app) + .patch('/api/memories/my_key') + .send({ key: ' new_key ', value: 'updated value' }); + + expect(response.status).toBe(200); + expect(createMemory).toHaveBeenCalledWith(expect.objectContaining({ key: 'new_key' })); + expect(deleteMemory).toHaveBeenCalledWith(expect.objectContaining({ key: 'my_key' })); + }); + + it('updates the value when the key is unchanged', async () => { + getUserMemories + .mockResolvedValueOnce([{ key: 'my_key', value: 'old value', tokenCount: 5 }]) + .mockResolvedValueOnce([{ key: 'my_key', value: 'updated value', tokenCount: 5 }]); + setMemory.mockResolvedValue({ ok: true }); + + const response = await request(app) + .patch('/api/memories/my_key') + .send({ value: 'updated value' }); + + expect(response.status).toBe(200); + expect(response.body.updated).toBe(true); + expect(setMemory).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user-1', key: 'my_key' }), + ); + }); + }); +}); diff --git a/api/server/routes/memories.js b/api/server/routes/memories.js index d19819dd80..ce00b4d5a5 100644 --- a/api/server/routes/memories.js +++ b/api/server/routes/memories.js @@ -1,4 +1,5 @@ const express = require('express'); +const { isValidMemoryKey } = require('@librechat/data-schemas'); const { Tokenizer, generateCheckAccess, @@ -188,6 +189,12 @@ router.post('/', createMemoryMiddleware, async (req, res) => { return res.status(400).json({ error: 'Value is required and must be a non-empty string.' }); } + if (!isValidMemoryKey(key.trim())) { + return res.status(400).json({ + error: 'Key must only contain lowercase letters and underscores.', + }); + } + const appConfig = req.config; const memoryConfig = appConfig?.memory; const charLimit = memoryConfig?.charLimit || 10000; @@ -315,7 +322,18 @@ router.patch('/:key', updateMemoryMiddleware, async (req, res) => { return res.status(400).json({ error: 'Value is required and must be a non-empty string.' }); } - const newKey = bodyKey || urlKey; + if (bodyKey !== undefined && typeof bodyKey !== 'string') { + return res.status(400).json({ error: 'Key must be a string.' }); + } + + const newKey = bodyKey === undefined ? urlKey : bodyKey.trim(); + + if (newKey !== urlKey && !isValidMemoryKey(newKey)) { + return res.status(400).json({ + error: 'Key must only contain lowercase letters and underscores.', + }); + } + const appConfig = req.config; const memoryConfig = appConfig?.memory; const charLimit = memoryConfig?.charLimit || 10000; diff --git a/api/server/routes/memories.test.js b/api/server/routes/memories.test.js index 54727b6490..34275115c5 100644 --- a/api/server/routes/memories.test.js +++ b/api/server/routes/memories.test.js @@ -55,19 +55,6 @@ jest.mock('@librechat/api', () => ({ contentFilterBlockResponse: jest.fn(), })); -jest.mock('librechat-data-provider', () => ({ - PermissionTypes: { MEMORIES: 'MEMORIES' }, - Permissions: { - USE: 'USE', - READ: 'READ', - CREATE: 'CREATE', - UPDATE: 'UPDATE', - OPT_OUT: 'OPT_OUT', - }, - ResourceType: { AGENT: 'agent' }, - PermissionBits: { VIEW: 1 }, -})); - jest.mock('~/server/services/PermissionService', () => ({ checkPermission: (...args) => mockCheckPermission(...args), findAccessibleResources: jest.fn().mockResolvedValue([]), diff --git a/packages/api/src/memory/handlers.ts b/packages/api/src/memory/handlers.ts index 709419cd47..0b56724489 100644 --- a/packages/api/src/memory/handlers.ts +++ b/packages/api/src/memory/handlers.ts @@ -1,5 +1,7 @@ -import { hasActivePiiPatterns, type FiltersConfig } from 'librechat-data-provider'; +import { isValidMemoryKey } from '@librechat/data-schemas'; +import { hasActivePiiPatterns } from 'librechat-data-provider'; import type { MemoryMethods } from '@librechat/data-schemas'; +import type { FiltersConfig } from 'librechat-data-provider'; import type { Request, Response } from 'express'; import type { MemoryContentInput } from '../protection/adapters/submissions'; import type { ProjectedStoredMemory } from './protection'; @@ -90,7 +92,7 @@ export function createMemoryManagementHandlers(deps: MemoryManagementHandlersDep error: `Key exceeds maximum length of 1000 characters. Current length: ${key.length} characters.`, }); } - if (key != null && !/^[a-z_]+$/.test(key)) { + if (key != null && !isValidMemoryKey(key)) { return res .status(400) .json({ error: 'Key must only contain lowercase letters and underscores.' }); diff --git a/packages/data-schemas/src/schema/index.ts b/packages/data-schemas/src/schema/index.ts index 2b9f6255eb..21f77ee052 100644 --- a/packages/data-schemas/src/schema/index.ts +++ b/packages/data-schemas/src/schema/index.ts @@ -29,7 +29,7 @@ export { default as tokenSchema } from './token'; export { default as toolCallSchema } from './toolCall'; export { default as transactionSchema } from './transaction'; export { default as userSchema } from './user'; -export { default as memorySchema } from './memory'; +export { default as memorySchema, isValidMemoryKey } from './memory'; export { default as toolFavoriteSchema } from './favorite'; export { default as groupSchema } from './group'; export { default as systemGrantSchema } from './systemGrant'; diff --git a/packages/data-schemas/src/schema/memory.ts b/packages/data-schemas/src/schema/memory.ts index fc58833458..346489b8de 100644 --- a/packages/data-schemas/src/schema/memory.ts +++ b/packages/data-schemas/src/schema/memory.ts @@ -1,6 +1,12 @@ import { Schema } from 'mongoose'; import type { IMemoryEntry } from '~/types/memory'; +const MEMORY_KEY_REGEX = /^[a-z_]+$/; + +export function isValidMemoryKey(key: string): boolean { + return MEMORY_KEY_REGEX.test(key); +} + const MemoryEntrySchema: Schema = new Schema({ userId: { type: Schema.Types.ObjectId, @@ -12,7 +18,7 @@ const MemoryEntrySchema: Schema = new Schema({ type: String, required: true, validate: { - validator: (v: string) => /^[a-z_]+$/.test(v), + validator: isValidMemoryKey, message: 'Key must only contain lowercase letters and underscores', }, },