Throw on invalid status codes #4212

jonchurch · 2020-03-10T02:53:31Z

Closes #3143

Will throw a `RangeError` if status code:

is less than 100
is greater than 999

This aligns with Node.js' behavior of throwing if given something outside that range

Will throw a `TypeError` if status code is:

Not an integer (string representation of integer included)

This is a choice we are making to limit the acceptable input.

Use `res.status` internally when setting status codes

the PR also ensures we use res.status internally when setting status codes, to allow us to use the validation logic internally.

Test changes

I cleaned up the tests to test acceptable range, and invalid codes, and removed the iojs logic as its not supported in v5.

TODO:

Update the PR description to be specific to the actual changes in this PR, possibly reopen the PR since direction has changed
- Notably, this PR currently throws on strings, redefines the valid range of codes to between 1xx and 9xx, throws on non-integer floats (e.g. 500.5, but allows 500.00 bc it is the same to JS), throws a RangeError if we get a status code outside 1xx to 9xx range
Ensure the tests are accurate to these changes, and clean up the tests in here
Update the v5 docs to reflect said changes (separate PR to expressjs.com)

related: expressjs/discussions#233

test/res.status.js

jonchurch · 2020-03-10T03:24:25Z

I realized after opening this that Node.js does not throw on inputs like 500.5, this PR however does. From the other PR, I think we decided to throw on these cases, but I wanted to make clear that from my limited testing Node.js is not throwing on floats.

gireeshpunathil · 2020-03-10T03:27:32Z

I wanted to make clear that from my limited testing Node.js is not throwing on floats.

@jonchurch - my assertion is that 500.5 is definitely invalid, so throwing is the right thing to do.

dougwilson · 2020-03-10T03:29:29Z

Yea, I don't have any issues with this throwing on a float; we want to throw on whatever Node.js throws on as the minimum bar. If we can also help the users by also throwing on definitely nonsensical inputs (like status codes with fractions) that makes sense of course :) !

lib/response.js

gireeshpunathil · 2020-03-20T06:34:51Z

as per the TC discussion, this is ready to merge, who is going to do that? @dougwilson , I see your red-X on this - is that still valid, or you are going to remove it and land?

jonchurch · 2020-04-01T20:16:38Z

Thinking more about this and something bothers me. I took the approach the previous PR did, since it had been reviewed, but now I'm questioning the use of res.status internally to set statuses.

If someone monkey-patches res.status it will alter the internal behavior of setting status codes on responses. That's not different though for other functions used in response, like res.type for example.

My question has two parts:

Is it a breaking change to be relying on res.status() to set status codes internally?
Do we want to distinguish between private vs public methods? (there are only two things marked private in response according to jsdoc comments)

See an example of the change in the diff: https://github.com/expressjs/express/pull/4212/files#diff-d86a59ede7d999db4b7bc43cb25a1c11L137-R142

dougwilson · 2020-04-01T20:22:15Z

Is it a breaking change to be relying on res.status() to set status codes internally?

Yes, as stated in #4223 (comment) , but this PR is already a breaking change, right? So I'm not sure if it's super relevant. The change itself makes sense to make, just like we call res.type internally and not directly get the content-type response header. Even getting headers we internally use req.get and not req.headers.

Do we want to distinguish between private vs public methods? (there are only two things marked private in response according to jsdoc comments)

I'm not really following on what this part of the question really is. The main reason the internals use Express' own API is especially useful for AOP type of design patterns, if the user so chooses to do them. The Node.js HTTP APIs do the same patterns as well, AFAIK.

jonchurch · 2020-04-01T20:28:14Z

I wasn't clear. Re: breaking, I meant that someone's v4 res.status monkey patch might affect code in unexpected ways under v5, because it is used in more places than before.

You've answered my second question I believe. We aren't interested in making some methods private and off limits to users.

Thanks, I wanted to bring up this point (re: effects of monkey-patching res.status with these changes) just so someone else could check it.

Realizing that we use a lot of these helper methods internally would indicate that this change is not out of step with what is standard.

jonchurch · 2020-04-01T20:30:21Z

Just read that linked comment and saw you did directly address the concern already 👍

dougwilson · 2020-04-02T02:26:15Z

Yep! I did, though, not directly address the monkey patching messing something up; that is indeed the case, but I don't think any more so than other aspects of Node.js and Javascript. I think that it is going to be possible for users to override something and cause a breakage, but I'm not sure that the effort in order to prevent such a thing is really worth it. From support experience, I think it is extremely rare for such an issue to really show up, haha.

abenhamdine · 2023-03-10T10:09:56Z

I think this PR should target branch https://github.com/expressjs/express/tree/5.0 and not master.
And if it's ok, it should be merged aswell in v5 branch.

this is the behavior of node's underlying HTTP module

blakeembrey · 2024-05-04T03:28:49Z

lib/response.js

-    deprecate('res.status(' + JSON.stringify(code) + '): use res.status(' + Math.floor(code) + ') instead')
+  // Check if the status code is not an integer
+  if (!Number.isInteger(code)) {
+    throw new TypeError(`Invalid status code: ${code}. Status code must be an integer.`);


Should we continue JSON.stringify here so it's easier to debug a string vs number input?

good idea, will implement this

blakeembrey · 2024-05-04T03:29:20Z

lib/response.js

+};
+
+
+res.status = function status(code) {


Duplicate method?

ty, copy paste fail at 3am

blakeembrey · 2024-05-04T03:30:08Z

lib/response.js

@@ -847,7 +872,7 @@ res.redirect = function redirect(url) {
  });

  // Respond
-  this.statusCode = status;
+  this.status(status);


Any reason for these changes? Since it's internal, presumably we already trust and can use the fast path to just assign?

this part of the diff is lifted from the original work to land this in v4

i didn't really consider that we should drop it

this diff in v4 was largely bc we accept status as optional param (I think) in a few public methods so wanted to lock it down

will take a closer look, open to your suggestion

Sounds good, this case is reasonable. The other one is static but I don't have a strong opinion either way, go for it.

I agree when we can easily have the fast path we should take it. In this case I doubt it is very meaningfully different but longer term it would be great to have better benchmarks in place to track how changes like this could impact the bigger perf picture.

wesleytodd

Some of the comments are unaddressed, so didn't want to approve, but I think once those are addressed this looks good.

wesleytodd · 2024-05-15T17:29:38Z

lib/response.js

@@ -847,7 +872,7 @@ res.redirect = function redirect(url) {
  });

  // Respond
-  this.statusCode = status;
+  this.status(status);


I agree when we can easily have the fast path we should take it. In this case I doubt it is very meaningfully different but longer term it would be great to have better benchmarks in place to track how changes like this could impact the bigger perf picture.

jonchurch mentioned this pull request Mar 10, 2020

express 5.0.0 expressjs/discussions#100

Closed

dougwilson suggested changes Mar 10, 2020

View reviewed changes

test/res.status.js Outdated Show resolved Hide resolved

dougwilson added the pr label Mar 10, 2020

This comment was marked as outdated.

Sign in to view

LinusU reviewed Mar 10, 2020

View reviewed changes

lib/response.js Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

gireeshpunathil mentioned this pull request Mar 13, 2020

Express TC Meeting 03-18-2020 expressjs/discussions#109

Closed

jonchurch mentioned this pull request Mar 21, 2020

Deprecate non integer status codes in v4 #4223

Merged

dougwilson added 5.x enhancement labels Mar 25, 2020

jonchurch added this to the 5.0 milestone Apr 1, 2020

jonchurch mentioned this pull request Apr 19, 2020

Gracefully handle invalid status codes #3143

Closed

dougwilson mentioned this pull request May 23, 2021

Check for invalid or unknown http status codes. #4598

Closed

dougwilson force-pushed the master branch from c0e5dc1 to 6660649 Compare December 16, 2021 03:36

dougwilson force-pushed the master branch from 2295290 to a84e73b Compare February 2, 2022 06:42

dougwilson force-pushed the master branch from 04f6451 to 4ed35b4 Compare March 1, 2022 04:41

dougwilson force-pushed the master branch 2 times, most recently from eb10dba to 340be0f Compare October 6, 2022 14:16

wesleytodd mentioned this pull request Mar 21, 2024

Release 5.0 #2237

Open

39 tasks

UlisesGascon mentioned this pull request Apr 24, 2024

Express 5.0 - last push! expressjs/discussions#233

Open

33 tasks

jonchurch added 2 commits April 27, 2024 23:22

remove invalid float test

a2a1448

fixup! remove invalid float test

d0bd835

This comment was marked as outdated.

Sign in to view

fix invalid range tests error assertions

0670a3b

jonchurch marked this pull request as draft April 28, 2024 03:35

jonchurch added 2 commits April 27, 2024 23:35

remove unused deprecate function

8fc8c95

add test to assert on 200.00 coming through as 200

063a3bb

this is the behavior of node's underlying HTTP module

jonchurch mentioned this pull request Apr 29, 2024

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

Open

jonchurch added 9 commits May 3, 2024 20:25

revert back to throwing only on > 999 and < 100

a6e4e9b

update implementation for > 999

cca1438

add test for 700 status code

ce18152

update history with change

bade7a1

update jsdoc

eabe2cd

clarify jsdoc comment

1a13582

one more round of jsdoc

8374bdb

update 501 test

cee48e7

add invalid status code test for res.sendStatus

5130879

jonchurch requested review from a team, wesleytodd and blakeembrey May 4, 2024 01:05

This comment was marked as outdated.

Sign in to view

jonchurch added 5 commits May 3, 2024 21:34

add test describe block for valid range

63192c4

fixup! add test describe block for valid range

f7b7e35

reduce the describe nesting

86eb68c

switch to testing status 100, to avoid 100-continue behavior

6bc5136

fix 900 test

bbf58ba

jonchurch marked this pull request as ready for review May 4, 2024 02:11

blakeembrey reviewed May 4, 2024

View reviewed changes

jonchurch requested a review from a team May 8, 2024 22:16

wesleytodd reviewed May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw on invalid status codes #4212

Throw on invalid status codes #4212

jonchurch commented Mar 10, 2020 •

edited

jonchurch commented Mar 10, 2020

gireeshpunathil commented Mar 10, 2020

dougwilson commented Mar 10, 2020

This comment was marked as outdated.

This comment was marked as outdated.

gireeshpunathil commented Mar 20, 2020

jonchurch commented Apr 1, 2020 •

edited

dougwilson commented Apr 1, 2020

jonchurch commented Apr 1, 2020

jonchurch commented Apr 1, 2020

dougwilson commented Apr 2, 2020

abenhamdine commented Mar 10, 2023

This comment was marked as outdated.

This comment was marked as outdated.

blakeembrey May 4, 2024

jonchurch May 8, 2024

blakeembrey May 4, 2024

jonchurch May 4, 2024

blakeembrey May 4, 2024

jonchurch May 4, 2024 •

edited

blakeembrey May 8, 2024

wesleytodd May 15, 2024

wesleytodd left a comment

wesleytodd May 15, 2024

Throw on invalid status codes #4212

Are you sure you want to change the base?

Throw on invalid status codes #4212

Conversation

jonchurch commented Mar 10, 2020 • edited

Will throw a RangeError if status code:

Will throw a TypeError if status code is:

Use res.status internally when setting status codes

Test changes

TODO:

jonchurch commented Mar 10, 2020

gireeshpunathil commented Mar 10, 2020

dougwilson commented Mar 10, 2020

This comment was marked as outdated.

This comment was marked as outdated.

gireeshpunathil commented Mar 20, 2020

jonchurch commented Apr 1, 2020 • edited

dougwilson commented Apr 1, 2020

jonchurch commented Apr 1, 2020

jonchurch commented Apr 1, 2020

dougwilson commented Apr 2, 2020

abenhamdine commented Mar 10, 2023

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonchurch May 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleytodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonchurch commented Mar 10, 2020 •

edited

Will throw a `RangeError` if status code:

Will throw a `TypeError` if status code is:

Use `res.status` internally when setting status codes

jonchurch commented Apr 1, 2020 •

edited

jonchurch May 4, 2024 •

edited