Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle invalid status codes #3137

Closed
wants to merge 1 commit into from

Conversation

nickclaw
Copy link

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.

this.statusCode = chunk;
chunk = statusCodes[chunk];
this.status(chunk);
chunk = statusCodes[this.statusCode] || String(this.statusCode);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly different behavior here: "code" vs undefined for invalid codes. Happy to change it back.

@dougwilson
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@nickclaw
Copy link
Author

Will do!

@nickclaw
Copy link
Author

Closing in lieu of #3143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants