diff --git a/api/server/experimental.js b/api/server/experimental.js index f61b52a9c4..2e7f5dff63 100644 --- a/api/server/experimental.js +++ b/api/server/experimental.js @@ -16,6 +16,7 @@ const { isEnabled, ErrorController, performStartupChecks, + handleJsonParseError, initializeFileStorage, } = require('@librechat/api'); const { connectDb, indexSync } = require('~/db'); @@ -245,6 +246,7 @@ if (cluster.isMaster) { app.use(noIndex); app.use(express.json({ limit: '3mb' })); app.use(express.urlencoded({ extended: true, limit: '3mb' })); + app.use(handleJsonParseError); app.use(mongoSanitize()); app.use(cors()); app.use(cookieParser()); diff --git a/api/server/index.js b/api/server/index.js index 311b9a796f..d0bb64405f 100644 --- a/api/server/index.js +++ b/api/server/index.js @@ -14,6 +14,7 @@ const { isEnabled, ErrorController, performStartupChecks, + handleJsonParseError, initializeFileStorage, } = require('@librechat/api'); const { connectDb, indexSync } = require('~/db'); @@ -81,6 +82,7 @@ const startServer = async () => { app.use(noIndex); app.use(express.json({ limit: '3mb' })); app.use(express.urlencoded({ extended: true, limit: '3mb' })); + app.use(handleJsonParseError); app.use(mongoSanitize()); app.use(cors()); app.use(cookieParser()); diff --git a/api/server/routes/prompts.js b/api/server/routes/prompts.js index 300072b4d2..c833719075 100644 --- a/api/server/routes/prompts.js +++ b/api/server/routes/prompts.js @@ -5,6 +5,7 @@ const { markPublicPromptGroups, buildPromptGroupFilter, formatPromptGroupsResponse, + safeValidatePromptGroupUpdate, createEmptyPromptGroupsResponse, filterAccessibleIdsBySharedLogic, } = require('@librechat/api'); @@ -344,7 +345,16 @@ const patchPromptGroup = async (req, res) => { if (req.user.role === SystemRoles.ADMIN) { delete filter.author; } - const promptGroup = await updatePromptGroup(filter, req.body); + + const validationResult = safeValidatePromptGroupUpdate(req.body); + if (!validationResult.success) { + return res.status(400).send({ + error: 'Invalid request body', + details: validationResult.error.errors, + }); + } + + const promptGroup = await updatePromptGroup(filter, validationResult.data); res.status(200).send(promptGroup); } catch (error) { logger.error(error); diff --git a/api/server/routes/prompts.test.js b/api/server/routes/prompts.test.js index e23676b8db..1aeca1c93c 100644 --- a/api/server/routes/prompts.test.js +++ b/api/server/routes/prompts.test.js @@ -544,6 +544,169 @@ describe('Prompt Routes - ACL Permissions', () => { }); }); + describe('PATCH /api/prompts/groups/:groupId - Update Prompt Group Security', () => { + let testGroup; + + beforeEach(async () => { + // Create a prompt group + testGroup = await PromptGroup.create({ + name: 'Security Test Group', + category: 'security-test', + author: testUsers.owner._id, + authorName: testUsers.owner.name, + productionId: new ObjectId(), + }); + + // Grant owner permissions + await grantPermission({ + principalType: PrincipalType.USER, + principalId: testUsers.owner._id, + resourceType: ResourceType.PROMPTGROUP, + resourceId: testGroup._id, + accessRoleId: AccessRoleIds.PROMPTGROUP_OWNER, + grantedBy: testUsers.owner._id, + }); + }); + + afterEach(async () => { + await PromptGroup.deleteMany({}); + await AclEntry.deleteMany({}); + }); + + it('should allow updating allowed fields (name, category, oneliner)', async () => { + const updateData = { + name: 'Updated Group Name', + category: 'updated-category', + oneliner: 'Updated description', + }; + + const response = await request(app) + .patch(`/api/prompts/groups/${testGroup._id}`) + .send(updateData) + .expect(200); + + expect(response.body.name).toBe(updateData.name); + expect(response.body.category).toBe(updateData.category); + expect(response.body.oneliner).toBe(updateData.oneliner); + }); + + it('should reject request with author field (400 Bad Request)', async () => { + const maliciousUpdate = { + name: 'Legit Update', + author: testUsers.noAccess._id.toString(), // Try to change ownership + }; + + const response = await request(app) + .patch(`/api/prompts/groups/${testGroup._id}`) + .send(maliciousUpdate) + .expect(400); + + // Verify the request was rejected + expect(response.body.error).toBe('Invalid request body'); + expect(response.body.details).toBeDefined(); + }); + + it('should reject request with authorName field (400 Bad Request)', async () => { + const maliciousUpdate = { + name: 'Legit Update', + authorName: 'Malicious Author Name', + }; + + const response = await request(app) + .patch(`/api/prompts/groups/${testGroup._id}`) + .send(maliciousUpdate) + .expect(400); + + // Verify the request was rejected + expect(response.body.error).toBe('Invalid request body'); + }); + + it('should reject request with _id field (400 Bad Request)', async () => { + const newId = new ObjectId(); + const maliciousUpdate = { + name: 'Legit Update', + _id: newId.toString(), + }; + + const response = await request(app) + .patch(`/api/prompts/groups/${testGroup._id}`) + .send(maliciousUpdate) + .expect(400); + + // Verify the request was rejected + expect(response.body.error).toBe('Invalid request body'); + }); + + it('should reject request with productionId field (400 Bad Request)', async () => { + const newProductionId = new ObjectId(); + const maliciousUpdate = { + name: 'Legit Update', + productionId: newProductionId.toString(), + }; + + const response = await request(app) + .patch(`/api/prompts/groups/${testGroup._id}`) + .send(maliciousUpdate) + .expect(400); + + // Verify the request was rejected + expect(response.body.error).toBe('Invalid request body'); + }); + + it('should reject request with createdAt field (400 Bad Request)', async () => { + const maliciousDate = new Date('2020-01-01'); + const maliciousUpdate = { + name: 'Legit Update', + createdAt: maliciousDate.toISOString(), + }; + + const response = await request(app) + .patch(`/api/prompts/groups/${testGroup._id}`) + .send(maliciousUpdate) + .expect(400); + + // Verify the request was rejected + expect(response.body.error).toBe('Invalid request body'); + }); + + it('should reject request with __v field (400 Bad Request)', async () => { + const maliciousUpdate = { + name: 'Legit Update', + __v: 999, + }; + + const response = await request(app) + .patch(`/api/prompts/groups/${testGroup._id}`) + .send(maliciousUpdate) + .expect(400); + + // Verify the request was rejected + expect(response.body.error).toBe('Invalid request body'); + }); + + it('should reject request with multiple sensitive fields (400 Bad Request)', async () => { + const maliciousUpdate = { + name: 'Legit Update', + author: testUsers.noAccess._id.toString(), + authorName: 'Hacker', + _id: new ObjectId().toString(), + productionId: new ObjectId().toString(), + createdAt: new Date('2020-01-01').toISOString(), + __v: 999, + }; + + const response = await request(app) + .patch(`/api/prompts/groups/${testGroup._id}`) + .send(maliciousUpdate) + .expect(400); + + // Verify the request was rejected with validation errors + expect(response.body.error).toBe('Invalid request body'); + expect(response.body.details).toBeDefined(); + expect(Array.isArray(response.body.details)).toBe(true); + }); + }); + describe('Pagination', () => { beforeEach(async () => { // Create multiple prompt groups for pagination testing diff --git a/packages/api/src/middleware/index.ts b/packages/api/src/middleware/index.ts index f3308a95a3..0aa5cf4f86 100644 --- a/packages/api/src/middleware/index.ts +++ b/packages/api/src/middleware/index.ts @@ -1,3 +1,4 @@ export * from './access'; export * from './error'; export * from './balance'; +export * from './json'; diff --git a/packages/api/src/middleware/json.spec.ts b/packages/api/src/middleware/json.spec.ts new file mode 100644 index 0000000000..8a7b7e2c10 --- /dev/null +++ b/packages/api/src/middleware/json.spec.ts @@ -0,0 +1,158 @@ +import { handleJsonParseError } from './json'; +import type { Request, Response, NextFunction } from 'express'; + +describe('handleJsonParseError', () => { + let req: Partial; + let res: Partial; + let next: NextFunction; + let jsonSpy: jest.Mock; + let statusSpy: jest.Mock; + + beforeEach(() => { + req = { + path: '/api/test', + method: 'POST', + ip: '127.0.0.1', + }; + + jsonSpy = jest.fn(); + statusSpy = jest.fn().mockReturnValue({ json: jsonSpy }); + + res = { + status: statusSpy, + json: jsonSpy, + }; + + next = jest.fn(); + }); + + describe('JSON parse errors', () => { + it('should handle JSON SyntaxError with 400 status', () => { + const err = new SyntaxError('Unexpected token < in JSON at position 0') as SyntaxError & { + status?: number; + body?: unknown; + }; + err.status = 400; + err.body = {}; + + handleJsonParseError(err, req as Request, res as Response, next); + + expect(statusSpy).toHaveBeenCalledWith(400); + expect(jsonSpy).toHaveBeenCalledWith({ + error: 'Invalid JSON format', + message: 'The request body contains malformed JSON', + }); + expect(next).not.toHaveBeenCalled(); + }); + + it('should not reflect user input in error message', () => { + const maliciousInput = ''; + const err = new SyntaxError( + `Unexpected token < in JSON at position 0: ${maliciousInput}`, + ) as SyntaxError & { + status?: number; + body?: unknown; + }; + err.status = 400; + err.body = maliciousInput; + + handleJsonParseError(err, req as Request, res as Response, next); + + expect(statusSpy).toHaveBeenCalledWith(400); + const errorResponse = jsonSpy.mock.calls[0][0]; + expect(errorResponse.message).not.toContain(maliciousInput); + expect(errorResponse.message).toBe('The request body contains malformed JSON'); + expect(next).not.toHaveBeenCalled(); + }); + + it('should handle JSON parse error with HTML tags in body', () => { + const err = new SyntaxError('Invalid JSON') as SyntaxError & { + status?: number; + body?: unknown; + }; + err.status = 400; + err.body = '

XSS

'; + + handleJsonParseError(err, req as Request, res as Response, next); + + expect(statusSpy).toHaveBeenCalledWith(400); + const errorResponse = jsonSpy.mock.calls[0][0]; + expect(errorResponse.message).not.toContain(''); + expect(errorResponse.message).not.toContain('', + '">', + ]; + + testCases.forEach((errorMsg) => { + const err = new SyntaxError(errorMsg) as SyntaxError & { + status?: number; + body?: unknown; + }; + err.status = 400; + err.body = errorMsg; + + jsonSpy.mockClear(); + statusSpy.mockClear(); + (next as jest.Mock).mockClear(); + + handleJsonParseError(err, req as Request, res as Response, next); + + const errorResponse = jsonSpy.mock.calls[0][0]; + // Verify the generic message is always returned, not the user input + expect(errorResponse.message).toBe('The request body contains malformed JSON'); + expect(errorResponse.error).toBe('Invalid JSON format'); + }); + }); + }); +}); diff --git a/packages/api/src/middleware/json.ts b/packages/api/src/middleware/json.ts new file mode 100644 index 0000000000..f292ef3a9f --- /dev/null +++ b/packages/api/src/middleware/json.ts @@ -0,0 +1,40 @@ +import { logger } from '@librechat/data-schemas'; +import type { Request, Response, NextFunction } from 'express'; + +/** + * Middleware to handle JSON parsing errors from express.json() + * Prevents user input from being reflected in error messages (XSS prevention) + * + * This middleware should be placed immediately after express.json() middleware. + * + * @param err - Error object from express.json() + * @param req - Express request object + * @param res - Express response object + * @param next - Express next function + * + * @example + * app.use(express.json({ limit: '3mb' })); + * app.use(handleJsonParseError); + */ +export function handleJsonParseError( + err: Error & { status?: number; body?: unknown }, + req: Request, + res: Response, + next: NextFunction, +): void { + if (err instanceof SyntaxError && err.status === 400 && 'body' in err) { + logger.warn('[JSON Parse Error] Invalid JSON received', { + path: req.path, + method: req.method, + ip: req.ip, + }); + + res.status(400).json({ + error: 'Invalid JSON format', + message: 'The request body contains malformed JSON', + }); + return; + } + + next(err); +} diff --git a/packages/api/src/prompts/index.ts b/packages/api/src/prompts/index.ts index b92957abb3..df3a9fe148 100644 --- a/packages/api/src/prompts/index.ts +++ b/packages/api/src/prompts/index.ts @@ -1,2 +1,3 @@ export * from './format'; export * from './migration'; +export * from './schemas'; diff --git a/packages/api/src/prompts/schemas.spec.ts b/packages/api/src/prompts/schemas.spec.ts new file mode 100644 index 0000000000..0008e31b51 --- /dev/null +++ b/packages/api/src/prompts/schemas.spec.ts @@ -0,0 +1,222 @@ +import { + updatePromptGroupSchema, + validatePromptGroupUpdate, + safeValidatePromptGroupUpdate, +} from './schemas'; + +describe('updatePromptGroupSchema', () => { + describe('allowed fields', () => { + it('should accept valid name field', () => { + const result = updatePromptGroupSchema.safeParse({ name: 'Test Group' }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.name).toBe('Test Group'); + } + }); + + it('should accept valid oneliner field', () => { + const result = updatePromptGroupSchema.safeParse({ oneliner: 'A short description' }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.oneliner).toBe('A short description'); + } + }); + + it('should accept valid category field', () => { + const result = updatePromptGroupSchema.safeParse({ category: 'testing' }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.category).toBe('testing'); + } + }); + + it('should accept valid projectIds array', () => { + const result = updatePromptGroupSchema.safeParse({ + projectIds: ['proj1', 'proj2'], + }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.projectIds).toEqual(['proj1', 'proj2']); + } + }); + + it('should accept valid removeProjectIds array', () => { + const result = updatePromptGroupSchema.safeParse({ + removeProjectIds: ['proj1'], + }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.removeProjectIds).toEqual(['proj1']); + } + }); + + it('should accept valid command field', () => { + const result = updatePromptGroupSchema.safeParse({ command: 'my-command-123' }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.command).toBe('my-command-123'); + } + }); + + it('should accept null command field', () => { + const result = updatePromptGroupSchema.safeParse({ command: null }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.command).toBeNull(); + } + }); + + it('should accept multiple valid fields', () => { + const input = { + name: 'Updated Name', + category: 'new-category', + oneliner: 'New description', + }; + const result = updatePromptGroupSchema.safeParse(input); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data).toEqual(input); + } + }); + + it('should accept empty object', () => { + const result = updatePromptGroupSchema.safeParse({}); + expect(result.success).toBe(true); + }); + }); + + describe('security - strips sensitive fields', () => { + it('should reject author field', () => { + const result = updatePromptGroupSchema.safeParse({ + name: 'Test', + author: '507f1f77bcf86cd799439011', + }); + expect(result.success).toBe(false); + }); + + it('should reject authorName field', () => { + const result = updatePromptGroupSchema.safeParse({ + name: 'Test', + authorName: 'Malicious Author', + }); + expect(result.success).toBe(false); + }); + + it('should reject _id field', () => { + const result = updatePromptGroupSchema.safeParse({ + name: 'Test', + _id: '507f1f77bcf86cd799439011', + }); + expect(result.success).toBe(false); + }); + + it('should reject productionId field', () => { + const result = updatePromptGroupSchema.safeParse({ + name: 'Test', + productionId: '507f1f77bcf86cd799439011', + }); + expect(result.success).toBe(false); + }); + + it('should reject createdAt field', () => { + const result = updatePromptGroupSchema.safeParse({ + name: 'Test', + createdAt: new Date().toISOString(), + }); + expect(result.success).toBe(false); + }); + + it('should reject updatedAt field', () => { + const result = updatePromptGroupSchema.safeParse({ + name: 'Test', + updatedAt: new Date().toISOString(), + }); + expect(result.success).toBe(false); + }); + + it('should reject __v field', () => { + const result = updatePromptGroupSchema.safeParse({ + name: 'Test', + __v: 999, + }); + expect(result.success).toBe(false); + }); + + it('should reject multiple sensitive fields in a single request', () => { + const result = updatePromptGroupSchema.safeParse({ + name: 'Legit Name', + author: '507f1f77bcf86cd799439011', + authorName: 'Hacker', + _id: 'newid123', + productionId: 'prodid456', + createdAt: '2020-01-01T00:00:00.000Z', + __v: 999, + }); + expect(result.success).toBe(false); + }); + }); + + describe('validation rules', () => { + it('should reject empty name', () => { + const result = updatePromptGroupSchema.safeParse({ name: '' }); + expect(result.success).toBe(false); + }); + + it('should reject name exceeding max length', () => { + const result = updatePromptGroupSchema.safeParse({ name: 'a'.repeat(256) }); + expect(result.success).toBe(false); + }); + + it('should reject oneliner exceeding max length', () => { + const result = updatePromptGroupSchema.safeParse({ oneliner: 'a'.repeat(501) }); + expect(result.success).toBe(false); + }); + + it('should reject category exceeding max length', () => { + const result = updatePromptGroupSchema.safeParse({ category: 'a'.repeat(101) }); + expect(result.success).toBe(false); + }); + + it('should reject command with invalid characters (uppercase)', () => { + const result = updatePromptGroupSchema.safeParse({ command: 'MyCommand' }); + expect(result.success).toBe(false); + }); + + it('should reject command with invalid characters (spaces)', () => { + const result = updatePromptGroupSchema.safeParse({ command: 'my command' }); + expect(result.success).toBe(false); + }); + + it('should reject command with invalid characters (special)', () => { + const result = updatePromptGroupSchema.safeParse({ command: 'my_command!' }); + expect(result.success).toBe(false); + }); + }); +}); + +describe('validatePromptGroupUpdate', () => { + it('should return validated data for valid input', () => { + const input = { name: 'Test', category: 'testing' }; + const result = validatePromptGroupUpdate(input); + expect(result).toEqual(input); + }); + + it('should throw ZodError for invalid input', () => { + expect(() => validatePromptGroupUpdate({ author: 'malicious-id' })).toThrow(); + }); +}); + +describe('safeValidatePromptGroupUpdate', () => { + it('should return success true for valid input', () => { + const result = safeValidatePromptGroupUpdate({ name: 'Test' }); + expect(result.success).toBe(true); + }); + + it('should return success false for invalid input with errors', () => { + const result = safeValidatePromptGroupUpdate({ author: 'malicious-id' }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.errors.length).toBeGreaterThan(0); + } + }); +}); diff --git a/packages/api/src/prompts/schemas.ts b/packages/api/src/prompts/schemas.ts new file mode 100644 index 0000000000..628c07d954 --- /dev/null +++ b/packages/api/src/prompts/schemas.ts @@ -0,0 +1,53 @@ +import { z } from 'zod'; +import { Constants } from 'librechat-data-provider'; + +/** + * Schema for validating prompt group update payloads. + * Only allows fields that users should be able to modify. + * Sensitive fields like author, authorName, _id, productionId, etc. are excluded. + */ +export const updatePromptGroupSchema = z + .object({ + /** The name of the prompt group */ + name: z.string().min(1).max(255).optional(), + /** Short description/oneliner for the prompt group */ + oneliner: z.string().max(500).optional(), + /** Category for organizing prompt groups */ + category: z.string().max(100).optional(), + /** Project IDs to add for sharing */ + projectIds: z.array(z.string()).optional(), + /** Project IDs to remove from sharing */ + removeProjectIds: z.array(z.string()).optional(), + /** Command shortcut for the prompt group */ + command: z + .string() + .max(Constants.COMMANDS_MAX_LENGTH as number) + .regex(/^[a-z0-9-]*$/, { + message: 'Command must only contain lowercase alphanumeric characters and hyphens', + }) + .optional() + .nullable(), + }) + .strict(); + +export type TUpdatePromptGroupSchema = z.infer; + +/** + * Validates and sanitizes a prompt group update payload. + * Returns only the allowed fields, stripping any sensitive fields. + * @param data - The raw request body to validate + * @returns The validated and sanitized payload + * @throws ZodError if validation fails + */ +export function validatePromptGroupUpdate(data: unknown): TUpdatePromptGroupSchema { + return updatePromptGroupSchema.parse(data); +} + +/** + * Safely validates a prompt group update payload without throwing. + * @param data - The raw request body to validate + * @returns A SafeParseResult with either the validated data or validation errors + */ +export function safeValidatePromptGroupUpdate(data: unknown) { + return updatePromptGroupSchema.safeParse(data); +}