Skip to content

Commit

Permalink
http2: throw from mapToHeaders
Browse files Browse the repository at this point in the history 
PR-URL: nodejs#24063
Reviewed-By: Matteo Collina <[email protected]>
Note: Landed with one collaborator approval after PR
      was open for 18 days
  • Loading branch information
@jasnell @refack
jasnell authored and refack committed Jan 10, 2019
1 parent 7ab43c5 commit 7ab1000
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 56 deletions.
20 changes: 6 additions & 14 deletions lib/internal/http2/core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1484,8 +1484,6 @@ class ClientHttp2Session extends Http2Session {
}

const headersList = mapToHeaders(headers);
if (!Array.isArray(headersList))
throw headersList;

const stream = new ClientHttp2Stream(this, undefined, undefined, {});
stream[kSentHeaders] = headers;
Expand Down Expand Up @@ -1890,8 +1888,6 @@ class Http2Stream extends Duplex {
this[kUpdateTimer]();

const headersList = mapToHeaders(headers, assertValidPseudoHeaderTrailer);
if (!Array.isArray(headersList))
throw headersList;
this[kSentTrailers] = headers;

// Send the trailers in setImmediate so we don't do it on nghttp2 stack.
Expand Down Expand Up @@ -2058,12 +2054,14 @@ function processRespondWithFD(self, fd, headers, offset = 0, length = -1,
const state = self[kState];
state.flags |= STREAM_FLAGS_HEADERS_SENT;

const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse);
self[kSentHeaders] = headers;
if (!Array.isArray(headersList)) {
self.destroy(headersList);
let headersList;
try {
headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse);
} catch (err) {
self.destroy(err);
return;
}
self[kSentHeaders] = headers;

// Close the writable side of the stream, but only as far as the writable
// stream implementation is concerned.
Expand Down Expand Up @@ -2287,8 +2285,6 @@ class ServerHttp2Stream extends Http2Stream {
options.readable = false;

const headersList = mapToHeaders(headers);
if (!Array.isArray(headersList))
throw headersList;

const streamOptions = options.endStream ? STREAM_OPTION_EMPTY_PAYLOAD : 0;

Expand Down Expand Up @@ -2365,8 +2361,6 @@ class ServerHttp2Stream extends Http2Stream {
}

const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse);
if (!Array.isArray(headersList))
throw headersList;
this[kSentHeaders] = headers;

state.flags |= STREAM_FLAGS_HEADERS_SENT;
Expand Down Expand Up @@ -2527,8 +2521,6 @@ class ServerHttp2Stream extends Http2Stream {
this[kUpdateTimer]();

const headersList = mapToHeaders(headers, assertValidPseudoHeaderResponse);
if (!Array.isArray(headersList))
throw headersList;
if (!this[kInfoHeaders])
this[kInfoHeaders] = [headers];
else
Expand Down
14 changes: 7 additions & 7 deletions lib/internal/http2/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -406,22 +406,22 @@ function assertValidPseudoHeader(key) {
if (!kValidPseudoHeaders.has(key)) {
const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key);
Error.captureStackTrace(err, assertValidPseudoHeader);
return err;
throw err;
}
}

function assertValidPseudoHeaderResponse(key) {
if (key !== ':status') {
const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key);
Error.captureStackTrace(err, assertValidPseudoHeaderResponse);
return err;
throw err;
}
}

function assertValidPseudoHeaderTrailer(key) {
const err = new ERR_HTTP2_INVALID_PSEUDOHEADER(key);
Error.captureStackTrace(err, assertValidPseudoHeaderTrailer);
return err;
throw err;
}

function mapToHeaders(map,
Expand Down Expand Up @@ -454,26 +454,26 @@ function mapToHeaders(map,
break;
default:
if (isSingleValueHeader)
return new ERR_HTTP2_HEADER_SINGLE_VALUE(key);
throw new ERR_HTTP2_HEADER_SINGLE_VALUE(key);
}
} else {
value = String(value);
}
if (isSingleValueHeader) {
if (singles.has(key))
return new ERR_HTTP2_HEADER_SINGLE_VALUE(key);
throw new ERR_HTTP2_HEADER_SINGLE_VALUE(key);
singles.add(key);
}
if (key[0] === ':') {
err = assertValuePseudoHeader(key);
if (err !== undefined)
return err;
throw err;
ret = `${key}\0${value}\0${ret}`;
count++;
continue;
}
if (isIllegalConnectionSpecificHeader(key, value)) {
return new ERR_HTTP2_INVALID_CONNECTION_HEADERS(key);
throw new ERR_HTTP2_INVALID_CONNECTION_HEADERS(key);
}
if (isArray) {
for (var k = 0; k < value.length; k++) {
Expand Down
30 changes: 11 additions & 19 deletions test/parallel/test-http2-util-assert-valid-pseudoheader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,16 @@ const common = require('../common');
// have to test it through mapToHeaders

const { mapToHeaders } = require('internal/http2/util');
const assert = require('assert');

function isNotError(val) {
assert(!(val instanceof Error));
}
// These should not throw
mapToHeaders({ ':status': 'a' });
mapToHeaders({ ':path': 'a' });
mapToHeaders({ ':authority': 'a' });
mapToHeaders({ ':scheme': 'a' });
mapToHeaders({ ':method': 'a' });

function isError(val) {
common.expectsError({
code: 'ERR_HTTP2_INVALID_PSEUDOHEADER',
type: TypeError,
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' }));
common.expectsError(() => mapToHeaders({ ':foo': 'a' }), {
code: 'ERR_HTTP2_INVALID_PSEUDOHEADER',
type: TypeError,
message: '":foo" is an invalid pseudoheader or is used incorrectly'
});
34 changes: 18 additions & 16 deletions test/parallel/test-http2-util-headers-list.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,11 @@ const {
':statuS': 204,
};

common.expectsError({
common.expectsError(() => mapToHeaders(headers), {
code: 'ERR_HTTP2_HEADER_SINGLE_VALUE',
type: TypeError,
message: 'Header field ":status" must only have a single value'
})(mapToHeaders(headers));
});
}

// The following are not allowed to have multiple values
Expand Down Expand Up @@ -224,10 +224,10 @@ const {
HTTP2_HEADER_X_CONTENT_TYPE_OPTIONS
].forEach((name) => {
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',
message: msg
})(mapToHeaders({ [name]: [1, 2, 3] }));
});
});

[
Expand Down Expand Up @@ -281,30 +281,32 @@ const {
'Proxy-Connection',
'Keep-Alive'
].forEach((name) => {
common.expectsError({
common.expectsError(() => mapToHeaders({ [name]: 'abc' }), {
code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS',
name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]',
message: 'HTTP/1 Connection specific headers are forbidden: ' +
`"${name.toLowerCase()}"`
})(mapToHeaders({ [name]: 'abc' }));
});
});

common.expectsError({
common.expectsError(() => mapToHeaders({ [HTTP2_HEADER_TE]: ['abc'] }), {
code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS',
name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]',
message: 'HTTP/1 Connection specific headers are forbidden: ' +
`"${HTTP2_HEADER_TE}"`
})(mapToHeaders({ [HTTP2_HEADER_TE]: ['abc'] }));
});

common.expectsError({
code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS',
name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]',
message: 'HTTP/1 Connection specific headers are forbidden: ' +
`"${HTTP2_HEADER_TE}"`
})(mapToHeaders({ [HTTP2_HEADER_TE]: ['abc', 'trailers'] }));
common.expectsError(
() => mapToHeaders({ [HTTP2_HEADER_TE]: ['abc', 'trailers'] }), {
code: 'ERR_HTTP2_INVALID_CONNECTION_HEADERS',
name: 'TypeError [ERR_HTTP2_INVALID_CONNECTION_HEADERS]',
message: 'HTTP/1 Connection specific headers are forbidden: ' +
`"${HTTP2_HEADER_TE}"`
});

assert(!(mapToHeaders({ te: 'trailers' }) instanceof Error));
assert(!(mapToHeaders({ te: ['trailers'] }) instanceof Error));
// These should not throw
mapToHeaders({ te: 'trailers' });
mapToHeaders({ te: ['trailers'] });


{
Expand Down

0 comments on commit 7ab1000

Please sign in to comment.