{"thread":{"id":"45664","subject":"[PATCH v3 0/2] http: few fixes for the proxy configuration handling","startedAt":"2017-04-11T17:18:04Z","lastAt":"2017-04-11T20:51:24Z","messageCount":5,"participants":["Sergey Ryazanov","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"316605","messageId":"20170411171750.18624-1-ryazanov.s.a@gmail.com","threadId":"45664","inReplyTo":null,"subject":"[PATCH v3 0/2] http: few fixes for the proxy configuration handling","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-11T17:17:48Z","receivedAt":"2017-04-11T17:18:04Z","isPatch":true,"sender":{"key":"ryazanov.s.a@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11219080?v=4"},"body":"Hello,\n\nthis is few patches, which fixes regressions in the proxy handling.\n\nChanges since v2:\n - fix grammar (thanks to Ævar)\n - add new patch which fixes the silent ignoring of proxy missconfiguration\n\nSergey Ryazanov (2):\n http: honor empty http.proxy option to bypass proxy\n http: fix the silent ignoring of proxy misconfiguraion\n\n http.c | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\n-- \n2.10.2\n\n"},{"id":"316607","messageId":"20170411171750.18624-3-ryazanov.s.a@gmail.com","threadId":"45664","inReplyTo":"20170411171750.18624-1-ryazanov.s.a@gmail.com","subject":"[PATCH v3 2/2] http: fix the silent ignoring of proxy misconfiguraion","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-11T17:17:50Z","receivedAt":"2017-04-11T17:18:09Z","isPatch":true,"sender":{"key":"ryazanov.s.a@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11219080?v=4"},"body":"Earlier, the whole http.proxy option string was passed to curl without\nany preprocessing so curl could complain about the invalid proxy\nconfiguration.\n\nAfter the commit 372370f167 (\"http: use credential API to handle proxy\nauthentication\", 2016-01-26), if the user specified an invalid HTTP\nproxy option in the configuration, then the option parsing is silently\nfails and NULL will be passed to curl as a proxy. This forces curl to\nfall back to detecting the proxy configuration from the environment,\ncausing the http.proxy option ignoring.\n\nFix this issue by checking the proxy option parsing result. If parsing\nfailed then print error message and die. Such behaviour allows user to\nquickly figure the proxy misconfiguration and correct it.\n\nHelped-by: Jeff King \nSigned-off-by: Sergey Ryazanov \n---\n\nChanges since v2:\n - new patch\n\n http.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 8be75b2..82664dd 100644\n--- a/http.c\n+++ b/http.c\n@@ -867,6 +867,9 @@ static CURL *get_curl_handle(void)\n \t\t\tstrbuf_release(&url);\n \t\t}\n \n+\t\tif (!proxy_auth.host)\n+\t\t\tdie(\"Invalid proxy URL '%s'\", curl_http_proxy);\n+\n \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-- \n2.10.2\n\n"},{"id":"316608","messageId":"20170411171750.18624-2-ryazanov.s.a@gmail.com","threadId":"45664","inReplyTo":"20170411171750.18624-1-ryazanov.s.a@gmail.com","subject":"[PATCH v3 1/2] http: honor empty http.proxy option to bypass proxy","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-11T17:17:49Z","receivedAt":"2017-04-11T17:18:10Z","isPatch":true,"sender":{"key":"ryazanov.s.a@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11219080?v=4"},"body":"Curl distinguishes between an empty proxy address and a NULL proxy\naddress. In the first case it completely disables proxy usage, but if\nthe proxy address option is NULL then curl attempts to determine the\nproxy address from the http_proxy environment variable.\n\nAccording to the documentation, if the http.proxy option is set to an\nempty string, git should bypass proxy and connect to the server\ndirectly:\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 372370f167 (\"http: use credential API to handle proxy\nauthentication\", 2016-01-26) parses the proxy option, then extracts the\nproxy host address and updates the curl configuration, making the\nprevious 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 the proxy option is empty then the proxy host field becomes NULL.\nThis forces curl to fall back to detecting the proxy configuration from\nthe environment, causing the http.proxy option to not work anymore.\n\nFix this issue by explicitly handling http.proxy being set the empty\nstring. This also makes the code a bit more clear and should help us\navoid such regressions in the future.\n\nHelped-by: Ævar Arnfjörð Bjarmason \nHelped-by: Jeff King \nSigned-off-by: Sergey Ryazanov \n---\n\nChanges since v1:\n - explicitly handle this case instead of mangling the common code\n\nChanges since v2:\n - fix grammar (thanks to Ævar)\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":"316614","messageId":"20170411173722.asjrkpbbm4p6k6ov@sigill.intra.peff.net","threadId":"45664","inReplyTo":"20170411171750.18624-3-ryazanov.s.a@gmail.com","subject":"Re: [PATCH v3 2/2] http: fix the silent ignoring of proxy misconfiguraion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T17:37:22Z","receivedAt":"2017-04-11T17:37:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 08:17:50PM +0300, Sergey Ryazanov wrote:\n\n> Earlier, the whole http.proxy option string was passed to curl without\n> any preprocessing so curl could complain about the invalid proxy\n> configuration.\n> \n> After the commit 372370f167 (\"http: use credential API to handle proxy\n> authentication\", 2016-01-26), if the user specified an invalid HTTP\n> proxy option in the configuration, then the option parsing is silently\n> fails and NULL will be passed to curl as a proxy. This forces curl to\n\ns/is silently/silently/\n\n> fall back to detecting the proxy configuration from the environment,\n> causing the http.proxy option ignoring.\n> \n> Fix this issue by checking the proxy option parsing result. If parsing\n> failed then print error message and die. Such behaviour allows user to\n> quickly figure the proxy misconfiguration and correct it.\n\nTwo minor grammos:\n\ns/error/an error/;\ns/user/the user/;\n\nIn the earlier discussion you mentioned a warning, but I like this die()\nmuch better.\n\nBoth patches look very clean, and nicely explained. Thanks for working\non this.\n\n-Peff\n"},{"id":"316645","messageId":"CAHNKnsT6-U2TY0KVNGXXkWytZt-ixqDM-WR3Qcq1A-3+NgxAUQ@mail.gmail.com","threadId":"45664","inReplyTo":"20170411173722.asjrkpbbm4p6k6ov@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] http: fix the silent ignoring of proxy misconfiguraion","fromName":"Sergey Ryazanov","fromEmail":"ryazanov.s.a@gmail.com","sentAt":"2017-04-11T20:50:57Z","receivedAt":"2017-04-11T20:51:24Z","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 8:37 PM, Jeff King wrote:\n> On Tue, Apr 11, 2017 at 08:17:50PM +0300, Sergey Ryazanov wrote:\n>> Earlier, the whole http.proxy option string was passed to curl without\n>> any preprocessing so curl could complain about the invalid proxy\n>> configuration.\n>>\n>> After the commit 372370f167 (\"http: use credential API to handle proxy\n>> authentication\", 2016-01-26), if the user specified an invalid HTTP\n>> proxy option in the configuration, then the option parsing is silently\n>> fails and NULL will be passed to curl as a proxy. This forces curl to\n>\n> s/is silently/silently/\n>\n>> fall back to detecting the proxy configuration from the environment,\n>> causing the http.proxy option ignoring.\n>>\n>> Fix this issue by checking the proxy option parsing result. If parsing\n>> failed then print error message and die. Such behaviour allows user to\n>> quickly figure the proxy misconfiguration and correct it.\n>\n> Two minor grammos:\n>\n> s/error/an error/;\n> s/user/the user/;\n>\n\nThank you. Just sent a series with suggested grammar fixes as v4.\n\n> In the earlier discussion you mentioned a warning, but I like this die()\n> much better.\n>\n\nI actually meant \"die\" but by some reason I typed \"warning\" :-/\n\n> Both patches look very clean, and nicely explained. Thanks for working\n> on this.\n>\n\nPeff, I would like to thank you and Ævar for your great help!\n\n-- \nSergey\n"}]}