fix: throw error when Content-Type is invalid#7036
fix: throw error when Content-Type is invalid#7036aviu16 wants to merge 2 commits intoexpressjs:masterfrom
Conversation
when you call res.set('Content-Type', value) with something thats not a
real MIME type, mime.contentType() returns false and that false was getting
coerced to the string "false" and set as the header. now it throws a
TypeError instead.
fixes expressjs#7034
329913c to
0d3809c
Compare
|
friendly ping: happy to adjust anything here if needed. |
|
added context on expected behavior: mime.contentType() can return false for invalid input, so this change throws a TypeError early instead of passing an invalid value into setHeader. happy to add/adjust tests if you want coverage around this branch. |
|
added regression coverage in test/res.set.js.\nnew case asserts invalid Content-Type input returns 500 with TypeError invalid content type.\n\nran locally: npm test -- test/res.set.js\nresult: passing. |
|
Friendly bump on this PR. When maintainers have a moment, could someone please take a look? Happy to make any requested changes quickly. Thanks! |
|
It has not been decided yet what we want to do with the issue - see #7034 (comment). It's hard to review PRs that "solve" issues when it has not been agreed what is/should be the correct behaviour. |
erdinccurebal
left a comment
There was a problem hiding this comment.
This addresses the same underlying issue as #7035 — mime.contentType() returning false for unknown types — but takes a stricter approach by throwing a TypeError.
A few thoughts:
-
Breaking change concern: Existing apps that set custom/non-standard Content-Type values (e.g.
res.set('Content-Type', 'custom-type')) would start throwing. This could break production code on upgrade. -
Error message:
'invalid content type'could be more descriptive — e.g.'Invalid Content-Type "' + value + '"'so the developer immediately knows which value caused the issue. -
Comparison with #7035: PR #7035 takes a more permissive approach (
mime.contentType(value) || value), preserving the original value when the lookup fails. That approach is more backwards-compatible but less strict.
I'd lean toward #7035's approach being safer for a patch release, since throwing is a breaking behavior change. But if the maintainers prefer strictness, this approach is cleaner.
Summary
Fixes #7034
When calling
res.set('Content-Type', value)wherevalueis not a valid MIME type (e.g.,'some-custom-type'),mime.contentType()returnsfalse. Previously, thisfalsevalue was coerced to the literal string"false"and set as the Content-Type header.Changes
This PR modifies
res.set()inlib/response.jsto throw aTypeErrorwhenmime.contentType()returnsfalse, making the error explicit and easier to catch during development.Rationale
As noted by @krzysdz in #7034, the Content-Type header must contain a valid media type with a
/separator (per RFC 9110). Silently setting the header to"false"creates invalid HTTP responses that are harder to debug.This change aligns with the existing
TypeErrorthrown when Content-Type is set to an Array (line 674).Test plan
npm test- 1244 passing)TypeErrorapplication/json,text/html)