search

http4s / http4s

A minimal, idiomatic Scala interface for HTTP
https://http4s.org/
Apache License 2.0
2.56k stars 789 forks source link

Set-Cookie with leading dot in Domain attribute gets parsed as extension attribute #3178

Open aninhumer opened 4 years ago

aninhumer commented 4 years ago

Currently, if I understand correctly, the CookieHeader.DomainName parser does not accept a value for the Domain field of a Set-Cookie which has a leading dot (e.g. Domain=.example.com;): https://github.com/http4s/http4s/blob/master/core/src/main/scala/org/http4s/parser/CookieHeader.scala#L102-L104 As far as I can tell, RFCs currently say a leading dot on the Domain field should be ignored, but not rejected. (I have also seen advice to provide it for maximum compatibility as some older browsers still expect it, although this is quite old and may not be necessary any more.)

Additionally, since CookieHeader.CookieAttrs operates with a fallthrough that places anything not matched in the extension field, the domain ends up being placed there: https://github.com/http4s/http4s/blob/master/core/src/main/scala/org/http4s/parser/CookieHeader.scala#L68-L94 (Moreover, as far as I can tell, this code will also clobber any previous attribute placed there, and only retain the last such value found.)

rossabaker commented 4 years ago

I think you're right, according to RFC6265, section 5.2.3

If the first character of the attribute-value string is %x2E ("."): Let cookie-domain be the attribute-value without the leading %x2E (".") character.

We can easily make the parser ignore the leading period, but it would be a lossy conversion to the model. Or we can just accept the leading dot, and pass it along as the String value. Either of these approaches is binary compatible and could be issued in a patch. Since we're just calling it a String, I'm inclined to pass along just the String.

On master, we could introduce a newtype for the domain value that preserves the original value for transparent interop and has improved equality semantics.

aninhumer commented 4 years ago

Yeah, I think that makes sense for preserving binary compat and losslessness. I notice that RFC also says "case-insensitively matches Domain", which I don't think is the case atm?

Also, I only mentioned the extension clobbering in passing as something else I noticed while looking into this, but that actually seems like a bigger issue for losslessness!

rossabaker commented 4 years ago

Yes, and the spec is a bit of a mess.

To maximize compatibility with user agents, servers SHOULD NOT produce two attributes with the same name in the same set-cookie-string. (See Section 5.3 for how user agents handle this case.)

But then I don't see what 5.3 says to do about it. I guess I'm not so concerned about preserving two max-values, but preserving multiple extensions seems necessary. Maybe can smuggle all the leftovers into the extension String, and since semi-colon is illegal, delimit multiple ones with a semi-colon.

rossabaker commented 4 years ago

These issues have a binary-compatible fix in #3184

Leaving this open to consider a better model for the cookie, so we're not punting these semantics to the end user.

rossabaker commented 4 years ago

Demilestoning. The bincompat fix will be in 0.20.18.

rossabaker commented 3 years ago

Moving further with this is a breaking change, so decide before 1.0.