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

Incorrectly parse cookies containing comma in value. #28

Open
midnight-wonderer opened this issue Jan 22, 2020 · 3 comments
Open

Incorrectly parse cookies containing comma in value. #28

midnight-wonderer opened this issue Jan 22, 2020 · 3 comments

Comments

@midnight-wonderer
Copy link

It is possible for a cookie to contains a comma in its value.
This is the real header I found in the wild:

Set-Cookie: token=2e732538-f38e-ae67-8217-5330686854ee,35cfb541def868b6778c0dc45e368322,hWiqTu7qtWt6wBbcUzWqK018Zb1iA5CeHPafl+7nFyAlE3XzPFCFsav2QrmFcSTLlnYNFvlS/7PCsJf9u+ychIZQFY4JGZb4dSoEIgaLtNKWEcD+/hHrAnUEaAtOg7ChfTbN42PS3wPhxOtAI5RcLw==; expires=Thu, 21-Jan-2021 05:20:32 GMT; path=/; secure; HttpOnly

Chrome parses it as
name: token
value: 2e732538-f38e-ae67-8217-5330686854ee,35cfb541def868b6778c0dc45e368322,hWiqTu7qtWt6wBbcUzWqK018Zb1iA5CeHPafl+7nFyAlE3XzPFCFsav2QrmFcSTLlnYNFvlS/7PCsJf9u+ychIZQFY4JGZb4dSoEIgaLtNKWEcD+/hHrAnUEaAtOg7ChfTbN42PS3wPhxOtAI5RcLw==
screenshot

However, this gem parses it as
name: 35cfb541def868b6778c0dc45e368322,hWiqTu7qtWt6wBbcUzWqK018Zb1iA5CeHPafl+7nFyAlE3XzPFCFsav2QrmFcSTLlnYNFvlS/7PCsJf9u+ychIZQFY4JGZb4dSoEIgaLtNKWEcD+/hHrAnUEaAtOg7ChfTbN42PS3wPhxOtAI5RcLw
value: =

Which cause errors.

@midnight-wonderer
Copy link
Author

midnight-wonderer commented Jan 22, 2020

For people looking for a workaround, I do have one.

By adding manual value parsing instead of relying on jar.parse(header, uri).

headers = [] # array of set-cookie header
uri = 'https://example.com'
jar = HTTP::CookieJar.new
kv_matcher = /\A([^\s=]*?)\s*=\s*([^\n;]*?)\s*(?:;|\z)/i
headers.each do |header|
  matched_data = kv_matcher.match(header)
  # next unless matched_data
  jar.parse(header, uri) do |cookie|
    cookie_name = matched_data[1]
    cookie_value = matched_data[2]
    cookie.name = cookie_name unless (cookie.name == cookie_name)
    cookie.value = cookie_value unless (cookie.value == cookie_value)
    cookie
  end
end

It works for my use case, but I can't guarantee it will work according to the standard.

@midnight-wonderer
Copy link
Author

midnight-wonderer commented Jan 22, 2020

Digging further, trying to send a pull request.
I found it is impossible to implement the browser-compatible version.

I feel the need to disclose that the header was from Linkedin.
It probably not conforms to the RFC document, but a large portion of traffics relies on it.

Here why the task is impossible:

  • Usually, when a web server wants to set multiple cookies, it will send multiple "set-cookie" headers to clients.
  • For some reason, some ruby gem decides to concatenate all set-cookie headers into a single string separated by commas. e.g., Suggestion: return response header values as arrays since they can have multiple values lostisland/faraday#44
    The decision was premature; there are not enough people reviewing then, so some consequences were not considered.
  • This gem decides to support the gems above.
    Ending up implement best effort string scanner to split cookie values based on commas.
    (See the test case to get a feel on how bad it is: https://github.com/sparklemotion/http-cookie/blob/7f94a9e5/test/test_http_cookie.rb#L296)
  • The thing is, even for a human, the test case above is ambiguous. What if the cookie name contains a comma?
    What if, for the test case above, the cookie name is actually "abc, name".
    Whos know, the comma-separated encoding is lossy and threw away some important data.
  • The best real-world example is the one I open this issue with.

The only solution is to drop support for such gem, it is not how HTTP headers are supposed to work in the first place.
No one seems to have a resource and motive to overtake the work at the moment.
In fact, this issue has persisted for years, even if virtually all people in the Ruby community rely on this gem for the cookie storage.

I truly hope some enterprises can pick this up.

@midnight-wonderer
Copy link
Author

No response so far.
Here's my solution:
https://github.com/the-cave/cookiejar-of-greed

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

No branches or pull requests

1 participant