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

Closed
wants to merge 5 commits into from

Conversation

Tuniwutzi
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.

ToDo:

  • 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
Member

@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).

@Tuniwutzi
Copy link
Author

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

@Tuniwutzi
Copy link
Author

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.

@bagder
Copy link
Member

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)!

@danielgustafsson
Copy link
Member

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

@Tuniwutzi
Copy link
Author

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.

@bagder
Copy link
Member

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.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants