Fix handling of HTTP upgrades with bodies

[pimterry]

Fixes #58394

Previously, when processing and accepting a Upgrade request, we ignored all indicators of a body in the request (content-length or transfer-encoding headers) and treated any information following the headers as part of the upgraded stream itself.

This was not correct. An HTTP request to upgrade can have a body, and it will always indicate this with standard headers to do so. If the request had a valid HTTP body, you shouldn't treat it as part of the new protocol stream you've upgraded to.

With this change, we now fully process the requests bodies separately instead, allowing us to automatically handle correct parsing of the body like any other HTTP request.

Fixing this is a matter of persuading llhttp to parse the body like normal. The llhttp return values for on_headers_complete (i.e. this parserOnIncoming function) are (docs):

• 0: Proceed normally.
• 1: Assume that request/response has no body, and proceed to parsing the next message.
• 2: Assume absence of body (as above) and make llhttp_execute() return HPE_PAUSED_UPGRADE.

The current Node code for this basically assumes that 2 is required for to finish parsing an accepted upgrade or CONNECT, but as far as I can tell (based on this) that's not true. In this case, the upgrade flag is already set (that's req.upgrade here) and that's what controls whether the upgrade happens after the message is completed (here). Setting 2 would set the upgrade flag to true (but it's already true) and set SKIPBODY to true (which is what we're trying to fix here, and isn't required because this condition skips the body for CONNECT or no-body-headers-present for upgrades anyway).

This is a breaking change if you are currently accepting HTTP Upgrade requests with request bodies successfully, or if you use socket-specific fields & methods on the upgraded stream argument.

In the former case, before now you will have received the request body and then the upgraded data on the same stream without any distinction or HTTP parsing applied. Now, you will need to separately read the request body from the request (the 1st argument) and the upgraded data from the upgrade stream (the 2nd argument). If you're not interested in request bodies, you can continue to just read from the upgrade stream directly.

In the latter case, if you want to access the raw socket, you should do so via request.socket, instead of expecting the 2nd argument to be a socket.

---

Separately, it's debatable whether we should also actually support bodies on CONNECT requests. Even with this change, we currently don't. That would require a small change to llhttp. The latest HTTP RFCs say:

Request message framing is independent of method semantics

but also

A CONNECT request message does not have content. The interpretation of data sent after the header section of the CONNECT request message is specific to the version of HTTP in use.

so to my mind it's somewhat unspecified (as opposed to Upgrade requests, where this isn't ambiguous at all imo). Opinions welcome on whether to modify llhttp to support that.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[codecov]

Codecov Report

❌ Patch coverage is 95.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.58%. Comparing base (fbef1cf) to head (6798b70).
⚠️ Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/_http_server.js	95.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60016      +/-   ##
==========================================
- Coverage   88.58%   88.58%   -0.01%     
==========================================
  Files         704      704              
  Lines      207858   207961     +103     
  Branches    40054    40073      +19     
==========================================
+ Hits       184131   184219      +88     
- Misses      15760    15769       +9     
- Partials     7967     7973       +6     

Files with missing lines	Coverage Δ
lib/_http_server.js	96.98% <95.00%> (-0.62%)	⬇️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69381/

[lpinca]

See also de4600e. It is an old fix for upgrade responses, but I think it is related because the behavior should be the same?

[pimterry]

See also de4600e. It is an old fix for upgrade responses, but I think it is related because the behavior should be the same?

@lpinca that bug is related to client parsing of server responses, whereas this change is only for server parsing of requests, so they're not directly related, and I'm not really sure how the same issue would apply in this scenario. There's no case where the server should receive the upgraded protocol data before it has handled the request and responded anyway (because you have to wait for the 101 response to confirm the upgrade).

In general, the behaviour should not actually be the same for body parsing in these two cases. When a server is parsing a client upgrade request, there could possibly be a body, because any HTTP request of any kind could include upgrade headers. When a client is parsing a 101 response though, there can never be a body (1XX never include a body), which is why telling the parser to skip the body is correct in that fix. That previous bug seems to occur because the headers imply there is a body (transfer-encoding: chunked) even though it's not actually allowed and no valid chunk is actually sent.

Maybe I'm missing something though - can you describe the kind of request that this change would parse wrong? I have tried tweaking the test here, and it works fine when using transfer-encoding: chunked and changing the first byte to be >127.

[pimterry]

I've just pushed a new commit to fix the issue discussed above, and also rebased onto main.

With this 2nd commit, this change now subtly changes the semantics of 'upgrade' but I think it's unavoidable while fixing this bug, and the previous behaviour was clearly broken.

The issue comes when a request body is sent slowly. Previously, we always emitted 'upgrade' as soon as the headers were sent. If you do though while allowing request bodies, that means the upgrade event can happen while the body is still being processed, which means the first data you see on the socket might be the request body not the upgraded stream that you're interested in, at the same time that that data will also appear in the request body stream itself.

This is very confusing and unhelpful. It's an edge case (most upgrade requests won't have a body, especially for the common case of websockets) but it's a perfectly valid one. The only reasonable solution I can see is to defer the 'upgrade' event until the request body is done. With the change here, the server upgrade event now emits when the request has been fully received, so all data on the socket is always from the upgraded stream, and the request body only appears on the request itself.

The updated test covers this case. To compare that to the same scenario on current node, right now we fire upgrade immediately, and the request body appears split between head (for the first part) and data subsequently emitted by the socket as part of the upgrade stream. I can't see another way to fix that without changing these semantics slightly. What do you think @lpinca?

[lpinca]

The only reasonable solution I can see is to defer the 'upgrade' event until the request body is done. With the change here, the server upgrade event now emits when the request has been fully received, so all data on the socket is always from the upgraded stream, and the request body only appears on the request itself.

If I understand correctly, the issue here is that you can't read the request body until the 'upgrade' event is emitted, but the event is emitted after all data is read so there is no way for the stream to flow. The client can send a lot of data in the request body until the stream is paused on the server, but it can't be read/resumed on the server because it is not returned to the user. Am I missing something?

[pimterry]

If I understand correctly, the issue here is that you can't read the request body until the 'upgrade' event is emitted, but the event is emitted after all data is read so there is no way for the stream to flow. The client can send a lot of data in the request body until the stream is paused on the server, but it can't be read/resumed on the server because it is not returned to the user. Am I missing something?

Yes, for some reason I thought that would be fine and the parser would buffer this somehow, but I did more testing and that does indeed run into issues. Past a certain point (about 85KB on my machine) the stream stops flowing, presumably because the buffers are full, so the body never completes and upgrade never fires.

Not really sure how to resolve this:

• We can't emit the current event before the body is done, because it's really not usable in this case - you can't separate the request body from post-request data stream when reading from the socket.
• But we can't wait until after the body is done, because we might run out of buffer and never emit at all.

Some options I can see:

• We actively ignore the request body on upgrades - we dump it during parsing, so it gets fully processed (technically resolving this bug) but you can never access it.
• We could set a limit on upgrade request body size, buffer to that point and then throw/reject the request/dump the body/something else.
• We buffer the request body separately, and provide it as a 4th arg on this 'upgrade' event. Weird but workable. Still needs an upper limit really.
• We redesign the event itself, and deprecate this one, so that you don't get access to the socket directly somehow.
  • Maybe we pass an upgrade stream as arg 2, instead of the socket itself. That could be a duplex that only produces the readable data after the request body finishes. Could still have .socket to get the raw socket where required, but that would at least solve the footgun. But requires a new event, or a very significant breaking change.
• Any other ideas?

It's very tempting to ignore the request body entirely (option 1) and for websockets (easily the most common case) it's irrelevant. Technically though you could upgrade to any protocol you want, and the RFC expects you to then handle the original request (implicitly including its body) as if it was sent over the new protocol, so it's not unreasonable to actually be able to access the request body somehow.

[lpinca]

I don't know. Discarding the data without being able to access it, does not seem good. It is worse than the current behavior. Currently the user can read the request body. For example, the user can check the Content-Length header and if it is set read the specified number of bytes from the socket as the request body and then proceed with the upgrade. Something similar can also be done for chunked encoding, but yes, it requires explicit handling.

[pimterry]

For example, the user can check the Content-Length header and if it is set read the specified number of bytes from the socket as the request body and then proceed with the upgrade.

Nobody does this though. WS for example inherits the equivalent bug - if I send a websocket upgrade request with a body, WS will parses the request body as part of websocket stream, and fails to start the connection. Nobody should send that request, since it's a GET with a body, but it's as valid as any other GET-with-body and people do weird things.

I think I've reduced my options to:

1. Buffer everything before emitting upgrade, probably add an option like maxUpgradeBodySize to the server (default?) so you can limit/disable this if you don't want it. If the request body hits the max size the whole thing gets dumped and maybe we log a warning. This is easy and solves the problem. It mildly changes semantics for the affected case by waiting before emitting (but the previous behaviour for this case is broken) and adds risk of memory consumption due to automatic buffering.
2. Or, we use some kind of proxy setup to wrap a separate stream around socket, so that reading skips the request body data, and only emits the upgraded stream's data. This is complicated but seems possible, sort-of similar to how HTTP/2 uses proxies in the compat API to expose 'socket' data on top of an H2 stream. This is more complicated to do well, and probably has some small perf implications by adding another stream around the socket, but they should be minimal and we can ensure they only apply to this one case (by only using the proxy stream if there's a request body). The only scenario where it's a breaking change is cases where you're manually working around this bug by parsing the body from the socket yourself, and the fix is easy: just remove that parsing and read directly.

I'm leaning towards option 2 I think - it's more complicated, but cleaner and more effective, and gives us a sensible API: you listen for upgrade, it fires immediately when the request arrives, and then you have two separate streams (req/socket) to listen for request body data and upgrade stream data respectively. Opinions very welcome.

[pimterry]

It'd be especially interesting to know what @mscdex thinks about these solution options, since the original report in #58394 is the only concrete example of this I've seen in the wild.

[lpinca]

Nobody does this though. WS for example inherits the equivalent bug - if I send a websocket upgrade request with a body, WS will parses the request body as part of websocket stream, and fails to start the connection. Nobody should send that request, since it's a GET with a body, but it's as valid as any other GET-with-body and people do weird things.

Yes, but it can be done. The data is accessible. By properly inspecting the request headers the developer can ignore the whole request body. WS implementations do not care much because exchanging data before the opening handshake completes is forbidden by the spec.

I have no opinions, but keeping the raw socket and the ability to read it directly seems sensible.

[pimterry]

@lpinca I'm reasonably confident I now have a plan that fixes everything 🤞

The current state of things:

• We return 0 from parserOnIncoming for upgrades, which allows the request body to be parsed, if present (if content-length or transfer-encoding: chunked are set)
• In parserOnExecuteCommon, after the headers finish & we know we have an upgrade, we check if the parser considers the request to already be completed (i.e. there was no request body at all, or the whole request body was already received).
• If it's not completed, then we wrap the socket in an UpgradeStream before emitting the event immediately (not waiting for the request body - just like the existing API)
• This UpgradeStream is effectively a passthrough duplex stream, but which doesn't start passing through until the request body finishes, and which uses a Proxy so that it appears identical to the underlying socket.
• When the request does complete, if we already emitted with the upgrade stream, we don't emit again - we detach the parser etc as normal, but then just switch over the behaviour in the already-emitted stream to pass everything through (including unshifting any after-body data here).

I think this preserves existing behaviour in all cases without request bodies, exactly as it was.

In the rare cases where you do have a request body, this does change the data you'll see on the socket (which is the point - this fixes the bug). It does not buffer indefinitely or block until you stream the request body though - instead, at the moment you start reading the upgrade stream, it starts the request body flowing automatically. That means if you just read the upgrade stream and ignore the request body (probably common) everything will work as expected. If you try to read both, everything will still work as expected. The only bad case is if you try to start streaming the upgrade stream immediately, but then stream the request body later on - you'll find it's already gone. I think this is a) unavoidable and b) doesn't work in the current implementation anyway, so nobody will be doing that successfully today.

I've bulked up the new with-body test to cover various different cases here as well. To try to make sure this is non-breaking, I've also manually tested enabled UpgradeStream locally even on the already-completed case, so it's applied in all cases, and this transparently works for almost all the existing upgrade tests as well (everything passes except the listenerCount checks in test-http-connect, which seems unconcerning). Like so:

--- a/lib/_http_server.js
+++ b/lib/_http_server.js
@@ -1076,7 +1076,9 @@ function onParserExecuteCommon(server, socket, parser, state, ret, d) {
 
-      // If the request is complete (no body, or all body read upfront) then
-      // we just emit the socket directly as the upgrade stream.
-      upgradeStream = socket;
+      upgradeStream = new UpgradeStream(socket, req);
+      upgradeStream.requestBodyCompleted(undefined);

What do you think?

[pimterry]

Force pushed to rebase & resolve conflicts with latest main

[lpinca]

If I remember correctly the callback is always called asynchronously even if the write is synchronous so there will be a small overhead over a raw net.Socket in case of multiple consecutive sync writes.

[pimterry]

Yes, there is definitely a small performance hit here. I doubt it will be significant for most cases, but it's not zero.

I'm open to ideas to improve that perf, but I think it's worth it for the correctness in this case, and the intention is that it only applies in this upgrade-with-body case, so it doesn't affect any non-body upgrade requests at all.

[lpinca]

ws tests do not cover the "body headers" + body request case so the relevant branch with the changes here is not taken, but it does not matter. I'm still thinking about the stream instance of net.Socket dilemma. If we follow the semver-major road (and I think we should), we can break it now without waiting for another major.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69876/

[Ethan-Arrowood]

nice work!

[pimterry]

I'm still thinking about the stream instance of net.Socket dilemma. If we follow the semver-major road (and I think we should), we can break it now without waiting for another major.

Any more thoughts on this @lpinca? I think this is ready to merge if you're happy, but it'd be good to confirm the plan here, or decide on & implement the alternative (most likely imo: remove the proxy entirely, so that arg 2 is a simple upgrade stream if a request body is present, not a socket at all - meaning more breakage, but less magic internally).

[lpinca]

I would like to remove the proxy. The documentation clarifies when the stream argument is a net.Socket and when it is not. Anyway this is my opinion. It would be better to have more. I'm also ok with this as is. It will take more time, but we can change it later, if needed.

[pimterry]

Alright, I'm on board @lpinca - let's break things now for the greater good instead of dancing around it.

I've pushed a fix to do exactly that (updating the docs to match), and also rebased & updated the commit message to document this.

I've separately tested manually what happens if I use this upgrade stream for all cases (i.e. all the non-request-body tests too) and the current breakage there is:

• test-http-connect: listener counts for some events don't match
• test-http-parser-freed-before-upgrade: expects stream.parser to be null, it's undefined instead
• test-http-upgrade-server-callback: stream instanceof net.Socket fails

If I skip those assertions, everything else works functionally ok.

In practice I think there will be some breakage in the real world, but very little. This shouldn't affect anybody handling upgrades without request bodies (=99.9% of use cases imo), and for the few cases where request bodies are received most stream parsing code will already be broken anyway. Other than fixing the stream content, nearly everything else shouldn't break even in that scenario - it's only very socket-specific & unusual things (reading local/remote address details after upgrade, passing sockets around via proc.send()) within this specific currently-broken scenario that might hit issues.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69976/

[pimterry]

@nodejs/tsc: as semver-major, this needs two approvals to confirm the API-breaking fix we've settled on here. The PR description is a reasonable summary, my last comment covers the likely breakage in practice.

[RafaelGSS]

LGTM

[mcollina]

lgtm

[mcollina]

Adding a request change here because I fear this would add significant overhead to all websockets.

[lpinca]

There is no impact for WebSocket implementations as they continue to receive a raw socket. The spec specifies that no data should be exchanged until the handshake is complete. Only implementations where the initial GET request has a body and the Content-Length or Transfer-Encoding: chunked header are affected but a strict compliant server should reject those connections anyway.

[mcollina]

lgtm

[nodejs-github-bot]

Landed in 4346c0f