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

curl sending expired cookie in 7.62.0 #3351

Closed
jeroen opened this issue Dec 8, 2018 · 4 comments
Closed

curl sending expired cookie in 7.62.0 #3351

jeroen opened this issue Dec 8, 2018 · 4 comments

Comments

@jeroen
Copy link
Contributor

jeroen commented Dec 8, 2018

After updating to 7.62.0 one of my unit tests started failing. The problem is that when the server unsets a cookie, and the libcurl client makes a subsequent request within 1 second after the response, curl will include the deleted cookie in the request.

Example:

I run into this with the R bindings, not sure if there is an easy way to reproduce in the cmd line. Basically the test performs the 4 steps below (using a single easy handle)

  1. create new easy handle, set curlopt_verbose = 1
  2. perform request to https://httbin.org/cookies/set?foo=123&bar=456. The server responds:
...
* Added cookie foo="123" for domain eu.httpbin.org, path /, expire 0
< Set-Cookie: foo=123; Secure; Path=/
* Added cookie bar="456" for domain eu.httpbin.org, path /, expire 0
< Set-Cookie: bar=456; Secure; Path=/
  1. perform subsequent request to https://httpbin.org/cookies/delete?bar. The server responds:
...
* Replaced cookie bar="" for domain eu.httpbin.org, path /, expire 1544281853
< Set-Cookie: bar=; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/

Hence the bar cookie has been marked as expired by the server.

  1. immediately perform another request https://httpbin.org/cookies. Now libcurl will send:
* Found bundle for host eu.httpbin.org: 0x101b3ead0 [can pipeline]
* Re-using existing connection! (#2) with host eu.httpbin.org
* Connected to eu.httpbin.org (34.246.221.52) port 443 (#2)
> GET /cookies HTTP/1.1
Host: eu.httpbin.org
User-Agent: R (3.5.1 x86_64-apple-darwin15.6.0 x86_64 darwin15.6.0)
Accept: */*
Accept-Encoding: gzip, deflate
Cookie: bar=; foo=123

Note the last line. It is including the expired bar= cookie which seems wrong. In previous versions of curl it would correctly omit the expired bar and just send Cookie: foo=123 instead.

Also note that the problem does not appear if you wait at least 1 second between step 3 and 4.

@bagder
Copy link
Member

bagder commented Dec 8, 2018

RFC 6265 says the following for the Max-Age value:

If delta-seconds is less than or equal to zero (0), let expiry-time be the earliest representable date and time.

... but the libcurl code seems to wrongly treat zero as any other value and therefore it won't expire this cookie within the same second:

curl/lib/cookie.c

Lines 669 to 684 in d997aa0

if(co->maxage) {
CURLofft offt;
offt = curlx_strtoofft((*co->maxage == '\"')?
&co->maxage[1]:&co->maxage[0], NULL, 10,
&co->expires);
if(offt == CURL_OFFT_FLOW)
/* overflow, used max value */
co->expires = CURL_OFF_T_MAX;
else if(!offt) {
if(CURL_OFF_T_MAX - now < co->expires)
/* would overflow */
co->expires = CURL_OFF_T_MAX;
else
co->expires += now;
}
}

@danielgustafsson, you've looked at cookies recently. Would you agree?

@bagder bagder added the HTTP label Dec 8, 2018
bagder added a commit that referenced this issue Dec 8, 2018
Reported-by: Jeroen Ooms
Fixes #3351
bagder added a commit that referenced this issue Dec 8, 2018
Reported-by: Jeroen Ooms
Fixes #3351
@bagder
Copy link
Member

bagder commented Dec 8, 2018

#3352 is my proposed fix with a new test to verify it

@danielgustafsson
Copy link
Member

I agree with your interpretation of the RFC and your fix.

@jeroen
Copy link
Contributor Author

jeroen commented Dec 9, 2018

Thanks for the quick fix. Hopefully this makes it into the upcoming release!

@bagder bagder closed this as completed in 1e9abfe Dec 9, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Mar 9, 2019
@bagder bagder added the cookies label Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

3 participants