http: improve errors thrown in header validation #16715

joyeecheung · 2017-11-03T13:05:08Z

Fixes: #16714

This PR:

Replace ERR_HTTP2_INVALID_HEADER_VALUE with ERR_HTTP_INVALID_HEADER_VALUE, don't really see a need to differentiate the two.
- The error code documentation of ERR_HTTP2_INVALID_HEADER_VALUE is kept.
- Include the header name and the value for debug-ability as [REQUEST] Better Errors in http validate headers #16714 suggests. Before it's Value must not be undefined or null(HTTP2) or The "value" argument must be specified(HTTP), after it's Invalid value "undefined" for header "test". If the user is setting the header in a loop this would be much more useful.
Use Error.captureStackTrace to exclude the validation functions from the stack trace

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http, http2

joyeecheung · 2017-11-03T13:15:33Z

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

joyeecheung · 2017-11-03T13:21:57Z

Would be nice to have common.expectsError checking excluded stack frames and printing the original message when the error classes are called without new. Will open a seperate PR for that.

apapirovski

LGTM with some nits 👍

apapirovski · 2017-11-03T13:29:46Z

doc/api/errors.md

-### ERR_HTTP2_INVALID_HEADER_VALUE
+### ERR_HTTP_INVALID_HEADER_VALUE
+<a id="ERR_HTTP_INVALID_HEADER_VALUE"></a>
+### ERR_HTTP_INVALID_HEADER_VALUE


Accidentally doubled here?

apapirovski · 2017-11-03T13:32:43Z

lib/internal/http2/compat.js

-    throw new errors.Error('ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED');
-  if (value === undefined || value === null)
-    throw new errors.TypeError('ERR_HTTP2_INVALID_HEADER_VALUE');
+  var err;


Nit: can we use let? (I don't think there's many/any vars in http2 except for for loops.)

I am still under the impression that we favor var in cases like this...might be just outdated impressions, I'll fix that.

apapirovski · 2017-11-03T13:33:47Z

test/parallel/test-http2-compat-serverresponse-trailers.js

    common.expectsError(
      () => response.setTrailer('test', undefined),
      {
-        code: 'ERR_HTTP2_INVALID_HEADER_VALUE',


I'm not 100% sure about this because the semantics of the http2 validation are slightly different, for example pseudo headers are an http2 only feature.

apapirovski · 2017-11-03T13:36:44Z

lib/_http_outgoing.js

-    throw new errors.TypeError('ERR_INVALID_CHAR', 'header content', name);
+    err = new errors.TypeError('ERR_INVALID_CHAR', 'header content', name);
+  }
+  if (err) {


Since this is an object, can we do a strict comparison to undefined? (This function is kind of in a hot path.)

apapirovski · 2017-11-03T13:37:10Z

lib/internal/http2/compat.js

+  } else if (value === undefined || value === null) {
+    err = new errors.TypeError('ERR_HTTP_INVALID_HEADER_VALUE', value, name);
+  }
+  if (err) {


Same as for http, could we compare to undefined to avoid the whole document.all perf issue? (This is in a hot path.)

maclover7

+1 to landing as semver-major

maclover7 · 2017-11-03T13:38:06Z

doc/api/errors.md


 <a id="ERR_HTTP2_INVALID_HEADER_VALUE"></a>
-### ERR_HTTP2_INVALID_HEADER_VALUE
+### ERR_HTTP_INVALID_HEADER_VALUE


This line is duplicated

Actually it's accidentally changed..we need to keep the documentation for ERR_HTTP2_INVALID_HEADER_VALUE because it's out there already.

If the codebase doesn't raise an error with this code though, should it be removed? 😬

maclover7 · 2017-11-03T13:38:25Z

doc/api/errors.md

+### ERR_HTTP_INVALID_HEADER_VALUE

-Used to indicate that an invalid HTTP/2 header value has been specified.
+Used to indicate that an invalid HTTP header value has been specified.


Should we explicitly say HTTP/1.1 and HTTP/2?

apapirovski · 2017-11-03T13:39:45Z

Is this semver-major? It doesn't modify error codes except for http2 which is experimental. (And I'm not even 100% confident the error code for h2 should change, see above.)

joyeecheung · 2017-11-03T13:42:29Z

@apapirovski Hmmm, actually I don't know what's our policy for "changing error code but it's an error in the experimental module"..

joyeecheung · 2017-11-03T13:43:27Z

@apapirovski Ah, wait, it also modifies the HTTP error codes (ERR_MISSING_ARGS -> ERR_HTTP_INVALID_HEADER_VALUE)

apapirovski · 2017-11-03T13:43:35Z

@joyeecheung They already changed roughly a million times during v8.x... that ship might've sailed. 😆

Ah, wait, it also modifies the HTTP error codes (ERR_MISSING_ARGS -> ERR_HTTP_INVALID_HEADER_VALUE)

Ah true. Wonder if we could split into two separate PRs/commits so most of this could land right away?

joyeecheung · 2017-11-03T14:51:18Z

@apapirovski

Ah true. Wonder if we could split into two separate PRs/commits so most of this could land right away?

Yeah that seems like a better idea. I will close this one and open another two.

http: improve errors thrown in header validation

a4c2e6e

joyeecheung force-pushed the improve-http-err branch from b09ead1 to a4c2e6e Compare November 3, 2017 13:05

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 3, 2017

apapirovski approved these changes Nov 3, 2017

View reviewed changes

maclover7 reviewed Nov 3, 2017

View reviewed changes

joyeecheung closed this Nov 3, 2017

This was referenced Nov 3, 2017

http2: improve errors thrown in header validation #16718

Closed

http: improve errors thrown in header validation #16719

Closed

Uh oh!

http: improve errors thrown in header validation #16715

http: improve errors thrown in header validation #16715

Uh oh!

Conversation

joyeecheung commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung commented Nov 3, 2017

Uh oh!

joyeecheung commented Nov 3, 2017

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Nov 3, 2017

Uh oh!

joyeecheung commented Nov 3, 2017

Uh oh!

apapirovski commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Nov 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joyeecheung commented Nov 3, 2017 •

edited

Loading

apapirovski commented Nov 3, 2017 •

edited

Loading

apapirovski commented Nov 3, 2017 •

edited

Loading