From cefe2f96ea588eae576c11b17c71971aec084b3f Mon Sep 17 00:00:00 2001 From: David Shulman Date: Sun, 17 Sep 2017 13:57:41 -0700 Subject: [PATCH] Wrap cert callback leaked exceptions in WinHttpHandler Similar to the fix done for CurlHandler (#21938), fix WinHttpHandler so that leaked exceptions from the user-provided certificate callback are properly wrapped. The ManagedHandler still needs to be fixed since it is not wrapping properly. Contributes to #21904 --- .../src/System/Net/Http/WinHttpException.cs | 12 ++++++++++++ .../System/Net/Http/WinHttpRequestCallback.cs | 19 +++++++++++++------ .../FunctionalTests/ServerCertificateTest.cs | 6 ++++-- ...ttpClientHandlerTest.ServerCertificates.cs | 12 ++++++++---- 4 files changed, 37 insertions(+), 12 deletions(-) diff --git a/src/Common/src/System/Net/Http/WinHttpException.cs b/src/Common/src/System/Net/Http/WinHttpException.cs index 1a4bcf8f5272..d77e3471351f 100644 --- a/src/Common/src/System/Net/Http/WinHttpException.cs +++ b/src/Common/src/System/Net/Http/WinHttpException.cs @@ -15,6 +15,11 @@ public WinHttpException(int error, string message) : base(error, message) this.HResult = ConvertErrorCodeToHR(error); } + public WinHttpException(int error, string message, Exception innerException) : base(message, innerException) + { + this.HResult = ConvertErrorCodeToHR(error); + } + public static int ConvertErrorCodeToHR(int error) { // This method allows common error detection code to be used by consumers @@ -52,6 +57,13 @@ public static WinHttpException CreateExceptionUsingError(int error) return e; } + public static WinHttpException CreateExceptionUsingError(int error, Exception innerException) + { + var e = new WinHttpException(error, GetErrorMessage(error), innerException); + ExceptionStackTrace.AddCurrentStack(e); + return e; + } + public static string GetErrorMessage(int error) { // Look up specific error message in WINHTTP.DLL since it is not listed in default system resources diff --git a/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs b/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs index 30c64f90c9f2..7bea4857cb7a 100644 --- a/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs +++ b/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs @@ -270,6 +270,7 @@ private static void OnRequestSendingRequest(WinHttpRequestState state) X509Chain chain = null; SslPolicyErrors sslPolicyErrors; + bool result = false; try { @@ -281,16 +282,16 @@ private static void OnRequestSendingRequest(WinHttpRequestState state) out chain, out sslPolicyErrors); - bool result = state.ServerCertificateValidationCallback( + result = state.ServerCertificateValidationCallback( state.RequestMessage, serverCertificate, chain, sslPolicyErrors); - if (!result) - { - throw WinHttpException.CreateExceptionUsingError( - (int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE); - } + } + catch (Exception ex) + { + throw WinHttpException.CreateExceptionUsingError( + (int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE, ex); } finally { @@ -301,6 +302,12 @@ private static void OnRequestSendingRequest(WinHttpRequestState state) serverCertificate.Dispose(); } + + if (!result) + { + throw WinHttpException.CreateExceptionUsingError( + (int)Interop.WinHttp.ERROR_WINHTTP_SECURE_FAILURE); + } } } diff --git a/src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/ServerCertificateTest.cs b/src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/ServerCertificateTest.cs index 7b7857172f91..0aad931e8997 100644 --- a/src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/ServerCertificateTest.cs +++ b/src/System.Net.Http.WinHttpHandler/tests/FunctionalTests/ServerCertificateTest.cs @@ -111,14 +111,16 @@ public async Task UseCallback_CallbackReturnsFailure_ThrowsInnerSecurityFailureE [OuterLoop] // TODO: Issue #11345 [Fact] - public async Task UseCallback_CallbackThrowsSpecificException_ThrowsInnerSpecificException() + public async Task UseCallback_CallbackThrowsSpecificException_SpecificExceptionPropagatesAsBaseException() { var handler = new WinHttpHandler(); handler.ServerCertificateValidationCallback = CustomServerCertificateValidationCallback; using (var client = new HttpClient(handler)) { _validationCallbackHistory.ThrowException = true; - await Assert.ThrowsAsync(() => client.GetAsync(System.Net.Test.Common.Configuration.Http.SecureRemoteEchoServer)); + HttpRequestException ex = await Assert.ThrowsAsync(() => + client.GetAsync(System.Net.Test.Common.Configuration.Http.SecureRemoteEchoServer)); + Assert.True(ex.GetBaseException() is CustomException); } } diff --git a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs index e4b5bb5909da..0389ac3d8c79 100644 --- a/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs +++ b/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.ServerCertificates.cs @@ -204,14 +204,18 @@ public async Task UseCallback_CallbackReturnsFailure_ThrowsException() } } - [ActiveIssue(21904, ~TargetFrameworkMonikers.Uap)] [OuterLoop] // TODO: Issue #11345 [ConditionalFact(nameof(BackendSupportsCustomCertificateHandling))] - public async Task UseCallback_CallbackThrowsException_ExceptionPropagatesAsInnerException() + public async Task UseCallback_CallbackThrowsException_ExceptionPropagatesAsBaseException() { + if (ManagedHandlerTestHelpers.IsEnabled) + { + return; // TODO #21904: ManagedHandler is not properly wrapping exception. + } + if (BackendDoesNotSupportCustomCertificateHandling) // can't use [Conditional*] right now as it's evaluated at the wrong time for the managed handler { - Console.WriteLine($"Skipping {nameof(UseCallback_CallbackThrowsException_ExceptionPropagatesAsInnerException)}()"); + Console.WriteLine($"Skipping {nameof(UseCallback_CallbackThrowsException_ExceptionPropagatesAsBaseException)}()"); return; } @@ -222,7 +226,7 @@ public async Task UseCallback_CallbackThrowsException_ExceptionPropagatesAsInner handler.ServerCertificateCustomValidationCallback = delegate { throw e; }; HttpRequestException ex = await Assert.ThrowsAsync(() => client.GetAsync(Configuration.Http.SecureRemoteEchoServer)); - Assert.Same(e, ex.InnerException); + Assert.Same(e, ex.GetBaseException()); } }