New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NodeJS disconnects when "Upgrade" HTTP header is used for example to advertise HTTP/2 support #4334

Closed
bazzadp opened this Issue Dec 17, 2015 · 11 comments

Comments

Projects
None yet
7 participants
@bazzadp
Copy link

bazzadp commented Dec 17, 2015

More details in this issue: icing/mod_h2#73 as this was originally raised with the Apache HTTP/2 project but they have identified this as a Node issue.

Nodejs https (and http) fails to parse certain HTTP responses with an Upgrade header.

This fails:

HTTP/1.1 200 OK
Date: Thu, 17 Dec 2015 15:17:46 GMT
Server: Apache/2.5.0-dev (Unix) OpenSSL/1.0.2e
Upgrade: h2
Connection: Upgrade, Keep-Alive
Last-Modified: Thu, 17 Dec 2015 15:17:39 GMT
ETag: "7d5-5271985fa6ac0"
Accept-Ranges: bytes
Content-Length: 2005
Keep-Alive: timeout=5
Content-Type: text/html

while this answer succeeds:

HTTP/1.1 200 OK
Date: Thu, 17 Dec 2015 15:19:59 GMT
Server: Apache/2.5.0-dev (Unix) OpenSSL/1.0.2e
Last-Modified: Thu, 17 Dec 2015 15:19:56 GMT
ETag: "7d5-527198e24df00"
Accept-Ranges: bytes
Content-Length: 2005
Keep-Alive: timeout=5
Connection: Keep-Alive
Content-Type: text/html

Either it is the Upgrade response header or the Upgrade token in the Connection.

To repeat, make a call my site (www.tunetheweb.com) using standard https call like below:

var https = require('https');

var options = {
  hostname: 'www.tunetheweb.com',
  path: '/',
};

var req = https.request(options, function(res) {
  console.log("statusCode: ", res.statusCode);
  console.log("headers: ", res.headers);

  res.on('data', function(d) {
    process.stdout.write(d);
  });
});
req.end();

req.on('error', function(e) {
  console.log(e);
});

Apache 2.4.18 introduced these headers when using HTTP/2 through the mod_http2 module and NodeJS is now unable to connect to any Apache server using HTTP/2. NodeJS was able to connect using Apache 2.4.17 even when H2 was supported as the upgrade headers were not advertised.

Upgrade headers are part of the HTTP spec (http://www.w3.org/Protocols/rfc2616/rfc2616.txt) and Node should ignore any unsupported upgrade protocol suggestions and continue using HTTP/1.1. Instead it disconnects.

Thanks,
Barry

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Dec 17, 2015

@indutny

This comment has been minimized.

Copy link
Member

indutny commented Dec 17, 2015

@bazzadp Hopefully, I haven't messed up reading HTTP2 spec, but it seems that this is described here:

https://http2.github.io/http2-spec/#rfc.section.3.2

From what I could tell the client MUST be the first party to send the Upgrade header. So I am afraid that this is Apache HTTP server that deviating from protocol here.

Here is the quote from the RFC 2616 that you have linked here:

The server understands and is willing to comply with the client's
   request, via the Upgrade message header field (section 14.42), for a
   change in the application protocol being used on this connection. The
   server will switch protocols to those defined by the response's
   Upgrade header field immediately after the empty line which
   terminates the 101 response.

This means that after server sends Upgrade - it MUST switch to the advertised protocol. So if you do a regular HTTP request and receive Upgrade in response headers, you expect that all data that comes next won't be HTTP/1.1, and thus node.js fails to parse it (because there is no HTTP2 support in core).

I'm afraid that this issue is won't fix, and should be fixed in mod_h2 instead. Please let me know if you think I am wrong (don't forget to include some arguments, though!).

Thanks!

@indutny indutny closed this Dec 17, 2015

@bazzadp

This comment has been minimized.

Copy link

bazzadp commented Dec 17, 2015

Hi @indutny, thanks for quick response.

Apologies but the link I included was from an old version of the HTTP/1.1 spec (2616). This has been superseded by this one: http://tools.ietf.org/html/rfc7230#section-6.7

7230 includes this section:

A server MAY send an Upgrade header field in any other response to
advertise that it implements support for upgrading to the listed
protocols, in order of descending preference, when appropriate for a
future request.

Now admittedly 2616 didn't include this paragraph but does look like by that, current, definition the server MAY advertise alternative protocol support over a HTTP/1.1 connection in the way Apache is doing.

Thoughts?

@indutny

This comment has been minimized.

Copy link
Member

indutny commented Dec 17, 2015

@bazzadp looks like you are right! Thanks for sharing. I will need some time to think about how it could be fixed.

indutny added a commit to indutny/io.js that referenced this issue Dec 17, 2015

http: do not emit `upgrade` on advertisement
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
@indutny

This comment has been minimized.

Copy link
Member

indutny commented Dec 17, 2015

Should be fixed by #4337, I guess?

indutny added a commit that referenced this issue Dec 18, 2015

http: do not emit `upgrade` on advertisement
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@djc

This comment has been minimized.

Copy link

djc commented Jan 27, 2016

Given the problems this causes, e.g. in mozilla/persona#4231 and keybase/keybase-issues#1925, it would be nice if this gets backported to any open LTS branches.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Jan 27, 2016

@djc There was some trepidation expressed in #4337 that the fix may be backwards incompatible. As such, it's not eligible for inclusion in LTS because the goal for LTS is to never ever break working code, security issues excepted.

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Jan 27, 2016

@bnoordhuis I'd say it breaks working code if the server starts supporting HTTP2. In this case we probably should stick to the spec.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Jan 27, 2016

It's not me you need to convince, I'm just the messenger.

@djc

This comment has been minimized.

Copy link

djc commented Jan 27, 2016

@bnoordhuis so what's the best venue if I want to try and do some convincing?

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Jan 27, 2016

@djc Convince the people that voiced concerns in the linked issue of the error of their ways. :-)

rvagg added a commit that referenced this issue Feb 10, 2016

http: do not emit `upgrade` on advertisement
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit to MylesBorins/node that referenced this issue Mar 1, 2016

http: do not emit `upgrade` on advertisement
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
PR-URL: nodejs#4337
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this issue Mar 1, 2016

http: do not emit `upgrade` on advertisement
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this issue Mar 2, 2016

http: do not emit `upgrade` on advertisement
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: #4334
PR-URL: #4337
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016

http: do not emit `upgrade` on advertisement
Do not emit `upgrade` if the server is just advertising its protocols
support as per RFC 7230 Section 6.7.

    A server MAY send an Upgrade header field in any other response
    to advertise that it implements support for upgrading to the
    listed protocols, in order of descending preference, when
    appropriate for a future request.

Fix: nodejs#4334
PR-URL: nodejs#4337
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment