Skip to content

Commit

Permalink
http2: header field valid checks
Browse files Browse the repository at this point in the history
checks validity for request, writeHead, and setHeader methods

PR-URL: #33193
Refs: #29829
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
  • Loading branch information
rexagod authored and BridgeAR committed May 30, 2020
1 parent 2c0a4fa commit df31f71
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 2 deletions.
13 changes: 12 additions & 1 deletion lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,13 @@ const {
hideStackFrames
} = require('internal/errors');
const { validateString } = require('internal/validators');
const { kSocket, kRequest, kProxySocket } = require('internal/http2/util');
const {
kSocket,
kRequest,
kProxySocket,
assertValidPseudoHeader
} = require('internal/http2/util');
const { _checkIsHttpToken: checkIsHttpToken } = require('_http_common');

const kBeginSend = Symbol('begin-send');
const kState = Symbol('state');
Expand Down Expand Up @@ -590,6 +596,11 @@ class Http2ServerResponse extends Stream {
return;
}

if (name[0] === ':')
assertValidPseudoHeader(name);
else if (!checkIsHttpToken(name))
this.destroy(new ERR_INVALID_HTTP_TOKEN('Header name', name));

this[kHeaders][name] = value;
}

Expand Down
17 changes: 16 additions & 1 deletion lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const {
MathMin,
ObjectAssign,
ObjectCreate,
ObjectKeys,
ObjectDefineProperty,
ObjectPrototypeHasOwnProperty,
Promise,
Expand All @@ -35,7 +36,10 @@ const tls = require('tls');
const { URL } = require('url');
const { setImmediate } = require('timers');

const { kIncomingMessage } = require('_http_common');
const {
kIncomingMessage,
_checkIsHttpToken: checkIsHttpToken
} = require('_http_common');
const { kServerResponse } = require('_http_server');
const JSStreamSocket = require('internal/js_stream_socket');

Expand Down Expand Up @@ -88,6 +92,7 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CALLBACK,
ERR_INVALID_CHAR,
ERR_INVALID_HTTP_TOKEN,
ERR_INVALID_OPT_VALUE,
ERR_OUT_OF_RANGE,
ERR_SOCKET_CLOSED
Expand All @@ -108,6 +113,7 @@ const { onServerStream,

const {
assertIsObject,
assertValidPseudoHeader,
assertValidPseudoHeaderResponse,
assertValidPseudoHeaderTrailer,
assertWithinRange,
Expand Down Expand Up @@ -1602,6 +1608,15 @@ class ClientHttp2Session extends Http2Session {

this[kUpdateTimer]();

if (headers !== null && headers !== undefined) {
for (const header of ObjectKeys(headers)) {
if (header[0] === ':') {
assertValidPseudoHeader(header);
} else if (header && !checkIsHttpToken(header))
this.destroy(new ERR_INVALID_HTTP_TOKEN('Header name', header));
}
}

assertIsObject(headers, 'headers');
assertIsObject(options, 'options');

Expand Down
1 change: 1 addition & 0 deletions lib/internal/http2/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,7 @@ function sessionName(type) {

module.exports = {
assertIsObject,
assertValidPseudoHeader,
assertValidPseudoHeaderResponse,
assertValidPseudoHeaderTrailer,
assertWithinRange,
Expand Down
55 changes: 55 additions & 0 deletions test/parallel/test-http2-invalidheaderfields-client.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
'use strict';
const common = require('../common');
if (!common.hasCrypto) { common.skip('missing crypto'); }
const assert = require('assert');
const http2 = require('http2');

const server1 = http2.createServer();

server1.listen(0, common.mustCall(() => {
const session = http2.connect(`http://localhost:${server1.address().port}`);
// Check for req headers
session.request({ 'no underscore': 123 });
session.on('error', common.mustCall((e) => {
assert.strictEqual(e.code, 'ERR_INVALID_HTTP_TOKEN');
server1.close();
}));
}));

const server2 = http2.createServer(common.mustCall((req, res) => {
// check for setHeader
res.setHeader('x y z', 123);
res.end();
}));

server2.listen(0, common.mustCall(() => {
const session = http2.connect(`http://localhost:${server2.address().port}`);
const req = session.request();
req.on('error', common.mustCall((e) => {
assert.strictEqual(e.code, 'ERR_HTTP2_STREAM_ERROR');
session.close();
server2.close();
}));
}));

const server3 = http2.createServer(common.mustCall((req, res) => {
// check for writeHead
assert.throws(common.mustCall(() => {
res.writeHead(200, {
'an invalid header': 123
});
}), {
code: 'ERR_HTTP2_INVALID_STREAM'
});
res.end();
}));

server3.listen(0, common.mustCall(() => {
const session = http2.connect(`http://localhost:${server3.address().port}`);
const req = session.request();
req.on('error', common.mustCall((e) => {
assert.strictEqual(e.code, 'ERR_HTTP2_STREAM_ERROR');
server3.close();
session.close();
}));
}));

0 comments on commit df31f71

Please sign in to comment.