Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make enhanced nav much more conservative, and better handle redirections #50551

Merged
merged 30 commits into from
Sep 13, 2023

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Sep 6, 2023

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.
    • 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.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Sep 6, 2023
@SteveSandersonMS SteveSandersonMS changed the title Stevesa/enhanced navigations Enhanced navigation redirections. Make form enhancement opt-in. Sep 6, 2023
@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review September 11, 2023 09:36
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner September 11, 2023 09:36
@SteveSandersonMS SteveSandersonMS changed the title Enhanced navigation redirections. Make form enhancement opt-in. Make enhanced nav much more conservative, and better handle redirections Sep 11, 2023
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/enhanced-navigations branch from ca01d7c to ac96696 Compare September 11, 2023 12:30
// Matches MVC's MemoryPoolHttpResponseStreamWriterFactory.DefaultBufferSize
var defaultBufferSize = 16 * 1024;
await using var writer = new HttpResponseStreamWriter(httpContext.Response.Body, Encoding.UTF8, defaultBufferSize, ArrayPool<byte>.Shared, ArrayPool<char>.Shared);
using var bufferWriter = new BufferedTextWriter(writer);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is separate from the rest of the PR, but I realised that RazorComponentResultExecutor is inconsistent with RazorComponentEndpointInvoker.

It's dangerous that we have two different implementations as they could be out of sync and not support the same features. And here in fact one of them was not using the new buffering solution. However I don't want to unpick all the complexities of unifying these code paths within this PR (and I think we likely should leave that for .NET 9), so I'm just changing RazorComponentResultExecutor to use the same buffering technique as RazorComponentEndpointInvoker. Longer term, we should unify the code.

Note that fixing this forced me to make some other unit test updates (see WaitForContentWrittenAsync in this PR), as the unit tests were relying on content writes being completely synchronous.

public static string CreateProtectedRedirectionUrl(HttpContext httpContext, string destinationUrl)
{
var protector = CreateProtector(httpContext);
var protectedUrl = protector.Protect(destinationUrl, TimeSpan.FromSeconds(10));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time needs to be configurable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Especially happy with the thoroughness of the E2E tests

@Bartmax
Copy link
Contributor

Bartmax commented Sep 23, 2023

This looks like too much magic for something that should have been be a simple data-interactive=false (or enhance-nav=false.)

Is there anyway to prevent enhance nav on rc1?
Is there anyway to configure this behavior (like on for everything [post/get]) or off for everything?

@ghost
Copy link

ghost commented Sep 23, 2023

Hi @Bartmax. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@SteveSandersonMS
Copy link
Member Author

Is there anyway to prevent enhance nav on rc1?

Yes, just set it to false at the root level (e.g., on your <html> element).

For POST requests it's always set on a per-form basis (defaulting to off) because it would be unsafe to assume that all forms in general should use enhanced posts (e.g., they may not even target a Blazor endpoint).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants