http: disallow sending obviously invalid status codes #6291

mscdex · 2016-04-20T00:51:59Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

http

Description of change

Disallows sending of invalid status codes.

mscdex · 2016-04-20T00:54:43Z

/cc @nodejs/http

CI: https://ci.nodejs.org/job/node-test-pull-request/2333/

jasnell · 2016-04-20T01:00:29Z

This should likely also catch anything less than 100 and greater than 599

mscdex · 2016-04-20T01:01:58Z

@jasnell True, but I was aiming to be as backwards compatible as possible with this. Who knows if anyone out there is using custom/unofficial status codes.

mscdex · 2016-04-20T01:04:23Z

Tagging this for possible backporting to v4.x, feel free to remove if you think it shouldn't be. I see this as a bug fix though.

jasnell · 2016-04-20T01:05:37Z

Anyone using anything less than 100 likely has other problems to deal with
as there are parsers that will die completely if there aren't three digits.
It's conceivable that someone is using numbers higher than 599. Perhaps
catch anything that's not 3-digits? 100 <= n <= 999?
On Apr 19, 2016 6:02 PM, "Brian White" notifications@github.com wrote:

@jasnell https://github.com/jasnell True, but I was aiming to be as
backwards compatible as possible with this. Who knows if anyone out there
is using custom/unofficial status codes.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6291 (comment)

mscdex · 2016-04-20T01:08:46Z

If we're going to validate the numerical value, I would lean more towards a 3-digit check.

What do other @nodejs/collaborators think?

indutny · 2016-04-20T01:17:03Z

What's the reason for introduction of this patch at this point? Is there any open issue about it?

mscdex · 2016-04-20T01:29:54Z

@indutny https://twitter.com/bengl/status/722571972790476800

indutny · 2016-04-20T01:33:21Z

Alright, thank you!

thefourtheye · 2016-04-20T02:12:07Z

lib/_http_server.js

@@ -187,6 +187,10 @@ ServerResponse.prototype.writeHead = function(statusCode, reason, obj) {
    headers = obj;
  }

+  const statusCode_ = +statusCode;
+  if (statusCode_ < 0 || !isFinite(statusCode_))


Why not Number.isInteger?

Number.isInteger() would return false for strings of valid numbers.

mscdex · 2016-04-20T02:39:57Z

Ok, I've modified it to check the number of digits now.

~~CI: https://ci.nodejs.org/job/node-test-pull-request/2335/~~
New CI: https://ci.nodejs.org/job/node-test-pull-request/2336/

jasnell · 2016-04-20T02:43:22Z

LGTM if CI is green

thefourtheye · 2016-04-20T02:45:10Z

I am okay with this. LGTM. semver major?

mscdex · 2016-04-20T02:46:59Z

@thefourtheye Possibly, but it is a bug fix shrug.

thefourtheye · 2016-04-20T02:47:40Z

lib/_http_server.js

@@ -187,6 +187,10 @@ ServerResponse.prototype.writeHead = function(statusCode, reason, obj) {
    headers = obj;
  }

+  statusCode |= 0;
+  if (statusCode < 100 || statusCode > 999)


Nit: I am not sure if this really matters but the expected port range is just 100-599 http://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml

Yes, @jasnell mentioned this already. However I think it might be better to be a bit more lenient in case of custom/unofficial/whatever status codes out in the wild.

Oh, sorry. I didn't read the comments completely.

jasnell · 2016-04-20T03:04:01Z

I think if we keep it at any three digit code we can easily justify landing
it as a patch.
On Apr 19, 2016 7:47 PM, "Brian White" notifications@github.com wrote:

@thefourtheye https://github.com/thefourtheye Probably, but it is a
bug fix shrug.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6291 (comment)

ChALkeR · 2016-04-20T03:31:24Z

LGTM

bengl · 2016-04-20T04:27:42Z

test/parallel/test-http-response-statuscode.js

+      break;
+    case 5:
+      assert.throws(common.mustCall(() => {
+        res.writeHead(1000);


Perhaps also have a case for a non-numeric string?

+1 ... null, 'this is not invalid, [], true, etc could all be included in the tests here, just to be overly careful ;-)

Suggested tests added.

indutny · 2016-04-20T04:34:50Z

LGTM

benjamingr · 2016-04-20T07:01:48Z

LGTM, I like this change.

cjihrig · 2016-04-20T13:47:15Z

lib/_http_server.js

@@ -187,6 +187,10 @@ ServerResponse.prototype.writeHead = function(statusCode, reason, obj) {
    headers = obj;
  }

+  statusCode |= 0;
+  if (statusCode < 100 || statusCode > 999)
+    throw new Error(`Invalid status code: ${statusCode}`);


Should this be a RangeError?

Should passing 'NotAStatusCode' cause a RangeError?

@ChALkeR It's debatable, but since statusCode is ultimately coerced to an integer value (e.g. 0 for non-numeric values) I thought maybe RangeError would fit better since 0 < 100. shrug

Ok, that works for me =).

cjihrig · 2016-04-20T13:47:53Z

LGTM with a comment.

jasnell · 2016-04-20T15:23:31Z

Just one additional minor comment on the tests. Otherwise still LGTM

PR-URL: #6291 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: nodejs#6291 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #6291 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-05-18T00:04:20Z

I'm adding this to v4.x-staging and to the v4.4.5 release candidate.

if anyone objects or if we see any weird regressions I will back it out

PR-URL: #6291 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * deps: * update npm to 2.15.5 (Rebecca Turner) #6663 * update ESLint to 2.7.0 (silverwind) #6132 * http: * Invalid status codes can no longer be sent. Limited to 3 digit numbers between 100 - 999 (Brian White) #6291

Notable changes: * **buffer**: * Buffer no longer errors if you call lastIndexOf with a search term longer than the buffer (Anna Henningsen) #6511 * deps: * update npm to 2.15.5 (Rebecca Turner) #6663 * http: * Invalid status codes can no longer be sent. Limited to 3 digit numbers between 100 - 999 (Brian White) #6291

Notable changes: * **buffer**: * Buffer no longer errors if you call lastIndexOf with a search term longer than the buffer (Anna Henningsen) #6511 * contextify: * Context objects are now properly garbage collected, this solves a problem some individuals were experiencing with extreme memory growth (Ali Ijaz Sheikh) #6871 * deps: * update npm to 2.15.5 (Rebecca Turner) #6663 * http: * Invalid status codes can no longer be sent. Limited to 3 digit numbers between 100 - 999 (Brian White) #6291

rmg · 2016-05-24T19:53:28Z

Code that would previously run and happily dish out invalid status codes will now die from unhandled exceptions. Isn't that sort of the definition of a breaking change? Upgrading from v4.4.4 to v4.4.5 shouldn't require people to modify their code.

rmg · 2016-05-24T20:08:49Z

It's too late now, and that I have the advantage of hindsight...

If this PR was 2 commits, with the first one adding the (semver-patch) statusCode |= 0 line and the second adding the (semver-major) strict range check, then we could have pulled in only part of the change for LTS.

Fishrock123 · 2016-05-25T16:13:15Z

I was also quite surprised to see this backported.

nfriedly · 2016-10-11T17:03:09Z

test/parallel/test-http-response-statuscode.js

+    case 0:
+      assert.throws(common.mustCall(() => {
+        res.writeHead(-1);
+      }, /invalid status code/i));


The closing parenthesis for common.mustCall() is misplaced here and on the other lines. It should read:

}), /invalid status code/i);

With the current code, the RegExp is passed to mustCall as the expected paramater, and then silently replaced with 1 because it's non-numeric. Then the assert.throws call doesn't validate the error message and passes the test as long as any error is thrown.

I'll fix this along with my patch for #9027 unless you'd prefer to see it in a separate PR.

aivus · 2016-11-17T09:49:54Z

Thank you, guys, we got broken app after patch-upgrade👏

FYI, http://semver.org/

PATCH version when you make backwards-compatible bug fixes.

mscdex added the http Issues or PRs related to the http subsystem. label Apr 20, 2016

mscdex force-pushed the http-disallow-bad-statuscode branch from 8406b4d to 3e01bad Compare April 20, 2016 00:58

mscdex added the lts-watch-v4.x label Apr 20, 2016

thefourtheye reviewed Apr 20, 2016
View reviewed changes

mscdex force-pushed the http-disallow-bad-statuscode branch from 3e01bad to f9063c7 Compare April 20, 2016 02:35

mscdex force-pushed the http-disallow-bad-statuscode branch from f9063c7 to dcda312 Compare April 20, 2016 02:42

thefourtheye reviewed Apr 20, 2016
View reviewed changes

bengl reviewed Apr 20, 2016
View reviewed changes

cjihrig reviewed Apr 20, 2016
View reviewed changes

mscdex force-pushed the http-disallow-bad-statuscode branch from dcda312 to ec50170 Compare April 20, 2016 13:51

mscdex force-pushed the http-disallow-bad-statuscode branch from ec50170 to 35b4f99 Compare April 20, 2016 15:37

jasnell closed this Apr 20, 2016

mscdex deleted the http-disallow-bad-statuscode branch April 21, 2016 00:33

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels May 18, 2016

MylesBorins mentioned this pull request May 18, 2016

v4.4.5 proposal #6824

Merged

kanongil mentioned this pull request May 24, 2016

Handle thrown error from res.setHeader() and res.writeHead() hapijs/hapi#3178

Merged

nfriedly reviewed Oct 11, 2016

View reviewed changes

Rob--W mentioned this pull request Nov 8, 2017

RangeError: Invalid status code: 0 http-party/node-http-proxy#1080

Closed

http: disallow sending obviously invalid status codes #6291

http: disallow sending obviously invalid status codes #6291

Conversation

mscdex commented Apr 20, 2016

Checklist

Affected core subsystem(s)

Description of change

mscdex commented Apr 20, 2016

jasnell commented Apr 20, 2016

mscdex commented Apr 20, 2016

mscdex commented Apr 20, 2016 • edited Loading

jasnell commented Apr 20, 2016

mscdex commented Apr 20, 2016

indutny commented Apr 20, 2016

mscdex commented Apr 20, 2016

indutny commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Apr 20, 2016 • edited Loading

jasnell commented Apr 20, 2016

thefourtheye commented Apr 20, 2016

mscdex commented Apr 20, 2016 • edited Loading

Choose a reason for hiding this comment

mscdex Apr 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 20, 2016

ChALkeR commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Apr 20, 2016

benjamingr commented Apr 20, 2016

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

cjihrig commented Apr 20, 2016

jasnell commented Apr 20, 2016

MylesBorins commented May 18, 2016

rmg commented May 24, 2016

rmg commented May 24, 2016

Fishrock123 commented May 25, 2016

Choose a reason for hiding this comment

aivus commented Nov 17, 2016 • edited Loading

mscdex commented Apr 20, 2016 •

edited

Loading

mscdex commented Apr 20, 2016 •

edited

Loading

mscdex commented Apr 20, 2016 •

edited

Loading

mscdex Apr 20, 2016 •

edited

Loading

aivus commented Nov 17, 2016 •

edited

Loading