From 7d545d40aa9e4ce1164d341412799ba94604e4a6 Mon Sep 17 00:00:00 2001 From: Javier Calvarro Nelson Date: Fri, 5 Jul 2019 16:28:41 +0200 Subject: [PATCH] Javiercn/js interop improvements part 1 (#11809) * Adds exception sanitization to RemoteJSRuntime * Adds default async timeouts of 1 minute for JS async calls to server-side blazor --- ...NetCore.Components.Server.netcoreapp3.0.cs | 2 + src/Components/Server/src/CircuitOptions.cs | 20 +++++- ...onsJSInteropDetailedErrorsConfiguration.cs | 24 +++++++ .../Server/src/Circuits/RemoteJSRuntime.cs | 21 ++++++ .../ComponentServiceCollectionExtensions.cs | 4 +- ...rosoft.AspNetCore.Components.Server.csproj | 2 +- .../Server/test/Circuits/TestCircuitHost.cs | 3 +- .../ServerFixtures/AspNetSiteServerFixture.cs | 6 +- .../ToggleExecutionModeServerFixture.cs | 10 +++ ...soft.AspNetCore.Components.E2ETests.csproj | 2 +- ...verInteropTestDefaultExceptionsBehavior.cs | 70 +++++++++++++++++++ ...nteropTestJsInvocationsTimeoutsBehavior.cs | 51 ++++++++++++++ .../ServerExecutionTests/TestSubclasses.cs | 6 +- .../test/testassets/BasicTestApp/Index.razor | 1 + .../BasicTestApp/LongRunningInterop.razor | 53 ++++++++++++++ .../BasicTestApp/wwwroot/js/jsinteroptests.js | 6 ++ .../test/testassets/Ignitor/Ignitor.csproj | 1 - .../test/testassets/TestServer/Startup.cs | 7 +- ...perComponentPrerenderingExtensionsTests.cs | 2 + 19 files changed, 281 insertions(+), 10 deletions(-) create mode 100644 src/Components/Server/src/Circuits/CircuitOptionsJSInteropDetailedErrorsConfiguration.cs create mode 100644 src/Components/test/E2ETest/ServerExecutionTests/ServerInteropTestDefaultExceptionsBehavior.cs create mode 100644 src/Components/test/E2ETest/ServerExecutionTests/ServerInteropTestJsInvocationsTimeoutsBehavior.cs create mode 100644 src/Components/test/testassets/BasicTestApp/LongRunningInterop.razor diff --git a/src/Components/Server/ref/Microsoft.AspNetCore.Components.Server.netcoreapp3.0.cs b/src/Components/Server/ref/Microsoft.AspNetCore.Components.Server.netcoreapp3.0.cs index 8e590622a6..4470d13745 100644 --- a/src/Components/Server/ref/Microsoft.AspNetCore.Components.Server.netcoreapp3.0.cs +++ b/src/Components/Server/ref/Microsoft.AspNetCore.Components.Server.netcoreapp3.0.cs @@ -27,6 +27,8 @@ namespace Microsoft.AspNetCore.Components.Server { public CircuitOptions() { } public System.TimeSpan DisconnectedCircuitRetentionPeriod { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } + public System.TimeSpan JSInteropDefaultCallTimeout { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } + public bool JSInteropDetailedErrors { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public int MaxRetainedDisconnectedCircuits { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } } public sealed partial class ComponentEndpointConventionBuilder : Microsoft.AspNetCore.Builder.IEndpointConventionBuilder, Microsoft.AspNetCore.SignalR.IHubEndpointConventionBuilder diff --git a/src/Components/Server/src/CircuitOptions.cs b/src/Components/Server/src/CircuitOptions.cs index d4f444fe91..5e1dcd371f 100644 --- a/src/Components/Server/src/CircuitOptions.cs +++ b/src/Components/Server/src/CircuitOptions.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 Microsoft.AspNetCore.DataProtection; namespace Microsoft.AspNetCore.Components.Server { @@ -46,5 +45,24 @@ namespace Microsoft.AspNetCore.Components.Server /// Defaults to 3 minutes. /// public TimeSpan DisconnectedCircuitRetentionPeriod { get; set; } = TimeSpan.FromMinutes(3); + + /// + /// Gets or sets a value that determines whether or not to send detailed exception messages from .NET interop method invocation + /// exceptions to JavaScript. + /// + /// + /// This value should only be turned on in development scenarios as turning it on in production might result in the leak of + /// sensitive information to untrusted parties. + /// + /// Defaults to false. + public bool JSInteropDetailedErrors { get; set; } + + /// + /// Gets or sets a value that indicates how long the server will wait before timing out an asynchronous JavaScript function invocation. + /// + /// + /// Defaults to 1 minute. + /// + public TimeSpan JSInteropDefaultCallTimeout { get; set; } = TimeSpan.FromMinutes(1); } } diff --git a/src/Components/Server/src/Circuits/CircuitOptionsJSInteropDetailedErrorsConfiguration.cs b/src/Components/Server/src/Circuits/CircuitOptionsJSInteropDetailedErrorsConfiguration.cs new file mode 100644 index 0000000000..3f619698f1 --- /dev/null +++ b/src/Components/Server/src/Circuits/CircuitOptionsJSInteropDetailedErrorsConfiguration.cs @@ -0,0 +1,24 @@ +// 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 Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Options; + +namespace Microsoft.AspNetCore.Components.Server +{ + internal class CircuitOptionsJSInteropDetailedErrorsConfiguration : IConfigureOptions + { + public CircuitOptionsJSInteropDetailedErrorsConfiguration(IConfiguration configuration) + { + Configuration = configuration; + } + + public IConfiguration Configuration { get; } + + public void Configure(CircuitOptions options) + { + options.JSInteropDetailedErrors = Configuration.GetValue(WebHostDefaults.DetailedErrorsKey); + } + } +} diff --git a/src/Components/Server/src/Circuits/RemoteJSRuntime.cs b/src/Components/Server/src/Circuits/RemoteJSRuntime.cs index 2ad9a66afa..34649ddb9b 100644 --- a/src/Components/Server/src/Circuits/RemoteJSRuntime.cs +++ b/src/Components/Server/src/Circuits/RemoteJSRuntime.cs @@ -3,19 +3,40 @@ using System; using Microsoft.AspNetCore.SignalR; +using Microsoft.Extensions.Options; using Microsoft.JSInterop; namespace Microsoft.AspNetCore.Components.Server.Circuits { internal class RemoteJSRuntime : JSRuntimeBase { + private readonly CircuitOptions _options; private CircuitClientProxy _clientProxy; + public RemoteJSRuntime(IOptions options) + { + _options = options.Value; + DefaultAsyncTimeout = _options.JSInteropDefaultCallTimeout; + } + internal void Initialize(CircuitClientProxy clientProxy) { _clientProxy = clientProxy ?? throw new ArgumentNullException(nameof(clientProxy)); } + protected override object OnDotNetInvocationException(Exception exception, string assemblyName, string methodIdentifier) + { + if (_options.JSInteropDetailedErrors) + { + return base.OnDotNetInvocationException(exception, assemblyName, methodIdentifier); + } + + var message = $"There was an exception invoking '{methodIdentifier}' on assembly '{assemblyName}'. For more details turn on " + + $"detailed exceptions in '{typeof(CircuitOptions).Name}.{nameof(CircuitOptions.JSInteropDetailedErrors)}'"; + + return message; + } + protected override void BeginInvokeJS(long asyncHandle, string identifier, string argsJson) { if (!_clientProxy.Connected) diff --git a/src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs b/src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs index 63db99ed44..826410e0d1 100644 --- a/src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs +++ b/src/Components/Server/src/DependencyInjection/ComponentServiceCollectionExtensions.cs @@ -78,9 +78,11 @@ namespace Microsoft.Extensions.DependencyInjection services.AddScoped(); services.AddScoped(); + services.TryAddEnumerable(ServiceDescriptor.Singleton, CircuitOptionsJSInteropDetailedErrorsConfiguration>()); + if (configure != null) { - services.Configure(configure); + services.Configure(configure); } return builder; diff --git a/src/Components/Server/src/Microsoft.AspNetCore.Components.Server.csproj b/src/Components/Server/src/Microsoft.AspNetCore.Components.Server.csproj index 9b42d39b56..734023b0f5 100644 --- a/src/Components/Server/src/Microsoft.AspNetCore.Components.Server.csproj +++ b/src/Components/Server/src/Microsoft.AspNetCore.Components.Server.csproj @@ -1,4 +1,4 @@ - + netcoreapp3.0 diff --git a/src/Components/Server/test/Circuits/TestCircuitHost.cs b/src/Components/Server/test/Circuits/TestCircuitHost.cs index f82ec5287e..afb279dd0e 100644 --- a/src/Components/Server/test/Circuits/TestCircuitHost.cs +++ b/src/Components/Server/test/Circuits/TestCircuitHost.cs @@ -13,6 +13,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Moq; +using Microsoft.Extensions.Options; namespace Microsoft.AspNetCore.Components.Server.Circuits { @@ -38,7 +39,7 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits serviceScope = serviceScope ?? Mock.Of(); clientProxy = clientProxy ?? new CircuitClientProxy(Mock.Of(), Guid.NewGuid().ToString()); var renderRegistry = new RendererRegistry(); - var jsRuntime = new RemoteJSRuntime(); + var jsRuntime = new RemoteJSRuntime(Options.Create(new CircuitOptions())); var dispatcher = Rendering.Renderer.CreateDefaultDispatcher(); if (remoteRenderer == null) diff --git a/src/Components/test/E2ETest/Infrastructure/ServerFixtures/AspNetSiteServerFixture.cs b/src/Components/test/E2ETest/Infrastructure/ServerFixtures/AspNetSiteServerFixture.cs index dfadf38985..2128c5dfe6 100644 --- a/src/Components/test/E2ETest/Infrastructure/ServerFixtures/AspNetSiteServerFixture.cs +++ b/src/Components/test/E2ETest/Infrastructure/ServerFixtures/AspNetSiteServerFixture.cs @@ -2,7 +2,9 @@ // 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; +using System.Linq; using System.Reflection; using Microsoft.AspNetCore.Hosting; @@ -18,6 +20,8 @@ namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures public AspNetEnvironment Environment { get; set; } = AspNetEnvironment.Production; + public List AdditionalArguments { get; set; } = new List(); + protected override IWebHost CreateWebHost() { if (BuildWebHostMethod == null) @@ -34,7 +38,7 @@ namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures "--urls", "http://127.0.0.1:0", "--contentroot", sampleSitePath, "--environment", Environment.ToString(), - }); + }.Concat(AdditionalArguments).ToArray()); } } } diff --git a/src/Components/test/E2ETest/Infrastructure/ServerFixtures/ToggleExecutionModeServerFixture.cs b/src/Components/test/E2ETest/Infrastructure/ServerFixtures/ToggleExecutionModeServerFixture.cs index 759b871852..56be3c9256 100644 --- a/src/Components/test/E2ETest/Infrastructure/ServerFixtures/ToggleExecutionModeServerFixture.cs +++ b/src/Components/test/E2ETest/Infrastructure/ServerFixtures/ToggleExecutionModeServerFixture.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures { @@ -15,6 +16,8 @@ namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures private AspNetSiteServerFixture.BuildWebHost _buildWebHostMethod; private IDisposable _serverToDispose; + public List AspNetFixtureAdditionalArguments { get; set; } = new List(); + public void UseAspNetHost(AspNetSiteServerFixture.BuildWebHost buildWebHostMethod) { _buildWebHostMethod = buildWebHostMethod @@ -35,6 +38,7 @@ namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures { // Use specified ASP.NET host server var underlying = new AspNetSiteServerFixture(); + underlying.AdditionalArguments.AddRange(AspNetFixtureAdditionalArguments); underlying.BuildWebHostMethod = _buildWebHostMethod; _serverToDispose = underlying; return underlying.RootUri.AbsoluteUri; @@ -45,6 +49,12 @@ namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures { _serverToDispose?.Dispose(); } + + internal ToggleExecutionModeServerFixture WithAdditionalArguments(string [] additionalArguments) + { + AspNetFixtureAdditionalArguments.AddRange(additionalArguments); + return this; + } } public enum ExecutionMode { Client, Server } diff --git a/src/Components/test/E2ETest/Microsoft.AspNetCore.Components.E2ETests.csproj b/src/Components/test/E2ETest/Microsoft.AspNetCore.Components.E2ETests.csproj index f4a5e45eb5..895b4f171c 100644 --- a/src/Components/test/E2ETest/Microsoft.AspNetCore.Components.E2ETests.csproj +++ b/src/Components/test/E2ETest/Microsoft.AspNetCore.Components.E2ETests.csproj @@ -7,7 +7,7 @@ netcoreapp3.0 Components.E2ETests - true + true false diff --git a/src/Components/test/E2ETest/ServerExecutionTests/ServerInteropTestDefaultExceptionsBehavior.cs b/src/Components/test/E2ETest/ServerExecutionTests/ServerInteropTestDefaultExceptionsBehavior.cs new file mode 100644 index 0000000000..1f725fd0bc --- /dev/null +++ b/src/Components/test/E2ETest/ServerExecutionTests/ServerInteropTestDefaultExceptionsBehavior.cs @@ -0,0 +1,70 @@ +// 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 BasicTestApp; +using Microsoft.AspNetCore.Components.E2ETest.Infrastructure; +using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures; +using Microsoft.AspNetCore.Components.Server; +using Microsoft.AspNetCore.E2ETesting; +using OpenQA.Selenium; +using OpenQA.Selenium.Support.UI; +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests +{ + public class ServerInteropTestDefaultExceptionsBehavior : BasicTestAppTestBase + { + public ServerInteropTestDefaultExceptionsBehavior( + BrowserFixture browserFixture, + ToggleExecutionModeServerFixture serverFixture, + ITestOutputHelper output) + : base(browserFixture, serverFixture.WithServerExecution(), output) + { + } + + protected override void InitializeAsyncCore() + { + Navigate(ServerPathBase, noReload: true); + MountTestComponent(); + } + + [Fact] + public void DotNetExceptionDetailsAreNotLoggedByDefault() + { + // Arrange + var expectedValues = new Dictionary + { + ["AsyncThrowSyncException"] = GetExpectedMessage("AsyncThrowSyncException"), + ["AsyncThrowAsyncException"] = GetExpectedMessage("AsyncThrowAsyncException"), + }; + + var actualValues = new Dictionary(); + + // Act + var interopButton = Browser.FindElement(By.Id("btn-interop")); + interopButton.Click(); + + var wait = new WebDriverWait(Browser, TimeSpan.FromSeconds(10)) + .Until(d => d.FindElement(By.Id("done-with-interop"))); + + foreach (var expectedValue in expectedValues) + { + var currentValue = Browser.FindElement(By.Id(expectedValue.Key)); + actualValues.Add(expectedValue.Key, currentValue.Text); + } + + // Assert + foreach (var expectedValue in expectedValues) + { + Assert.Equal(expectedValue.Value, actualValues[expectedValue.Key]); + } + + string GetExpectedMessage(string method) => + $"\"There was an exception invoking '{method}' on assembly 'BasicTestApp'. For more details turn on " + + $"detailed exceptions in '{typeof(CircuitOptions).Name}.{nameof(CircuitOptions.JSInteropDetailedErrors)}'\""; + } + } +} diff --git a/src/Components/test/E2ETest/ServerExecutionTests/ServerInteropTestJsInvocationsTimeoutsBehavior.cs b/src/Components/test/E2ETest/ServerExecutionTests/ServerInteropTestJsInvocationsTimeoutsBehavior.cs new file mode 100644 index 0000000000..696667a397 --- /dev/null +++ b/src/Components/test/E2ETest/ServerExecutionTests/ServerInteropTestJsInvocationsTimeoutsBehavior.cs @@ -0,0 +1,51 @@ +// 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.Threading.Tasks; +using BasicTestApp; +using Microsoft.AspNetCore.Components.E2ETest.Infrastructure; +using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures; +using Microsoft.AspNetCore.E2ETesting; +using OpenQA.Selenium; +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests +{ + public class ServerInteropTestJsInvocationsTimeoutsBehavior : BasicTestAppTestBase + { + public ServerInteropTestJsInvocationsTimeoutsBehavior( + BrowserFixture browserFixture, + ToggleExecutionModeServerFixture serverFixture, + ITestOutputHelper output) + : base(browserFixture, serverFixture.WithServerExecution(), output) + { + } + + protected override void InitializeAsyncCore() + { + Navigate(ServerPathBase, noReload: true); + MountTestComponent(); + } + + [Fact] + public async Task LongRunningJavaScriptFunctionsResultInCancellationAndWorkingAppAfterFunctionCompletion() + { + // Act & Assert + var interopButton = Browser.FindElement(By.Id("btn-interop")); + interopButton.Click(); + + Browser.Exists(By.Id("done-with-interop")); + + Browser.Exists(By.Id("task-was-cancelled")); + + // wait 10 seconds, js method completes in 5 seconds, after this point it would have triggered a completion for sure. + await Task.Delay(10000); + + var circuitFunctional = Browser.FindElement(By.Id("circuit-functional")); + circuitFunctional.Click(); + + Browser.Exists(By.Id("done-circuit-functional")); + } + } +} diff --git a/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs b/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs index 1f16095af5..c99c1d3828 100644 --- a/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs +++ b/src/Components/test/E2ETest/ServerExecutionTests/TestSubclasses.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using BasicTestApp; -using Microsoft.AspNetCore.Components.E2ETest.Infrastructure; using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures; using Microsoft.AspNetCore.Components.E2ETest.Tests; using Microsoft.AspNetCore.E2ETesting; @@ -37,9 +36,12 @@ namespace Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests public class ServerInteropTest : InteropTest { public ServerInteropTest(BrowserFixture browserFixture, ToggleExecutionModeServerFixture serverFixture, ITestOutputHelper output) - : base(browserFixture, serverFixture.WithServerExecution(), output) + : base(browserFixture, serverFixture.WithServerExecution().WithAdditionalArguments(GetAdditionalArguments()), output) { } + + private static string[] GetAdditionalArguments() => + new string[] { "--circuit-detailed-errors", "true" }; } public class ServerRoutingTest : RoutingTest diff --git a/src/Components/test/testassets/BasicTestApp/Index.razor b/src/Components/test/testassets/BasicTestApp/Index.razor index ca0d6bfa22..9aa1cdfa04 100644 --- a/src/Components/test/testassets/BasicTestApp/Index.razor +++ b/src/Components/test/testassets/BasicTestApp/Index.razor @@ -4,6 +4,7 @@