From e4077832ef7998210e290593560732526723adf6 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 10 Sep 2026 23:41:39 +0200 Subject: [PATCH] fix(backend): set multer's fieldArrayIndexLimit to actually close CVE-2026-82333 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The advisory is explicit that the 2.3.0 version bump alone doesn't remediate the array-index DoS — an app must also set limits.fieldArrayIndexLimit. Set it on every multer instance, sized to what each route's form actually needs. --- .../__tests__/routes/publicContracts.test.js | 49 +++++++++++++++++++ backend/src/config/multerConfig.js | 18 +++++-- backend/src/middleware/errorHandler.js | 10 ++++ backend/src/routes/adminBackup.js | 6 ++- backend/src/routes/adminBusinessProfile.js | 4 +- backend/src/routes/adminCMS.js | 4 +- backend/src/routes/adminContracts.js | 4 +- backend/src/routes/adminEvents/logo.js | 4 +- backend/src/routes/adminExpenses.js | 5 +- backend/src/routes/adminInvoices.js | 4 +- backend/src/routes/adminPhotos.js | 7 ++- backend/src/routes/adminSettings.js | 9 +++- backend/src/routes/gallery.js | 7 ++- backend/src/routes/publicContracts.js | 5 +- backend/src/routes/v1/events.js | 4 +- 15 files changed, 123 insertions(+), 17 deletions(-) diff --git a/backend/__tests__/routes/publicContracts.test.js b/backend/__tests__/routes/publicContracts.test.js index c258f296..47d3ad7f 100644 --- a/backend/__tests__/routes/publicContracts.test.js +++ b/backend/__tests__/routes/publicContracts.test.js @@ -25,14 +25,18 @@ process.env.STORAGE_PATH = path.join(tmpDir, 'storage'); fs.mkdirSync(process.env.STORAGE_PATH, { recursive: true }); process.env.JWT_SECRET = process.env.JWT_SECRET || 'crm-route-test-secret'; +const express = require('express'); +const cookieParser = require('cookie-parser'); const request = require('supertest'); const { bootCrmDb, seedMinimal, createPublicToken, buildRouteApp } = require('../integration/helpers/crmDb'); const tokenGuards = require('../../src/utils/publicTokenGuards'); +const { errorHandler } = require('../../src/middleware/errorHandler'); describe('publicContracts routes', () => { let db; let cleanup; let app; + let appWithErrorHandler; let customerId; let contractId; @@ -51,6 +55,17 @@ describe('publicContracts routes', () => { contractId = inserted[0]?.id ?? inserted[0]; app = buildRouteApp('/api/public/contracts', require('../../src/routes/publicContracts')); + + // A second app instance wired to the REAL production error handler + // (buildRouteApp's is a simplified stand-in that only reads + // err.statusCode/err.status, which a bare MulterError doesn't set). + // Used below to verify the actual 4xx contract end-to-end, not just + // that multer aborted the request. + appWithErrorHandler = express(); + appWithErrorHandler.use(express.json()); + appWithErrorHandler.use(cookieParser()); + appWithErrorHandler.use('/api/public/contracts', require('../../src/routes/publicContracts')); + appWithErrorHandler.use(errorHandler); }, 120000); afterAll(async () => { @@ -131,6 +146,40 @@ describe('publicContracts routes', () => { .attach('file', Buffer.from('%PDF-1.4 fake'), 'signed.pdf'); expect(res.status).toBe(404); }); + + // CVE-2026-82333 regression (#1374 follow-up): multer 2.3.0 added an + // opt-in `fieldArrayIndexLimit` that must be set to actually close the + // field-parser DoS — the version bump alone does nothing. This route is + // unauthenticated (token-in-URL only), so it's the sharpest place to + // prove a crafted request with an oversized array-index field name + // (`evil[999999999]`) is rejected rather than accepted or left to hang. + it('rejects a multipart request with an oversized array-index field name', async () => { + const token = await createPublicToken(db, 'contract_action_tokens', { + contract_id: contractId, + }); + const res = await request(app) + .post(`/api/public/contracts/${token}/upload-signed-pdf`) + .field('evil[999999999]', 'x') + .attach('file', Buffer.from('%PDF-1.4 fake'), 'signed.pdf'); + // multer aborts the request before the handler runs; buildRouteApp's + // generic error handler falls back to 500 for a bare MulterError + // (see appWithErrorHandler test below for the real 4xx contract), so + // here we only assert the upload was NOT accepted/processed. + expect(res.status).toBeGreaterThanOrEqual(400); + expect(res.body.error).not.toBe(undefined); + }); + + it('maps the oversized array-index rejection to a 400 through the real error handler', async () => { + const token = await createPublicToken(db, 'contract_action_tokens', { + contract_id: contractId, + }); + const res = await request(appWithErrorHandler) + .post(`/api/public/contracts/${token}/upload-signed-pdf`) + .field('evil[999999999]', 'x') + .attach('file', Buffer.from('%PDF-1.4 fake'), 'signed.pdf'); + expect(res.status).toBe(400); + expect(res.body.code).toBe('VALIDATION_ERROR'); + }); }); describe('GET /:token/pdf', () => { diff --git a/backend/src/config/multerConfig.js b/backend/src/config/multerConfig.js index 48f2aa69..85b31e69 100644 --- a/backend/src/config/multerConfig.js +++ b/backend/src/config/multerConfig.js @@ -120,7 +120,14 @@ const createPhotoUploader = (options = {}) => { files: options.maxFiles || 2000, fieldSize: 10 * 1024 * 1024, parts: 10000, - headerPairs: 2000 + headerPairs: 2000, + // CVE-2026-82333: no preset in this factory is currently wired up to + // a route (nothing imports createPhotoUploader et al. — routes build + // their own multer instances directly), but every preset gets the + // limit anyway so it can't be adopted later without it. None of the + // uploaders this factory builds have a legitimate use for + // array-indexed field names. + fieldArrayIndexLimit: 0 }, fileFilter: createFileFilter(ALLOWED_TYPES.media, { validateMagicNumbers: true @@ -146,7 +153,8 @@ const createLogoUploader = (options = {}) => { } }), limits: { - fileSize: options.maxSize || SIZE_LIMITS.medium + fileSize: options.maxSize || SIZE_LIMITS.medium, + fieldArrayIndexLimit: 0 // CVE-2026-82333 — see createPhotoUploader comment }, fileFilter: createFileFilter(ALLOWED_TYPES.logos, { skipMagicValidation: ['image/svg+xml'] @@ -172,7 +180,8 @@ const createFaviconUploader = (options = {}) => { } }), limits: { - fileSize: options.maxSize || SIZE_LIMITS.small + fileSize: options.maxSize || SIZE_LIMITS.small, + fieldArrayIndexLimit: 0 // CVE-2026-82333 — see createPhotoUploader comment }, fileFilter: createFileFilter(ALLOWED_TYPES.favicons, { skipMagicValidation: ['image/x-icon', 'image/vnd.microsoft.icon'] @@ -194,7 +203,8 @@ const createGalleryUploader = (destDir, options = {}) => { dest: destDir, limits: { fileSize: options.maxSize || SIZE_LIMITS.large, - files: options.maxFiles || 10 + files: options.maxFiles || 10, + fieldArrayIndexLimit: 0 // CVE-2026-82333 — see createPhotoUploader comment }, fileFilter: createFileFilter(ALLOWED_TYPES.photos) }; diff --git a/backend/src/middleware/errorHandler.js b/backend/src/middleware/errorHandler.js index 782a2370..60cefd85 100644 --- a/backend/src/middleware/errorHandler.js +++ b/backend/src/middleware/errorHandler.js @@ -92,6 +92,16 @@ const handleKnownErrors = (err) => { return new ValidationError('Unexpected file field'); } + // CVE-2026-82333: multer 2.3.0's fieldArrayIndexLimit rejects multipart + // field names with an oversized bracket array index (e.g. `a[99999999]`) + // before the DoS-prone field parser runs. Without this mapping the + // resulting MulterError has no .statusCode/.status and falls through to + // a 500 here, so map it to a proper 400 like the other multer limits. + if (err.code === 'LIMIT_FIELD_ARRAY_INDEX') { + const { ValidationError } = require('../utils/errors'); + return new ValidationError('Field name array index too large'); + } + return err; }; diff --git a/backend/src/routes/adminBackup.js b/backend/src/routes/adminBackup.js index 619b1e4d..1e2846b6 100644 --- a/backend/src/routes/adminBackup.js +++ b/backend/src/routes/adminBackup.js @@ -191,7 +191,11 @@ const picpeakUpload = multer({ destination: (req, file, cb) => cb(null, os.tmpdir()), filename: (req, file, cb) => cb(null, `picpeak-upload-${Date.now()}-${crypto.randomBytes(6).toString('hex')}.picpeak`), }), - limits: { fileSize: 5 * 1024 * 1024 * 1024 }, // 5 GB — .picpeak with photos can be large + // CVE-2026-82333: this route only ever consumes a single unnamed file + // field (`backup`) — no legitimate bracket-indexed field name (e.g. + // `a[0]`) exists in its form. fieldArrayIndexLimit: 0 rejects any field + // name using array-index syntax at all, closing multer's field-parser DoS. + limits: { fileSize: 5 * 1024 * 1024 * 1024, fieldArrayIndexLimit: 0 }, // 5 GB — .picpeak with photos can be large }); // Upload + restore a .picpeak onto THIS instance. DESTRUCTIVE: full override of diff --git a/backend/src/routes/adminBusinessProfile.js b/backend/src/routes/adminBusinessProfile.js index 2c5305ba..889f35d9 100644 --- a/backend/src/routes/adminBusinessProfile.js +++ b/backend/src/routes/adminBusinessProfile.js @@ -109,7 +109,9 @@ const pdfLogoStorage = multer.diskStorage({ const pdfLogoUpload = multer({ storage: pdfLogoStorage, - limits: { fileSize: 5 * 1024 * 1024 }, + // CVE-2026-82333: single unnamed `logo` field only — no legitimate + // array-indexed field names, so reject any bracket-index field name. + limits: { fileSize: 5 * 1024 * 1024, fieldArrayIndexLimit: 0 }, fileFilter: (_req, file, cb) => { const allowed = ['image/png', 'image/jpeg', 'image/svg+xml']; if (allowed.includes(file.mimetype)) cb(null, true); diff --git a/backend/src/routes/adminCMS.js b/backend/src/routes/adminCMS.js index 0df5902b..b08d0e64 100644 --- a/backend/src/routes/adminCMS.js +++ b/backend/src/routes/adminCMS.js @@ -31,7 +31,9 @@ const pageLogoStorage = multer.diskStorage({ const pageLogoUpload = multer({ storage: pageLogoStorage, - limits: { fileSize: 5 * 1024 * 1024 }, + // CVE-2026-82333: single unnamed `logo` field only — no legitimate + // array-indexed field names, so reject any bracket-index field name. + limits: { fileSize: 5 * 1024 * 1024, fieldArrayIndexLimit: 0 }, fileFilter: (_req, file, cb) => { const allowed = ['image/jpeg', 'image/png', 'image/gif', 'image/svg+xml']; if (validateFileType(file.originalname, file.mimetype, allowed)) cb(null, true); diff --git a/backend/src/routes/adminContracts.js b/backend/src/routes/adminContracts.js index d6c3564e..219d602d 100644 --- a/backend/src/routes/adminContracts.js +++ b/backend/src/routes/adminContracts.js @@ -72,7 +72,9 @@ const signedPdfStorage = multer.diskStorage({ const signedPdfUpload = multer({ storage: signedPdfStorage, - limits: { fileSize: 10 * 1024 * 1024 }, // 10 MB + // CVE-2026-82333: single unnamed `file` field only — no legitimate + // array-indexed field names, so reject any bracket-index field name. + limits: { fileSize: 10 * 1024 * 1024, fieldArrayIndexLimit: 0 }, // 10 MB fileFilter: (req, file, cb) => { const allowed = ['application/pdf']; if (validateFileType(file.originalname, file.mimetype, allowed)) return cb(null, true); diff --git a/backend/src/routes/adminEvents/logo.js b/backend/src/routes/adminEvents/logo.js index 2e33fee6..75cb864c 100644 --- a/backend/src/routes/adminEvents/logo.js +++ b/backend/src/routes/adminEvents/logo.js @@ -30,7 +30,9 @@ const eventLogoStorage = multer.diskStorage({ const eventLogoUpload = multer({ storage: eventLogoStorage, - limits: { fileSize: 5 * 1024 * 1024 }, // 5MB + // CVE-2026-82333: single unnamed `logo` field only — no legitimate + // array-indexed field names, so reject any bracket-index field name. + limits: { fileSize: 5 * 1024 * 1024, fieldArrayIndexLimit: 0 }, // 5MB fileFilter: (req, file, cb) => { const allowedMimeTypes = ['image/jpeg', 'image/png', 'image/gif', 'image/svg+xml']; if (validateFileType(file.originalname, file.mimetype, allowedMimeTypes)) { diff --git a/backend/src/routes/adminExpenses.js b/backend/src/routes/adminExpenses.js index 80046707..876f8dd8 100644 --- a/backend/src/routes/adminExpenses.js +++ b/backend/src/routes/adminExpenses.js @@ -41,7 +41,10 @@ function diskUpload(subdir) { }, filename: (_req, file, cb) => cb(null, `${subdir.split('/').pop()}-${Date.now()}${path.extname(file.originalname) || ''}`), }), - limits: { fileSize: 15 * 1024 * 1024 }, + // CVE-2026-82333: both callers (`inboundUpload` → 'file', `proofUpload` + // → 'proof') take a single unnamed field — no legitimate array-indexed + // field names, so reject any bracket-index field name. + limits: { fileSize: 15 * 1024 * 1024, fieldArrayIndexLimit: 0 }, fileFilter: (_req, file, cb) => (ALLOWED_MIME.includes(file.mimetype) ? cb(null, true) : cb(new Error('Only PDF, JPEG or PNG files are allowed'))), }); } diff --git a/backend/src/routes/adminInvoices.js b/backend/src/routes/adminInvoices.js index cc4cfe6e..ec6dd56c 100644 --- a/backend/src/routes/adminInvoices.js +++ b/backend/src/routes/adminInvoices.js @@ -67,7 +67,9 @@ const importedInvoiceStorage = multer.diskStorage({ }); const importedInvoiceUpload = multer({ storage: importedInvoiceStorage, - limits: { fileSize: 10 * 1024 * 1024 }, + // CVE-2026-82333: single unnamed `pdf` field only — no legitimate + // array-indexed field names, so reject any bracket-index field name. + limits: { fileSize: 10 * 1024 * 1024, fieldArrayIndexLimit: 0 }, fileFilter: (_req, file, cb) => { if (file.mimetype === 'application/pdf') cb(null, true); else cb(new Error('Only PDF files are allowed for imported invoices')); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index e6c18c7a..64444637 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -72,7 +72,12 @@ const upload = multer({ files: 2000, // Hard safety ceiling; actual limit enforced dynamically fieldSize: 10 * 1024 * 1024, // 10MB for non-file fields parts: 10000, - headerPairs: 2000 + headerPairs: 2000, + // CVE-2026-82333: files arrive as repeated `photos` parts via + // .array('photos', N) — not bracket-indexed field names like + // `photos[0]` — so no legitimate field name uses array-index syntax + // at all. Reject any that do. + fieldArrayIndexLimit: 0 }, fileFilter: (req, file, cb) => { // req.allowedMimeTypes is populated by the middleware that runs before multer diff --git a/backend/src/routes/adminSettings.js b/backend/src/routes/adminSettings.js index 861ca574..67a5f432 100644 --- a/backend/src/routes/adminSettings.js +++ b/backend/src/routes/adminSettings.js @@ -50,7 +50,10 @@ const { validateFileType } = require('../utils/fileSecurityUtils'); const upload = multer({ storage, - limits: { fileSize: 5 * 1024 * 1024 }, // 5MB + // CVE-2026-82333: single unnamed field (`logo` or `watermarkLogo`) per + // route — no legitimate array-indexed field names, so reject any + // bracket-index field name. + limits: { fileSize: 5 * 1024 * 1024, fieldArrayIndexLimit: 0 }, // 5MB fileFilter: (req, file, cb) => { // Note: SVG files are excluded from magic number validation for logos const allowedMimeTypes = ['image/jpeg', 'image/png', 'image/gif', 'image/svg+xml']; @@ -78,7 +81,9 @@ const faviconStorage = multer.diskStorage({ const faviconUpload = multer({ storage: faviconStorage, - limits: { fileSize: 2 * 1024 * 1024 }, // 2MB — roomy enough for a 512×512+ square PNG + // CVE-2026-82333: single unnamed `favicon` field only — no legitimate + // array-indexed field names, so reject any bracket-index field name. + limits: { fileSize: 2 * 1024 * 1024, fieldArrayIndexLimit: 0 }, // 2MB — roomy enough for a 512×512+ square PNG fileFilter: (req, file, cb) => { const allowedMimeTypes = ['image/png', 'image/x-icon', 'image/vnd.microsoft.icon']; const name = file.originalname.toLowerCase(); diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index c9d95470..1fde4b86 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -2342,7 +2342,12 @@ router.post('/:eventId/upload', verifyGalleryAccess, denySlideshowToken, async ( dest: tempUploadDir, limits: { fileSize: 50 * 1024 * 1024, // 50MB per file (separate concern from #613) - files: maxFilesPerUpload + files: maxFilesPerUpload, + // CVE-2026-82333: files arrive as repeated `photos` parts via + // .array(), not bracket-indexed field names like `photos[0]` — no + // legitimate field name uses array-index syntax at all. Reject any + // that do. + fieldArrayIndexLimit: 0 }, fileFilter: (req, file, cb) => { if (validateFileType(file.originalname, file.mimetype, allowedMimeTypes)) { diff --git a/backend/src/routes/publicContracts.js b/backend/src/routes/publicContracts.js index 374f45c1..bc143573 100644 --- a/backend/src/routes/publicContracts.js +++ b/backend/src/routes/publicContracts.js @@ -62,7 +62,10 @@ const signedPdfStorage = multer.diskStorage({ const signedPdfUpload = multer({ storage: signedPdfStorage, - limits: { fileSize: 10 * 1024 * 1024 }, // 10 MB + // CVE-2026-82333: single unnamed `file` field only, and this route is + // unauthenticated (token-only) — no legitimate array-indexed field + // names, so reject any bracket-index field name. + limits: { fileSize: 10 * 1024 * 1024, fieldArrayIndexLimit: 0 }, // 10 MB fileFilter: (req, file, cb) => { if (validateFileType(file.originalname, file.mimetype, ['application/pdf'])) return cb(null, true); return cb(new Error('Only PDF files are allowed')); diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index d562b77a..dbb6cd1d 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -57,7 +57,9 @@ const photoStorage = multer.diskStorage({ }); const photoUpload = multer({ storage: photoStorage, - limits: { fileSize: 100 * 1024 * 1024 }, // 100MB per file for v1 + // CVE-2026-82333: single unnamed `photo` field only — no legitimate + // array-indexed field names, so reject any bracket-index field name. + limits: { fileSize: 100 * 1024 * 1024, fieldArrayIndexLimit: 0 }, // 100MB per file for v1 fileFilter: (_req, file, cb) => { if (/^image\//.test(file.mimetype)) cb(null, true); else cb(new Error('Only image uploads are accepted on this endpoint'));