From f80dbd3918008dc9b9303f1189f9ade8e32d687a Mon Sep 17 00:00:00 2001 From: Nick Darvey Date: Mon, 24 Jun 2019 22:36:05 +1000 Subject: [PATCH 1/4] Fixes dispatching of derived type arguments to hub methods with synthetic parameters --- .../Core/src/Internal/DefaultHubDispatcher.cs | 2 +- .../Internal/DefaultHubDispatcherTests.cs | 143 ++++++++++++++++++ 2 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 src/SignalR/server/SignalR/test/Internal/DefaultHubDispatcherTests.cs diff --git a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs index 86fe4b32ac..b64804a9d6 100644 --- a/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs +++ b/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs @@ -261,7 +261,7 @@ namespace Microsoft.AspNetCore.SignalR.Internal for (var parameterPointer = 0; parameterPointer < arguments.Length; parameterPointer++) { if (hubMethodInvocationMessage.Arguments.Length > hubInvocationArgumentPointer && - hubMethodInvocationMessage.Arguments[hubInvocationArgumentPointer].GetType() == descriptor.OriginalParameterTypes[parameterPointer]) + descriptor.OriginalParameterTypes[parameterPointer].IsAssignableFrom(hubMethodInvocationMessage.Arguments[hubInvocationArgumentPointer].GetType())) { // The types match so it isn't a synthetic argument, just copy it into the arguments array arguments[parameterPointer] = hubMethodInvocationMessage.Arguments[hubInvocationArgumentPointer]; diff --git a/src/SignalR/server/SignalR/test/Internal/DefaultHubDispatcherTests.cs b/src/SignalR/server/SignalR/test/Internal/DefaultHubDispatcherTests.cs new file mode 100644 index 0000000000..f28ec35cb1 --- /dev/null +++ b/src/SignalR/server/SignalR/test/Internal/DefaultHubDispatcherTests.cs @@ -0,0 +1,143 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.IO.Pipelines; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Connections; +using Microsoft.AspNetCore.SignalR.Internal; +using Microsoft.AspNetCore.SignalR.Protocol; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using Xunit; + +namespace Microsoft.AspNetCore.SignalR.Tests.Internal +{ + public class DefaultHubDispatcherTests + { + private class MockHubConnectionContext : HubConnectionContext + { + public TaskCompletionSource ReceivedCompleted = new TaskCompletionSource(); + public List Values = new List(); + + public MockHubConnectionContext(ConnectionContext connectionContext, HubConnectionContextOptions contextOptions, ILoggerFactory loggerFactory) + : base(connectionContext, contextOptions, loggerFactory) { } + + public override ValueTask WriteAsync(HubMessage message, CancellationToken cancellationToken) + { + if (message is StreamItemMessage streamItemMessage) + Values.Add((TValue)streamItemMessage.Item); + + else if (message is CompletionMessage completionMessage) + { + ReceivedCompleted.TrySetResult(null); + + if (!string.IsNullOrEmpty(completionMessage.Error)) + { + throw new Exception("Error invoking hub method: " + completionMessage.Error); + } + } + + else throw new NotImplementedException(); + + return default; + } + } + + private static DefaultHubDispatcher CreateDispatcher() where THub : Hub + { + var serviceCollection = new ServiceCollection(); + serviceCollection.TryAddScoped(typeof(IHubActivator<>), typeof(DefaultHubActivator<>)); + var provider = serviceCollection.BuildServiceProvider(); + var serviceScopeFactory = provider.GetService(); + + return new DefaultHubDispatcher( + serviceScopeFactory, + new HubContext(new DefaultHubLifetimeManager(NullLogger>.Instance)), + Options.Create(new HubOptions()), + Options.Create(new HubOptions()), + new Logger>(NullLoggerFactory.Instance)); + } + + private static MockHubConnectionContext CreateConnectionContext() + { + var pair = DuplexPipe.CreateConnectionPair(PipeOptions.Default, PipeOptions.Default); + var connection = new DefaultConnectionContext(Guid.NewGuid().ToString(), pair.Application, pair.Transport); + var contextOptions = new HubConnectionContextOptions() { KeepAliveInterval = TimeSpan.Zero }; + + return new MockHubConnectionContext( + connection, + contextOptions, + NullLoggerFactory.Instance); + } + + /// + /// For . + /// + private interface ITestDerivedParameter + { + public string Value { get; } + } + + /// + /// For . + /// + private abstract class TestDerivedParameterBase + { + public TestDerivedParameterBase(string value) => Value = value; + public string Value { get; } + } + + /// + /// For . + /// + private class TestDerivedParameter : TestDerivedParameterBase, ITestDerivedParameter + { + public TestDerivedParameter(string value) : base(value) { } + } + + /// + /// For . + /// + private class TestDerivedParameterHub : Hub + { + public async IAsyncEnumerable TestSubclass(TestDerivedParameterBase param, [EnumeratorCancellation]CancellationToken token) + { + await Task.Yield(); + yield return param.Value; + } + + public async IAsyncEnumerable TestImplementation(ITestDerivedParameter param, [EnumeratorCancellation]CancellationToken token) + { + await Task.Yield(); + yield return param.Value; + } + } + + /// + /// Hub methods might be written by users in a way that accepts an interface or base class as a parameter + /// and deserialization could supply a derived class (e.g. Json.NET's TypeNameHandling = TypeNameHandling.All). + /// This test ensures implementation and subclass arguments are correctly bound for dispatch. + /// + [Theory] + [InlineData(nameof(TestDerivedParameterHub.TestImplementation))] + [InlineData(nameof(TestDerivedParameterHub.TestSubclass))] + public async Task DispatchesDerivedArguments(string methodName) + { + var message = new TestDerivedParameter("Yup"); + var connectionContext = CreateConnectionContext(); + var dispatcher = CreateDispatcher(); + + await dispatcher.DispatchMessageAsync(connectionContext, new StreamInvocationMessage("123", methodName, new[] { message })); + await (connectionContext as MockHubConnectionContext).ReceivedCompleted.Task; + + Assert.Single(connectionContext.Values, message.Value); + } + } +} From 47bb845d48a8be361113a0450037923810da7061 Mon Sep 17 00:00:00 2001 From: Nick Darvey Date: Tue, 25 Jun 2019 15:49:51 +1000 Subject: [PATCH 2/4] Moves derived parameter tests into HubConnectionHandlerTests --- .../HubConnectionHandlerTestUtils/Hubs.cs | 36 +++++ .../SignalR/test/HubConnectionHandlerTests.cs | 56 +++++++ .../Internal/DefaultHubDispatcherTests.cs | 143 ------------------ 3 files changed, 92 insertions(+), 143 deletions(-) delete mode 100644 src/SignalR/server/SignalR/test/Internal/DefaultHubDispatcherTests.cs diff --git a/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs b/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs index f99cb6c76e..94f3fff9dd 100644 --- a/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs +++ b/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs @@ -661,6 +661,30 @@ namespace Microsoft.AspNetCore.SignalR.Tests return output.Reader; } + public async IAsyncEnumerable DerivedParameterInterfaceAsyncEnumerable(IDerivedParameterTestObject param) + { + await Task.Yield(); + yield return param.Value; + } + + public async IAsyncEnumerable DerivedParameterBaseClassAsyncEnumerable(DerivedParameterTestObjectBase param) + { + await Task.Yield(); + yield return param.Value; + } + + public async IAsyncEnumerable DerivedParameterInterfaceAsyncEnumerableWithCancellation(IDerivedParameterTestObject param, [EnumeratorCancellation] CancellationToken token) + { + await Task.Yield(); + yield return param.Value; + } + + public async IAsyncEnumerable DerivedParameterBaseClassAsyncEnumerableWithCancellation(DerivedParameterTestObjectBase param, [EnumeratorCancellation] CancellationToken token) + { + await Task.Yield(); + yield return param.Value; + } + public class AsyncEnumerableImpl : IAsyncEnumerable { private readonly IAsyncEnumerable _inner; @@ -753,6 +777,18 @@ namespace Microsoft.AspNetCore.SignalR.Tests } } } + + public interface IDerivedParameterTestObject + { + public string Value { get; set; } + } + + public abstract class DerivedParameterTestObjectBase : IDerivedParameterTestObject + { + public string Value { get; set; } + } + + public class DerivedParameterTestObject : DerivedParameterTestObjectBase { } } public class SimpleHub : Hub diff --git a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs index 56b77c0b7a..dc58bd227b 100644 --- a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs +++ b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs @@ -3576,6 +3576,62 @@ namespace Microsoft.AspNetCore.SignalR.Tests } } + /// + /// Hub methods might be written by users in a way that accepts an interface or base class as a parameter + /// and deserialization could supply a derived class. + /// This test ensures implementation and subclass arguments are correctly bound for dispatch. + /// + [Theory] + [InlineData(nameof(StreamingHub.DerivedParameterInterfaceAsyncEnumerable))] + [InlineData(nameof(StreamingHub.DerivedParameterBaseClassAsyncEnumerable))] + [InlineData(nameof(StreamingHub.DerivedParameterInterfaceAsyncEnumerableWithCancellation))] + [InlineData(nameof(StreamingHub.DerivedParameterBaseClassAsyncEnumerableWithCancellation))] + public async Task CanPassDerivedParameterToStreamHubMethod(string method) + { + using (StartVerifiableLog()) + { + var argument = new StreamingHub.DerivedParameterTestObject { Value = "test" }; + var protocolOptions = new NewtonsoftJsonHubProtocolOptions + { + PayloadSerializerSettings = new JsonSerializerSettings() + { + // The usage of TypeNameHandling.All is a security risk. + // If you're implementing this in your own application instead use your own 'type' field and a custom JsonConverter + // or ensure you're restricting to only known types with a custom SerializationBinder. + // See https://github.com/aspnet/AspNetCore/issues/11495#issuecomment-505047422 + TypeNameHandling = TypeNameHandling.All + } + }; + var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider( + services => services.AddSignalR() + .AddNewtonsoftJsonProtocol(o => o.PayloadSerializerSettings = protocolOptions.PayloadSerializerSettings), + LoggerFactory); + var connectionHandler = serviceProvider.GetService>(); + var invocationBinder = new Mock(); + invocationBinder.Setup(b => b.GetStreamItemType(It.IsAny())).Returns(typeof(string)); + + using (var client = new TestClient( + protocol: new NewtonsoftJsonHubProtocol(Options.Create(protocolOptions)), + invocationBinder: invocationBinder.Object)) + { + var connectionHandlerTask = await client.ConnectAsync(connectionHandler); + + // Wait for a connection, or for the endpoint to fail. + await client.Connected.OrThrowIfOtherFails(connectionHandlerTask).OrTimeout(); + + var messages = await client.StreamAsync(method, argument).OrTimeout(); + + Assert.Equal(2, messages.Count); + HubConnectionHandlerTestUtils.AssertHubMessage(new StreamItemMessage(string.Empty, argument.Value), messages[0]); + HubConnectionHandlerTestUtils.AssertHubMessage(CompletionMessage.Empty(string.Empty), messages[1]); + + client.Dispose(); + + await connectionHandlerTask.OrTimeout(); + } + } + } + private class CustomHubActivator : IHubActivator where THub : Hub { public int ReleaseCount; diff --git a/src/SignalR/server/SignalR/test/Internal/DefaultHubDispatcherTests.cs b/src/SignalR/server/SignalR/test/Internal/DefaultHubDispatcherTests.cs deleted file mode 100644 index f28ec35cb1..0000000000 --- a/src/SignalR/server/SignalR/test/Internal/DefaultHubDispatcherTests.cs +++ /dev/null @@ -1,143 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; -using System.Collections.Generic; -using System.IO.Pipelines; -using System.Runtime.CompilerServices; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Connections; -using Microsoft.AspNetCore.SignalR.Internal; -using Microsoft.AspNetCore.SignalR.Protocol; -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.DependencyInjection.Extensions; -using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.Abstractions; -using Microsoft.Extensions.Options; -using Xunit; - -namespace Microsoft.AspNetCore.SignalR.Tests.Internal -{ - public class DefaultHubDispatcherTests - { - private class MockHubConnectionContext : HubConnectionContext - { - public TaskCompletionSource ReceivedCompleted = new TaskCompletionSource(); - public List Values = new List(); - - public MockHubConnectionContext(ConnectionContext connectionContext, HubConnectionContextOptions contextOptions, ILoggerFactory loggerFactory) - : base(connectionContext, contextOptions, loggerFactory) { } - - public override ValueTask WriteAsync(HubMessage message, CancellationToken cancellationToken) - { - if (message is StreamItemMessage streamItemMessage) - Values.Add((TValue)streamItemMessage.Item); - - else if (message is CompletionMessage completionMessage) - { - ReceivedCompleted.TrySetResult(null); - - if (!string.IsNullOrEmpty(completionMessage.Error)) - { - throw new Exception("Error invoking hub method: " + completionMessage.Error); - } - } - - else throw new NotImplementedException(); - - return default; - } - } - - private static DefaultHubDispatcher CreateDispatcher() where THub : Hub - { - var serviceCollection = new ServiceCollection(); - serviceCollection.TryAddScoped(typeof(IHubActivator<>), typeof(DefaultHubActivator<>)); - var provider = serviceCollection.BuildServiceProvider(); - var serviceScopeFactory = provider.GetService(); - - return new DefaultHubDispatcher( - serviceScopeFactory, - new HubContext(new DefaultHubLifetimeManager(NullLogger>.Instance)), - Options.Create(new HubOptions()), - Options.Create(new HubOptions()), - new Logger>(NullLoggerFactory.Instance)); - } - - private static MockHubConnectionContext CreateConnectionContext() - { - var pair = DuplexPipe.CreateConnectionPair(PipeOptions.Default, PipeOptions.Default); - var connection = new DefaultConnectionContext(Guid.NewGuid().ToString(), pair.Application, pair.Transport); - var contextOptions = new HubConnectionContextOptions() { KeepAliveInterval = TimeSpan.Zero }; - - return new MockHubConnectionContext( - connection, - contextOptions, - NullLoggerFactory.Instance); - } - - /// - /// For . - /// - private interface ITestDerivedParameter - { - public string Value { get; } - } - - /// - /// For . - /// - private abstract class TestDerivedParameterBase - { - public TestDerivedParameterBase(string value) => Value = value; - public string Value { get; } - } - - /// - /// For . - /// - private class TestDerivedParameter : TestDerivedParameterBase, ITestDerivedParameter - { - public TestDerivedParameter(string value) : base(value) { } - } - - /// - /// For . - /// - private class TestDerivedParameterHub : Hub - { - public async IAsyncEnumerable TestSubclass(TestDerivedParameterBase param, [EnumeratorCancellation]CancellationToken token) - { - await Task.Yield(); - yield return param.Value; - } - - public async IAsyncEnumerable TestImplementation(ITestDerivedParameter param, [EnumeratorCancellation]CancellationToken token) - { - await Task.Yield(); - yield return param.Value; - } - } - - /// - /// Hub methods might be written by users in a way that accepts an interface or base class as a parameter - /// and deserialization could supply a derived class (e.g. Json.NET's TypeNameHandling = TypeNameHandling.All). - /// This test ensures implementation and subclass arguments are correctly bound for dispatch. - /// - [Theory] - [InlineData(nameof(TestDerivedParameterHub.TestImplementation))] - [InlineData(nameof(TestDerivedParameterHub.TestSubclass))] - public async Task DispatchesDerivedArguments(string methodName) - { - var message = new TestDerivedParameter("Yup"); - var connectionContext = CreateConnectionContext(); - var dispatcher = CreateDispatcher(); - - await dispatcher.DispatchMessageAsync(connectionContext, new StreamInvocationMessage("123", methodName, new[] { message })); - await (connectionContext as MockHubConnectionContext).ReceivedCompleted.Task; - - Assert.Single(connectionContext.Values, message.Value); - } - } -} From 3fc4bee7e26a4323e49354851fc11625904bf096 Mon Sep 17 00:00:00 2001 From: Nick Darvey Date: Wed, 26 Jun 2019 10:35:03 +1000 Subject: [PATCH 3/4] Adds DerivedParameterKnownTypesBinder so that we're following better practice in our tests --- .../HubConnectionHandlerTestUtils/Hubs.cs | 21 +++++++++++++++++++ .../SignalR/test/HubConnectionHandlerTests.cs | 7 ++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs b/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs index 94f3fff9dd..b0aeeffae3 100644 --- a/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs +++ b/src/SignalR/server/SignalR/test/HubConnectionHandlerTestUtils/Hubs.cs @@ -3,12 +3,14 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Runtime.CompilerServices; using System.Text; using System.Threading; using System.Threading.Channels; using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; +using Newtonsoft.Json.Serialization; namespace Microsoft.AspNetCore.SignalR.Tests { @@ -789,6 +791,25 @@ namespace Microsoft.AspNetCore.SignalR.Tests } public class DerivedParameterTestObject : DerivedParameterTestObjectBase { } + + public class DerivedParameterKnownTypesBinder : ISerializationBinder + { + private static readonly IEnumerable _knownTypes = new List() + { + typeof(DerivedParameterTestObject) + }; + + public static ISerializationBinder Instance { get; } = new DerivedParameterKnownTypesBinder(); + + public void BindToName(Type serializedType, out string assemblyName, out string typeName) + { + assemblyName = null; + typeName = serializedType.Name; + } + + public Type BindToType(string assemblyName, string typeName) => + _knownTypes.Single(type => type.Name == typeName); + } } public class SimpleHub : Hub diff --git a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs index dc58bd227b..b4aad749d2 100644 --- a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs +++ b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs @@ -3595,11 +3595,8 @@ namespace Microsoft.AspNetCore.SignalR.Tests { PayloadSerializerSettings = new JsonSerializerSettings() { - // The usage of TypeNameHandling.All is a security risk. - // If you're implementing this in your own application instead use your own 'type' field and a custom JsonConverter - // or ensure you're restricting to only known types with a custom SerializationBinder. - // See https://github.com/aspnet/AspNetCore/issues/11495#issuecomment-505047422 - TypeNameHandling = TypeNameHandling.All + TypeNameHandling = TypeNameHandling.All, + SerializationBinder = StreamingHub.DerivedParameterKnownTypesBinder.Instance } }; var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider( From 32998b9e3c2483dae623a665df171b8e773b1d78 Mon Sep 17 00:00:00 2001 From: Nick Darvey Date: Wed, 26 Jun 2019 12:50:42 +1000 Subject: [PATCH 4/4] Adds comment about the security of TypeNameHandling.All --- src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs index b4aad749d2..ed17fe0702 100644 --- a/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs +++ b/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs @@ -3595,6 +3595,10 @@ namespace Microsoft.AspNetCore.SignalR.Tests { PayloadSerializerSettings = new JsonSerializerSettings() { + // The usage of TypeNameHandling.All is a security risk. + // If you're implementing this in your own application instead use your own 'type' field and a custom JsonConverter + // or ensure you're restricting to only known types with a custom SerializationBinder like we are here. + // See https://github.com/aspnet/AspNetCore/issues/11495#issuecomment-505047422 TypeNameHandling = TypeNameHandling.All, SerializationBinder = StreamingHub.DerivedParameterKnownTypesBinder.Instance }