{"thread":{"id":"61760","subject":"[PATCH/RFC] http.c: cookie file tightening","startedAt":"2024-07-09T23:03:54Z","lastAt":"2024-07-10T16:33:51Z","messageCount":4,"participants":["Junio C Hamano","Jeff King","Piotr Szlazak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498386","messageId":"xmqqed82cgmj.fsf@gitster.g","threadId":"61760","inReplyTo":null,"subject":"[PATCH/RFC] http.c: cookie file tightening","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-09T23:03:48Z","receivedAt":"2024-07-09T23:03:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The http.cookiefile configuration variable is used to call\ncurl_easy_setopt() to set CURLOPT_COOKIEFILE and if http.savecookies\nis set, the same value is used for CURLOPT_COOKIEJAR.  The former is\nused only to read cookies at startup, the latter is used to write\ncookies at the end.\n\nThe manual pages https://curl.se/libcurl/c/CURLOPT_COOKIEFILE.html\nand https://curl.se/libcurl/c/CURLOPT_COOKIEJAR.html talk about two\ninteresting special values.\n\n * \"\" (an empty string) given to CURLOPT_COOKIEFILE means not to\n   read cookies from any file upon startup.\n\n * It is not specified what \"\" (an empty string) given to\n   CURLOPT_COOKIEJAR does; presumably open a file whose name is an\n   empty string and write cookies to it?  In any case, that is not\n   what we want to see happen, ever.\n\n * \"-\" (a dash) given to CURLOPT_COOKIEFILE makes cURL read cookies\n   from the standard input, and given to CURLOPT_COOKIEJAR makes\n   cURL write cookies to the standard output.  Neither of which we\n   want ever to happen.\n\nSo, let's make sure we avoid these nonsense cases.  Specifically,\nwhen http.cookies is set to \"-\", ignore it with a warning, and when\nit is set to \"\" and http.savecookies is set, ignore http.savecookies\nwith a warning.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * I have no confidence in me doing http correctly, so I am asking\n   from folks who have touched http.c in the past 6 months for help.\n\n   A proposed documentation update to talk about an empty string by\n   Piotr, who is also on CC:, triggered this update.\n\n http.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git c/http.c w/http.c\nindex 13fa94bef3..86ccca49f0 100644\n--- c/http.c\n+++ w/http.c\n@@ -1466,7 +1466,16 @@ struct active_request_slot *get_active_slot(void)\n \tslot->finished = NULL;\n \tslot->callback_data = NULL;\n \tslot->callback_func = NULL;\n+\n+\tif (curl_cookie_file && !strcmp(curl_cookie_file, \"-\")) {\n+\t\twarning(_(\"refusing to read cookies from http.cookiefile '-'\"));\n+\t\tFREE_AND_NULL(curl_cookie_file);\n+\t}\n \tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);\n+\tif (curl_save_cookies && (!curl_cookie_file || !curl_cookie_file[0])) {\n+\t\tcurl_save_cookies = 0;\n+\t\twarning(_(\"ignoring http.savecookies for empty http.cookiefile\"));\n+\t}\n \tif (curl_save_cookies)\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEJAR, curl_cookie_file);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, pragma_header);\n"},{"id":"498397","messageId":"20240709234941.GA1525171@coredump.intra.peff.net","threadId":"61760","inReplyTo":"xmqqed82cgmj.fsf@gitster.g","subject":"Re: [PATCH/RFC] http.c: cookie file tightening","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-09T23:49:41Z","receivedAt":"2024-07-09T23:49:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2024 at 04:03:48PM -0700, Junio C Hamano wrote:\n\n> The manual pages https://curl.se/libcurl/c/CURLOPT_COOKIEFILE.html\n> and https://curl.se/libcurl/c/CURLOPT_COOKIEJAR.html talk about two\n> interesting special values.\n> \n>  * \"\" (an empty string) given to CURLOPT_COOKIEFILE means not to\n>    read cookies from any file upon startup.\n> \n>  * It is not specified what \"\" (an empty string) given to\n>    CURLOPT_COOKIEJAR does; presumably open a file whose name is an\n>    empty string and write cookies to it?  In any case, that is not\n>    what we want to see happen, ever.\n> \n>  * \"-\" (a dash) given to CURLOPT_COOKIEFILE makes cURL read cookies\n>    from the standard input, and given to CURLOPT_COOKIEJAR makes\n>    cURL write cookies to the standard output.  Neither of which we\n>    want ever to happen.\n> \n> So, let's make sure we avoid these nonsense cases.  Specifically,\n> when http.cookies is set to \"-\", ignore it with a warning, and when\n> it is set to \"\" and http.savecookies is set, ignore http.savecookies\n> with a warning.\n>\n> [...]\n>\n>  * I have no confidence in me doing http correctly, so I am asking\n>    from folks who have touched http.c in the past 6 months for help.\n\nI don't have any experience with any of the cookie options, but your\nexplanation here all makes sense. It might be worth including a test,\nthough the interesting part is probably how things broke _before_ this\npatch. After it, it's pretty obvious what should happen.\n\nSo I'll try to comment from the general http.c perspective.\n\n> diff --git c/http.c w/http.c\n> index 13fa94bef3..86ccca49f0 100644\n> --- c/http.c\n> +++ w/http.c\n> @@ -1466,7 +1466,16 @@ struct active_request_slot *get_active_slot(void)\n>  \tslot->finished = NULL;\n>  \tslot->callback_data = NULL;\n>  \tslot->callback_func = NULL;\n> +\n> +\tif (curl_cookie_file && !strcmp(curl_cookie_file, \"-\")) {\n> +\t\twarning(_(\"refusing to read cookies from http.cookiefile '-'\"));\n> +\t\tFREE_AND_NULL(curl_cookie_file);\n> +\t}\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);\n> +\tif (curl_save_cookies && (!curl_cookie_file || !curl_cookie_file[0])) {\n> +\t\tcurl_save_cookies = 0;\n> +\t\twarning(_(\"ignoring http.savecookies for empty http.cookiefile\"));\n> +\t}\n>  \tif (curl_save_cookies)\n>  \t\tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEJAR, curl_cookie_file);\n\nThis all looks OK to me. A few things I wondered while reading:\n\n  - is curl_cookie_file always an allocated string? The answer is yes,\n    because it comes from git_config_pathname(). Good.\n\n  - get_active_slot() will be called a lot of times, as we reuse the\n    curl handles over and over (the \"slot\" terminology is due to the\n    parallelism for dumb-http fetch; smart-http just reuses the one\n    handle each time). So is this the best place to put the check?\n\n    You actually unset the options when issuing the warning, so we'd\n    never see the warning multiple times, even if this code is run\n    repeatedly. Good.\n\n    I do suspect these curl_easy_setopt() calls for cookies could just\n    go into get_curl_handle(), which sets up the handle initially. But\n    it's possible there's some subtle reason why they're here, and\n    certainly moving them is orthogonal to your goal. And in the\n    meantime, putting your new checks alongside the use of the variables\n    makes sense.\n\n-Peff\n"},{"id":"498436","messageId":"2d76e43e-db79-4572-8f41-60fbbea10af6@gmail.com","threadId":"61760","inReplyTo":"20240709234941.GA1525171@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] http.c: cookie file tightening","fromName":"Piotr Szlazak","fromEmail":"piotr.szlazak@gmail.com","sentAt":"2024-07-10T10:35:02Z","receivedAt":"2024-07-10T10:35:07Z","isPatch":true,"sender":{"key":"piotr.szlazak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11884243?v=4"},"body":"On 10.07.2024 01:49, Jeff King wrote:\n> On Tue, Jul 09, 2024 at 04:03:48PM -0700, Junio C Hamano wrote:\n>\n>> The manual pages https://curl.se/libcurl/c/CURLOPT_COOKIEFILE.html\n>> and https://curl.se/libcurl/c/CURLOPT_COOKIEJAR.html talk about two\n>> interesting special values.\n>>\n>>   * \"\" (an empty string) given to CURLOPT_COOKIEFILE means not to\n>>     read cookies from any file upon startup.\n>>\n>>   * It is not specified what \"\" (an empty string) given to\n>>     CURLOPT_COOKIEJAR does; presumably open a file whose name is an\n>>     empty string and write cookies to it?  In any case, that is not\n>>     what we want to see happen, ever.\n>>\n>>   * \"-\" (a dash) given to CURLOPT_COOKIEFILE makes cURL read cookies\n>>     from the standard input, and given to CURLOPT_COOKIEJAR makes\n>>     cURL write cookies to the standard output.  Neither of which we\n>>     want ever to happen.\n>>\n>> So, let's make sure we avoid these nonsense cases.  Specifically,\n>> when http.cookies is set to \"-\", ignore it with a warning, and when\n>> it is set to \"\" and http.savecookies is set, ignore http.savecookies\n>> with a warning.\n>>\n>> [...]\n>>\n>>   * I have no confidence in me doing http correctly, so I am asking\n>>     from folks who have touched http.c in the past 6 months for help.\n> I don't have any experience with any of the cookie options, but your\n> explanation here all makes sense. It might be worth including a test,\n> though the interesting part is probably how things broke _before_ this\n> patch. After it, it's pretty obvious what should happen.\n>\n> So I'll try to comment from the general http.c perspective.\n\nHello!\nI'm able to perform some checks as I have Git repository behind HAProxy \nload balancer which sets HTTP cookie to record which backend should \nprocess consecutive requests.\n\nIndeed, if http.cookieFile='-' is used, git stops and waits for input. \nIt does *not* work even if I do:\n$ echo '/path/to/file' | git -c http.cookieFile='-' ...\n\nOn the other hand there is no problem if http.cookieFile='' and \nhttp.saveCookies=true is used together. Git operation is successful. But \nif GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 is enabled, I can see \nfollowing warning it the output:\n > 12:19:56.280263 http.c:820 == Info: WARNING: failed to save cookies in\nIt comes from:\nhttps://github.com/curl/curl/blob/master/lib/cookie.c#L1758\nBut cookies were accepted by the client and sent back to the server.\n\nPS. I'm using Git 2.42.0.\n\nRegards!\n-- \nPiotr Szlazak\n\n>\n>> diff --git c/http.c w/http.c\n>> index 13fa94bef3..86ccca49f0 100644\n>> --- c/http.c\n>> +++ w/http.c\n>> @@ -1466,7 +1466,16 @@ struct active_request_slot *get_active_slot(void)\n>>   \tslot->finished = NULL;\n>>   \tslot->callback_data = NULL;\n>>   \tslot->callback_func = NULL;\n>> +\n>> +\tif (curl_cookie_file && !strcmp(curl_cookie_file, \"-\")) {\n>> +\t\twarning(_(\"refusing to read cookies from http.cookiefile '-'\"));\n>> +\t\tFREE_AND_NULL(curl_cookie_file);\n>> +\t}\n>>   \tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);\n>> +\tif (curl_save_cookies && (!curl_cookie_file || !curl_cookie_file[0])) {\n>> +\t\tcurl_save_cookies = 0;\n>> +\t\twarning(_(\"ignoring http.savecookies for empty http.cookiefile\"));\n>> +\t}\n>>   \tif (curl_save_cookies)\n>>   \t\tcurl_easy_setopt(slot->curl, CURLOPT_COOKIEJAR, curl_cookie_file);\n> This all looks OK to me. A few things I wondered while reading:\n>\n>    - is curl_cookie_file always an allocated string? The answer is yes,\n>      because it comes from git_config_pathname(). Good.\n>\n>    - get_active_slot() will be called a lot of times, as we reuse the\n>      curl handles over and over (the \"slot\" terminology is due to the\n>      parallelism for dumb-http fetch; smart-http just reuses the one\n>      handle each time). So is this the best place to put the check?\n>\n>      You actually unset the options when issuing the warning, so we'd\n>      never see the warning multiple times, even if this code is run\n>      repeatedly. Good.\n>\n>      I do suspect these curl_easy_setopt() calls for cookies could just\n>      go into get_curl_handle(), which sets up the handle initially. But\n>      it's possible there's some subtle reason why they're here, and\n>      certainly moving them is orthogonal to your goal. And in the\n>      meantime, putting your new checks alongside the use of the variables\n>      makes sense.\n>\n> -Peff\n"},{"id":"498467","messageId":"xmqqy169xl3p.fsf@gitster.g","threadId":"61760","inReplyTo":"2d76e43e-db79-4572-8f41-60fbbea10af6@gmail.com","subject":"Re: [PATCH/RFC] http.c: cookie file tightening","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-10T16:33:46Z","receivedAt":"2024-07-10T16:33:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Piotr Szlazak <piotr.szlazak@gmail.com> writes:\n\n> On the other hand there is no problem if http.cookieFile='' and\n> http.saveCookies=true is used together. Git operation is\n> successful. But if GIT_TRACE_CURL=1 GIT_TRACE_CURL_NO_DATA=1 is\n> enabled, I can see following warning it the output:\n>> 12:19:56.280263 http.c:820 == Info: WARNING: failed to save cookies in\n> It comes from:\n> https://github.com/curl/curl/blob/master/lib/cookie.c#L1758\n> But cookies were accepted by the client and sent back to the server.\n\nThanks for your experiments.\n\nI do not know if it is safe to call the above observed sympotom\n\"there is no problem\".  What does it even mean to set cookieFile to\nan empty string and ask the cookies to be saved?  What does the user\nwho makes such a pair of requests\n\n\t[http]\n\t\tsaveCookies = yes\n\t\tcookieFile = \"\"\n\nexpect to happen?  The session begins with an empty set of cookies,\ncookies that come from the other side are maintained in-core during\nthe session, and then at the very end of the session what do they\nwant to happen to their cookies?  \"The system will try to save them\nbut without finding a sensible place to save, it gives a warning\nwithout molesting the main goal of the process (which is to interact\nwith the other side)\" sounds like a rather strange wish.\n\nI'd consider \"The system notices that there is no sensible place to\nstore, so it warns about the conflicting request and ignores\nhttp.saveCookies\" a bit more sensible behaviour in such a situation,\nbut obviously I am biased.\n"}]}