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

[v5] Lock down valid status code ranges to 1xx through 5xx #5623

Open
jonchurch opened this issue Apr 28, 2024 · 11 comments
Open

[v5] Lock down valid status code ranges to 1xx through 5xx #5623

jonchurch opened this issue Apr 28, 2024 · 11 comments
Assignees
Labels

Comments

@jonchurch
Copy link
Member

jonchurch commented Apr 28, 2024

Node's http module accepts status codes from 100 up to 999.

Do we want to lock this down further in express to only accept status codes in the 1xx to 5xx ranges? Proposed behavior is to throw a RangeError when encountering one outside the accepted range.

Im for it.

@jonchurch jonchurch added the 5.x label Apr 28, 2024
@jonchurch jonchurch self-assigned this Apr 28, 2024
@jonchurch jonchurch changed the title [v5] Lock down valid status code ranges [v5] Lock down valid status code ranges to 1xx through 5xx Apr 28, 2024
@UlisesGascon
Copy link
Member

The status code of a response is a three-digit integer code that describes the result of the request and the semantics of the response, including whether the request was successful and what content is enclosed (if any). All valid status codes are within the range of 100 to 599, inclusive. RFC9110 Reference

The specs are clear on this, I am +1

@jonchurch

This comment was marked as off-topic.

@wesleytodd

This comment was marked as off-topic.

@jonchurch

This comment was marked as off-topic.

@jonchurch
Copy link
Member Author

jonchurch commented Apr 29, 2024

@blakeembrey brought up a good point, do we want to take this opinionated approach which would prevent anyone from building something on top of express which defines custom http status codes? I don't really know, bc I am not aware of anyone who does that.

edit: that's not how this works actually, these would be strings, which we've already said in the past we wanted to make res.status only take integers anyways. So this example is moot.
The benefit I see is that it would prevent folks doing something weird like if (err) { res.status(err.code ?? 500)

But that's not a huge benefit really, compared to the reduction in flexibility.

I see express as an HTTP framework, and 999 as an invalid HTTP status code. It feels natural to me to take the HTTP spec's definition of a valid status code.

If folks complain, we could add res.statusNoThrow(999) at the request of real users know who what they're doing

@blakeembrey
Copy link
Member

There's modules written jokingly using express 7xx: https://github.com/devmaximilian/http-700

There also appears to be some valid reason for a CDN to support your server returning a non-standard code: https://www.fastly.com/documentation/reference/http/http-statuses/

I'm in favor of not constraining it, unless it turned out this was a large source of error for users. Both sides seem kind of weak so I'm more in favor of staying un-opinionated.

I'd love to take a strong stance either way, not a weak stance adding two different methods or flags anywhere. The only way I'd be in favor of something like that is to potentially have a "HTTP status code" map and if it's not in the map throw. E.g. if (!res.supportedStatuses[x]) throw ... as this does not impact performance or make support more complex.

@UlisesGascon
Copy link
Member

I agree with @blakeembrey. I've seen cases where people use status codes outside of the standard for ad-hoc integrations and edge cases.

Perhaps we could add this and other similar spec "opinionated" features into a separate middleware? This could follow the helmet approach for security but focus on a more strict/opinionated way of using Express. Just to clarify, I love that Express is not opinionated, but maybe we could provide easy plugins for end users 🙂

@jonchurch
Copy link
Member Author

jonchurch commented Apr 30, 2024

Sounds like a plan! Let's not limit the range of status codes in v5.

To be clear, we are in agreement still that we want to throw a RangeError if someone attempts to set a status code outside of what Node expects?

That was the original feature in #4212 , Node throws if given res.status(1000 and you can't really handle it in Express currently. Hence throwing from express internals

@blakeembrey
Copy link
Member

To be clear, we are in agreement still that we want to throw a RangeError if someone attempts to set a status code outside of what Node expects?

SGTM, in favor of accepting only integers >= 100 and < 1000.

@wesleytodd
Copy link
Member

Totally 👍 to this decision. I think we could add a feature to enable "strict status codes" or something but that would be a minor addition and could land any time.

@aagamezl
Copy link

We could add a new boolean setting to the settings table, the name could be as @wesleytodd proposed strict status codes and when set to true, restricts valid status codes to the range 100 to 599 throwing a RangeError when the status code is not in the valid range.

I would like to implement this if everyone is okay with the new setting and behavior.

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

No branches or pull requests

5 participants