Skip to content
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.

Sign up for GitHub

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

http: improve errors thrown in header validation #16715

Closed
wants to merge 1 commit into from
Closed

http: improve errors thrown in header validation #16715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,9 +818,11 @@ HTTP/1 connection specific headers are forbidden to be used in HTTP/2
requests and responses.

<a id="ERR_HTTP2_INVALID_HEADER_VALUE"></a>
### ERR_HTTP2_INVALID_HEADER_VALUE
### ERR_HTTP_INVALID_HEADER_VALUE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is duplicated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<a id="ERR_HTTP_INVALID_HEADER_VALUE"></a>
### ERR_HTTP_INVALID_HEADER_VALUE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally doubled here?


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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


<a id="ERR_HTTP2_INVALID_INFO_STATUS"></a>
### ERR_HTTP2_INVALID_INFO_STATUS
Expand Down
38 changes: 18 additions & 20 deletions lib/_http_outgoing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,16 +437,7 @@ function _storeHeader(firstLine, headers) {

function storeHeader(self, state, key, value, validate) {
if (validate) {
if (typeof key !== 'string' || !key || !checkIsHttpToken(key)) {
throw new errors.TypeError(
'ERR_INVALID_HTTP_TOKEN', 'Header name', key);
}
if (value === undefined) {
throw new errors.TypeError('ERR_MISSING_ARGS', `header "${key}"`);
} else if (checkInvalidHeaderChar(value)) {
debug('Header "%s" contains invalid characters', key);
throw new errors.TypeError('ERR_INVALID_CHAR', 'header content', key);
}
validateHeader(key, value);
}
state.header += key + ': ' + escapeHeaderValue(value) + CRLF;
matchHeader(self, state, key, value);
Expand Down Expand Up @@ -494,20 +485,27 @@ function matchHeader(self, state, field, value) {
}
}

function validateHeader(msg, name, value) {
if (typeof name !== 'string' || !name || !checkIsHttpToken(name))
throw new errors.TypeError('ERR_INVALID_HTTP_TOKEN', 'Header name', name);
if (value === undefined)
throw new errors.TypeError('ERR_MISSING_ARGS', 'value');
if (msg._header)
throw new errors.Error('ERR_HTTP_HEADERS_SENT', 'set');
if (checkInvalidHeaderChar(value)) {
function validateHeader(name, value) {
var err;
if (typeof name !== 'string' || !name || !checkIsHttpToken(name)) {
err = new errors.TypeError('ERR_INVALID_HTTP_TOKEN', 'Header name', name);
} else if (value === undefined) {
err = new errors.TypeError('ERR_HTTP_INVALID_HEADER_VALUE', value, name);
} else if (checkInvalidHeaderChar(value)) {
debug('Header "%s" contains invalid characters', name);
throw new errors.TypeError('ERR_INVALID_CHAR', 'header content', name);
err = new errors.TypeError('ERR_INVALID_CHAR', 'header content', name);
}
if (err) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Error.captureStackTrace(err, validateHeader);
throw err;
}
}

OutgoingMessage.prototype.setHeader = function setHeader(name, value) {
validateHeader(this, name, value);
if (this._header) {
throw new errors.Error('ERR_HTTP_HEADERS_SENT', 'set');
}
validateHeader(name, value);

if (!this[outHeadersKey])
this[outHeadersKey] = {};
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,6 @@ E('ERR_HTTP2_INFO_STATUS_NOT_ALLOWED',
'Informational status codes cannot be used');
E('ERR_HTTP2_INVALID_CONNECTION_HEADERS',
'HTTP/1 Connection specific headers are forbidden: "%s"');
E('ERR_HTTP2_INVALID_HEADER_VALUE', 'Value must not be undefined or null');
E('ERR_HTTP2_INVALID_INFO_STATUS',
(code) => `Invalid informational status code: ${code}`);
E('ERR_HTTP2_INVALID_PACKED_SETTINGS_LENGTH',
Expand Down Expand Up @@ -241,6 +240,7 @@ E('ERR_HTTP2_UNSUPPORTED_PROTOCOL',
E('ERR_HTTP_HEADERS_SENT',
'Cannot %s headers after they are sent to the client');
E('ERR_HTTP_INVALID_CHAR', 'Invalid character in statusMessage.');
E('ERR_HTTP_INVALID_HEADER_VALUE', 'Invalid value "%s" for header "%s"');
E('ERR_HTTP_INVALID_STATUS_CODE',
(originalStatusCode) => `Invalid status code: ${originalStatusCode}`);
E('ERR_HTTP_TRAILER_INVALID',
Expand Down
18 changes: 12 additions & 6 deletions lib/internal/http2/compat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,18 @@ let statusMessageWarned = false;
// close as possible to the current require('http') API

function assertValidHeader(name, value) {
if (name === '' || typeof name !== 'string')
throw new errors.TypeError('ERR_INVALID_HTTP_TOKEN', 'Header name', name);
if (isPseudoHeader(name))
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (name === '' || typeof name !== 'string') {
err = new errors.TypeError('ERR_INVALID_HTTP_TOKEN', 'Header name', name);
} else if (isPseudoHeader(name)) {
err = new errors.Error('ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED');
} else if (value === undefined || value === null) {
err = new errors.TypeError('ERR_HTTP_INVALID_HEADER_VALUE', value, name);
}
if (err) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Error.captureStackTrace(err, assertValidHeader);
throw err;
}
}

function isPseudoHeader(name) {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http-mutable-headers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ const s = http.createServer(common.mustCall((req, res) => {
common.expectsError(
() => res.setHeader('someHeader'),
{
code: 'ERR_MISSING_ARGS',
code: 'ERR_HTTP_INVALID_HEADER_VALUE',
type: TypeError,
message: 'The "value" argument must be specified'
message: 'Invalid value "undefined" for header "someHeader"'
}
);
common.expectsError(
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http-outgoing-proto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ assert.throws(() => {
const outgoingMessage = new OutgoingMessage();
outgoingMessage.setHeader('test');
}, common.expectsError({
code: 'ERR_MISSING_ARGS',
code: 'ERR_HTTP_INVALID_HEADER_VALUE',
type: TypeError,
message: 'The "value" argument must be specified'
message: 'Invalid value "undefined" for header "test"'
}));

assert.throws(() => {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-http-write-head.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ const s = http.createServer(common.mustCall((req, res) => {
common.expectsError(
() => res.setHeader('foo', undefined),
{
code: 'ERR_MISSING_ARGS',
code: 'ERR_HTTP_INVALID_HEADER_VALUE',
type: TypeError,
message: 'The "value" argument must be specified'
message: 'Invalid value "undefined" for header "foo"'
}
);

Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-http2-compat-serverresponse-headers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,16 @@ server.listen(0, common.mustCall(function() {
assert.throws(function() {
response.setHeader(real, null);
}, common.expectsError({
code: 'ERR_HTTP2_INVALID_HEADER_VALUE',
code: 'ERR_HTTP_INVALID_HEADER_VALUE',
type: TypeError,
message: 'Value must not be undefined or null'
message: 'Invalid value "null" for header "foo-bar"'
}));
assert.throws(function() {
response.setHeader(real, undefined);
}, common.expectsError({
code: 'ERR_HTTP2_INVALID_HEADER_VALUE',
code: 'ERR_HTTP_INVALID_HEADER_VALUE',
type: TypeError,
message: 'Value must not be undefined or null'
message: 'Invalid value "undefined" for header "foo-bar"'
}));
common.expectsError(
() => response.setHeader(), // header name undefined
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-http2-compat-serverresponse-trailers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,17 @@ server.listen(0, common.mustCall(() => {
common.expectsError(
() => response.setTrailer('test', undefined),
{
code: 'ERR_HTTP2_INVALID_HEADER_VALUE',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

code: 'ERR_HTTP_INVALID_HEADER_VALUE',
type: TypeError,
message: 'Value must not be undefined or null'
message: 'Invalid value "undefined" for header "test"'
}
);
common.expectsError(
() => response.setTrailer('test', null),
{
code: 'ERR_HTTP2_INVALID_HEADER_VALUE',
code: 'ERR_HTTP_INVALID_HEADER_VALUE',
type: TypeError,
message: 'Value must not be undefined or null'
message: 'Invalid value "null" for header "test"'
}
);
common.expectsError(
Expand Down