From f67bbb2bc5e1722c91a106f166629a026c6f0d6a Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 14 Mar 2026 03:09:26 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=B9=20fix:=20Sanitize=20Artifact=20Fil?= =?UTF-8?q?enames=20in=20Code=20Execution=20Output=20(#12222)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: sanitize artifact filenames to prevent path traversal in code output * test: Mock sanitizeFilename function in process.spec.js to return the original filename - Added a mock implementation for the `sanitizeFilename` function in the `process.spec.js` test file to return the original filename, ensuring that tests can run without altering the filename during the testing process. * fix: use path.relative for traversal check, sanitize all filenames, add security logging - Replace startsWith with path.relative pattern in saveLocalBuffer, consistent with deleteLocalFile and getLocalFileStream in the same file - Hoist sanitizeFilename call before the image/non-image branch so both code paths store the sanitized name in MongoDB - Log a warning when sanitizeFilename mutates a filename (potential traversal) - Log a specific warning when saveLocalBuffer throws a traversal error, so security events are distinguishable from generic network errors in the catch * test: improve traversal test coverage and remove mock reimplementation - Remove partial sanitizeFilename reimplementation from process-traversal tests; use controlled mock returns to verify processCodeOutput wiring instead - Add test for image branch sanitization - Use mkdtempSync for test isolation in crud-traversal to avoid parallel worker collisions - Add prefix-collision bypass test case (../user10/evil vs user1 directory) * fix: use path.relative in isValidPath to prevent prefix-collision bypass Pre-existing startsWith check without path separator had the same class of prefix-collision vulnerability fixed in saveLocalBuffer. --- .../Code/__tests__/process-traversal.spec.js | 124 ++++++++++++++++++ api/server/services/Files/Code/process.js | 20 ++- .../services/Files/Code/process.spec.js | 1 + .../Local/__tests__/crud-traversal.spec.js | 69 ++++++++++ api/server/services/Files/Local/crud.js | 16 ++- 5 files changed, 221 insertions(+), 9 deletions(-) create mode 100644 api/server/services/Files/Code/__tests__/process-traversal.spec.js create mode 100644 api/server/services/Files/Local/__tests__/crud-traversal.spec.js diff --git a/api/server/services/Files/Code/__tests__/process-traversal.spec.js b/api/server/services/Files/Code/__tests__/process-traversal.spec.js new file mode 100644 index 0000000000..2db366d06b --- /dev/null +++ b/api/server/services/Files/Code/__tests__/process-traversal.spec.js @@ -0,0 +1,124 @@ +jest.mock('uuid', () => ({ v4: jest.fn(() => 'mock-uuid') })); + +jest.mock('@librechat/data-schemas', () => ({ + logger: { warn: jest.fn(), debug: jest.fn(), error: jest.fn() }, +})); + +jest.mock('@librechat/agents', () => ({ + getCodeBaseURL: jest.fn(() => 'http://localhost:8000'), +})); + +const mockSanitizeFilename = jest.fn(); + +jest.mock('@librechat/api', () => ({ + logAxiosError: jest.fn(), + getBasePath: jest.fn(() => ''), + sanitizeFilename: mockSanitizeFilename, +})); + +jest.mock('librechat-data-provider', () => ({ + ...jest.requireActual('librechat-data-provider'), + mergeFileConfig: jest.fn(() => ({ serverFileSizeLimit: 100 * 1024 * 1024 })), + getEndpointFileConfig: jest.fn(() => ({ + fileSizeLimit: 100 * 1024 * 1024, + supportedMimeTypes: ['*/*'], + })), + fileConfig: { checkType: jest.fn(() => true) }, +})); + +jest.mock('~/models', () => ({ + createFile: jest.fn().mockResolvedValue({}), + getFiles: jest.fn().mockResolvedValue([]), + updateFile: jest.fn(), + claimCodeFile: jest.fn().mockResolvedValue({ file_id: 'mock-uuid', usage: 0 }), +})); + +const mockSaveBuffer = jest.fn().mockResolvedValue('/uploads/user123/mock-uuid__output.csv'); + +jest.mock('~/server/services/Files/strategies', () => ({ + getStrategyFunctions: jest.fn(() => ({ + saveBuffer: mockSaveBuffer, + })), +})); + +jest.mock('~/server/services/Files/permissions', () => ({ + filterFilesByAgentAccess: jest.fn().mockResolvedValue([]), +})); + +jest.mock('~/server/services/Files/images/convert', () => ({ + convertImage: jest.fn(), +})); + +jest.mock('~/server/utils', () => ({ + determineFileType: jest.fn().mockResolvedValue({ mime: 'text/csv' }), +})); + +jest.mock('axios', () => + jest.fn().mockResolvedValue({ + data: Buffer.from('file-content'), + }), +); + +const { createFile } = require('~/models'); +const { processCodeOutput } = require('../process'); + +const baseParams = { + req: { + user: { id: 'user123' }, + config: { + fileStrategy: 'local', + imageOutputType: 'webp', + fileConfig: {}, + }, + }, + id: 'code-file-id', + apiKey: 'test-key', + toolCallId: 'tool-1', + conversationId: 'conv-1', + messageId: 'msg-1', + session_id: 'session-1', +}; + +describe('processCodeOutput path traversal protection', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + test('sanitizeFilename is called with the raw artifact name', async () => { + mockSanitizeFilename.mockReturnValueOnce('output.csv'); + await processCodeOutput({ ...baseParams, name: 'output.csv' }); + expect(mockSanitizeFilename).toHaveBeenCalledWith('output.csv'); + }); + + test('sanitized name is used in saveBuffer fileName', async () => { + mockSanitizeFilename.mockReturnValueOnce('sanitized-name.txt'); + await processCodeOutput({ ...baseParams, name: '../../../tmp/poc.txt' }); + + expect(mockSanitizeFilename).toHaveBeenCalledWith('../../../tmp/poc.txt'); + const call = mockSaveBuffer.mock.calls[0][0]; + expect(call.fileName).toBe('mock-uuid__sanitized-name.txt'); + }); + + test('sanitized name is stored as filename in the file record', async () => { + mockSanitizeFilename.mockReturnValueOnce('safe-output.csv'); + await processCodeOutput({ ...baseParams, name: 'unsafe/../../output.csv' }); + + const fileArg = createFile.mock.calls[0][0]; + expect(fileArg.filename).toBe('safe-output.csv'); + }); + + test('sanitized name is used for image file records', async () => { + const { convertImage } = require('~/server/services/Files/images/convert'); + convertImage.mockResolvedValueOnce({ + filepath: '/images/user123/mock-uuid.webp', + bytes: 100, + }); + + mockSanitizeFilename.mockReturnValueOnce('safe-chart.png'); + await processCodeOutput({ ...baseParams, name: '../../../chart.png' }); + + expect(mockSanitizeFilename).toHaveBeenCalledWith('../../../chart.png'); + const fileArg = createFile.mock.calls[0][0]; + expect(fileArg.filename).toBe('safe-chart.png'); + }); +}); diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 3f0bfcfc87..e878b00255 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -3,7 +3,7 @@ const { v4 } = require('uuid'); const axios = require('axios'); const { logger } = require('@librechat/data-schemas'); const { getCodeBaseURL } = require('@librechat/agents'); -const { logAxiosError, getBasePath } = require('@librechat/api'); +const { logAxiosError, getBasePath, sanitizeFilename } = require('@librechat/api'); const { Tools, megabyte, @@ -146,6 +146,13 @@ const processCodeOutput = async ({ ); } + const safeName = sanitizeFilename(name); + if (safeName !== name) { + logger.warn( + `[processCodeOutput] Filename sanitized: "${name}" -> "${safeName}" | conv=${conversationId}`, + ); + } + if (isImage) { const usage = isUpdate ? (claimed.usage ?? 0) + 1 : 1; const _file = await convertImage(req, buffer, 'high', `${file_id}${fileExt}`); @@ -156,7 +163,7 @@ const processCodeOutput = async ({ file_id, messageId, usage, - filename: name, + filename: safeName, conversationId, user: req.user.id, type: `image/${appConfig.imageOutputType}`, @@ -200,7 +207,7 @@ const processCodeOutput = async ({ ); } - const fileName = `${file_id}__${name}`; + const fileName = `${file_id}__${safeName}`; const filepath = await saveBuffer({ userId: req.user.id, buffer, @@ -213,7 +220,7 @@ const processCodeOutput = async ({ filepath, messageId, object: 'file', - filename: name, + filename: safeName, type: mimeType, conversationId, user: req.user.id, @@ -229,6 +236,11 @@ const processCodeOutput = async ({ await createFile(file, true); return Object.assign(file, { messageId, toolCallId }); } catch (error) { + if (error?.message === 'Path traversal detected in filename') { + logger.warn( + `[processCodeOutput] Path traversal blocked for file "${name}" | conv=${conversationId}`, + ); + } logAxiosError({ message: 'Error downloading/processing code environment file', error, diff --git a/api/server/services/Files/Code/process.spec.js b/api/server/services/Files/Code/process.spec.js index f01a623f90..b89a6c6307 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -58,6 +58,7 @@ jest.mock('@librechat/agents', () => ({ jest.mock('@librechat/api', () => ({ logAxiosError: jest.fn(), getBasePath: jest.fn(() => ''), + sanitizeFilename: jest.fn((name) => name), })); // Mock models diff --git a/api/server/services/Files/Local/__tests__/crud-traversal.spec.js b/api/server/services/Files/Local/__tests__/crud-traversal.spec.js new file mode 100644 index 0000000000..57ba221d68 --- /dev/null +++ b/api/server/services/Files/Local/__tests__/crud-traversal.spec.js @@ -0,0 +1,69 @@ +jest.mock('@librechat/api', () => ({ deleteRagFile: jest.fn() })); +jest.mock('@librechat/data-schemas', () => ({ + logger: { warn: jest.fn(), error: jest.fn() }, +})); + +const mockTmpBase = require('fs').mkdtempSync( + require('path').join(require('os').tmpdir(), 'crud-traversal-'), +); + +jest.mock('~/config/paths', () => { + const path = require('path'); + return { + publicPath: path.join(mockTmpBase, 'public'), + uploads: path.join(mockTmpBase, 'uploads'), + }; +}); + +const fs = require('fs'); +const path = require('path'); +const { saveLocalBuffer } = require('../crud'); + +describe('saveLocalBuffer path containment', () => { + beforeAll(() => { + fs.mkdirSync(path.join(mockTmpBase, 'public', 'images'), { recursive: true }); + fs.mkdirSync(path.join(mockTmpBase, 'uploads'), { recursive: true }); + }); + + afterAll(() => { + fs.rmSync(mockTmpBase, { recursive: true, force: true }); + }); + + test('rejects filenames with path traversal sequences', async () => { + await expect( + saveLocalBuffer({ + userId: 'user1', + buffer: Buffer.from('malicious'), + fileName: '../../../etc/passwd', + basePath: 'uploads', + }), + ).rejects.toThrow('Path traversal detected in filename'); + }); + + test('rejects prefix-collision traversal (startsWith bypass)', async () => { + fs.mkdirSync(path.join(mockTmpBase, 'uploads', 'user10'), { recursive: true }); + await expect( + saveLocalBuffer({ + userId: 'user1', + buffer: Buffer.from('malicious'), + fileName: '../user10/evil', + basePath: 'uploads', + }), + ).rejects.toThrow('Path traversal detected in filename'); + }); + + test('allows normal filenames', async () => { + const result = await saveLocalBuffer({ + userId: 'user1', + buffer: Buffer.from('safe content'), + fileName: 'file-id__output.csv', + basePath: 'uploads', + }); + + expect(result).toBe('/uploads/user1/file-id__output.csv'); + + const filePath = path.join(mockTmpBase, 'uploads', 'user1', 'file-id__output.csv'); + expect(fs.existsSync(filePath)).toBe(true); + fs.unlinkSync(filePath); + }); +}); diff --git a/api/server/services/Files/Local/crud.js b/api/server/services/Files/Local/crud.js index 1f38a01f83..c86774d472 100644 --- a/api/server/services/Files/Local/crud.js +++ b/api/server/services/Files/Local/crud.js @@ -78,7 +78,13 @@ async function saveLocalBuffer({ userId, buffer, fileName, basePath = 'images' } fs.mkdirSync(directoryPath, { recursive: true }); } - fs.writeFileSync(path.join(directoryPath, fileName), buffer); + const resolvedDir = path.resolve(directoryPath); + const resolvedPath = path.resolve(resolvedDir, fileName); + const rel = path.relative(resolvedDir, resolvedPath); + if (rel.startsWith('..') || path.isAbsolute(rel) || rel.includes(`..${path.sep}`)) { + throw new Error('Path traversal detected in filename'); + } + fs.writeFileSync(resolvedPath, buffer); const filePath = path.posix.join('/', basePath, userId, fileName); @@ -165,9 +171,8 @@ async function getLocalFileURL({ fileName, basePath = 'images' }) { } /** - * Validates if a given filepath is within a specified subdirectory under a base path. This function constructs - * the expected base path using the base, subfolder, and user id from the request, and then checks if the - * provided filepath starts with this constructed base path. + * Validates that a filepath is strictly contained within a subdirectory under a base path, + * using path.relative to prevent prefix-collision bypasses. * * @param {ServerRequest} req - The request object from Express. It should contain a `user` property with an `id`. * @param {string} base - The base directory path. @@ -180,7 +185,8 @@ async function getLocalFileURL({ fileName, basePath = 'images' }) { const isValidPath = (req, base, subfolder, filepath) => { const normalizedBase = path.resolve(base, subfolder, req.user.id); const normalizedFilepath = path.resolve(filepath); - return normalizedFilepath.startsWith(normalizedBase); + const rel = path.relative(normalizedBase, normalizedFilepath); + return !rel.startsWith('..') && !path.isAbsolute(rel) && !rel.includes(`..${path.sep}`); }; /**