Skip to content

http2/http1 compatibility API error with connection header #23748

Closed
Closed
http2/http1 compatibility API error with connection header#23748
Labels
http2Issues and PRs related to the http2 subsystem.

Description

@FallingSnow
FallingSnow
opened on Oct 19, 2018
  • 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.

Activity

  1. jasnell commented on Oct 19, 2018

    @jasnell
    jasnell
    on Oct 19, 2018
    Member

    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

  2. apapirovski commented on Oct 19, 2018

    @apapirovski
    apapirovski
    on Oct 19, 2018
    Contributor

    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?

  3. sebdeckers commented on Oct 19, 2018

    @sebdeckers
    sebdeckers
    on Oct 19, 2018
    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)

  4. jasnell commented on Oct 19, 2018

    @jasnell
    jasnell
    on Oct 19, 2018
    Member

    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

  5. mcollina commented on Oct 19, 2018

    @mcollina
    mcollina
    on Oct 19, 2018
    SponsorMember

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

  6. sagitsofan commented on Oct 19, 2018

    @sagitsofan
    sagitsofan
    on Oct 19, 2018
    Contributor

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

  7. jasnell
    added
    http2Issues and PRs related to the http2 subsystem.
    on Oct 19, 2018

  8. sagitsofan commented on Nov 30, 2018

    @sagitsofan
    sagitsofan
    on Nov 30, 2018
    Contributor

    #23908 is finished and waiting

  9. sagitsofan
    added a commit that references this issue on Dec 15, 2018

    lib: http2 compatibility error with connection header

    685d968
  10. BridgeAR
    added a commit that references this issue on Mar 4, 2019

    http2: improve compatibility with http/1

    8c597df
  11. BridgeAR
    closed this as completedin 8c597dfon Mar 4, 2019
  12. BethGriggs
    added a commit that references this issue on Apr 16, 2019

    http2: improve compatibility with http/1

    cc25f22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    http2Issues and PRs related to the http2 subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions