Fix SocketsHttpHandler system proxy usage logic

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 #36553.
That will result in the system proxy code and tests ending up in a more logical place.

As part of this fix, I also optimized how SocketsHttpHandler is calling IWebProxy. I explained this
in the comments of HttpSystemProxy.IsBypassed(). In summary, IWebProxy.IsBypassed() shouldn't be used.
In most cases it is the same amount of work than calling IWebProxy.GetProxy(). And the latter can
be used to return a valid proxy uri, or null if a proxy shouldn't be used for that particular
destination uri.

Fixes #33866

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

@@ -57,29 +57,20 @@ 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 ManualSettingsOnly =>
+            !AutoDetect && string.IsNullOrEmpty(AutoConfigUrl) && !string.IsNullOrEmpty(Proxy);
+
+        public bool ManualSettingsUsed => !string.IsNullOrEmpty(Proxy);
 
-        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/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/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_ThrowsNotImplementedException()
+        {
+            FakeRegistry.WinInetProxySettings.AutoDetect = true;
+            Assert.True(HttpSystemProxy.TryCreate(out IWebProxy webProxy));
+            Assert.Throws<NotImplementedException>(() => webProxy.IsBypassed(new Uri("http://www.microsoft.com/")));
+        }
+    }
+}

src/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/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

@@ -279,10 +279,7 @@ public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool doRe
             Uri proxyUri = null;
             try
             {
-                if (!_proxy.IsBypassed(request.RequestUri))
-                {
-                    proxyUri = _proxy.GetProxy(request.RequestUri);
-                }
+                proxyUri = _proxy.GetProxy(request.RequestUri);
             }
             catch (Exception ex)
             {

src/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,14 @@ 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 not exposed outside.
+            // It's more efficient for consumers of any IWebProxy interface to call IWebProxy.GetProxy()
+            // directly. GetProxy() returns either a non-null Uri (the proxy to use) or null to indicate no
+            // proxy to use, i.e. the equivalent of IsBypassed==true. Callers needing to get a proxy Uri
+            // can end up slowing performance by calling IsBypassed() first before calling GetProxy().
+            // Most times the effort of calculating the return value of IsBypassed() does the same amount
+            // of work as calling GetProxy().
+            throw new NotImplementedException("SocketsHttpHandler should not be calling this method");
         }
 
         public ICredentials Credentials