Skip to content

http2: add compat trailers, adjust multi-headers #15193

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

Closed
apapirovski wants to merge 3 commits into from
Closed

http2: add compat trailers, adjust multi-headers #15193

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ac07be
http2: add compat trailers, adjust multi-headers
apapirovski Sep 5, 2017
52e79ea
Add proper support for raw headers & trailers, incorporate feedback
apapirovski Sep 5, 2017
85f9ab0
End readable side of the stream on trailers or bodyless requests
apapirovski Sep 5, 2017
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
29 changes: 28 additions & 1 deletion lib/internal/http2/compat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ function onStreamData(chunk) {
this.pause();
}

function onStreamTrailers(trailers) {
const request = this[kRequest];
request[kTrailers] = trailers;
// also causes the request stream to end
request.push(null);
}

function onStreamEnd() {
// Cause the request stream to end as well.
const request = this[kRequest];
Expand Down Expand Up @@ -120,6 +127,7 @@ class Http2ServerRequest extends Readable {
// Pause the stream..
stream.pause();
stream.on('data', onStreamData);
stream.on('trailers', onStreamTrailers);
stream.on('end', onStreamEnd);
stream.on('error', onStreamError);
stream.on('close', onStreamClosedRequest);
Expand Down Expand Up @@ -164,7 +172,16 @@ class Http2ServerRequest extends Readable {
}

get trailers() {
return this[kTrailers];
return this[kTrailers] || [];
Copy link
Member

@mcollina mcollina Sep 5, 2017

Choose a reason for hiding this comment

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

this is not the exact behavior. The returned this[kTrailers]  will be editable, while [] will not.

Copy link
Contributor Author

@apapirovski apapirovski Sep 5, 2017

Choose a reason for hiding this comment

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

Hmm, good point, thanks! Will update.

}

get rawTrailers() {
const trailers = this[kTrailers];
if (trailers === undefined)
return [];
const tuples = Object.entries(trailers);
const flattened = Array.prototype.concat.apply([], tuples);
Copy link
Member

@mcollina mcollina Sep 5, 2017

Choose a reason for hiding this comment

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

Can you use [].concat(tuples) instead? Why is this needed?

Copy link
Contributor Author

@apapirovski apapirovski Sep 5, 2017

Choose a reason for hiding this comment

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

It's a [[name, value], [name, value]] type of structure. [].concat(tuples) won't flatten it.

That said, I would ideally like to land this without this code and instead with a real solution for rawHeaders and rawTrailers (one that actually provides the real raw data to the user). I'm going to create a new commit with that behaviour and would appreciate if you or @jasnell could review.

Copy link
Member

@mcollina mcollina Sep 5, 2017

Choose a reason for hiding this comment

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

I think

[].concat(...Object.entries(obj))

would read better.

return flattened.map(String);
}

get httpVersionMajor() {
Expand Down Expand Up @@ -392,6 +409,16 @@ class Http2ServerResponse extends Stream {
return '';
}

set statusMessage(msg) {
if (statusMessageWarned === false) {
process.emitWarning(
'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
'UnsupportedWarning'
);
statusMessageWarned = true;
}
}
Copy link
Member

@mcollina mcollina Sep 5, 2017

Choose a reason for hiding this comment

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

this same block of code is shown in two other spots, can you refactor it to a single location? It needs a unit test as well.

Copy link
Contributor Author

@apapirovski apapirovski Sep 5, 2017

Choose a reason for hiding this comment

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

Definitely. Will update.


flushHeaders() {
if (this[kStream].headersSent === false)
this[kBeginSend]();
Expand Down
43 changes: 31 additions & 12 deletions lib/internal/http2/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const {
HTTP2_HEADER_RANGE,
HTTP2_HEADER_REFERER,
HTTP2_HEADER_RETRY_AFTER,
HTTP2_HEADER_SET_COOKIE,
HTTP2_HEADER_USER_AGENT,

HTTP2_HEADER_CONNECTION,
Expand Down Expand Up @@ -474,18 +475,36 @@ function toHeaderObject(headers) {
if (existing === undefined) {
obj[name] = value;
} else if (!kSingleValueHeaders.has(name)) {
if (name === HTTP2_HEADER_COOKIE) {
// https://tools.ietf.org/html/rfc7540#section-8.1.2.5
// "...If there are multiple Cookie header fields after decompression,
// these MUST be concatenated into a single octet string using the
// two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before
// being passed into a non-HTTP/2 context."
obj[name] = `${existing}; ${value}`;
} else {
if (Array.isArray(existing))
existing.push(value);
else
obj[name] = [existing, value];
switch (name) {
case HTTP2_HEADER_COOKIE:
// https://tools.ietf.org/html/rfc7540#section-8.1.2.5
// "...If there are multiple Cookie header fields after decompression,
// these MUST be concatenated into a single octet string using the
// two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ") before
// being passed into a non-HTTP/2 context."
obj[name] = `${existing}; ${value}`;
break;
case HTTP2_HEADER_SET_COOKIE:
// https://tools.ietf.org/html/rfc7230#section-3.2.2
// "Note: In practice, the "Set-Cookie" header field ([RFC6265]) often
// appears multiple times in a response message and does not use the
// list syntax, violating the above requirements on multiple header
// fields with the same name. Since it cannot be combined into a
// single field-value, recipients ought to handle "Set-Cookie" as a
// special case while processing header fields."
if (Array.isArray(existing))
existing.push(value);
else
obj[name] = [existing, value];
break;
default:
// https://tools.ietf.org/html/rfc7230#section-3.2.2
// "A recipient MAY combine multiple header fields with the same field
// name into one "field-name: field-value" pair, without changing the
// semantics of the message, by appending each subsequent field value
// to the combined field value in order, separated by a comma."
obj[name] = `${existing}, ${value}`;
break;
}
}
}
Expand Down
57 changes: 57 additions & 0 deletions test/parallel/test-http2-compat-serverrequest-trailers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Flags: --expose-http2
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const h2 = require('http2');

// Http2ServerRequest should have getter for trailers & rawTrailers

const expectedTrailers = {
'x-foo': 'xOxOxOx, OxOxOxO, xOxOxOx, OxOxOxO'
};

const server = h2.createServer();
server.listen(0, common.mustCall(function() {
const port = server.address().port;
server.once('request', common.mustCall(function(request, response) {
let data = '';
request.setEncoding('utf8');
request.on('data', common.mustCall((chunk) => data += chunk));
request.on('end', common.mustCall(() => {
const trailers = request.trailers;
for (const [name, value] of Object.entries(expectedTrailers)) {
assert.strictEqual(trailers[name], value);
}
assert.strictEqual(data, 'test\ntest');
response.end();
}));
}));

const url = `http://localhost:${port}`;
const client = h2.connect(url, common.mustCall(function() {
const headers = {
':path': '/foobar',
':method': 'POST',
':scheme': 'http',
':authority': `localhost:${port}`
};
const request = client.request(headers, {
getTrailers(trailers) {
trailers['x-fOo'] = 'xOxOxOx';
trailers['x-foO'] = 'OxOxOxO';
trailers['X-fOo'] = 'xOxOxOx';
trailers['X-foO'] = 'OxOxOxO';
}
});
request.resume();
request.on('end', common.mustCall(function() {
server.close();
client.destroy();
}));
request.write('test\n');
request.end('test');
}));
}));
7 changes: 2 additions & 5 deletions test/parallel/test-http2-cookies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,8 @@ server.on('stream', common.mustCall(onStream));

function onStream(stream, headers, flags) {

assert(Array.isArray(headers.abc));
assert.strictEqual(headers.abc.length, 3);
assert.strictEqual(headers.abc[0], '1');
assert.strictEqual(headers.abc[1], '2');
assert.strictEqual(headers.abc[2], '3');
assert.strictEqual(typeof headers.abc, 'string');
assert.strictEqual(headers.abc, '1, 2, 3');
assert.strictEqual(typeof headers.cookie, 'string');
assert.strictEqual(headers.cookie, 'a=b; c=d; e=f');

Expand Down
12 changes: 6 additions & 6 deletions test/parallel/test-http2-multiheaders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ src['__Proto__'] = 'baz';

function checkHeaders(headers) {
assert.deepStrictEqual(headers['accept'],
[ 'abc', 'def', 'ghijklmnop' ]);
'abc, def, ghijklmnop');
assert.deepStrictEqual(headers['www-authenticate'],
[ 'foo', 'bar', 'baz' ]);
'foo, bar, baz');
assert.deepStrictEqual(headers['proxy-authenticate'],
[ 'foo', 'bar', 'baz' ]);
assert.deepStrictEqual(headers['x-foo'], [ 'foo', 'bar', 'baz' ]);
assert.deepStrictEqual(headers['constructor'], [ 'foo', 'bar', 'baz' ]);
'foo, bar, baz');
assert.deepStrictEqual(headers['x-foo'], 'foo, bar, baz');
assert.deepStrictEqual(headers['constructor'], 'foo, bar, baz');
// eslint-disable-next-line no-proto
assert.deepStrictEqual(headers['__proto__'], [ 'foo', 'bar', 'baz' ]);
assert.deepStrictEqual(headers['__proto__'], 'foo, bar, baz');
}

server.on('stream', common.mustCall((stream, headers) => {
Expand Down