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_easy_setopt(handle, CURLOPT_COOKIEFILE, NULL) doesn't disable cookies #6889

Closed
StefanKarpinski opened this issue Apr 13, 2021 · 9 comments

Comments

@StefanKarpinski
Copy link

If one enables the cookie engine by doing curl_easy_setopt(handle, CURLOPT_COOKIEFILE,"") or some such, one would expect that subsequently doing curl_easy_setopt(handle, CURLOPT_COOKIEFILE, NULL) would disable it again — indeed, this is how most curl options work. Not sure if this is a bug report or a feature request.

@bagder
Copy link
Member

bagder commented Apr 13, 2021

I don't think it can be seen as anything else than an oversight that we cannot switch off the cookie engine for an easy handle once it has been turned on.

But if we are to introduce such a way - and I think we should - maybe it would make more sense to follow the previous pattern of "cookie commands" that we support with CURLOPT_COOKIELIST and add a switch off command there? Maybe just call it OFF ?

@jay
Copy link
Member

jay commented Apr 13, 2021

maybe it would make more sense to follow the previous pattern of "cookie commands" that we support

the fix would probably be simple, however CURLOPT_COOKIEJAR I don't see a good way to do the same because data->cookies may be used by a share handle i think

diff --git a/lib/setopt.c b/lib/setopt.c
index 022dd38..4612c03 100644
--- a/lib/setopt.c
+++ b/lib/setopt.c
@@ -752,6 +752,10 @@ CURLcode Curl_vsetopt(struct Curl_easy *data, CURLoption option, va_list param)
       }
       data->state.cookielist = cl; /* store the list for later use */
     }
+    else {
+      curl_slist_free_all(data->state.cookielist);
+      data->state.cookielist = NULL;
+    }
     break;
 
   case CURLOPT_COOKIEJAR:

@bagder
Copy link
Member

bagder commented Apr 13, 2021

the fix would probably be simple

This just clears the list of files to read cookies from. It would also need to clear all the cookies from memory and switch off further cookie handling.

@jay
Copy link
Member

jay commented Apr 13, 2021

IIRC the cookie engine isn't turned on by CURLOPT_COOKIEFILE until the transfer is started

@bagder
Copy link
Member

bagder commented Apr 13, 2021

Right, but what if the handle was already used for one or more transfers with cookies and then you switch it off...

@jay
Copy link
Member

jay commented Apr 13, 2021

I see what you're saying. I don't think the cookie engine should explicitly be shut off by CURLOPT_COOKIEFILE, NULL, but it probably should clear the filenames. The doc doesn't say NULL shuts it off.

@StefanKarpinski
Copy link
Author

StefanKarpinski commented Apr 14, 2021

For what it's worth, I would expect setting it to NULL to turn it off and here's my reasoning: the default is documented as NULL and if all I do is set CURLOPT_COOKIEFILE to some path and then set it back to NULL without doing anything else, I would expect the easy handle to behave the way it would if I had left things in the default configuration.

@bagder
Copy link
Member

bagder commented Apr 14, 2021

I think there's a lot of sense in that reasoning...

@bagder
Copy link
Member

bagder commented Apr 14, 2021

Proposed approach in #6891

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

Successfully merging a pull request may close this issue.

4 participants
@StefanKarpinski @bagder @jay and others