From 056da5114aeb201e805e8aa03eb9f6e32bdb9bf1 Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Wed, 18 Jul 2018 14:32:58 -0700 Subject: [PATCH 1/4] Add partial handshake test (#2630) --- .../HubConnectionHandlerTests.cs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs index 3e688fc1f3..308330b0fa 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs @@ -374,6 +374,32 @@ namespace Microsoft.AspNetCore.SignalR.Tests } } + [Fact] + public async Task ConnectionClosesOnServerWithPartialHandshakeMessageAndCompletedPipe() + { + var connectionHandler = HubConnectionHandlerTestUtils.GetHubConnectionHandler(typeof(HubT)); + + using (var client = new TestClient()) + { + // partial handshake + var payload = Encoding.UTF8.GetBytes("{\"protocol\": \"json\",\"ver"); + await client.Connection.Application.Output.WriteAsync(payload).OrTimeout(); + + var connectionHandlerTask = await client.ConnectAsync(connectionHandler, sendHandshakeRequestMessage: false, expectedHandshakeResponseMessage: false); + // Complete the pipe to 'close' the connection + client.Connection.Application.Output.Complete(); + + // This will never complete as the pipe was completed and nothing can be written to it + var handshakeReadTask = client.ReadAsync(true); + + // Check that the connection was closed on the server + await connectionHandlerTask.OrTimeout(); + Assert.False(handshakeReadTask.IsCompleted); + + client.Dispose(); + } + } + [Fact] public async Task LifetimeManagerOnDisconnectedAsyncCalledIfLifetimeManagerOnConnectedAsyncThrows() { From 433eeb6943df7fa2d6c21e3a933224a045147c2d Mon Sep 17 00:00:00 2001 From: BrennanConroy Date: Thu, 19 Jul 2018 14:50:14 -0700 Subject: [PATCH 2/4] Abort connection on protocol error (#2654) --- .../HubConnectionContext.cs | 23 ++++++++++++++++- .../HubConnectionHandlerTestUtils/Hubs.cs | 15 +++++++++++ .../HubConnectionHandlerTests.cs | 25 ++++++++++++++++++- 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs index 768edbf132..a67fa133df 100644 --- a/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs +++ b/src/Microsoft.AspNetCore.SignalR.Core/HubConnectionContext.cs @@ -36,6 +36,7 @@ namespace Microsoft.AspNetCore.SignalR private bool _receivedMessageThisInterval = false; private ReadOnlyMemory _cachedPingMessage; private bool _clientTimeoutActive; + private bool _connectedAborted; /// /// Initializes a new instance of the class. @@ -105,6 +106,11 @@ namespace Microsoft.AspNetCore.SignalR public virtual ValueTask WriteAsync(HubMessage message, CancellationToken cancellationToken = default) { + if (_connectedAborted) + { + return default; + } + // Try to grab the lock synchronously, if we fail, go to the slower path if (!_writeLock.Wait(0)) { @@ -135,6 +141,11 @@ namespace Microsoft.AspNetCore.SignalR /// public virtual ValueTask WriteAsync(SerializedHubMessage message, CancellationToken cancellationToken = default) { + if (_connectedAborted) + { + return default; + } + // Try to grab the lock synchronously, if we fail, go to the slower path if (!_writeLock.Wait(0)) { @@ -170,6 +181,8 @@ namespace Microsoft.AspNetCore.SignalR { Log.FailedWritingMessage(_logger, ex); + Abort(); + return new ValueTask(new FlushResult(isCanceled: false, isCompleted: true)); } } @@ -187,6 +200,8 @@ namespace Microsoft.AspNetCore.SignalR { Log.FailedWritingMessage(_logger, ex); + Abort(); + return new ValueTask(new FlushResult(isCanceled: false, isCompleted: true)); } } @@ -200,6 +215,8 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); + + Abort(); } finally { @@ -220,6 +237,7 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); + Abort(); } finally { @@ -239,6 +257,7 @@ namespace Microsoft.AspNetCore.SignalR catch (Exception ex) { Log.FailedWritingMessage(_logger, ex); + Abort(); } finally { @@ -312,6 +331,8 @@ namespace Microsoft.AspNetCore.SignalR return; } + _connectedAborted = true; + Input.CancelPendingRead(); // We fire and forget since this can trigger user code to run @@ -531,7 +552,7 @@ namespace Microsoft.AspNetCore.SignalR LoggerMessage.Define(LogLevel.Error, new EventId(5, "HandshakeFailed"), "Failed connection handshake."); private static readonly Action _failedWritingMessage = - LoggerMessage.Define(LogLevel.Debug, new EventId(6, "FailedWritingMessage"), "Failed writing message."); + LoggerMessage.Define(LogLevel.Error, new EventId(6, "FailedWritingMessage"), "Failed writing message. Aborting connection."); private static readonly Action _protocolVersionFailed = LoggerMessage.Define(LogLevel.Warning, new EventId(7, "ProtocolVersionFailed"), "Server does not support version {Version} of the {Protocol} protocol."); diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs index f2dad3db45..7eb0aec498 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTestUtils/Hubs.cs @@ -159,6 +159,21 @@ namespace Microsoft.AspNetCore.SignalR.Tests { return Clients.Caller.SendAsync("Send", message); } + + public Task ProtocolError() + { + return Clients.Caller.SendAsync("Send", new string('x', 3000), new SelfRef()); + } + + private class SelfRef + { + public SelfRef() + { + Self = this; + } + + public SelfRef Self; + } } public abstract class TestHub : Hub diff --git a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs index 308330b0fa..02a5146e02 100644 --- a/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs +++ b/test/Microsoft.AspNetCore.SignalR.Tests/HubConnectionHandlerTests.cs @@ -16,6 +16,7 @@ using Microsoft.AspNetCore.Http.Connections.Features; using Microsoft.AspNetCore.SignalR.Internal; using Microsoft.AspNetCore.SignalR.Protocol; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Options; using Moq; using Newtonsoft.Json; @@ -102,9 +103,11 @@ namespace Microsoft.AspNetCore.SignalR.Tests { var connectionHandlerTask = await client.ConnectAsync(connectionHandler); - await client.InvokeAsync(nameof(AbortHub.Kill)); + await client.SendInvocationAsync(nameof(AbortHub.Kill)).OrTimeout(); await connectionHandlerTask.OrTimeout(); + + Assert.Null(client.TryRead()); } } @@ -2358,6 +2361,26 @@ namespace Microsoft.AspNetCore.SignalR.Tests } } + [Fact] + public async Task ConnectionAbortedIfSendFailsWithProtocolError() + { + var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(services => + { + services.AddSignalR(options => options.EnableDetailedErrors = true); + }); + var connectionHandler = serviceProvider.GetService>(); + + using (var client = new TestClient()) + { + var connectionHandlerTask = await client.ConnectAsync(connectionHandler).OrTimeout(); + + await client.SendInvocationAsync(nameof(MethodHub.ProtocolError)).OrTimeout(); + + await client.Connected.OrTimeout(); + await connectionHandlerTask.OrTimeout(); + } + } + private class CustomHubActivator : IHubActivator where THub : Hub { public int ReleaseCount; From c954cd5bf89a95b41adc38d837fae1567171293c Mon Sep 17 00:00:00 2001 From: "ASP.NET CI" Date: Sun, 22 Jul 2018 12:27:29 -0700 Subject: [PATCH 3/4] Update dependencies.props [auto-updated: dependencies] --- build/dependencies.props | 96 ++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index 49ab41702b..f6c40af002 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -5,57 +5,57 @@ 0.10.13 3.1.0 - 2.2.0-preview1-34694 + 2.2.0-preview1-34755 2.2.0-preview1-17099 1.7.3.4 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 4.5.0 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 - 2.2.0-preview1-34694 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 + 2.2.0-preview1-34755 2.2.0-preview1-26618-02 15.6.1 4.7.49 From fd5d27c7cb3c10e76701a68e54ddd46c00375b97 Mon Sep 17 00:00:00 2001 From: Eilon Lipton Date: Tue, 24 Jul 2018 10:57:40 -0700 Subject: [PATCH 4/4] Update CONTRIBUTING.md --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 64ff041d5c..eac4268e4c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,4 +1,4 @@ Contributing ====== -Information on contributing to this repo is in the [Contributing Guide](https://github.com/aspnet/Home/blob/dev/CONTRIBUTING.md) in the Home repo. +Information on contributing to this repo is in the [Contributing Guide](https://github.com/aspnet/Home/blob/master/CONTRIBUTING.md) in the Home repo.