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

date parsing: Cookie expiry fallback #4152

wants to merge 5 commits into from


Copy link

@Tuniwutzi Tuniwutzi commented Jul 25, 2019

This PR suggests a different behavior for parsing cookie expiry dates.

Two changes were made:

  1. Add Curl_parse_expiry which is used to parse HTTP cookie expiry dates. The only difference to curl_getdate is that it does not return an error in case of PARSEDATE_LATER. This means that cookies with expiry later than 2038 do not get parsed as session cookies any more, but as cookies with the latest possible expiry timestamp.
    Reason: I use curl in a project where the server returns cookies with expiry dates of 2087 and later (for whatever reason). Curl treating these as session cookies and ignoring them upon restart caused big issues and seems counter-intuitive to me.
  2. Change the behavior of parsedate to not return TIME_T_MAX when we are unsure if we can represent the given date in time_t. Instead, return the timestamp for 01.01.2038, 00:00 in the timezone specified in the date string.
    Reason: This ensures that we never return a later timestamp than specified in the datestring, so the returncode PARSEDATE_LATER does not lie. Note that this does not change the behavior of curl_getdate, which returns an error if the returncode is not PARSEDATE_OK.

Note: I added Curl_parse_expiry to avoid a change to the behavior of the public interface by changing curl_getdate.


  • Make parsedate consistent by replacing this code:
    if(yearnum < 1903) {
        *output = TIME_T_MIN;
        return PARSEDATE_SOONER;
    With an equivalent mechanic to that used in overflow.
  • Make Curl_parse_expiry deal with PARSEDATE_SOONER the same way it deals with PARSEDATE_LATER for consistency
  • Add unit tests for this behavior

Copy link

@bagder bagder left a comment

Choose a reason for hiding this comment

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

I think the reasoning and fix are perfectly convincing. It would be lovely with a unit test that verifies that this actually work like this too (and will avoid future regressions).

Copy link

Good, as soon as I have time I will address the todos in the description and add a unit test.

Copy link

I tried to add a unit test but I'm not sure how to do it. I'll try to figure it out when I have some time again, but if someone has pointers for me it would be appreciated.

Copy link

bagder commented Aug 6, 2019

There's a little README describing how to write and run unit tests.

If you run into problems when following that, please ask and we can help out (and update the docs if necessary)!

Copy link

@Tuniwutzi How are you going with the unit test, do you need any assistance?

Copy link

Unfortunately I was having some issues when I tried adding and running a new test. Then a lot of other work came up for me and I forgot to finish this.

@danielgustafsson Any assistance would be greatly appreciated, since I'm short on time at the moment. So if you want to lend a hand, I would definitely not complain! Otherwise I'll hopefully get to it sometime in December.

Copy link

bagder commented Nov 28, 2019

This ensures that we never return a later timestamp than specified in the datestring, so the returncode PARSEDATE_LATER does not lie

Why is it important? Out of all "later" dates we can specify, most of them will be larger...

bagder added a commit that referenced this pull request Nov 28, 2019
... and use internally. This function will return TIME_T_MAX instead of
failure if the parsed data is found to be larger than what can be
represented. TIME_T_MAX being the largest value curl can represent.

Reported-by: JanB on github
Ref: #4152
@bagder bagder closed this in 0044443 Nov 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
None yet

Successfully merging this pull request may close these issues.

None yet

4 participants