feat(uploads): headersSent guard on stream errors + text/html MIME for GridFS#3745
Conversation
…r 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.
|
Warning Review limit reached
More reviews will be available in 52 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR hardens the uploads module with two complementary changes: a ChangesUploads module hardening: stream error guards and text/html MIME support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens the uploads download endpoints against mid-stream GridFS failures that can otherwise trigger ERR_HTTP_HEADERS_SENT, and extends the built-in MIME→extension mapping so text/html buffers stored in GridFS get .html filenames by default.
Changes:
- Add
res.headersSentguard inget()andgetSharp()stream error handlers to avoid double-sending responses; log + destroy the response when headers are already sent. - Add
'text/html': 'html'to the built-inMIME_TO_EXTmap used bycreateFromBuffer(). - Add/extend unit tests to cover the new headers-sent behavior and the built-in
text/htmlmapping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| modules/uploads/controllers/uploads.controller.js | Add logger + headersSent guard to prevent ERR_HTTP_HEADERS_SENT crashes during streaming. |
| modules/uploads/services/uploads.service.js | Add built-in text/html → html mapping for generated filenames. |
| modules/uploads/tests/uploads.controller.contenttype.unit.tests.js | Add logger mocking and new unit tests for pre/post-headers stream error behavior. |
| modules/uploads/tests/uploads.createFromBuffer.unit.tests.js | Add unit test verifying text/html resolves to .html without config overrides. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3745 +/- ##
==========================================
+ Coverage 89.96% 90.03% +0.06%
==========================================
Files 148 148
Lines 4885 4898 +13
Branches 1542 1546 +4
==========================================
+ Hits 4395 4410 +15
+ Misses 385 383 -2
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/uploads/controllers/uploads.controller.js`:
- Around line 42-43: The null-stream check in the getStream handler currently
calls responses.error(res, 404, ...)() but does not return, so execution
continues and dereferences stream (stream.on(...)) causing a TypeError and later
ERR_HTTP_HEADERS_SENT; update the getStream block to immediately return after
calling responses.error(res, 404, ...)(), and apply the same fix to the getSharp
path (the falsy sharp/stream check) so both handlers exit early when the
resource is missing.
- Around line 80-92: getSharp’s current error handler only listens on the
GridFS/source stream; attach the same headersSent-gated handler to the sharp
Transform (or replace inline piping with stream.pipeline) so sharp() transform
errors are caught. Locate the code that calls stream.pipe(sharp(...)).pipe(res)
(the sharp() Transform) and either: 1) create a const transform = sharp(...);
add transform.on('error', (err) => { if (!res.headersSent) {
responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err);
} else { logger.error('uploads.getSharp - sharp transform error after headers
sent', err); res.destroy(err); } }); and then pipe stream -> transform -> res;
or 2) use stream.pipeline(stream, sharp(...), res, (err) => { if (err) { if
(!res.headersSent) { responses.error(res, 422, 'Unprocessable Entity',
errors.getMessage(err))(err); } else { logger.error('uploads.getSharp - pipeline
error after headers sent', err); res.destroy(err); } } }); Ensure references to
responses.error, logger.error and res.destroy remain the same for consistent
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9439798-f7f8-40d2-beb6-775ae31cf2ca
📒 Files selected for processing (4)
modules/uploads/controllers/uploads.controller.jsmodules/uploads/services/uploads.service.jsmodules/uploads/tests/uploads.controller.contenttype.unit.tests.jsmodules/uploads/tests/uploads.createFromBuffer.unit.tests.js
…rrors 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.
PierreBrisorgueil
dismissed
coderabbitai[bot]’s stale review
Both findings addressed in follow-up commit: added return on null-stream 404 paths (get + getSharp) and added error handler on sharp Transform via buildTransform() helper. Comments are on outdated diff.
Closes #3744
Summary
get()andgetSharp()stream error handlers now checkres.headersSentbefore attempting to write a 422 response. If GridFS errors mid-transfer — afterres.set('Content-Type', ...)has already flushed the response head — the handler callslogger.error(...)+res.destroy(err)instead of double-sending, which would throwERR_HTTP_HEADERS_SENTand crash the worker process. Pre-header errors still return 422 as before. Defense-in-depth adopted from trawl hardening; every devkit downstream gets it via/update-stack.'text/html': 'html'added toMIME_TO_EXTinuploads.service.jsso downstream features that store rendered HTML snapshots in GridFS (e.g. error page captures, scrap previews) get correct.htmlfilenames without requiring per-deploymentconfig.uploads.mimeTypesoverrides.text/htmlis intentionally absent fromSAFE_MIME, soget()still downgrades it toapplication/octet-stream+Content-Disposition: attachment— the stored-XSS defense is fully intact.uploads.controller.contenttype.unit.tests.js); MIME map test for built-intext/htmlentry added touploads.createFromBuffer.unit.tests.js. Full unit suite: 1622/1622 pass.Files changed
modules/uploads/controllers/uploads.controller.js— addloggerimport, headersSent guard inget()+getSharp()modules/uploads/services/uploads.service.js— add'text/html': 'html'toMIME_TO_EXTmodules/uploads/tests/uploads.controller.contenttype.unit.tests.js— addmockLoggermock + 4 headersSent guard testsmodules/uploads/tests/uploads.createFromBuffer.unit.tests.js— add text/html MIME map testInvariants preserved
SAFE_MIME/SAFE_IMAGE_MIME/normalizeMime/Content-Disposition: attachmentare all untouchedtext/htmlnot inSAFE_MIME→ still served asapplication/octet-streamwith attachment dispositionReferences
Trawl → Devkit alignment plan:
docs/superpowers/plans/2026-05-30-trawl-devkit-perfect-alignment.mdTask C.3Summary by CodeRabbit
Bug Fixes
New Features