From e079fc4900113711a704933e3059fdf215e50317 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 15 Mar 2026 10:39:42 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=8E=20fix:=20Enforce=20File=20Count=20?= =?UTF-8?q?and=20Size=20Limits=20Across=20All=20Attachment=20Paths=20(#122?= =?UTF-8?q?39)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🐛 fix: Enforce fileLimit and totalSizeLimit in Attached Files panel The Files side panel (PanelTable) was not checking fileLimit or totalSizeLimit from fileConfig when attaching previously uploaded files, allowing users to bypass per-endpoint file count and total size limits. * 🔧 fix: Address review findings on file limit enforcement - Fix totalSizeLimit double-counting size of already-attached files - Clarify fileLimit error message: "File limit reached: N files (endpoint)" - Replace Array.from(...).reduce with for...of loop to avoid intermediate allocation - Extract inline `type TFile` into standalone `import type` per project conventions * ✅ test: Add PanelTable handleFileClick file limit tests Cover fileLimit guard, totalSizeLimit guard, passing case, double-count prevention for re-attached files, and boundary case. * 🔧 test: Harden PanelTable test mock setup - Use explicit endpoint key matching mockConversation.endpoint instead of relying on default fallback behavior - Add supportedMimeTypes to mock config for explicit MIME coverage - Throw on missing filename cell in clickFilenameCell to prevent silent false-positive blocking assertions * ♻️ refactor: Align file validation ordering and messaging across upload paths - Reorder handleFileClick checks to match validateFiles: disabled → fileLimit → fileSizeLimit → checkType → totalSizeLimit - Change fileSizeLimit comparison from > to >= in handleFileClick to match validateFiles behavior - Align validateFiles error strings with localized key wording: "File limit reached:", "File size limit exceeded:", etc. - Remove stray console.log in validateFiles MIME-type check * ✅ test: Add validateFiles unit tests for both paths' consistency 13 tests covering disabled, empty, fileLimit (reject + boundary), fileSizeLimit (>= at limit + under limit), checkType, totalSizeLimit (reject + at limit), duplicate detection, and check ordering. Ensures both validateFiles and handleFileClick enforce the same validation rules in the same order. --- .../components/SidePanel/Files/PanelTable.tsx | 38 ++- .../Files/__tests__/PanelTable.spec.tsx | 239 ++++++++++++++++++ client/src/locales/en/translation.json | 2 + .../src/utils/__tests__/validateFiles.spec.ts | 172 +++++++++++++ client/src/utils/files.ts | 9 +- 5 files changed, 448 insertions(+), 12 deletions(-) create mode 100644 client/src/components/SidePanel/Files/__tests__/PanelTable.spec.tsx create mode 100644 client/src/utils/__tests__/validateFiles.spec.ts diff --git a/client/src/components/SidePanel/Files/PanelTable.tsx b/client/src/components/SidePanel/Files/PanelTable.tsx index 2fc8f7031b..e67e16abdd 100644 --- a/client/src/components/SidePanel/Files/PanelTable.tsx +++ b/client/src/components/SidePanel/Files/PanelTable.tsx @@ -24,14 +24,14 @@ import { type ColumnFiltersState, } from '@tanstack/react-table'; import { - fileConfig as defaultFileConfig, - checkOpenAIStorage, - mergeFileConfig, megabyte, + mergeFileConfig, + checkOpenAIStorage, isAssistantsEndpoint, getEndpointFileConfig, - type TFile, + fileConfig as defaultFileConfig, } from 'librechat-data-provider'; +import type { TFile } from 'librechat-data-provider'; import { MyFilesModal } from '~/components/Chat/Input/Files/MyFilesModal'; import { useFileMapContext, useChatContext } from '~/Providers'; import { useLocalize, useUpdateFiles } from '~/hooks'; @@ -86,7 +86,7 @@ export default function DataTable({ columns, data }: DataTablePro const fileMap = useFileMapContext(); const { showToast } = useToastContext(); - const { setFiles, conversation } = useChatContext(); + const { files, setFiles, conversation } = useChatContext(); const { data: fileConfig = null } = useGetFileConfig({ select: (data) => mergeFileConfig(data), }); @@ -142,7 +142,15 @@ export default function DataTable({ columns, data }: DataTablePro return; } - if (fileData.bytes > (endpointFileConfig.fileSizeLimit ?? Number.MAX_SAFE_INTEGER)) { + if (endpointFileConfig.fileLimit && files.size >= endpointFileConfig.fileLimit) { + showToast({ + message: `${localize('com_ui_attach_error_limit')} ${endpointFileConfig.fileLimit} files (${endpoint})`, + status: 'error', + }); + return; + } + + if (fileData.bytes >= (endpointFileConfig.fileSizeLimit ?? Number.MAX_SAFE_INTEGER)) { showToast({ message: `${localize('com_ui_attach_error_size')} ${ (endpointFileConfig.fileSizeLimit ?? 0) / megabyte @@ -160,6 +168,22 @@ export default function DataTable({ columns, data }: DataTablePro return; } + if (endpointFileConfig.totalSizeLimit) { + const existing = files.get(fileData.file_id); + let currentTotalSize = 0; + for (const f of files.values()) { + currentTotalSize += f.size; + } + currentTotalSize -= existing?.size ?? 0; + if (currentTotalSize + fileData.bytes > endpointFileConfig.totalSizeLimit) { + showToast({ + message: `${localize('com_ui_attach_error_total_size')} ${endpointFileConfig.totalSizeLimit / megabyte} MB (${endpoint})`, + status: 'error', + }); + return; + } + } + addFile({ progress: 1, attached: true, @@ -175,7 +199,7 @@ export default function DataTable({ columns, data }: DataTablePro metadata: fileData.metadata, }); }, - [addFile, fileMap, conversation, localize, showToast, fileConfig], + [addFile, files, fileMap, conversation, localize, showToast, fileConfig], ); const filenameFilter = table.getColumn('filename')?.getFilterValue() as string; diff --git a/client/src/components/SidePanel/Files/__tests__/PanelTable.spec.tsx b/client/src/components/SidePanel/Files/__tests__/PanelTable.spec.tsx new file mode 100644 index 0000000000..2639d3c100 --- /dev/null +++ b/client/src/components/SidePanel/Files/__tests__/PanelTable.spec.tsx @@ -0,0 +1,239 @@ +import React from 'react'; +import { render, screen, fireEvent } from '@testing-library/react'; +import { FileSources } from 'librechat-data-provider'; +import type { TFile } from 'librechat-data-provider'; +import type { ExtendedFile } from '~/common'; +import DataTable from '../PanelTable'; +import { columns } from '../PanelColumns'; + +const mockShowToast = jest.fn(); +const mockAddFile = jest.fn(); + +let mockFileMap: Record = {}; +let mockFiles: Map = new Map(); +let mockConversation: Record | null = { endpoint: 'openAI' }; +let mockRawFileConfig: Record | null = { + endpoints: { + openAI: { fileLimit: 10, supportedMimeTypes: ['application/pdf', 'text/plain'] }, + }, +}; + +jest.mock('@librechat/client', () => ({ + Table: ({ children, ...props }: { children: React.ReactNode }) => ( + {children}
+ ), + Button: ({ + children, + ...props + }: { children: React.ReactNode } & React.ButtonHTMLAttributes) => ( + + ), + TableRow: ({ children, ...props }: { children: React.ReactNode }) => ( + {children} + ), + TableHead: ({ children, ...props }: { children: React.ReactNode }) => ( + {children} + ), + TableBody: ({ children, ...props }: { children: React.ReactNode }) => ( + {children} + ), + TableCell: ({ + children, + ...props + }: { children: React.ReactNode } & React.TdHTMLAttributes) => ( + {children} + ), + FilterInput: () => , + TableHeader: ({ children, ...props }: { children: React.ReactNode }) => ( + {children} + ), + useToastContext: () => ({ showToast: mockShowToast }), +})); + +jest.mock('~/Providers', () => ({ + useFileMapContext: () => mockFileMap, + useChatContext: () => ({ + files: mockFiles, + setFiles: jest.fn(), + conversation: mockConversation, + }), +})); + +jest.mock('~/hooks', () => ({ + useLocalize: () => (key: string) => key, + useUpdateFiles: () => ({ addFile: mockAddFile }), +})); + +jest.mock('~/data-provider', () => ({ + useGetFileConfig: ({ select }: { select?: (d: unknown) => unknown }) => ({ + data: select != null ? select(mockRawFileConfig) : mockRawFileConfig, + }), +})); + +jest.mock('~/components/Chat/Input/Files/MyFilesModal', () => ({ + MyFilesModal: () => null, +})); + +jest.mock('../PanelFileCell', () => ({ row }: { row: { original: TFile } }) => ( + {row.original?.filename} +)); + +function makeFile(overrides: Partial = {}): TFile { + return { + user: 'user-1', + file_id: 'file-1', + bytes: 1024, + embedded: false, + filename: 'test.pdf', + filepath: '/files/test.pdf', + object: 'file', + type: 'application/pdf', + usage: 0, + source: FileSources.local, + ...overrides, + }; +} + +function makeExtendedFile(overrides: Partial = {}): ExtendedFile { + return { + file_id: 'ext-1', + size: 1024, + progress: 1, + source: FileSources.local, + ...overrides, + }; +} + +function renderTable(data: TFile[]) { + return render(); +} + +function clickFilenameCell() { + const cells = screen.getAllByRole('button'); + const filenameCell = cells.find( + (cell) => cell.tagName === 'TD' && cell.textContent && !cell.textContent.includes('com_ui_'), + ); + if (!filenameCell) { + throw new Error('Could not find filename cell with role="button" — check mock setup'); + } + fireEvent.click(filenameCell); + return filenameCell; +} + +describe('PanelTable handleFileClick', () => { + beforeEach(() => { + mockShowToast.mockClear(); + mockAddFile.mockClear(); + mockFiles = new Map(); + mockConversation = { endpoint: 'openAI' }; + mockRawFileConfig = { + endpoints: { + openAI: { + fileLimit: 5, + totalSizeLimit: 10, + supportedMimeTypes: ['application/pdf', 'text/plain'], + }, + }, + }; + }); + + it('calls addFile when within file limits', () => { + const file = makeFile(); + mockFileMap = { [file.file_id]: file }; + + renderTable([file]); + clickFilenameCell(); + + expect(mockAddFile).toHaveBeenCalledTimes(1); + expect(mockAddFile).toHaveBeenCalledWith( + expect.objectContaining({ + file_id: file.file_id, + attached: true, + progress: 1, + }), + ); + expect(mockShowToast).not.toHaveBeenCalledWith(expect.objectContaining({ status: 'error' })); + }); + + it('blocks attachment when fileLimit is reached', () => { + const file = makeFile({ file_id: 'new-file', filename: 'new.pdf' }); + mockFileMap = { [file.file_id]: file }; + + mockFiles = new Map( + Array.from({ length: 5 }, (_, i) => [ + `existing-${i}`, + makeExtendedFile({ file_id: `existing-${i}` }), + ]), + ); + + renderTable([file]); + clickFilenameCell(); + + expect(mockAddFile).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('com_ui_attach_error_limit'), + status: 'error', + }), + ); + }); + + it('blocks attachment when totalSizeLimit would be exceeded', () => { + const MB = 1024 * 1024; + const largeFile = makeFile({ file_id: 'large-file', bytes: 6 * MB }); + mockFileMap = { [largeFile.file_id]: largeFile }; + + mockFiles = new Map([ + ['existing-1', makeExtendedFile({ file_id: 'existing-1', size: 5 * MB })], + ]); + + renderTable([largeFile]); + clickFilenameCell(); + + expect(mockAddFile).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('com_ui_attach_error_total_size'), + status: 'error', + }), + ); + }); + + it('does not double-count size of already-attached file', () => { + const MB = 1024 * 1024; + const file = makeFile({ file_id: 'reattach', bytes: 5 * MB }); + mockFileMap = { [file.file_id]: file }; + + mockFiles = new Map([ + ['reattach', makeExtendedFile({ file_id: 'reattach', size: 5 * MB })], + ['other', makeExtendedFile({ file_id: 'other', size: 4 * MB })], + ]); + + renderTable([file]); + clickFilenameCell(); + + expect(mockAddFile).toHaveBeenCalledTimes(1); + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('com_ui_attach_error_total_size'), + }), + ); + }); + + it('allows attachment when just under fileLimit', () => { + const file = makeFile({ file_id: 'under-limit' }); + mockFileMap = { [file.file_id]: file }; + + mockFiles = new Map( + Array.from({ length: 4 }, (_, i) => [ + `existing-${i}`, + makeExtendedFile({ file_id: `existing-${i}` }), + ]), + ); + + renderTable([file]); + clickFilenameCell(); + + expect(mockAddFile).toHaveBeenCalledTimes(1); + }); +}); diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index 196ea2ad4a..f45cdd5f8c 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -748,7 +748,9 @@ "com_ui_attach_error": "Cannot attach file. Create or select a conversation, or try refreshing the page.", "com_ui_attach_error_disabled": "File uploads are disabled for this endpoint", "com_ui_attach_error_openai": "Cannot attach Assistant files to other endpoints", + "com_ui_attach_error_limit": "File limit reached:", "com_ui_attach_error_size": "File size limit exceeded for endpoint:", + "com_ui_attach_error_total_size": "Total file size limit exceeded for endpoint:", "com_ui_attach_error_type": "Unsupported file type for endpoint:", "com_ui_attach_remove": "Remove file", "com_ui_attach_warn_endpoint": "Non-Assistant files may be ignored without a compatible tool", diff --git a/client/src/utils/__tests__/validateFiles.spec.ts b/client/src/utils/__tests__/validateFiles.spec.ts new file mode 100644 index 0000000000..6d690bf62a --- /dev/null +++ b/client/src/utils/__tests__/validateFiles.spec.ts @@ -0,0 +1,172 @@ +import { megabyte, fileConfig as defaultFileConfig } from 'librechat-data-provider'; +import type { EndpointFileConfig, FileConfig } from 'librechat-data-provider'; +import type { ExtendedFile } from '~/common'; +import { validateFiles } from '../files'; + +const supportedMimeTypes = defaultFileConfig.endpoints.default.supportedMimeTypes; + +function makeEndpointConfig(overrides: Partial = {}): EndpointFileConfig { + return { + fileLimit: 10, + fileSizeLimit: 25 * megabyte, + totalSizeLimit: 100 * megabyte, + supportedMimeTypes, + disabled: false, + ...overrides, + }; +} + +function makeFile(name: string, type: string, size: number): File { + const content = new ArrayBuffer(size); + return new File([content], name, { type }); +} + +function makeExtendedFile(overrides: Partial = {}): ExtendedFile { + return { + file_id: 'ext-1', + size: 1024, + progress: 1, + type: 'application/pdf', + ...overrides, + }; +} + +describe('validateFiles', () => { + let setError: jest.Mock; + let files: Map; + let endpointFileConfig: EndpointFileConfig; + const fileConfig: FileConfig | null = null; + + beforeEach(() => { + setError = jest.fn(); + files = new Map(); + endpointFileConfig = makeEndpointConfig(); + }); + + it('returns true when all checks pass', () => { + const fileList = [makeFile('doc.pdf', 'application/pdf', 1024)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(true); + expect(setError).not.toHaveBeenCalled(); + }); + + it('rejects when endpoint is disabled', () => { + endpointFileConfig = makeEndpointConfig({ disabled: true }); + const fileList = [makeFile('doc.pdf', 'application/pdf', 1024)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(false); + expect(setError).toHaveBeenCalledWith('com_ui_attach_error_disabled'); + }); + + it('rejects empty files (zero bytes)', () => { + const fileList = [makeFile('empty.pdf', 'application/pdf', 0)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(false); + expect(setError).toHaveBeenCalledWith('com_error_files_empty'); + }); + + it('rejects when fileLimit would be exceeded', () => { + endpointFileConfig = makeEndpointConfig({ fileLimit: 3 }); + files = new Map([ + ['f1', makeExtendedFile({ file_id: 'f1', filename: 'one.pdf', size: 2048 })], + ['f2', makeExtendedFile({ file_id: 'f2', filename: 'two.pdf', size: 3072 })], + ]); + const fileList = [ + makeFile('a.pdf', 'application/pdf', 1024), + makeFile('b.pdf', 'application/pdf', 2048), + ]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(false); + expect(setError).toHaveBeenCalledWith('File limit reached: 3 files'); + }); + + it('allows upload when exactly at fileLimit boundary', () => { + endpointFileConfig = makeEndpointConfig({ fileLimit: 3 }); + files = new Map([ + ['f1', makeExtendedFile({ file_id: 'f1', filename: 'one.pdf', size: 2048 })], + ['f2', makeExtendedFile({ file_id: 'f2', filename: 'two.pdf', size: 3072 })], + ]); + const fileList = [makeFile('a.pdf', 'application/pdf', 1024)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(true); + }); + + it('rejects unsupported MIME type', () => { + const fileList = [makeFile('data.xyz', 'application/x-unknown', 1024)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(false); + expect(setError).toHaveBeenCalledWith('Unsupported file type: application/x-unknown'); + }); + + it('rejects when file size equals fileSizeLimit (>= comparison)', () => { + const limit = 5 * megabyte; + endpointFileConfig = makeEndpointConfig({ fileSizeLimit: limit }); + const fileList = [makeFile('exact.pdf', 'application/pdf', limit)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(false); + expect(setError).toHaveBeenCalledWith(`File size limit exceeded: ${limit / megabyte} MB`); + }); + + it('allows file just under fileSizeLimit', () => { + const limit = 5 * megabyte; + endpointFileConfig = makeEndpointConfig({ fileSizeLimit: limit }); + const fileList = [makeFile('under.pdf', 'application/pdf', limit - 1)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(true); + }); + + it('rejects when totalSizeLimit would be exceeded', () => { + const limit = 10 * megabyte; + endpointFileConfig = makeEndpointConfig({ totalSizeLimit: limit }); + files = new Map([['f1', makeExtendedFile({ file_id: 'f1', size: 6 * megabyte })]]); + const fileList = [makeFile('big.pdf', 'application/pdf', 5 * megabyte)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(false); + expect(setError).toHaveBeenCalledWith(`Total file size limit exceeded: ${limit / megabyte} MB`); + }); + + it('allows when totalSizeLimit is exactly met', () => { + const limit = 10 * megabyte; + endpointFileConfig = makeEndpointConfig({ totalSizeLimit: limit }); + files = new Map([['f1', makeExtendedFile({ file_id: 'f1', size: 5 * megabyte })]]); + const fileList = [makeFile('fits.pdf', 'application/pdf', 5 * megabyte)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(true); + }); + + it('rejects duplicate files', () => { + files = new Map([ + [ + 'f1', + makeExtendedFile({ + file_id: 'f1', + file: makeFile('doc.pdf', 'application/pdf', 1024), + filename: 'doc.pdf', + size: 1024, + type: 'application/pdf', + }), + ], + ]); + const fileList = [makeFile('doc.pdf', 'application/pdf', 1024)]; + const result = validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(result).toBe(false); + expect(setError).toHaveBeenCalledWith('com_error_files_dupe'); + }); + + it('enforces check ordering: disabled before fileLimit', () => { + endpointFileConfig = makeEndpointConfig({ disabled: true, fileLimit: 1 }); + files = new Map([['f1', makeExtendedFile({ file_id: 'f1', filename: 'existing.pdf' })]]); + const fileList = [makeFile('doc.pdf', 'application/pdf', 1024)]; + validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(setError).toHaveBeenCalledWith('com_ui_attach_error_disabled'); + }); + + it('enforces check ordering: fileLimit before fileSizeLimit', () => { + const limit = 1; + endpointFileConfig = makeEndpointConfig({ fileLimit: 1, fileSizeLimit: limit }); + files = new Map([['f1', makeExtendedFile({ file_id: 'f1', filename: 'existing.pdf' })]]); + const fileList = [makeFile('huge.pdf', 'application/pdf', limit)]; + validateFiles({ files, fileList, setError, endpointFileConfig, fileConfig }); + expect(setError).toHaveBeenCalledWith('File limit reached: 1 files'); + }); +}); diff --git a/client/src/utils/files.ts b/client/src/utils/files.ts index b4d362d456..be81a31b79 100644 --- a/client/src/utils/files.ts +++ b/client/src/utils/files.ts @@ -251,7 +251,7 @@ export const validateFiles = ({ const currentTotalSize = existingFiles.reduce((total, file) => total + file.size, 0); if (fileLimit && fileList.length + files.size > fileLimit) { - setError(`You can only upload up to ${fileLimit} files at a time.`); + setError(`File limit reached: ${fileLimit} files`); return false; } @@ -282,19 +282,18 @@ export const validateFiles = ({ } if (!checkType(originalFile.type, mimeTypesToCheck)) { - console.log(originalFile); - setError('Currently, unsupported file type: ' + originalFile.type); + setError(`Unsupported file type: ${originalFile.type}`); return false; } if (fileSizeLimit && originalFile.size >= fileSizeLimit) { - setError(`File size exceeds ${fileSizeLimit / megabyte} MB.`); + setError(`File size limit exceeded: ${fileSizeLimit / megabyte} MB`); return false; } } if (totalSizeLimit && currentTotalSize + incomingTotalSize > totalSizeLimit) { - setError(`The total size of the files cannot exceed ${totalSizeLimit / megabyte} MB.`); + setError(`Total file size limit exceeded: ${totalSizeLimit / megabyte} MB`); return false; }