-
-
Notifications
You must be signed in to change notification settings - Fork 16.7k
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
Comments
The specs are clear on this, I am +1 |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@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. But that's not a huge benefit really, compared to the reduction in flexibility. I see express as an HTTP framework, and If folks complain, we could add |
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. |
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 🙂 |
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 That was the original feature in #4212 , Node throws if given |
SGTM, in favor of accepting only integers |
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. |
We could add a new boolean setting to the settings table, the name could be as @wesleytodd proposed I would like to implement this if everyone is okay with the new setting and behavior. |
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.
The text was updated successfully, but these errors were encountered: