{"thread":{"id":"45661","subject":"[PATCH v2] http: honnor empty http.proxy option to bypass proxy","startedAt":"2017-04-11T09:21:32Z","lastAt":"2017-04-11T14:59:25Z","messageCount":6,"participants":["Sergey Ryazanov","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"316561","messageId":"20170411092050.15867-1-ryazanov.s.a@gmail.com","threadId":"45661","inReplyTo":null,"subject":"[PATCH v2] http: honnor empty http.proxy option to bypass proxy","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-11T09:20:50Z","receivedAt":"2017-04-11T09:21:32Z","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, which makes previous call a noop:\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\nFix this issue by explicitly handling empty http.proxy option. This also\nmakes code a bit more clear and should help us avoid such regressions in\nthe future.\n\nSigned-off-by: Sergey Ryazanov \n---\n\nChanges since v1:\n - explicitly handle this case instead of mangling the common code\n\n http.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 96d84bb..8be75b2 100644\n--- a/http.c\n+++ b/http.c\n@@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)\n \t\t}\n \t}\n \n-\tif (curl_http_proxy) {\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\tif (curl_http_proxy && curl_http_proxy[0] == '\\0') {\n+\t\t/*\n+\t\t * Handle case with the empty http.proxy value here to keep\n+\t\t * common code clean.\n+\t\t * NB: empty option disables proxying at all.\n+\t\t */\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, \"\");\n+\t} else if (curl_http_proxy) {\n #if LIBCURL_VERSION_NUM >= 0x071800\n \t\tif (starts_with(curl_http_proxy, \"socks5h\"))\n \t\t\tcurl_easy_setopt(result,\n-- \n2.10.2\n\n"},{"id":"316564","messageId":"CACBZZX4QfeiL7kB2+3pPbKjPGnQRXrGooQ-QrpgUtPnZRF=o8A@mail.gmail.com","threadId":"45661","inReplyTo":"20170411092050.15867-1-ryazanov.s.a@gmail.com","subject":"Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-11T10:09:46Z","receivedAt":"2017-04-11T10:11:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since nobody pointed this out already, some grammar/spelling fixes.\nAlso CC-ing Knut who wrote the commit you're referencing.\n\n- http: honnor empty http.proxy option to bypass proxy\n+ http: honor empty http.proxy option to bypass proxy\n\nOn Tue, Apr 11, 2017 at 11:20 AM, Sergey Ryazanov\n wrote:\n> Curl distinguish between empty proxy address and NULL proxy address. In\n\n\n...distinguishes between an empty proxy address and a NULL proxy address...\n\n> the first case it completly disable proxy usage, but if proxy address\n\nit completely disables ... but if the proxy ....\n\n> option is NULL then curl attempt to determine proxy address from\n\n...attempts to determine the proxy ...\n\n> http_proxy environment variable.\n\n... the http_....\n\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...to the documentation, if http_proxy is set to an empty string, git\nshould bypass the proxy and connect to the server directly.\n\n\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, which makes previous call a noop:\n\n[Using the format we usually cite commits with, see SubmittingPatches]\n\nCommit 372370f167 (\"http: use credential API to handle proxy\nauthentication\", 2016-01-26) parses the proxy option, then extracts\nthe proxy host address and updates the curl configuration, making the\nprevious call a noop.\n\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\nBut if the proxy option is empty then the proxy host field becomes\nNULL. This forces curl to fall back to detecting the proxy\nconfiguration from the environment, causing the http.rpoxy option to\nnot work anymore.\n\n>\n> Fix this issue by explicitly handling empty http.proxy option. This also\n> makes code a bit more clear and should help us avoid such regressions in\n> the future.\n\nFix this issue by explicitly handling http.proxy being set to the\nempty string. This also makes the code a ....\n\n>\n> Signed-off-by: Sergey Ryazanov \n> ---\n>\n> Changes since v1:\n> - explicitly handle this case instead of mangling the common code\n>\n> http.c | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 96d84bb..8be75b2 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)\n> }\n> }\n>\n> - if (curl_http_proxy) {\n> - curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> + if (curl_http_proxy && curl_http_proxy[0] == '\\0') {\n> + /*\n> + * Handle case with the empty http.proxy value here to keep\n> + * common code clean.\n> + * NB: empty option disables proxying at all.\n> + */\n> + curl_easy_setopt(result, CURLOPT_PROXY, \"\");\n> + } else if (curl_http_proxy) {\n> #if LIBCURL_VERSION_NUM >= 0x071800\n> if (starts_with(curl_http_proxy, \"socks5h\"))\n> curl_easy_setopt(result,\n> --\n> 2.10.2\n>\n"},{"id":"316568","messageId":"CAHNKnsQvGrDnp9fivTwJxw1JCP+R3VXQKNS2kKvsaG=sWhRxTg@mail.gmail.com","threadId":"45661","inReplyTo":"CACBZZX4QfeiL7kB2+3pPbKjPGnQRXrGooQ-QrpgUtPnZRF=o8A@mail.gmail.com","subject":"Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-11T10:21:49Z","receivedAt":"2017-04-11T10:22:41Z","isPatch":true,"sender":{"key":"ryazanov.s.a@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11219080?v=4"},"body":"On Tue, Apr 11, 2017 at 1:09 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Since nobody pointed this out already, some grammar/spelling fixes.\n> Also CC-ing Knut who wrote the commit you're referencing.\n>\nThanks a lot! Will fix in v3.\n\n-- \nSergey\n"},{"id":"316593","messageId":"20170411130659.ehit7jdhnk43m23g@sigill.intra.peff.net","threadId":"45661","inReplyTo":"20170411092050.15867-1-ryazanov.s.a@gmail.com","subject":"Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T13:06:59Z","receivedAt":"2017-04-11T13:07:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 12:20:50PM +0300, Sergey Ryazanov wrote:\n\n> diff --git a/http.c b/http.c\n> index 96d84bb..8be75b2 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)\n> \t\t}\n> \t}\n> \n> -\tif (curl_http_proxy) {\n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\tif (curl_http_proxy && curl_http_proxy[0] == '\\0') {\n> +\t\t/*\n> +\t\t * Handle case with the empty http.proxy value here to keep\n> +\t\t * common code clean.\n> +\t\t * NB: empty option disables proxying at all.\n> +\t\t */\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXY, \"\");\n> +\t} else if (curl_http_proxy) {\n\nThis seems pretty reasonable. You could bump this under the existing \"if\n(curl_http_proxy)\" condition, but that would add an extra level of\nindentation to the rest of the parsing.\n\nOne thing I do wonder, though: can credential_from_url() ever return a\nNULL host field for other inputs, and what should we do on those inputs?\n\nFor example, if I set the value to just \"https://\" with no hostname,\nthen I think we'll end up with a NULL proxy_auth.host field. And we'll\nfeed that NULL to CURLOPT_PROXY, which will cause it to use the\ndefaults.\n\nI don't know _what_ \"https://\" should do. It's clearly bogus. But\ntelling curl to use the defaults seems funny. In that sense, your\noriginal was much better (we'd feed it to curl, which would be free to\ncomplain).\n\n-Peff\n"},{"id":"316596","messageId":"CAHNKnsRxNMS4fYTaoHSRgcTmCr+rcjVOGE1kqJGHqLTgBxdC0w@mail.gmail.com","threadId":"45661","inReplyTo":"20170411130659.ehit7jdhnk43m23g@sigill.intra.peff.net","subject":"Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-11T14:56:56Z","receivedAt":"2017-04-11T14:57:22Z","isPatch":true,"sender":{"key":"ryazanov.s.a@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11219080?v=4"},"body":"On Tue, Apr 11, 2017 at 4:06 PM, Jeff King wrote:\n> On Tue, Apr 11, 2017 at 12:20:50PM +0300, Sergey Ryazanov wrote:\n>> diff --git a/http.c b/http.c\n>> index 96d84bb..8be75b2 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)\n>> }\n>> }\n>>\n>> - if (curl_http_proxy) {\n>> - curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n>> + if (curl_http_proxy && curl_http_proxy[0] == '\\0') {\n>> + /*\n>> + * Handle case with the empty http.proxy value here to keep\n>> + * common code clean.\n>> + * NB: empty option disables proxying at all.\n>> + */\n>> + curl_easy_setopt(result, CURLOPT_PROXY, \"\");\n>> + } else if (curl_http_proxy) {\n>\n> This seems pretty reasonable. You could bump this under the existing \"if\n> (curl_http_proxy)\" condition, but that would add an extra level of\n> indentation to the rest of the parsing.\n>\nBecause of this additional indentation I decide to add new if () block.\n\n> One thing I do wonder, though: can credential_from_url() ever return a\n> NULL host field for other inputs, and what should we do on those inputs?\n>\n> For example, if I set the value to just \"https://\" with no hostname,\n> then I think we'll end up with a NULL proxy_auth.host field. And we'll\n> feed that NULL to CURLOPT_PROXY, which will cause it to use the\n> defaults.\n>\n> I don't know _what_ \"https://\" should do. It's clearly bogus. But\n> telling curl to use the defaults seems funny. In that sense, your\n> original was much better (we'd feed it to curl, which would be free to\n> complain).\n>\n\nI thought about this situation too. IMHO the best solution here is to\ncheck host after credential_from_url() call. If host is NULL then we\nshould show warning message and exit. Then user could fix its\nconfiguration.\n\nSince I in any case will send v3 with grammar fixes then I could add\nnew patch to the series. This new patch will check host for NULL and\nprint warning message. Are you Ok with such solution?\n\n-- \nSergey\n"},{"id":"316597","messageId":"20170411145917.mjhwfgc5vmgwtx5x@sigill.intra.peff.net","threadId":"45661","inReplyTo":"CAHNKnsRxNMS4fYTaoHSRgcTmCr+rcjVOGE1kqJGHqLTgBxdC0w@mail.gmail.com","subject":"Re: [PATCH v2] http: honnor empty http.proxy option to bypass proxy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T14:59:17Z","receivedAt":"2017-04-11T14:59:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 05:56:56PM +0300, Sergey Ryazanov wrote:\n\n> > I don't know _what_ \"https://\" should do. It's clearly bogus. But\n> > telling curl to use the defaults seems funny. In that sense, your\n> > original was much better (we'd feed it to curl, which would be free to\n> > complain).\n> >\n> \n> I thought about this situation too. IMHO the best solution here is to\n> check host after credential_from_url() call. If host is NULL then we\n> should show warning message and exit. Then user could fix its\n> configuration.\n> \n> Since I in any case will send v3 with grammar fixes then I could add\n> new patch to the series. This new patch will check host for NULL and\n> print warning message. Are you Ok with such solution?\n\nYeah, I think that would be fine. It pretty clearly seems like an error,\nso any behavior that isn't \"quietly pretend that there is no proxy\" is\nprobably fine.\n\n-Peff\n"}]}