Skip to content
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.

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

http: make --insecure-http-parser configurable per-stream or per-server #31448

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
15 changes: 15 additions & 0 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -2031,6 +2031,9 @@ Found'`.
<!-- YAML
added: v0.1.13
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/31448
description: The `insecureHTTPParser` option is supported now.
- version: v13.3.0
pr-url: https://github.com/nodejs/node/pull/30570
description: The `maxHeaderSize` option is supported now.
Expand All @@ -2046,6 +2049,10 @@ changes:
* `ServerResponse` {http.ServerResponse} Specifies the `ServerResponse` class
to be used. Useful for extending the original `ServerResponse`. **Default:**
`ServerResponse`.
* `insecureHTTPParser` {boolean} Use an insecure HTTP parser that accepts
invalid HTTP headers when `true`. Using the insecure parser should be
avoided, see [`--insecure-http-parser`][] for more information.
addaleax marked this conversation as resolved.
Show resolved Hide resolved
**Default:** `false`
* `maxHeaderSize` {number} Optionally overrides the value of
[`--max-http-header-size`][] for requests received by this server, i.e.
the maximum length of request headers in bytes.
Expand Down Expand Up @@ -2155,6 +2162,9 @@ This can be overridden for servers and client requests by passing the
<!-- YAML
added: v0.3.6
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/31448
description: The `insecureHTTPParser` option is supported now.
- version: v13.3.0
pr-url: https://github.com/nodejs/node/pull/30570
description: The `maxHeaderSize` option is supported now.
Expand Down Expand Up @@ -2191,6 +2201,10 @@ changes:
request to. **Default:** `'localhost'`.
* `hostname` {string} Alias for `host`. To support [`url.parse()`][],
`hostname` will be used if both `host` and `hostname` are specified.
* `insecureHTTPParser` {boolean} Use an insecure HTTP parser that accepts
invalid HTTP headers when `true`. Using the insecure parser should be
avoided, see [`--insecure-http-parser`][] for more information.
addaleax marked this conversation as resolved.
Show resolved Hide resolved
**Default:** `false`
* `localAddress` {string} Local interface to bind for network connections.
* `lookup` {Function} Custom lookup function. **Default:** [`dns.lookup()`][].
* `maxHeaderSize` {number} Optionally overrides the value of
Expand Down Expand Up @@ -2364,6 +2378,7 @@ will be emitted in the following order:
Setting the `timeout` option or using the `setTimeout()` function will
not abort the request or do anything besides add a `'timeout'` event.

[`--insecure-http-parser`]: cli.html#cli_insecure_http_parser
[`--max-http-header-size`]: cli.html#cli_max_http_header_size_size
[`'checkContinue'`]: #http_event_checkcontinue
[`'request'`]: #http_event_request
Expand Down
11 changes: 10 additions & 1 deletion lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,14 @@ function ClientRequest(input, options, cb) {
validateInteger(maxHeaderSize, 'maxHeaderSize', 0);
this.maxHeaderSize = maxHeaderSize;

const insecureHTTPParser = options.insecureHTTPParser;
if (insecureHTTPParser !== undefined &&
typeof insecureHTTPParser !== 'boolean') {
throw new ERR_INVALID_ARG_TYPE(
'insecureHTTPParser', 'boolean', insecureHTTPParser);
}
this.insecureHTTPParser = insecureHTTPParser;

this.path = options.path || '/';
if (cb) {
this.once('response', cb);
Expand Down Expand Up @@ -683,7 +691,8 @@ function tickOnSocket(req, socket) {
parser.initialize(HTTPParser.RESPONSE,
new HTTPClientAsyncResource('HTTPINCOMINGMESSAGE', req),
req.maxHeaderSize || 0,
isLenient());
req.insecureHTTPParser === undefined ?
isLenient() : req.insecureHTTPParser);
parser.socket = socket;
parser.outgoing = req;
req.parser = parser;
Expand Down
11 changes: 10 additions & 1 deletion lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,14 @@ function Server(options, requestListener) {
validateInteger(maxHeaderSize, 'maxHeaderSize', 0);
this.maxHeaderSize = maxHeaderSize;

const insecureHTTPParser = options.insecureHTTPParser;
if (insecureHTTPParser !== undefined &&
typeof insecureHTTPParser !== 'boolean') {
throw new ERR_INVALID_ARG_TYPE(
'insecureHTTPParser', 'boolean', insecureHTTPParser);
}
this.insecureHTTPParser = insecureHTTPParser;

net.Server.call(this, { allowHalfOpen: true });

if (requestListener) {
Expand Down Expand Up @@ -416,7 +424,8 @@ function connectionListenerInternal(server, socket) {
HTTPParser.REQUEST,
new HTTPServerAsyncResource('HTTPINCOMINGMESSAGE', socket),
server.maxHeaderSize || 0,
isLenient(),
server.insecureHTTPParser === undefined ?
isLenient() : server.insecureHTTPParser,
);
parser.socket = socket;

Expand Down
82 changes: 82 additions & 0 deletions test/parallel/test-http-insecure-parser-per-stream.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');
const MakeDuplexPair = require('../common/duplexpair');

// Test that setting the `maxHeaderSize` option works on a per-stream-basis.

// Test 1: The server sends an invalid header.
{
const { clientSide, serverSide } = MakeDuplexPair();

const req = http.request({
createConnection: common.mustCall(() => clientSide),
insecureHTTPParser: true
}, common.mustCall((res) => {
assert.strictEqual(res.headers.hello, 'foo\x08foo');
res.resume(); // We don’t actually care about contents.
res.on('end', common.mustCall());
}));
req.end();

serverSide.resume(); // Dump the request
serverSide.end('HTTP/1.1 200 OK\r\n' +
'Hello: foo\x08foo\r\n' +
'Content-Length: 0\r\n' +
'\r\n\r\n');
}

// Test 2: The same as Test 1 except without the option, to make sure it fails.
{
const { clientSide, serverSide } = MakeDuplexPair();

const req = http.request({
createConnection: common.mustCall(() => clientSide)
}, common.mustNotCall());
req.end();
req.on('error', common.mustCall());

serverSide.resume(); // Dump the request
serverSide.end('HTTP/1.1 200 OK\r\n' +
'Hello: foo\x08foo\r\n' +
'Content-Length: 0\r\n' +
'\r\n\r\n');
}

// Test 3: The client sends an invalid header.
{
const testData = 'Hello, World!\n';
const server = http.createServer(
{ insecureHTTPParser: true },
common.mustCall((req, res) => {
res.statusCode = 200;
res.setHeader('Content-Type', 'text/plain');
res.end(testData);
}));

server.on('clientError', common.mustNotCall());

const { clientSide, serverSide } = MakeDuplexPair();
serverSide.server = server;
server.emit('connection', serverSide);

clientSide.write('GET / HTTP/1.1\r\n' +
'Hello: foo\x08foo\r\n' +
'\r\n\r\n');
}

// Test 4: The same as Test 3 except without the option, to make sure it fails.
{
const server = http.createServer(common.mustNotCall());

server.on('clientError', common.mustCall());

const { clientSide, serverSide } = MakeDuplexPair();
serverSide.server = server;
server.emit('connection', serverSide);

clientSide.write('GET / HTTP/1.1\r\n' +
'Hello: foo\x08foo\r\n' +
'\r\n\r\n');
}