Skip to content

Cookie handling changed to match most browsers behavior. - #1050

Closed
ksa-real wants to merge 2 commits into
curl:masterfrom
ksa-real:cookies
Closed

ksa-real wants to merge 2 commits into
curl:masterfrom
ksa-real:cookies

Conversation

@ksa-real

@ksa-real ksa-real commented Oct 3, 2016 •

Copy link
Copy Markdown
Contributor

Cokie with the same domain but different tailmatching property are
now considered different and do not replace each other.
If header contains following lines then two cookies will be set:
Set-Cookie: foo=bar; domain=.foo.com; expires=Thu Mar 3 GMT 8:56:27 2033
Set-Cookie: foo=baz; domain=foo.com; expires=Thu Mar 3 GMT 8:56:27 2033

This matches Chrome, Opera, Safari, and Firefox behavior. When sending
stored tokens to foo.com Chrome, Opera, and Firefox send them in the
stored order, while Safari pre-sort the cookies.

Cokie with the same same domain but different tailmatching property are
now considered different and do not replace each other.
If header contains following lines then two cookies will be set:
Set-Cookie: foo=bar; domain=.foo.com; expires=Thu Mar 3 GMT 8:56:27 2033
Set-Cookie: foo=baz; domain=foo.com; expires=Thu Mar 3 GMT 8:56:27 2033

This matches Chrome, Opera, Safari, and Firefox behavior. When sending
stored tokens to foo.com Chrome, Opera, Firefox store send them in the
stored order, while Safari pre-sort the cookies.
@mention-bot

Copy link
Copy Markdown

@ksa-real, thanks for your PR! By analyzing the history of the files in this pull request, we identified @umgnay, @bagder and @bgilbert to be potential reviewers.

@bagder bagder added the HTTP label Oct 3, 2016
Comment thread lib/cookie.c Outdated
if(clist->domain && co->domain) {
if(Curl_raw_equal(clist->domain, co->domain))
if(Curl_raw_equal(clist->domain, co->domain) &&
clist->tailmatch == co->tailmatch)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer having the second condition within parentheses too, to make it easier for readers of the code to figure out precedence.

@bagder bagder left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from that little nit, this looks like a small and elegant fix and even with a test case so I think we can merge once fixed.

@bagder bagder closed this in 54e48b1 Oct 3, 2016
@bagder

bagder commented Oct 3, 2016

Copy link
Copy Markdown
Member

thanks!

@lock lock Bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

3 participants