http: fix parsing of binary upgrade response body #17806

bnoordhuis · 2017-12-21T15:38:10Z

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: #17789
CI: https://ci.nodejs.org/job/node-test-commit/15003/

bnoordhuis · 2017-12-21T15:40:25Z

lib/_http_client.js

@@ -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.


Before anyone asks - yes, these could be constants, please suggest good names. :-)

(They don't have names in http_parser either though.)

Just made these up on the spot. Feel free to ignore. How about kSkipBodyAsUpgrade, kSkipBodyNoUpgrade... and I don't know for the 0 case.

bnoordhuis · 2017-12-21T15:41:11Z

lib/_http_client.js

-  // to the content-length of the entity-body had the request
-  // been a GET.
-  var isHeadResponse = req.method === 'HEAD';
-  debug('AGENT isHeadResponse', isHeadResponse);


Removed because not very useful and slightly misleading - this is not inside the agent.

bnoordhuis · 2017-12-21T15:41:57Z

lib/_http_server.js

@@ -616,7 +616,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
  } else {
    server.emit('request', req, res);
  }
-  return false; // Not a HEAD response. (Not even a response!)


Bogus comment, removed.

bnoordhuis · 2017-12-21T15:42:38Z

lib/_http_common.js

-  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.


Slightly wrong comment, removed.

richardlau · 2017-12-22T03:05:45Z

test/parallel/test-http-upgrade-binary.js

+// 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(function(conn) {


This callback and the one passed to listen could also be wrapped in common.mustCall.

cjihrig · 2017-12-22T13:58:28Z

lib/_http_client.js

@@ -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.


Just made these up on the spot. Feel free to ignore. How about kSkipBodyAsUpgrade, kSkipBodyNoUpgrade... and I don't know for the 0 case.

maclover7 · 2018-01-02T23:03:47Z

ping @bnoordhuis

joyeecheung · 2018-01-17T17:22:40Z

Is this ready to land? Or do you want to fix the nits before this PR gets landed? @bnoordhuis

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: nodejs#17789

bnoordhuis · 2018-01-19T18:27:20Z

Rebased + new CI: https://ci.nodejs.org/job/node-test-commit/15534/

I've incorporated the comments about the test. I decided to leave the constants unnamed.

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: nodejs#17789 PR-URL: nodejs#17806 Fixes: nodejs#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>

BridgeAR · 2018-01-21T00:52:31Z

Landed in de4600e

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: #17789 PR-URL: #17806 Fixes: #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>

MylesBorins · 2018-02-27T06:10:52Z

Should this be backported to LTS? It lands cleanly on 8.x but will need a manual backport to v6.x

bnoordhuis · 2018-02-27T09:06:34Z

Yes, should be back-ported. I'll look into the v6.x back-port.

jeroenvollenbrock · 2018-04-11T21:04:22Z

@gibfahn Is there any reason this one was not included in 8.10.0? I can confirm the bug persists in node 8.11.1 :)

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: #17789 PR-URL: #17806 Fixes: #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>

MylesBorins · 2018-04-11T21:09:56Z

@jeroenvollenbrock not sure why it was missed... I've landed it on staging

@bnoordhuis did you have a chance to look into the 6.x backport?

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: nodejs#17789 PR-URL: nodejs#17806 Fixes: nodejs#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>

shellscape · 2018-12-11T14:04:45Z

@lpinca @MylesBorins it looks like Node 10.14.1 has a regression and this problem has snuck back into the release. This gist can consistently reproduce the error: https://gist.github.com/shellscape/2020916f92be5d61f4d411fb6d2bce61

Note that stepping back down to any 10.x version lower than 10.14.0 does not reproduce the error.

lpinca · 2018-12-11T14:20:42Z

@shellscape none of these commits https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V10.md#2018-11-29-version-10141-dubnium-lts-mylesborins seem related.

Try to reproduce it only with core modules. This fix landed with regression tests so chances are the issue you are experiencing is different / not in node core.

shellscape · 2018-12-11T14:23:42Z

@lpinca indeed, which baffled me. we've spun up brand new containers to verify the issue was isolated to 10.14.x. same containers from scratch with 10.13.x are completely fine.

MylesBorins · 2018-12-11T14:35:01Z

I'll dig into the repro and see what's going on

shellscape · 2018-12-11T14:37:34Z

@MylesBorins much appreciated. if you end up needing that in a separate repository, say the word and it shall be done.

BridgeAR · 2018-12-11T14:42:10Z

@shellscape would you be so kind and open a new issue about this with as much information as possible? That way it's easier to track the progress :)

shellscape · 2018-12-11T14:43:29Z

@BridgeAR I'll do my best, please don't nuke it right away if there's something lacking, happy to clean it up to spec. It'll be my first Node org issue.

BridgeAR · 2018-12-11T14:50:05Z

Don't worry, we normally only close an issue if they it's resolved, not a core issue or in the wrong repository.

As @lpinca pointed out, it's best to try to reduce the test case further by removing any third party modules. That way looking into it becomes much easier.

shellscape · 2018-12-11T14:54:33Z

@BridgeAR #24958. Trying to reproduce this with core modules is a bit beyond my range of ability. Gave it a shot, but there's a lot of knowledge needed to reproduce the same conditions that I apparently don't have as yet.

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 21, 2017

bnoordhuis commented Dec 21, 2017

View reviewed changes

jasnell approved these changes Dec 21, 2017

View reviewed changes

lpinca approved these changes Dec 21, 2017

View reviewed changes

TimothyGu approved these changes Dec 21, 2017

View reviewed changes

mithunsasidharan approved these changes Dec 22, 2017

View reviewed changes

richardlau approved these changes Dec 22, 2017

View reviewed changes

cjihrig approved these changes Dec 22, 2017

View reviewed changes

lpinca mentioned this pull request Jan 6, 2018

calling verifyClient callback multiple times results in "RSV1 must be clear" errors websockets/ws#1272

Closed

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 19, 2018

BridgeAR approved these changes Jan 19, 2018

View reviewed changes

bnoordhuis force-pushed the fix17789 branch from 8b3a66e to 86ec9f5 Compare January 19, 2018 18:26

BridgeAR closed this Jan 21, 2018

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

bnoordhuis mentioned this pull request Feb 3, 2018

Process 100, 102-199 status codes according to specs. #18033

Closed

4 tasks

MylesBorins added lts-watch-v6.x labels Feb 27, 2018

bnoordhuis added the backport-requested-v6.x label Feb 27, 2018

lpinca mentioned this pull request Apr 6, 2018

Websocket RangeError: Invalid WebSocket frame: RSV2 and RSV3 must be clear websockets/ws#1354

Closed

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

shellscape mentioned this pull request Dec 11, 2018

Error: RSV1 must be clear websockets/ws#1140

Closed

shellscape mentioned this pull request Dec 11, 2018

Node 10.14.x Regression in parsing of binary upgrade response body #24958

Closed

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: fix parsing of binary upgrade response body #17806

http: fix parsing of binary upgrade response body #17806

bnoordhuis commented Dec 21, 2017 •

edited

Loading

bnoordhuis Dec 21, 2017

cjihrig Dec 22, 2017

bnoordhuis Dec 21, 2017

bnoordhuis Dec 21, 2017

bnoordhuis Dec 21, 2017

richardlau Dec 22, 2017

cjihrig Dec 22, 2017

maclover7 commented Jan 2, 2018

joyeecheung commented Jan 17, 2018

bnoordhuis commented Jan 19, 2018 •

edited

Loading

BridgeAR commented Jan 21, 2018

MylesBorins commented Feb 27, 2018

bnoordhuis commented Feb 27, 2018

jeroenvollenbrock commented Apr 11, 2018

MylesBorins commented Apr 11, 2018

shellscape commented Dec 11, 2018

lpinca commented Dec 11, 2018 •

edited

Loading

shellscape commented Dec 11, 2018

MylesBorins commented Dec 11, 2018

shellscape commented Dec 11, 2018

BridgeAR commented Dec 11, 2018 •

edited

Loading

shellscape commented Dec 11, 2018

BridgeAR commented Dec 11, 2018

shellscape commented Dec 11, 2018

http: fix parsing of binary upgrade response body #17806

http: fix parsing of binary upgrade response body #17806

Conversation

bnoordhuis commented Dec 21, 2017 • edited Loading

bnoordhuis Dec 21, 2017

Choose a reason for hiding this comment

cjihrig Dec 22, 2017

Choose a reason for hiding this comment

bnoordhuis Dec 21, 2017

Choose a reason for hiding this comment

bnoordhuis Dec 21, 2017

Choose a reason for hiding this comment

bnoordhuis Dec 21, 2017

Choose a reason for hiding this comment

richardlau Dec 22, 2017

Choose a reason for hiding this comment

cjihrig Dec 22, 2017

Choose a reason for hiding this comment

maclover7 commented Jan 2, 2018

joyeecheung commented Jan 17, 2018

bnoordhuis commented Jan 19, 2018 • edited Loading

BridgeAR commented Jan 21, 2018

MylesBorins commented Feb 27, 2018

bnoordhuis commented Feb 27, 2018

jeroenvollenbrock commented Apr 11, 2018

MylesBorins commented Apr 11, 2018

shellscape commented Dec 11, 2018

lpinca commented Dec 11, 2018 • edited Loading

shellscape commented Dec 11, 2018

MylesBorins commented Dec 11, 2018

shellscape commented Dec 11, 2018

BridgeAR commented Dec 11, 2018 • edited Loading

shellscape commented Dec 11, 2018

BridgeAR commented Dec 11, 2018

shellscape commented Dec 11, 2018

bnoordhuis commented Dec 21, 2017 •

edited

Loading

bnoordhuis commented Jan 19, 2018 •

edited

Loading

lpinca commented Dec 11, 2018 •

edited

Loading

BridgeAR commented Dec 11, 2018 •

edited

Loading