diff options
author | Ben Noordhuis <info@bnoordhuis.nl> | 2017-12-21 16:27:39 +0100 |
---|---|---|
committer | Ruben Bridgewater <ruben@bridgewater.de> | 2018-01-21 01:50:56 +0100 |
commit | de4600ea2bf2eb5a1367825dc84ea7561b0c3529 (patch) | |
tree | b10031503112b039aa049e42951e5e0b8772f40a | |
parent | e1c29f2c529ffdbf9cf8f05d4ed27ccfcede2719 (diff) | |
download | android-node-v8-de4600ea2bf2eb5a1367825dc84ea7561b0c3529.tar.gz android-node-v8-de4600ea2bf2eb5a1367825dc84ea7561b0c3529.tar.bz2 android-node-v8-de4600ea2bf2eb5a1367825dc84ea7561b0c3529.zip |
http: fix parsing of binary upgrade response body
Fix a bug where a connection upgrade response with a Transfer-Encoding
header and a body whose first byte is > 127 causes said byte to be
dropped on the floor when passing the remainder of the message to
the 'upgrade' event listeners.
Fixes: https://github.com/nodejs/node/issues/17789
PR-URL: https://github.com/nodejs/node/pull/17806
Fixes: https://github.com/nodejs/node/issues/17789
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
-rw-r--r-- | lib/_http_client.js | 24 | ||||
-rw-r--r-- | lib/_http_common.js | 15 | ||||
-rw-r--r-- | lib/_http_server.js | 2 | ||||
-rw-r--r-- | test/parallel/test-http-upgrade-binary.js | 28 |
4 files changed, 41 insertions, 28 deletions
diff --git a/lib/_http_client.js b/lib/_http_client.js index e2aaf86a5e..b49aa711c4 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -458,7 +458,6 @@ function parserOnIncomingClient(res, shouldKeepAlive) { var socket = this.socket; var req = socket._httpMessage; - // propagate "domain" setting... if (req.domain && !res.domain) { debug('setting "res.domain"'); @@ -471,29 +470,22 @@ function parserOnIncomingClient(res, shouldKeepAlive) { // We already have a response object, this means the server // sent a double response. socket.destroy(); - return; + return 0; // No special treatment. } req.res = res; // Responses to CONNECT request is handled as Upgrade. - if (req.method === 'CONNECT') { + const method = req.method; + if (method === 'CONNECT') { res.upgrade = true; - return 2; // skip body, and the rest + return 2; // Skip body and treat as Upgrade. } - // Responses to HEAD requests are crazy. - // HEAD responses aren't allowed to have an entity-body - // but *can* have a content-length which actually corresponds - // to the content-length of the entity-body had the request - // been a GET. - var isHeadResponse = req.method === 'HEAD'; - debug('AGENT isHeadResponse', isHeadResponse); - if (res.statusCode === 100) { // restart the parser, as this is a continue message. req.res = null; // Clear res so that we don't hit double-responses. req.emit('continue'); - return true; + return 1; // Skip body but don't treat as Upgrade. } if (req.shouldKeepAlive && !shouldKeepAlive && !req.upgradeOrConnect) { @@ -503,7 +495,6 @@ function parserOnIncomingClient(res, shouldKeepAlive) { req.shouldKeepAlive = false; } - DTRACE_HTTP_CLIENT_RESPONSE(socket, req); LTTNG_HTTP_CLIENT_RESPONSE(socket, req); COUNTER_HTTP_CLIENT_RESPONSE(); @@ -521,7 +512,10 @@ function parserOnIncomingClient(res, shouldKeepAlive) { if (!handled) res._dump(); - return isHeadResponse; + if (method === 'HEAD') + return 1; // Skip body but don't treat as Upgrade. + + return 0; // No special treatment. } // client diff --git a/lib/_http_common.js b/lib/_http_common.js index 6ad1310477..b4caf5939e 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -106,19 +106,10 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, parser.incoming.upgrade = upgrade; - var skipBody = 0; // response to HEAD or CONNECT + if (upgrade) + return 2; // Skip body and treat as Upgrade. - if (!upgrade) { - // For upgraded connections and CONNECT method request, we'll emit this - // after parser.execute so that we can capture the first part of the new - // protocol. - skipBody = parser.onIncoming(parser.incoming, shouldKeepAlive); - } - - if (typeof skipBody !== 'number') - return skipBody ? 1 : 0; - else - return skipBody; + return parser.onIncoming(parser.incoming, shouldKeepAlive); } // XXX This is a mess. diff --git a/lib/_http_server.js b/lib/_http_server.js index dee0de74bc..c60119822a 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -627,7 +627,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) { } else { server.emit('request', req, res); } - return false; // Not a HEAD response. (Not even a response!) + return 0; // No special treatment. } function resetSocketTimeout(server, socket, state) { diff --git a/test/parallel/test-http-upgrade-binary.js b/test/parallel/test-http-upgrade-binary.js new file mode 100644 index 0000000000..002ac9c564 --- /dev/null +++ b/test/parallel/test-http-upgrade-binary.js @@ -0,0 +1,28 @@ +'use strict'; +const { mustCall } = require('../common'); +const assert = require('assert'); +const http = require('http'); +const net = require('net'); + +// https://github.com/nodejs/node/issues/17789 - a connection upgrade response +// that has a Transfer-Encoding header and a body whose first byte is > 127 +// triggers a bug where said byte is skipped over. +net.createServer(mustCall(function(conn) { + conn.write('HTTP/1.1 101 Switching Protocols\r\n' + + 'Connection: upgrade\r\n' + + 'Transfer-Encoding: chunked\r\n' + + 'Upgrade: websocket\r\n' + + '\r\n' + + '\u0080', 'latin1'); + this.close(); +})).listen(0, mustCall(function() { + http.get({ + host: this.address().host, + port: this.address().port, + headers: { 'Connection': 'upgrade', 'Upgrade': 'websocket' }, + }).on('upgrade', mustCall((res, conn, head) => { + assert.strictEqual(head.length, 1); + assert.strictEqual(head[0], 128); + conn.destroy(); + })); +})); |