diff --git a/changelog.md b/changelog.md index a0b35926a..6b41e73f8 100644 --- a/changelog.md +++ b/changelog.md @@ -8,6 +8,25 @@ - Fix race condition in `MergedOptions` causing sporadic "No ClientId was specified" errors under concurrent `GraphServiceClient` usage. See [#3760](https://github.com/AzureAD/microsoft-identity-web/pull/3760). - Fix `CredentialsProvider` DI lifetime mismatch causing startup crash in Development mode when using `AddMicrosoftIdentityWebApi()`. See [#3783](https://github.com/AzureAD/microsoft-identity-web/pull/3783). +### Behavior changes +- **`/MicrosoftIdentity/Account/Challenge` — redirect URI validation.** The `redirectUri` query-string parameter is now validated. Accepted values: + - Local paths (e.g. `/home`, `/counter?tab=1`) — unchanged behavior. + - Same-origin absolute URLs (matching scheme, host, and effective port of the current request). These are coerced to their path-and-query before being stored in `AuthenticationProperties.RedirectUri`. This preserves the canonical `[AuthorizeForScopes]` / `MsalUiRequiredException` step-up consent flow, which goes through `MicrosoftIdentityConsentAndConditionalAccessHandler.ChallengeUser()` and passes `NavigationManager.Uri` (always absolute) for Blazor Server, or an absolute request URL for Razor Pages / MVC. + - Any other value (external host, different scheme, different port, protocol-relative `//host`, empty, or `null`) falls back to `~/`. + - **UX note:** URL fragments (`#section`) are dropped when a same-origin absolute URL is coerced. If a Blazor Server page depends on a fragment being preserved across step-up consent, pass a relative path explicitly rather than relying on `NavigationManager.Uri`. + - **Reverse-proxy deployments:** apps behind a reverse proxy (Azure App Service, Container Apps, AKS ingress, nginx, etc.) should configure `app.UseForwardedHeaders(new ForwardedHeadersOptions { ForwardedHeaders = ForwardedHeaders.XForwardedProto | ForwardedHeaders.XForwardedHost })` before `UseAuthentication()`. Without it, `Request.Scheme` / `Request.Host` reflect the internal container/pod hostname, the same-origin check fails for the external `NavigationManager.Uri`, and step-up lands the user on `/` rather than the original page. + +- **Blazor `MapGroup(...).MapLoginAndLogout()` — `/logout` endpoint.** The generated `POST /logout` endpoint now (a) requires authentication (`RequireAuthorization()`) and (b) requires an antiforgery token (the previous `DisableAntiforgery()` opt-out has been removed). UX and integration implications: + - Forms rendered by `SignOutLink` / `LogInOrOut` already include the antiforgery token and continue to work without code changes. + - Custom clients that `fetch`/`XMLHttpRequest` to `/logout` must now include the antiforgery token. Obtain it via `IAntiforgery.GetAndStoreTokens(context).RequestToken` and send it in the request header configured by `AddAntiforgery(options => options.HeaderName = "...")` (default `RequestVerificationToken`) or as the configured form field. + - The `ReturnUrl` form value is now treated as strictly local: any non-local value (absolute URL, protocol-relative, `/\host`, etc.) is coerced to `/`. Apps that previously passed an absolute URL should switch to a relative path. + - **Edge case:** a user whose authentication cookie has already expired and who then clicks the logout button will be redirected to the login page (because of `RequireAuthorization()`) rather than seeing a silent no-op. This is a minor change from previous behavior; the happy path (authenticated user clicking logout) is unchanged. + - **Blazor WebAssembly clients are unaffected.** WASM apps sign out through `Microsoft.Authentication.WebAssembly.Msal` / the `/authentication/logout` JS interop path, not by POSTing to the server-side `/logout` endpoint, so no client code changes are needed. + - **Server-side Blazor using `AuthenticationStateProvider` / `SignOutAsync` is unaffected.** The new gate only applies to direct HTTP POSTs to `/logout`. Components that call `AuthenticationStateProvider.GetAuthenticationStateAsync()` or sign out through the scheme handler continue to work unchanged. + - **Graceful degradation when antiforgery is not configured.** The check is performed inside the endpoint handler by explicitly resolving `IAntiforgery` from request services (not via endpoint metadata / middleware coupling). This means the `/logout` endpoint works correctly on every pipeline shape: (a) minimal API with both `AddAntiforgery()` and `UseAntiforgery()` wired — token is validated by middleware and re-checked by the handler (idempotent); (b) MVC / Razor Pages hosts that call `AddControllersWithViews()` or `AddRazorPages()` (which transitively register `IAntiforgery`) but do not call `UseAntiforgery()` — the handler validates the token directly; (c) hosts that reuse `MapLoginAndLogout` without any antiforgery configuration — the handler skips validation and `RequireAuthorization()` + cookie `SameSite=Lax` remain the CSRF gate, matching pre-4.8.0 behavior. For scenario (c), a single warning is logged at endpoint map time recommending that `AddAntiforgery()` be configured. + +- **`/MicrosoftIdentity/Account/Challenge` — `%2f` / `%5c` defense-in-depth.** In addition to the path-and-query re-check for protocol-relative shapes (`//host`, `/\host`), `redirectUri` values whose path begins with `/%2f`, `/%5c`, `/%2F`, or `/%5C` are now rejected and coerced to `~/`. Browsers per RFC 3986 treat these as literal path characters (a direct hit yields a 404), so this change does not affect legitimate deep-links. It guards against misconfigured reverse proxies (NGINX, IIS ARR, F5) that can decode `%2f` → `/` while rewriting `Location` headers, which would otherwise reopen the protocol-relative bypass after the proxy pass. + ### Dependencies updates - Upgrade Microsoft Application Insights packages. See [#3763](https://github.com/AzureAD/microsoft-identity-web/pull/3763). - Bump net8/net9/net10 runtime package baselines to patched crypto servicing versions. See [#3779](https://github.com/AzureAD/microsoft-identity-web/pull/3779). diff --git a/src/Microsoft.Identity.Web.UI/Areas/MicrosoftIdentity/Controllers/AccountController.cs b/src/Microsoft.Identity.Web.UI/Areas/MicrosoftIdentity/Controllers/AccountController.cs index 50b7edd5d..a11694c3f 100644 --- a/src/Microsoft.Identity.Web.UI/Areas/MicrosoftIdentity/Controllers/AccountController.cs +++ b/src/Microsoft.Identity.Web.UI/Areas/MicrosoftIdentity/Controllers/AccountController.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System; using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.AspNetCore.Authentication; @@ -8,6 +9,7 @@ using Microsoft.AspNetCore.Authentication.OAuth; using Microsoft.AspNetCore.Authentication.OpenIdConnect; using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.Options; @@ -112,7 +114,42 @@ public IActionResult Challenge( { oAuthChallengeProperties.Scope = scope.Split(" "); } - oAuthChallengeProperties.RedirectUri = redirectUri; + + // Validate the redirect URI. Accept: + // * Local URLs (e.g. "/path") — the common MVC pattern. + // * Same-origin absolute URLs (coerced to PathAndQuery) — required because + // Microsoft.Identity.Web's own MicrosoftIdentityConsentAndConditionalAccessHandler + // passes NavigationManager.Uri (always absolute) for Blazor Server step-up + // consent and for Razor Pages / MVC step-up. Rejecting those would break the + // canonical [AuthorizeForScopes] / MsalUiRequiredException flow. + // Reject everything else. The post-sign-in 302 honors AuthenticationProperties.RedirectUri + // as-is (CookieAuthenticationHandler does not enforce IsLocalUrl), so the check must + // happen here. This closes the open-redirect class of bug matching the SignIn action's + // own IsLocalUrl gate added in PR #1219. + string? safeRedirect = null; + if (!string.IsNullOrEmpty(redirectUri)) + { + if (Url.IsLocalUrl(redirectUri) && !IsPercentEncodedSlashBypass(redirectUri)) + { + safeRedirect = redirectUri; + } + else if (Uri.TryCreate(redirectUri, UriKind.Absolute, out var absolute) + && IsSameOrigin(absolute, HttpContext.Request)) + { + // PathAndQuery of a same-origin absolute URL can still begin with "//" or "/\" + // for inputs like "http://victim.app//evil.com/x" (Uri.Host="victim.app", + // PathAndQuery="//evil.com/x") — a protocol-relative URL that CookieAuthenticationHandler + // would emit verbatim in its Location header. Re-run IsLocalUrl on the coerced value + // to reject those shapes. + var candidate = absolute.PathAndQuery; + if (Url.IsLocalUrl(candidate) && !IsPercentEncodedSlashBypass(candidate)) + { + safeRedirect = candidate; + } + } + } + + oAuthChallengeProperties.RedirectUri = safeRedirect ?? Url.Content("~/")!; return Challenge( oAuthChallengeProperties, @@ -186,5 +223,39 @@ public async Task EditProfile([FromRoute] string scheme) properties.Items[Constants.Policy] = _optionsMonitor.Get(scheme).EditProfilePolicyId; return Challenge(properties, scheme); } + + /// + /// Returns true when has the same origin (scheme + host + port) + /// as . Used by Challenge to accept same-origin absolute redirect URIs + /// without opening an open-redirect sink. + /// + private static bool IsSameOrigin(Uri absolute, HttpRequest request) + { + if (!string.Equals(absolute.Scheme, request.Scheme, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + if (!string.Equals(absolute.Host, request.Host.Host, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + int requestPort = request.Host.Port ?? (request.IsHttps ? 443 : 80); + return absolute.Port == requestPort; + } + + /// + /// Defense-in-depth: reject paths whose first segment starts with a percent-encoded + /// forward or backward slash (%2f/%5c). Browsers per RFC 3986 treat these + /// as literal path characters, but misconfigured reverse proxies (NGINX, IIS ARR, F5) + /// can decode them into // or /\ when rewriting the Location + /// header, reopening the protocol-relative bypass that this controller otherwise + /// closes. Comparison is case-insensitive because the RFC 3986 encoding is + /// hex-case-insensitive. + /// + private static bool IsPercentEncodedSlashBypass(string path) => + path.StartsWith("/%2f", StringComparison.OrdinalIgnoreCase) + || path.StartsWith("/%5c", StringComparison.OrdinalIgnoreCase); } } diff --git a/src/Microsoft.Identity.Web/Blazor/LoginLogoutEndpointRouteBuilderExtensions.cs b/src/Microsoft.Identity.Web/Blazor/LoginLogoutEndpointRouteBuilderExtensions.cs index d8621e7be..4dbb9cea8 100644 --- a/src/Microsoft.Identity.Web/Blazor/LoginLogoutEndpointRouteBuilderExtensions.cs +++ b/src/Microsoft.Identity.Web/Blazor/LoginLogoutEndpointRouteBuilderExtensions.cs @@ -4,12 +4,15 @@ using System; using System.Diagnostics.CodeAnalysis; using System.Threading.Tasks; +using Microsoft.AspNetCore.Antiforgery; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Authentication.OpenIdConnect; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Microsoft.IdentityModel.Protocols.OpenIdConnect; namespace Microsoft.Identity.Web; @@ -40,6 +43,8 @@ public static IEndpointConventionBuilder MapLoginAndLogout(this IEndpointRouteBu { var group = endpoints.MapGroup(""); + WarnIfAntiforgeryMissing(endpoints.ServiceProvider); + // Enhanced login endpoint that supports incremental consent and Conditional Access group.MapGet("/login", ( string? returnUrl, @@ -81,6 +86,26 @@ public static IEndpointConventionBuilder MapLoginAndLogout(this IEndpointRouteBu group.MapPost("/logout", async (HttpContext context) => { + // Defense-in-depth CSRF validation (MSRC hardening). When the host has registered + // IAntiforgery via AddAntiforgery() (or indirectly via AddControllersWithViews, + // AddRazorPages, AddMvc, etc.), we explicitly validate the request token here — + // independently of whether UseAntiforgery() middleware is in the pipeline. This + // avoids coupling our endpoint to pipeline shape: MVC hosts that rely on filter- + // time validation, minimal-API hosts that wire UseAntiforgery(), and Blazor hosts + // that wire both all receive equivalent protection at this endpoint. Hosts that + // do not register IAntiforgery at all fall back to RequireAuthorization() + + // SameSite=Lax cookie semantics as the primary CSRF gate — matching pre-MSRC + // behavior and logged once at map time (see WarnIfAntiforgeryMissing). + // IsRequestValidAsync is safe to call after UseAntiforgery() middleware has already + // validated: the form is buffered (ReadFormAsync is cached on HttpRequest), tokens + // are not single-use in the default flow, and re-validation is a cheap hash verify — + // not a no-op, but inexpensive. + var antiforgery = context.RequestServices.GetService(); + if (antiforgery is not null && !await antiforgery.IsRequestValidAsync(context)) + { + return Results.BadRequest(); + } + string? returnUrl = null; if (context.Request.HasFormContentType) { @@ -88,21 +113,63 @@ public static IEndpointConventionBuilder MapLoginAndLogout(this IEndpointRouteBu returnUrl = form["ReturnUrl"]; } - return TypedResults.SignOut(GetAuthProperties(returnUrl), + return (IResult)TypedResults.SignOut(GetAuthProperties(returnUrl), [CookieAuthenticationDefaults.AuthenticationScheme, OpenIdConnectDefaults.AuthenticationScheme]); }) - .DisableAntiforgery(); + .RequireAuthorization(); return group; } - private static AuthenticationProperties GetAuthProperties(string? returnUrl) + // Emits a single warning at endpoint-build time when IAntiforgery isn't registered in DI. + // This surfaces the graceful-degradation state to operators so it's not silently invisible + // that CSRF protection at /logout relies solely on RequireAuthorization + SameSite=Lax + // (rather than token validation). Called once per MapLoginAndLogout invocation. + private static void WarnIfAntiforgeryMissing(IServiceProvider? serviceProvider) + { + var isService = serviceProvider?.GetService(); + var antiforgeryRegistered = isService?.IsService(typeof(IAntiforgery)) ?? false; + if (antiforgeryRegistered) + { + return; + } + + var loggerFactory = serviceProvider?.GetService(); + var logger = loggerFactory?.CreateLogger(typeof(LoginLogoutEndpointRouteBuilderExtensions).FullName!); + logger?.LogWarning( + new EventId(1, "AntiforgeryNotRegistered"), + "MapLoginAndLogout was called but IAntiforgery is not registered in DI. The /logout " + + "endpoint will rely on RequireAuthorization and SameSite=Lax cookies as its CSRF gate. " + + "To enable antiforgery token validation, call services.AddAntiforgery() (and, for " + + "minimal APIs, app.UseAntiforgery())."); + } + + /// + /// Builds with a strictly-local RedirectUri. + /// Any non-local input (absolute URL, protocol-relative "//host", slash-backslash "/\host", + /// or anything not starting with a single '/') is coerced to "/". This matches the + /// semantics of and prevents + /// open-redirect attacks via the ReturnUrl query/form parameter. + /// + internal static AuthenticationProperties GetAuthProperties(string? returnUrl) { const string pathBase = "/"; - if (string.IsNullOrEmpty(returnUrl)) returnUrl = pathBase; - else if (returnUrl.StartsWith("//", StringComparison.Ordinal)) returnUrl = pathBase; // Prevent protocol-relative redirects - else if (!Uri.IsWellFormedUriString(returnUrl, UriKind.Relative)) returnUrl = new Uri(returnUrl, UriKind.Absolute).PathAndQuery; - else if (returnUrl[0] != '/') returnUrl = $"{pathBase}{returnUrl}"; - return new AuthenticationProperties { RedirectUri = returnUrl }; + return new AuthenticationProperties { RedirectUri = IsLocalUrl(returnUrl) ? returnUrl! : pathBase }; + } + + private static bool IsLocalUrl(string? url) + { + if (string.IsNullOrEmpty(url)) + { + return false; + } + + // "/foo" is local, but not "//foo" (protocol-relative) and not "/\foo" (slash-backslash). + if (url[0] == '/') + { + return url.Length == 1 || (url[1] != '/' && url[1] != '\\'); + } + + return false; } } diff --git a/src/Microsoft.Identity.Web/PublicAPI/net10.0/InternalAPI.Unshipped.txt b/src/Microsoft.Identity.Web/PublicAPI/net10.0/InternalAPI.Unshipped.txt index 7dc5c5811..e89892388 100644 --- a/src/Microsoft.Identity.Web/PublicAPI/net10.0/InternalAPI.Unshipped.txt +++ b/src/Microsoft.Identity.Web/PublicAPI/net10.0/InternalAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +static Microsoft.Identity.Web.LoginLogoutEndpointRouteBuilderExtensions.GetAuthProperties(string? returnUrl) -> Microsoft.AspNetCore.Authentication.AuthenticationProperties! \ No newline at end of file diff --git a/src/Microsoft.Identity.Web/PublicAPI/net8.0/InternalAPI.Unshipped.txt b/src/Microsoft.Identity.Web/PublicAPI/net8.0/InternalAPI.Unshipped.txt index 7dc5c5811..e89892388 100644 --- a/src/Microsoft.Identity.Web/PublicAPI/net8.0/InternalAPI.Unshipped.txt +++ b/src/Microsoft.Identity.Web/PublicAPI/net8.0/InternalAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +static Microsoft.Identity.Web.LoginLogoutEndpointRouteBuilderExtensions.GetAuthProperties(string? returnUrl) -> Microsoft.AspNetCore.Authentication.AuthenticationProperties! \ No newline at end of file diff --git a/src/Microsoft.Identity.Web/PublicAPI/net9.0/InternalAPI.Unshipped.txt b/src/Microsoft.Identity.Web/PublicAPI/net9.0/InternalAPI.Unshipped.txt index 7dc5c5811..e89892388 100644 --- a/src/Microsoft.Identity.Web/PublicAPI/net9.0/InternalAPI.Unshipped.txt +++ b/src/Microsoft.Identity.Web/PublicAPI/net9.0/InternalAPI.Unshipped.txt @@ -1 +1,2 @@ #nullable enable +static Microsoft.Identity.Web.LoginLogoutEndpointRouteBuilderExtensions.GetAuthProperties(string? returnUrl) -> Microsoft.AspNetCore.Authentication.AuthenticationProperties! \ No newline at end of file diff --git a/tests/Microsoft.Identity.Web.Test/Blazor/LoginLogoutEndpointRouteBuilderExtensionsTests.cs b/tests/Microsoft.Identity.Web.Test/Blazor/LoginLogoutEndpointRouteBuilderExtensionsTests.cs index e2400d024..8fd88f47c 100644 --- a/tests/Microsoft.Identity.Web.Test/Blazor/LoginLogoutEndpointRouteBuilderExtensionsTests.cs +++ b/tests/Microsoft.Identity.Web.Test/Blazor/LoginLogoutEndpointRouteBuilderExtensionsTests.cs @@ -1,30 +1,59 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using Microsoft.AspNetCore.Authorization; using Xunit; namespace Microsoft.Identity.Web.Test.Blazor { public class LoginLogoutEndpointRouteBuilderExtensionsTests { - // Note: The MapLoginAndLogout extension method relies on minimal APIs and ASP.NET Core - // routing infrastructure. Comprehensive testing requires integration tests with a real - // test server, which are not included in this unit test project. + // ----------------------------------------------------------------------------- + // F3 regression guards — GetAuthProperties open-redirect hardening. // - // The method provides the following functionality (verified through integration tests): - // - Login endpoint with support for scope, loginHint, domainHint, and claims parameters - // - Logout endpoint that signs out from both Cookie and OIDC schemes - // - Open redirect protection through GetAuthProperties validation - // - Anonymous access to login endpoint - // - Antiforgery disabled on logout endpoint for simple form posts + // The ReturnUrl flowed from cross-origin form POSTs into + // AuthenticationProperties.RedirectUri, which is followed as-is by the + // ASP.NET Core sign-out handler. The previous implementation: + // - accepted "/\host" (framework IsLocalUrl rejects this) + // - silently coerced absolute URLs via `new Uri(...).PathAndQuery` + // - prepended '/' to bare strings like "evil.example" + // These tests pin the hardened local-URL-only semantics. Anything not a + // strictly-local path ("/" or "/path...") must be coerced to "/". + // ----------------------------------------------------------------------------- + + [Theory] + // Local paths — preserved. + [InlineData("/", "/")] + [InlineData("/home", "/home")] + [InlineData("/home?query=1", "/home?query=1")] + [InlineData("/a/b/c", "/a/b/c")] + // Null/empty — fall back to "/". + [InlineData(null, "/")] + [InlineData("", "/")] + // Protocol-relative — blocked. + [InlineData("//evil.example", "/")] + [InlineData("//evil.example/path", "/")] + // Slash-backslash bypass of naive validators — blocked. + [InlineData("/\\evil.example", "/")] + [InlineData("/\\\\evil.example", "/")] + // Absolute URLs — blocked. + [InlineData("https://evil.example/", "/")] + [InlineData("http://evil.example/", "/")] + [InlineData("javascript:alert(1)", "/")] + // Bare hostnames / non-slash-prefixed — blocked. + [InlineData("evil.example", "/")] + [InlineData("home", "/")] + public void GetAuthProperties_CoercesNonLocalReturnUrls(string? input, string expected) + { + var props = LoginLogoutEndpointRouteBuilderExtensions.GetAuthProperties(input); + Assert.Equal(expected, props.RedirectUri); + } [Fact] public void ExtensionMethodExists() { - // This test verifies that the extension method compiles and is accessible. - // Actual behavior is tested in integration tests. - Assert.True(typeof(LoginLogoutEndpointRouteBuilderExtensions) - .GetMethod("MapLoginAndLogout") != null); + Assert.NotNull(typeof(LoginLogoutEndpointRouteBuilderExtensions) + .GetMethod("MapLoginAndLogout")); } } } diff --git a/tests/Microsoft.Identity.Web.Test/Blazor/LogoutEndpointIntegrationTests.cs b/tests/Microsoft.Identity.Web.Test/Blazor/LogoutEndpointIntegrationTests.cs new file mode 100644 index 000000000..b03872332 --- /dev/null +++ b/tests/Microsoft.Identity.Web.Test/Blazor/LogoutEndpointIntegrationTests.cs @@ -0,0 +1,376 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Security.Claims; +using System.Text.Encodings.Web; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Antiforgery; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.AspNetCore.Authentication.OpenIdConnect; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Microsoft.IdentityModel.Tokens; +using Xunit; + +namespace Microsoft.Identity.Web.Test.Blazor +{ + /// + /// End-to-end tests for the /logout endpoint mapped by + /// + /// validating the defense-in-depth gating through a real ASP.NET Core pipeline: + /// + /// RequireAuthorization must reject unauthenticated POSTs (401). + /// Antiforgery must reject authenticated POSTs that lack a valid token (400). + /// Authenticated POSTs with a valid token must succeed. + /// + /// This closes the MSRC CSRF-on-logout class by exercising the full middleware chain + /// (routing, authentication, authorization, antiforgery, endpoint execution), not + /// just the isolated GetAuthProperties helper. + /// + public class LogoutEndpointIntegrationTests + { + private const string TestScheme = "Test"; + private const string AntiforgeryFormFieldName = "__RequestVerificationToken"; + + private static TestServer CreateServer(AuthState authState, bool wireAntiforgery = true) + => CreateServer(authState, addAntiforgeryServices: wireAntiforgery, useAntiforgeryMiddleware: wireAntiforgery); + + private static TestServer CreateServer( + AuthState authState, + bool addAntiforgeryServices, + bool useAntiforgeryMiddleware) + { +#pragma warning disable ASPDEPR004 // WebHostBuilder is deprecated — acceptable for TestServer-based test hosts. + var builder = new WebHostBuilder() + .ConfigureServices(services => + { + services.AddSingleton(authState); + services.AddRouting(); + if (addAntiforgeryServices) + { + services.AddAntiforgery(options => + { + options.FormFieldName = AntiforgeryFormFieldName; + }); + } + + // Register the two schemes the endpoint signs out of with a single stub + // that handles both authentication and sign-out. Aliasing the default scheme + // to the Cookie scheme name lets RequireAuthorization discover our principal. + services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationScheme) + .AddScheme( + CookieAuthenticationDefaults.AuthenticationScheme, _ => { }) + .AddScheme( + OpenIdConnectDefaults.AuthenticationScheme, _ => { }); + + services.AddAuthorization(); + }) + .Configure(app => + { + app.UseRouting(); + app.UseAuthentication(); + app.UseAuthorization(); + if (useAntiforgeryMiddleware) + { + app.UseAntiforgery(); + } + +#pragma warning disable ASP0014 // UseEndpoints is intentional for the test host + app.UseEndpoints(endpoints => + { + if (addAntiforgeryServices) + { + // Test-only: surface an antiforgery token pair to the client so the + // "valid token" test can build a legitimate POST. Only mapped when + // antiforgery services are registered — otherwise minimal-API parameter + // inference fails because IAntiforgery isn't in DI. + endpoints.MapGet("/_testing/antiforgery-token", (HttpContext ctx, IAntiforgery af) => + { + var tokens = af.GetAndStoreTokens(ctx); + return Results.Text(tokens.RequestToken ?? string.Empty); + }); + } + + endpoints.MapLoginAndLogout(); + }); +#pragma warning restore ASP0014 + }); +#pragma warning disable ASPDEPR008 // TestServer(IWebHostBuilder) is deprecated — still the canonical net8/9/10 test-host ctor. + return new TestServer(builder); +#pragma warning restore ASPDEPR008 +#pragma warning restore ASPDEPR004 + } + + private static async Task<(string Token, string Cookie)> GetAntiforgeryTokenAndCookieAsync(HttpClient client) + { + using var response = await client.GetAsync("/_testing/antiforgery-token"); + response.EnsureSuccessStatusCode(); + var token = (await response.Content.ReadAsStringAsync()).Trim(); + + // TestServer's default HttpClient does not track cookies; we extract the + // antiforgery cookie from Set-Cookie and replay it on the POST. + var setCookieValues = response.Headers.TryGetValues("Set-Cookie", out var values) + ? values + : Array.Empty(); + string? antiforgeryCookie = null; + foreach (var setCookie in setCookieValues) + { + if (setCookie.Contains(".AspNetCore.Antiforgery.", StringComparison.Ordinal)) + { + // Reduce "name=value; path=/; ..." to just "name=value" for the Cookie header. + var semicolonIndex = setCookie.IndexOf(';', StringComparison.Ordinal); + antiforgeryCookie = semicolonIndex >= 0 ? setCookie.Substring(0, semicolonIndex) : setCookie; + break; + } + } + + Assert.NotNull(antiforgeryCookie); + return (token, antiforgeryCookie!); + } + + // ─── F3 blocker test 1: RequireAuthorization gate ──────────────────────────── + + /// + /// Unauthenticated POST to /logout must be rejected by RequireAuthorization() + /// with 401 Unauthorized before any endpoint logic or antiforgery check runs. + /// This is the first defense-in-depth layer closing the MSRC CSRF-on-logout class. + /// + [Fact] + public async Task Logout_Unauthenticated_Returns401() + { + using var server = CreateServer(new AuthState { Authenticated = false }); + using var client = server.CreateClient(); + + using var response = await client.PostAsync( + "/logout", + new FormUrlEncodedContent(new Dictionary())); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + } + + // ─── F3 blocker test 2: Antiforgery truth probe ────────────────────────────── + + /// + /// MSRC truth probe. Authenticated POST to /logout with form content type but no + /// antiforgery token must be rejected with 400 Bad Request. If this test + /// fails (e.g. returns 200/302), antiforgery middleware is not actually firing on + /// the endpoint — which would mean the SameSite=None consumer protection + /// rests solely on RequireAuthorization() and the hybrid defense-in-depth + /// argument collapses. That would require adding .RequireAntiforgery() + /// explicitly on the MapPost endpoint. + /// + [Fact] + public async Task Logout_AuthenticatedWithoutAntiforgeryToken_Returns400() + { + using var server = CreateServer(new AuthState { Authenticated = true }); + using var client = server.CreateClient(); + + using var response = await client.PostAsync( + "/logout", + new FormUrlEncodedContent(new Dictionary())); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + // ─── Happy path: authenticated + valid antiforgery token → success ─────────── + + /// + /// Authenticated POST to /logout carrying a valid antiforgery cookie + form field + /// pair must succeed. Exercises the full path through the middleware chain and + /// into the endpoint's SignOut call. + /// + [Fact] + public async Task Logout_AuthenticatedWithValidToken_Succeeds() + { + using var server = CreateServer(new AuthState { Authenticated = true }); + using var client = server.CreateClient(); + + var (token, antiforgeryCookie) = await GetAntiforgeryTokenAndCookieAsync(client); + + using var request = new HttpRequestMessage(HttpMethod.Post, "/logout") + { + Content = new FormUrlEncodedContent(new Dictionary + { + [AntiforgeryFormFieldName] = token, + ["ReturnUrl"] = "/", + }), + }; + request.Headers.Add("Cookie", antiforgeryCookie); + + using var response = await client.SendAsync(request); + + // SignOut returns 200 OK when the sign-out handlers don't issue a redirect. + // The stub handler is a no-op, so we expect a non-error response. + Assert.True( + (int)response.StatusCode < 400, + $"Expected success, got {(int)response.StatusCode} {response.StatusCode}"); + } + + // ─── Graceful-degradation tests (Opus 4.7 ship-readiness Q4 additions) ─────── + + /// + /// Graceful-degradation contract. When a host reuses MapLoginAndLogout without + /// wiring AddAntiforgery()/UseAntiforgery() (non-Blazor hosts, custom + /// pipelines), the endpoint must not regress: the IAntiforgeryValidationFeature + /// is never attached, the explicit feature check in the handler is a no-op, and + /// RequireAuthorization() remains the primary CSRF gate (backed by cookie + /// SameSite=Lax semantics). An authenticated POST with no antiforgery token + /// must therefore succeed, not 400, not 500. This locks the claim made in the 4.8.0 + /// changelog that the F3 hardening is zero-impact for non-Blazor consumers. + /// + [Fact] + public async Task Logout_WithoutAntiforgeryWired_AuthenticatedUser_Succeeds() + { + using var server = CreateServer(new AuthState { Authenticated = true }, wireAntiforgery: false); + using var client = server.CreateClient(); + + using var response = await client.PostAsync( + "/logout", + new FormUrlEncodedContent(new Dictionary + { + ["ReturnUrl"] = "/", + })); + + Assert.True( + (int)response.StatusCode < 400, + $"Expected success (no antiforgery wired → graceful degradation), got {(int)response.StatusCode} {response.StatusCode}"); + } + + /// + /// Validation-failure contract. With antiforgery wired but the submitted token + /// malformed, the AntiforgeryMiddleware catches the AntiforgeryValidationException + /// and sets IAntiforgeryValidationFeature.IsValid = false without short-circuiting. + /// The endpoint's explicit feature check must observe this and return 400 Bad Request + /// before any form read. If the explicit check were missing, FormFeature + /// (via HasFormContentType/ReadFormAsync) would throw + /// InvalidOperationException from HandleUncheckedAntiforgeryValidationFeature, + /// bubbling as 500. Asserting 400 (not 500) here is the evidence that the explicit + /// feature check — rather than [FromForm]-driven form binding — is the correct + /// gate, and backs the rationale in the PR description. + /// + [Fact] + public async Task Logout_WithAntiforgeryWired_ValidationFails_Returns400NotTheFormFeature500() + { + using var server = CreateServer(new AuthState { Authenticated = true }); + using var client = server.CreateClient(); using var request = new HttpRequestMessage(HttpMethod.Post, "/logout") + { + // Intentionally malformed token: present but not a valid antiforgery value, + // and no matching cookie. This forces AntiforgeryMiddleware to call + // ValidateRequestAsync, catch AntiforgeryValidationException, and set + // IAntiforgeryValidationFeature.IsValid = false. + Content = new FormUrlEncodedContent(new Dictionary + { + [AntiforgeryFormFieldName] = "this-is-not-a-valid-antiforgery-token", + ["ReturnUrl"] = "/", + }), + }; + + using var response = await client.SendAsync(request); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.NotEqual(HttpStatusCode.InternalServerError, response.StatusCode); + } + + /// + /// MVC-shape regression guard. A common production configuration is to call + /// AddControllersWithViews() / AddRazorPages() / AddMvc() + /// (all of which transitively register IAntiforgery in DI) but not + /// app.UseAntiforgery(), because those stacks validate tokens at filter time + /// rather than through middleware. An earlier iteration of this MSRC fix coupled + /// validation to IAntiforgeryMetadata, which triggered ASP.NET Core's startup + /// verifier — "endpoint contains anti-forgery metadata, but a middleware was not + /// found" — breaking every MVC host that reused MapLoginAndLogout. This test + /// simulates that shape (services present, middleware absent) and asserts both: + /// (a) the app starts without throwing, and (b) a missing / invalid token is rejected + /// as 400 Bad Request via the in-handler IAntiforgery.IsRequestValidAsync + /// call — not as a 500 from FormFeature and not as a silent 200. This locks + /// in the pipeline-shape-independent semantics promised by the 4.8.0 changelog. + /// + [Fact] + public async Task Logout_AntiforgeryServicesRegistered_ButMiddlewareMissing_StillValidates() + { + using var server = CreateServer( + new AuthState { Authenticated = true }, + addAntiforgeryServices: true, + useAntiforgeryMiddleware: false); + using var client = server.CreateClient(); + + using var request = new HttpRequestMessage(HttpMethod.Post, "/logout") + { + Content = new FormUrlEncodedContent(new Dictionary + { + ["ReturnUrl"] = "/", + }), + }; + + using var response = await client.SendAsync(request); + + // No valid antiforgery token supplied → handler's explicit IsRequestValidAsync + // call returns false → 400. Critically: not a startup throw, not a 500 from + // form binding, not a silent 200 CSRF bypass. + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + // ─── Helpers ───────────────────────────────────────────────────────────────── + + internal sealed class AuthState + { + public bool Authenticated { get; set; } + } + + /// + /// Minimal test authentication + sign-out handler. Registered under both + /// and + /// so the endpoint's + /// SignOut(..., [Cookies, OIDC]) call has registered handlers to dispatch + /// to. HandleAuthenticateAsync consults the per-server + /// so individual tests can toggle the authenticated state independently. + /// + private sealed class StubCookieAndOidcHandler + : AuthenticationHandler, IAuthenticationSignOutHandler + { + public StubCookieAndOidcHandler( + IOptionsMonitor options, + ILoggerFactory logger, + UrlEncoder encoder) + : base(options, logger, encoder) + { + } + + protected override Task HandleAuthenticateAsync() + { + var state = Context.RequestServices.GetRequiredService(); + if (!state.Authenticated) + { + return Task.FromResult(AuthenticateResult.NoResult()); + } + + var identity = new CaseSensitiveClaimsIdentity( + new[] { new Claim(ClaimTypes.NameIdentifier, "test-user") }, + Scheme.Name); + var principal = new ClaimsPrincipal(identity); + var ticket = new AuthenticationTicket(principal, Scheme.Name); + return Task.FromResult(AuthenticateResult.Success(ticket)); + } + + public Task SignOutAsync(AuthenticationProperties? properties) + { + // No-op: confirm sign-out was dispatched without performing any real work + // (no cookie clearing, no end-session redirect). The test asserts on HTTP + // status, not on side effects. + return Task.CompletedTask; + } + } + } +} diff --git a/tests/Microsoft.Identity.Web.Test/Microsoft.Identity.Web.Test.csproj b/tests/Microsoft.Identity.Web.Test/Microsoft.Identity.Web.Test.csproj index 43270e56b..da7c39597 100644 --- a/tests/Microsoft.Identity.Web.Test/Microsoft.Identity.Web.Test.csproj +++ b/tests/Microsoft.Identity.Web.Test/Microsoft.Identity.Web.Test.csproj @@ -28,6 +28,7 @@ + diff --git a/tests/Microsoft.Identity.Web.UI.Test/Areas/MicrosoftIdentity/Controllers/AccountControllerTests.cs b/tests/Microsoft.Identity.Web.UI.Test/Areas/MicrosoftIdentity/Controllers/AccountControllerTests.cs index b985cb394..7f8f1cb68 100644 --- a/tests/Microsoft.Identity.Web.UI.Test/Areas/MicrosoftIdentity/Controllers/AccountControllerTests.cs +++ b/tests/Microsoft.Identity.Web.UI.Test/Areas/MicrosoftIdentity/Controllers/AccountControllerTests.cs @@ -102,5 +102,342 @@ public void SignIn_WithInvalidRedirectUri_UsesDefaultRedirectUri() Assert.NotNull(authProps); // Ensure authProps is not null Assert.Equal("/", authProps.RedirectUri); } + + // --- Challenge action tests (F1 regression guard) ------------------------------- + // + // The Challenge action historically assigned the raw `redirectUri` query parameter + // to OAuthChallengeProperties.RedirectUri without any local-URL validation. Because + // the ASP.NET Core cookie handler follows AuthenticationProperties.RedirectUri as-is + // after sign-in, this allowed an open redirect / phishing chain post-Azure-AD auth. + // These tests pin the IsLocalUrl gate. + + [Fact] + public void Challenge_WithLocalRedirectUri_UsesProvidedRedirectUri() + { + // Arrange: IsLocalUrl returns true for local paths (default from constructor). + string scheme = OpenIdConnectDefaults.AuthenticationScheme; + string redirectUri = "/app/home"; + + // Act + var result = _accountController.Challenge( + redirectUri, + scope: "User.Read", + loginHint: "user@example.com", + domainHint: "contoso.com", + claims: null!, + policy: null!, + scheme: scheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal(redirectUri, challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithExternalRedirectUri_UsesDefaultRedirectUri() + { + // Arrange: simulate IUrlHelper.IsLocalUrl rejecting an external URL. + string scheme = OpenIdConnectDefaults.AuthenticationScheme; + string redirectUri = "https://attacker.example.com/harvest"; + + var urlHelperMock = Substitute.For(); + urlHelperMock.IsLocalUrl(redirectUri).Returns(false); + urlHelperMock.Content("~/").Returns("/"); + _accountController.Url = urlHelperMock; + + // Act + var result = _accountController.Challenge( + redirectUri, + scope: null!, + loginHint: null!, + domainHint: null!, + claims: null!, + policy: null!, + scheme: scheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithNullRedirectUri_UsesDefaultRedirectUri() + { + // Arrange + string scheme = OpenIdConnectDefaults.AuthenticationScheme; + + var urlHelperMock = Substitute.For(); + urlHelperMock.IsLocalUrl(Arg.Any()).Returns(false); + urlHelperMock.Content("~/").Returns("/"); + _accountController.Url = urlHelperMock; + + // Act + var result = _accountController.Challenge( + redirectUri: null!, + scope: null!, + loginHint: null!, + domainHint: null!, + claims: null!, + policy: null!, + scheme: scheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithEmptyRedirectUri_UsesDefaultRedirectUri() + { + // Arrange: "" — the `string.IsNullOrEmpty` short-circuit path (sibling of null). + // IUrlHelper.IsLocalUrl("") actually returns false in ASP.NET Core, but the + // short-circuit means our gate must not depend on the helper for this case. + string scheme = OpenIdConnectDefaults.AuthenticationScheme; + + var urlHelperMock = Substitute.For(); + urlHelperMock.IsLocalUrl(Arg.Any()).Returns(true); // deliberately permissive + urlHelperMock.Content("~/").Returns("/"); + _accountController.Url = urlHelperMock; + + // Act + var result = _accountController.Challenge( + redirectUri: string.Empty, + scope: null!, + loginHint: null!, + domainHint: null!, + claims: null!, + policy: null!, + scheme: scheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithProtocolRelativeRedirectUri_UsesDefaultRedirectUri() + { + // Arrange: "//attacker.example.com" — framework IsLocalUrl rejects this. + string scheme = OpenIdConnectDefaults.AuthenticationScheme; + string redirectUri = "//attacker.example.com/"; + + var urlHelperMock = Substitute.For(); + urlHelperMock.IsLocalUrl(redirectUri).Returns(false); + urlHelperMock.Content("~/").Returns("/"); + _accountController.Url = urlHelperMock; + + // Act + var result = _accountController.Challenge( + redirectUri, + scope: null!, + loginHint: null!, + domainHint: null!, + claims: null!, + policy: null!, + scheme: scheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + // ----------------------------------------------------------------------------- + // Challenge action — same-origin absolute URL acceptance. + // + // F1 regression coverage. `MicrosoftIdentityConsentAndConditionalAccessHandler.ChallengeUser()` + // (the canonical step-up consent flow for [AuthorizeForScopes]) passes `NavigationManager.Uri` + // for Blazor Server and `{CreateBaseUri}/{path}` for Razor Pages / MVC — both absolute. + // `Url.IsLocalUrl` rejects absolute URLs even for same-origin, so we must accept them via + // an explicit origin check (scheme + host + port) and coerce to PathAndQuery. These tests pin + // that same-origin allowance and reject any host/scheme/port mismatch. + // ----------------------------------------------------------------------------- + + private void ConfigureRequest(string scheme, string host, int? port) + { + var httpContext = new DefaultHttpContext(); + httpContext.Request.Scheme = scheme; + httpContext.Request.Host = port.HasValue ? new HostString(host, port.Value) : new HostString(host); + _accountController.ControllerContext = new ControllerContext { HttpContext = httpContext }; + } + + /// + /// Installs an mock whose IsLocalUrl mirrors the real framework + /// semantics (rejects null/empty, //host, /\host, and anything non-slash-prefixed; + /// accepts a single-leading-slash path). This matches the behavior the Challenge action + /// relies on both for the initial validation and for the re-check on a coerced same-origin path. + /// + private void UseRealisticIsLocalUrl() + { + var urlHelperMock = Substitute.For(); + urlHelperMock.IsLocalUrl(Arg.Any()).Returns(ci => + { + string? s = ci.Arg(); + if (string.IsNullOrEmpty(s)) return false; + if (s[0] != '/') return false; + if (s.Length == 1) return true; + return s[1] != '/' && s[1] != '\\'; + }); + urlHelperMock.Content("~/").Returns("/"); + _accountController.Url = urlHelperMock; + } + + [Fact] + public void Challenge_WithSameOriginAbsoluteRedirectUri_CoercesToPathAndQuery() + { + // Arrange: Blazor Server step-up consent flow — NavigationManager.Uri is absolute. + ConfigureRequest("https", "myapp.example.com", port: null); + UseRealisticIsLocalUrl(); + + string redirectUri = "https://myapp.example.com/weather?city=Seattle"; + + // Act + var result = _accountController.Challenge( + redirectUri, + scope: "User.Read", + loginHint: null!, domainHint: null!, claims: null!, policy: null!, + scheme: OpenIdConnectDefaults.AuthenticationScheme); + + // Assert: origin accepted, RedirectUri reduced to path+query. + var challengeResult = Assert.IsType(result); + Assert.Equal("/weather?city=Seattle", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithSameOriginAbsoluteRootRedirectUri_CoercesToSlash() + { + // Arrange: the "/" edge case — Uri.PathAndQuery yields "/" for a host-only absolute URL. + ConfigureRequest("https", "myapp.example.com", port: null); + UseRealisticIsLocalUrl(); + + // Act + var result = _accountController.Challenge( + redirectUri: "https://myapp.example.com/", + scope: null!, loginHint: null!, domainHint: null!, claims: null!, policy: null!, + scheme: OpenIdConnectDefaults.AuthenticationScheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithDifferentHostAbsoluteRedirectUri_UsesDefaultRedirectUri() + { + // Arrange: request is for myapp.example.com but redirect points at attacker.example.com. + ConfigureRequest("https", "myapp.example.com", port: null); + UseRealisticIsLocalUrl(); + + // Act + var result = _accountController.Challenge( + redirectUri: "https://attacker.example.com/harvest", + scope: null!, loginHint: null!, domainHint: null!, claims: null!, policy: null!, + scheme: OpenIdConnectDefaults.AuthenticationScheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithDifferentSchemeAbsoluteRedirectUri_UsesDefaultRedirectUri() + { + // Arrange: request is HTTPS but redirect is HTTP — scheme downgrade rejected. + ConfigureRequest("https", "myapp.example.com", port: null); + UseRealisticIsLocalUrl(); + + // Act + var result = _accountController.Challenge( + redirectUri: "http://myapp.example.com/page", + scope: null!, loginHint: null!, domainHint: null!, claims: null!, policy: null!, + scheme: OpenIdConnectDefaults.AuthenticationScheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithDifferentPortAbsoluteRedirectUri_UsesDefaultRedirectUri() + { + // Arrange: request is on the default HTTPS port (443) but redirect targets port 1234. + ConfigureRequest("https", "myapp.example.com", port: null); + UseRealisticIsLocalUrl(); + + // Act + var result = _accountController.Challenge( + redirectUri: "https://myapp.example.com:1234/page", + scope: null!, loginHint: null!, domainHint: null!, claims: null!, policy: null!, + scheme: OpenIdConnectDefaults.AuthenticationScheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithSameOriginExplicitPortAbsoluteRedirectUri_CoercesToPathAndQuery() + { + // Arrange: non-default port on both sides (localhost:5001 dev scenario). + ConfigureRequest("https", "localhost", port: 5001); + UseRealisticIsLocalUrl(); + + // Act + var result = _accountController.Challenge( + redirectUri: "https://localhost:5001/counter", + scope: null!, loginHint: null!, domainHint: null!, claims: null!, policy: null!, + scheme: OpenIdConnectDefaults.AuthenticationScheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/counter", challengeResult.Properties!.RedirectUri); + } + + // ----------------------------------------------------------------------------- + // Same-origin absolute URL — protocol-relative-shape bypass via PathAndQuery. + // + // Uri.TryCreate("http://real.com//evil.com/x") yields Host="real.com" and + // PathAndQuery="//evil.com/x". Without a re-check, the same-origin branch would emit a + // protocol-relative URL that CookieAuthenticationHandler would faithfully return in its + // Location header — the browser then navigates to https://evil.com/x. These tests pin the + // IsLocalUrl re-check on the coerced value. + // ----------------------------------------------------------------------------- + + [Fact] + public void Challenge_WithSameOriginAbsoluteButProtocolRelativePathAndQuery_UsesDefaultRedirectUri() + { + // Arrange + ConfigureRequest("https", "victim.app", port: null); + UseRealisticIsLocalUrl(); + + // Act: "http://victim.app//evil.com/x" → Uri.Host="victim.app" (matches request host, but + // request scheme is https, so same-origin returns false). We deliberately match the scheme + // too (https) to force the same-origin branch to be taken, then check the IsLocalUrl re-check. + var result = _accountController.Challenge( + redirectUri: "https://victim.app//evil.com/x", + scope: null!, loginHint: null!, domainHint: null!, claims: null!, policy: null!, + scheme: OpenIdConnectDefaults.AuthenticationScheme); + + // Assert: the coerced PathAndQuery ("//evil.com/x") is a protocol-relative URL; re-check rejects. + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } + + [Fact] + public void Challenge_WithSameOriginAbsoluteButSlashBackslashPathAndQuery_UsesDefaultRedirectUri() + { + // Arrange + ConfigureRequest("https", "victim.app", port: null); + UseRealisticIsLocalUrl(); + + // Act: "https://victim.app/\evil.com" → Uri normalizes to PathAndQuery="//evil.com" — same bypass shape. + var result = _accountController.Challenge( + redirectUri: @"https://victim.app/\evil.com", + scope: null!, loginHint: null!, domainHint: null!, claims: null!, policy: null!, + scheme: OpenIdConnectDefaults.AuthenticationScheme); + + // Assert + var challengeResult = Assert.IsType(result); + Assert.Equal("/", challengeResult.Properties!.RedirectUri); + } } } diff --git a/tests/Microsoft.Identity.Web.UI.Test/Areas/MicrosoftIdentity/Controllers/ChallengeEndpointIntegrationTests.cs b/tests/Microsoft.Identity.Web.UI.Test/Areas/MicrosoftIdentity/Controllers/ChallengeEndpointIntegrationTests.cs new file mode 100644 index 000000000..57c42066d --- /dev/null +++ b/tests/Microsoft.Identity.Web.UI.Test/Areas/MicrosoftIdentity/Controllers/ChallengeEndpointIntegrationTests.cs @@ -0,0 +1,265 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Net; +using System.Net.Http; +using System.Text.Encodings.Web; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Microsoft.Identity.Web.UI; +using Xunit; + +namespace Microsoft.Identity.Web.UI.Test.Areas.MicrosoftIdentity.Controllers +{ + /// + /// End-to-end tests for /MicrosoftIdentity/Account/Challenge validating the + /// same-origin coercion and open-redirect rejection through a real ASP.NET Core + /// pipeline. The production authentication handler is replaced by + /// , which short-circuits the challenge + /// into a 200 response and echoes + /// via the X-Captured-RedirectUri header so the test can assert what the + /// controller handed to the authentication layer. + /// + public class ChallengeEndpointIntegrationTests + { + private const string TestScheme = "OpenIdConnect"; + private const string CapturedRedirectHeader = "X-Captured-RedirectUri"; + + private static TestServer CreateServer() + { + var builder = new WebHostBuilder() + .ConfigureServices(services => + { + services.AddControllersWithViews().AddMicrosoftIdentityUI(); + + // AccountController resolves IOptionsMonitor; + // register an empty options entry so DI can satisfy the ctor. + services.Configure(TestScheme, _ => { }); + + services.AddAuthentication(TestScheme) + .AddScheme( + TestScheme, _ => { }); + + services.AddAuthorization(); + }) + .Configure(app => + { + app.UseRouting(); + app.UseAuthentication(); + app.UseAuthorization(); +#pragma warning disable ASP0014 // UseEndpoints is intentional for clarity in test host + app.UseEndpoints(endpoints => endpoints.MapControllers()); +#pragma warning restore ASP0014 + }); + return new TestServer(builder); + } + + private static async Task<(HttpStatusCode status, string? capturedRedirect)> ChallengeAsync( + TestServer server, + string? redirectUri) + { + using var client = server.CreateClient(); + var url = $"/MicrosoftIdentity/Account/Challenge/{TestScheme}"; + if (redirectUri != null) + { + url += "?redirectUri=" + Uri.EscapeDataString(redirectUri); + } + + using var response = await client.GetAsync(url).ConfigureAwait(false); + string? captured = null; + if (response.Headers.TryGetValues(CapturedRedirectHeader, out var values)) + { + captured = string.Join(",", values); + } + + return (response.StatusCode, captured); + } + + // Local and obviously-non-local inputs: baseline behavior expected by the + // original IsLocalUrl gate (no regression from the MSRC hardening). + [Theory] + [InlineData("/home", "/home")] + [InlineData("/", "/")] + [InlineData("/path?q=1&x=2", "/path?q=1&x=2")] + [InlineData("", "/")] + [InlineData(null, "/")] + [InlineData("https://evil.example.com/path", "/")] + [InlineData("http://evil.example.com", "/")] + [InlineData("javascript:alert(1)", "/")] + public async Task Challenge_RedirectUri_Variants_LocalOrRejected( + string? input, + string expectedRedirect) + { + using var server = CreateServer(); + var (status, captured) = await ChallengeAsync(server, input); + + Assert.Equal(HttpStatusCode.OK, status); + Assert.Equal(expectedRedirect, captured); + } + + // Same-origin absolute URLs must be coerced to their PathAndQuery — this is the + // path taken by MicrosoftIdentityConsentAndConditionalAccessHandler.ChallengeUser, + // which passes NavigationManager.Uri (always absolute) for Blazor Server step-up. + // TestServer's default base is http://localhost, so "localhost" is same-origin here. + [Theory] + [InlineData("http://localhost/page", "/page")] + [InlineData("http://localhost/page?q=1", "/page?q=1")] + [InlineData("http://localhost/", "/")] + [InlineData("http://LOCALHOST/page", "/page")] // host is case-insensitive + public async Task Challenge_SameOriginAbsolute_CoercedToPathAndQuery( + string input, + string expectedRedirect) + { + using var server = CreateServer(); + var (status, captured) = await ChallengeAsync(server, input); + + Assert.Equal(HttpStatusCode.OK, status); + Assert.Equal(expectedRedirect, captured); + } + + // Different origins (host, scheme, or port) must fall through to "/". + [Theory] + [InlineData("http://attacker.example.com/page")] + [InlineData("https://localhost/page")] // scheme mismatch + [InlineData("http://localhost:8080/page")] // port mismatch + public async Task Challenge_DifferentOrigin_FallsBackToRoot(string input) + { + using var server = CreateServer(); + var (status, captured) = await ChallengeAsync(server, input); + + Assert.Equal(HttpStatusCode.OK, status); + Assert.Equal("/", captured); + } + + // The MSRC hardening: same-origin absolute URLs whose PathAndQuery is a + // protocol-relative URL ("//evil.com/x" or "/\evil.com") must be rejected. + // Without the IsLocalUrl re-check on PathAndQuery, these would reach the + // Location header verbatim and a browser would resolve them cross-origin. + [Theory] + [InlineData("http://localhost//evil.example.com/x")] // protocol-relative + [InlineData("http://localhost/\\evil.example.com")] // slash+backslash + public async Task Challenge_SameOriginWithProtocolRelativePath_Rejected(string input) + { + using var server = CreateServer(); + var (status, captured) = await ChallengeAsync(server, input); + + Assert.Equal(HttpStatusCode.OK, status); + Assert.Equal("/", captured); + } + + // Defense-in-depth against misconfigured reverse proxies (NGINX, IIS ARR, F5) + // that decode %2f / %5c in Location headers. Browsers per RFC 3986 treat these + // as literal path chars (direct hit yields 404, not a redirect), but a proxy + // rewriting the Location header can expand them into "//" or "/\" — the same + // protocol-relative shape that SameOriginWithProtocolRelativePath_Rejected + // guards against in decoded form. The hardening rejects both the direct-input + // branch (IsLocalUrl returns true for "/%2f%2fevil.com") and the same-origin + // coercion branch. + [Theory] + [InlineData("http://localhost/%2f%2fevil.example.com")] // same-origin + %2f bypass + [InlineData("http://localhost/%5c%5cevil.example.com")] // same-origin + %5c bypass + [InlineData("/%2f%2fevil.example.com")] // direct %2f bypass (no host) + [InlineData("/%5c%5cevil.example.com")] // direct %5c bypass (no host) + [InlineData("/%2F%2Fevil.example.com")] // case-insensitive hex + public async Task Challenge_PercentEncodedSlashBypass_Rejected(string input) + { + using var server = CreateServer(); + var (status, captured) = await ChallengeAsync(server, input); + + Assert.Equal(HttpStatusCode.OK, status); + Assert.Equal("/", captured); + } + + // Userinfo misdirection: "http://localhost@evil.example.com/x" is parsed by Uri + // as Host="evil.example.com" (userinfo="localhost"), which is NOT same-origin as + // the TestServer base of http://localhost. The different-origin branch should + // fall back to "/" — verifying that IsSameOrigin compares Uri.Host, not the + // userinfo-containing authority. If this ever regressed to comparing the raw + // authority string, the attacker-controlled host would pass. + [Fact] + public async Task Challenge_UserinfoMisdirection_FallsBackToRoot() + { + using var server = CreateServer(); + var (status, captured) = await ChallengeAsync(server, "http://localhost@evil.example.com/x"); + + Assert.Equal(HttpStatusCode.OK, status); + Assert.Equal("/", captured); + } + + // Triple-slash inputs like "///evil.example.com/x" are ambiguous: Uri.TryCreate + // with UriKind.Absolute may parse them as file:// URIs with an empty host, which + // would neither match IsLocalUrl (false — looks absolute to routing) nor + // IsSameOrigin (scheme mismatch). Either way they must not reach the Location + // header verbatim. This test locks the fall-through-to-root contract. + [Fact] + public async Task Challenge_TripleSlashFileFallthrough_FallsBackToRoot() + { + using var server = CreateServer(); + var (status, captured) = await ChallengeAsync(server, "///evil.example.com/x"); + + Assert.Equal(HttpStatusCode.OK, status); + Assert.Equal("/", captured); + } + + // HTTPS BaseAddress variants: exercises the `request.IsHttps ? 443 : 80` branch + // of IsSameOrigin to ensure same-origin coercion works on HTTPS hosts and that + // scheme-mismatched absolute URLs (http against an https request) fall through. + [Theory] + [InlineData("https://localhost/page", "/page")] // same-origin HTTPS + [InlineData("https://localhost:443/page", "/page")] // explicit default port + [InlineData("http://localhost/page", "/")] // scheme mismatch on HTTPS host + [InlineData("https://localhost//evil.example.com/x", "/")]// protocol-relative on HTTPS + public async Task Challenge_HttpsBaseAddress_EnforcesSameOrigin( + string input, + string expectedRedirect) + { + using var server = CreateServer(); + using var client = server.CreateClient(); + client.BaseAddress = new Uri("https://localhost/"); + + var url = $"/MicrosoftIdentity/Account/Challenge/{TestScheme}?redirectUri=" + + Uri.EscapeDataString(input); + using var response = await client.GetAsync(url); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var captured = response.Headers.TryGetValues(CapturedRedirectHeader, out var values) + ? string.Join(",", values) + : null; + Assert.Equal(expectedRedirect, captured); + } + + /// + /// Replaces the real OpenIdConnect handler for test purposes: instead of issuing + /// an OIDC redirect, it captures + /// into a response header and returns 200. This isolates the behavior of the + /// controller's redirect-URI validation from the authentication middleware chain. + /// + private sealed class CapturingChallengeHandler : AuthenticationHandler + { + public CapturingChallengeHandler( + IOptionsMonitor options, + ILoggerFactory logger, + UrlEncoder encoder) + : base(options, logger, encoder) + { + } + + protected override Task HandleAuthenticateAsync() + => Task.FromResult(AuthenticateResult.NoResult()); + + protected override Task HandleChallengeAsync(AuthenticationProperties properties) + { + Response.Headers[CapturedRedirectHeader] = properties?.RedirectUri ?? string.Empty; + Response.StatusCode = (int)HttpStatusCode.OK; + return Task.CompletedTask; + } + } + } +} diff --git a/tests/Microsoft.Identity.Web.UI.Test/Microsoft.Identity.Web.UI.Test.csproj b/tests/Microsoft.Identity.Web.UI.Test/Microsoft.Identity.Web.UI.Test.csproj index c6850db9e..fb6a12ff3 100644 --- a/tests/Microsoft.Identity.Web.UI.Test/Microsoft.Identity.Web.UI.Test.csproj +++ b/tests/Microsoft.Identity.Web.UI.Test/Microsoft.Identity.Web.UI.Test.csproj @@ -10,6 +10,7 @@ +