From a7e2aab4ac49063e49843737328ed3d45dd57be3 Mon Sep 17 00:00:00 2001 From: iskore Date: Sat, 3 Feb 2018 21:49:14 -0500 Subject: [PATCH 1/8] http: setEncoding override for incoming packets added override to socket.setEncoding to not allow encoding changes for incoming HTTP requests added tests to ensure method throws JavaScript error because an HTTP buffer must be in US-ASCII, this function should not be allowed and should throw an Error currently, the process encounters a fatal v8 error and crashes error report detailed in [issue #18118](nodejs#18118) Fixes: nodejs#18118 Ref: nodejs#18178 --- lib/_http_server.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/_http_server.js b/lib/_http_server.js index 498800dd1ee445..2bd8bbd226d9a6 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -383,6 +383,7 @@ function connectionListenerInternal(server, socket) { // Override on to unconsume on `data`, `readable` listeners socket.on = socketOnWrap; + socket.setEncoding = socketSetEncoding; // We only consume the socket if it has never been consumed before. if (socket._handle) { @@ -683,6 +684,10 @@ function onSocketPause() { } } +function socketSetEncoding() { + throw new errors.Error('ERR_METHOD_NOT_IMPLEMENTED', 'setEncoding'); +} + function unconsume(parser, socket) { if (socket._handle) { if (parser._consumed) From 345ff3ce510e9bff7b6ffe0421799feec97128d1 Mon Sep 17 00:00:00 2001 From: iskore Date: Sat, 3 Feb 2018 21:52:07 -0500 Subject: [PATCH 2/8] test: added tests for setEncoding error check added test to ensure setEncoding inside socket connection would throw an error Fixes: nodejs#18118 Ref: nodejs#18178 --- .../test-http-socket-encoding-error.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 test/parallel/test-http-socket-encoding-error.js diff --git a/test/parallel/test-http-socket-encoding-error.js b/test/parallel/test-http-socket-encoding-error.js new file mode 100644 index 00000000000000..cfb96f4f81bc2e --- /dev/null +++ b/test/parallel/test-http-socket-encoding-error.js @@ -0,0 +1,23 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); + +const server = http.createServer().listen(0, connectToServer); + +server.on('connection', (socket) => { + common.expectsError(() => socket.setEncoding(''), + { + code: 'ERR_METHOD_NOT_IMPLEMENTED', + type: Error + }); + + socket.end(); +}); + +function connectToServer() { + const client = new http.Agent().createConnection(this.address().port, () => { + client.end(); + }) + .on('end', () => server.close()); +} From 4643c63fc73e1e5a8374a00a6fa92b28ea45885f Mon Sep 17 00:00:00 2001 From: Nick Soggin Date: Mon, 5 Mar 2018 19:29:58 -0500 Subject: [PATCH 3/8] docs, errors, http: disallow setEncoding on incoming packets added ERR_HTTP_INCOMING_SOCKET_ENCODING error and error docs throw ERR_HTTP_INCOMING_SOCKET_ENCODING error when incoming request socket encoding is manipulated error report detailed in nodejs#18118 Fixes: nodejs#18118 Ref: nodejs#18178 --- doc/api/errors.md | 6 ++++++ lib/_http_server.js | 2 +- lib/internal/errors.js | 3 +++ test/parallel/test-http-socket-encoding-error.js | 2 +- 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 62a62d76656223..5889e6e66a70cd 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -794,6 +794,12 @@ An invalid symlink type was passed to the [`fs.symlink()`][] or An attempt was made to add more headers after the headers had already been sent. + +### ERR_HTTP_INCOMING_SOCKET_ENCODING + +An attempt was made to manipulate the encoding of an incoming request packet. +This is not allowed (RFC2616). + ### ERR_HTTP_INVALID_HEADER_VALUE diff --git a/lib/_http_server.js b/lib/_http_server.js index 2bd8bbd226d9a6..25c23288a09f3d 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -685,7 +685,7 @@ function onSocketPause() { } function socketSetEncoding() { - throw new errors.Error('ERR_METHOD_NOT_IMPLEMENTED', 'setEncoding'); + throw new errors.Error('ERR_HTTP_INCOMING_SOCKET_ENCODING', 'setEncoding'); } function unconsume(parser, socket) { diff --git a/lib/internal/errors.js b/lib/internal/errors.js index f8073d15fe8d49..426c154fb19557 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -762,6 +762,9 @@ E('ERR_HTTP2_STREAM_SELF_DEPENDENCY', E('ERR_HTTP2_UNSUPPORTED_PROTOCOL', 'protocol "%s" is unsupported.', Error); E('ERR_HTTP_HEADERS_SENT', 'Cannot %s headers after they are sent to the client', Error); +E('ERR_HTTP_INCOMING_SOCKET_ENCODING', + 'Incoming socket encoding is not allowed (RFC 2616)', + Error); E('ERR_HTTP_INVALID_HEADER_VALUE', 'Invalid value "%s" for header "%s"', TypeError); E('ERR_HTTP_INVALID_STATUS_CODE', 'Invalid status code: %s', RangeError); diff --git a/test/parallel/test-http-socket-encoding-error.js b/test/parallel/test-http-socket-encoding-error.js index cfb96f4f81bc2e..58a22b9fb6400e 100644 --- a/test/parallel/test-http-socket-encoding-error.js +++ b/test/parallel/test-http-socket-encoding-error.js @@ -8,7 +8,7 @@ const server = http.createServer().listen(0, connectToServer); server.on('connection', (socket) => { common.expectsError(() => socket.setEncoding(''), { - code: 'ERR_METHOD_NOT_IMPLEMENTED', + code: 'ERR_HTTP_INCOMING_SOCKET_ENCODING', type: Error }); From f3c429cf597889a60f1a205819a135c46ec6f1b6 Mon Sep 17 00:00:00 2001 From: Nick Soggin Date: Wed, 14 Mar 2018 15:29:01 -0400 Subject: [PATCH 4/8] docs, errors: note RFC compliance in error message and documentation added note and link to RFC7230 >A recipient MUST parse an HTTP message as a sequence of octets in an encoding that is a superset of US-ASCII [USASCII]. Ref: https://tools.ietf.org/html/rfc7230#section-3 Ref: nodejs#18118 Ref: nodejs#18178 Ref: nodejs#19344 --- doc/api/errors.md | 2 +- lib/internal/errors.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 5889e6e66a70cd..df8eb06afea490 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -798,7 +798,7 @@ An attempt was made to add more headers after the headers had already been sent. ### ERR_HTTP_INCOMING_SOCKET_ENCODING An attempt was made to manipulate the encoding of an incoming request packet. -This is not allowed (RFC2616). +This is forbidden by [RFC7230 Section 3](https://tools.ietf.org/html/rfc7230#section-3) ### ERR_HTTP_INVALID_HEADER_VALUE diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 426c154fb19557..a63cc1e665b458 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -763,7 +763,7 @@ E('ERR_HTTP2_UNSUPPORTED_PROTOCOL', 'protocol "%s" is unsupported.', Error); E('ERR_HTTP_HEADERS_SENT', 'Cannot %s headers after they are sent to the client', Error); E('ERR_HTTP_INCOMING_SOCKET_ENCODING', - 'Incoming socket encoding is not allowed (RFC 2616)', + 'Changing the incoming socket encoding is not possible (see RFC7230 Section 3)', Error); E('ERR_HTTP_INVALID_HEADER_VALUE', 'Invalid value "%s" for header "%s"', TypeError); From 3613e8d4ee8d3035a5d8e7495348041813d02663 Mon Sep 17 00:00:00 2001 From: Nick Soggin Date: Wed, 14 Mar 2018 16:01:12 -0400 Subject: [PATCH 5/8] tests, http: fixed call to ERR_HTTP_INCOMING_SOCKET_ENCODING internal/errors declaration updated to match new scheme PR-URL: https://github.com/nodejs/node/pull/19344 Ref: https://tools.ietf.org/html/rfc7230#section-3 Ref: nodejs#18118 Ref: nodejs#18178 Ref: nodejs#19344 --- lib/_http_server.js | 3 ++- lib/internal/errors.js | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 25c23288a09f3d..b0a09785f8c430 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -45,6 +45,7 @@ const { const { IncomingMessage } = require('_http_incoming'); const { ERR_HTTP_HEADERS_SENT, + ERR_HTTP_INCOMING_SOCKET_ENCODING, ERR_HTTP_INVALID_STATUS_CODE, ERR_INVALID_CHAR } = require('internal/errors').codes; @@ -685,7 +686,7 @@ function onSocketPause() { } function socketSetEncoding() { - throw new errors.Error('ERR_HTTP_INCOMING_SOCKET_ENCODING', 'setEncoding'); + throw new ERR_HTTP_INCOMING_SOCKET_ENCODING('setEncoding'); } function unconsume(parser, socket) { diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a63cc1e665b458..0771ea020053b8 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -763,7 +763,7 @@ E('ERR_HTTP2_UNSUPPORTED_PROTOCOL', 'protocol "%s" is unsupported.', Error); E('ERR_HTTP_HEADERS_SENT', 'Cannot %s headers after they are sent to the client', Error); E('ERR_HTTP_INCOMING_SOCKET_ENCODING', - 'Changing the incoming socket encoding is not possible (see RFC7230 Section 3)', + 'Changing the incoming socket encoding is not possible (RFC7230 Section 3)', Error); E('ERR_HTTP_INVALID_HEADER_VALUE', 'Invalid value "%s" for header "%s"', TypeError); From 190ef7edb1966a727f3756589c902f1392f82a89 Mon Sep 17 00:00:00 2001 From: Nick Soggin Date: Thu, 29 Mar 2018 17:09:34 -0400 Subject: [PATCH 6/8] docs, http: minor format changes per review request PR-URL: https://github.com/nodejs/node/pull/19344 Ref: https://tools.ietf.org/html/rfc7230#section-3 Ref: nodejs#18118 Ref: nodejs#18178 Ref: nodejs#19344 --- doc/api/errors.md | 4 +++- lib/_http_server.js | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index df8eb06afea490..0b1abf178e7788 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -798,7 +798,7 @@ An attempt was made to add more headers after the headers had already been sent. ### ERR_HTTP_INCOMING_SOCKET_ENCODING An attempt was made to manipulate the encoding of an incoming request packet. -This is forbidden by [RFC7230 Section 3](https://tools.ietf.org/html/rfc7230#section-3) +This is forbidden by [RFC7230 Section 3][rfc7230-3] ### ERR_HTTP_INVALID_HEADER_VALUE @@ -1702,3 +1702,5 @@ Creation of a [`zlib`][] object failed due to incorrect configuration. [vm]: vm.html [WHATWG Supported Encodings]: util.html#util_whatwg_supported_encodings [`zlib`]: zlib.html + +[rfc7230-3]: https://tools.ietf.org/html/rfc7230#section-3 diff --git a/lib/_http_server.js b/lib/_http_server.js index b0a09785f8c430..0ef356ab8bac8c 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -384,6 +384,7 @@ function connectionListenerInternal(server, socket) { // Override on to unconsume on `data`, `readable` listeners socket.on = socketOnWrap; + socket.setEncoding = socketSetEncoding; // We only consume the socket if it has never been consumed before. From 06a34ebbb8775859afd2751ecd928ac86fdf85a3 Mon Sep 17 00:00:00 2001 From: Nick Soggin Date: Tue, 3 Apr 2018 11:45:11 -0400 Subject: [PATCH 7/8] docs: update ERR_HTTP_INCOMING_SOCKET_ENCODING per request --- doc/api/errors.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 0b1abf178e7788..9e9c9e6b85abdc 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -797,8 +797,8 @@ An attempt was made to add more headers after the headers had already been sent. ### ERR_HTTP_INCOMING_SOCKET_ENCODING -An attempt was made to manipulate the encoding of an incoming request packet. -This is forbidden by [RFC7230 Section 3][rfc7230-3] +An attempt to set text encoding of a HTTP request or response `Socket` +is not permitted. [RFC7230 Section 3][rfc7230-3] ### ERR_HTTP_INVALID_HEADER_VALUE From cd2c3ce6b35206c74b709840191bcff414076ee1 Mon Sep 17 00:00:00 2001 From: Nick Soggin Date: Thu, 19 Apr 2018 13:08:52 -0400 Subject: [PATCH 8/8] http: minor change per review request PR-URL: https://github.com/nodejs/node/pull/19344 --- lib/_http_server.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 0ef356ab8bac8c..b0a09785f8c430 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -384,7 +384,6 @@ function connectionListenerInternal(server, socket) { // Override on to unconsume on `data`, `readable` listeners socket.on = socketOnWrap; - socket.setEncoding = socketSetEncoding; // We only consume the socket if it has never been consumed before.