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.

Sign up for GitHub

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

Jump to bottom

Allow space in cookie value for Set-Cookie header #14435

Open
bararchy opened this issue Apr 4, 2024 · 5 comments · May be fixed by #14455
Open

Allow space in cookie value for Set-Cookie header #14435

bararchy opened this issue Apr 4, 2024 · 5 comments · May be fixed by #14455
Labels
good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. kind:feature topic:stdlib:networking

Comments

@bararchy
Copy link
Contributor

bararchy commented Apr 4, 2024

Right now Crystals does not support parsing cookies that has unquoted space in the Set-Cookie header, but, as stated in https://datatracker.ietf.org/doc/html/rfc6265#section-5.2

NOTE: The algorithm below is more permissive than the grammar in
   [Section 4.1](https://datatracker.ietf.org/doc/html/rfc6265#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](https://datatracker.ietf.org/doc/html/rfc6265#section-4.1) forbids whitespace in
   these positions.  User agents use this algorithm so as to
   interoperate with servers that do not follow the recommendations in
   [Section 4](https://datatracker.ietf.org/doc/html/rfc6265#section-4).

Many servers support this and allows this behavior, which means the HTTP::Client should parse the cookies and not avoid them.

A simple example is the following:

require "http"

cookies = HTTP::Cookies.from_server_headers(
  HTTP::Headers{"Set-Cookie" => "Cu=ID=12&Username=bla@bla.com&CustomerID=1&CustomerName=BAR FOO&RoleID=1&EmailAddress=a@a.com&Firstname=doo&Lastname=boo&Warehouse=AA; expires=Fri, 19-Apr-2024 14:35:28 GMT; path=/"}
)

cookies

I expect cookies to have 1 cookie, but it doesn't. if I remove the space from CustomerName=BAR FOO and instead make it BARFOO I see:

<HTTP::Cookies:0x729d08a7d8a0
 @cookies=
  {"Cu" =>
    #<HTTP::Cookie:0x729d08a68f20
     @creation_time=2024-04-04 18:52:30.826519756 UTC,
     @domain=nil,
     @expires=2024-04-19 14:35:28.0 UTC,
     @extension=nil,
     @http_only=false,
     @max_age=nil,
     @name="Cu",
     @path="/",
     @samesite=nil,
     @secure=false,
     @value=
      "ID=12&Username=bla@bla.com&CustomerID=1&CustomerName=BARFOO&RoleID=1&EmailAddress=a@a.com&Firstname=doo&Lastname=boo&Warehouse=AA">}>

And everything is all good and well.

There is a similar issue that happened in NMAP: nmap/nmap#844 and they also fixed it to support this behavior.
@Blacksmoke16 Blacksmoke16 added the good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. label Apr 6, 2024
anton7c3 added a commit to anton7c3/crystal that referenced this issue Apr 8, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
f4121f3 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
@anton7c3 anton7c3 linked a pull request Apr 8, 2024 that will close this issue
Open
anton7c3 added a commit to anton7c3/crystal that referenced this issue Apr 8, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
eeb056a 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
anton7c3 added a commit to anton7c3/crystal that referenced this issue Apr 8, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
ebce743 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
@bararchy
Copy link
Contributor Author

bararchy commented Apr 8, 2024

@Blacksmoke16 😏
image

I guess my Crystal community counter reset after 10 years of doing Crystal 😁

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Apr 8, 2024 •
edited

xD. community:newcomer is the Crystal repo version of good-first-issue, not that the reporter is new ;)

@straight-shoota
Copy link
Member

It might not be a bad idea to rename this tag 😆

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Apr 8, 2024 •
edited

I will say GH provides native integration with some of the default labels. E.g. help wanted will call out those tickets when looking at the repos in an org for example:

image

Also having an actual good first issue label includes those ticket on a repo's https://github.com/athena-framework/athena/contribute page.

So, it could definitely be a good idea if we wanted to make use of some of those integrations.

@Blacksmoke16 Blacksmoke16 mentioned this issue Apr 8, 2024
Open
anton7c3 added a commit to anton7c3/crystal that referenced this issue Apr 15, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
4a2a3d1 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
anton7c3 added a commit to anton7c3/crystal that referenced this issue Apr 15, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
c748b0d 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
anton7c3 added a commit to anton7c3/crystal that referenced this issue Apr 15, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
912f1c1 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
@bararchy
Copy link
Contributor Author

Seems we get a fix at: #14435 just needs someone to approve it ;)

anton7c3 added a commit to anton7c3/crystal that referenced this issue May 16, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
dac7efe 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
anton7c3 added a commit to anton7c3/crystal that referenced this issue May 16, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
1c91eaa 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
anton7c3 added a commit to anton7c3/crystal that referenced this issue May 31, 2024
@anton7c3
fix(http/cookies): allow spaces to be present in the cookie value
5d152d1 
Fixes crystal-lang#14435.

Move parse_set_cookie example to the correct location.
anton7c3 added a commit to anton7c3/crystal that referenced this issue May 31, 2024
@anton7c3
feat(cookies): add spec to check spaces before and after quotes
4609fc3 
Fixes crystal-lang#14435.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue This is an issue suited for newcomers to become aquianted with working on the codebase. kind:feature topic:stdlib:networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(http/cookies): allow spaces to be present in the cookie value anton7c3/crystal
3 participants
@straight-shoota @bararchy @Blacksmoke16