From 4a861f24449bef7b2bd78990a60d37645202c5c1 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 12 Jun 2024 16:19:51 -0500 Subject: [PATCH] Fix IsLocalUrl in EF Note that in 6.1, IsLocalUrl got copied into the EF package --- .../Extensions/StringsExtensions.cs | 20 +++++- .../IsLocalUrlTests.cs | 62 +++++++++++++++++++ .../Validation/IsLocalUrlTests.cs | 1 - 3 files changed, 80 insertions(+), 3 deletions(-) create mode 100644 test/EntityFramework.Storage.UnitTests/IsLocalUrlTests.cs diff --git a/src/EntityFramework.Storage/Extensions/StringsExtensions.cs b/src/EntityFramework.Storage/Extensions/StringsExtensions.cs index d0b295f3a..9f26cb0b4 100644 --- a/src/EntityFramework.Storage/Extensions/StringsExtensions.cs +++ b/src/EntityFramework.Storage/Extensions/StringsExtensions.cs @@ -136,6 +136,8 @@ public static string CleanUrlPath(this string url) [DebuggerStepThrough] public static bool IsLocalUrl(this string url) { + // This implementation is a copy of a https://github.com/dotnet/aspnetcore/blob/3f1acb59718cadf111a0a796681e3d3509bb3381/src/Mvc/Mvc.Core/src/Routing/UrlHelperBase.cs#L315 + // We originally copied that code to avoid a dependency, but we could potentially remove this entirely by switching to the Microsoft.NET.Sdk.Web sdk. if (string.IsNullOrEmpty(url)) { return false; @@ -153,7 +155,7 @@ public static bool IsLocalUrl(this string url) // url doesn't start with "//" or "/\" if (url[1] != '/' && url[1] != '\\') { - return true; + return !HasControlCharacter(url.AsSpan(1)); } return false; @@ -171,13 +173,27 @@ public static bool IsLocalUrl(this string url) // url doesn't start with "~//" or "~/\" if (url[2] != '/' && url[2] != '\\') { - return true; + 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; + } } [DebuggerStepThrough] diff --git a/test/EntityFramework.Storage.UnitTests/IsLocalUrlTests.cs b/test/EntityFramework.Storage.UnitTests/IsLocalUrlTests.cs new file mode 100644 index 000000000..8f93f495b --- /dev/null +++ b/test/EntityFramework.Storage.UnitTests/IsLocalUrlTests.cs @@ -0,0 +1,62 @@ +using System.Collections.Generic; +using FluentAssertions; +using Xunit; +using Duende.IdentityServer.EntityFramework.Extensions; + +namespace UnitTests.Validation; + +public class IsLocalUrlTests +{ + private const string queryParameters = "?client_id=mvc.code" + + "&redirect_uri=https%3A%2F%2Flocalhost%3A44302%2Fsignin-oidc" + + "&response_type=code" + + "&scope=openid%20profile%20email%20custom.profile%20resource1.scope1%20resource2.scope1%20offline_access" + + "&code_challenge=LcJN1shWmezC0J5EU7QOi7N_amBuvMDb6PcTY0sB2YY" + + "&code_challenge_method=S256" + + "&response_mode=form_post" + + "&nonce=nonce" + + "&state=state"; + + public static IEnumerable TestCases => + new List + { + new object[] { "/connect/authorize/callback" + queryParameters, true }, + new object[] { "//evil.com/" + queryParameters, false }, + // Tab character + new object[] { "/\t/evil.com/connect/authorize/callback" + queryParameters, false }, + // Tabs and Spaces + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/ \t/evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + new object[] { "/\t /evil.com/connect/authorize/callback" + queryParameters, false }, + // Various new line related things + new object[] { "/\n/evil.com/" + queryParameters, false }, + new object[] { "/\n\n/evil.com/" + queryParameters, false }, + new object[] { "/\r/evil.com/" + queryParameters, false }, + new object[] { "/\r\r/evil.com/" + queryParameters, false }, + new object[] { "/\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\r\n\r\n/evil.com/" + queryParameters, false }, + // Tabs and Newlines + new object[] { "/\t\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\n\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\r/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\r/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\t\r\n\r\n/evil.com/" + queryParameters, false }, + new object[] { "/\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\n\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\r/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\r/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\n/evil.com\t/" + queryParameters, false }, + new object[] { "/\r\n\r\n/evil.com\t/" + queryParameters, false }, + }; + + [Theory] + [MemberData(nameof(TestCases))] + public void IsLocalUrl(string returnUrl, bool expected) + { + returnUrl.IsLocalUrl().Should().Be(expected); + } +} \ No newline at end of file diff --git a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs index d9835cfcf..6319748ac 100644 --- a/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs +++ b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs @@ -78,7 +78,6 @@ public async void GetAuthorizationContextAsync(string returnUrl, bool expected) [Theory] [MemberData(nameof(TestCases))] - // TODO - Test the duplicated method in the EF package in later release branches public void IsLocalUrl(string returnUrl, bool expected) { returnUrl.IsLocalUrl().Should().Be(expected);