From c385f2ba888c1bd4a6fc0430d33f10d30ab8b464 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 11 May 2026 16:24:04 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=A6=20feat:=20Configure=20Skill=20Impo?= =?UTF-8?q?rt=20Size=20Limit=20(#13073)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: configure skill import size limit * fix: validate skill import size in ui * fix: align skill import size boundary * fix: show exact skill import limit --- api/server/routes/skills.js | 22 ++- api/server/routes/skills.test.js | 45 +++++ .../Skills/dialogs/UploadSkillDialog.tsx | 35 +++- .../__tests__/UploadSkillDialog.spec.tsx | 165 ++++++++++++++++++ client/src/locales/en/translation.json | 2 + .../api/src/skills/__tests__/import.test.ts | 18 ++ packages/api/src/skills/import.ts | 11 +- .../data-provider/src/file-config.spec.ts | 16 ++ packages/data-provider/src/file-config.ts | 19 ++ packages/data-provider/src/types/files.ts | 6 + 10 files changed, 329 insertions(+), 10 deletions(-) create mode 100644 client/src/components/Skills/dialogs/__tests__/UploadSkillDialog.spec.tsx diff --git a/api/server/routes/skills.js b/api/server/routes/skills.js index 4052fceb37..694009a480 100644 --- a/api/server/routes/skills.js +++ b/api/server/routes/skills.js @@ -16,6 +16,7 @@ const { PermissionTypes, Permissions, FileContext, + mergeFileConfig, } = require('librechat-data-provider'); const { createSkill, @@ -52,6 +53,11 @@ const MAX_IMPORT_SIZE = 50 * 1024 * 1024; // 50 MB const memoryStorage = multer.memoryStorage(); +function getSkillImportSizeLimit(req) { + const fileConfig = mergeFileConfig(req.config?.fileConfig); + return fileConfig.skills?.fileSizeLimit ?? MAX_IMPORT_SIZE; +} + const skillImportFilter = (_req, file, cb) => { const ext = path.extname(file.originalname).toLowerCase(); if (ALLOWED_EXTENSIONS.has(ext)) { @@ -62,11 +68,12 @@ const skillImportFilter = (_req, file, cb) => { } }; -const skillUpload = multer({ - storage: memoryStorage, - fileFilter: skillImportFilter, - limits: { fileSize: MAX_IMPORT_SIZE }, -}); +const skillUpload = (req, res, next) => + multer({ + storage: memoryStorage, + fileFilter: skillImportFilter, + limits: { fileSize: getSkillImportSizeLimit(req) }, + }).single('file')(req, res, next); // Per-file upload (for adding individual files to an existing skill) const MAX_SINGLE_FILE_SIZE = 10 * 1024 * 1024; // 10 MB @@ -135,6 +142,9 @@ function resolveSkillStorage(req, { isImage = false } = {}) { // Import handler (zip/md/skill → create skill + files) // --------------------------------------------------------------------------- const importHandler = createImportHandler({ + limits: (req) => ({ + maxZipBytes: getSkillImportSizeLimit(req), + }), createSkill, getSkillById, deleteSkill, @@ -269,7 +279,7 @@ router.post( checkSkillCreate, fileUploadIpLimiter, fileUploadUserLimiter, - skillUpload.single('file'), + skillUpload, restoreTenantContextFromReq, importHandler, ); diff --git a/api/server/routes/skills.test.js b/api/server/routes/skills.test.js index 2b7166a22d..af99e4bc5e 100644 --- a/api/server/routes/skills.test.js +++ b/api/server/routes/skills.test.js @@ -3,6 +3,27 @@ const request = require('supertest'); const JSZip = require('jszip'); const mongoose = require('mongoose'); const { MongoMemoryServer } = require('mongodb-memory-server'); + +jest.mock('librechat-data-provider', () => { + const actual = jest.requireActual('librechat-data-provider'); + return { + ...actual, + mergeFileConfig: jest.fn((dynamic) => { + const skillFileSizeLimit = dynamic?.skills?.fileSizeLimit; + return { + ...actual.fileConfig, + ...dynamic, + skills: { + ...(actual.fileConfig.skills ?? { fileSizeLimit: 50 * 1024 * 1024 }), + ...(skillFileSizeLimit !== undefined + ? { fileSizeLimit: skillFileSizeLimit * 1024 * 1024 } + : {}), + }, + }; + }), + }; +}); + const { SystemRoles, ResourceType, @@ -11,6 +32,8 @@ const { PermissionBits, } = require('librechat-data-provider'); +let mockFileConfig; + jest.mock('~/server/services/Config', () => ({ getCachedTools: jest.fn().mockResolvedValue({}), getAppConfig: jest.fn().mockResolvedValue({ @@ -23,6 +46,7 @@ jest.mock('~/server/middleware/config/app', () => (req, _res, next) => { req.config = { fileStrategy: 'local', paths: { uploads: '/tmp/uploads', images: '/tmp/images' }, + fileConfig: mockFileConfig, }; next(); }); @@ -127,6 +151,7 @@ afterEach(async () => { await SkillFile.deleteMany({}); await AclEntry.deleteMany({}); currentTestUser = testUsers.owner; + mockFileConfig = undefined; }); afterAll(async () => { @@ -300,6 +325,26 @@ describe('Skill routes', () => { }); describe('POST /api/skills/import', () => { + it('enforces fileConfig.skills.fileSizeLimit before import handling', async () => { + mockFileConfig = { + skills: { + fileSizeLimit: 1, + }, + }; + + const res = await request(app) + .post('/api/skills/import') + .attach('file', Buffer.alloc(2 * 1024 * 1024), { + filename: 'too-large.skill', + contentType: 'application/zip', + }); + + const { mergeFileConfig } = require('librechat-data-provider'); + expect(mergeFileConfig).toHaveBeenCalledWith(mockFileConfig); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/file too large/i); + }); + it('persists storage metadata for imported skill files', async () => { const savedFilepath = 'https://cdn.example.com/r/us-east-2/uploads/user123/imported-script.sh'; diff --git a/client/src/components/Skills/dialogs/UploadSkillDialog.tsx b/client/src/components/Skills/dialogs/UploadSkillDialog.tsx index d1d917ae2f..3cd1fd5408 100644 --- a/client/src/components/Skills/dialogs/UploadSkillDialog.tsx +++ b/client/src/components/Skills/dialogs/UploadSkillDialog.tsx @@ -2,7 +2,12 @@ import { useRef, useCallback, useState } from 'react'; import { Upload } from 'lucide-react'; import { useNavigate } from 'react-router-dom'; import { OGDialog, OGDialogContent, Spinner, useToastContext } from '@librechat/client'; -import { useImportSkillMutation } from '~/data-provider'; +import { + megabyte, + mergeFileConfig, + fileConfig as defaultFileConfig, +} from 'librechat-data-provider'; +import { useGetFileConfig, useImportSkillMutation } from '~/data-provider'; import { useLocalize } from '~/hooks'; import { cn } from '~/utils'; @@ -11,12 +16,30 @@ interface UploadSkillDialogProps { setIsOpen: (open: boolean) => void; } +function formatMegabytes(bytes: number): string { + const value = bytes / megabyte; + return Number.isInteger(value) ? `${value}` : value.toFixed(1); +} + export default function UploadSkillDialog({ isOpen, setIsOpen }: UploadSkillDialogProps) { const localize = useLocalize(); const navigate = useNavigate(); const { showToast } = useToastContext(); const fileInputRef = useRef(null); const [isDragging, setIsDragging] = useState(false); + const { data: skillFileConfig = { fileConfig: defaultFileConfig } } = useGetFileConfig({ + select: (data) => ({ + configuredSizeLimitMb: data?.skills?.fileSizeLimit, + fileConfig: mergeFileConfig(data), + }), + }); + const { configuredSizeLimitMb, fileConfig } = skillFileConfig; + const skillImportSizeLimit = + fileConfig.skills?.fileSizeLimit ?? defaultFileConfig.skills?.fileSizeLimit ?? 0; + const displayedSizeLimit = + configuredSizeLimitMb !== undefined + ? `${configuredSizeLimitMb}` + : formatMegabytes(skillImportSizeLimit); const importMutation = useImportSkillMutation({ onSuccess: (skill) => { @@ -38,11 +61,18 @@ export default function UploadSkillDialog({ isOpen, setIsOpen }: UploadSkillDial if (importMutation.isLoading) { return; } + if (file.size > skillImportSizeLimit) { + showToast({ + status: 'error', + message: localize('com_ui_skill_upload_size_error', { 0: displayedSizeLimit }), + }); + return; + } const formData = new FormData(); formData.append('file', file, file.name); importMutation.mutate(formData); }, - [importMutation], + [displayedSizeLimit, importMutation, localize, showToast, skillImportSizeLimit], ); const handleFileInput = useCallback( @@ -109,6 +139,7 @@ export default function UploadSkillDialog({ isOpen, setIsOpen }: UploadSkillDial diff --git a/client/src/components/Skills/dialogs/__tests__/UploadSkillDialog.spec.tsx b/client/src/components/Skills/dialogs/__tests__/UploadSkillDialog.spec.tsx new file mode 100644 index 0000000000..803124adbc --- /dev/null +++ b/client/src/components/Skills/dialogs/__tests__/UploadSkillDialog.spec.tsx @@ -0,0 +1,165 @@ +import { fireEvent, render, screen } from '@testing-library/react'; +import type { ReactNode } from 'react'; +import type { FileConfigInput } from 'librechat-data-provider'; +import UploadSkillDialog from '../UploadSkillDialog'; + +const mockMutate = jest.fn(); +const mockNavigate = jest.fn(); +const mockSetIsOpen = jest.fn(); +const mockShowToast = jest.fn(); +let mockFileConfigInput: FileConfigInput | undefined = { + skills: { + fileSizeLimit: 1, + }, +}; + +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useNavigate: () => mockNavigate, +})); + +jest.mock( + '@librechat/client', + () => { + const React = jest.requireActual('react'); + return { + OGDialog: ({ open, children }: { open: boolean; children: ReactNode }) => + open ? React.createElement('div', null, children) : null, + OGDialogContent: ({ children }: { children: ReactNode }) => + React.createElement('div', null, children), + Spinner: () => React.createElement('div', { 'data-testid': 'spinner' }), + useToastContext: () => ({ + showToast: mockShowToast, + }), + }; + }, + { virtual: true }, +); + +jest.mock('~/data-provider', () => ({ + useGetFileConfig: ({ select }: { select?: (data: FileConfigInput | undefined) => unknown }) => ({ + data: select != null ? select(mockFileConfigInput) : mockFileConfigInput, + }), + useImportSkillMutation: () => ({ + mutate: mockMutate, + isLoading: false, + }), +})); + +jest.mock('~/hooks', () => ({ + useLocalize: + () => + (key: string, params?: Record): string => { + const translations: Record = { + com_ui_skill_upload_title: 'Upload skill', + com_ui_skill_upload_drag: 'Drag and drop or click to upload', + com_ui_skill_upload_requirements: 'File requirements', + com_ui_skill_upload_req_md: + '.md file must contain skill name and description formatted in YAML', + com_ui_skill_upload_req_zip: '.zip or .skill file must include a SKILL.md file', + com_ui_skill_upload_req_size: `File size must not exceed ${params?.[0]} MB`, + com_ui_skill_upload_size_error: `Skill import must not exceed ${params?.[0]} MB`, + com_ui_skill_created: 'Skill created', + com_ui_create_skill_upload_error: 'Failed to read the uploaded file', + }; + return translations[key] ?? key; + }, +})); + +jest.mock('~/utils', () => ({ + cn: (...classes: Array) => classes.filter(Boolean).join(' '), +})); + +function getFileInput(container: HTMLElement): HTMLInputElement { + const input = container.querySelector('input[type="file"]'); + if (!(input instanceof HTMLInputElement)) { + throw new Error('Upload input was not rendered'); + } + return input; +} + +describe('UploadSkillDialog', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockFileConfigInput = { + skills: { + fileSizeLimit: 1, + }, + }; + }); + + it('renders the configured skill import size limit', () => { + render(); + + expect(screen.getByText('File size must not exceed 1 MB')).toBeInTheDocument(); + }); + + it('renders fractional configured skill import size limits exactly', () => { + mockFileConfigInput = { + skills: { + fileSizeLimit: 1.06, + }, + }; + + render(); + + expect(screen.getByText('File size must not exceed 1.06 MB')).toBeInTheDocument(); + }); + + it('rejects files above the configured skill import limit before upload', () => { + const { container } = render(); + const file = new File([new Uint8Array(1024 * 1024 + 1)], 'too-large.skill', { + type: 'application/zip', + }); + + fireEvent.change(getFileInput(container), { + target: { + files: [file], + }, + }); + + expect(mockMutate).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith({ + status: 'error', + message: 'Skill import must not exceed 1 MB', + }); + }); + + it('uploads files exactly at the configured skill import limit', () => { + const appendSpy = jest.spyOn(FormData.prototype, 'append'); + const { container } = render(); + const file = new File([new Uint8Array(1024 * 1024)], 'exact-limit.skill', { + type: 'application/zip', + }); + + fireEvent.change(getFileInput(container), { + target: { + files: [file], + }, + }); + + expect(mockShowToast).not.toHaveBeenCalled(); + expect(appendSpy).toHaveBeenCalledWith('file', file, file.name); + expect(mockMutate).toHaveBeenCalledWith(expect.any(FormData)); + appendSpy.mockRestore(); + }); + + it('uploads files under the configured skill import limit', () => { + const appendSpy = jest.spyOn(FormData.prototype, 'append'); + const { container } = render(); + const file = new File([new Uint8Array(1024)], 'small.skill', { + type: 'application/zip', + }); + + fireEvent.change(getFileInput(container), { + target: { + files: [file], + }, + }); + + expect(mockShowToast).not.toHaveBeenCalled(); + expect(appendSpy).toHaveBeenCalledWith('file', file, file.name); + expect(mockMutate).toHaveBeenCalledWith(expect.any(FormData)); + appendSpy.mockRestore(); + }); +}); diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index a6b77c40ac..8067919b00 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -1524,8 +1524,10 @@ "com_ui_skill_upload_drag": "Drag and drop or click to upload", "com_ui_skill_upload_file": "Upload File", "com_ui_skill_upload_req_md": ".md file must contain skill name and description formatted in YAML", + "com_ui_skill_upload_req_size": "File size must not exceed {{0}} MB", "com_ui_skill_upload_req_zip": ".zip or .skill file must include a SKILL.md file", "com_ui_skill_upload_requirements": "File requirements", + "com_ui_skill_upload_size_error": "Skill import must not exceed {{0}} MB", "com_ui_skill_upload_title": "Upload skill", "com_ui_skill_version": "v{{0}}", "com_ui_skill_view_rendered": "View rendered", diff --git a/packages/api/src/skills/__tests__/import.test.ts b/packages/api/src/skills/__tests__/import.test.ts index 39c3e6405a..626edbdb80 100644 --- a/packages/api/src/skills/__tests__/import.test.ts +++ b/packages/api/src/skills/__tests__/import.test.ts @@ -217,6 +217,24 @@ describe('parseFrontmatter', () => { }); describe('createImportHandler', () => { + it('uses request-scoped import limits', async () => { + const buffer = await zipWithAdditionalFiles(0, 0); + const deps = mockImportDeps(() => ({ + maxZipBytes: buffer.length - 1, + })); + const handler = createImportHandler(deps); + const res = mockResponse(); + + await handler(mockZipRequest(buffer), res); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.body).toEqual( + expect.objectContaining({ + error: expect.stringContaining('File too large'), + }), + ); + }); + it('counts rejected oversized zip entries toward the cumulative decompressed limit', async () => { const kib = 1024; const deps = mockImportDeps({ diff --git a/packages/api/src/skills/import.ts b/packages/api/src/skills/import.ts index 817830cbe5..013a021933 100644 --- a/packages/api/src/skills/import.ts +++ b/packages/api/src/skills/import.ts @@ -164,7 +164,7 @@ function isDuplicateKeyError(error: unknown): boolean { } export interface ImportSkillDeps { - limits?: Partial; + limits?: Partial | ((req: ServerRequest) => Partial | undefined); createSkill: (data: CreateSkillInput) => Promise; getSkillById: (id: string | Types.ObjectId) => Promise<(ISkill & { _id: Types.ObjectId }) | null>; deleteSkill: (id: string) => Promise<{ deleted: boolean }>; @@ -257,6 +257,13 @@ function getImportLimits(limits?: Partial): ImportLimits { }; } +function resolveImportLimits( + limits: ImportSkillDeps['limits'], + req: ServerRequest, +): Partial | undefined { + return typeof limits === 'function' ? limits(req) : limits; +} + /** Resolve author metadata from the request user. */ function getAuthorInfo(req: ServerRequest) { const user = req.user; @@ -353,7 +360,7 @@ async function handleZip( file: Express.Multer.File, ) { const userId = req.user.id; - const limits = getImportLimits(deps.limits); + const limits = getImportLimits(resolveImportLimits(deps.limits, req)); const zipBuffer = file.buffer; diff --git a/packages/data-provider/src/file-config.spec.ts b/packages/data-provider/src/file-config.spec.ts index ec846af5a4..f597353876 100644 --- a/packages/data-provider/src/file-config.spec.ts +++ b/packages/data-provider/src/file-config.spec.ts @@ -836,6 +836,22 @@ describe('getEndpointFileConfig', () => { expect(result.fileSizeLimit).toBe(10 * 1024 * 1024); }); + it('should convert skills fileSizeLimit from MB to bytes', () => { + const merged = mergeFileConfig({ + skills: { + fileSizeLimit: 15, + }, + }); + + expect(merged.skills?.fileSizeLimit).toBe(15 * 1024 * 1024); + }); + + it('should default skills fileSizeLimit to 50 MB', () => { + const merged = mergeFileConfig(undefined); + + expect(merged.skills?.fileSizeLimit).toBe(50 * 1024 * 1024); + }); + it('should preserve disabled: false in merged config', () => { const dynamicConfig = { endpoints: { diff --git a/packages/data-provider/src/file-config.ts b/packages/data-provider/src/file-config.ts index 9344884789..457c4950cc 100644 --- a/packages/data-provider/src/file-config.ts +++ b/packages/data-provider/src/file-config.ts @@ -397,6 +397,7 @@ export const megabyte = 1024 * 1024; export const mbToBytes = (mb: number): number => mb * megabyte; const defaultSizeLimit = mbToBytes(512); +const defaultSkillImportSizeLimit = mbToBytes(50); const defaultTokenLimit = 100000; const assistantsFileConfig = { fileLimit: 10, @@ -426,6 +427,9 @@ export const fileConfig = { disabled: false, }, }, + skills: { + fileSizeLimit: defaultSkillImportSizeLimit, + }, serverFileSizeLimit: defaultSizeLimit, avatarSizeLimit: mbToBytes(2), fileTokenLimit: defaultTokenLimit, @@ -459,8 +463,13 @@ export const endpointFileConfigSchema = z.object({ supportedMimeTypes: supportedMimeTypesSchema.optional(), }); +const skillFileConfigSchema = z.object({ + fileSizeLimit: z.number().min(0).optional(), +}); + export const fileConfigSchema = z.object({ endpoints: z.record(endpointFileConfigSchema).optional(), + skills: skillFileConfigSchema.optional(), serverFileSizeLimit: z.number().min(0).optional(), avatarSizeLimit: z.number().min(0).optional(), fileTokenLimit: z.number().min(0).optional(), @@ -652,6 +661,9 @@ export function mergeFileConfig(dynamic: z.infer | unde endpoints: { ...fileConfig.endpoints, }, + skills: { + ...fileConfig.skills, + }, ocr: { ...fileConfig.ocr, supportedMimeTypes: fileConfig.ocr?.supportedMimeTypes || [], @@ -681,6 +693,13 @@ export function mergeFileConfig(dynamic: z.infer | unde mergedConfig.fileTokenLimit = dynamic.fileTokenLimit; } + if (dynamic.skills?.fileSizeLimit !== undefined) { + mergedConfig.skills = { + ...mergedConfig.skills, + fileSizeLimit: mbToBytes(dynamic.skills.fileSizeLimit), + }; + } + // Merge clientImageResize configuration if (dynamic.clientImageResize !== undefined) { mergedConfig.clientImageResize = { diff --git a/packages/data-provider/src/types/files.ts b/packages/data-provider/src/types/files.ts index 1d6f17c371..d977b1eb99 100644 --- a/packages/data-provider/src/types/files.ts +++ b/packages/data-provider/src/types/files.ts @@ -51,6 +51,9 @@ export type FileConfig = { endpoints: { [key: string]: EndpointFileConfig; }; + skills?: { + fileSizeLimit?: number; + }; fileTokenLimit?: number; serverFileSizeLimit?: number; avatarSizeLimit?: number; @@ -76,6 +79,9 @@ export type FileConfigInput = { endpoints?: { [key: string]: EndpointFileConfig; }; + skills?: { + fileSizeLimit?: number; + }; serverFileSizeLimit?: number; avatarSizeLimit?: number; clientImageResize?: {