From ea8535bb4c96818654a8a74186e69ff13afc2a1e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 3 Oct 2026 13:23:58 +0000 Subject: [PATCH 1/4] fix(auth): prevent open redirect on OIDC login returnUrl Decode and strip control characters before accepting a return URL so encoded protocol-relative targets such as /%09/evil.com cannot bypass the local-path check. Co-authored-by: Sipke Schoorstra --- .../LocalReturnPath.cs | 84 +++++++++++++++++++ .../Controllers/AuthenticationController.cs | 18 +--- .../Components/LoginPanel.razor | 11 +-- .../_Imports.razor | 1 + .../ExternalAuthenticationReturnPath.cs | 17 +--- .../ExternalAuthenticationWasmTests.cs | 7 +- .../DirectOpenIdConnectLoginTests.cs | 22 ++++- .../Login/LocalReturnPathTests.cs | 30 +++++++ .../Login/LoginChooserTests.cs | 5 ++ .../Services/LoginMethodChooserState.cs | 12 +-- .../Controllers/CultureController.cs | 5 ++ .../Pages/Login/Login.razor.cs | 12 +-- .../OpenIdConnectAuthorizationService.cs | 20 ++++- 13 files changed, 182 insertions(+), 62 deletions(-) create mode 100644 src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs create mode 100644 src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs diff --git a/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs b/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs new file mode 100644 index 000000000..ec2b4c1a6 --- /dev/null +++ b/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs @@ -0,0 +1,84 @@ +using System.Text; + +namespace Elsa.Studio.Authentication.Abstractions; + +/// +/// Neutralizes open-redirect targets so post-authentication navigation stays inside the Studio host. +/// +public static class LocalReturnPath +{ + /// + /// Returns a rooted local path, or / when the candidate is missing, absolute, protocol-relative, or otherwise unsafe. + /// + public static string Normalize(string? candidate) + { + if (string.IsNullOrWhiteSpace(candidate)) + { + return "/"; + } + + var sanitized = DecodeAndStrip(candidate); + if (!IsSafeLocalPath(sanitized)) + { + return "/"; + } + + return sanitized; + } + + private static bool IsSafeLocalPath(string candidate) + { + if (string.IsNullOrEmpty(candidate) || + !candidate.StartsWith("/", StringComparison.Ordinal) || + candidate.StartsWith("//", StringComparison.Ordinal) || + candidate.Contains('\\') || + candidate.Contains("://", StringComparison.Ordinal)) + { + return false; + } + + return Uri.TryCreate(candidate, UriKind.Relative, out var uri) && !uri.IsAbsoluteUri; + } + + private static string DecodeAndStrip(string candidate) + { + var current = candidate.Trim(); + for (var i = 0; i < 8; i++) + { + string decoded; + try + { + decoded = Uri.UnescapeDataString(current); + } + catch (UriFormatException) + { + return string.Empty; + } + + if (decoded == current) + { + break; + } + + current = decoded; + } + + return StripControlCharacters(current.Trim()); + } + + private static string StripControlCharacters(string value) + { + var builder = new StringBuilder(value.Length); + foreach (var ch in value) + { + if (char.IsControl(ch)) + { + continue; + } + + builder.Append(ch); + } + + return builder.ToString(); + } +} diff --git a/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs b/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs index b75d0e602..602b2843f 100644 --- a/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs +++ b/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs @@ -1,3 +1,4 @@ +using Elsa.Studio.Authentication.Abstractions; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Authentication.OpenIdConnect; @@ -17,7 +18,7 @@ public class AuthenticationController : Controller [HttpGet("login")] public IActionResult Login([FromQuery] string? returnUrl = null) { - return Challenge(new AuthenticationProperties { RedirectUri = NormalizeReturnUrl(returnUrl) }, OpenIdConnectDefaults.AuthenticationScheme); + return Challenge(new AuthenticationProperties { RedirectUri = LocalReturnPath.Normalize(returnUrl) }, OpenIdConnectDefaults.AuthenticationScheme); } /// @@ -31,21 +32,8 @@ public IActionResult Login([FromQuery] string? returnUrl = null) public IActionResult Logout([FromForm] string? returnUrl = null) { return SignOut( - new AuthenticationProperties { RedirectUri = NormalizeReturnUrl(returnUrl) }, + new AuthenticationProperties { RedirectUri = LocalReturnPath.Normalize(returnUrl) }, CookieAuthenticationDefaults.AuthenticationScheme, OpenIdConnectDefaults.AuthenticationScheme); } - - private static string NormalizeReturnUrl(string? candidate) - { - if (string.IsNullOrWhiteSpace(candidate) || - !candidate.StartsWith("/", StringComparison.Ordinal) || - candidate.StartsWith("//", StringComparison.Ordinal) || - candidate.StartsWith("/\\", StringComparison.Ordinal)) - { - return "/"; - } - - return Uri.TryCreate(candidate, UriKind.Relative, out _) ? candidate : "/"; - } } diff --git a/src/modules/Elsa.Studio.Authentication.UI/Components/LoginPanel.razor b/src/modules/Elsa.Studio.Authentication.UI/Components/LoginPanel.razor index e8c2d00eb..bae9513dd 100644 --- a/src/modules/Elsa.Studio.Authentication.UI/Components/LoginPanel.razor +++ b/src/modules/Elsa.Studio.Authentication.UI/Components/LoginPanel.razor @@ -199,14 +199,5 @@ string.Equals(method.Kind, "local", StringComparison.OrdinalIgnoreCase) || string.Equals(method.Kind, "elsa-identity", StringComparison.OrdinalIgnoreCase); - private static string NormalizeReturnPath(string? candidate) - { - if (string.IsNullOrWhiteSpace(candidate) || - !candidate.StartsWith("/", StringComparison.Ordinal) || - candidate.StartsWith("//", StringComparison.Ordinal) || - candidate.Contains('\\')) - return "/"; - - return Uri.TryCreate(candidate, UriKind.Relative, out _) ? candidate : "/"; - } + private static string NormalizeReturnPath(string? candidate) => LocalReturnPath.Normalize(candidate); } diff --git a/src/modules/Elsa.Studio.Authentication.UI/_Imports.razor b/src/modules/Elsa.Studio.Authentication.UI/_Imports.razor index 7de5c8304..814baa6b0 100644 --- a/src/modules/Elsa.Studio.Authentication.UI/_Imports.razor +++ b/src/modules/Elsa.Studio.Authentication.UI/_Imports.razor @@ -1,3 +1,4 @@ +@using Elsa.Studio.Authentication.Abstractions @using Elsa.Studio.Authentication.Abstractions.Contracts @using Elsa.Studio.Authentication.Abstractions.Models @using Elsa.Studio.Authentication.UI.Components diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.BlazorWasm/Services/ExternalAuthenticationReturnPath.cs b/src/modules/Elsa.Studio.ExternalAuthentication.BlazorWasm/Services/ExternalAuthenticationReturnPath.cs index 9e09d0f18..9f94f694d 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.BlazorWasm/Services/ExternalAuthenticationReturnPath.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.BlazorWasm/Services/ExternalAuthenticationReturnPath.cs @@ -1,21 +1,10 @@ +using Elsa.Studio.Authentication.Abstractions; + namespace Elsa.Studio.ExternalAuthentication.BlazorWasm.Services; /// Validates targets which remain inside the registered Studio client. public static class ExternalAuthenticationReturnPath { /// Returns a safe client-local path, falling back to the Studio root. - public static string Normalize(string? value) - { - if (string.IsNullOrWhiteSpace(value) || - !value.StartsWith("/", StringComparison.Ordinal) || - value.StartsWith("//", StringComparison.Ordinal) || - value.Contains("\\", StringComparison.Ordinal) || - !Uri.TryCreate(value, UriKind.Relative, out var uri) || - uri.IsAbsoluteUri) - { - return "/"; - } - - return value; - } + public static string Normalize(string? value) => LocalReturnPath.Normalize(value); } diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorWasm/ExternalAuthenticationWasmTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorWasm/ExternalAuthenticationWasmTests.cs index ffc25c7a4..9f974aad3 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorWasm/ExternalAuthenticationWasmTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorWasm/ExternalAuthenticationWasmTests.cs @@ -19,9 +19,12 @@ public class ExternalAuthenticationWasmTests [InlineData(null, "/")] [InlineData("", "/")] [InlineData("/workflows?version=1", "/workflows?version=1")] + [InlineData("/workflows/definitions?x=1", "/workflows/definitions?x=1")] [InlineData("https://evil.example/", "/")] - [InlineData("//evil.example/", "/")] - [InlineData("/\\evil", "/")] + [InlineData("//evil.com", "/")] + [InlineData("/\\evil.com", "/")] + [InlineData("/%09/evil.com", "/")] + [InlineData("/%2F/evil.com", "/")] public void ReturnPathsRemainClientLocal(string? candidate, string expected) => Assert.Equal(expected, ExternalAuthenticationReturnPath.Normalize(candidate)); diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs index e0f5e4beb..6119891b8 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs @@ -20,9 +20,14 @@ public async Task DirectOpenIdConnectContributesItsLegacyChallengeToTheSharedLog [Theory] [InlineData("https://attacker.example", "/")] - [InlineData("//attacker.example", "/")] - [InlineData("/\\attacker.example", "/")] + [InlineData("http://attacker.example", "/")] + [InlineData("//evil.com", "/")] + [InlineData("/\\evil.com", "/")] + [InlineData("\\\\evil.com", "/")] + [InlineData("/%09/evil.com", "/")] + [InlineData("/%2F/evil.com", "/")] [InlineData("/workflows?version=1", "/workflows?version=1")] + [InlineData("/workflows/definitions?x=1", "/workflows/definitions?x=1")] public void DirectOpenIdConnectChallengeAcceptsOnlyLocalReturnUrls(string returnUrl, string expectedReturnUrl) { var result = Assert.IsType(new AuthenticationController().Login(returnUrl)); @@ -30,4 +35,17 @@ public void DirectOpenIdConnectChallengeAcceptsOnlyLocalReturnUrls(string return Assert.NotNull(result.Properties); Assert.Equal(expectedReturnUrl, result.Properties!.RedirectUri); } + + [Theory] + [InlineData("/%09/evil.com", "/")] + [InlineData("//evil.com", "/")] + [InlineData("/\\evil.com", "/")] + [InlineData("/workflows/definitions?x=1", "/workflows/definitions?x=1")] + public void DirectOpenIdConnectSignOutAcceptsOnlyLocalReturnUrls(string returnUrl, string expectedReturnUrl) + { + var result = Assert.IsType(new AuthenticationController().Logout(returnUrl)); + + Assert.NotNull(result.Properties); + Assert.Equal(expectedReturnUrl, result.Properties!.RedirectUri); + } } diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs new file mode 100644 index 000000000..46da54414 --- /dev/null +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs @@ -0,0 +1,30 @@ +using Elsa.Studio.Authentication.Abstractions; +using Xunit; + +namespace Elsa.Studio.ExternalAuthentication.Tests.Login; + +public class LocalReturnPathTests +{ + [Theory] + [InlineData("/%09/evil.com", "/")] + [InlineData("//evil.com", "/")] + [InlineData("/\\evil.com", "/")] + [InlineData("\\\\evil.com", "/")] + [InlineData("/%2F/evil.com", "/")] + [InlineData("http://evil.com", "/")] + [InlineData("https://evil.com/phish", "/")] + [InlineData("/workflows/definitions?x=1", "/workflows/definitions?x=1")] + public void Normalize_RejectsOpenRedirectsAndPreservesLocalPaths(string? candidate, string expected) + { + Assert.Equal(expected, LocalReturnPath.Normalize(candidate)); + Assert.Equal(expected, Elsa.Studio.ExternalAuthentication.Services.LocalReturnPath.Normalize(candidate)); + } + + [Fact] + public void Normalize_RejectsDecodedControlCharactersThatCreateProtocolRelativeUrls() + { + Assert.Equal("/", LocalReturnPath.Normalize("/\t/evil.com")); + Assert.Equal("/", LocalReturnPath.Normalize("/\r/evil.com")); + Assert.Equal("/", LocalReturnPath.Normalize("/\n/evil.com")); + } +} diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LoginChooserTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LoginChooserTests.cs index e55db50b1..1ce1c855c 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LoginChooserTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LoginChooserTests.cs @@ -110,7 +110,12 @@ public void InvalidReturnPaths_AreNeverForwarded() Assert.Equal("/", LocalReturnPath.Normalize("https://attacker.example")); Assert.Equal("/", LocalReturnPath.Normalize("//attacker.example")); Assert.Equal("/", LocalReturnPath.Normalize("/\\attacker.example")); + Assert.Equal("/", LocalReturnPath.Normalize("/%09/evil.com")); + Assert.Equal("/", LocalReturnPath.Normalize("/%2F/evil.com")); + Assert.Equal("/", LocalReturnPath.Normalize("//evil.com")); + Assert.Equal("/", LocalReturnPath.Normalize("/\\evil.com")); Assert.Equal("/workflows?tab=active", LocalReturnPath.Normalize("/workflows?tab=active")); + Assert.Equal("/workflows/definitions?x=1", LocalReturnPath.Normalize("/workflows/definitions?x=1")); } [Fact] diff --git a/src/modules/Elsa.Studio.ExternalAuthentication/Services/LoginMethodChooserState.cs b/src/modules/Elsa.Studio.ExternalAuthentication/Services/LoginMethodChooserState.cs index aefc2dc25..b0607ffe7 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication/Services/LoginMethodChooserState.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication/Services/LoginMethodChooserState.cs @@ -15,14 +15,6 @@ public static IReadOnlyList Order(IEnumerableGuards local post-authentication paths before a host constructs broker requests. public static class LocalReturnPath { - public static string Normalize(string? candidate) - { - if (string.IsNullOrWhiteSpace(candidate) || - !candidate.StartsWith("/", StringComparison.Ordinal) || - candidate.StartsWith("//", StringComparison.Ordinal) || - candidate.Contains('\\')) - return "/"; - - return Uri.TryCreate(candidate, UriKind.Relative, out _) ? candidate : "/"; - } + public static string Normalize(string? candidate) => + Authentication.Abstractions.LocalReturnPath.Normalize(candidate); } diff --git a/src/modules/Elsa.Studio.Localization.BlazorServer/Controllers/CultureController.cs b/src/modules/Elsa.Studio.Localization.BlazorServer/Controllers/CultureController.cs index b0390b6f6..4dae857f4 100644 --- a/src/modules/Elsa.Studio.Localization.BlazorServer/Controllers/CultureController.cs +++ b/src/modules/Elsa.Studio.Localization.BlazorServer/Controllers/CultureController.cs @@ -25,6 +25,11 @@ public IActionResult Set(string? culture, string redirectUri) new RequestCulture(culture, culture))); } + if (!Url.IsLocalUrl(redirectUri)) + { + return LocalRedirect("~/"); + } + return LocalRedirect(redirectUri); } } \ No newline at end of file diff --git a/src/modules/Elsa.Studio.Login/Pages/Login/Login.razor.cs b/src/modules/Elsa.Studio.Login/Pages/Login/Login.razor.cs index 1cf3c4cb5..5b9b7c27c 100644 --- a/src/modules/Elsa.Studio.Login/Pages/Login/Login.razor.cs +++ b/src/modules/Elsa.Studio.Login/Pages/Login/Login.razor.cs @@ -1,3 +1,4 @@ +using Elsa.Studio.Authentication.Abstractions; using Elsa.Studio.Contracts; using Elsa.Studio.Login.Contracts; using Elsa.Studio.Login.Services; @@ -49,14 +50,13 @@ private async Task TryLogin(LoginArgs args) ((AccessTokenAuthenticationStateProvider)AuthenticationStateProvider).NotifyAuthenticationStateChanged(); var uri = new Uri(NavigationManager.Uri); - if (QueryHelpers.ParseQuery(uri.Query).TryGetValue("returnUrl", out var returnUrl)) + string? returnUrl = null; + if (QueryHelpers.ParseQuery(uri.Query).TryGetValue("returnUrl", out var values)) { - NavigationManager.NavigateTo(returnUrl.FirstOrDefault() ?? "", true); - } - else - { - NavigationManager.NavigateTo("", true); + returnUrl = values.FirstOrDefault(); } + + NavigationManager.NavigateTo(LocalReturnPath.Normalize(returnUrl), true); } private async Task ValidateCredentials(string? username, string? password) diff --git a/src/modules/Elsa.Studio.Login/Services/OpenIdConnectAuthorizationService.cs b/src/modules/Elsa.Studio.Login/Services/OpenIdConnectAuthorizationService.cs index 1c09e3f84..ca9d02225 100644 --- a/src/modules/Elsa.Studio.Login/Services/OpenIdConnectAuthorizationService.cs +++ b/src/modules/Elsa.Studio.Login/Services/OpenIdConnectAuthorizationService.cs @@ -1,4 +1,5 @@ -using Elsa.Studio.Login.Contracts; +using Elsa.Studio.Authentication.Abstractions; +using Elsa.Studio.Login.Contracts; using Elsa.Studio.Login.Models; using Microsoft.AspNetCore.Components; using Microsoft.AspNetCore.WebUtilities; @@ -79,10 +80,11 @@ public async Task ReceiveAuthorizationCode(string code, string? state, Cancellat await jwtAccessor.WriteTokenAsync(TokenNames.IdToken, tokens.IdToken ?? ""); string returnUrl = "/"; - if (!String.IsNullOrWhiteSpace(state)) + if (!string.IsNullOrWhiteSpace(state)) { - returnUrl = Encoding.UTF8.GetString(WebEncoders.Base64UrlDecode(state)); + returnUrl = NormalizeStateReturnUrl(Encoding.UTF8.GetString(WebEncoders.Base64UrlDecode(state))); } + navigationManager.NavigateTo(returnUrl, true); } @@ -110,6 +112,18 @@ private async Task ThrowTokenExchangeExceptionAsync(HttpResponseMessage response throw new HttpRequestException(message, null, response.StatusCode); } + private static string NormalizeStateReturnUrl(string candidate) + { + if (!candidate.StartsWith('/') && + !candidate.StartsWith('\\') && + candidate.IndexOf("://", StringComparison.Ordinal) < 0) + { + candidate = "/" + candidate; + } + + return LocalReturnPath.Normalize(candidate); + } + private static string GetCorrelationIds(HttpResponseMessage response) { var headers = CorrelationIdHeaderNames From 7ea154f5d411ee473d3d24500a459d2460bddcd9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 3 Oct 2026 13:48:21 +0000 Subject: [PATCH 2/4] fix(auth): preserve original return URLs and PathBase Return the original local path after validating a decoded copy, accept scheme-less relative ElsaLogin destinations, fail closed on over-encoded protocol-relative URLs, and keep PathBase in the legacy OIDC state helper. Co-authored-by: Sipke Schoorstra --- .../LocalReturnPath.cs | 108 +++++++++++------- .../Controllers/AuthenticationController.cs | 15 ++- .../ExternalAuthenticationWasmTests.cs | 11 +- .../DirectOpenIdConnectLoginTests.cs | 38 +++--- ...Studio.ExternalAuthentication.Tests.csproj | 1 + .../Login/LocalReturnPathCorpus.cs | 66 +++++++++++ .../Login/LocalReturnPathTests.cs | 26 +++-- .../Login/LoginChooserTests.cs | 17 +-- .../Elsa.Studio.Login.csproj | 4 + .../OpenIdConnectAuthorizationService.cs | 23 ++-- 10 files changed, 205 insertions(+), 104 deletions(-) create mode 100644 src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathCorpus.cs diff --git a/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs b/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs index ec2b4c1a6..7324a72d8 100644 --- a/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs +++ b/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs @@ -1,5 +1,3 @@ -using System.Text; - namespace Elsa.Studio.Authentication.Abstractions; /// @@ -7,78 +5,102 @@ namespace Elsa.Studio.Authentication.Abstractions; /// public static class LocalReturnPath { + private const int MaxDecodePasses = 8; + /// - /// Returns a rooted local path, or / when the candidate is missing, absolute, protocol-relative, or otherwise unsafe. + /// Returns the original candidate when it is a local destination, or / when it is missing, absolute, protocol-relative, or otherwise unsafe. /// + /// + /// Validation runs on a decoded copy. The returned value is the original string so encoded query values and path segments stay byte-for-byte intact. + /// Scheme-less relative paths such as workflows/x are treated as local after prefixing / for the check only. + /// Decoding that does not reach a fixed point within fails closed to /. + /// public static string Normalize(string? candidate) { - if (string.IsNullOrWhiteSpace(candidate)) + if (string.IsNullOrEmpty(candidate)) { return "/"; } - var sanitized = DecodeAndStrip(candidate); - if (!IsSafeLocalPath(sanitized)) + var decoded = candidate; + for (var pass = 0; pass < MaxDecodePasses; pass++) { - return "/"; + string next; + try + { + next = Uri.UnescapeDataString(decoded); + } + catch (UriFormatException) + { + return "/"; + } + + if (next == decoded) + { + return IsLocalDestination(candidate) && IsLocalDestination(decoded) ? candidate : "/"; + } + + decoded = next; } - return sanitized; + return "/"; } - private static bool IsSafeLocalPath(string candidate) + private static bool IsLocalDestination(string path) { - if (string.IsNullOrEmpty(candidate) || - !candidate.StartsWith("/", StringComparison.Ordinal) || - candidate.StartsWith("//", StringComparison.Ordinal) || - candidate.Contains('\\') || - candidate.Contains("://", StringComparison.Ordinal)) + if (IsRootedLocalPath(path)) + { + return true; + } + + if (path.Length == 0 || char.IsWhiteSpace(path[0]) || HasSchemeOrHost(path)) { return false; } - return Uri.TryCreate(candidate, UriKind.Relative, out var uri) && !uri.IsAbsoluteUri; + return IsRootedLocalPath("/" + path); } - private static string DecodeAndStrip(string candidate) + private static bool IsRootedLocalPath(string path) => + path.StartsWith('/') && + !path.StartsWith("//", StringComparison.Ordinal) && + !path.Any(c => c == '\\' || char.IsControl(c)); + + private static bool HasSchemeOrHost(string path) { - var current = candidate.Trim(); - for (var i = 0; i < 8; i++) + if (path.StartsWith("//", StringComparison.Ordinal) || + path.StartsWith('\\') || + path.Contains('\\')) { - string decoded; - try - { - decoded = Uri.UnescapeDataString(current); - } - catch (UriFormatException) - { - return string.Empty; - } + return true; + } - if (decoded == current) - { - break; - } + var schemeSeparator = path.IndexOf(':'); + if (schemeSeparator <= 0) + { + return false; + } - current = decoded; + var queryStart = path.IndexOf('?'); + if (queryStart >= 0 && schemeSeparator > queryStart) + { + return false; } - return StripControlCharacters(current.Trim()); - } + if (!char.IsAsciiLetter(path[0])) + { + return false; + } - private static string StripControlCharacters(string value) - { - var builder = new StringBuilder(value.Length); - foreach (var ch in value) + for (var i = 1; i < schemeSeparator; i++) { - if (char.IsControl(ch)) + var c = path[i]; + if (!char.IsAsciiLetterOrDigit(c) && c != '+' && c != '-' && c != '.') { - continue; + return false; } - - builder.Append(ch); } - return builder.ToString(); + return true; } } diff --git a/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs b/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs index 602b2843f..b40b3a42e 100644 --- a/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs +++ b/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs @@ -18,7 +18,7 @@ public class AuthenticationController : Controller [HttpGet("login")] public IActionResult Login([FromQuery] string? returnUrl = null) { - return Challenge(new AuthenticationProperties { RedirectUri = LocalReturnPath.Normalize(returnUrl) }, OpenIdConnectDefaults.AuthenticationScheme); + return Challenge(new AuthenticationProperties { RedirectUri = SafeRedirectUri(returnUrl) }, OpenIdConnectDefaults.AuthenticationScheme); } /// @@ -32,8 +32,19 @@ public IActionResult Login([FromQuery] string? returnUrl = null) public IActionResult Logout([FromForm] string? returnUrl = null) { return SignOut( - new AuthenticationProperties { RedirectUri = LocalReturnPath.Normalize(returnUrl) }, + new AuthenticationProperties { RedirectUri = SafeRedirectUri(returnUrl) }, CookieAuthenticationDefaults.AuthenticationScheme, OpenIdConnectDefaults.AuthenticationScheme); } + + private string SafeRedirectUri(string? returnUrl) + { + var path = LocalReturnPath.Normalize(returnUrl); + if (Url?.IsLocalUrl(path) == false) + { + return "/"; + } + + return path; + } } diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorWasm/ExternalAuthenticationWasmTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorWasm/ExternalAuthenticationWasmTests.cs index 9f974aad3..47819809d 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorWasm/ExternalAuthenticationWasmTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorWasm/ExternalAuthenticationWasmTests.cs @@ -6,6 +6,7 @@ using Elsa.Studio.ExternalAuthentication.BlazorWasm.Services; using Elsa.Studio.ExternalAuthentication.Client; using Elsa.Studio.ExternalAuthentication.Models; +using Elsa.Studio.ExternalAuthentication.Tests.Login; using Microsoft.AspNetCore.Components; using Microsoft.Extensions.DependencyInjection; using Microsoft.JSInterop; @@ -16,15 +17,7 @@ namespace Elsa.Studio.ExternalAuthentication.Tests.BlazorWasm; public class ExternalAuthenticationWasmTests { [Theory] - [InlineData(null, "/")] - [InlineData("", "/")] - [InlineData("/workflows?version=1", "/workflows?version=1")] - [InlineData("/workflows/definitions?x=1", "/workflows/definitions?x=1")] - [InlineData("https://evil.example/", "/")] - [InlineData("//evil.com", "/")] - [InlineData("/\\evil.com", "/")] - [InlineData("/%09/evil.com", "/")] - [InlineData("/%2F/evil.com", "/")] + [MemberData(nameof(LocalReturnPathCorpus.Cases), MemberType = typeof(LocalReturnPathCorpus))] public void ReturnPathsRemainClientLocal(string? candidate, string expected) => Assert.Equal(expected, ExternalAuthenticationReturnPath.Normalize(candidate)); diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs index 6119891b8..1f7e74a9e 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs @@ -1,5 +1,7 @@ using Elsa.Studio.Authentication.OpenIdConnect.BlazorServer.Controllers; using Elsa.Studio.Authentication.OpenIdConnect.BlazorServer.Services; +using Elsa.Studio.ExternalAuthentication.Tests.Login; +using Elsa.Studio.Login.Services; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Mvc; using Xunit; @@ -19,16 +21,8 @@ public async Task DirectOpenIdConnectContributesItsLegacyChallengeToTheSharedLog } [Theory] - [InlineData("https://attacker.example", "/")] - [InlineData("http://attacker.example", "/")] - [InlineData("//evil.com", "/")] - [InlineData("/\\evil.com", "/")] - [InlineData("\\\\evil.com", "/")] - [InlineData("/%09/evil.com", "/")] - [InlineData("/%2F/evil.com", "/")] - [InlineData("/workflows?version=1", "/workflows?version=1")] - [InlineData("/workflows/definitions?x=1", "/workflows/definitions?x=1")] - public void DirectOpenIdConnectChallengeAcceptsOnlyLocalReturnUrls(string returnUrl, string expectedReturnUrl) + [MemberData(nameof(LocalReturnPathCorpus.Cases), MemberType = typeof(LocalReturnPathCorpus))] + public void DirectOpenIdConnectChallengeAcceptsOnlyLocalReturnUrls(string? returnUrl, string expectedReturnUrl) { var result = Assert.IsType(new AuthenticationController().Login(returnUrl)); @@ -37,15 +31,29 @@ public void DirectOpenIdConnectChallengeAcceptsOnlyLocalReturnUrls(string return } [Theory] - [InlineData("/%09/evil.com", "/")] - [InlineData("//evil.com", "/")] - [InlineData("/\\evil.com", "/")] - [InlineData("/workflows/definitions?x=1", "/workflows/definitions?x=1")] - public void DirectOpenIdConnectSignOutAcceptsOnlyLocalReturnUrls(string returnUrl, string expectedReturnUrl) + [MemberData(nameof(LocalReturnPathCorpus.Cases), MemberType = typeof(LocalReturnPathCorpus))] + public void DirectOpenIdConnectSignOutAcceptsOnlyLocalReturnUrls(string? returnUrl, string expectedReturnUrl) { var result = Assert.IsType(new AuthenticationController().Logout(returnUrl)); Assert.NotNull(result.Properties); Assert.Equal(expectedReturnUrl, result.Properties!.RedirectUri); } + + [Fact] + public void DirectOpenIdConnectChallengeRejectsBoundControlCharacterReturnUrls() + { + var result = Assert.IsType(new AuthenticationController().Login("/\t/evil.com")); + + Assert.NotNull(result.Properties); + Assert.Equal("/", result.Properties!.RedirectUri); + } + + [Fact] + public void LegacyOidcStateHelperKeepsPathBase() + { + var current = "https://studio.example/elsa/studio/workflows/definitions?x=1"; + + Assert.Equal("/elsa/studio/workflows/definitions?x=1", OpenIdConnectAuthorizationService.CaptureReturnPath(current)); + } } diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Elsa.Studio.ExternalAuthentication.Tests.csproj b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Elsa.Studio.ExternalAuthentication.Tests.csproj index 2db6e343f..fee32aa58 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Elsa.Studio.ExternalAuthentication.Tests.csproj +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Elsa.Studio.ExternalAuthentication.Tests.csproj @@ -36,6 +36,7 @@ + diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathCorpus.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathCorpus.cs new file mode 100644 index 000000000..4b8b31754 --- /dev/null +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathCorpus.cs @@ -0,0 +1,66 @@ +using Xunit; + +namespace Elsa.Studio.ExternalAuthentication.Tests.Login; + +/// +/// Shared table-driven return-path cases used by LocalReturnPath, OIDC, WASM, and chooser tests. +/// +public static class LocalReturnPathCorpus +{ + public static TheoryData Cases { get; } = CreateCases(); + + public static string EncodeTimes(string value, int times) + { + var current = value; + for (var i = 0; i < times; i++) + { + current = Uri.EscapeDataString(current); + } + + return current; + } + + private static TheoryData CreateCases() + { + var cases = new TheoryData + { + { null, "/" }, + { "", "/" }, + { "/%09/evil.com", "/" }, + { "//evil.com", "/" }, + { "/\\evil.com", "/" }, + { "\\\\evil.com", "/" }, + { "/%2F/evil.com", "/" }, + { "http://evil.com", "/" }, + { "https://evil.com/phish", "/" }, + { "https://attacker.example", "/" }, + { "http://attacker.example", "/" }, + { "https://evil.example/", "/" }, + { "//attacker.example", "/" }, + { "/\\attacker.example", "/" }, + { "javascript:alert(1)", "/" }, + { " //evil.com", "/" }, + { "/workflows/definitions?x=1", "/workflows/definitions?x=1" }, + { "/workflows?version=1", "/workflows?version=1" }, + { "/workflows?tab=active", "/workflows?tab=active" }, + { "/caf%C3%A9", "/caf%C3%A9" }, + { "/a?x=%26y", "/a?x=%26y" }, + { "/a?ref=https%3A%2F%2Fexample.com", "/a?ref=https%3A%2F%2Fexample.com" }, + { "/a%2Fb", "/a%2Fb" }, + { "/a?ref=https://example.com", "/a?ref=https://example.com" }, + { "/workflows?documentation=https://example.com", "/workflows?documentation=https://example.com" }, + { "/workflows?filter=red%26blue", "/workflows?filter=red%26blue" }, + { "/workflows?name=a%26b", "/workflows?name=a%26b" }, + { "/workflows?filter=a%26b", "/workflows?filter=a%26b" }, + { "workflows/x", "workflows/x" }, + { "workflows/definitions?tab=active", "workflows/definitions?tab=active" }, + { "workflows/instances", "workflows/instances" } + }; + + cases.Add(EncodeTimes("//evil.com", 9), "/"); + cases.Add(EncodeTimes("//", 9), "/"); + cases.Add("/" + EncodeTimes("//evil.com", 9), "/"); + + return cases; + } +} diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs index 46da54414..e32b1c935 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs @@ -6,14 +6,7 @@ namespace Elsa.Studio.ExternalAuthentication.Tests.Login; public class LocalReturnPathTests { [Theory] - [InlineData("/%09/evil.com", "/")] - [InlineData("//evil.com", "/")] - [InlineData("/\\evil.com", "/")] - [InlineData("\\\\evil.com", "/")] - [InlineData("/%2F/evil.com", "/")] - [InlineData("http://evil.com", "/")] - [InlineData("https://evil.com/phish", "/")] - [InlineData("/workflows/definitions?x=1", "/workflows/definitions?x=1")] + [MemberData(nameof(LocalReturnPathCorpus.Cases), MemberType = typeof(LocalReturnPathCorpus))] public void Normalize_RejectsOpenRedirectsAndPreservesLocalPaths(string? candidate, string expected) { Assert.Equal(expected, LocalReturnPath.Normalize(candidate)); @@ -27,4 +20,21 @@ public void Normalize_RejectsDecodedControlCharactersThatCreateProtocolRelativeU Assert.Equal("/", LocalReturnPath.Normalize("/\r/evil.com")); Assert.Equal("/", LocalReturnPath.Normalize("/\n/evil.com")); } + + [Fact] + public void Normalize_PreservesEncodedLinksByteForByte() + { + Assert.Equal("/caf%C3%A9", LocalReturnPath.Normalize("/caf%C3%A9")); + Assert.Equal("/a?x=%26y", LocalReturnPath.Normalize("/a?x=%26y")); + Assert.Equal("/a?ref=https%3A%2F%2Fexample.com", LocalReturnPath.Normalize("/a?ref=https%3A%2F%2Fexample.com")); + Assert.Equal("/a%2Fb", LocalReturnPath.Normalize("/a%2Fb")); + Assert.Equal("/a?ref=https://example.com", LocalReturnPath.Normalize("/a?ref=https://example.com")); + } + + [Fact] + public void Normalize_RejectsNineTimesEncodedProtocolRelativeUrls() + { + Assert.Equal("/", LocalReturnPath.Normalize(LocalReturnPathCorpus.EncodeTimes("//evil.com", 9))); + Assert.Equal("/", LocalReturnPath.Normalize("/" + LocalReturnPathCorpus.EncodeTimes("//evil.com", 9))); + } } diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LoginChooserTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LoginChooserTests.cs index 1ce1c855c..a616a6740 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LoginChooserTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LoginChooserTests.cs @@ -104,19 +104,10 @@ public void TrustedIconRegistry_UsesOnlyRegistrationsAndFallsBackSafely() Assert.Equal("Identity provider", registry.Resolve("https://untrusted.example/icon.svg").AccessibleName); } - [Fact] - public void InvalidReturnPaths_AreNeverForwarded() - { - Assert.Equal("/", LocalReturnPath.Normalize("https://attacker.example")); - Assert.Equal("/", LocalReturnPath.Normalize("//attacker.example")); - Assert.Equal("/", LocalReturnPath.Normalize("/\\attacker.example")); - Assert.Equal("/", LocalReturnPath.Normalize("/%09/evil.com")); - Assert.Equal("/", LocalReturnPath.Normalize("/%2F/evil.com")); - Assert.Equal("/", LocalReturnPath.Normalize("//evil.com")); - Assert.Equal("/", LocalReturnPath.Normalize("/\\evil.com")); - Assert.Equal("/workflows?tab=active", LocalReturnPath.Normalize("/workflows?tab=active")); - Assert.Equal("/workflows/definitions?x=1", LocalReturnPath.Normalize("/workflows/definitions?x=1")); - } + [Theory] + [MemberData(nameof(LocalReturnPathCorpus.Cases), MemberType = typeof(LocalReturnPathCorpus))] + public void InvalidReturnPaths_AreNeverForwarded(string? candidate, string expected) => + Assert.Equal(expected, LocalReturnPath.Normalize(candidate)); [Fact] public void Chooser_RendersTextFirstLocalAndExternalMethods() diff --git a/src/modules/Elsa.Studio.Login/Elsa.Studio.Login.csproj b/src/modules/Elsa.Studio.Login/Elsa.Studio.Login.csproj index 0c53b8c1a..1c1f7eda6 100644 --- a/src/modules/Elsa.Studio.Login/Elsa.Studio.Login.csproj +++ b/src/modules/Elsa.Studio.Login/Elsa.Studio.Login.csproj @@ -24,4 +24,8 @@ + + + + diff --git a/src/modules/Elsa.Studio.Login/Services/OpenIdConnectAuthorizationService.cs b/src/modules/Elsa.Studio.Login/Services/OpenIdConnectAuthorizationService.cs index ca9d02225..7bed43d85 100644 --- a/src/modules/Elsa.Studio.Login/Services/OpenIdConnectAuthorizationService.cs +++ b/src/modules/Elsa.Studio.Login/Services/OpenIdConnectAuthorizationService.cs @@ -29,7 +29,8 @@ public async Task RedirectToAuthorizationServer() var generated = await pkceStateService.GeneratePkceCodeChallenge(); url += $"&code_challenge={generated.CodeChallenge}&code_challenge_method={generated.Method}"; } - if (navigationManager.ToBaseRelativePath(navigationManager.Uri) is { } returnUrl and not "/") + var returnUrl = CaptureReturnPath(navigationManager.Uri); + if (returnUrl is not "/") { url += "&state=" + WebEncoders.Base64UrlEncode(Encoding.UTF8.GetBytes(returnUrl)); } @@ -82,12 +83,18 @@ public async Task ReceiveAuthorizationCode(string code, string? state, Cancellat string returnUrl = "/"; if (!string.IsNullOrWhiteSpace(state)) { - returnUrl = NormalizeStateReturnUrl(Encoding.UTF8.GetString(WebEncoders.Base64UrlDecode(state))); + returnUrl = LocalReturnPath.Normalize(Encoding.UTF8.GetString(WebEncoders.Base64UrlDecode(state))); } navigationManager.NavigateTo(returnUrl, true); } + /// + /// Captures the current request's path, including PathBase, so a hosted Studio does not drop its sub-path after OIDC sign-in. + /// + internal static string CaptureReturnPath(string uri) => + LocalReturnPath.Normalize(new Uri(uri).PathAndQuery); + private async Task ThrowTokenExchangeExceptionAsync(HttpResponseMessage response, OpenIdConnectConfiguration config, CancellationToken cancellationToken) { var errorSummary = await ReadErrorSummaryAsync(response.Content, cancellationToken); @@ -112,18 +119,6 @@ private async Task ThrowTokenExchangeExceptionAsync(HttpResponseMessage response throw new HttpRequestException(message, null, response.StatusCode); } - private static string NormalizeStateReturnUrl(string candidate) - { - if (!candidate.StartsWith('/') && - !candidate.StartsWith('\\') && - candidate.IndexOf("://", StringComparison.Ordinal) < 0) - { - candidate = "/" + candidate; - } - - return LocalReturnPath.Normalize(candidate); - } - private static string GetCorrelationIds(HttpResponseMessage response) { var headers = CorrelationIdHeaderNames From cd6f22eb6b201691131ef1fd87af9eb3462e7eb0 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 3 Oct 2026 14:15:10 +0000 Subject: [PATCH 3/4] fix(auth): accept only rooted local return paths Normalize now returns only a single-slash rooted path or /. Legacy ElsaLogin and ElsaIdentityLoginMethod root base-relative values against NavigationManager.BaseUri so PathBase is kept. Controller tests use a real UrlHelper so Url.IsLocalUrl always runs. Co-authored-by: Sipke Schoorstra --- .../LocalReturnPath.cs | 76 ++++--------------- .../Components/ElsaIdentityLoginMethod.razor | 2 +- .../Controllers/AuthenticationController.cs | 2 +- .../Components/LoginPanel.razor | 3 +- .../ServerBrokerAuthenticationTests.cs | 56 ++++++++++++++ .../DirectOpenIdConnectLoginTests.cs | 20 ++++- .../Login/LocalReturnPathCorpus.cs | 24 +++--- .../Login/LocalReturnPathTests.cs | 21 ++--- .../Pages/Login/Login.razor.cs | 8 +- 9 files changed, 121 insertions(+), 91 deletions(-) diff --git a/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs b/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs index 7324a72d8..3bb344457 100644 --- a/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs +++ b/src/modules/Elsa.Studio.Authentication.Abstractions/LocalReturnPath.cs @@ -8,12 +8,11 @@ public static class LocalReturnPath private const int MaxDecodePasses = 8; /// - /// Returns the original candidate when it is a local destination, or / when it is missing, absolute, protocol-relative, or otherwise unsafe. + /// Returns the original candidate when it is a rooted local path, or / when it is missing, relative, absolute, protocol-relative, or otherwise unsafe. /// /// /// Validation runs on a decoded copy. The returned value is the original string so encoded query values and path segments stay byte-for-byte intact. - /// Scheme-less relative paths such as workflows/x are treated as local after prefixing / for the check only. - /// Decoding that does not reach a fixed point within fails closed to /. + /// Only a single leading / (not // or /\) is accepted. Decoding that does not reach a fixed point within fails closed to /. /// public static string Normalize(string? candidate) { @@ -25,19 +24,10 @@ public static string Normalize(string? candidate) var decoded = candidate; for (var pass = 0; pass < MaxDecodePasses; pass++) { - string next; - try - { - next = Uri.UnescapeDataString(decoded); - } - catch (UriFormatException) - { - return "/"; - } - + var next = Uri.UnescapeDataString(decoded); if (next == decoded) { - return IsLocalDestination(candidate) && IsLocalDestination(decoded) ? candidate : "/"; + return IsRootedLocalPath(candidate) && IsRootedLocalPath(decoded) ? candidate : "/"; } decoded = next; @@ -46,61 +36,25 @@ public static string Normalize(string? candidate) return "/"; } - private static bool IsLocalDestination(string path) + /// + /// Roots a base-relative candidate against 's path, then normalizes. + /// + public static string RootAgainstBase(string? candidate, string baseUri) { - if (IsRootedLocalPath(path)) + var basePath = new Uri(baseUri).AbsolutePath; + if (!basePath.EndsWith('/')) { - return true; + basePath += "/"; } - if (path.Length == 0 || char.IsWhiteSpace(path[0]) || HasSchemeOrHost(path)) - { - return false; - } - - return IsRootedLocalPath("/" + path); + var rooted = candidate is { Length: > 0 } && candidate[0] != '/' + ? basePath + candidate + : candidate; + return Normalize(rooted); } private static bool IsRootedLocalPath(string path) => path.StartsWith('/') && !path.StartsWith("//", StringComparison.Ordinal) && !path.Any(c => c == '\\' || char.IsControl(c)); - - private static bool HasSchemeOrHost(string path) - { - if (path.StartsWith("//", StringComparison.Ordinal) || - path.StartsWith('\\') || - path.Contains('\\')) - { - return true; - } - - var schemeSeparator = path.IndexOf(':'); - if (schemeSeparator <= 0) - { - return false; - } - - var queryStart = path.IndexOf('?'); - if (queryStart >= 0 && schemeSeparator > queryStart) - { - return false; - } - - if (!char.IsAsciiLetter(path[0])) - { - return false; - } - - for (var i = 1; i < schemeSeparator; i++) - { - var c = path[i]; - if (!char.IsAsciiLetterOrDigit(c) && c != '+' && c != '-' && c != '.') - { - return false; - } - } - - return true; - } } diff --git a/src/modules/Elsa.Studio.Authentication.ElsaIdentity.UI/Components/ElsaIdentityLoginMethod.razor b/src/modules/Elsa.Studio.Authentication.ElsaIdentity.UI/Components/ElsaIdentityLoginMethod.razor index b96fab79e..9d14b2766 100644 --- a/src/modules/Elsa.Studio.Authentication.ElsaIdentity.UI/Components/ElsaIdentityLoginMethod.razor +++ b/src/modules/Elsa.Studio.Authentication.ElsaIdentity.UI/Components/ElsaIdentityLoginMethod.razor @@ -66,7 +66,7 @@ await JwtAccessor.WriteTokenAsync(Elsa.Studio.Authentication.ElsaIdentity.TokenNames.RefreshToken, result.RefreshToken!); if (AuthenticationStateProvider is Elsa.Studio.Authentication.ElsaIdentity.Services.AccessTokenAuthenticationStateProvider stateProvider) stateProvider.NotifyAuthenticationStateChanged(); - Navigation.NavigateTo(Context.ReturnPath, forceLoad: true); + Navigation.NavigateTo(Elsa.Studio.Authentication.Abstractions.LocalReturnPath.RootAgainstBase(Context.ReturnPath, Navigation.BaseUri), forceLoad: true); return true; }); } diff --git a/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs b/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs index b40b3a42e..fb40afca3 100644 --- a/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs +++ b/src/modules/Elsa.Studio.Authentication.OpenIdConnect.BlazorServer/Controllers/AuthenticationController.cs @@ -40,7 +40,7 @@ public IActionResult Logout([FromForm] string? returnUrl = null) private string SafeRedirectUri(string? returnUrl) { var path = LocalReturnPath.Normalize(returnUrl); - if (Url?.IsLocalUrl(path) == false) + if (!Url.IsLocalUrl(path)) { return "/"; } diff --git a/src/modules/Elsa.Studio.Authentication.UI/Components/LoginPanel.razor b/src/modules/Elsa.Studio.Authentication.UI/Components/LoginPanel.razor index bae9513dd..2c9f88045 100644 --- a/src/modules/Elsa.Studio.Authentication.UI/Components/LoginPanel.razor +++ b/src/modules/Elsa.Studio.Authentication.UI/Components/LoginPanel.razor @@ -199,5 +199,6 @@ string.Equals(method.Kind, "local", StringComparison.OrdinalIgnoreCase) || string.Equals(method.Kind, "elsa-identity", StringComparison.OrdinalIgnoreCase); - private static string NormalizeReturnPath(string? candidate) => LocalReturnPath.Normalize(candidate); + private string NormalizeReturnPath(string? candidate) => + LocalReturnPath.RootAgainstBase(candidate, Navigation.BaseUri); } diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorServer/ServerBrokerAuthenticationTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorServer/ServerBrokerAuthenticationTests.cs index 0a945821f..10d30e84f 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorServer/ServerBrokerAuthenticationTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/BlazorServer/ServerBrokerAuthenticationTests.cs @@ -276,6 +276,62 @@ public async Task LocalLoginCallbackError_ReturnsToChooserWithASafeErrorOutcome( Assert.Contains("error=sign_in_failed", redirect.Url); } + [Fact] + public async Task Callback_RelativeReturnPath_LocalRedirectsToRoot() + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddAuthentication(ServerExternalAuthenticationStateProvider.Scheme) + .AddCookie(ServerExternalAuthenticationStateProvider.Scheme); + using var serviceProvider = services.BuildServiceProvider(); + using var callbackScope = serviceProvider.CreateScope(); + var context = new DefaultHttpContext { RequestServices = callbackScope.ServiceProvider }; + context.Request.Scheme = "https"; + context.Request.Host = new HostString("studio.example.test"); + var accessor = new HttpContextAccessor { HttpContext = context }; + var options = new ExternalAuthenticationClientOptions { ClientId = "studio-server", ClientSecret = "secret" }; + var broker = new FakeBrokerApi(); + var anonymous = new FakeAnonymousBackendApiClientProvider(broker); + var stateProvider = new ServerExternalAuthenticationStateProvider(accessor, anonymous, new ServerExternalAuthenticationRefreshCoordinator(), options); + var controller = CreateController(context, anonymous, new FakeTransactionStore(new("state", "verifier", "workflows/x", DateTimeOffset.UtcNow.AddMinutes(1))), stateProvider, options); + + var result = await controller.Callback("completion-code", "state", null, CancellationToken.None); + + Assert.Equal("/", Assert.IsType(result).Url); + } + + [Fact] + public async Task Logout_RelativeReturnPath_LocalRedirectsToRoot() + { + var ticket = CreateCurrentTicket(); + var context = CreateAuthenticatedContext(ticket.Principal, new RecordingAuthenticationService(ticket)); + var broker = new FakeBrokerApi { LogoutResult = new(true, null, null) }; + var anonymous = new FakeAnonymousBackendApiClientProvider(broker); + var transactions = new FakeTransactionStore(new("unused", "", "/", DateTimeOffset.UtcNow.AddMinutes(1))); + var state = new ServerExternalAuthenticationStateProvider(new HttpContextAccessor { HttpContext = context }, anonymous, new ServerExternalAuthenticationRefreshCoordinator(), Options()); + + var result = await CreateController(context, anonymous, transactions, state, Options()).Logout("local", "workflows/x", CancellationToken.None); + + Assert.Equal("/", Assert.IsType(result).Url); + } + + [Fact] + public void LogoutCallback_RelativeReturnPath_LocalRedirectsToRoot() + { + var context = new DefaultHttpContext(); + var anonymous = new FakeAnonymousBackendApiClientProvider(); + var transactions = new FakeTransactionStore(new("unused", "", "workflows/x", DateTimeOffset.UtcNow.AddMinutes(1), "logout")); + var state = new ServerExternalAuthenticationStateProvider( + new HttpContextAccessor { HttpContext = context }, + anonymous, + new ServerExternalAuthenticationRefreshCoordinator(), + Options()); + + var result = CreateController(context, anonymous, transactions, state, Options()).LogoutCallback(); + + Assert.Equal("/", Assert.IsType(result).Url); + } + [Fact] public async Task Callback_UsesConfidentialBasicExchangeAndExactState() { diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs index 1f7e74a9e..8b997f33a 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs @@ -3,7 +3,11 @@ using Elsa.Studio.ExternalAuthentication.Tests.Login; using Elsa.Studio.Login.Services; using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Routing; +using Microsoft.AspNetCore.Routing; using Xunit; namespace Elsa.Studio.ExternalAuthentication.Tests.Compatibility; @@ -24,7 +28,7 @@ public async Task DirectOpenIdConnectContributesItsLegacyChallengeToTheSharedLog [MemberData(nameof(LocalReturnPathCorpus.Cases), MemberType = typeof(LocalReturnPathCorpus))] public void DirectOpenIdConnectChallengeAcceptsOnlyLocalReturnUrls(string? returnUrl, string expectedReturnUrl) { - var result = Assert.IsType(new AuthenticationController().Login(returnUrl)); + var result = Assert.IsType(CreateController().Login(returnUrl)); Assert.NotNull(result.Properties); Assert.Equal(expectedReturnUrl, result.Properties!.RedirectUri); @@ -34,7 +38,7 @@ public void DirectOpenIdConnectChallengeAcceptsOnlyLocalReturnUrls(string? retur [MemberData(nameof(LocalReturnPathCorpus.Cases), MemberType = typeof(LocalReturnPathCorpus))] public void DirectOpenIdConnectSignOutAcceptsOnlyLocalReturnUrls(string? returnUrl, string expectedReturnUrl) { - var result = Assert.IsType(new AuthenticationController().Logout(returnUrl)); + var result = Assert.IsType(CreateController().Logout(returnUrl)); Assert.NotNull(result.Properties); Assert.Equal(expectedReturnUrl, result.Properties!.RedirectUri); @@ -43,7 +47,7 @@ public void DirectOpenIdConnectSignOutAcceptsOnlyLocalReturnUrls(string? returnU [Fact] public void DirectOpenIdConnectChallengeRejectsBoundControlCharacterReturnUrls() { - var result = Assert.IsType(new AuthenticationController().Login("/\t/evil.com")); + var result = Assert.IsType(CreateController().Login("/\t/evil.com")); Assert.NotNull(result.Properties); Assert.Equal("/", result.Properties!.RedirectUri); @@ -56,4 +60,14 @@ public void LegacyOidcStateHelperKeepsPathBase() Assert.Equal("/elsa/studio/workflows/definitions?x=1", OpenIdConnectAuthorizationService.CaptureReturnPath(current)); } + + private static AuthenticationController CreateController() + { + var actionContext = new ActionContext(new DefaultHttpContext(), new RouteData(), new ActionDescriptor()); + return new AuthenticationController + { + ControllerContext = new ControllerContext(actionContext), + Url = new UrlHelper(actionContext) + }; + } } diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathCorpus.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathCorpus.cs index 4b8b31754..0bfdf7e96 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathCorpus.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathCorpus.cs @@ -33,28 +33,24 @@ public static string EncodeTimes(string value, int times) { "/%2F/evil.com", "/" }, { "http://evil.com", "/" }, { "https://evil.com/phish", "/" }, - { "https://attacker.example", "/" }, - { "http://attacker.example", "/" }, - { "https://evil.example/", "/" }, - { "//attacker.example", "/" }, - { "/\\attacker.example", "/" }, { "javascript:alert(1)", "/" }, + { "http:evil.com", "/" }, + { "javascript%3Aalert(1)", "/" }, + { "%6Aavascript:alert(1)", "/" }, + { "\t//evil.com", "/" }, + { "c|/windows", "/" }, + { "c|evil.com", "/" }, { " //evil.com", "/" }, + { "workflows/x", "/" }, + { "workflows/definitions?tab=active", "/" }, + { "workflows/instances", "/" }, { "/workflows/definitions?x=1", "/workflows/definitions?x=1" }, { "/workflows?version=1", "/workflows?version=1" }, - { "/workflows?tab=active", "/workflows?tab=active" }, { "/caf%C3%A9", "/caf%C3%A9" }, { "/a?x=%26y", "/a?x=%26y" }, { "/a?ref=https%3A%2F%2Fexample.com", "/a?ref=https%3A%2F%2Fexample.com" }, { "/a%2Fb", "/a%2Fb" }, - { "/a?ref=https://example.com", "/a?ref=https://example.com" }, - { "/workflows?documentation=https://example.com", "/workflows?documentation=https://example.com" }, - { "/workflows?filter=red%26blue", "/workflows?filter=red%26blue" }, - { "/workflows?name=a%26b", "/workflows?name=a%26b" }, - { "/workflows?filter=a%26b", "/workflows?filter=a%26b" }, - { "workflows/x", "workflows/x" }, - { "workflows/definitions?tab=active", "workflows/definitions?tab=active" }, - { "workflows/instances", "workflows/instances" } + { "/a?ref=https://example.com", "/a?ref=https://example.com" } }; cases.Add(EncodeTimes("//evil.com", 9), "/"); diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs index e32b1c935..0c0b30e84 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Login/LocalReturnPathTests.cs @@ -22,19 +22,22 @@ public void Normalize_RejectsDecodedControlCharactersThatCreateProtocolRelativeU } [Fact] - public void Normalize_PreservesEncodedLinksByteForByte() + public void Normalize_RejectsNineTimesEncodedProtocolRelativeUrls() { - Assert.Equal("/caf%C3%A9", LocalReturnPath.Normalize("/caf%C3%A9")); - Assert.Equal("/a?x=%26y", LocalReturnPath.Normalize("/a?x=%26y")); - Assert.Equal("/a?ref=https%3A%2F%2Fexample.com", LocalReturnPath.Normalize("/a?ref=https%3A%2F%2Fexample.com")); - Assert.Equal("/a%2Fb", LocalReturnPath.Normalize("/a%2Fb")); - Assert.Equal("/a?ref=https://example.com", LocalReturnPath.Normalize("/a?ref=https://example.com")); + Assert.Equal("/", LocalReturnPath.Normalize(LocalReturnPathCorpus.EncodeTimes("//evil.com", 9))); + Assert.Equal("/", LocalReturnPath.Normalize("/" + LocalReturnPathCorpus.EncodeTimes("//evil.com", 9))); } [Fact] - public void Normalize_RejectsNineTimesEncodedProtocolRelativeUrls() + public void ElsaLogin_RootsBaseRelativePathsAgainstPathBase() { - Assert.Equal("/", LocalReturnPath.Normalize(LocalReturnPathCorpus.EncodeTimes("//evil.com", 9))); - Assert.Equal("/", LocalReturnPath.Normalize("/" + LocalReturnPathCorpus.EncodeTimes("//evil.com", 9))); + const string studioBase = "https://studio.example/studio/"; + + Assert.Equal("/studio/workflows/x", Elsa.Studio.Login.Pages.Login.Login.ResolveReturnUrl("workflows/x", studioBase)); + Assert.Equal("/studio/workflows/definitions?tab=active", Elsa.Studio.Login.Pages.Login.Login.ResolveReturnUrl("workflows/definitions?tab=active", studioBase)); + Assert.Equal("/studio/c|/windows", Elsa.Studio.Login.Pages.Login.Login.ResolveReturnUrl("c|/windows", studioBase)); + Assert.Equal("/studio/http:evil.com", Elsa.Studio.Login.Pages.Login.Login.ResolveReturnUrl("http:evil.com", studioBase)); + Assert.Equal("/", Elsa.Studio.Login.Pages.Login.Login.ResolveReturnUrl("//evil.com", studioBase)); + Assert.Equal("/workflows/definitions?x=1", Elsa.Studio.Login.Pages.Login.Login.ResolveReturnUrl("/workflows/definitions?x=1", studioBase)); } } diff --git a/src/modules/Elsa.Studio.Login/Pages/Login/Login.razor.cs b/src/modules/Elsa.Studio.Login/Pages/Login/Login.razor.cs index 5b9b7c27c..779064e76 100644 --- a/src/modules/Elsa.Studio.Login/Pages/Login/Login.razor.cs +++ b/src/modules/Elsa.Studio.Login/Pages/Login/Login.razor.cs @@ -56,9 +56,15 @@ private async Task TryLogin(LoginArgs args) returnUrl = values.FirstOrDefault(); } - NavigationManager.NavigateTo(LocalReturnPath.Normalize(returnUrl), true); + NavigationManager.NavigateTo(ResolveReturnUrl(returnUrl, NavigationManager.BaseUri), true); } + /// + /// Roots a legacy ElsaLogin returnUrl against the Studio base path before the shared helper accepts only rooted destinations. + /// + internal static string ResolveReturnUrl(string? returnUrl, string baseUri) => + LocalReturnPath.RootAgainstBase(returnUrl, baseUri); + private async Task ValidateCredentials(string? username, string? password) { if (string.IsNullOrEmpty(username) || string.IsNullOrEmpty(password)) From 8bb6b25d8020bc74d9650afa061645a34a188af7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 3 Oct 2026 14:16:36 +0000 Subject: [PATCH 4/4] test(auth): construct UrlHelper with ControllerActionDescriptor ControllerContext rejects a plain ActionDescriptor, so the OIDC controller fixture now uses ControllerActionDescriptor. Co-authored-by: Sipke Schoorstra --- .../Compatibility/DirectOpenIdConnectLoginTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs index 8b997f33a..ce3dc83a2 100644 --- a/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs +++ b/src/modules/Elsa.Studio.ExternalAuthentication.Tests/Compatibility/DirectOpenIdConnectLoginTests.cs @@ -5,7 +5,7 @@ using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; -using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Routing; using Microsoft.AspNetCore.Routing; using Xunit; @@ -63,7 +63,7 @@ public void LegacyOidcStateHelperKeepsPathBase() private static AuthenticationController CreateController() { - var actionContext = new ActionContext(new DefaultHttpContext(), new RouteData(), new ActionDescriptor()); + var actionContext = new ActionContext(new DefaultHttpContext(), new RouteData(), new ControllerActionDescriptor()); return new AuthenticationController { ControllerContext = new ControllerContext(actionContext),