Merge pull request #503 from mjcheetham/curl-envars

mjcheetham · web-flow · commit 90a8d6df8db3 · 2021-11-02T10:43:40.000Z
Match libcurl lowercase proxy environment variable behaviour
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);
             }
