Fix SocketsHttpHandler system proxy usage logic (dotnet/corefx#37153)

SocketsHttpHandler was not using the proxy bypass list specified in IE settings in cases where
there is a combination of both 'AutoDetect' and 'Manual' settings in the IE settings dialog.

SocketsHttpHandler uses WinHttpHandler's WinInetProxyHelper class as part of the HttpSystemProxy
class. But it was consuming it in an incorrect way. WinInetProxyHelper was originally written to
be used only with the rest of the WinHTTP stack for WinHttpHandler. When WinHTTP GetProxyForUrl() was returning a ProxyBypass string, HttpSystemProxy was ignoring it. It was
assuming that the string for Proxy was correct. But in cases where ProxyBypass is returned, the
Proxy string is only used if the destination uri doesn't match any of the strings in the ProxyBypass
list. That logic would normally be handled automatically by WinHttpHandler. But HttpSystemProxy
was simply discarding the ProxyBypass string returned by WinHTTP GetProxyForUrl().

In order to address this fix, I added new tests for HttpSystemProxy. I utilized the existing mock
layers for registry and WinHTTP interop that are contained in the WinHttpHandler Unit Tests. It might
seem weird to have tests for the internal HttpSystemProxy class located in the WinHttpHandler folder.
But, we already pull in the source code for WinInetProxyHelper from WinHttpHandler into the
SocketsHttpHandler compilation. Using these Unit Tests with the mocking layers allows us to test a
broad range of scenarios (registry settings and network failures) that are normally difficult to test.
I have a plan, though, to refactor the system proxy code and tests as part of implementing dotnet/corefx#36553.
That will result in the system proxy code and tests ending up in a more logical place.

Fixes dotnet/corefx#33866


Commit migrated from dotnet/corefx@6da9a3b

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinInetProxyHelper.cs

@@ -57,29 +57,19 @@ public WinInetProxyHelper()
             }
         }
 
-        public string AutoConfigUrl { get; set; }
+        public string AutoConfigUrl { get; private set; }
 
-        public bool AutoDetect { get; set; }
+        public bool AutoDetect { get; private set; }
 
-        public bool AutoSettingsUsed
-        {
-            get
-            {
-                return AutoDetect || !string.IsNullOrEmpty(AutoConfigUrl);
-            }
-        }
+        public bool AutoSettingsUsed => AutoDetect || !string.IsNullOrEmpty(AutoConfigUrl);
 
-        public bool ManualSettingsOnly
-        {
-            get
-            {
-                return !AutoDetect && string.IsNullOrEmpty(AutoConfigUrl) && !string.IsNullOrEmpty(Proxy);
-            }
-        }
+        public bool ManualSettingsUsed => !string.IsNullOrEmpty(Proxy);
+
+        public bool ManualSettingsOnly => !AutoSettingsUsed && ManualSettingsUsed;
 
-        public string Proxy { get; set; }
+        public string Proxy { get; private set; }
 
-        public string ProxyBypass { get; set; }
+        public string ProxyBypass { get; private set; }
 
         public bool RecentAutoDetectionFailure =>
             _autoDetectionFailed &&

src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/Configurations.props

@@ -1,7 +1,7 @@
-<Project DefaultTargets="Build">
+<Project DefaultTargets="Build">
   <PropertyGroup>
     <BuildConfigurations>
-      netstandard-Windows_NT;
+      netcoreapp-Windows_NT;
     </BuildConfigurations>
   </PropertyGroup>
-</Project>
+</Project>

src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/HttpSystemProxyTest.cs

@@ -0,0 +1,98 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Collections.Generic;
+
+using Xunit;
+using Xunit.Abstractions;
+
+namespace System.Net.Http.WinHttpHandlerUnitTests
+{
+    public class HttpSystemProxyTest
+    {
+        private const string ManualSettingsProxyHost = "myproxy.local";
+        private const string ManualSettingsProxyBypassList = "localhost;*.local";
+
+        private readonly ITestOutputHelper _output;
+
+        public HttpSystemProxyTest(ITestOutputHelper output)
+        {
+            _output = output;
+            TestControl.ResetAll();
+        }
+
+        public static IEnumerable<object[]> ManualSettingsMemberData()
+        {
+            yield return new object[] { new Uri("http://example.org"), false };
+            yield return new object[] { new Uri("http://example.local"), true };
+            yield return new object[] { new Uri("http://localhost"), true };
+        }
+
+        [Fact]
+        public void TryCreate_WinInetProxySettingsAllOff_ReturnsFalse()
+        {
+            Assert.False(HttpSystemProxy.TryCreate(out IWebProxy webProxy));
+        }
+
+        [Theory]
+        [MemberData(nameof(ManualSettingsMemberData))]
+        public void GetProxy_BothAutoDetectAndManualSettingsButFailedAutoDetect_ManualSettingsUsed(
+            Uri destination, bool bypassProxy)
+        {
+            FakeRegistry.WinInetProxySettings.AutoDetect = true;
+            FakeRegistry.WinInetProxySettings.Proxy = ManualSettingsProxyHost;
+            FakeRegistry.WinInetProxySettings.ProxyBypass = ManualSettingsProxyBypassList;
+            TestControl.PACFileNotDetectedOnNetwork = true;
+
+            Assert.True(HttpSystemProxy.TryCreate(out IWebProxy webProxy));
+
+            // The first GetProxy() call will try using WinInetProxyHelper (and thus WinHTTP) since AutoDetect is on.
+            Uri proxyUri1 = webProxy.GetProxy(destination);
+
+            // The second GetProxy call will skip using WinHTTP since AutoDetect is on but
+            // there was a recent AutoDetect failure. This tests the codepath in HttpSystemProxy
+            // which queries WinInetProxyHelper.RecentAutoDetectionFailure.
+            Uri proxyUri2 = webProxy.GetProxy(destination);
+
+            if (bypassProxy)
+            {
+                Assert.Null(proxyUri1);
+                Assert.Null(proxyUri2);
+            }
+            else
+            {
+                Assert.Equal(ManualSettingsProxyHost, proxyUri1.Host);
+                Assert.Equal(ManualSettingsProxyHost, proxyUri2.Host);
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(ManualSettingsMemberData))]
+        public void GetProxy_ManualSettingsOnly_ManualSettingsUsed(
+            Uri destination, bool bypassProxy)
+        {
+            FakeRegistry.WinInetProxySettings.Proxy = ManualSettingsProxyHost;
+            FakeRegistry.WinInetProxySettings.ProxyBypass = ManualSettingsProxyBypassList;
+
+            Assert.True(HttpSystemProxy.TryCreate(out IWebProxy webProxy));
+            Uri proxyUri = webProxy.GetProxy(destination);
+            if (bypassProxy)
+            {
+                Assert.Null(proxyUri);
+            }
+            else
+            {
+                Assert.Equal(ManualSettingsProxyHost, proxyUri.Host);
+            }
+        }
+
+        [Fact]
+        public void IsBypassed_ReturnsFalse()
+        {
+            FakeRegistry.WinInetProxySettings.AutoDetect = true;
+            Assert.True(HttpSystemProxy.TryCreate(out IWebProxy webProxy));
+            Assert.False(webProxy.IsBypassed(new Uri("http://www.microsoft.com/")));
+        }
+    }
+}

src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/System.Net.Http.WinHttpHandler.Unit.Tests.csproj

@@ -127,6 +127,9 @@
     <Compile Include="..\..\src\System\Net\Http\WinInetProxyHelper.cs">
       <Link>ProductionCode\WinInetProxyHelper.cs</Link>
     </Compile>
+    <Compile Include="..\..\..\System.Net.Http\src\System\Net\Http\SocketsHttpHandler\HttpSystemProxy.cs">
+      <Link>ProductionCode\HttpSystemProxy.cs</Link>
+    </Compile>
     <Compile Include="APICallHistory.cs" />
     <Compile Include="ClientCertificateHelper.cs" />
     <Compile Include="ClientCertificateScenarioTest.cs" />
@@ -135,6 +138,7 @@
     <Compile Include="FakeRegistry.cs" />
     <Compile Include="FakeSafeWinHttpHandle.cs" />
     <Compile Include="FakeX509Certificates.cs" />
+    <Compile Include="HttpSystemProxyTest.cs" />
     <Compile Include="SafeWinHttpHandleTest.cs" />
     <Compile Include="SendRequestHelper.cs" />
     <Compile Include="TestServer.cs" />

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpSystemProxy.cs

@@ -64,7 +64,7 @@ private HttpSystemProxy(WinInetProxyHelper proxyHelper, SafeWinHttpHandle sessio
             _proxyHelper = proxyHelper;
             _sessionHandle = sessionHandle;
 
-            if (proxyHelper.ManualSettingsOnly)
+            if (proxyHelper.ManualSettingsUsed)
             {
                 if (NetEventSource.IsEnabled) NetEventSource.Info(proxyHelper, $"ManualSettingsUsed, {proxyHelper.Proxy}");
                 ParseProxyConfig(proxyHelper.Proxy, out _insecureProxyUri, out _secureProxyUri);
@@ -275,7 +275,48 @@ private static int GetProxySubstringLength(string proxyString, int idx)
         /// </summary>
         public Uri GetProxy(Uri uri)
         {
-            if (_proxyHelper.ManualSettingsOnly)
+            // We need WinHTTP to detect and/or process a PAC (JavaScript) file. This maps to
+            // "Automatically detect settings" and/or "Use automatic configuration script" from IE
+            // settings. But, calling into WinHTTP can be slow especially when it has to call into
+            // the out-of-process service to discover, load, and run the PAC file. So, we skip
+            // calling into WinHTTP if there was a recent failure to detect a PAC file on the network.
+            // This is a common error. The default IE settings on a Windows machine consist of the
+            // single checkbox for "Automatically detect settings" turned on and most networks
+            // won't actually discover a PAC file on the network since WPAD protocol isn't configured.
+            if (_proxyHelper.AutoSettingsUsed && !_proxyHelper.RecentAutoDetectionFailure)
+            {
+                var proxyInfo = new Interop.WinHttp.WINHTTP_PROXY_INFO();
+                try
+                {
+                    if (_proxyHelper.GetProxyForUrl(_sessionHandle, uri, out proxyInfo))
+                    {
+                        // If WinHTTP just specified a Proxy with no ProxyBypass list, then
+                        // we can return the Proxy uri directly.
+                        if (proxyInfo.ProxyBypass == IntPtr.Zero)
+                        {
+                            return GetUriFromString(Marshal.PtrToStringUni(proxyInfo.Proxy));
+                        }
+
+                        // A bypass list was also specified. This means that WinHTTP has fallen back to
+                        // using the manual IE settings specified and there is a ProxyBypass list also.
+                        // Since we're not really using the full WinHTTP stack, we need to use HttpSystemProxy
+                        // to do the computation of the final proxy uri merging the information from the Proxy
+                        // and ProxyBypass strings.
+                    }
+                    else
+                    {
+                        return null;
+                    }
+                }
+                finally
+                {
+                    Marshal.FreeHGlobal(proxyInfo.Proxy);
+                    Marshal.FreeHGlobal(proxyInfo.ProxyBypass);
+                }
+            }
+
+            // Fallback to manual settings if present.
+            if (_proxyHelper.ManualSettingsUsed)
             {
                 if (_bypassLocal)
                 {
@@ -332,25 +373,6 @@ public Uri GetProxy(Uri uri)
                 return (uri.Scheme == UriScheme.Https || uri.Scheme == UriScheme.Wss) ? _secureProxyUri : _insecureProxyUri;
             }
 
-            // For anything else ask WinHTTP. To improve performance, we don't call into
-            // WinHTTP if there was a recent failure to detect a PAC file on the network.
-            if (!_proxyHelper.RecentAutoDetectionFailure)
-            {
-                var proxyInfo = new Interop.WinHttp.WINHTTP_PROXY_INFO();
-                try
-                {
-                    if (_proxyHelper.GetProxyForUrl(_sessionHandle, uri, out proxyInfo))
-                    {
-                        return GetUriFromString(Marshal.PtrToStringUni(proxyInfo.Proxy));
-                    }
-                }
-                finally
-                {
-                    Marshal.FreeHGlobal(proxyInfo.Proxy);
-                    Marshal.FreeHGlobal(proxyInfo.ProxyBypass);
-                }
-            }
-
             return null;
         }
 
@@ -359,21 +381,13 @@ public Uri GetProxy(Uri uri)
         /// </summary>
         public bool IsBypassed(Uri uri)
         {
-            if (_proxyHelper.ManualSettingsOnly)
-            {
-                if (_bypassLocal)
-                {
-                    // TODO #23150: implement bypass match.
-                }
-                return false;
-            }
-            else if (_proxyHelper.AutoSettingsUsed)
-            {
-                // Always return false for now to avoid query to WinHtttp.
-                // If URI should be bypassed GetProxy() will return null;
-                return false;
-            }
-            return true;
+            // This HttpSystemProxy class is only consumed by SocketsHttpHandler and is not exposed outside of
+            // SocketsHttpHandler. The current pattern for consumption of IWebProxy is to call IsBypassed first.
+            // If it returns false, then the caller will call GetProxy. For this proxy implementation, computing
+            // the return value for IsBypassed is as costly as calling GetProxy. We want to avoid doing extra
+            // work. So, this proxy implementation for the IsBypassed method can always return false. Then the
+            // GetProxy method will return non-null for a proxy, or null if no proxy should be used.
+            return false;
         }
 
         public ICredentials Credentials