Skip to content
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

http2/http1 compatibility API error with connection header #23748

Closed
FallingSnow opened this issue Oct 19, 2018 · 7 comments
Closed

http2/http1 compatibility API error with connection header #23748

FallingSnow opened this issue Oct 19, 2018 · 7 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@FallingSnow
Copy link

  • Version: v10.11.0
  • Platform: Linux archlinux 4.18.14-1-ck-skylake deps: update openssl to 1.0.1j #1 SMP PREEMPT Sat Oct 13 16:38:26 EDT 2018 x86_64 GNU/Linux
  • Subsystem: http2

Original Issue: hapijs/hapi#3830

What you expect:

No error should be thrown. The connection header should be ignored by the http2 module.

What actually happens:

An error is thrown.

Description:

ERR_HTTP2_INVALID_CONNECTION_HEADERS error seems to break the http2 compatibility API. By adding a connection header to an http2 response, the ERR_HTTP2_INVALID_CONNECTION_HEADERS error is thrown. Seeing as http2 is supposed to be backwards compatible with http1, shouldn't http2 just ignore the connection header?

MDN states:

Also, Connection and Keep-Alive are ignored in HTTP/2; connection management is handled by other mechanisms there.

@jasnell
Copy link
Member

jasnell commented Oct 19, 2018

This is a tricky one. The connection header is forbidden by HTTP/2 for any value other than 'trailers'. The spec, however, is not clear on whether it is an terminal error or not. Silently ignoring it if it is set to any other value is quite risky and could violate various assumptions and I believe that nghttp2 will reject automatically if it's there (I will verify that belief later on tonight).

HTTP/2 is definitely not backwards compatible with HTTP/1. The compat API is a best attempt to get it as close as possible. Will have to think a bit on how to handle this.

/cc @nodejs/http2

@apapirovski
Copy link
Member

We could just log a warning once on the compatibility side and ignore the header (not pass it on). That would probably make the most sense here?

@sebdeckers
Copy link
Contributor

The compatibility API could be viewed as an "intermediary":

8.1.2.2. Connection-Specific Header Fields

HTTP/2 does not use the Connection header field to indicate connection-specific header fields; in this protocol, connection-specific metadata is conveyed by other means. An endpoint MUST NOT generate an HTTP/2 message containing connection-specific header fields; any message containing connection-specific header fields MUST be treated as malformed (Section 8.1.2.6).

The only exception to this is the TE header field, which MAY be present in an HTTP/2 request; when it is, it MUST NOT contain any value other than "trailers".

This means that an intermediary transforming an HTTP/1.x message to HTTP/2 will need to remove any header fields nominated by the Connection header field, along with the Connection header field itself. Such intermediaries SHOULD also remove other connection-specific header fields, such as Keep-Alive, Proxy-Connection, Transfer-Encoding, and Upgrade, even if they are not nominated by the Connection header field.

(Emphasis added)

@jasnell
Copy link
Member

jasnell commented Oct 19, 2018

Emitting a warning could be ok. I wonder if a config option would be better tho? Changing the behavior now would be semver-major, adding an option would be semver-minor

@mcollina
Copy link
Member

I think we should change the behavior. This looks more like a bugfix to me than semver-major.

@sagitsofan
Copy link
Contributor

sagitsofan commented Oct 19, 2018

I think it is better to ignore the connection in the header + log a warning.
I can pick this one.

@sagitsofan
Copy link
Contributor

sagitsofan commented Nov 30, 2018

#23908 is finished and waiting

sagitsofan added a commit to sagitsofan/node that referenced this issue Dec 15, 2018
Ignoring the connection header and disable the
`ERR_HTTP2_INVALID_CONNECTION_HEADERS` error.

Added a warning log on the compatibility.

Fixes: nodejs#23748
BridgeAR pushed a commit that referenced this issue Mar 4, 2019
When using the compatibility API the connection header is from now on
ignored instead of throwing an `ERR_HTTP2_INVALID_CONNECTION_HEADERS`
error.
This logs a warning in such case to notify the user about the ignored
header.

PR-URL: #23908
Fixes: #23748
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this issue Mar 5, 2019
When using the compatibility API the connection header is from now on
ignored instead of throwing an `ERR_HTTP2_INVALID_CONNECTION_HEADERS`
error.
This logs a warning in such case to notify the user about the ignored
header.

PR-URL: #23908
Fixes: #23748
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this issue Apr 16, 2019
When using the compatibility API the connection header is from now on
ignored instead of throwing an `ERR_HTTP2_INVALID_CONNECTION_HEADERS`
error.
This logs a warning in such case to notify the user about the ignored
header.

PR-URL: #23908
Fixes: #23748
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
6 participants