Skip to content

Commit

Permalink
http: make --insecure-http-parser configurable per-stream or per-server
Browse files Browse the repository at this point in the history
From the issue:

> Some servers deviate from HTTP spec enougth that Node.js can't
> communicate with them, but "work" when `--insecure-http-parser`
> is enabled globally. It would be useful to be able to use this
> mode, as a client, only when connecting to known bad servers.

This is largely equivalent to nodejs#31446
in terms of code changes.

Fixes: nodejs#31440
Refs: nodejs#31446

PR-URL: nodejs#31448
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
addaleax committed Jan 24, 2020
1 parent 58ec670 commit 22525e2
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 2 deletions.
15 changes: 15 additions & 0 deletions doc/api/http.md
Original file line number Diff line number Diff line change
Expand Up @@ -1830,6 +1830,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: v9.6.0, v8.12.0
pr-url: https://github.com/nodejs/node/pull/15752
description: The `options` argument is supported now.
Expand All @@ -1841,6 +1844,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.
**Default:** `false`
* `requestListener` {Function}

* Returns: {http.Server}
Expand Down Expand Up @@ -1943,6 +1950,9 @@ Defaults to 8KB. Configurable using the [`--max-http-header-size`][] CLI option.
<!-- 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: v10.9.0
pr-url: https://github.com/nodejs/node/pull/21616
description: The `url` parameter can now be passed along with a separate
Expand All @@ -1962,6 +1972,10 @@ changes:
* `family` {number} IP address family to use when resolving `host` or
`hostname`. Valid values are `4` or `6`. When unspecified, both IP v4 and
v6 will be used.
* `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.
**Default:** `false`
* `port` {number} Port of remote server. **Default:** `80`.
* `localAddress` {string} Local interface to bind for network connections.
* `socketPath` {string} Unix Domain Socket (cannot be used if one of `host`
Expand Down Expand Up @@ -2123,6 +2137,7 @@ will be emitted in the following order:
Note that 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 @@ -147,6 +147,14 @@ function ClientRequest(input, options, cb) {
method = this.method = 'GET';
}

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 @@ -628,7 +636,8 @@ function tickOnSocket(req, socket) {
req.socket = socket;
req.connection = socket;
parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol],
isLenient());
req.insecureHTTPParser === undefined ?
isLenient() : req.insecureHTTPParser);
parser.socket = socket;
parser.outgoing = req;
req.parser = parser;
Expand Down
12 changes: 11 additions & 1 deletion lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const { IncomingMessage } = require('_http_incoming');
const {
ERR_HTTP_HEADERS_SENT,
ERR_HTTP_INVALID_STATUS_CODE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CHAR
} = require('internal/errors').codes;
const Buffer = require('buffer').Buffer;
Expand Down Expand Up @@ -290,6 +291,14 @@ function Server(options, requestListener) {
this[kIncomingMessage] = options.IncomingMessage || IncomingMessage;
this[kServerResponse] = options.ServerResponse || ServerResponse;

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 @@ -344,7 +353,8 @@ function connectionListenerInternal(server, socket) {

var parser = parsers.alloc();
parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol],
isLenient());
server.insecureHTTPParser === undefined ?
isLenient() : server.insecureHTTPParser);
parser.socket = socket;

// We are starting to wait for our headers.
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');
}

0 comments on commit 22525e2

Please sign in to comment.