From ed77a077520090b3922ad3e9bc2eb07466ddca06 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Wed, 27 May 2026 18:54:52 +0200 Subject: [PATCH 1/3] Reject ASCII control characters in cookie auth return URLs CookieAuthenticationHandler.IsHostRelative now mirrors the control-character guard from SharedUrlHelper.IsLocalUrl and UrlHelperBase.CheckIsLocalUrl, blocking U+0000-U+001F and U+007F in ReturnUrl query values to prevent Location header injection through the LoginPath / LogoutPath redirect flow. Adds 35 new theory rows in CookieTests covering control-character rejection, benign honor cases, and broader non-host-relative rejection (protocol-relative, "/\", "~/", absolute, relative, scheme confusion, whitespace). Fixes #66806 --- .../src/CookieAuthenticationHandler.cs | 41 +++++++++- .../Authentication/test/CookieTests.cs | 79 +++++++++++++++++++ 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs b/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs index 0e9ad0883808..ee4d694d74fe 100644 --- a/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs +++ b/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs @@ -445,17 +445,52 @@ await Events.RedirectToReturnUrl( } } + // Mirror of Microsoft.AspNetCore.Internal.SharedUrlHelper.IsLocalUrl + // (src/Shared/ResultsHelpers/SharedUrlHelper.cs). Keep the two in sync. + // Intentional difference: cookie auth rejects "~/" paths because ApplyHeaders + // writes the value verbatim into the Location header and never resolves + // PathBase like IUrlHelper.Content does — so the "~/..." branch from + // IsLocalUrl is omitted here. private static bool IsHostRelative(string path) { if (string.IsNullOrEmpty(path)) { return false; } - if (path.Length == 1) + + // Allows "/" or "/foo" but not "//" or "/\". + if (path[0] == '/') + { + // path is exactly "/" + if (path.Length == 1) + { + return true; + } + + // path doesn't start with "//" or "/\" + if (path[1] != '/' && path[1] != '\\') + { + return !HasControlCharacter(path.AsSpan(1)); + } + + return false; + } + + return false; + + static bool HasControlCharacter(ReadOnlySpan readOnlySpan) { - return path[0] == '/'; + // URLs may not contain ASCII control characters. + for (var i = 0; i < readOnlySpan.Length; i++) + { + if (char.IsControl(readOnlySpan[i])) + { + return true; + } + } + + return false; } - return path[0] == '/' && path[1] != '/' && path[1] != '\\'; } /// diff --git a/src/Security/Authentication/test/CookieTests.cs b/src/Security/Authentication/test/CookieTests.cs index bc9ff451415d..41ad1c911b5e 100644 --- a/src/Security/Authentication/test/CookieTests.cs +++ b/src/Security/Authentication/test/CookieTests.cs @@ -77,6 +77,85 @@ public async Task AjaxLogoutRedirectToReturnUrlTurnsInto200WithLocationHeader() Assert.StartsWith("/", responded.Single()); } + [Theory] + [InlineData("/\tfoo")] + [InlineData("/\rfoo")] + [InlineData("/\nfoo")] + [InlineData("/\r\nfoo")] + [InlineData("/\0foo")] + [InlineData("/\u0001foo")] + [InlineData("/\u001Ffoo")] + [InlineData("/\u007Ffoo")] + [InlineData("/foo\r\nLocation:%20evil")] + [InlineData("/foo\tbar")] + public async Task LogoutReturnUrlWithControlCharacterIsRejected(string returnUrl) + { + using var host = await CreateHost(o => o.LogoutPath = "/signout"); + using var server = host.GetTestServer(); + var transaction = await SendAsync( + server, + "http://example.com/signout?ReturnUrl=" + Uri.EscapeDataString(returnUrl)); + + Assert.False(transaction.Response.Headers.Contains("Location")); + } + + [Theory] + [InlineData("/")] + [InlineData("/foo")] + [InlineData("/foo/bar")] + [InlineData("/foo?x=1#y")] + public async Task LogoutReturnUrlWithoutControlCharacterIsHonored(string returnUrl) + { + using var host = await CreateHost(o => o.LogoutPath = "/signout"); + using var server = host.GetTestServer(); + var transaction = await SendAsync( + server, + "http://example.com/signout?ReturnUrl=" + Uri.EscapeDataString(returnUrl)); + + var location = Assert.Single(transaction.Response.Headers.GetValues("Location")); + Assert.Equal(returnUrl, location); + } + + [Theory] + // Protocol-relative URLs (path[1] == '/'). + [InlineData("//www.example.com")] + [InlineData("//www.example.com/foobar.html")] + [InlineData("///www.example.com")] + [InlineData("//////www.example.com")] + // Forward-then-backslash (path[1] == '\\'). + [InlineData("/\\")] + [InlineData("/\\foo")] + [InlineData("/\\evil.com")] + // Application-relative (~) is rejected — cookie auth never resolves PathBase like MVC's IUrlHelper.Content does. + [InlineData("~/")] + [InlineData("~/foo")] + [InlineData("~/foo.html")] + // Absolute URLs (path[0] != '/'). + [InlineData("http://www.example.com")] + [InlineData("https://www.example.com")] + [InlineData("HtTpS://www.example.com")] + [InlineData("http://localhost/foobar.html")] + [InlineData("javascript:alert(1)")] + // Relative URLs (path[0] != '/'). + [InlineData("foo.html")] + [InlineData("../foo.html")] + [InlineData("fold/foo.html")] + // Single-slash scheme confusion. + [InlineData("http:/foo.html")] + [InlineData("hTtP:foo.html")] + // Whitespace-only (length 1, path[0] != '/'). + [InlineData(" ")] + public async Task LogoutReturnUrlNonHostRelativeIsRejected(string returnUrl) + { + using var host = await CreateHost(o => o.LogoutPath = "/signout"); + using var server = host.GetTestServer(); + var transaction = await SendAsync( + server, + "http://example.com/signout?ReturnUrl=" + Uri.EscapeDataString(returnUrl)); + + Assert.False(transaction.Response.Headers.Contains("Location")); + } + [Fact] public async Task AjaxChallengeRedirectTurnsInto200WithLocationHeader() { From f953fb648819766752343aad20da34cc090404b1 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Tue, 2 Jun 2026 23:08:21 +0200 Subject: [PATCH 2/3] Share IsLocalUrl between cookie auth and SharedUrlHelper - Link SharedUrlHelper.cs into Cookies project and forward CookieAuthenticationHandler.IsHostRelative - Forward UrlHelperBase.CheckIsLocalUrl to SharedUrlHelper.IsLocalUrl - Add SECURITY note on SharedUrlHelper.IsLocalUrl flagging it as an open-redirect guard --- src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs | 60 +------------------ .../src/CookieAuthenticationHandler.cs | 50 ++-------------- ...t.AspNetCore.Authentication.Cookies.csproj | 1 + src/Shared/ResultsHelpers/SharedUrlHelper.cs | 4 ++ 4 files changed, 11 insertions(+), 104 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs b/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs index 2b0fa0b1a8f2..c1598ac349ec 100644 --- a/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs +++ b/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs @@ -6,6 +6,7 @@ using System.Globalization; using System.Text; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Internal; using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Routing; @@ -313,64 +314,7 @@ internal static void NormalizeRouteValuesForPage( } internal static bool CheckIsLocalUrl([NotNullWhen(true)] string? url) - { - if (string.IsNullOrEmpty(url)) - { - return false; - } - - // Allows "/" or "/foo" but not "//" or "/\". - if (url[0] == '/') - { - // url is exactly "/" - if (url.Length == 1) - { - return true; - } - - // url doesn't start with "//" or "/\" - if (url[1] != '/' && url[1] != '\\') - { - return !HasControlCharacter(url.AsSpan(1)); - } - - return false; - } - - // Allows "~/" or "~/foo" but not "~//" or "~/\". - if (url[0] == '~' && url.Length > 1 && url[1] == '/') - { - // url is exactly "~/" - if (url.Length == 2) - { - return true; - } - - // url doesn't start with "~//" or "~/\" - if (url[2] != '/' && url[2] != '\\') - { - return !HasControlCharacter(url.AsSpan(2)); - } - - return false; - } - - return false; - - static bool HasControlCharacter(ReadOnlySpan readOnlySpan) - { - // URLs may not contain ASCII control characters. - for (var i = 0; i < readOnlySpan.Length; i++) - { - if (char.IsControl(readOnlySpan[i])) - { - return true; - } - } - - return false; - } - } + => SharedUrlHelper.IsLocalUrl(url); private static object CalculatePageName(ActionContext? context, RouteValueDictionary? ambientValues, string pageName) { diff --git a/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs b/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs index ee4d694d74fe..bce83818b29b 100644 --- a/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs +++ b/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs @@ -7,6 +7,7 @@ using System.Text.Encodings.Web; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; +using Microsoft.AspNetCore.Internal; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -445,53 +446,10 @@ await Events.RedirectToReturnUrl( } } - // Mirror of Microsoft.AspNetCore.Internal.SharedUrlHelper.IsLocalUrl - // (src/Shared/ResultsHelpers/SharedUrlHelper.cs). Keep the two in sync. - // Intentional difference: cookie auth rejects "~/" paths because ApplyHeaders - // writes the value verbatim into the Location header and never resolves - // PathBase like IUrlHelper.Content does — so the "~/..." branch from - // IsLocalUrl is omitted here. private static bool IsHostRelative(string path) - { - if (string.IsNullOrEmpty(path)) - { - return false; - } - - // Allows "/" or "/foo" but not "//" or "/\". - if (path[0] == '/') - { - // path is exactly "/" - if (path.Length == 1) - { - return true; - } - - // path doesn't start with "//" or "/\" - if (path[1] != '/' && path[1] != '\\') - { - return !HasControlCharacter(path.AsSpan(1)); - } - - return false; - } - - return false; - - static bool HasControlCharacter(ReadOnlySpan readOnlySpan) - { - // URLs may not contain ASCII control characters. - for (var i = 0; i < readOnlySpan.Length; i++) - { - if (char.IsControl(readOnlySpan[i])) - { - return true; - } - } - - return false; - } - } + => !string.IsNullOrEmpty(path) + && path[0] == '/' // Suppresses the "~/..." branch of SharedUrlHelper.IsLocalUrl so only true host-relative paths are accepted. + && SharedUrlHelper.IsLocalUrl(path); /// protected override async Task HandleForbiddenAsync(AuthenticationProperties properties) diff --git a/src/Security/Authentication/Cookies/src/Microsoft.AspNetCore.Authentication.Cookies.csproj b/src/Security/Authentication/Cookies/src/Microsoft.AspNetCore.Authentication.Cookies.csproj index 6a3843e97683..254c5623a081 100644 --- a/src/Security/Authentication/Cookies/src/Microsoft.AspNetCore.Authentication.Cookies.csproj +++ b/src/Security/Authentication/Cookies/src/Microsoft.AspNetCore.Authentication.Cookies.csproj @@ -13,6 +13,7 @@ + diff --git a/src/Shared/ResultsHelpers/SharedUrlHelper.cs b/src/Shared/ResultsHelpers/SharedUrlHelper.cs index aa64d071bfef..e1c301378f6e 100644 --- a/src/Shared/ResultsHelpers/SharedUrlHelper.cs +++ b/src/Shared/ResultsHelpers/SharedUrlHelper.cs @@ -30,6 +30,10 @@ internal static class SharedUrlHelper return contentPath; } + // SECURITY: This is the open-redirect guard used by MVC URL helpers, Results.LocalRedirect, + // and CookieAuthenticationHandler. Changes to the accepted/rejected shapes (control characters, + // "//", "/\", "~/", etc.) must be reviewed against all call sites and the existing test corpus. + // Do not relax any check without security review. internal static bool IsLocalUrl([NotNullWhen(true)] string? url) { if (string.IsNullOrEmpty(url)) From e9c6095b9604dab978b86c28828b35f5945a12d3 Mon Sep 17 00:00:00 2001 From: Roman Konecny Date: Tue, 16 Jun 2026 19:45:46 +0200 Subject: [PATCH 3/3] Quarantine flaky InputDate E2E tests Re-quarantine InputDateInteractsWithEditContext_NonNullableDateTime (#31734), _NullableDateTimeOffset (#67243), and _TimeInput (#35018) due to recurring SendKeys keyboard-input timing flakiness in the components-e2e pipeline blocking this backport. --- src/Components/test/E2ETest/Tests/FormsInputDateTest.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Components/test/E2ETest/Tests/FormsInputDateTest.cs b/src/Components/test/E2ETest/Tests/FormsInputDateTest.cs index 0e034d019e6f..2cf0898a5a2a 100644 --- a/src/Components/test/E2ETest/Tests/FormsInputDateTest.cs +++ b/src/Components/test/E2ETest/Tests/FormsInputDateTest.cs @@ -34,6 +34,7 @@ protected override void InitializeAsyncCore() } [Fact] + [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/31734")] public void InputDateInteractsWithEditContext_NonNullableDateTime() { var appElement = Browser.MountTestComponent(); @@ -66,6 +67,7 @@ public void InputDateInteractsWithEditContext_NonNullableDateTime() } [Fact] + [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/67243")] public void InputDateInteractsWithEditContext_NullableDateTimeOffset() { var appElement = Browser.MountTestComponent(); @@ -89,6 +91,7 @@ public void InputDateInteractsWithEditContext_NullableDateTimeOffset() } [Fact] + [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/35018")] public void InputDateInteractsWithEditContext_TimeInput() { var appElement = Browser.MountTestComponent();