Skip to content

Commit 764213c

Browse files
apapirovskiMylesBorins
apapirovski
authored and
MylesBorins
committed
http2: add compat trailers, adjust multi-headers
Adds Http2ServerRequest trailers & rawTrailers functionality. Also fixes behaviour of multi-headers to conform with the spec (all values but set-cookie and cookie should be comma delimited, cookie should be semi-colon delimited and only set-cookie should be an array). Adds setter for statusMessage that warns, for backwards compatibility. End readable side of the stream on trailers or bodyless requests Refs: expressjs/express#3390 (comment) PR-URL: #15193 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent ddbcc9e commit 764213c

10 files changed

+298
-49
lines changed

‎lib/internal/http2/compat.js

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ const kStream = Symbol('stream');
1313
const kRequest = Symbol('request');
1414
const kResponse = Symbol('response');
1515
const kHeaders = Symbol('headers');
16+
const kRawHeaders = Symbol('rawHeaders');
1617
const kTrailers = Symbol('trailers');
18+
const kRawTrailers = Symbol('rawTrailers');
1719

1820
let statusMessageWarned = false;
1921

@@ -45,12 +47,28 @@ function isPseudoHeader(name) {
4547
}
4648
}
4749

50+
function statusMessageWarn() {
51+
if (statusMessageWarned === false) {
52+
process.emitWarning(
53+
'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
54+
'UnsupportedWarning'
55+
);
56+
statusMessageWarned = true;
57+
}
58+
}
59+
4860
function onStreamData(chunk) {
4961
const request = this[kRequest];
5062
if (!request.push(chunk))
5163
this.pause();
5264
}
5365

66+
function onStreamTrailers(trailers, flags, rawTrailers) {
67+
const request = this[kRequest];
68+
Object.assign(request[kTrailers], trailers);
69+
request[kRawTrailers].push(...rawTrailers);
70+
}
71+
5472
function onStreamEnd() {
5573
// Cause the request stream to end as well.
5674
const request = this[kRequest];
@@ -106,20 +124,24 @@ function onAborted(hadError, code) {
106124
}
107125

108126
class Http2ServerRequest extends Readable {
109-
constructor(stream, headers, options) {
127+
constructor(stream, headers, options, rawHeaders) {
110128
super(options);
111129
this[kState] = {
112130
statusCode: null,
113131
closed: false,
114132
closedCode: constants.NGHTTP2_NO_ERROR
115133
};
116134
this[kHeaders] = headers;
135+
this[kRawHeaders] = rawHeaders;
136+
this[kTrailers] = {};
137+
this[kRawTrailers] = [];
117138
this[kStream] = stream;
118139
stream[kRequest] = this;
119140

120141
// Pause the stream..
121142
stream.pause();
122143
stream.on('data', onStreamData);
144+
stream.on('trailers', onStreamTrailers);
123145
stream.on('end', onStreamEnd);
124146
stream.on('error', onStreamError);
125147
stream.on('close', onStreamClosedRequest);
@@ -155,18 +177,17 @@ class Http2ServerRequest extends Readable {
155177
}
156178

157179
get rawHeaders() {
158-
const headers = this[kHeaders];
159-
if (headers === undefined)
160-
return [];
161-
const tuples = Object.entries(headers);
162-
const flattened = Array.prototype.concat.apply([], tuples);
163-
return flattened.map(String);
180+
return this[kRawHeaders];
164181
}
165182

166183
get trailers() {
167184
return this[kTrailers];
168185
}
169186

187+
get rawTrailers() {
188+
return this[kRawTrailers];
189+
}
190+
170191
get httpVersionMajor() {
171192
return 2;
172193
}
@@ -382,30 +403,25 @@ class Http2ServerResponse extends Stream {
382403
}
383404

384405
get statusMessage() {
385-
if (statusMessageWarned === false) {
386-
process.emitWarning(
387-
'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
388-
'UnsupportedWarning'
389-
);
390-
statusMessageWarned = true;
391-
}
406+
statusMessageWarn();
392407

393408
return '';
394409
}
395410

411+
set statusMessage(msg) {
412+
statusMessageWarn();
413+
}
414+
396415
flushHeaders() {
397416
if (this[kStream].headersSent === false)
398417
this[kBeginSend]();
399418
}
400419

401420
writeHead(statusCode, statusMessage, headers) {
402-
if (typeof statusMessage === 'string' && statusMessageWarned === false) {
403-
process.emitWarning(
404-
'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
405-
'UnsupportedWarning'
406-
);
407-
statusMessageWarned = true;
421+
if (typeof statusMessage === 'string') {
422+
statusMessageWarn();
408423
}
424+
409425
if (headers === undefined && typeof statusMessage === 'object') {
410426
headers = statusMessage;
411427
}
@@ -542,9 +558,10 @@ class Http2ServerResponse extends Stream {
542558
}
543559
}
544560

545-
function onServerStream(stream, headers, flags) {
561+
function onServerStream(stream, headers, flags, rawHeaders) {
546562
const server = this;
547-
const request = new Http2ServerRequest(stream, headers);
563+
const request = new Http2ServerRequest(stream, headers, undefined,
564+
rawHeaders);
548565
const response = new Http2ServerResponse(stream);
549566

550567
// Check for the CONNECT method

‎lib/internal/http2/core.js

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ function onSessionHeaders(id, cat, flags, headers) {
185185
'report this as a bug in Node.js');
186186
}
187187
streams.set(id, stream);
188-
process.nextTick(emit.bind(owner, 'stream', stream, obj, flags));
188+
process.nextTick(emit.bind(owner, 'stream', stream, obj, flags, headers));
189189
} else {
190190
let event;
191191
let status;
@@ -218,7 +218,10 @@ function onSessionHeaders(id, cat, flags, headers) {
218218
'report this as a bug in Node.js');
219219
}
220220
debug(`[${sessionName(owner[kType])}] emitting stream '${event}' event`);
221-
process.nextTick(emit.bind(stream, event, obj, flags));
221+
process.nextTick(emit.bind(stream, event, obj, flags, headers));
222+
}
223+
if (endOfStream) {
224+
stream.push(null);
222225
}
223226
}
224227

@@ -2271,9 +2274,9 @@ function socketOnTimeout() {
22712274

22722275
// Handles the on('stream') event for a session and forwards
22732276
// it on to the server object.
2274-
function sessionOnStream(stream, headers, flags) {
2277+
function sessionOnStream(stream, headers, flags, rawHeaders) {
22752278
debug(`[${sessionName(this[kType])}] emit server stream event`);
2276-
this[kServer].emit('stream', stream, headers, flags);
2279+
this[kServer].emit('stream', stream, headers, flags, rawHeaders);
22772280
}
22782281

22792282
function sessionOnPriority(stream, parent, weight, exclusive) {

‎lib/internal/http2/util.js

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ const {
3535
HTTP2_HEADER_RANGE,
3636
HTTP2_HEADER_REFERER,
3737
HTTP2_HEADER_RETRY_AFTER,
38+
HTTP2_HEADER_SET_COOKIE,
3839
HTTP2_HEADER_USER_AGENT,
3940

4041
HTTP2_HEADER_CONNECTION,
@@ -474,18 +475,36 @@ function toHeaderObject(headers) {
474475
if (existing === undefined) {
475476
obj[name] = value;
476477
} else if (!kSingleValueHeaders.has(name)) {
477-
if (name === HTTP2_HEADER_COOKIE) {
478-
// https://tools.ietf.org/html/rfc7540#section-8.1.2.5
479-
// "...If there are multiple Cookie header fields after decompression,
480-
// these MUST be concatenated into a single octet string using the
481-
// two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before
482-
// being passed into a non-HTTP/2 context."
483-
obj[name] = `${existing}; ${value}`;
484-
} else {
485-
if (Array.isArray(existing))
486-
existing.push(value);
487-
else
488-
obj[name] = [existing, value];
478+
switch (name) {
479+
case HTTP2_HEADER_COOKIE:
480+
// https://tools.ietf.org/html/rfc7540#section-8.1.2.5
481+
// "...If there are multiple Cookie header fields after decompression,
482+
// these MUST be concatenated into a single octet string using the
483+
// two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before
484+
// being passed into a non-HTTP/2 context."
485+
obj[name] = `${existing}; ${value}`;
486+
break;
487+
case HTTP2_HEADER_SET_COOKIE:
488+
// https://tools.ietf.org/html/rfc7230#section-3.2.2
489+
// "Note: In practice, the "Set-Cookie" header field ([RFC6265]) often
490+
// appears multiple times in a response message and does not use the
491+
// list syntax, violating the above requirements on multiple header
492+
// fields with the same name. Since it cannot be combined into a
493+
// single field-value, recipients ought to handle "Set-Cookie" as a
494+
// special case while processing header fields."
495+
if (Array.isArray(existing))
496+
existing.push(value);
497+
else
498+
obj[name] = [existing, value];
499+
break;
500+
default:
501+
// https://tools.ietf.org/html/rfc7230#section-3.2.2
502+
// "A recipient MAY combine multiple header fields with the same field
503+
// name into one "field-name: field-value" pair, without changing the
504+
// semantics of the message, by appending each subsequent field value
505+
// to the combined field value in order, separated by a comma."
506+
obj[name] = `${existing}, ${value}`;
507+
break;
489508
}
490509
}
491510
}

‎test/parallel/test-http2-compat-serverrequest-end.js

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Flags: --expose-http2
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const h2 = require('http2');
8+
9+
// Http2ServerRequest should always end readable stream
10+
// even on GET requests with no body
11+
12+
const server = h2.createServer();
13+
server.listen(0, common.mustCall(function() {
14+
const port = server.address().port;
15+
server.once('request', common.mustCall(function(request, response) {
16+
request.on('data', () => {});
17+
request.on('end', common.mustCall(() => {
18+
response.on('finish', common.mustCall(function() {
19+
server.close();
20+
}));
21+
response.end();
22+
}));
23+
}));
24+
25+
const url = `http://localhost:${port}`;
26+
const client = h2.connect(url, common.mustCall(function() {
27+
const headers = {
28+
':path': '/foobar',
29+
':method': 'GET',
30+
':scheme': 'http',
31+
':authority': `localhost:${port}`
32+
};
33+
const request = client.request(headers);
34+
request.resume();
35+
request.on('end', common.mustCall(function() {
36+
client.destroy();
37+
}));
38+
request.end();
39+
}));
40+
}));

‎test/parallel/test-http2-compat-serverrequest-trailers.js

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// Flags: --expose-http2
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const assert = require('assert');
8+
const h2 = require('http2');
9+
10+
// Http2ServerRequest should have getter for trailers & rawTrailers
11+
12+
const expectedTrailers = {
13+
'x-foo': 'xOxOxOx, OxOxOxO, xOxOxOx, OxOxOxO',
14+
'x-foo-test': 'test, test'
15+
};
16+
17+
const server = h2.createServer();
18+
server.listen(0, common.mustCall(function() {
19+
const port = server.address().port;
20+
server.once('request', common.mustCall(function(request, response) {
21+
let data = '';
22+
request.setEncoding('utf8');
23+
request.on('data', common.mustCall((chunk) => data += chunk));
24+
request.on('end', common.mustCall(() => {
25+
const trailers = request.trailers;
26+
for (const [name, value] of Object.entries(expectedTrailers)) {
27+
assert.strictEqual(trailers[name], value);
28+
}
29+
assert.deepStrictEqual([
30+
'x-foo',
31+
'xOxOxOx',
32+
'x-foo',
33+
'OxOxOxO',
34+
'x-foo',
35+
'xOxOxOx',
36+
'x-foo',
37+
'OxOxOxO',
38+
'x-foo-test',
39+
'test, test'
40+
], request.rawTrailers);
41+
assert.strictEqual(data, 'test\ntest');
42+
response.end();
43+
}));
44+
}));
45+
46+
const url = `http://localhost:${port}`;
47+
const client = h2.connect(url, common.mustCall(function() {
48+
const headers = {
49+
':path': '/foobar',
50+
':method': 'POST',
51+
':scheme': 'http',
52+
':authority': `localhost:${port}`
53+
};
54+
const request = client.request(headers, {
55+
getTrailers(trailers) {
56+
trailers['x-fOo'] = 'xOxOxOx';
57+
trailers['x-foO'] = 'OxOxOxO';
58+
trailers['X-fOo'] = 'xOxOxOx';
59+
trailers['X-foO'] = 'OxOxOxO';
60+
trailers['x-foo-test'] = 'test, test';
61+
}
62+
});
63+
request.resume();
64+
request.on('end', common.mustCall(function() {
65+
server.close();
66+
client.destroy();
67+
}));
68+
request.write('test\n');
69+
request.end('test');
70+
}));
71+
}));

‎test/parallel/test-http2-compat-serverresponse-statusmessage-property-set.js

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// Flags: --expose-http2
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const assert = require('assert');
8+
const h2 = require('http2');
9+
10+
// Http2ServerResponse.statusMessage should warn
11+
12+
const unsupportedWarned = common.mustCall(1);
13+
process.on('warning', ({ name, message }) => {
14+
const expectedMessage =
15+
'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)';
16+
if (name === 'UnsupportedWarning' && message === expectedMessage)
17+
unsupportedWarned();
18+
});
19+
20+
const server = h2.createServer();
21+
server.listen(0, common.mustCall(function() {
22+
const port = server.address().port;
23+
server.once('request', common.mustCall(function(request, response) {
24+
response.on('finish', common.mustCall(function() {
25+
response.statusMessage = 'test';
26+
response.statusMessage = 'test'; // only warn once
27+
assert.strictEqual(response.statusMessage, ''); // no change
28+
server.close();
29+
}));
30+
response.end();
31+
}));
32+
33+
const url = `http://localhost:${port}`;
34+
const client = h2.connect(url, common.mustCall(function() {
35+
const headers = {
36+
':path': '/',
37+
':method': 'GET',
38+
':scheme': 'http',
39+
':authority': `localhost:${port}`
40+
};
41+
const request = client.request(headers);
42+
request.on('response', common.mustCall(function(headers) {
43+
assert.strictEqual(headers[':status'], 200);
44+
}, 1));
45+
request.on('end', common.mustCall(function() {
46+
client.destroy();
47+
}));
48+
request.end();
49+
request.resume();
50+
}));
51+
}));

0 commit comments

Comments
 (0)