Skip to content

Commit

Permalink
http2: add compat trailers, adjust multi-headers
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
apapirovski authored and MylesBorins committed Sep 12, 2017
1 parent ddbcc9e commit 764213c
Show file tree
Hide file tree
Showing 10 changed files with 298 additions and 49 deletions.
61 changes: 39 additions & 22 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ const kStream = Symbol('stream');
const kRequest = Symbol('request');
const kResponse = Symbol('response');
const kHeaders = Symbol('headers');
const kRawHeaders = Symbol('rawHeaders');
const kTrailers = Symbol('trailers');
const kRawTrailers = Symbol('rawTrailers');

let statusMessageWarned = false;

Expand Down Expand Up @@ -45,12 +47,28 @@ function isPseudoHeader(name) {
}
}

function statusMessageWarn() {
if (statusMessageWarned === false) {
process.emitWarning(
'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
'UnsupportedWarning'
);
statusMessageWarned = true;
}
}

function onStreamData(chunk) {
const request = this[kRequest];
if (!request.push(chunk))
this.pause();
}

function onStreamTrailers(trailers, flags, rawTrailers) {
const request = this[kRequest];
Object.assign(request[kTrailers], trailers);
request[kRawTrailers].push(...rawTrailers);
}

function onStreamEnd() {
// Cause the request stream to end as well.
const request = this[kRequest];
Expand Down Expand Up @@ -106,20 +124,24 @@ function onAborted(hadError, code) {
}

class Http2ServerRequest extends Readable {
constructor(stream, headers, options) {
constructor(stream, headers, options, rawHeaders) {
super(options);
this[kState] = {
statusCode: null,
closed: false,
closedCode: constants.NGHTTP2_NO_ERROR
};
this[kHeaders] = headers;
this[kRawHeaders] = rawHeaders;
this[kTrailers] = {};
this[kRawTrailers] = [];
this[kStream] = stream;
stream[kRequest] = this;

// 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 @@ -155,18 +177,17 @@ class Http2ServerRequest extends Readable {
}

get rawHeaders() {
const headers = this[kHeaders];
if (headers === undefined)
return [];
const tuples = Object.entries(headers);
const flattened = Array.prototype.concat.apply([], tuples);
return flattened.map(String);
return this[kRawHeaders];
}

get trailers() {
return this[kTrailers];
}

get rawTrailers() {
return this[kRawTrailers];
}

get httpVersionMajor() {
return 2;
}
Expand Down Expand Up @@ -382,30 +403,25 @@ class Http2ServerResponse extends Stream {
}

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

return '';
}

set statusMessage(msg) {
statusMessageWarn();
}

flushHeaders() {
if (this[kStream].headersSent === false)
this[kBeginSend]();
}

writeHead(statusCode, statusMessage, headers) {
if (typeof statusMessage === 'string' && statusMessageWarned === false) {
process.emitWarning(
'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)',
'UnsupportedWarning'
);
statusMessageWarned = true;
if (typeof statusMessage === 'string') {
statusMessageWarn();
}

if (headers === undefined && typeof statusMessage === 'object') {
headers = statusMessage;
}
Expand Down Expand Up @@ -542,9 +558,10 @@ class Http2ServerResponse extends Stream {
}
}

function onServerStream(stream, headers, flags) {
function onServerStream(stream, headers, flags, rawHeaders) {
const server = this;
const request = new Http2ServerRequest(stream, headers);
const request = new Http2ServerRequest(stream, headers, undefined,
rawHeaders);
const response = new Http2ServerResponse(stream);

// Check for the CONNECT method
Expand Down
11 changes: 7 additions & 4 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ function onSessionHeaders(id, cat, flags, headers) {
'report this as a bug in Node.js');
}
streams.set(id, stream);
process.nextTick(emit.bind(owner, 'stream', stream, obj, flags));
process.nextTick(emit.bind(owner, 'stream', stream, obj, flags, headers));
} else {
let event;
let status;
Expand Down Expand Up @@ -218,7 +218,10 @@ function onSessionHeaders(id, cat, flags, headers) {
'report this as a bug in Node.js');
}
debug(`[${sessionName(owner[kType])}] emitting stream '${event}' event`);
process.nextTick(emit.bind(stream, event, obj, flags));
process.nextTick(emit.bind(stream, event, obj, flags, headers));
}
if (endOfStream) {
stream.push(null);
}
}

Expand Down Expand Up @@ -2271,9 +2274,9 @@ function socketOnTimeout() {

// Handles the on('stream') event for a session and forwards
// it on to the server object.
function sessionOnStream(stream, headers, flags) {
function sessionOnStream(stream, headers, flags, rawHeaders) {
debug(`[${sessionName(this[kType])}] emit server stream event`);
this[kServer].emit('stream', stream, headers, flags);
this[kServer].emit('stream', stream, headers, flags, rawHeaders);
}

function sessionOnPriority(stream, parent, weight, exclusive) {
Expand Down
43 changes: 31 additions & 12 deletions lib/internal/http2/util.js
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
40 changes: 40 additions & 0 deletions test/parallel/test-http2-compat-serverrequest-end.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Flags: --expose-http2
'use strict';

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

// Http2ServerRequest should always end readable stream
// even on GET requests with no body

const server = h2.createServer();
server.listen(0, common.mustCall(function() {
const port = server.address().port;
server.once('request', common.mustCall(function(request, response) {
request.on('data', () => {});
request.on('end', common.mustCall(() => {
response.on('finish', common.mustCall(function() {
server.close();
}));
response.end();
}));
}));

const url = `http://localhost:${port}`;
const client = h2.connect(url, common.mustCall(function() {
const headers = {
':path': '/foobar',
':method': 'GET',
':scheme': 'http',
':authority': `localhost:${port}`
};
const request = client.request(headers);
request.resume();
request.on('end', common.mustCall(function() {
client.destroy();
}));
request.end();
}));
}));
71 changes: 71 additions & 0 deletions test/parallel/test-http2-compat-serverrequest-trailers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
// 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',
'x-foo-test': 'test, test'
};

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.deepStrictEqual([
'x-foo',
'xOxOxOx',
'x-foo',
'OxOxOxO',
'x-foo',
'xOxOxOx',
'x-foo',
'OxOxOxO',
'x-foo-test',
'test, test'
], request.rawTrailers);
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';
trailers['x-foo-test'] = 'test, test';
}
});
request.resume();
request.on('end', common.mustCall(function() {
server.close();
client.destroy();
}));
request.write('test\n');
request.end('test');
}));
}));
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Flags: --expose-http2
'use strict';

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

// Http2ServerResponse.statusMessage should warn

const unsupportedWarned = common.mustCall(1);
process.on('warning', ({ name, message }) => {
const expectedMessage =
'Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)';
if (name === 'UnsupportedWarning' && message === expectedMessage)
unsupportedWarned();
});

const server = h2.createServer();
server.listen(0, common.mustCall(function() {
const port = server.address().port;
server.once('request', common.mustCall(function(request, response) {
response.on('finish', common.mustCall(function() {
response.statusMessage = 'test';
response.statusMessage = 'test'; // only warn once
assert.strictEqual(response.statusMessage, ''); // no change
server.close();
}));
response.end();
}));

const url = `http://localhost:${port}`;
const client = h2.connect(url, common.mustCall(function() {
const headers = {
':path': '/',
':method': 'GET',
':scheme': 'http',
':authority': `localhost:${port}`
};
const request = client.request(headers);
request.on('response', common.mustCall(function(headers) {
assert.strictEqual(headers[':status'], 200);
}, 1));
request.on('end', common.mustCall(function() {
client.destroy();
}));
request.end();
request.resume();
}));
}));
Loading

0 comments on commit 764213c

Please sign in to comment.