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

Align cookie sorting with RFC6265 #2524

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@danielgustafsson
Member

danielgustafsson commented Apr 23, 2018

While reading code I realised that the cookie sort order isn't really complying withRFC6265. This may be intentional, but I figured I'd hack up a patch as a proposal to improve on that as sort of a "does this make sense" question. According to RFC6265 section 5.4, cookies with equal path lengths SHOULD be sorted by creation-time (earlier first). This adds a creation-time record to the cookie struct in order to make cookie sorting more deterministic. Also remove the strcmp() matching in the sorting as there is no lexicographic ordering in RFC6265. Existing tests are updated to match.

Also includes a commit with a few typo fixes.

@bagder

This comment has been minimized.

Member

bagder commented Apr 23, 2018

I'm not entirely sure why this is, but I think I might've opted to leave that out and instead sort on names because...

Since we don't store creation time in the cookie file, reading back the cookies risk getting them in a different order / time and then sort them differently which is undesired. Any thoughts on how to handle that?

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Apr 23, 2018

@bagder

This comment has been minimized.

Member

bagder commented Apr 23, 2018

Here's a thought:

If the creation time is only used for sorting, then the exact time isn't important but only the relative time. Then as long as the cookies are saved to file in creation-order, they can be loaded back in that order and get an artificial time just cookie-epoch and an increasing number for each loaded cookie. As long as the last loaded cookie's time ends up earlier than the first possible "live" cookie... But then perhaps we could use a simple counter for the live ones as well!?

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Apr 23, 2018

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Apr 24, 2018

Thinking more about this, we run into a problem with this scheme if there are multiple cookie files via CURLOPT_COOKIEFILE. Not sure how we can ensure correct ordering of the cookies across the files.

@bagder

This comment has been minimized.

Member

bagder commented Apr 26, 2018

Right, and there's a potential problem when we read a file that is created by someone/something else as we then assume the order of the cookies means something that the creator maybe didn't intend. But on the other hand, the cookie order is not controlled by those users even without this change.

I think this simplified creation-date logic can still work, even if it doesn't work cross-files on load. I think that's a minor issue we can still live with.

Can you think of any major downside that isn't basically present already?

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Apr 27, 2018

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-cookie_creation_time branch from 301d7c6 to 332a4e5 May 5, 2018

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented May 5, 2018

Pushed a rebased version which implements the sort-order creation-time scheme.

@bagder

This comment has been minimized.

Member

bagder commented Aug 24, 2018

Coming back to this @danielgustafsson. Do you still think this is a good addition? If so, can you please fix the conflict and rebase this on top of the latest master and we can work on getting this merged, at least in the next feature window.

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-cookie_creation_time branch from 332a4e5 to ac5f729 Aug 28, 2018

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Aug 28, 2018

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-cookie_creation_time branch 2 times, most recently from b1d3b17 to 3ee2369 Aug 28, 2018

cookies: support creation-time attribute for cookies
According to RFC6265 section 5.4, cookies with equal path lengths
SHOULD be sorted by creation-time (earlier first). This adds a
creation-time record to the cookie struct in order to make cookie
sorting more deterministic. The creation-time is defined as the
order of the cookies in the jar, the first cookie read fro the
jar being the oldest. The creation-time is thus not serialized
into the jar. Also remove the strcmp() matching in the sorting as
there is no lexicographic ordering in RFC6265. Existing tests are
updated to match.

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-cookie_creation_time branch from 3ee2369 to a68ee3a Aug 29, 2018

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-cookie_creation_time branch from a68ee3a to f93cff3 Aug 29, 2018

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Aug 29, 2018

This now builds green after fixing a silly test failure, I also found and added another typo. Should be ready for review now.

@bagder

bagder approved these changes Aug 31, 2018

@bagder bagder closed this in e2ef8d6 Aug 31, 2018

@bagder

This comment has been minimized.

Member

bagder commented Aug 31, 2018

Awesome work @danielgustafsson! Thanks a lot.

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Aug 31, 2018

Excited to see this go in, thanks for reviewing!

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

cookies: support creation-time attribute for cookies
According to RFC6265 section 5.4, cookies with equal path lengths
SHOULD be sorted by creation-time (earlier first). This adds a
creation-time record to the cookie struct in order to make cookie
sorting more deterministic. The creation-time is defined as the
order of the cookies in the jar, the first cookie read fro the
jar being the oldest. The creation-time is thus not serialized
into the jar. Also remove the strcmp() matching in the sorting as
there is no lexicographic ordering in RFC6265. Existing tests are
updated to match.

Closes curl#2524

@lock lock bot locked as resolved and limited conversation to collaborators Nov 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.