Gracefully handle invalid status codes#3137
Gracefully handle invalid status codes#3137nickclaw wants to merge 1 commit intoexpressjs:masterfrom
Conversation
| this.statusCode = chunk; | ||
| chunk = statusCodes[chunk]; | ||
| this.status(chunk); | ||
| chunk = statusCodes[this.statusCode] || String(this.statusCode); |
There was a problem hiding this comment.
Slightly different behavior here: "code" vs undefined for invalid codes. Happy to change it back.
|
Since previous version of Node.js did allow this and it worked, doing this is a breaking change to Express that cannot land until 5.x |
I have never seen this. Can you please provide a test case that demonstrates this behavior? That would be the only reason to land something like this on 4.x, if you can show how Express is causing an uncatchable exception that will hang the response. |
Whoops, more testing showed you were right. It's just the uncaught exception in conjunction with bad handling of errors in the app I was looking at. Would it be worth throwing a TypeError instead if you want this for v5? Or do you prefer merging #2797? |
|
I mean, #2797 seemed fine enough to merge, and I think there was a bunch of conversation before that throwing an error would be less surprising than silently changing the error, especially to figure out where in the code you're giving the method the wrong input. |
|
I definitely agree throwing an error is better, I guess the only issue I have with #2797 is that it's making different checks than what node is checking for -- but maybe that's appropriate if we don't know what changes they'll make in the future. |
|
Ah, yea, just looked at it and I see now. This is actually the third open PR to add this check. Maybe the other one has the code number check? |
|
There's #3111, which is functionally the same as #2797. Both don't handle sendStatus or invalid numeric input. Anyways, feel free to close this if you want. Otherwise I'd be happy to rewrite it to a) throw a type error, and b) not change the behavior around the currently deprecated ways to set status which are being removed in v5. |
|
Gotcha. Yea, the check for the numbers I think is good, to match Node.js core. This means I believe your PR is better, and will review your PR with my thoughts on changes that need to be made :) |
|
If you can retarget the PR to be against 5.0 branch as well, that would be awesome :) |
|
Will do! |
|
Closing in lieu of #3143 |
2 participants
Currently if an invalid status code is sent in the response, node throws an exception on
writeHeadthat afaik can't be handled in express. Instead it just leads to a hanging request. I realize that it's best practice for people to only send valid status codes, but I have seen:if (err) res.sendStatus(err.code || 500)far too many times.
This change is similar to #2797, which I think is ultimately a better solution. But this fix should be non breaking.