diff --git a/modules/uploads/controllers/uploads.controller.js b/modules/uploads/controllers/uploads.controller.js index 008dd3979..e0af281b2 100644 --- a/modules/uploads/controllers/uploads.controller.js +++ b/modules/uploads/controllers/uploads.controller.js @@ -6,6 +6,7 @@ import _ from 'lodash'; import config from '../../../config/index.js'; import errors from '../../../lib/helpers/errors.js'; +import logger from '../../../lib/services/logger.js'; import responses from '../../../lib/helpers/responses.js'; import UploadsService from '../services/uploads.service.js'; @@ -38,9 +39,21 @@ const normalizeMime = (raw) => String(raw).toLowerCase().split(';')[0].trim(); const get = async (req, res) => { try { const stream = await UploadsService.getStream({ _id: req.upload._id }); - if (!stream) responses.error(res, 404, 'Not Found', 'No Upload with that identifier can been found')(); + if (!stream) return responses.error(res, 404, 'Not Found', 'No Upload with that identifier can been found')(); stream.on('error', (err) => { - responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); + // Guard against ERR_HTTP_HEADERS_SENT when GridFS fails mid-transfer. + // Response headers are written when the first chunk is piped to res + // (`stream.pipe(res)` below). If GridFS errors after that point, a + // second status+body write would throw ERR_HTTP_HEADERS_SENT and crash + // the worker process. Pre-header errors still reach the client as 422; + // post-header errors destroy the socket so Express surfaces them via + // its default error handler instead of double-sending. + if (!res.headersSent) { + responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); + } else { + logger.error('uploads.get - stream error after headers sent', err); + res.destroy(err); + } }); const raw = req.upload.contentType || req.upload.metadata?.contentType || 'application/octet-stream'; const norm = normalizeMime(raw); @@ -63,27 +76,45 @@ const get = async (req, res) => { const getSharp = async (req, res) => { try { const stream = await UploadsService.getStream({ _id: req.upload._id }); - if (!stream) responses.error(res, 404, 'Not Found', 'No Upload with that identifier can been found')(); - stream.on('error', (err) => { - responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); - }); + if (!stream) return responses.error(res, 404, 'Not Found', 'No Upload with that identifier can been found')(); + // headersSent guard for the GridFS source stream. + // pipe() does not forward 'error' events between streams; each stream in the + // chain needs its own listener. Source and Transform errors are handled below. + // Pre-header errors reach the client as 422; post-header errors destroy the + // socket so Express surfaces them via its default handler instead of + // attempting a second write that would throw ERR_HTTP_HEADERS_SENT. + const onStreamError = (label) => (err) => { + if (!res.headersSent) { + responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); + } else { + logger.error(`uploads.getSharp - ${label} error after headers sent`, err); + res.destroy(err); + } + }; + stream.on('error', onStreamError('source stream')); const raw = req.upload.contentType || req.upload.metadata?.contentType || 'application/octet-stream'; const norm = normalizeMime(raw); const contentType = SAFE_IMAGE_MIME.has(norm) ? norm : 'image/jpeg'; res.set('Content-Type', contentType); - switch (req.sharpOption) { - case 'blur': - stream.pipe(sharp().resize(req.sharpSize).blur(config.uploads.sharp.blur)).pipe(res); - break; - case 'bw': - stream.pipe(sharp().resize(req.sharpSize).grayscale()).pipe(res); - break; - case 'blur&bw': - stream.pipe(sharp().resize(req.sharpSize).grayscale().blur(config.uploads.sharp.blur)).pipe(res); - break; - default: - stream.pipe(sharp().resize(req.sharpSize)).pipe(res); - } + const buildTransform = () => { + let transform; + switch (req.sharpOption) { + case 'blur': + transform = sharp().resize(req.sharpSize).blur(config.uploads.sharp.blur); + break; + case 'bw': + transform = sharp().resize(req.sharpSize).grayscale(); + break; + case 'blur&bw': + transform = sharp().resize(req.sharpSize).grayscale().blur(config.uploads.sharp.blur); + break; + default: + transform = sharp().resize(req.sharpSize); + } + transform.on('error', onStreamError('sharp transform')); + return transform; + }; + stream.pipe(buildTransform()).pipe(res); } catch (err) { responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); } diff --git a/modules/uploads/services/uploads.service.js b/modules/uploads/services/uploads.service.js index 4cce8cd15..d0ece1233 100644 --- a/modules/uploads/services/uploads.service.js +++ b/modules/uploads/services/uploads.service.js @@ -15,6 +15,9 @@ const MIME_TO_EXT = { 'image/png': 'png', 'image/gif': 'gif', 'application/pdf': 'pdf', + // Allows downstream features that store rendered HTML snapshots in GridFS + // alongside binary uploads (e.g. error pages, scrap previews). + 'text/html': 'html', }; /** diff --git a/modules/uploads/tests/uploads.controller.contenttype.unit.tests.js b/modules/uploads/tests/uploads.controller.contenttype.unit.tests.js index 6f7f9fa87..649aa5771 100644 --- a/modules/uploads/tests/uploads.controller.contenttype.unit.tests.js +++ b/modules/uploads/tests/uploads.controller.contenttype.unit.tests.js @@ -35,6 +35,11 @@ describe('Uploads controller content-type allowlist unit tests:', () => { }; }; + let mockLogger; + // Captures the 'error' listener registered on the sharp Transform so tests + // can fire it manually to simulate a sharp-pipeline mid-stream failure. + let sharpListeners; + beforeEach(async () => { jest.resetModules(); @@ -44,6 +49,9 @@ describe('Uploads controller content-type allowlist unit tests:', () => { remove: jest.fn(), }; + mockLogger = { error: jest.fn(), warn: jest.fn(), info: jest.fn() }; + sharpListeners = {}; + jest.unstable_mockModule('../services/uploads.service.js', () => ({ default: mockUploadsService, })); @@ -56,12 +64,16 @@ describe('Uploads controller content-type allowlist unit tests:', () => { // sharp is a real dependency; mock it to avoid needing native bindings in unit context. // pipe must return `dest` (matching the real Stream.pipe contract) so that chained // calls stream.pipe(transform).pipe(res) work without throwing. + // on() captures listeners so tests can fire transform error events. jest.unstable_mockModule('sharp', () => ({ default: jest.fn(() => ({ resize: jest.fn().mockReturnThis(), blur: jest.fn().mockReturnThis(), grayscale: jest.fn().mockReturnThis(), pipe: jest.fn((dest) => dest), + on: jest.fn((event, handler) => { + sharpListeners[event] = handler; + }), })), })); @@ -76,6 +88,10 @@ describe('Uploads controller content-type allowlist unit tests:', () => { }, })); + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: mockLogger, + })); + const mod = await import('../controllers/uploads.controller.js'); UploadsController = mod.default; }); @@ -360,4 +376,161 @@ describe('Uploads controller content-type allowlist unit tests:', () => { expect(res.set).toHaveBeenCalledWith('Content-Type', 'image/jpeg'); }); }); + + // ── get — headersSent guard ─────────────────────────────────────────────── + + describe('get — headersSent guard on stream error', () => { + /** + * Install a fake stream that captures the `error` listener so tests can + * fire it manually — simulating either a pre-flush or mid-flush GridFS + * failure depending on `res.headersSent`. + */ + const installStream = () => { + const listeners = {}; + const stream = { + pipe: jest.fn().mockReturnThis(), + on: jest.fn((event, handler) => { + listeners[event] = handler; + return stream; + }), + }; + mockUploadsService.getStream.mockResolvedValueOnce(stream); + return { stream, listeners }; + }; + + const buildRes = () => { + const res = {}; + res.status = jest.fn().mockReturnValue(res); + res.json = jest.fn().mockReturnValue(res); + res.set = jest.fn(); + res.destroy = jest.fn(); + res.headersSent = false; + return res; + }; + + const req = { + upload: { + _id: 'u1', + contentType: 'image/jpeg', + length: 1234, + metadata: { contentType: 'image/jpeg' }, + }, + }; + + test('responds via responses.error when stream errors before headers are sent', async () => { + const { listeners } = installStream(); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + const res = buildRes(); + + await UploadsController.get(req, res); + + res.headersSent = false; + listeners.error(new Error('gridfs chunk missing')); + + expect(responses.error).toHaveBeenCalled(); + expect(res.destroy).not.toHaveBeenCalled(); + }); + + test('calls res.destroy(err) and logger.error when stream errors after headers are sent', async () => { + const { listeners } = installStream(); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + const res = buildRes(); + + await UploadsController.get(req, res); + + res.headersSent = true; + const err = new Error('gridfs mid-stream abort'); + listeners.error(err); + + // Must NOT attempt a new response body on an already-flushed response. + expect(responses.error).not.toHaveBeenCalled(); + expect(res.destroy).toHaveBeenCalledWith(err); + // Error must reach the logger so mid-stream GridFS failures are visible. + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining('uploads.get'), err); + }); + }); + + // ── getSharp — headersSent guard ────────────────────────────────────────── + + describe('getSharp — headersSent guard on stream error', () => { + const installStream = () => { + const listeners = {}; + const stream = { + pipe: jest.fn().mockReturnThis(), + on: jest.fn((event, handler) => { + listeners[event] = handler; + return stream; + }), + }; + mockUploadsService.getStream.mockResolvedValueOnce(stream); + return { stream, listeners }; + }; + + const buildRes = () => { + const res = {}; + res.status = jest.fn().mockReturnValue(res); + res.json = jest.fn().mockReturnValue(res); + res.set = jest.fn(); + res.destroy = jest.fn(); + res.headersSent = false; + return res; + }; + + const req = { + upload: { + _id: 'u1', + contentType: 'image/jpeg', + metadata: { contentType: 'image/jpeg' }, + }, + sharpSize: null, + sharpOption: null, + }; + + test('responds via responses.error when stream errors before headers are sent', async () => { + const { listeners } = installStream(); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + const res = buildRes(); + + await UploadsController.getSharp(req, res); + + res.headersSent = false; + listeners.error(new Error('early gridfs failure')); + + expect(responses.error).toHaveBeenCalled(); + expect(res.destroy).not.toHaveBeenCalled(); + }); + + test('calls res.destroy(err) and logger.error when source stream errors after headers are sent', async () => { + const { listeners } = installStream(); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + const res = buildRes(); + + await UploadsController.getSharp(req, res); + + res.headersSent = true; + const err = new Error('gridfs source mid-stream abort'); + listeners.error(err); + + expect(responses.error).not.toHaveBeenCalled(); + expect(res.destroy).toHaveBeenCalledWith(err); + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining('uploads.getSharp'), err); + }); + + test('calls res.destroy(err) and logger.error when sharp transform errors after headers are sent', async () => { + installStream(); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + const res = buildRes(); + + await UploadsController.getSharp(req, res); + + // Simulate a sharp transform error mid-stream (after headers are flushed). + res.headersSent = true; + const err = new Error('sharp transform mid-stream abort'); + sharpListeners.error(err); + + expect(responses.error).not.toHaveBeenCalled(); + expect(res.destroy).toHaveBeenCalledWith(err); + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining('uploads.getSharp'), err); + }); + }); }); diff --git a/modules/uploads/tests/uploads.createFromBuffer.unit.tests.js b/modules/uploads/tests/uploads.createFromBuffer.unit.tests.js index ffe0904e5..99a691e0f 100644 --- a/modules/uploads/tests/uploads.createFromBuffer.unit.tests.js +++ b/modules/uploads/tests/uploads.createFromBuffer.unit.tests.js @@ -175,6 +175,24 @@ describe('Uploads createFromBuffer unit tests:', () => { expect(filename).toMatch(/^[a-f0-9]{64}\.html$/); }); + test('should resolve text/html to .html extension via built-in MIME_TO_EXT (no config.uploads.mimeTypes needed)', async () => { + // text/html is in the built-in MIME_TO_EXT map so downstream features that + // store rendered HTML snapshots in GridFS get a correct .html filename + // without requiring per-deployment mimeTypes config. + mockConfig.uploads.snapshot = { + kind: 'snapshot', + formats: ['text/html'], + limits: { fileSize: 1 * 1024 * 1024 }, + }; + mockGridfs.createFromBuffer.mockResolvedValue({ ...fakeFile, contentType: 'text/html' }); + + const buffer = Buffer.alloc(512); + await UploadsService.createFromBuffer(buffer, 'text/html', 'snapshot'); + + const [, filename] = mockGridfs.createFromBuffer.mock.calls[0]; + expect(filename).toMatch(/^[a-f0-9]{64}\.html$/); + }); + test('should throw error when kind has no formats configured', async () => { // Adding 'broken' kind at runtime — service reads config dynamically via module reference mockConfig.uploads.broken = { kind: 'broken', limits: { fileSize: 1024 } };