Harden StartCircuit (#12825)

* Harden StartCircuit

Fixes: #12057

Adds some upfront argument validation as well as error handling for
circuit intialization failures.

Author: rynowak

src/Components/Components/ref/Microsoft.AspNetCore.Components.netstandard2.0.cs

@@ -278,6 +278,10 @@ protected LayoutComponentBase() { }
         [Microsoft.AspNetCore.Components.ParameterAttribute]
         public Microsoft.AspNetCore.Components.RenderFragment Body { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
     }
+    public sealed partial class LocationChangeException : System.Exception
+    {
+        public LocationChangeException(string message, System.Exception innerException) { }
+    }
     [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
     public readonly partial struct MarkupString
     {

src/Components/Components/src/LocationChangeException.cs

@@ -0,0 +1,23 @@
+// 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;
+
+namespace Microsoft.AspNetCore.Components
+{
+    /// <summary>
+    /// An exception thrown when <see cref="NavigationManager.LocationChanged"/> throws an exception.
+    /// </summary>
+    public sealed class LocationChangeException : Exception
+    {
+        /// <summary>
+        /// Creates a new instance of <see cref="LocationChangeException"/>.
+        /// </summary>
+        /// <param name="message">The exception message.</param>
+        /// <param name="innerException">The inner exception.</param>
+        public LocationChangeException(string message, Exception innerException)
+            : base(message, innerException)
+        {
+        }
+    }
+}

src/Components/Components/src/NavigationManager.cs

@@ -2,7 +2,6 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.Runtime.InteropServices.ComTypes;
 using Microsoft.AspNetCore.Components.Routing;
 
 namespace Microsoft.AspNetCore.Components
@@ -129,8 +128,9 @@ protected void Initialize(string baseUri, string uri)
 
             _isInitialized = true;
 
-            Uri = uri;
+            // Setting BaseUri before Uri so they get validated.
             BaseUri = baseUri;
+            Uri = uri;
         }
 
         /// <summary>
@@ -201,7 +201,14 @@ internal static string NormalizeBaseUri(string baseUri)
         /// </summary>
         protected void NotifyLocationChanged(bool isInterceptedLink)
         {
-            _locationChanged?.Invoke(this, new LocationChangedEventArgs(_uri, isInterceptedLink));
+            try
+            {
+                _locationChanged?.Invoke(this, new LocationChangedEventArgs(_uri, isInterceptedLink));
+            }
+            catch (Exception ex)
+            {
+                throw new LocationChangeException("An exception occurred while dispatching a location changed event.", ex);
+            }
         }
 
         private void AssertInitialized()

src/Components/Components/test/NavigationManagerTest.cs

@@ -38,6 +38,24 @@ public void ComputesCorrectValidBaseRelativePaths(string baseUri, string uri, st
             Assert.Equal(expectedResult, actualResult);
         }
 
+        [Theory]
+        [InlineData("scheme://host/", "otherscheme://host/")]
+        [InlineData("scheme://host/", "scheme://otherhost/")]
+        [InlineData("scheme://host/path/", "scheme://host/")]
+        public void Initialize_ThrowsForInvalidBaseRelativePaths(string baseUri, string absoluteUri)
+        {
+            var navigationManager = new TestNavigationManager();
+
+            var ex = Assert.Throws<ArgumentException>(() =>
+            {
+                navigationManager.Initialize(baseUri, absoluteUri);
+            });
+
+            Assert.Equal(
+                $"The URI '{absoluteUri}' is not contained by the base URI '{baseUri}'.",
+                ex.Message);
+        }
+
         [Theory]
         [InlineData("scheme://host/", "otherscheme://host/")]
         [InlineData("scheme://host/", "scheme://otherhost/")]
@@ -76,9 +94,18 @@ public void ToBaseRelativePath_ThrowsForInvalidBaseRelativePaths(string baseUri,
 
         private class TestNavigationManager : NavigationManager
         {
+            public TestNavigationManager()
+            {
+            }
+
             public TestNavigationManager(string baseUri = null, string uri = null)
             {
-                Initialize(baseUri ?? "http://example.com/", uri ?? "http://example.com/welcome-page");
+                Initialize(baseUri ?? "http://example.com/", uri ?? baseUri ?? "http://example.com/welcome-page");
+            }
+
+            public new void Initialize(string baseUri, string uri)
+            {
+                base.Initialize(baseUri, uri);
             }
 
             protected override void NavigateToCore(string uri, bool forceLoad)

src/Components/Server/src/Circuits/DefaultCircuitFactory.cs

@@ -43,6 +43,8 @@ public override CircuitHost CreateCircuitHost(
             string uri,
             ClaimsPrincipal user)
         {
+            // We do as much intialization as possible eagerly in this method, which makes the error handling
+            // story much simpler. If we throw from here, it's handled inside the initial hub method.
             var components = ResolveComponentMetadata(httpContext, client);
 
             var scope = _scopeFactory.CreateScope();

src/Components/Server/src/ComponentHub.cs

@@ -110,6 +110,18 @@ public string StartCircuit(string baseUri, string uri)
                 return null;
             }
 
+            if (baseUri == null ||
+                uri == null ||
+                !Uri.IsWellFormedUriString(baseUri, UriKind.Absolute) ||
+                !Uri.IsWellFormedUriString(uri, UriKind.Absolute))
+            {
+                // We do some really minimal validation here to prevent obviously wrong data from getting in
+                // without duplicating too much logic.
+                Log.InvalidInputData(_logger);
+                _ = NotifyClientError(Clients.Caller, $"The uris provided are invalid.");
+                return null;
+            }
+
             var circuitClient = new CircuitClientProxy(Clients.Caller, Context.ConnectionId);
             if (DefaultCircuitFactory.ResolveComponentMetadata(Context.GetHttpContext(), circuitClient).Count == 0)
             {
@@ -122,26 +134,35 @@ public string StartCircuit(string baseUri, string uri)
                 return null;
             }
 
-            var circuitHost = _circuitFactory.CreateCircuitHost(
-                Context.GetHttpContext(),
-                circuitClient,
-                baseUri,
-                uri,
-                Context.User);
-
-            circuitHost.UnhandledException += CircuitHost_UnhandledException;
-
-            // Fire-and-forget the initialization process, because we can't block the
-            // SignalR message loop (we'd get a deadlock if any of the initialization
-            // logic relied on receiving a subsequent message from SignalR), and it will
-            // take care of its own errors anyway.
-            _ = circuitHost.InitializeAsync(Context.ConnectionAborted);
-
-            _circuitRegistry.Register(circuitHost);
-
-            CircuitHost = circuitHost;
-
-            return circuitHost.CircuitId;
+            try
+            {
+                var circuitHost = _circuitFactory.CreateCircuitHost(
+                    Context.GetHttpContext(),
+                    circuitClient,
+                    baseUri,
+                    uri,
+                    Context.User);
+
+                circuitHost.UnhandledException += CircuitHost_UnhandledException;
+
+                // Fire-and-forget the initialization process, because we can't block the
+                // SignalR message loop (we'd get a deadlock if any of the initialization
+                // logic relied on receiving a subsequent message from SignalR), and it will
+                // take care of its own errors anyway.
+                _ = circuitHost.InitializeAsync(Context.ConnectionAborted);
+
+                // It's safe to *publish* the circuit now because nothing will be able
+                // to run inside it until after InitializeAsync completes.
+                _circuitRegistry.Register(circuitHost);
+                CircuitHost = circuitHost;
+                return circuitHost.CircuitId;
+            }
+            catch (Exception ex)
+            {
+                Log.CircuitInitializationFailed(_logger, ex);
+                NotifyClientError(Clients.Caller, "The circuit failed to initialize.");
+                return null;
+            }
         }
 
         /// <summary>
@@ -292,6 +313,12 @@ private static class Log
             private static readonly Action<ILogger, string, Exception> _circuitTerminatedGracefully =
                 LoggerMessage.Define<string>(LogLevel.Debug, new EventId(7, "CircuitTerminatedGracefully"), "Circuit '{CircuitId}' terminated gracefully");
 
+            private static readonly Action<ILogger, string, Exception> _invalidInputData =
+                LoggerMessage.Define<string>(LogLevel.Debug, new EventId(8, "InvalidInputData"), "Call to '{CallSite}' received invalid input data");
+
+            private static readonly Action<ILogger, Exception> _circuitInitializationFailed =
+                LoggerMessage.Define(LogLevel.Debug, new EventId(9, "CircuitInitializationFailed"), "Circuit initialization failed");
+
             public static void NoComponentsRegisteredInEndpoint(ILogger logger, string endpointDisplayName)
             {
                 _noComponentsRegisteredInEndpoint(logger, endpointDisplayName, null);
@@ -317,6 +344,10 @@ public static void FailedToTransmitException(ILogger logger, string circuitId, E
             public static void CircuitHostNotInitialized(ILogger logger, [CallerMemberName] string callSite = "") => _circuitHostNotInitialized(logger, callSite, null);
 
             public static void CircuitTerminatedGracefully(ILogger logger, string circuitId) => _circuitTerminatedGracefully(logger, circuitId, null);
+
+            public static void InvalidInputData(ILogger logger, [CallerMemberName] string callSite = "") => _invalidInputData(logger, callSite, null);
+
+            public static void CircuitInitializationFailed(ILogger logger, Exception exception) => _circuitInitializationFailed(logger, exception);
         }
     }
 }

src/Components/test/E2ETest/ServerExecutionTests/ComponentHubReliabilityTest.cs

@@ -61,7 +61,65 @@ public async Task CannotStartMultipleCircuits()
             await Client.ExpectCircuitError(() => Client.HubConnection.SendAsync(
                 "StartCircuit",
                 baseUri,
-                baseUri.GetLeftPart(UriPartial.Authority)));
+                baseUri + "/home"));
+
+            // Assert
+            var actualError = Assert.Single(Errors);
+            Assert.Matches(expectedError, actualError);
+            Assert.DoesNotContain(Logs, l => l.LogLevel > LogLevel.Information);
+        }
+
+        [Fact]
+        public async Task CannotStartCircuitWithNullData()
+        {
+            // Arrange
+            var expectedError = "The uris provided are invalid.";
+            var rootUri = _serverFixture.RootUri;
+            var uri = new Uri(rootUri, "/subdir");
+            Assert.True(await Client.ConnectAsync(uri, prerendered: false, connectAutomatically: false), "Couldn't connect to the app");
+
+            // Act
+            await Client.ExpectCircuitError(() => Client.HubConnection.SendAsync("StartCircuit", null, null));
+
+            // Assert
+            var actualError = Assert.Single(Errors);
+            Assert.Matches(expectedError, actualError);
+            Assert.DoesNotContain(Logs, l => l.LogLevel > LogLevel.Information);
+        }
+
+        [Fact]
+        public async Task CannotStartCircuitWithInvalidUris()
+        {
+            // Arrange
+            var expectedError = "The uris provided are invalid.";
+            var rootUri = _serverFixture.RootUri;
+            var uri = new Uri(rootUri, "/subdir");
+            Assert.True(await Client.ConnectAsync(uri, prerendered: false, connectAutomatically: false), "Couldn't connect to the app");
+
+            // Act
+            await Client.ExpectCircuitError(() => Client.HubConnection.SendAsync("StartCircuit", uri.AbsoluteUri, "/foo"));
+
+            // Assert
+            var actualError = Assert.Single(Errors);
+            Assert.Matches(expectedError, actualError);
+            Assert.DoesNotContain(Logs, l => l.LogLevel > LogLevel.Information);
+        }
+
+        // This is a hand-chosen example of something that will cause an exception in creating the circuit host.
+        // We want to test this case so that we know what happens when creating the circuit host blows up.
+        [Fact]
+        public async Task StartCircuitCausesInitializationError()
+        {
+            // Arrange
+            var expectedError = "The circuit failed to initialize.";
+            var rootUri = _serverFixture.RootUri;
+            var uri = new Uri(rootUri, "/subdir");
+            Assert.True(await Client.ConnectAsync(uri, prerendered: false, connectAutomatically: false), "Couldn't connect to the app");
+
+            // Act
+            //
+            // These are valid URIs by the BaseUri doesn't contain the Uri - so it fails to initialize.
+            await Client.ExpectCircuitError(() => Client.HubConnection.SendAsync("StartCircuit", uri, "http://example.com"), TimeSpan.FromHours(1));
 
             // Assert
             var actualError = Assert.Single(Errors);