Blazor: send a local returnUrl from BlazorAuthenticationChallengeHandler (fix #3895)

ChallengeUser passed NavigationManager.Uri - always absolute - as returnUrl, but
the /login endpoint mapped by MapLoginAndLogout validates returnUrl with
RedirectUriHelper.IsLocalUrl, which rejects absolute URLs and falls back to '/'.
Every incremental-consent or Conditional Access round-trip therefore dropped the
user on the app root instead of returning them to their page.

Send new Uri(navigation.Uri).PathAndQuery instead: local, keeps the app path
base, drops the fragment - the same coercion AccountController.Challenge applies
to same-origin absolute URLs on the MVC path, applied here at the sending end so
the endpoint's hardened local-only validation stays exactly as pinned by its
regression tests. The coerced value is re-checked with IsLocalUrl (a path of
'//host/x' yields a protocol-relative PathAndQuery) and falls back to '/',
mirroring the MVC re-check.

Tests: BlazorAuthenticationChallengeHandlerTests gains a concrete
TestNavigationManager (Initialize + NavigateToCore override), three
returnUrl-shape tests (local path+query preserved, path base preserved,
protocol-relative path shape coerced to '/'), and the two HandleExceptionAsync
tests previously skipped as unmockable now run for real.

Author: DevonEast

src/Microsoft.Identity.Web/Blazor/BlazorAuthenticationChallengeHandler.cs

@@ -72,7 +72,22 @@ public async Task<bool> HandleExceptionAsync(Exception exception)
     /// </summary>
     public void ChallengeUser(ClaimsPrincipal user, string[]? scopes = null, string? claims = null)
     {
-        var currentUri = navigation.Uri;
+        // NavigationManager.Uri is always absolute, but the /login endpoint mapped by
+        // MapLoginAndLogout validates returnUrl with RedirectUriHelper.IsLocalUrl, which
+        // rejects absolute URLs and falls back to "/" — so passing the absolute URI loses
+        // the user's page after the consent round-trip. Send the app-local PathAndQuery
+        // instead (preserves any path base, drops the fragment — matching the MVC
+        // AccountController.Challenge coercion of same-origin absolute URLs).
+        //
+        // Defensive re-check: PathAndQuery can still begin with "//" or "/\" for a request
+        // path like "//evil.example/x", which a downstream Location header would treat as
+        // protocol-relative. Re-run IsLocalUrl on the coerced value and fall back to "/",
+        // mirroring the endpoint's own validation.
+        var returnUrl = new Uri(navigation.Uri).PathAndQuery;
+        if (!RedirectUriHelper.IsLocalUrl(returnUrl))
+        {
+            returnUrl = "/";
+        }
 
         // Build scopes string (add OIDC scopes)
         var allScopes = (scopes ?? [])
@@ -87,7 +102,7 @@ public void ChallengeUser(ClaimsPrincipal user, string[]? scopes = null, string?
         var domainHint = Uri.EscapeDataString(GetDomainHint(user));
 
         // Build the challenge URL
-        var challengeUrl = $"/authentication/login?returnUrl={Uri.EscapeDataString(currentUri)}" +
+        var challengeUrl = $"/authentication/login?returnUrl={Uri.EscapeDataString(returnUrl)}" +
                           $"&scope={scopeString}" +
                           $"&loginHint={loginHint}" +
                           $"&domainHint={domainHint}";

tests/Microsoft.Identity.Web.Test/Blazor/BlazorAuthenticationChallengeHandlerTests.cs

@@ -17,6 +17,29 @@ namespace Microsoft.Identity.Web.Test.Blazor
 {
     public class BlazorAuthenticationChallengeHandlerTests
     {
+        /// <summary>
+        /// Concrete <see cref="NavigationManager"/> for unit tests. <c>Uri</c> is driven by
+        /// <see cref="NavigationManager.Initialize(string, string)"/> and navigation is captured
+        /// by overriding <see cref="NavigationManager.NavigateToCore(string, NavigationOptions)"/> —
+        /// the same pattern the ASP.NET Core repo uses to test NavigationManager consumers.
+        /// </summary>
+        private sealed class TestNavigationManager : NavigationManager
+        {
+            public string? LastNavigatedTo { get; private set; }
+            public bool LastForceLoad { get; private set; }
+
+            public TestNavigationManager(string baseUri, string uri)
+            {
+                Initialize(baseUri, uri);
+            }
+
+            protected override void NavigateToCore(string uri, NavigationOptions options)
+            {
+                LastNavigatedTo = uri;
+                LastForceLoad = options.ForceLoad;
+            }
+        }
+
         private readonly NavigationManager _mockNavigationManager;
         private readonly AuthenticationStateProvider _mockAuthStateProvider;
         private readonly IConfiguration _configuration;
@@ -106,7 +129,7 @@ public async Task IsAuthenticatedAsync_ReturnsFalseForUnauthenticatedUser()
             Assert.False(isAuthenticated);
         }
 
-        [Fact(Skip = "NavigationManager.Uri and NavigationManager.NavigateTo cannot be mocked. Integration tests verify this behavior.")]
+        [Fact]
         public async Task HandleExceptionAsync_DetectsMicrosoftIdentityWebChallengeUserException()
         {
             // Arrange
@@ -119,23 +142,31 @@ public async Task HandleExceptionAsync_DetectsMicrosoftIdentityWebChallengeUserE
             var authState = new AuthenticationState(user);
             _mockAuthStateProvider.GetAuthenticationStateAsync().Returns(authState);
 
+            var navigation = new TestNavigationManager(
+                "https://app.contoso.com/",
+                "https://app.contoso.com/weather?day=2");
+
             var handler = new BlazorAuthenticationChallengeHandler(
-                _mockNavigationManager,
+                navigation,
                 _mockAuthStateProvider,
                 _configuration);
 
             var scopes = new[] { "user.read" };
             var msalException = new MsalUiRequiredException("error_code", "error_message");
             var challengeException = new MicrosoftIdentityWebChallengeUserException(msalException, scopes);
 
-            // Act & Assert
-            // Note: Since NavigationManager.NavigateTo is not virtual, actual navigation behavior
-            // is tested in integration tests. Here we verify exception detection logic.
+            // Act
             var handled = await handler.HandleExceptionAsync(challengeException);
+
+            // Assert
             Assert.True(handled);
+            Assert.NotNull(navigation.LastNavigatedTo);
+            Assert.True(navigation.LastForceLoad);
+            Assert.Contains("scope=", navigation.LastNavigatedTo, StringComparison.Ordinal);
+            Assert.Contains(Uri.EscapeDataString("user.read"), navigation.LastNavigatedTo, StringComparison.Ordinal);
         }
 
-        [Fact(Skip = "NavigationManager.Uri and NavigationManager.NavigateTo cannot be mocked. Integration tests verify this behavior.")]
+        [Fact]
         public async Task HandleExceptionAsync_DetectsMicrosoftIdentityWebChallengeUserExceptionAsInnerException()
         {
             // Arrange
@@ -148,8 +179,12 @@ public async Task HandleExceptionAsync_DetectsMicrosoftIdentityWebChallengeUserE
             var authState = new AuthenticationState(user);
             _mockAuthStateProvider.GetAuthenticationStateAsync().Returns(authState);
 
+            var navigation = new TestNavigationManager(
+                "https://app.contoso.com/",
+                "https://app.contoso.com/weather?day=2");
+
             var handler = new BlazorAuthenticationChallengeHandler(
-                _mockNavigationManager,
+                navigation,
                 _mockAuthStateProvider,
                 _configuration);
 
@@ -158,9 +193,96 @@ public async Task HandleExceptionAsync_DetectsMicrosoftIdentityWebChallengeUserE
             var challengeException = new MicrosoftIdentityWebChallengeUserException(msalException, scopes);
             var outerException = new InvalidOperationException("Outer exception", challengeException);
 
-            // Act & Assert
+            // Act
             var handled = await handler.HandleExceptionAsync(outerException);
+
+            // Assert
             Assert.True(handled);
+            Assert.NotNull(navigation.LastNavigatedTo);
+        }
+
+        // -----------------------------------------------------------------------------
+        // returnUrl shape (issue #3895): the /login endpoint mapped by MapLoginAndLogout
+        // validates returnUrl with RedirectUriHelper.IsLocalUrl, which rejects absolute
+        // URLs and falls back to "/". ChallengeUser must therefore send the app-local
+        // PathAndQuery of the current page — not NavigationManager.Uri verbatim — or the
+        // user loses their page after the consent round-trip.
+        // -----------------------------------------------------------------------------
+
+        [Fact]
+        public void ChallengeUser_SendsLocalReturnUrl_PreservingPathAndQuery()
+        {
+            // Arrange
+            var navigation = new TestNavigationManager(
+                "https://app.contoso.com/",
+                "https://app.contoso.com/admin/reports?tab=2");
+
+            var handler = new BlazorAuthenticationChallengeHandler(
+                navigation,
+                _mockAuthStateProvider,
+                _configuration);
+
+            // Act
+            handler.ChallengeUser(new ClaimsPrincipal(new CaseSensitiveClaimsIdentity()), new[] { "user.read" });
+
+            // Assert
+            Assert.NotNull(navigation.LastNavigatedTo);
+            Assert.StartsWith(
+                $"/authentication/login?returnUrl={Uri.EscapeDataString("/admin/reports?tab=2")}",
+                navigation.LastNavigatedTo,
+                StringComparison.Ordinal);
+            Assert.True(navigation.LastForceLoad);
+        }
+
+        [Fact]
+        public void ChallengeUser_LocalReturnUrl_PreservesPathBase()
+        {
+            // Arrange — app hosted under a path base ("/app"). PathAndQuery keeps it;
+            // NavigationManager.ToBaseRelativePath would lose it.
+            var navigation = new TestNavigationManager(
+                "https://host.contoso.com/app/",
+                "https://host.contoso.com/app/page?x=1");
+
+            var handler = new BlazorAuthenticationChallengeHandler(
+                navigation,
+                _mockAuthStateProvider,
+                _configuration);
+
+            // Act
+            handler.ChallengeUser(new ClaimsPrincipal(new CaseSensitiveClaimsIdentity()));
+
+            // Assert
+            Assert.NotNull(navigation.LastNavigatedTo);
+            Assert.StartsWith(
+                $"/authentication/login?returnUrl={Uri.EscapeDataString("/app/page?x=1")}",
+                navigation.LastNavigatedTo,
+                StringComparison.Ordinal);
+        }
+
+        [Fact]
+        public void ChallengeUser_ProtocolRelativePathShape_CoercedToRoot()
+        {
+            // Arrange — PathAndQuery of "https://host//evil.example/x" is "//evil.example/x":
+            // a protocol-relative shape that a downstream Location header would follow off-origin.
+            // The handler must re-check IsLocalUrl on the coerced value and fall back to "/".
+            var navigation = new TestNavigationManager(
+                "https://host.contoso.com/",
+                "https://host.contoso.com//evil.example/x");
+
+            var handler = new BlazorAuthenticationChallengeHandler(
+                navigation,
+                _mockAuthStateProvider,
+                _configuration);
+
+            // Act
+            handler.ChallengeUser(new ClaimsPrincipal(new CaseSensitiveClaimsIdentity()));
+
+            // Assert
+            Assert.NotNull(navigation.LastNavigatedTo);
+            Assert.StartsWith(
+                $"/authentication/login?returnUrl={Uri.EscapeDataString("/")}",
+                navigation.LastNavigatedTo,
+                StringComparison.Ordinal);
         }
 
         [Fact]
@@ -185,9 +307,10 @@ public async Task HandleExceptionAsync_ReturnsFalseForNonChallengeException()
             Assert.False(handled);
         }
 
-        // Note: Additional tests for ChallengeUser, GetLoginHint, and GetDomainHint
-        // behavior are covered in integration tests since NavigationManager.NavigateTo()
-        // and NavigationManager.Uri are not virtual and cannot be mocked.
-        // These tests validate URL construction and parameter passing through real Blazor components.
+        // Note: NavigationManager.Uri and NavigateTo ARE unit-testable via a concrete
+        // subclass that calls Initialize() and overrides NavigateToCore (see
+        // TestNavigationManager above) — the same pattern the ASP.NET Core repo uses.
+        // End-to-end URL construction through real Blazor components remains covered
+        // by integration tests.
     }
 }