dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.22k stars 9.95k forks source link

Blazor WASM: RemoteAuthenticatorViewCore prevents use of ApplicationPaths.RemoteRegisterPath #25153

Open hallidev opened 4 years ago

hallidev commented 4 years ago

I am building a Blazor WASM application that's connected to IdentityServer4.

The template for Single User Accounts provides LoginDisplay.razor which includes the following line:

<a href="authentication/register">Register</a>

After getting IdentityServer4 integrated via the following article, I noticed that the Register link simply redirected back to the Blazor app without any error messages.

https://docs.microsoft.com/en-us/aspnet/core/blazor/security/webassembly/hosted-with-identity-server?view=aspnetcore-3.1&tabs=visual-studio

It wasn't clear what the RemoteAuthenticatorView was doing, so I dug into the code and came across the following:

https://github.com/dotnet/aspnetcore/blob/c21841f52e066400aae85a776f7b901fdca904e3/src/Components/WebAssembly/WebAssembly.Authentication/src/RemoteAuthenticatorViewCore.cs#L356

It's clear that ApplicationPaths.RemoteRegisterPath is being used for the redirect, but there is no documentation describing this. After setting the value to my external identity site https://localhost:44312/Identity/Account/Register (different site entirely), I was presented with Sorry, there's nothing at this address. in the Blazor app.

Looking closer at the above line of code, the .PathAndQuery piece at the end prevents ApplicationPaths.RemoteRegisterPath from ever being outside the Blazor app.

As a result, setting ApplicationPaths.RemoteRegisterPath to https://localhost:44312/Identity/Account/Register executes a redirect to https://localhost:44357/Identity/Account/Register (my Blazor app).

Please confirm that this is a bug.

Edit: ApplicationPaths.RemoteProfilePath seems to have the same issue.

ghost commented 4 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

killswtch commented 3 years ago

The documentation for ApplicationPaths.RemoteRegisterPath states that "It might be absolute and point outside of the application." however as @hallidev has pointed out, the code for the redirection would prevent it from ever being outside of the application.

It would seem to be a bug, not an enhancement as labelled. It looks like someone may have been a bit overzealous during a security review. That line should probably be:

var registerUrl = $"{ApplicationPaths.RemoteRegisterPath}?returnUrl={Uri.EscapeDataString(loginUrl)}";

And for RedirectToProfile:

private ValueTask RedirectToProfile() => JS.InvokeVoidAsync("location.replace", ApplicationPaths.RemoteProfilePath);

hallidev commented 3 years ago

@killswtch I love how this bug is considered so low priority or such an ordeal to fix that 5 months later it has seen no movement. I bailed on Blazor entirely due to issues like this and am actively encouraging others to bail. I’ll check back on progress in a year or two

IvanFeric commented 3 years ago

I have this same issue. As a workaround, I've temporarily created a custom component that inherits RemoteAuthenticatorView and changed how it handles profile and register redirection. After that, I just used that component instead of <RemoteAuthenticatorView>.

Here's the code in case anybody needs it:

public class CustomRemoteAuthenticatorView : RemoteAuthenticatorView
{
  [Inject] internal IJSRuntime Js { get; set; } = null!;
  [Inject] internal NavigationManager Navigation { get; set; } = null!;

  protected override async Task OnParametersSetAsync()
  {
    if (Action == RemoteAuthenticationActions.Profile && ApplicationPaths.RemoteProfilePath != null)
    {
      UserProfile ??= LoggingIn;
      await RedirectToProfile();
    }
    else if (Action == RemoteAuthenticationActions.Register && ApplicationPaths.RemoteRegisterPath != null)
    {
      Registering ??= LoggingIn;
      await RedirectToRegister();
    }
    else
    {
      await base.OnParametersSetAsync();
    }
  }

  private ValueTask RedirectToRegister()
  {
    var loginUrl = Navigation.ToAbsoluteUri(ApplicationPaths.LogInPath).PathAndQuery;
    var registerUrl = Navigation.ToAbsoluteUri($"{ApplicationPaths.RemoteRegisterPath}?returnUrl={Uri.EscapeDataString(loginUrl)}");

    return Js.InvokeVoidAsync("location.replace", registerUrl);
  }

  private ValueTask RedirectToProfile() => Js.InvokeVoidAsync("location.replace", Navigation.ToAbsoluteUri(ApplicationPaths.RemoteProfilePath));
}
ghost commented 1 year ago

Thanks for contacting us. We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 11 months ago

To learn more about what this message means, what to expect next, and how this issue will be handled you can read our Triage Process document. We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. Because it's not immediately obvious what is causing this behavior, we would like to keep this around to collect more feedback, which can later help us determine how to handle this. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact work.

ghost commented 9 months ago

To learn more about what this message means, what to expect next, and how this issue will be handled you can read our Triage Process document. We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. Because it's not immediately obvious what is causing this behavior, we would like to keep this around to collect more feedback, which can later help us determine how to handle this. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact work.