{"thread":{"id":"45650","subject":"[PATCH] http: honnor empty http.proxy option to bypass proxy","startedAt":"2017-04-10T15:16:14Z","lastAt":"2017-04-11T08:24:32Z","messageCount":3,"participants":["Sergey Ryazanov","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316495","messageId":"20170410151556.10054-1-ryazanov.s.a@gmail.com","threadId":"45650","inReplyTo":null,"subject":"[PATCH] http: honnor empty http.proxy option to bypass proxy","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-10T15:15:56Z","receivedAt":"2017-04-10T15:16:14Z","isPatch":true,"sender":{"key":"ryazanov.s.a@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11219080?v=4"},"body":"Curl distinguish between empty proxy address and NULL proxy address. In\nthe first case it completly disable proxy usage, but if proxy address\noption is NULL then curl attempt to determine proxy address from\nhttp_proxy environment variable.\n\nAccording to documentation, if http.proxy configured to empty string\nthen git should bypass proxy and connects to the server directly:\n\n    export http_proxy=http://network-proxy/\n    cd ~/foobar-project\n    git config remote.origin.proxy \"\"\n    git fetch\n\nPreviously, proxy host was configured by one line:\n\n    curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n\nCommit 372370f (http: use credential API to handle proxy auth...) parses\nproxy option, extracts proxy host address and additionaly updates curl\nconfiguration:\n\n    credential_from_url(&proxy_auth, curl_http_proxy);\n    curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n\nBut if proxy option is empty then proxy host field become NULL this\nforce curl to fallback to proxy configuration detection from\nenvironment. This caused empty http.proxy option not working any more.\n\nAvoid setting NULL CURLOPT_PROXY from proxy_auth.host to fix this issue.\n\nSigned-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>\n---\n http.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 96d84bb..bf0e709 100644\n--- a/http.c\n+++ b/http.c\n@@ -861,7 +861,12 @@ static CURL *get_curl_handle(void)\n \t\t\tstrbuf_release(&url);\n \t\t}\n \n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n+\t\t/*\n+\t\t * Avoid setting CURLOPT_PROXY to NULL if empty http.proxy\n+\t\t * option configured.\n+\t\t */\n+\t\tif (proxy_auth.host)\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n #if LIBCURL_VERSION_NUM >= 0x071304\n \t\tvar_override(&curl_no_proxy, getenv(\"NO_PROXY\"));\n \t\tvar_override(&curl_no_proxy, getenv(\"no_proxy\"));\n-- \n2.10.2\n\n"},{"id":"316497","messageId":"20170410163350.3zqm33tgqafsp76u@sigill.intra.peff.net","threadId":"45650","inReplyTo":"20170410151556.10054-1-ryazanov.s.a@gmail.com","subject":"Re: [PATCH] http: honnor empty http.proxy option to bypass proxy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T16:33:50Z","receivedAt":"2017-04-10T16:33:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 10, 2017 at 06:15:56PM +0300, Sergey Ryazanov wrote:\n\n> Curl distinguish between empty proxy address and NULL proxy address. In\n> the first case it completly disable proxy usage, but if proxy address\n> option is NULL then curl attempt to determine proxy address from\n> http_proxy environment variable.\n> \n> According to documentation, if http.proxy configured to empty string\n> then git should bypass proxy and connects to the server directly:\n> \n>     export http_proxy=http://network-proxy/\n>     cd ~/foobar-project\n>     git config remote.origin.proxy \"\"\n>     git fetch\n> \n> Previously, proxy host was configured by one line:\n> \n>     curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> \n> Commit 372370f (http: use credential API to handle proxy auth...) parses\n> proxy option, extracts proxy host address and additionaly updates curl\n> configuration:\n> \n>     credential_from_url(&proxy_auth, curl_http_proxy);\n>     curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n> \n> But if proxy option is empty then proxy host field become NULL this\n> force curl to fallback to proxy configuration detection from\n> environment. This caused empty http.proxy option not working any more.\n\nThat makes sense. And if I understand correctly, this was a regression\nin 372370f; before that we fed curl_http_proxy directly, and it was\neither NULL or not, depending on whether we had seen the config option.\n\nIt looks like we _still_ set CURLOPT_PROXY to curl_http_proxy, and then\nimmediately afterward set it to proxy_auth.host. That should make the\nfirst one always a noop, I would think, and it should be removed.\n\nBut...\n\n> diff --git a/http.c b/http.c\n> index 96d84bb..bf0e709 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -861,7 +861,12 @@ static CURL *get_curl_handle(void)\n>  \t\t\tstrbuf_release(&url);\n>  \t\t}\n>  \n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n> +\t\t/*\n> +\t\t * Avoid setting CURLOPT_PROXY to NULL if empty http.proxy\n> +\t\t * option configured.\n> +\t\t */\n> +\t\tif (proxy_auth.host)\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n\nHere that second one becomes conditional, and we rely on the earlier\nsetting (but only sometimes). I would think this whole thing would be\nmore clear if we dropped the first CURLOPT_PROXY call entirely, and just\ndid:\n\n  /*\n   * If we parsed a null host from the URL, we must convert that\n   * back into an empty string so that curl knows we want no proxy at\n   * all (not to find the default).\n   */\n  curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host ?\n                                          proxy_auth.host : \"\");\n\n-Peff\n"},{"id":"316552","messageId":"CAHNKnsRPXW+4XgMKAubv3hyaXKGVE9o6Gmuhaq2gsLyo+N7xPQ@mail.gmail.com","threadId":"45650","inReplyTo":"20170410163350.3zqm33tgqafsp76u@sigill.intra.peff.net","subject":"Re: [PATCH] http: honnor empty http.proxy option to bypass proxy","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-11T08:24:03Z","receivedAt":"2017-04-11T08:24:32Z","isPatch":true,"sender":{"key":"ryazanov.s.a@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11219080?v=4"},"body":"On Mon, Apr 10, 2017 at 7:33 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Apr 10, 2017 at 06:15:56PM +0300, Sergey Ryazanov wrote:\n>> Curl distinguish between empty proxy address and NULL proxy address. In\n>> the first case it completly disable proxy usage, but if proxy address\n>> option is NULL then curl attempt to determine proxy address from\n>> http_proxy environment variable.\n>>\n>> According to documentation, if http.proxy configured to empty string\n>> then git should bypass proxy and connects to the server directly:\n>>\n>>     export http_proxy=http://network-proxy/\n>>     cd ~/foobar-project\n>>     git config remote.origin.proxy \"\"\n>>     git fetch\n>>\n>> Previously, proxy host was configured by one line:\n>>\n>>     curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n>>\n>> Commit 372370f (http: use credential API to handle proxy auth...) parses\n>> proxy option, extracts proxy host address and additionaly updates curl\n>> configuration:\n>>\n>>     credential_from_url(&proxy_auth, curl_http_proxy);\n>>     curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n>>\n>> But if proxy option is empty then proxy host field become NULL this\n>> force curl to fallback to proxy configuration detection from\n>> environment. This caused empty http.proxy option not working any more.\n>\n> That makes sense. And if I understand correctly, this was a regression\n> in 372370f;\n\nYep this regression introduced in 372370f\n\n> before that we fed curl_http_proxy directly, and it was\n> either NULL or not, depending on whether we had seen the config option.\n\nTo be more precisely, before the mentioned changeset was committed we\neither set CURLOPT_PROXY option to some not NULL value (including\nempty string) or do not touch it at all. There are additional test few\nlines above the discussed code.\n\n> It looks like we _still_ set CURLOPT_PROXY to curl_http_proxy, and then\n> immediately afterward set it to proxy_auth.host. That should make the\n> first one always a noop, I would think,\n\nYep, exactly.\n\n> and it should be removed.\n>\n>\n> But...\n>\n>> diff --git a/http.c b/http.c\n>> index 96d84bb..bf0e709 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -861,7 +861,12 @@ static CURL *get_curl_handle(void)\n>>                       strbuf_release(&url);\n>>               }\n>>\n>> -             curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n>> +             /*\n>> +              * Avoid setting CURLOPT_PROXY to NULL if empty http.proxy\n>> +              * option configured.\n>> +              */\n>> +             if (proxy_auth.host)\n>> +                     curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n>\n> Here that second one becomes conditional, and we rely on the earlier\n> setting (but only sometimes). I would think this whole thing would be\n> more clear if we dropped the first CURLOPT_PROXY call entirely, and just\n> did:\n>\n>   /*\n>    * If we parsed a null host from the URL, we must convert that\n>    * back into an empty string so that curl knows we want no proxy at\n>    * all (not to find the default).\n>    */\n>   curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host ?\n>                                           proxy_auth.host : \"\");\n>\n\nEmpty http.proxy option value is an edge case. Until we started parse\nproxy value itself, all the complexity was reside inside curl library.\nSince 372370f commit we should take some care about this case in our\ncode too. This could be a tiny hack inside common code to make patch\nless invasive. Or we could explicitly handle this case to make code\nclear and avoid such regressions in the future.\n\nI will make V2 with second approach to see which one is better.\n\n-- \nSergey\n"}]}