Gracefully handle invalid status codes

[nickclaw]

#3137 off the 5.0 branch

[dougwilson]

You should avoid reassignment of function arguments, please

[dougwilson]

Also, I don't think we should parse at all. Throw if it is not a number primitive

[nickclaw]

Node/express currently works with any numeric string as a status code (node does statusCode |= 0) under the hood, seems like only accepting numbers could be an annoying change for anyone relying on it. Thoughts?

[dougwilson]

AFAIK that was done so they could add the restriction in a minor release instead of a major. Unless you want to make sure that this.status('500 cats') does not silently work, then I think it should just require a number. In fact, it should require that number to be an integer, so this.status(500.1) should throw as well.

The purpose is to make it obvious when there is a programming error, not to silently correct mistakes.

[dougwilson]

Basically, if the purpose of this isn't to catch obviously invalid input up front, then there doesn't seem a point to the PR to me, as we can just let Node.js catch it as it does today.

[dougwilson]

Since you opened a new PR instead of just continuing on the old one, we've lost all the conversation and your description. Can you please transcribe it all over to this PR so people can read it as they come along?

[nickclaw]

Transcription of initial PR

nickclaw

Currently if an invalid status code is sent in the response, node throws an exception on writeHead that 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.

dougwilson

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

dougwilson

that afaik can't be handled in express. Instead it just leads to a hanging request.

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.

nickclaw

Can you please provide a test case that demonstrates this behavior?

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?

dougwilson

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.

nickclaw

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.

dougwilson

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?

nickclaw

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.

dougwilson

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 :)

dougwilson

If you can retarget the PR to be against 5.0 branch as well, that would be awesome :)

nickclaw

Will do!

[jonchurch]

Functionality wise, this essentially needs one more check to make sure a number is an integer. In order to catch 200.1 etc.

One question I have though @dougwilson is about the spirit of this change, are we trying to catch error cases in line w/ what Node.js will throw on? Or create a slightly more strict error case.

I ask because of two cases: '200' and 200.0 (a valid status which happens to be a string, and a valid status which is not an integer) are two statuses that Node.js accepts as valid. (Or rather, when using Express today to set these statuses, they are accepted without generating an Error at the native HTTP level).

I can add in the check for strings like you mentioned in a comment:

...Unless you want to make sure that this.status('500 cats') does not silently work, then I think it should just require a number.

[nickclaw]

@jonchurch cool to see this is being addressed, I had totally forgotten about it :)

Are you going to be making a PR to replace this one? I can close this PR if so.

[LinusU]

@jonchurch just a small note: 200.0 and 200 in JavaScript are exactly identical so there will be no way to detect res.status(200.0); or maybe it was just a typo and you meant 200.1? ☺️

That being said, I think that being strict about it and only allowing numbers (not strings) and checking with Number.isInteger would be a good approach!

Another question is wether or not we should allow 600 - 999, currently this PR does, but I think that HTTP status codes can only be 100 - 599? 🤔

[dougwilson]

One question I have though @dougwilson is about the spirit of this change, are we trying to catch error cases in line w/ what Node.js will throw on? Or create a slightly more strict error case.

The spirit of the change is to throw on the cases in which Node.js will throw on later. We could add more on top of that, but it should be for a good reason without hurting users. For example Koa is very strict and it prevents uses that Node.js does allow that they need.

The pain point that is trying to be addressed is that if res.status() is given a value that Node.js rejects, it will get rejected at a completely different part of the code than where res.status() is called which is at least confusing and hard to debug.

[jonchurch]

Closing this since it's being worked on in #4212