feat(uploads): headersSent guard on stream errors + text/html MIME for GridFS (#3745)

PierreBrisorgueil · web-flow · commit 2ff416183563 · 2026-05-31T20:41:43.000+02:00
* feat(uploads): headersSent guard on stream errors + text/html MIME for GridFS (#3744) Add res.headersSent check in get() and getSharp() stream error handlers so a GridFS failure mid-transfer (after headers are already flushed) destroys the socket via res.destroy(err) + logger.error instead of attempting a second status+body write that throws ERR_HTTP_HEADERS_SENT. Pre-header errors still return 422 as before. Add 'text/html': 'html' to MIME_TO_EXT so downstream features storing rendered HTML snapshots in GridFS get correct .html filenames without per-deployment config overrides. text/html is intentionally absent from SAFE_MIME, so the Content-Type allowlist + Content-Disposition: attachment XSS defenses remain intact. Port stream-error tests for the headersSent guard and add a MIME map test for text/html. Full unit suite: 1622/1622 pass. Adopted from trawl prod hardening; every devkit downstream gets it via /update-stack. * fix(uploads): add return on null-stream 404 + guard sharp transform errors Address two CodeRabbit findings: 1. Add return before responses.error(res, 404) in both get() and getSharp() so execution does not fall through to stream.on() when getStream returns falsy — preventing a TypeError and ERR_HTTP_HEADERS_SENT crash on the null-stream path. 2. Attach the headersSent-gated error handler to the sharp Transform (not just the GridFS source stream). pipe() does not forward error events between streams, so sharp-pipeline failures were unhandled and could crash the worker process. Refactored to buildTransform() helper that creates the transform, attaches the error handler, and returns it. Update comment to accurately describe when headersSent flips to true (first chunk written, not res.set()). Add test: sharp transform error after headers sent → res.destroy + logger. Full unit suite: 1623/1623 pass.
diff --git 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
@@ -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
@@ -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
@@ -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 } };
