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

Add setting to control parsing of headers #1550

Closed
richdougherty opened this Issue Nov 24, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@richdougherty
Contributor

richdougherty commented Nov 24, 2017

In Play 2.6 we switched from Netty to Akka HTTP as the default HTTP backend. This has led to a number of bugs because of the different way that Akka HTTP handles headers compared to Netty. Netty headers are raw string whereas Akka HTTP uses type-safe objects for many headers. These type safe objects are parsed from the underlying header values according to the their specification.

Type-safe headers are a good idea in principle but it runs into problems when:

  1. Play users need to send/receive non-spec compliant headers (playframework/playframework#7549)
  2. Akka HTTP isn't currently spec-compliant (playframework/playframework#7515, playframework/playframework#7997, playframework/playframework#7501)
  3. Akka HTTP is spec compliant, but there is more than one way to be spec compliant and Play users need to send/receive headers in a specific way (playframework/playframework#7737, playframework/playframework#7733, #966, playframework/playframework#7498)

Akka HTTP does make it possible to use RawHeaders. Play uses RawHeaders for HTTP responses, but we can't use RawHeaders when receiving requests, since the header objects are created internally by Akka HTTP inside the header parsing code.

My suggestion is that we add a new setting to Akka HTTP to give Play (and other users) more control over how HTTP headers are parsed. Here are a few ideas for how a setting could work:

  1. A Boolean setting that switches Akka HTTP into "raw" mode - all headers are parsed as RawHeaders.
  2. A String => Boolean setting that maps header names to a Boolean value, giving more fine-grained control over which headers can be parsed into typed headers and which should be parsed as RawHeaders.
  3. A HeaderParser => HeaderParser (e.g. where type HeaderParser = (String, String) => Either[ErrorResult, HttpHeader]) setting that allows the user to override the default HeaderParser logic and patch their own logic in. This trivially allows replacing all headers with ReadHeaders. Users would also be able to continue to use typed headers and even alter the parsing logic for those headers if they need.
@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 29, 2017

Member

Thanks, @richdougherty for filing this ticket. We also thought about moving more of the header parsing out of the core to the higher levels. I guess for now (and for play's purposes) the simplest thing would be to add the simple boolean setting that disables header parsing for most headers (a few basic ones like Content-Length, Content-Type etc need to be parsed for consumption in the core).

Member

jrudolph commented Nov 29, 2017

Thanks, @richdougherty for filing this ticket. We also thought about moving more of the header parsing out of the core to the higher levels. I guess for now (and for play's purposes) the simplest thing would be to add the simple boolean setting that disables header parsing for most headers (a few basic ones like Content-Length, Content-Type etc need to be parsed for consumption in the core).

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 29, 2017

jrudolph added a commit to jrudolph/akka-http that referenced this issue Nov 29, 2017

jrudolph added a commit that referenced this issue Nov 30, 2017

Merge pull request #1577 from jrudolph/jr/w/1550-disable-header-parsi…
…ng-for-most-headers

+htc #1550 allow disabling of parsing to modeled headers

@jrudolph jrudolph added this to the 10.0.11 milestone Nov 30, 2017

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Nov 30, 2017

Member

Fixed by #1577 which introduces a flag implementing the first option of adding a setting to disable parsing of all headers.

Member

jrudolph commented Nov 30, 2017

Fixed by #1577 which introduces a flag implementing the first option of adding a setting to disable parsing of all headers.

@jrudolph jrudolph closed this Nov 30, 2017

@richdougherty

This comment has been minimized.

Show comment
Hide comment
@richdougherty

richdougherty Dec 2, 2017

Contributor

Thanks @jrudolph!

Contributor

richdougherty commented Dec 2, 2017

Thanks @jrudolph!

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