From 0c9684c2c4057908cf697a366a3b1527326e507d Mon Sep 17 00:00:00 2001 From: David Shulman Date: Tue, 16 Apr 2019 17:15:30 -0700 Subject: [PATCH] Fix ClientWebSocket close handshake when using proxy (#36928) When the client websocket was going through a proxy, it would sometimes transition to the 'Aborted' state and not the 'Closed' state after a successful closing handshake. I first opened this bug a year ago but finally was able to get back to it and root cause the problem. The problem only occured with the managed websocket. The .NET Framework did not have this problem. And some proxies didn't cause a problem with managed websocket (such as Fiddler). The root cause is a misinterpretation of RFC 6455, Section 7.1.1. This describes the behavior of how the websocket's TCP connection should be closed. The most graceful way is to wait for the server to initiate the close of the socket. In cases where it is taking too long to wait for the server to start the TCP close, then the client can start the TCP close of the socket. But no matter how the socket is finally closed, the websocket state should transition to 'Closed' as soon as a valid closing handshake was performed (i.e. close frames both sent and received). And this occurs before any logic for closing the TCP connection. The code in managed websocket has a timer to wait 1 second for the server to start a close. If the timer finishes, then the managed websocket calls an Abort() method to close the socket. This ends up transitioning the websocket to an 'Aborted' state which is incorrect. The state should only be moved to the 'Aborted' state if a closing handshake was not completed. I added a new test to support this change and moved the LoopbackProxyServer code to Common. Fixes #28027 --- .../System/Net/WebSockets/ManagedWebSocket.cs | 12 +++++-- .../System/Net/Http}/LoopbackProxyServer.cs | 6 ++-- .../System.Net.Http.Functional.Tests.csproj | 4 ++- .../tests/ConnectTest.cs | 36 ++++++++++++++----- .../System.Net.WebSockets.Client.Tests.csproj | 3 ++ 5 files changed, 47 insertions(+), 14 deletions(-) rename src/{System.Net.Http/tests/FunctionalTests => Common/tests/System/Net/Http}/LoopbackProxyServer.cs (99%) diff --git a/src/Common/src/System/Net/WebSockets/ManagedWebSocket.cs b/src/Common/src/System/Net/WebSockets/ManagedWebSocket.cs index 86c5ba9e50ae..93a24c221c46 100644 --- a/src/Common/src/System/Net/WebSockets/ManagedWebSocket.cs +++ b/src/Common/src/System/Net/WebSockets/ManagedWebSocket.cs @@ -782,7 +782,11 @@ private async Task HandleReceivedCloseAsync(MessageHeader header, CancellationTo lock (StateUpdateLock) { _receivedCloseFrame = true; - if (_state < WebSocketState.CloseReceived) + if (_sentCloseFrame && _state < WebSocketState.Closed) + { + _state = WebSocketState.Closed; + } + else if (_state < WebSocketState.CloseReceived) { _state = WebSocketState.CloseReceived; } @@ -1153,7 +1157,11 @@ private async Task SendCloseFrameAsync(WebSocketCloseStatus closeStatus, string lock (StateUpdateLock) { _sentCloseFrame = true; - if (_state <= WebSocketState.CloseReceived) + if (_receivedCloseFrame && _state < WebSocketState.Closed) + { + _state = WebSocketState.Closed; + } + else if (_state < WebSocketState.CloseSent) { _state = WebSocketState.CloseSent; } diff --git a/src/System.Net.Http/tests/FunctionalTests/LoopbackProxyServer.cs b/src/Common/tests/System/Net/Http/LoopbackProxyServer.cs similarity index 99% rename from src/System.Net.Http/tests/FunctionalTests/LoopbackProxyServer.cs rename to src/Common/tests/System/Net/Http/LoopbackProxyServer.cs index 979f3be1f8a8..e278908140fa 100644 --- a/src/System.Net.Http/tests/FunctionalTests/LoopbackProxyServer.cs +++ b/src/Common/tests/System/Net/Http/LoopbackProxyServer.cs @@ -12,7 +12,7 @@ using System.Threading; using System.Threading.Tasks; -namespace System.Net.Http.Functional.Tests +namespace System.Net.Test.Common { /// /// Provides a test-only HTTP proxy. Handles multiple connections/requests and CONNECT tunneling for HTTPS @@ -210,7 +210,7 @@ private async Task ProcessConnectMethod(NetworkStream clientStream, string remot { byte[] buffer = new byte[8000]; int bytesRead; - while ((bytesRead = await clientStream.ReadAsync(buffer)) > 0) + while ((bytesRead = await clientStream.ReadAsync(buffer, 0, buffer.Length)) > 0) { await serverStream.WriteAsync(buffer, 0, bytesRead); } @@ -228,7 +228,7 @@ private async Task ProcessConnectMethod(NetworkStream clientStream, string remot { byte[] buffer = new byte[8000]; int bytesRead; - while ((bytesRead = await serverStream.ReadAsync(buffer)) > 0) + while ((bytesRead = await serverStream.ReadAsync(buffer, 0, buffer.Length)) > 0) { await clientStream.WriteAsync(buffer, 0, bytesRead); } diff --git a/src/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj b/src/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj index 6003b5e8392f..ba1aeac10b93 100644 --- a/src/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj +++ b/src/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj @@ -65,6 +65,9 @@ Common\System\Net\VerboseTestLogging.cs + + Common\System\Net\Http\LoopbackProxyServer.cs + Common\System\Net\Http\LoopbackServer.cs @@ -120,7 +123,6 @@ - diff --git a/src/System.Net.WebSockets.Client/tests/ConnectTest.cs b/src/System.Net.WebSockets.Client/tests/ConnectTest.cs index 2d83ecff250c..37d7e4655e51 100644 --- a/src/System.Net.WebSockets.Client/tests/ConnectTest.cs +++ b/src/System.Net.WebSockets.Client/tests/ConnectTest.cs @@ -16,7 +16,7 @@ public class ConnectTest : ClientWebSocketTestBase { public ConnectTest(ITestOutputHelper output) : base(output) { } - [OuterLoop] // TODO: Issue #11345 + [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(UnavailableWebSocketServers))] public async Task ConnectAsync_NotWebSocketServer_ThrowsWebSocketExceptionWithMessage(Uri server, string exceptionMessage, WebSocketError errorCode) { @@ -40,21 +40,21 @@ public async Task ConnectAsync_NotWebSocketServer_ThrowsWebSocketExceptionWithMe } } - [OuterLoop] // TODO: Issue #11345 + [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] public async Task EchoBinaryMessage_Success(Uri server) { await WebSocketHelper.TestEcho(server, WebSocketMessageType.Binary, TimeOutMilliseconds, _output); } - [OuterLoop] // TODO: Issue #11345 + [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] public async Task EchoTextMessage_Success(Uri server) { await WebSocketHelper.TestEcho(server, WebSocketMessageType.Text, TimeOutMilliseconds, _output); } - [OuterLoop] // TODO: Issue #11345 + [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoHeadersServers))] public async Task ConnectAsync_AddCustomHeaders_Success(Uri server) { @@ -92,7 +92,7 @@ public async Task ConnectAsync_AddCustomHeaders_Success(Uri server) } [ActiveIssue(18784, TargetFrameworkMonikers.NetFramework)] - [OuterLoop] + [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported))] public async Task ConnectAsync_AddHostHeader_Success() { @@ -136,7 +136,7 @@ public async Task ConnectAsync_AddHostHeader_Success() } } - [OuterLoop] // TODO: Issue #11345 + [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoHeadersServers))] public async Task ConnectAsync_CookieHeaders_Success(Uri server) { @@ -185,7 +185,7 @@ public async Task ConnectAsync_CookieHeaders_Success(Uri server) } } - [OuterLoop] // TODO: Issue #11345 + [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] public async Task ConnectAsync_PassNoSubProtocol_ServerRequires_ThrowsWebSocketException(Uri server) { @@ -210,7 +210,7 @@ public async Task ConnectAsync_PassNoSubProtocol_ServerRequires_ThrowsWebSocketE } } - [OuterLoop] // TODO: Issue #11345 + [OuterLoop("Uses external servers")] [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] public async Task ConnectAsync_PassMultipleSubProtocols_ServerRequires_ConnectionUsesAgreedSubProtocol(Uri server) { @@ -248,5 +248,25 @@ await LoopbackServer.CreateClientAndServerAsync(async uri => Assert.True(await LoopbackHelper.WebSocketHandshakeAsync(connection)); }), new LoopbackServer.Options { WebSocketEndpoint = true }); } + + [OuterLoop("Uses external servers")] + [ConditionalTheory(nameof(WebSocketsSupported)), MemberData(nameof(EchoServers))] + public async Task ConnectAndCloseAsync_UseProxyServer_ExpectedClosedState(Uri server) + { + using (var cws = new ClientWebSocket()) + using (var cts = new CancellationTokenSource(TimeOutMilliseconds)) + using (LoopbackProxyServer proxyServer = LoopbackProxyServer.Create()) + { + cws.Options.Proxy = new WebProxy(proxyServer.Uri); + await cws.ConnectAsync(server, cts.Token); + + string expectedCloseStatusDescription = "Client close status"; + await cws.CloseAsync(WebSocketCloseStatus.NormalClosure, expectedCloseStatusDescription, cts.Token); + + Assert.Equal(WebSocketState.Closed, cws.State); + Assert.Equal(WebSocketCloseStatus.NormalClosure, cws.CloseStatus); + Assert.Equal(expectedCloseStatusDescription, cws.CloseStatusDescription); + } + } } } diff --git a/src/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj b/src/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj index 8849fdb1e865..c47fa527a389 100644 --- a/src/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj +++ b/src/System.Net.WebSockets.Client/tests/System.Net.WebSockets.Client.Tests.csproj @@ -28,6 +28,9 @@ Common\System\Net\Configuration.WebSockets.cs + + Common\System\Net\Http\LoopbackProxyServer.cs + Common\System\Net\Http\LoopbackServer.cs