node servers should handle upgrade headers gracefully

[mosesn]

Today, node servers cut the connection when they receive upgrade headers, which isn't advised in the http/1.1 or http/2 specs. As http/2 gains a wider mindshare, h2c will see an uptick in adoption in intra-datacenter use, so if node wants to continue being viable in microservice architectures, it makes sense to at least play nicely with h2c clients, even if it doesn't want to implement h2c by default.

Instead, node servers should ignore the upgrade header and treat it as a normal request if the implementor hasn't defined anything special to do to it.

[mscdex]

I don't see anything in the linked http/1.1 spec section that says how to properly deny a connection upgrade request (from a client). It does mention "ignoring" the Upgrade header, and maybe that's badly worded, but to me that's different than ignoring a connection upgrade request which is both an Upgrade header and Connection: upgrade.

[dougwilson]

All Connection: upgrade means is that the Upgrade header is specific to that connection; it doesn't change the meaning of the request (unless you're a proxy). This is defined in https://tools.ietf.org/html/rfc7230#section-6.1 . An Upgrade header will almost always be combined with Connection: Upgrade so a proxy does not forward it on and then get confused when the server tries to hijack the socket with a different protocol.

The HTTP specs are very large, can you point where in the spec the difference between having an Upgrade header and what an "upgrade request" is? Requests with Upgrade: h2c and Connection: Upgrade are becoming more common due to http/2 clients that understand plan http/2.

[jasnell]

Once the basic http/2 implementation is done, I do plan to revisit the way upgrade is handled currently so that negotiation between http/1 and http/2 is possible. There are going to be some quirks to deal with due to differences between the APIs, but it is definitely on my radar.

[mosesn]

That sounds great! As we discussed on Twitter, I think it might make sense to have an intermediate step where node still can't upgrade to h2c but can at least speak HTTP/1.1 to h2c clients that send upgrade headers by default.

[valler]

I found this issue after reading the docs on upgrade.

Emitted each time a server responds to a request with an upgrade. If this event is not being listened for, clients receiving an upgrade header will have their connections closed.

To my understanding, what should trigger a client to react is when the server responds with:

• 101 (Switching Protocols) and an upgrade hasn't been requested. The client may or may not be listening for the upgrade event in this scenario.
• An upgrade header in combination with a status other than 101, for clients that do explicitly try to invoke an upgrade. Here the client is typically listening for the event.

The event should not fire, and the connection should close in both cases.

What actually matters is, if the upgrade has been requested with this particular request, and whether or not the server responds with a valid 101,upgrade,connection combination.

My first comment on GitHub, so, hello everybody.

[mosesn]

@jasnell have you had a chance to revisit this? I believe node still breaks with h2c clients.

[mosesn]

@jasnell now that you've landed h2, can we revisit h2c?

[apapirovski]

Can this be revisited by any chance? Even if we ignore the implementation details of h2c switching — the current http behaviour is completely against the spec.

The "Upgrade" header field is intended to provide a simple mechanism
for transitioning from HTTP/1.1 to some other protocol on the same
connection. A client MAY send a list of protocols in the Upgrade
header field of a request to invite the server to switch to one or
more of those protocols, in order of descending preference, before
sending the final response. A server MAY ignore a received Upgrade
header field if it wishes to continue using the current protocol on
that connection. Upgrade cannot be used to insist on a protocol
change.

As @dougwilson mentioned above, Connection only refers to the fact that the Upgrade header is for the current connection and not that the server shouldn't ignore the Upgrade header.

It seems like this would ideally be changed so that 'upgrade' is emitted but not required to be listened to (and the connection is never cut).

I can't imagine much would break if this change were implemented whereas it's pretty certain that the current behaviour already breaks things.

[grantila]

The RFC states regarding a client sending the Upgrade header; "Upgrade cannot be used to insist on a protocol change." It is up to the server to decide.

My suggestion for this to work both client-side and server-side in Node, is to allow http1 and http2 (or at least http1) sessions (connections) to be detachable from their socket, and leave this flow up to user code to handle, if at all.

Client-side

It is almost possible to convert an upgraded client-socket (after having gotten the 'upgrade' event) into an Http2Session using what's described by @apapirovski in #16256. The problem is the head buffer. It would be nicer if Node could virtually unshift this back on the socket. Otherwise, we need a more robust way to create an Http2Session from a [socket, head] pair.

Server-side

When you get the 'upgrade' event, it is (should be) up to the user to decide whether to upgrade or not. What if the socket is not detached, but the user can choose to synchronously detach the socket in this event? If it isn't detached when the event handler returns to Node, Node replies normally and the session is still http1 (Node will have to use the head buffer internally). If the user detached it, the user is also responsible for sending the right response (101 headers and two newlines), and then forward the socket to whatever protocol handler. Again, it could create an Http2Session from the socket potentially, with the same caveat as in the Client-side; we might need a [socket, head] pair.

EDIT: Maybe introduce a 'beforeUpgrade' event for the above to keep compatibility.

Care must be taken if Upgrade was in a non-GET method (is this allowed/done/possible?), where the server must first read the entire body before internally switching over to another protocol handler...

[mosesn]

@grantila I don't really care how this is resolved as long as the default is no longer that node hangs up if a client sends it an upgrade header.

[grantila]

@mosesn the problem is, the rest of us do care

[mosesn]

@grantila sorry for being grumpy, I filed this ticket seven months ago to fix a specific issue I'm running into, and I've been totally unable to get anybody to agree on a fix to it. So I think your solution seems reasonable but it's not clear to me that it fixes the fundamental issue in this ticket, which is that unconfigured node servers hang up when they receive upgrade headers.

[apapirovski]

@nodejs/http @nodejs/http2 I would like to at least discuss this before v10.x lands because this will almost certainly be semver-major and I don't want it to wait until yet another major version.

[addaleax]

@apapirovski To be clear, the suggestion is to ignore such a header by default? Or do we actually want to implement a transparent switch to HTTP/2 (which might be doable)?

[addaleax]

Also @nodejs/tsc because the deadline that we set for semver-majors in 10.0.0 without TSC confirmation has passed

[apapirovski]

@addaleax the suggestion is to conform to the spec which says that upgrade headers can be ignored whereas we just cut the connection. The handling of connection: upgrade in particular is super broken.

[addaleax]

I’m totally fine with ignoring the header by default, but we (you? me?) should get a PR up asap.

We might also want to provide an option to fall back to the old behaviour, I guess?

[apapirovski]

Yeah, sorry been unavailable for a while at an inconvenient time but I've had a list of these things that I think need to get into 10.x. I have a few more that I'm working on PRs for, hopefully can open today or tomorrow.

[mcollina]

@lpinca what do you think?

@apapirovski do we have a fix for this already? If we do, we might try to fit it in before 10.0.0, but the deadline has passed already. Ideally it should be landed before next Wednesday, so that we can talk about the backport at the next TSC meeting. However, we cannot promise anything at this point.

[apapirovski]

@addaleax @mcollina I'll get a PR going today.

[lpinca]

I think it's fine to ignore if there is no handler but I'm not sure if and how much code it will break. I guess not much.

[apapirovski]

Ignore me. Upon another read through of the spec, looks like we're all good with http_parser.

[apapirovski]

PR in #19981