akka / akka-http

The Streaming-first HTTP server/module of Akka
https://doc.akka.io/docs/akka-http
Other
1.34k stars 594 forks source link

Cookies are not parsed when modeled-header-parsing = off #3163

Open REASY opened 4 years ago

REASY commented 4 years ago

Akka HTTP Server in case if the parameter akka.http.parsing.modeled-header-parsing = off will not parse cookie header into the Cookie type. This will cause CookieDirectives to return None every time when Cookie type is expected: cookie and optionalCookie directives won't work because they rely on findCookie which expects Cookie type.

Should this be better documented? Or it is a bug and cookie has to be correctly parsed no matter what akka.http.parsing.modeled-header-parsing says?

raboof commented 4 years ago

Should this be better documented?

Can't hurt to clarify that when you disable that option some directives (that rely on those parsed headers) will no longer work

Or it is a bug and cookie has to be correctly parsed no matter what akka.http.parsing.modeled-header-parsing says?

I think it's working as originally intended - but that doesn't mean it couldn't be improved. I'm not sure if it'd be worth it to make those directives so 'smart' that they will parse the 'raw' headers themselves in case modeled-header-parsing is turned off, but it might be worth a try?

bc-jz commented 4 years ago

I have come across this issue as well. I would like to disable modeled header parsing due to the limitation of the Strict-Transport-Security ModeledHeader not being able to process the "preload" hsts value. However, in doing so, I can no longer make use of the Cookie ModeledHeader which means I need to write my own methods to read the Cookie as a RawHeader instead of using cookie() or optionalCookie().

I would like to still be able to have the Cookie parsed even if modeled header parsing is disabled because I like how the akka parsing behavior turns the request header Cookie value into a Seq of HttpCookiePair objects and removes invalid cookies automatically.

@raboof As a resolution for this I wanted to see if you would consider either: 1) Add Cookie to the list of headers that are always parsed: https://github.com/akka/akka-http/blob/78ec8a81f9c83e7d1d24ff3a34507ff31ad70d64/akka-http-core/src/main/scala/akka/http/impl/engine/parsing/HttpHeaderParser.scala#L447-L459

or

2) Add a new setting for always_parse_cookie_header or something like that, which could then be utilized within the logic that generates the filter for the list of headers to parse: https://github.com/akka/akka-http/blob/78ec8a81f9c83e7d1d24ff3a34507ff31ad70d64/akka-http-core/src/main/scala/akka/http/impl/engine/parsing/HttpHeaderParser.scala#L475-L476

In the above logic we could have a 3rd condition where if parser.settings.modeledHeaderParsing is false but parser.settings.alwaysParseCookieHeader is true, then the value returned is: alwaysParsedHeaders ++ "cookie"

raboof commented 4 years ago

I wonder if we could create a directive that selectively converts headers from raw to modeled? That would be less invasive, and would help for all other header types as well.

jrudolph commented 4 years ago

Why do you turn off modeled-header-parsing in the first place? Even if parsing fails a RawHeader should be created. Shouldn't that sidestep issues with the Strict-Transport-Security header?

That said...

I wonder if we could create a directive that selectively converts headers from raw to modeled? That would be less invasive, and would help for all other header types as well.

I would even go further and make header parsing lazy as much as possible and move the logic into HttpMessage to make it work.

bc-jz commented 4 years ago

@jrudolph Turning off modeled-header-parsing was done to prevent the header's value from getting altered by akka in the header parsing process. The parsing does not fail, it does create a ModeledHeader for Strict-Transport-Security but it will drop the "preload" value, ex:

Strict-Transport-Security: max-age=3153600; includeSubDomains; preload

This is an aside but, the ModeledHeader version of ^that^ only stores the max-age and includeSubDomains information losing the "preload" setting. I am running into this when making an http request using AkkaHttpClient. The request method on that client will return an HttpResponse object that has already undergone header parsing. I did not find any easy way to short circuit the behavior here to allow me to read the Strict-Transport-Security header in a raw form before parsing, but maybe that is possible.

This issue has been brought up a few times in the past but each time the fix was decided to ignore the "preload" value:

https://github.com/akka/akka/pull/20275#issuecomment-210683220 https://github.com/akka/akka/issues/20271#issuecomment-210463189

This is apparently due to the difficulties with keeping binary compatibility with the change, as you mention here: https://github.com/akka/akka-http/issues/3220#issuecomment-638876033

Ultimately I am just trying to find the easiest way to not lose track of the "preload" value on the Strict-Transport-Security header. Ideally I would keep modeled-header-parsing enabled and just have Strict-Transport-Security come through with all 3 possible HSTS values or as a RawHeader.

Getting back on topic of this thread, my best solution right now is to turn off modeled-header-parsing but that also means that the Cookie header no longer gets parsed which makes things more difficult. It is totally possibly to write some parsing code to pull the values I need from the Cookie as a RawHeader but it'd but great if I could have a way to still parse specifically the Cookie header as mentioned by the OP of this thread. If there is a more general solution that can be applied than what I suggested ( @raboof )then I would have no problem with that.

Thanks for taking a look at this.

jrudolph commented 4 years ago

This is an aside but, the ModeledHeader version of ^that^ only stores the max-age and includeSubDomains information losing the "preload" setting. I am running into this when making an http request using AkkaHttpClient. The request method on that client will return an HttpResponse object that has already undergone header parsing. I did not find any easy way to short circuit the behavior here to allow me to read the Strict-Transport-Security header in a raw form before parsing, but maybe that is possible.

Understood, that's unhelpful, indeed. I guess, we need a guideline how to deal with these kind of decisions. We do not support full character-to-character roundtrips for parsed header but we should at least support a "semantic roundtrip" where every information is preserved when a header was parsed. Created #3474 to track the bigger issue around that.

It is totally possibly to write some parsing code to pull the values I need from the Cookie as a RawHeader but it'd but great if I could have a way to still parse specifically the Cookie header as mentioned by the OP of this thread.

Ah, so there seems to another problem, that modeled-header-parsing also prevents using HttpHeader.parse and ModeledCompanion.parseFromValueString. I think this is something we could somewhat more easily fix so that you never lose the possibility to force parsing a header.

jrudolph commented 4 years ago

Ah, so there seems to another problem, that modeled-header-parsing also prevents using HttpHeader.parse and ModeledCompanion.parseFromValueString. I think this is something we could somewhat more easily fix so that you never lose the possibility to force parsing a header.

Ah, no, I was wrong about that. You should be able to either parse the cookie header using

HttpHeader.parse(rawHeader.name, rawHeader.value)

or using

Cookie.parseFromValueString(rawHeader.value)

Can you try if that works?

bc-jz commented 4 years ago

@jrudolph Thank you! That is my ignorance not finding this syntax to access the Akka parsers, but Cookie.parseFromValueString() is exactly what I wanted. I can use that to convert the RawHeader cookie value string into it's Seq[HttpCookiePair]. I'll still have to disable modeled-header-parsing in general but this makes it a lot smoother.

I am going to handle this with mapRequest() and a simple method to read the request headers and convert a RawHeader "Cookie" into a Cookie modeled header.

I do still support having more control with regards to accessing the raw header values with modeled headers enabled for situations like with Strict-Transfer-Security. I'd prefer to not have to disable modeling in general to handle it. I very much appreciate your feedback and for creating https://github.com/akka/akka-http/issues/3474 to look at this header problem in general.