settings: match curl lowercase proxy envar behaviour

mjcheetham · mjcheetham · commit 81cdb218c977 · 2021-11-02T10:33:38.000Z
libcurl supports multiple different environment variables [1] to configure proxy behaviour: http_proxy, https_proxy, all_proxy, and no_proxy. Unlike most other environment variables these proxy envars are normally _lowercase_, not uppercase. This convention was set by libwww back in the early 1990s. When libcurl was first released however, it was not aware of this schism and only implemented checks for uppercase variants of these envars: HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, and NO_PROXY. In time, libcurl learned to also read the lowercase variants, and gives them precedence over the uppercase forms (since the former are quasi- standards). However, to further complicate the matter, libcurl no longer reads the uppercase HTTP_PROXY variable specifically. This change was made to address a security concern with some CGI webservers [2]. The problem is that today GCM only reads the uppercase variants of the environment variables! This is inconsistent with libcurl, and therefore Git's behaviour (that we aim to be consistent/co-operative with). We change GCM's behaviour to match that of libcurl/Git in that the lowercase proxy envars are preferred to the uppercase ones, and the uppercase HTTP_PROXY variable is ignored. Dropping support for the HTTP_PROXY uppercase envar is technically a breaking change, but if the user had only set this uppercase envar then Git would not be proxying the actual remote calls, only GCM, which is most likely not what the user wanted. [1] https://everything.curl.dev/usingcurl/proxies#proxy-environment-variables [2] https://everything.curl.dev/usingcurl/proxies#http_proxy-in-lower-case-only
diff --git a/docs/netconfig.md b/docs/netconfig.md
@@ -49,21 +49,35 @@ GCM Core supports other ways of configuring a proxy for convenience and compatib
 1. GCM-specific configuration options (_**only** respected by GCM; **deprecated**_):
    - `credential.httpProxy`
    - `credential.httpsProxy`
-1. cURL environment variables (_also respected by Git_):
-   - `HTTP_PROXY`
-   - `HTTPS_PROXY`
-   - `ALL_PROXY`
-1. `GCM_HTTP_PROXY` environment variable (_**only** respected by GCM; **deprecated**_)
+2. cURL environment variables (_also respected by Git_):
+   - `http_proxy`
+   - `https_proxy`/`HTTPS_PROXY`
+   - `all_proxy`/`ALL_PROXY`
+3. `GCM_HTTP_PROXY` environment variable (_**only** respected by GCM; **deprecated**_)
+
+Note that with the cURL environment variables there are both lowercase and
+uppercase variants.
+
+**_Lowercase variants take precedence over the uppercase form._** This is
+consistent with how libcurl (and therefore Git) operates.
+
+The `http_proxy` variable exists only in the lowercase variant and libcurl does
+_not_ consider any uppercase form. _GCM Core also reflects this behavior._
+
+See <https://everything.curl.dev/usingcurl/proxies#proxy-environment-variables>
+for more information.
 
 ### Bypassing addresses
 
 In some circumstances you may wish to bypass a configured proxy for specific
-addresses. GCM Core supports the cURL environment variable `NO_PROXY` for this
-scenariom, as does Git itself.
+addresses. GCM Core supports the cURL environment variable `no_proxy` (and
+`NO_PROXY`) for this scenario, as does Git itself.
+
+Like with the [other cURL proxy environment variables](#other-proxy-options),
+the lowercase variant will take precedence over the uppercase form.
 
-The `NO_PROXY` environment variable should contain a comma (`,`) or space (` `)
-separated list of host names that should not be proxied (should connect
-directly).
+This environment variable should contain a comma (`,`) or space (` `) separated
+list of host names that should not be proxied (should connect directly).
 
 GCM Core attempts to match [libcurl's behaviour](https://curl.se/libcurl/c/CURLOPT_NOPROXY.html),
 which is briefly summarized here:
@@ -88,7 +102,7 @@ Hostname|Matches?
 **Example:**
 
 ```text
-NO_PROXY="contoso.com,www.fabrikam.com"
+no_proxy="contoso.com,www.fabrikam.com"
 ```
 
 ## TLS Verification
diff --git a/src/shared/Core.Tests/SettingsTests.cs b/src/shared/Core.Tests/SettingsTests.cs
@@ -788,8 +788,10 @@ public void Settings_ProxyConfiguration_Precedence_ReturnsValue()
             // 2. Standard Git configuration
             //      http.proxy
             // 3. cURL environment variables
+            //      http_proxy
             //      HTTPS_PROXY
-            //      HTTP_PROXY
+            //      http_proxy (note that uppercase HTTP_PROXY is not supported by libcurl)
+            //      all_proxy
             //      ALL_PROXY
             // 4. GCM proxy environment variable (deprecated)
             //      GCM_HTTP_PROXY
@@ -820,15 +822,71 @@ void RunTest(Uri expectedValue)
             envars.Variables[Constants.EnvironmentVariables.CurlHttpProxy] = value2.ToString();
             RunTest(value2);
 
-             // Test case 2: http.proxy > cURL environment variables
-             string httpProxyKey = $"{Constants.GitConfiguration.Http.SectionName}.{Constants.GitConfiguration.Http.Proxy}";
-             git.Configuration.Global[httpProxyKey] = new[] {value3.ToString()};
-             RunTest(value3);
+            // Test case 2: http.proxy > cURL environment variables
+            string httpProxyKey = $"{Constants.GitConfiguration.Http.SectionName}.{Constants.GitConfiguration.Http.Proxy}";
+            git.Configuration.Global[httpProxyKey] = new[] {value3.ToString()};
+            RunTest(value3);
 
-             // Test case 3: credential.httpProxy > http.proxy
-             string credentialProxyKey = $"{Constants.GitConfiguration.Credential.SectionName}.{Constants.GitConfiguration.Credential.HttpProxy}";
-             git.Configuration.Global[credentialProxyKey] = new[] {value4.ToString()};
-             RunTest(value4);
+            // Test case 3: credential.httpProxy > http.proxy
+            string credentialProxyKey = $"{Constants.GitConfiguration.Credential.SectionName}.{Constants.GitConfiguration.Credential.HttpProxy}";
+            git.Configuration.Global[credentialProxyKey] = new[] {value4.ToString()};
+            RunTest(value4);
+        }
+
+        [Fact]
+        public void Settings_ProxyConfiguration_CurlEnvarPrecedence_PrefersLowercase()
+        {
+            // Expected precedence:
+            //  https_proxy
+            //  HTTPS_PROXY
+            //  http_proxy (note that uppercase HTTP_PROXY is not supported by libcurl)
+            //  all_proxy
+            //  ALL_PROXY
+
+            const string remoteUrl = "https://example.com/foo.git";
+            var remoteUri = new Uri(remoteUrl);
+
+            var value1 = new Uri("http://proxy1.example.com");
+            var value2 = new Uri("http://proxy2.example.com");
+
+            // The differentiation between upper- and lowercase environment variables is not possible
+            // on some platforms (Windows) so force the comparer to case-sensitive in the test so we
+            // can run this on all platforms.
+            var envars = new TestEnvironment(envarComparer: StringComparer.Ordinal);
+            var git = new TestGit();
+
+            void RunTest(Uri expectedValue)
+            {
+                var settings = new Settings(envars, git)
+                {
+                    RemoteUri = remoteUri
+                };
+                ProxyConfiguration actualConfig = settings.GetProxyConfiguration();
+                Assert.Equal(expectedValue, actualConfig.Address);
+            }
+
+            // Test case 1: https_proxy > HTTPS_PROXY
+            envars.Variables[Constants.EnvironmentVariables.CurlHttpsProxy] = value1.ToString();
+            envars.Variables[Constants.EnvironmentVariables.CurlHttpsProxyUpper] = value2.ToString();
+            RunTest(value1);
+
+            // Test case 2a: https_proxy > http_proxy
+            envars.Variables.Clear();
+            envars.Variables[Constants.EnvironmentVariables.CurlHttpsProxy] = value1.ToString();
+            envars.Variables[Constants.EnvironmentVariables.CurlHttpProxy] = value2.ToString();
+            RunTest(value1);
+
+            // Test case 2b: HTTPS_PROXY > http_proxy
+            envars.Variables.Clear();
+            envars.Variables[Constants.EnvironmentVariables.CurlHttpsProxyUpper] = value1.ToString();
+            envars.Variables[Constants.EnvironmentVariables.CurlHttpProxy] = value2.ToString();
+            RunTest(value1);
+
+            // Test case 3: all_proxy > ALL_PROXY
+            envars.Variables.Clear();
+            envars.Variables[Constants.EnvironmentVariables.CurlAllProxy] = value1.ToString();
+            envars.Variables[Constants.EnvironmentVariables.CurlAllProxyUpper] = value2.ToString();
+            RunTest(value1);
         }
 
         [Fact]
diff --git a/src/shared/Core/Constants.cs b/src/shared/Core/Constants.cs
@@ -53,10 +53,24 @@ public static class EnvironmentVariables
             public const string GcmAuthority          = "GCM_AUTHORITY";
             public const string GitTerminalPrompts    = "GIT_TERMINAL_PROMPT";
             public const string GcmAllowWia           = "GCM_ALLOW_WINDOWSAUTH";
-            public const string CurlNoProxy           = "NO_PROXY";
-            public const string CurlAllProxy          = "ALL_PROXY";
-            public const string CurlHttpProxy         = "HTTP_PROXY";
-            public const string CurlHttpsProxy        = "HTTPS_PROXY";
+
+            /*
+             * Unlike other environment variables, these proxy variables are normally lowercase only.
+             * However, libcurl also implemented checks for the uppercase variants! The lowercase
+             * variants should take precedence over the uppercase ones since the former are quasi-standard.
+             *
+             * One exception to this is that libcurl does not even look for the uppercase variant of
+             * http_proxy (for some security reasons).
+             */
+            public const string CurlNoProxy           = "no_proxy";
+            public const string CurlNoProxyUpper      = "NO_PROXY";
+            public const string CurlHttpsProxy        = "https_proxy";
+            public const string CurlHttpsProxyUpper   = "HTTPS_PROXY";
+            public const string CurlHttpProxy         = "http_proxy";
+            // Note there is no uppercase variant of the http_proxy since libcurl doesn't use it
+            public const string CurlAllProxy          = "all_proxy";
+            public const string CurlAllProxyUpper     = "ALL_PROXY";
+
             public const string GcmHttpProxy          = "GCM_HTTP_PROXY";
             public const string GitSslNoVerify        = "GIT_SSL_NO_VERIFY";
             public const string GitSslCaInfo          = "GIT_SSL_CAINFO";
diff --git a/src/shared/Core/Settings.cs b/src/shared/Core/Settings.cs
@@ -580,7 +580,11 @@ ProxyConfiguration CreateConfiguration(Uri uri, bool isLegacy = false)
                 uri.TryGetUserInfo(out string userName, out string password);
 
                 // Get the proxy bypass host names
-                _environment.Variables.TryGetValue(KnownEnvars.CurlNoProxy, out string noProxyStr);
+                // Check both lowercase "no_proxy" and uppercase "NO_PROXY" variants (preferring the former)
+                if (!_environment.Variables.TryGetValue(KnownEnvars.CurlNoProxy, out string noProxyStr))
+                {
+                    _environment.Variables.TryGetValue(KnownEnvars.CurlNoProxyUpper, out noProxyStr);
+                }
 
                 return new ProxyConfiguration(address, userName, password, noProxyStr, isLegacy);
             }
@@ -598,8 +602,10 @@ ProxyConfiguration CreateConfiguration(Uri uri, bool isLegacy = false)
              *        http.proxy
              *
              *   3. cURL environment variables
+             *        https_proxy
              *        HTTPS_PROXY
-             *        HTTP_PROXY
+             *        http_proxy (note that uppercase HTTP_PROXY is not supported by libcurl)
+             *        all_proxy
              *        ALL_PROXY
              *
              *   4. GCM proxy environment variable (deprecated)
@@ -610,7 +616,7 @@ ProxyConfiguration CreateConfiguration(Uri uri, bool isLegacy = false)
              *
              * For HTTP URIs we only check the HTTP variants.
              *
-             * We also support the cURL "NO_PROXY" environment variable in conjunction with any
+             * We also support the cURL "no_proxy" / "NO_PROXY" environment variables in conjunction with any
              * of the above supported proxy address configurations. This comma separated list of
              * host names (or host name wildcards) should be respected and the proxy should NOT
              * be used for these addresses.
@@ -634,10 +640,14 @@ ProxyConfiguration CreateConfiguration(Uri uri, bool isLegacy = false)
                 return CreateConfiguration(proxyUri);
             }
 
-            // 3. cURL environment variables
+            // 3. cURL environment variables (both lower- and uppercase variants)
+            // Prefer the lowercase variants as these are quasi-standard.
             if (isHttps && TryGetUriSetting(KnownEnvars.CurlHttpsProxy, null, null, out proxyUri) ||
+                isHttps && TryGetUriSetting(KnownEnvars.CurlHttpsProxyUpper, null, null, out proxyUri) ||
                 TryGetUriSetting(KnownEnvars.CurlHttpProxy, null, null, out proxyUri) ||
-                TryGetUriSetting(KnownEnvars.CurlAllProxy, null, null, out proxyUri))
+                // Note that the uppercase HTTP_PROXY is not recognized by libcurl
+                TryGetUriSetting(KnownEnvars.CurlAllProxy, null, null, out proxyUri) ||
+                TryGetUriSetting(KnownEnvars.CurlAllProxyUpper, null, null, out proxyUri))
             {
                 return CreateConfiguration(proxyUri);
             }
