From fdb12aed1e32e877f6bbf2a4b699b88a7eec147d Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 22 Sep 2026 01:21:57 +0200 Subject: [PATCH] test(files): upload filters fail closed on an excluded or unresolvable type --- server/src/nest/files/files.controller.ts | 13 +- server/src/nest/journey/journey.controller.ts | 32 +++-- server/tests/unit/nest/files.service.test.ts | 4 +- .../unit/nest/files/upload-filters.test.ts | 120 ++++++++++++++++++ 4 files changed, 150 insertions(+), 19 deletions(-) create mode 100644 server/tests/unit/nest/files/upload-filters.test.ts diff --git a/server/src/nest/files/files.controller.ts b/server/src/nest/files/files.controller.ts index b303839a0..490053f5d 100644 --- a/server/src/nest/files/files.controller.ts +++ b/server/src/nest/files/files.controller.ts @@ -56,14 +56,21 @@ export function filesUploadFileFilter(allowedTypes: AllowedFileTypesService): Op // allowed-extension list is now an async read. The decision runs in a // detached async function that always answers through `cb` — multer holds // the file stream until it does (make-middleware.js says as much). A - // rejection refuses the file, so the filter still fails closed. + // rejection refuses the file, so the filter still fails closed. Only the + // await is wrapped in try/catch, so a throw from cb() itself is never + // re-routed into a second cb() call. void (async () => { - const allowed = (await allowedTypes.get()).split(',').map((e) => e.trim().toLowerCase()); + let allowed: string[]; + try { + allowed = (await allowedTypes.get()).split(',').map((e) => e.trim().toLowerCase()); + } catch { + return reject(); + } const fileExt = ext.replace('.', ''); // Video is accepted as media regardless of the admin doc-types allowlist (#823). if (allowed.includes(fileExt) || isVideoExtension(fileExt) || (allowed.includes('*') && !BLOCKED_EXTENSIONS.includes(ext))) return cb(null, true); reject(); - })().catch(() => reject()); + })(); }; } diff --git a/server/src/nest/journey/journey.controller.ts b/server/src/nest/journey/journey.controller.ts index 0e14a3509..09527a102 100644 --- a/server/src/nest/journey/journey.controller.ts +++ b/server/src/nest/journey/journey.controller.ts @@ -78,23 +78,27 @@ export function journeyImageFileFilter(allowedTypes: AllowedFileTypesService): O return cb(err); } const ext = path.extname(file.originalname).toLowerCase().replace('.', ''); - // R1.5: multer's fileFilter is a callback API that cannot await, and the - // allowed-extension list is now an async read. The decision runs in a - // detached async function that always answers through `cb`; a rejection - // refuses the file, so the filter still fails closed. - void (async () => { - const allowed = (await allowedTypes.get()).split(',').map((e) => e.trim().toLowerCase()); - if (!allowed.includes('*') && !allowed.includes(ext)) { - const err: Error & { statusCode?: number } = new Error(`File type .${ext} is not allowed`); - err.statusCode = 400; - return cb(err); - } - cb(null, true); - })().catch(() => { + const reject = () => { const err: Error & { statusCode?: number } = new Error(`File type .${ext} is not allowed`); err.statusCode = 400; cb(err); - }); + }; + // R1.5: multer's fileFilter is a callback API that cannot await, and the + // allowed-extension list is now an async read. The decision runs in a + // detached async function that always answers through `cb`; a rejection + // refuses the file, so the filter still fails closed. Only the await is + // wrapped in try/catch, so a throw from cb() itself is never re-routed + // into a second cb() call. + void (async () => { + let allowed: string[]; + try { + allowed = (await allowedTypes.get()).split(',').map((e) => e.trim().toLowerCase()); + } catch { + return reject(); + } + if (!allowed.includes('*') && !allowed.includes(ext)) return reject(); + cb(null, true); + })(); }; } diff --git a/server/tests/unit/nest/files.service.test.ts b/server/tests/unit/nest/files.service.test.ts index 210bc9c32..dc4c69745 100644 --- a/server/tests/unit/nest/files.service.test.ts +++ b/server/tests/unit/nest/files.service.test.ts @@ -343,7 +343,7 @@ describe('updateFile', () => { const { user, trip } = seedTrip(); const place = createPlace(testDb, trip.id); const file = await makeFile(trip.id, user.id, {}, { description: 'keep me', place_id: String(place.id) }); - const current = await svc.getFileById(file.id, trip.id)!; + const current = (await svc.getFileById(file.id, trip.id))!; const updated = await svc.updateFile(file.id, current, {}) as Record; expect(updated.description).toBe('keep me'); expect(updated.place_id).toBe(place.id); @@ -354,7 +354,7 @@ describe('updateFile', () => { const place = createPlace(testDb, trip.id); const reservation = createReservation(testDb, trip.id); const file = await makeFile(trip.id, user.id, {}, { description: 'old', place_id: String(place.id), reservation_id: String(reservation.id) }); - const current = await svc.getFileById(file.id, trip.id)!; + const current = (await svc.getFileById(file.id, trip.id))!; const updated = await svc.updateFile(file.id, current, { description: '', place_id: '', reservation_id: null }) as Record; expect(updated.description).toBeNull(); // '' → NULL on update too (post-migration fix: symmetric with createFile) expect(updated.place_id).toBeNull(); diff --git a/server/tests/unit/nest/files/upload-filters.test.ts b/server/tests/unit/nest/files/upload-filters.test.ts new file mode 100644 index 000000000..ab3b6141e --- /dev/null +++ b/server/tests/unit/nest/files/upload-filters.test.ts @@ -0,0 +1,120 @@ +import { describe, it, expect, vi } from 'vitest'; +import type { Request } from 'express'; +import { filesUploadFileFilter } from '../../../../src/nest/files/files.controller'; +import { journeyImageFileFilter } from '../../../../src/nest/journey/journey.controller'; +import type { AllowedFileTypesService } from '../../../../src/nest/files/allowed-file-types.service'; + +/** + * Unit coverage for the two upload fileFilters that read the operator's + * allowed-extension list asynchronously (files.controller.ts, + * journey.controller.ts). Both filters run a synchronous pre-check first (a + * blocked extension / non-image mimetype), then a detached async IIFE that + * always answers through `cb` exactly once — even when the allow-list lookup + * itself rejects (fail closed, never `cb(null, true)`). + */ + +const req = {} as Request; + +function fakeFile(overrides: Partial = {}): Express.Multer.File { + return { + fieldname: 'file', + originalname: 'photo.jpg', + encoding: '7bit', + mimetype: 'image/jpeg', + size: 100, + stream: undefined as never, + destination: '', + filename: '', + path: '', + buffer: undefined as never, + ...overrides, + }; +} + +/** A stub whose get() can either resolve to a comma-separated list or reject. */ +function allowedTypesStub(result: string | Error): AllowedFileTypesService { + return { + get: vi.fn(() => (result instanceof Error ? Promise.reject(result) : Promise.resolve(result))), + } as unknown as AllowedFileTypesService; +} + +/** Wait a couple of microtask turns so the filter's detached async IIFE settles. */ +async function flush(): Promise { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); +} + +describe('filesUploadFileFilter', () => { + it('UPLOAD-FILTER-001: a blocked extension is rejected synchronously, before the allow-list is read', async () => { + const allowedTypes = allowedTypesStub('jpg,png'); + const filter = filesUploadFileFilter(allowedTypes)!; + const cb = vi.fn(); + filter(req, fakeFile({ originalname: 'malware.exe', mimetype: 'application/octet-stream' }), cb); + await flush(); + expect(cb).toHaveBeenCalledTimes(1); + const err = cb.mock.calls[0][0] as (Error & { statusCode?: number }) | null; + expect(err).toBeInstanceOf(Error); + expect(err?.statusCode).toBe(400); + expect(allowedTypes.get).not.toHaveBeenCalled(); + }); + + it('UPLOAD-FILTER-002: an unresolvable allow-list rejects the file, never cb(null, true)', async () => { + const allowedTypes = allowedTypesStub(new Error('db down')); + const filter = filesUploadFileFilter(allowedTypes)!; + const cb = vi.fn(); + filter(req, fakeFile({ originalname: 'photo.jpg', mimetype: 'image/jpeg' }), cb); + await flush(); + expect(cb).toHaveBeenCalledTimes(1); + const [err, accept] = cb.mock.calls[0] as [(Error & { statusCode?: number }) | null, boolean | undefined]; + expect(err).toBeInstanceOf(Error); + expect(accept).toBeUndefined(); + }); + + it('UPLOAD-FILTER-003: an allowed extension resolves cb(null, true) exactly once', async () => { + const allowedTypes = allowedTypesStub('jpg,png'); + const filter = filesUploadFileFilter(allowedTypes)!; + const cb = vi.fn(); + filter(req, fakeFile({ originalname: 'photo.jpg', mimetype: 'image/jpeg' }), cb); + await flush(); + expect(cb).toHaveBeenCalledTimes(1); + expect(cb).toHaveBeenCalledWith(null, true); + }); +}); + +describe('journeyImageFileFilter', () => { + it('UPLOAD-FILTER-004: a non-image mimetype is rejected synchronously, before the allow-list is read', async () => { + const allowedTypes = allowedTypesStub('jpg,png'); + const filter = journeyImageFileFilter(allowedTypes)!; + const cb = vi.fn(); + filter(req, fakeFile({ originalname: 'notes.pdf', mimetype: 'application/pdf' }), cb); + await flush(); + expect(cb).toHaveBeenCalledTimes(1); + const err = cb.mock.calls[0][0] as (Error & { statusCode?: number }) | null; + expect(err).toBeInstanceOf(Error); + expect(err?.statusCode).toBe(400); + expect(allowedTypes.get).not.toHaveBeenCalled(); + }); + + it('UPLOAD-FILTER-005: an unresolvable allow-list rejects the file, never cb(null, true)', async () => { + const allowedTypes = allowedTypesStub(new Error('db down')); + const filter = journeyImageFileFilter(allowedTypes)!; + const cb = vi.fn(); + filter(req, fakeFile({ originalname: 'photo.jpg', mimetype: 'image/jpeg' }), cb); + await flush(); + expect(cb).toHaveBeenCalledTimes(1); + const [err, accept] = cb.mock.calls[0] as [(Error & { statusCode?: number }) | null, boolean | undefined]; + expect(err).toBeInstanceOf(Error); + expect(accept).toBeUndefined(); + }); + + it('UPLOAD-FILTER-006: an allowed image extension resolves cb(null, true) exactly once', async () => { + const allowedTypes = allowedTypesStub('jpg,png'); + const filter = journeyImageFileFilter(allowedTypes)!; + const cb = vi.fn(); + filter(req, fakeFile({ originalname: 'photo.jpg', mimetype: 'image/jpeg' }), cb); + await flush(); + expect(cb).toHaveBeenCalledTimes(1); + expect(cb).toHaveBeenCalledWith(null, true); + }); +});