http2: throw from mapToHeaders

PR-URL: https://github.com/nodejs/node/pull/24063
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Note: Landed with one collaborator approval after PR
      was open for 18 days
This commit is contained in:
James M Snell 2018-11-03 10:27:18 -07:00
parent 9379ab3786
commit e94d16daf2
No known key found for this signature in database
GPG Key ID: 7341B15C070877AC
4 changed files with 42 additions and 56 deletions

View File

@ -1484,8 +1484,6 @@ class ClientHttp2Session extends Http2Session {
} }
const headersList = mapToHeaders(headers); const headersList = mapToHeaders(headers);
if (!Array.isArray(headersList))
throw headersList;
const stream = new ClientHttp2Stream(this, undefined, undefined, {}); const stream = new ClientHttp2Stream(this, undefined, undefined, {});
stream[kSentHeaders] = headers; stream[kSentHeaders] = headers;
@ -1890,8 +1888,6 @@ class Http2Stream extends Duplex {
this[kUpdateTimer](); this[kUpdateTimer]();
const headersList = mapToHeaders(headers, assertValidPseudoHeaderTrailer); const headersList = mapToHeaders(headers, assertValidPseudoHeaderTrailer);
if (!Array.isArray(headersList))
throw headersList;
this[kSentTrailers] = headers; this[kSentTrailers] = headers;
// Send the trailers in setImmediate so we don't do it on nghttp2 stack. // Send the trailers in setImmediate so we don't do it on nghttp2 stack.
@ -2058,12 +2054,14 @@ function processRespondWithFD(self, fd, headers, offset = 0, length = -1,
const state = self[kState]; const state = self[kState];
state.flags |= STREAM_FLAGS_HEADERS_SENT; state.flags |= STREAM_FLAGS_HEADERS_SENT;
const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); let headersList;
self[kSentHeaders] = headers; try {
if (!Array.isArray(headersList)) { headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse);
self.destroy(headersList); } catch (err) {
self.destroy(err);
return; return;
} }
self[kSentHeaders] = headers;
// Close the writable side of the stream, but only as far as the writable // Close the writable side of the stream, but only as far as the writable
// stream implementation is concerned. // stream implementation is concerned.
@ -2287,8 +2285,6 @@ class ServerHttp2Stream extends Http2Stream {
options.readable = false; options.readable = false;
const headersList = mapToHeaders(headers); const headersList = mapToHeaders(headers);
if (!Array.isArray(headersList))
throw headersList;
const streamOptions = options.endStream ? STREAM_OPTION_EMPTY_PAYLOAD : 0; const streamOptions = options.endStream ? STREAM_OPTION_EMPTY_PAYLOAD : 0;
@ -2365,8 +2361,6 @@ class ServerHttp2Stream extends Http2Stream {
} }
const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse);
if (!Array.isArray(headersList))
throw headersList;
this[kSentHeaders] = headers; this[kSentHeaders] = headers;
state.flags |= STREAM_FLAGS_HEADERS_SENT; state.flags |= STREAM_FLAGS_HEADERS_SENT;
@ -2527,8 +2521,6 @@ class ServerHttp2Stream extends Http2Stream {
this[kUpdateTimer](); this[kUpdateTimer]();
const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse); const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse);
if (!Array.isArray(headersList))
throw headersList;
if (!this[kInfoHeaders]) if (!this[kInfoHeaders])
this[kInfoHeaders] = [headers]; this[kInfoHeaders] = [headers];
else else

View File

@ -406,7 +406,7 @@ function assertValidPseudoHeader(key) {
if (!kValidPseudoHeaders.has(key)) { if (!kValidPseudoHeaders.has(key)) {
const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key); const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key);
Error.captureStackTrace(err, assertValidPseudoHeader); Error.captureStackTrace(err, assertValidPseudoHeader);
return err; throw err;
} }
} }
@ -414,14 +414,14 @@ function assertValidPseudoHeaderResponse(key) {
if (key !== ':status') { if (key !== ':status') {
const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key); const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key);
Error.captureStackTrace(err, assertValidPseudoHeaderResponse); Error.captureStackTrace(err, assertValidPseudoHeaderResponse);
return err; throw err;
} }
} }
function assertValidPseudoHeaderTrailer(key) { function assertValidPseudoHeaderTrailer(key) {
const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key); const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key);
Error.captureStackTrace(err, assertValidPseudoHeaderTrailer); Error.captureStackTrace(err, assertValidPseudoHeaderTrailer);
return err; throw err;
} }
function mapToHeaders(map, function mapToHeaders(map,
@ -454,26 +454,26 @@ function mapToHeaders(map,
break; break;
default: default:
if (isSingleValueHeader) if (isSingleValueHeader)
return new ERR_HTTP2_HEADER_SINGLE_VALUE(key); throw new ERR_HTTP2_HEADER_SINGLE_VALUE(key);
} }
} else { } else {
value = String(value); value = String(value);
} }
if (isSingleValueHeader) { if (isSingleValueHeader) {
if (singles.has(key)) if (singles.has(key))
return new ERR_HTTP2_HEADER_SINGLE_VALUE(key); throw new ERR_HTTP2_HEADER_SINGLE_VALUE(key);
singles.add(key); singles.add(key);
} }
if (key[0] === ':') { if (key[0] === ':') {
err = assertValuePseudoHeader(key); err = assertValuePseudoHeader(key);
if (err !== undefined) if (err !== undefined)
return err; throw err;
ret = `${key}\0${value}\0${ret}`; ret = `${key}\0${value}\0${ret}`;
count++; count++;
continue; continue;
} }
if (isIllegalConnectionSpecificHeader(key, value)) { if (isIllegalConnectionSpecificHeader(key, value)) {
return new ERR_HTTP2_INVALID_CONNECTION_HEADERS(key); throw new ERR_HTTP2_INVALID_CONNECTION_HEADERS(key);
} }
if (isArray) { if (isArray) {
for (var k = 0; k < value.length; k++) { for (var k = 0; k < value.length; k++) {

View File

@ -8,24 +8,16 @@ const common = require('../common');
// have to test it through mapToHeaders // have to test it through mapToHeaders
const { mapToHeaders } = require('internal/http2/util'); const { mapToHeaders } = require('internal/http2/util');
const assert = require('assert');
function isNotError(val) { // These should not throw
assert(!(val instanceof Error)); mapToHeaders({ ':status': 'a' });
} mapToHeaders({ ':path': 'a' });
mapToHeaders({ ':authority': 'a' });
mapToHeaders({ ':scheme': 'a' });
mapToHeaders({ ':method': 'a' });
function isError(val) { common.expectsError(() => mapToHeaders({ ':foo': 'a' }), {
common.expectsError({ code: 'ERR_HTTP2_INVALID_PSEUDOHEADER',
code: 'ERR_HTTP2_INVALID_PSEUDOHEADER', type: TypeError,
type: TypeError, message: '":foo" is an invalid pseudoheader or is used incorrectly'
message: '":foo" is an invalid pseudoheader or is used incorrectly' });
})(val);
}
isNotError(mapToHeaders({ ':status': 'a' }));
isNotError(mapToHeaders({ ':path': 'a' }));
isNotError(mapToHeaders({ ':authority': 'a' }));
isNotError(mapToHeaders({ ':scheme': 'a' }));
isNotError(mapToHeaders({ ':method': 'a' }));
isError(mapToHeaders({ ':foo': 'a' }));

View File

@ -175,11 +175,11 @@ const {
':statuS': 204, ':statuS': 204,
}; };
common.expectsError({ common.expectsError(() => mapToHeaders(headers), {
code: 'ERR_HTTP2_HEADER_SINGLE_VALUE', code: 'ERR_HTTP2_HEADER_SINGLE_VALUE',
type: TypeError, type: TypeError,
message: 'Header field ":status" must only have a single value' message: 'Header field ":status" must only have a single value'
})(mapToHeaders(headers)); });
} }
// The following are not allowed to have multiple values // The following are not allowed to have multiple values
@ -224,10 +224,10 @@ const {
HTTP2_HEADER_X_CONTENT_TYPE_OPTIONS HTTP2_HEADER_X_CONTENT_TYPE_OPTIONS
].forEach((name) => { ].forEach((name) => {
const msg = `Header field "${name}" must only have a single value`; const msg = `Header field "${name}" must only have a single value`;
common.expectsError({ common.expectsError(() => mapToHeaders({ [name]: [1, 2, 3] }), {
code: 'ERR_HTTP2_HEADER_SINGLE_VALUE', code: 'ERR_HTTP2_HEADER_SINGLE_VALUE',
message: msg message: msg
})(mapToHeaders({ [name]: [1, 2, 3] })); });
}); });
[ [
@ -281,30 +281,32 @@ const {
'Proxy-Connection', 'Proxy-Connection',
'Keep-Alive' 'Keep-Alive'
].forEach((name) => { ].forEach((name) => {
common.expectsError({ common.expectsError(() => mapToHeaders({ [name]: 'abc' }), {
code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS',
name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]', name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]',
message: 'HTTP/1 Connection specific headers are forbidden: ' + message: 'HTTP/1 Connection specific headers are forbidden: ' +
`"${name.toLowerCase()}"` `"${name.toLowerCase()}"`
})(mapToHeaders({ [name]: 'abc' })); });
}); });
common.expectsError({ common.expectsError(() => mapToHeaders({ [HTTP2_HEADER_TE]: ['abc'] }), {
code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS',
name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]', name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]',
message: 'HTTP/1 Connection specific headers are forbidden: ' + message: 'HTTP/1 Connection specific headers are forbidden: ' +
`"${HTTP2_HEADER_TE}"` `"${HTTP2_HEADER_TE}"`
})(mapToHeaders({ [HTTP2_HEADER_TE]: ['abc'] })); });
common.expectsError({ common.expectsError(
code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS', () => mapToHeaders({ [HTTP2_HEADER_TE]: ['abc', 'trailers'] }), {
name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]', code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS',
message: 'HTTP/1 Connection specific headers are forbidden: ' + name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]',
`"${HTTP2_HEADER_TE}"` message: 'HTTP/1 Connection specific headers are forbidden: ' +
})(mapToHeaders({ [HTTP2_HEADER_TE]: ['abc', 'trailers'] })); `"${HTTP2_HEADER_TE}"`
});
assert(!(mapToHeaders({ te: 'trailers' }) instanceof Error)); // These should not throw
assert(!(mapToHeaders({ te: ['trailers'] }) instanceof Error)); mapToHeaders({ te: 'trailers' });
mapToHeaders({ te: ['trailers'] });
{ {