Skip to content
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

Allow optional spaces when separating cookie value from directives #5196

Merged
merged 4 commits into from Sep 15, 2021
Merged

Allow optional spaces when separating cookie value from directives #5196

merged 4 commits into from Sep 15, 2021

Conversation

alejandrohdezma
Copy link
Contributor

@alejandrohdezma alejandrohdezma commented Sep 15, 2021

What has been done here?

Allow optional spaces when separating cookie value from directives

Why?

According to rfc-6265 spaces after ; separating a set-cookie directives are optional:

This specification uses the Augmented Backus-Naur Form (ABNF)
notation of [RFC5234].

The following core rules are included by reference, as defined in
[RFC5234], Appendix B.1: ALPHA (letters), CR (carriage return), CRLF
(CR LF), CTLs (controls), DIGIT (decimal 0-9), DQUOTE (double quote),
HEXDIG (hexadecimal 0-9/A-F/a-f), LF (line feed), NUL (null octet),
OCTET (any 8-bit sequence of data except NUL), SP (space), HTAB
(horizontal tab), CHAR (any [USASCII] character), VCHAR (any visible
[USASCII] character), and WSP (whitespace).

The OWS (optional whitespace) rule is used where zero or more linear
whitespace characters MAY appear:

OWS = *( [ obs-fold ] WSP )
; "optional" whitespace
obs-fold = CRLF

OWS SHOULD either not be produced or be produced as a single SP
character.

After the changes introduced in the ResponseCookie parser before releasing 0.22.x version, reading cookies that aren't using spaces is broken.

You can test this behavior quickly in a worksheet:

import cats.effect.IO
import org.http4s._
import org.http4s.headers.`Set-Cookie`
import org.typelevel.ci._

val cookie = Header.Raw(ci"set-cookie", "Key=value;Path=/;Secure;HTTPOnly")

implicitly[Header.Select[`Set-Cookie`]].from(List(cookie))

☝🏼 That doesn't work

👇🏼 While this does

import cats.effect.IO
import org.http4s._
import org.http4s.headers.`Set-Cookie`
import org.typelevel.ci._

val cookie = Header.Raw(ci"set-cookie", "Key=value; Path=/; Secure; HTTPOnly")

implicitly[Header.Select[`Set-Cookie`]].from(List(cookie))

Thanks to @ChristopherDavenport for pointing me to the solution 😸.

Reference: rfc6265 (https://datatracker.ietf.org/doc/html/rfc6265#section-2.2)

 This specification uses the Augmented Backus-Naur Form (ABNF)
   notation of [RFC5234].

   The following core rules are included by reference, as defined in
   [RFC5234], Appendix B.1: ALPHA (letters), CR (carriage return), CRLF
   (CR LF), CTLs (controls), DIGIT (decimal 0-9), DQUOTE (double quote),
   HEXDIG (hexadecimal 0-9/A-F/a-f), LF (line feed), NUL (null octet),
   OCTET (any 8-bit sequence of data except NUL), SP (space), HTAB
   (horizontal tab), CHAR (any [USASCII] character), VCHAR (any visible
   [USASCII] character), and WSP (whitespace).

   The OWS (optional whitespace) rule is used where zero or more linear
   whitespace characters MAY appear:

   OWS            = *( [ obs-fold ] WSP )
                    ; "optional" whitespace
   obs-fold       = CRLF

   OWS SHOULD either not be produced or be produced as a single SP
   character.

Signed-off-by: Alejandro Hernández <info@alejandrohdezma.com>
Co-authored-by: Bjørn Madsen <bm@aeons.dk>
@rossabaker
Copy link
Member

The quoted part of the RFC is not relevant. It describes the OWS rule, which appears only around the Cookie string:

   cookie-header = "Cookie:" OWS cookie-string OWS

The relevant section is 5.2, which states:

The algorithm below is more permissive than the grammar in
Section 4.1. For example, the algorithm strips leading and trailing
whitespace from the cookie name and value (but maintains internal
whitespace), whereas the grammar in Section 4.1 forbids whitespace in
these positions.

According to 5.2, we must tolerate and trim whitespace after the semi-colon, but before it, and also around the =. What we were sent is a violation of the spec, but the spec also tells us how to recover from common violations. This PR is a step in the right direction and fixes the bad cookie we were sent, but is not yet a complete implementation. I'm happy to merge this when the build passes to fix the problem found in the wild, but then we should open a separate issue to achieve full compliance.

It still needs a scalafmt.

Reference: 648ec04
Signed-off-by: Alejandro Hernández <info@alejandrohdezma.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants