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

Deleting cookies no longer works #3445

Closed
jeroen opened this Issue Jan 7, 2019 · 13 comments

Comments

Projects
None yet
4 participants
@jeroen
Copy link
Contributor

jeroen commented Jan 7, 2019

After fixing #3351 my unit tests were passing, but now they broke again.

What it does

Basically the unit test lets the server set a cookie, then deletes it, and then lists cookies:

  1. create a handle
  2. make a request to https://eu.httpbin.org/cookies/set?foo=123&bar=ftw which sets cookies foo and bar
  3. make a request to https://eu.httpbin.org/cookies/delete?foo which deletes cookie foo.
  4. list cookies with curl_easy_getinfo(handle, CURLINFO_COOKIELIST, &cookies))

Previous output

Up till version 7.62, CURLINFO_COOKIELIST would contain both cookies, with the expired cookie in the list with the timestamp at which it expired, and value as NULL like this:

          domain  flag path secure          expiration name value
1 eu.httpbin.org FALSE    /  FALSE 2019-01-07 23:26:58  foo  <NA>
2 eu.httpbin.org FALSE    /   TRUE                <NA>  bar   ftw

Current output with HTTPS (definitely a bug)

The expired cookie is not deleted at all:

          domain  flag path secure expiration name value
1 eu.httpbin.org FALSE    /   TRUE       <NA>  foo   123
2 eu.httpbin.org FALSE    /   TRUE       <NA>  bar   ftw

New output output with HTTP (maybe a bug?)

The expired cookie is entirely omitted from CURLINFO_COOKIELIST :

          domain  flag path secure expiration name value
1 eu.httpbin.org FALSE    /  FALSE       <NA>  bar   ftw

Full log from R

Sorry for the R code :-)

 library(curl)
 h <- new_handle(verbose = TRUE)
 req <- curl_fetch_memory('https://eu.httpbin.org/cookies/set?foo=123&bar=ftw', handle = h)
*   Trying 34.248.41.77...
* TCP_NODELAY set
* Connected to eu.httpbin.org (34.248.41.77) port 443 (#0)
* ALPN, offering http/1.1
* TLS 1.2 connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
* Server certificate: eu.httpbin.org
* Server certificate: Let's Encrypt Authority X3
* Server certificate: DST Root CA X3
> GET /cookies/set?foo=123&bar=ftw HTTP/1.1
Host: eu.httpbin.org
User-Agent: R (3.5.2 x86_64-apple-darwin15.6.0 x86_64 darwin15.6.0)
Accept: */*
Accept-Encoding: gzip, deflate

< HTTP/1.1 302 FOUND
< Connection: keep-alive
< Server: gunicorn/19.9.0
< Date: Mon, 07 Jan 2019 22:38:11 GMT
< Content-Type: text/html; charset=utf-8
< Content-Length: 223
< Location: /cookies
* Added cookie foo="123" for domain eu.httpbin.org, path /, expire 0
< Set-Cookie: foo=123; Secure; Path=/
* Added cookie bar="ftw" for domain eu.httpbin.org, path /, expire 0
< Set-Cookie: bar=ftw; Secure; Path=/
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Credentials: true
< Via: 1.1 vegur
<
* Ignoring the response-body
* Connection #0 to host eu.httpbin.org left intact
* Issue another request to this URL: 'https://eu.httpbin.org/cookies'
* Found bundle for host eu.httpbin.org: 0x7fc449506050 [can pipeline]
* Re-using existing connection! (#0) with host eu.httpbin.org
* Connected to eu.httpbin.org (34.248.41.77) port 443 (#0)
> GET /cookies HTTP/1.1
Host: eu.httpbin.org
User-Agent: R (3.5.2 x86_64-apple-darwin15.6.0 x86_64 darwin15.6.0)
Accept: */*
Accept-Encoding: gzip, deflate
Cookie: bar=ftw; foo=123

< HTTP/1.1 200 OK
< Connection: keep-alive
< Server: gunicorn/19.9.0
< Date: Mon, 07 Jan 2019 22:38:11 GMT
< Content-Type: application/json
< Content-Length: 59
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Credentials: true
< Via: 1.1 vegur
<
* Connection #0 to host eu.httpbin.org left intact
req <- curl_fetch_memory('https://eu.httpbin.org/cookies/delete?foo', handle = h)
* Found bundle for host eu.httpbin.org: 0x7fc449506050 [can pipeline]
* Re-using existing connection! (#0) with host eu.httpbin.org
* Connected to eu.httpbin.org (34.248.41.77) port 443 (#0)
> GET /cookies/delete?foo HTTP/1.1
Host: eu.httpbin.org
User-Agent: R (3.5.2 x86_64-apple-darwin15.6.0 x86_64 darwin15.6.0)
Accept: */*
Accept-Encoding: gzip, deflate
Cookie: bar=ftw; foo=123

< HTTP/1.1 302 FOUND
< Connection: keep-alive
< Server: gunicorn/19.9.0
< Date: Mon, 07 Jan 2019 22:38:11 GMT
< Content-Type: text/html; charset=utf-8
< Content-Length: 223
< Location: /cookies
< Set-Cookie: foo=; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Credentials: true
< Via: 1.1 vegur
<
* Ignoring the response-body
* Connection #0 to host eu.httpbin.org left intact
* Issue another request to this URL: 'https://eu.httpbin.org/cookies'
* Found bundle for host eu.httpbin.org: 0x7fc449506050 [can pipeline]
* Re-using existing connection! (#0) with host eu.httpbin.org
* Connected to eu.httpbin.org (34.248.41.77) port 443 (#0)
> GET /cookies HTTP/1.1
Host: eu.httpbin.org
User-Agent: R (3.5.2 x86_64-apple-darwin15.6.0 x86_64 darwin15.6.0)
Accept: */*
Accept-Encoding: gzip, deflate
Cookie: bar=ftw; foo=123

< HTTP/1.1 200 OK
< Connection: keep-alive
< Server: gunicorn/19.9.0
< Date: Mon, 07 Jan 2019 22:38:11 GMT
< Content-Type: application/json
< Content-Length: 59
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Credentials: true
< Via: 1.1 vegur
<
* Connection #0 to host eu.httpbin.org left intact
curl::handle_cookies(h)
          domain  flag path secure expiration name value
1 eu.httpbin.org FALSE    /   TRUE       <NA>  foo   123
2 eu.httpbin.org FALSE    /   TRUE       <NA>  bar   ftw
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Jan 7, 2019

What libcurl version is this? It sounds like you might want a52e46f, which is in 7.63.0...

@jeroen

This comment has been minimized.

Copy link
Contributor

jeroen commented Jan 7, 2019

This is master branch 7.64.0-DEV

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Jan 7, 2019

(@danielgustafsson mentioned on IRC he'll take a look at this issue tomorrow)

@jay

This comment has been minimized.

Copy link
Member

jay commented Jan 8, 2019

In 7.63 there's 1e9abfe by @jeroen which deletes the cookie but since then there's 7a09b52 by @danielgustafsson, the only possibility I see is a mismatch on deletion this probably did it

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Jan 8, 2019

I suspect this happens because the cookie update isn't using secure but the creation of it does:

  1. The two cookies are created like this over a HTTPS connection:
Set-Cookie: foo=123; Secure; Path=/
Set-Cookie: bar=ftw; Secure; Path=/
  1. The update that wants to expire the cookie is set without secure even though still done over HTTPS:
Set-Cookie: foo=; Expires=Thu, 01-Jan-1970 00:00:00 GMT; Max-Age=0; Path=/

I can't find this strict behavior mandated in draft-ietf-httpbis-cookie-alone though. @danielgustafsson, what do you think?

@danielgustafsson

This comment has been minimized.

Copy link
Member

danielgustafsson commented Jan 8, 2019

@jeroen

This comment has been minimized.

Copy link
Contributor

jeroen commented Jan 8, 2019

Sorry forget what I said a minute ago, deleted that message.

I can confirm that the cookie does get deleted when running the same test in with http:// instead of https://. However we get another change in behavior: previously the deleted cookie would get listed with an expiration date in the past, and value set to NULL.

The new behavior is that the expired is not listed at all anymore. So to summarize: if we set and then delete a cookie, and then check the CURLINFO_COOKIELIST we see this:

Old output (either HTTP and HTTPS)

The expired cookie is included with CURLINFO_COOKIELIST with expiration date in the past and the value set to NULL:

          domain  flag path secure          expiration name value
1 eu.httpbin.org FALSE    /  FALSE 2019-01-07 23:26:58  foo  <NA>
2 eu.httpbin.org FALSE    /   TRUE                <NA>  bar   ftw

Current output with HTTPS (definitely a bug)

The expired cookie is not deleted at all:

          domain  flag path secure expiration name value
1 eu.httpbin.org FALSE    /   TRUE       <NA>  foo   123
2 eu.httpbin.org FALSE    /   TRUE       <NA>  bar   ftw

Current output with HTTP (maybe a bug?)

The expired cookie is entirely omitted from CURLINFO_COOKIELIST :

          domain  flag path secure expiration name value
1 eu.httpbin.org FALSE    /  FALSE       <NA>  bar   ftw
@jay

This comment has been minimized.

Copy link
Member

jay commented Jan 8, 2019

Didn't you do that in 1e9abfe

@jeroen

This comment has been minimized.

Copy link
Contributor

jeroen commented Jan 8, 2019

Me? I only reported a condition where curl was sending expired cookies if you would do a subsequent request within 1sec after deletion: #3351. I think the above issue(s) are orthogonal to that.

@jay

This comment has been minimized.

Copy link
Member

jay commented Jan 8, 2019

Oops I glanced and thought you authored that commit but now I see it was @bagder. Anyway I thought that was the intention.

bagder added a commit that referenced this issue Jan 9, 2019

cookies: allow secure override when done over HTTPS
Added test 1562 to verify.

Reported-by: Jeroen Ooms
Fixes #3445

@bagder bagder assigned bagder and unassigned danielgustafsson Jan 9, 2019

@bagder bagder closed this in afeb8d9 Jan 10, 2019

@jeroen

This comment has been minimized.

Copy link
Contributor

jeroen commented Jan 10, 2019

Thanks for fixing!

Are you also interested in the other (minor) problem? That the behavior of CURLINFO_COOKIELIST has changed to completely omit expired cookies (previous behavior was to include expired cookies in CURLINFO_COOKIELIST with value NULL and the timestamp at which they expired.

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Jan 10, 2019

Is that really a problem?

curl does not attempt to keep expired cookies around as they would never get sent anymore so they are (rather aggressively) removed. I don't think we've ever claimed to keep them around - I don't think this breaks any documented behavior.

We've always removed expired cookies, its just that recently we've started to do it slightly more often.

@jeroen

This comment has been minimized.

Copy link
Contributor

jeroen commented Jan 10, 2019

No it's not a problem, just wanted to confirm this is intentional. Then I will update my tests accordingly.

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