From 0005026d4eca7d681976b5d1967f6de86538cca3 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 13 Sep 2023 16:30:34 +0100 Subject: [PATCH] Make enhanced nav much more conservative, and better handle redirections (#50551) This PR makes enhanced navigation/forms **much** more conservative: * For links, enhanced nav remains on by default, but: * It's now easy to turn it off or back on hierarchically or on a per-link basis using a `data-enhance-nav` attribute in HTML. * If the destination turns out to be a non-Blazor endpoint, enhanced nav will not apply, and the client-side JS will retry as a full page load. This is to ensure we don't confuse other pages that aren't designed to be patched into an existing page alongside the Blazor JS logic. * For posts, enhanced nav is now off by default * You turn it on by adding `data-enhance` to the form element or `Enhance` if it's an `EditForm` (they're equivalent). This is nonhierarchical since it's not desirable to enable this globally. * If the destination turns out to be a non-Blazor endpoint, enhanced nav considers this an error (same reason as for links above). But unlike with links, form posts *won't* retry since it's not safe to do that for POST requests. Developers shouldn't enhance forms that post to non-Blazor endpoints. * Redirection handling is much more comprehensive * This is way more tricky than I expected. Allowing for combinations of GET/POST, enhanced nav, streaming, Blazor and non-Blazor endpoints, internal and external redirection destinations turns out to break down into 16 separate cases, which you can find [listed in this E2E page](https://github.com/dotnet/aspnetcore/pull/50551/files#diff-477ec16959ae9dddaca3e76e1e40c4d70147210d0fde068357d9ad3e74896337). * 15 of those 16 cases now work automatically without errors. The remaining case is "form with enhanced nav does POST to a non-Blazor endpoint which redirects to an external URL", which cannot be supported without having some global server-side logic for all endpoints (not just Blazor ones), so that case explicitly throws a clear error telling the developer not to apply enhanced nav to forms that go to non-Blazor endpoints that redirect to external URLs. * There are various subtleties about making the "Back" button work correctly after these enhanced redirections, but I think all of them are covered properly now. The E2E cases all validate clicking "Back" afterwards. * Previously it was possible for an internal URL to redirect to an external URL that enabled CORS for your site, and then we would have received and merged that external content into your page. That's no longer the case since we now put `no-cors` on the fetch request, so it should now be impossible to receive content from an external origin (and it falls back on retrying as a full-page load for GET, or an error with a clear message for POST). * Disclosure of redirection URLs is now avoided. * Previously, for external or streaming redirections, we told the client-side JS code which URL to redirect to. That disclosed more info than a `fetch` normally would. As of this PR, the URL is data protected so all the client can do is navigate to an endpoint that issues a real 302 to the real URL. As such JS no longer has access to any more info than it normally would with a `fetch`. With all this, it should be much less likely for enhanced nav/redirections to run into incompatibility issues. And much easier for developers to disable it for whole regions in the UI if they want. --- .../src/Builder/OpaqueRedirection.cs | 94 ++++++++ ...omponentsEndpointRouteBuilderExtensions.cs | 1 + .../RazorComponentsServiceOptions.cs | 19 ++ .../Endpoints/src/PublicAPI.Unshipped.txt | 2 + .../src/RazorComponentEndpointInvoker.cs | 1 + .../EndpointHtmlRenderer.Prerendering.cs | 19 +- .../EndpointHtmlRenderer.Streaming.cs | 30 ++- .../Results/RazorComponentResultExecutor.cs | 22 +- .../test/RazorComponentResultTest.cs | 43 +++- .../Web.JS/dist/Release/blazor.web.js | 2 +- .../src/Rendering/StreamingRendering.ts | 23 +- .../src/Services/NavigationEnhancement.ts | 110 +++++++-- src/Components/Web/src/Forms/EditForm.cs | 22 +- .../Web/src/PublicAPI.Unshipped.txt | 2 + .../EnhancedNavigationTest.cs | 96 +++----- .../EnhancedNavigationTestUtil.cs | 4 + .../FormWithParentBindingContextTest.cs | 32 ++- .../ServerRenderingTests/RedirectionTest.cs | 224 ++++++++++++++++++ .../RazorComponentEndpointsStartup.cs | 23 ++ .../ComponentWithFormBoundParameter.razor | 2 +- .../Components/ComponentWithFormInside.razor | 2 +- .../Pages/EnhancedNav/Other.razor | 7 + .../Pages/EnhancedNav/PageThatRedirects.razor | 10 - .../PageThatRedirectsWhileStreaming.razor | 13 - .../Pages/Forms/AmbiguousForms.razor | 4 +- .../Pages/Forms/ComponentThatThrows.razor | 2 +- .../Pages/Forms/DefaultForm.razor | 2 +- .../DefaultFormBoundCollectionParameter.razor | 4 +- ...DefaultFormBoundComplexTypeParameter.razor | 2 +- ...mplexTypeParameterMultipleComponents.razor | 2 +- .../DefaultFormBoundDictionaryParameter.razor | 2 +- ...ltFormBoundDictionaryParameterErrors.razor | 2 +- ...FormBoundMultiplePrimitiveParameters.razor | 2 +- ...tiplePrimitiveParametersChangedNames.razor | 2 +- .../Forms/DefaultFormBoundParameter.razor | 2 +- ...DefaultFormBoundParameterAnnotations.razor | 2 +- .../DefaultFormBoundPrimitiveParameter.razor | 4 +- .../Forms/DefaultFormMaxCollectionLimit.razor | 2 +- .../Forms/DefaultFormMaxRecursionDepth.razor | 2 +- .../DefaultFormWithBodyOnInitialized.razor | 2 +- .../Pages/Forms/DisappearingForm.razor | 2 +- .../FormAntiforgeryAfterResponseStarted.razor | 2 +- .../Pages/Forms/FormDisableAntiforgery.razor | 2 +- .../Pages/Forms/FormNoAntiforgery.razor | 2 +- .../Pages/Forms/FormNoHandler.razor | 2 +- .../Pages/Forms/FormThatBindsGuid.razor | 2 +- .../Pages/Forms/FormThatBindsInteger.razor | 2 +- .../Pages/Forms/ModifyHttpContextForm.razor | 2 +- .../Pages/Forms/MutateAndReRenderChild.razor | 2 +- .../Pages/Forms/NamedForm.razor | 2 +- .../Pages/Forms/NamedFormBoundParameter.razor | 2 +- .../NamedFormBoundPrimitiveParameter.razor | 2 +- ...imitiveParameterValidatorIntegration.razor | 2 +- .../NamedFormContextNoFormContextLayout.razor | 2 +- .../Pages/Forms/NestedNamedForm.razor | 2 +- .../Pages/Forms/NonEnhancedEditForm.razor | 17 ++ .../Pages/Forms/NonEnhancedPlainForm.razor | 18 ++ .../Forms/NonStreamingRenderingForm.razor | 2 +- .../Pages/Forms/PlainForm.razor | 2 +- .../Pages/Forms/PostRedirectGet.razor | 4 +- .../Forms/PostRedirectGetStreaming.razor | 4 +- .../Pages/Forms/StreamingRenderingForm.razor | 2 +- .../Pages/Redirections/RedirectGet.razor | 15 ++ .../Pages/Redirections/RedirectHome.razor | 92 +++++++ .../Pages/Redirections/RedirectPost.razor | 17 ++ .../Redirections/RedirectStreamingGet.razor | 16 ++ .../Redirections/RedirectStreamingPost.razor | 18 ++ .../Pages/Redirections/_Imports.razor | 1 + .../Shared/EnhancedNavLayout.razor | 11 +- 69 files changed, 899 insertions(+), 191 deletions(-) create mode 100644 src/Components/Endpoints/src/Builder/OpaqueRedirection.cs create mode 100644 src/Components/test/E2ETest/ServerRenderingTests/RedirectionTest.cs create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/EnhancedNav/Other.razor delete mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/EnhancedNav/PageThatRedirects.razor delete mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/EnhancedNav/PageThatRedirectsWhileStreaming.razor create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Forms/NonEnhancedEditForm.razor create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Forms/NonEnhancedPlainForm.razor create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Redirections/RedirectGet.razor create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Redirections/RedirectHome.razor create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Redirections/RedirectPost.razor create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Redirections/RedirectStreamingGet.razor create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Redirections/RedirectStreamingPost.razor create mode 100644 src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/Redirections/_Imports.razor diff --git a/src/Components/Endpoints/src/Builder/OpaqueRedirection.cs b/src/Components/Endpoints/src/Builder/OpaqueRedirection.cs new file mode 100644 index 000000000000..38012c6d01d2 --- /dev/null +++ b/src/Components/Endpoints/src/Builder/OpaqueRedirection.cs @@ -0,0 +1,94 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Security.Cryptography; +using System.Text.Encodings.Web; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.DataProtection; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; + +namespace Microsoft.AspNetCore.Components.Endpoints; + +internal partial class OpaqueRedirection +{ + // During streaming SSR, a component may try to perform a redirection. Since the response has already started + // this can only work if we communicate the redirection back via some command that can get handled by JS, + // rather than a true 301/302/etc. But we don't want to disclose the redirection target URL to JS because that + // info would not normally be available, e.g., when using 'fetch'. So we data-protect the URL and round trip + // through a special endpoint that can issue a true redirection. + // + // The same is used during enhanced navigation if it happens to go to a Blazor endpoint that calls + // NavigationManager.NavigateTo, for the same reasons. + // + // However, if enhanced navigation goes to a non-Blazor endpoint, the server won't do anything special and just + // returns a regular 301/302/etc. To handle this, + // + // - If it's redirected to an internal URL, the browser will just follow the redirection automatically + // and client-side code will then: + // - Check if it went to a Blazor endpoint, and if so, simply update the client-side URL to match + // - Or if it's a non-Blazor endpoint, behaves like "external URL" below + // - If it's to an external URL: + // - If it's a GET request, the client-side code will retry as a non-enhanced request + // - For other request types, we have to let it fail as it would be unsafe to retry + + private const string RedirectionDataProtectionProviderPurpose = "Microsoft.AspNetCore.Components.Endpoints.OpaqueRedirection,V1"; + private const string RedirectionEndpointBaseRelativeUrl = "_framework/opaque-redirect"; + + public static string CreateProtectedRedirectionUrl(HttpContext httpContext, string destinationUrl) + { + var protector = CreateProtector(httpContext); + var options = httpContext.RequestServices.GetRequiredService>(); + var lifetime = options.Value.TemporaryRedirectionUrlValidityDuration; + var protectedUrl = protector.Protect(destinationUrl, lifetime); + return $"{RedirectionEndpointBaseRelativeUrl}?url={UrlEncoder.Default.Encode(protectedUrl)}"; + } + + public static void AddBlazorOpaqueRedirectionEndpoint(IEndpointRouteBuilder endpoints) + { + endpoints.MapGet($"/{RedirectionEndpointBaseRelativeUrl}", httpContext => + { + if (!httpContext.Request.Query.TryGetValue("url", out var protectedUrl)) + { + httpContext.Response.StatusCode = 400; + return Task.CompletedTask; + } + + var protector = CreateProtector(httpContext); + string url; + + try + { + url = protector.Unprotect(protectedUrl[0]!); + } + catch (CryptographicException ex) + { + if (httpContext.RequestServices.GetService>() is { } logger) + { + Log.OpaqueUrlUnprotectionFailed(logger, ex); + } + + httpContext.Response.StatusCode = 400; + return Task.CompletedTask; + } + + httpContext.Response.Redirect(url); + return Task.CompletedTask; + }); + } + + private static ITimeLimitedDataProtector CreateProtector(HttpContext httpContext) + { + var dataProtectionProvider = httpContext.RequestServices.GetRequiredService(); + return dataProtectionProvider.CreateProtector(RedirectionDataProtectionProviderPurpose).ToTimeLimitedDataProtector(); + } + + public static partial class Log + { + [LoggerMessage(1, LogLevel.Information, "Opaque URL unprotection failed.", EventName = "OpaqueUrlUnprotectionFailed")] + public static partial void OpaqueUrlUnprotectionFailed(ILogger logger, Exception exception); + } +} diff --git a/src/Components/Endpoints/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs b/src/Components/Endpoints/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs index 91df1b731e8c..d8cfbb5b0d2e 100644 --- a/src/Components/Endpoints/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs +++ b/src/Components/Endpoints/src/Builder/RazorComponentsEndpointRouteBuilderExtensions.cs @@ -31,6 +31,7 @@ public static class RazorComponentsEndpointRouteBuilderExtensions EnsureRazorComponentServices(endpoints); AddBlazorWebJsEndpoint(endpoints); + OpaqueRedirection.AddBlazorOpaqueRedirectionEndpoint(endpoints); return GetOrCreateDataSource(endpoints).DefaultBuilder; } diff --git a/src/Components/Endpoints/src/DependencyInjection/RazorComponentsServiceOptions.cs b/src/Components/Endpoints/src/DependencyInjection/RazorComponentsServiceOptions.cs index 7d0fda142c1d..b2f8497bd9f8 100644 --- a/src/Components/Endpoints/src/DependencyInjection/RazorComponentsServiceOptions.cs +++ b/src/Components/Endpoints/src/DependencyInjection/RazorComponentsServiceOptions.cs @@ -10,6 +10,9 @@ namespace Microsoft.AspNetCore.Components.Endpoints; /// public sealed class RazorComponentsServiceOptions { + // Fairly long default lifetime to allow for clock skew across servers + private TimeSpan _temporaryRedirectionUrlValidityDuration = TimeSpan.FromMinutes(5); + internal FormDataMapperOptions _formMappingOptions = new(); /// @@ -63,4 +66,20 @@ public int MaxFormMappingKeySize get => _formMappingOptions.MaxKeyBufferSize; set => _formMappingOptions.MaxKeyBufferSize = value; } + + /// + /// Gets or sets the lifetime of data protection validity for temporary redirection URLs + /// emitted by Blazor server-side rendering. These are only used transiently so the lifetime + /// only needs to be long enough for a client to receive the URL and begin navigation to it. + /// However, it should also be long enough to allow for clock skew across servers. + /// + public TimeSpan TemporaryRedirectionUrlValidityDuration + { + get => _temporaryRedirectionUrlValidityDuration; + set + { + ArgumentOutOfRangeException.ThrowIfLessThanOrEqual(value.TotalMilliseconds, 0); + _temporaryRedirectionUrlValidityDuration = value; + } + } } diff --git a/src/Components/Endpoints/src/PublicAPI.Unshipped.txt b/src/Components/Endpoints/src/PublicAPI.Unshipped.txt index ff9698692651..1e978ea3911a 100644 --- a/src/Components/Endpoints/src/PublicAPI.Unshipped.txt +++ b/src/Components/Endpoints/src/PublicAPI.Unshipped.txt @@ -29,6 +29,8 @@ Microsoft.AspNetCore.Components.Endpoints.RazorComponentsServiceOptions.MaxFormM Microsoft.AspNetCore.Components.Endpoints.RazorComponentsServiceOptions.MaxFormMappingRecursionDepth.get -> int Microsoft.AspNetCore.Components.Endpoints.RazorComponentsServiceOptions.MaxFormMappingRecursionDepth.set -> void Microsoft.AspNetCore.Components.Endpoints.RazorComponentsServiceOptions.RazorComponentsServiceOptions() -> void +Microsoft.AspNetCore.Components.Endpoints.RazorComponentsServiceOptions.TemporaryRedirectionUrlValidityDuration.get -> System.TimeSpan +Microsoft.AspNetCore.Components.Endpoints.RazorComponentsServiceOptions.TemporaryRedirectionUrlValidityDuration.set -> void Microsoft.AspNetCore.Components.Endpoints.RootComponentMetadata Microsoft.AspNetCore.Components.Endpoints.RootComponentMetadata.RootComponentMetadata(System.Type! rootComponentType) -> void Microsoft.AspNetCore.Components.Endpoints.RootComponentMetadata.Type.get -> System.Type! diff --git a/src/Components/Endpoints/src/RazorComponentEndpointInvoker.cs b/src/Components/Endpoints/src/RazorComponentEndpointInvoker.cs index 51eb2d5c551e..7dafc88bafab 100644 --- a/src/Components/Endpoints/src/RazorComponentEndpointInvoker.cs +++ b/src/Components/Endpoints/src/RazorComponentEndpointInvoker.cs @@ -35,6 +35,7 @@ private async Task RenderComponentCore(HttpContext context) { context.Response.ContentType = RazorComponentResultExecutor.DefaultContentType; _renderer.InitializeStreamingRenderingFraming(context); + EndpointHtmlRenderer.MarkAsAllowingEnhancedNavigation(context); var endpoint = context.GetEndpoint() ?? throw new InvalidOperationException($"An endpoint must be set on the '{nameof(HttpContext)}'."); diff --git a/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Prerendering.cs b/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Prerendering.cs index 9a8663ec5098..3b192d99552b 100644 --- a/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Prerendering.cs +++ b/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Prerendering.cs @@ -50,6 +50,11 @@ protected override IComponent ResolveComponentForRenderMode([DynamicallyAccessed return null; } + public static void MarkAsAllowingEnhancedNavigation(HttpContext context) + { + context.Response.Headers.Add("blazor-enhanced-nav", "allow"); + } + public ValueTask PrerenderComponentAsync( HttpContext httpContext, [DynamicallyAccessedMembers(Component)] Type componentType, @@ -149,13 +154,15 @@ public static ValueTask HandleNavigationExcepti "Navigation commands can not be issued during server-side prerendering after the response from the server has started. Applications must buffer the" + "response and avoid using features like FlushAsync() before all components on the page have been rendered to prevent failed navigation commands."); } - else if (IsPossibleExternalDestination(httpContext.Request, navigationException.Location) && httpContext.Request.Headers.ContainsKey("blazor-enhanced-nav")) + else if (IsPossibleExternalDestination(httpContext.Request, navigationException.Location) + && IsProgressivelyEnhancedNavigation(httpContext.Request)) { - // It's unsafe to do a 301/302/etc to an external destination when this was requested via fetch, because - // assuming it doesn't expose CORS headers, we won't be allowed to follow the redirection nor will - // we even find out what the destination URL would have been. But since it's our own JS code making this - // fetch request, we can have a custom protocol for describing the URL we wanted to redirect to. - httpContext.Response.Headers.Add("blazor-enhanced-nav-redirect-location", navigationException.Location); + // For progressively-enhanced nav, we prefer to use opaque redirections for external URLs rather than + // forcing the request to be retried, since that allows post-redirect-get to work, plus avoids a + // duplicated request. The client can't rely on receiving this header, though, since non-Blazor endpoints + // wouldn't return it. + httpContext.Response.Headers.Add("blazor-enhanced-nav-redirect-location", + OpaqueRedirection.CreateProtectedRedirectionUrl(httpContext, navigationException.Location)); return new ValueTask(PrerenderedComponentHtmlContent.Empty); } else diff --git a/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs b/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs index 0b96f6549c7c..5a6d11e5b98d 100644 --- a/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs +++ b/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs @@ -15,7 +15,6 @@ namespace Microsoft.AspNetCore.Components.Endpoints; internal partial class EndpointHtmlRenderer { - private const string _progressivelyEnhancedNavRequestHeaderName = "blazor-enhanced-nav"; private const string _streamingRenderingFramingHeaderName = "ssr-framing"; private TextWriter? _streamingUpdatesWriter; private HashSet? _visitedComponentIdsInCurrentStreamingBatch; @@ -23,7 +22,7 @@ internal partial class EndpointHtmlRenderer public void InitializeStreamingRenderingFraming(HttpContext httpContext) { - if (httpContext.Request.Headers.ContainsKey(_progressivelyEnhancedNavRequestHeaderName)) + if (IsProgressivelyEnhancedNavigation(httpContext.Request)) { var id = Guid.NewGuid().ToString(); httpContext.Response.Headers.Add(_streamingRenderingFramingHeaderName, id); @@ -60,7 +59,7 @@ public async Task SendStreamingUpdatesAsync(HttpContext httpContext, Task untilT } catch (NavigationException navigationException) { - HandleNavigationAfterResponseStarted(writer, navigationException.Location); + HandleNavigationAfterResponseStarted(writer, httpContext, navigationException.Location); } catch (Exception ex) { @@ -176,10 +175,22 @@ private static void HandleExceptionAfterResponseStarted(HttpContext httpContext, writer.Write(""); } - private static void HandleNavigationAfterResponseStarted(TextWriter writer, string destinationUrl) + private static void HandleNavigationAfterResponseStarted(TextWriter writer, HttpContext httpContext, string destinationUrl) { - writer.Write("