On 10.07.2024 01:49, Jeff King wrote:
On Tue, Jul 09, 2024 at 04:03:48PM -0700, Junio C Hamano wrote:
The manual pages https://curl.se/libcurl/c/CURLOPT_COOKIEFILE.html
and https://curl.se/libcurl/c/CURLOPT_COOKIEJAR.html talk about two
interesting special values.
* "" (an empty string) given to CURLOPT_COOKIEFILE means not to
read cookies from any file upon startup.
* It is not specified what "" (an empty string) given to
CURLOPT_COOKIEJAR does; presumably open a file whose name is an
empty string and write cookies to it? In any case, that is not
what we want to see happen, ever.
* "-" (a dash) given to CURLOPT_COOKIEFILE makes cURL read cookies
from the standard input, and given to CURLOPT_COOKIEJAR makes
cURL write cookies to the standard output. Neither of which we
want ever to happen.
So, let's make sure we avoid these nonsense cases. Specifically,
when http.cookies is set to "-", ignore it with a warning, and when
it is set to "" and http.savecookies is set, ignore http.savecookies
with a warning.
[...]
* I have no confidence in me doing http correctly, so I am asking
from folks who have touched http.c in the past 6 months for help.
I don't have any experience with any of the cookie options, but your
explanation here all makes sense. It might be worth including a test,
though the interesting part is probably how things broke _before_ this
patch. After it, it's pretty obvious what should happen.
So I'll try to comment from the general http.c perspective.
Hello!
I'm able to perform some checks as I have Git repository behind HAProxy
load balancer which sets HTTP cookie to record which backend should
process consecutive requests.
Indeed, if http.cookieFile='-' is used, git stops and waits for input.
It does *not* work even if I do:
$ echo '/path/to/file' | git -c http.cookieFile='-' ...
On the other hand there is no problem if http.cookieFile='' and
http.saveCookies=true is used together. Git operation is successful. But
if GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 is enabled, I can see
following warning it the output:
> 12:19:56.280263 http.c:820 == Info: WARNING: failed to save cookies in
It comes from:
https://github.com/curl/curl/blob/master/lib/cookie.c#L1758
But cookies were accepted by the client and sent back to the server.
PS. I'm using Git 2.42.0.
Regards!
--
Piotr Szlazak
diff --git c/http.c w/http.c
index 13fa94bef3..86ccca49f0 100644
--- c/http.c
+++ w/http.c
@@ -1466,7 +1466,16 @@ struct active_request_slot *get_active_slot(void)
slot->finished = NULL;
slot->callback_data = NULL;
slot->callback_func = NULL;
+
+ if (curl_cookie_file && !strcmp(curl_cookie_file, "-")) {
+ warning(_("refusing to read cookies from http.cookiefile '-'"));
+ FREE_AND_NULL(curl_cookie_file);
+ }
curl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);
+ if (curl_save_cookies && (!curl_cookie_file || !curl_cookie_file[0])) {
+ curl_save_cookies = 0;
+ warning(_("ignoring http.savecookies for empty http.cookiefile"));
+ }
if (curl_save_cookies)
curl_easy_setopt(slot->curl, CURLOPT_COOKIEJAR, curl_cookie_file);
This all looks OK to me. A few things I wondered while reading:
- is curl_cookie_file always an allocated string? The answer is yes,
because it comes from git_config_pathname(). Good.
- get_active_slot() will be called a lot of times, as we reuse the
curl handles over and over (the "slot" terminology is due to the
parallelism for dumb-http fetch; smart-http just reuses the one
handle each time). So is this the best place to put the check?
You actually unset the options when issuing the warning, so we'd
never see the warning multiple times, even if this code is run
repeatedly. Good.
I do suspect these curl_easy_setopt() calls for cookies could just
go into get_curl_handle(), which sets up the handle initially. But
it's possible there's some subtle reason why they're here, and
certainly moving them is orthogonal to your goal. And in the
meantime, putting your new checks alongside the use of the variables
makes sense.
-Peff
