http: more strict statusCode validation #9031

nfriedly · 2016-10-11T18:03:04Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

HTTP

Description of change

This removes the http statusCode coercion, and instead performs more strict validation on the original value, either using it unchanged or throwing.

Of note, some values that were previously accepted no longer are:

Any otherwise-valid string: '200' previously was accepted, it throws a TypeError with this change
Numbers with decimals: 200.6 was previously accepted (and silently coerced to 200), it throws a RangeError with this change.

All non-number values will now get a TypeError instead of a RangeError.

Fixes #9027

mscdex · 2016-10-11T18:37:46Z

test/common.js

@@ -402,8 +402,10 @@ function runCallChecks(exitCode) {
 }


-exports.mustCall = function(fn, expected) {
-  if (typeof expected !== 'number') expected = 1;
+exports.mustCall = function(fn, expected = 1) {


The changes in this file should be in a separate PR, as they are unrelated.

I can move this to it's own PR, and started to in #9035 - but the problem is that the tests then fail without the other changes from that commit. And, then the second commit isn't actually tested without those changes.

So, I can break this up if you really want, but I don't think it makes sense to in this case.

It would really be nice to have the common.mustCall change in a separate PR (including the fixes to tests that don't pass a number). Then when it lands you can rebase this one.

Ok, that makes sense. Done in #10692

Can you also remove the changes from this PR.

Kind of, although it isn't actually tested unless I at least keep the fixes in test-http-response-statuscode.js. I was thinking I would clean this one up after that one lands like @targos said, so that wouldn't be an issue then.

#10692 is now merged in; I'll rebase this branch soon (probably tomorrow).

Updates expected to use modern default syntax and also validate the value and throw an error rather than silently overwriting invalid arguments. Would prevent issues such as the one fixed in the first commit on nodejs#9031

nfriedly · 2016-10-11T21:07:16Z

Sorry about all of the references here on github - it took me a couple of tries to get things split up in response to @mscdex's comments, and then a couple more to put them back once I realized that it wouldn't work because the tests wouldn't pass :/

jasnell · 2017-01-09T04:54:41Z

lib/_http_server.js

@@ -189,7 +189,7 @@ ServerResponse.prototype.writeHead = function(statusCode, reason, obj) {

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


Why is this change needed?

It will print the actual value before it is converted to an integer

I'd prefer not. The test that leads to this error is being performed on the coerced value. I would rather display the coerced value than the actual value.

Speaking from experience, that's confusing to an end user, because it ends up as 0 for most invalid input. (In my case, someone had been setting error.code to a custom status code, but then an ENOENT error caused this to throw "RangeError: Invalid status code: 0", and I spent a good part of a day tracking it down because I was looking for the wrong thing.)

However, you do have a valid point, so what about a couple of alternatives - either include both the coerced value and the original value in the message, or else have a type check earlier that throws on non-Number input with the original value & type?

I'm not going to object to the change being made, I just don't prefer it.

nfriedly · 2017-01-16T23:04:44Z

I just rebased this against master, and ended up making some changes. I couldn't just use this.statusCode anymore because the range check is now the first thing that happens in writeHead(). So, instead, I just made the validation more strict so that statusCode is never modified.

This leads to two changes:

String input is not accepted anymore: '200' previously worked but now throws. ~~This could lead to a pretty confusing error message, so I think I may need to put some more work in here.~~ Fixed - I added a type check first.
Numbers with decimals are not accepted anymore - before, 200.6 was silently floored to 200, now it throws.

HTTP ServerResponse.writeHead previously coerced the given statusCode to an integer to validate it, then threw an error with the converted number if it was invalid. This meant that non-numeric input led to confusing error messages. This change makes the input validation more strict, throwing on any non-integer input rather than silently coercing it. Of note: * Strings (e.g. '200') were previously accepted, now they cause a TypeError * Numbers with decimals (e.g. 200.6) were previously floored, now they cause a RangeError (with the original value included in the error message). Fixes nodejs#9027

jasnell · 2017-03-01T02:21:45Z

@nfriedly ... is this still something you want to pursue

@nodejs/http

cjihrig · 2017-03-01T03:13:27Z

Closing per #9027 (comment)

nfriedly · 2017-03-01T03:13:33Z

@jasnell It looks like a4bb9fd fixed the original issue, so I'm content with closing this PR and calling it good.

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 11, 2016

mscdex reviewed Oct 11, 2016

View reviewed changes

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 11, 2016

nfriedly force-pushed the issue-9027 branch from 46429be to 9207c46 Compare October 11, 2016 19:48

nfriedly mentioned this pull request Oct 11, 2016

validate arguments to mustCall(), default value #9035

Closed

3 tasks

nfriedly force-pushed the issue-9027 branch 3 times, most recently from 0910731 to 10702ba Compare October 11, 2016 20:28

nfriedly force-pushed the issue-9027 branch 2 times, most recently from 2047e28 to 7cf406e Compare October 11, 2016 21:05

jasnell changed the title ~~Fix error message for invalid httm status code, fix tests.~~ Fix error message for invalid http status code, fix tests. Oct 11, 2016

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

targos added this to the 8.0.0 milestone Jan 8, 2017

nfriedly mentioned this pull request Jan 8, 2017

test: validate arguments to mustCall(), fix incorrect tests #10692

Closed

3 tasks

jasnell reviewed Jan 9, 2017

View reviewed changes

nfriedly force-pushed the issue-9027 branch from 7cf406e to 714221b Compare January 11, 2017 12:56

nodejs-github-bot added the dont-land-on-v7.x label Jan 11, 2017

nfriedly changed the title ~~Fix error message for invalid http status code, fix tests.~~ Fix error message for invalid http status code Jan 11, 2017

nfriedly force-pushed the issue-9027 branch from 714221b to 55cf0c6 Compare January 16, 2017 22:57

nodejs-github-bot added the dont-land-on-v7.x label Jan 16, 2017

nfriedly force-pushed the issue-9027 branch from 55cf0c6 to 4cb5ac7 Compare January 17, 2017 00:13

nodejs-github-bot added the dont-land-on-v7.x label Jan 17, 2017

nfriedly force-pushed the issue-9027 branch from 4cb5ac7 to 407f3ed Compare January 17, 2017 00:18

nodejs-github-bot added the dont-land-on-v7.x label Jan 17, 2017

nfriedly changed the title ~~Fix error message for invalid http status code~~ http: more strict statusCode validation Jan 17, 2017

targos removed the dont-land-on-v7.x label Jan 28, 2017

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

cjihrig closed this Mar 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: more strict statusCode validation #9031

http: more strict statusCode validation #9031

nfriedly commented Oct 11, 2016 •

edited

Loading

mscdex Oct 11, 2016

nfriedly Oct 11, 2016

targos Jan 8, 2017

nfriedly Jan 8, 2017

cjihrig Jan 8, 2017

nfriedly Jan 8, 2017

nfriedly Jan 10, 2017

nfriedly commented Oct 11, 2016

jasnell Jan 9, 2017

targos Jan 9, 2017

jasnell Jan 10, 2017

nfriedly Jan 11, 2017 •

edited

Loading

jasnell Jan 11, 2017

nfriedly commented Jan 16, 2017 •

edited

Loading

jasnell commented Mar 1, 2017

cjihrig commented Mar 1, 2017

nfriedly commented Mar 1, 2017

http: more strict statusCode validation #9031

http: more strict statusCode validation #9031

Conversation

nfriedly commented Oct 11, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfriedly commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfriedly Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfriedly commented Jan 16, 2017 • edited Loading

jasnell commented Mar 1, 2017

cjihrig commented Mar 1, 2017

nfriedly commented Mar 1, 2017

nfriedly commented Oct 11, 2016 •

edited

Loading

nfriedly Jan 11, 2017 •

edited

Loading

nfriedly commented Jan 16, 2017 •

edited

Loading