Repository navigation
http2/http1 compatibility API error with connection header #23748
Description
Activity
This is a tricky one. The
connectionheader 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
Reacted by Sebastiaan Deckers and Pranshu SrivastavaWe 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?
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)
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
I think we should change the behavior. This looks more like a bugfix to me than semver-major.
I think it is better to ignore the connection in the header + log a warning.
I can pick this one.#23908 is finished and waiting
- added a commit that references this issue
on Dec 15, 2018 - added a commit that references this issue
on Mar 4, 2019 - added a commit that references this issue
on Apr 16, 2019
Original Issue: hapijs/hapi#3830
What you expect:
No error should be thrown. The
connectionheader should be ignored by the http2 module.What actually happens:
An error is thrown.
Description:
ERR_HTTP2_INVALID_CONNECTION_HEADERSerror seems to break the http2 compatibility API. By adding aconnectionheader to an http2 response, theERR_HTTP2_INVALID_CONNECTION_HEADERSerror is thrown. Seeing as http2 is supposed to be backwards compatible with http1, shouldn't http2 just ignore theconnectionheader?MDN states: