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/src/IdentityServer/Extensions/StringsExtensions.cs b/src/IdentityServer/Extensions/StringsExtensions.cs index 95475fe45..07c1bf743 100644 --- a/src/IdentityServer/Extensions/StringsExtensions.cs +++ b/src/IdentityServer/Extensions/StringsExtensions.cs @@ -139,6 +139,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; @@ -156,7 +158,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; @@ -174,13 +176,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 new file mode 100644 index 000000000..6319748ac --- /dev/null +++ b/test/IdentityServer.UnitTests/Validation/IsLocalUrlTests.cs @@ -0,0 +1,179 @@ +using System.Collections.Generic; +using Duende.IdentityServer.Configuration; +using Duende.IdentityServer.Extensions; +using Duende.IdentityServer.Services; +using Duende.IdentityServer.Validation; +using FluentAssertions; +using Microsoft.Extensions.Logging; +using UnitTests.Common; +using UnitTests.Endpoints.Authorize; +using Xunit; + +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 async void GetAuthorizationContextAsync(string returnUrl, bool expected) + { + var interactionService = new DefaultIdentityServerInteractionService(null, null, null, null, null, null, null, + GetReturnUrlParser(), new LoggerFactory().CreateLogger()); + var actual = await interactionService.GetAuthorizationContextAsync(returnUrl); + if (expected) + { + actual.Should().NotBeNull(); + } + else + { + actual.Should().BeNull(); + } + } + + [Theory] + [MemberData(nameof(TestCases))] + public void IsLocalUrl(string returnUrl, bool expected) + { + returnUrl.IsLocalUrl().Should().Be(expected); + } + + [Theory] + [MemberData(nameof(TestCases))] + public void GetIdentityServerRelativeUrl(string returnUrl, bool expected) + { + var serverUrls = new MockServerUrls + { + Origin = "https://localhost:5001", + BasePath = "/" + }; + var actual = serverUrls.GetIdentityServerRelativeUrl(returnUrl); + if (expected) + { + actual.Should().NotBeNull(); + } + else + { + actual.Should().BeNull(); + } + } + + [Theory] + [MemberData(nameof(TestCases))] + public async void OidcReturnUrlParser_ParseAsync(string returnUrl, bool expected) + { + var oidcParser = GetOidcReturnUrlParser(); + var actual = await oidcParser.ParseAsync(returnUrl); + if (expected) + { + actual.Should().NotBeNull(); + } + else + { + actual.Should().BeNull(); + } + } + + [Theory] + [MemberData(nameof(TestCases))] + public void OidcReturnUrlParser_IsValidReturnUrl(string returnUrl, bool expected) + { + var oidcParser = GetOidcReturnUrlParser(); + oidcParser.IsValidReturnUrl(returnUrl).Should().Be(expected); + } + + + [Theory] + [MemberData(nameof(TestCases))] + public void ReturnUrlParser_IsValidReturnUrl(string returnUrl, bool expected) + { + var parser = GetReturnUrlParser(); + parser.IsValidReturnUrl(returnUrl).Should().Be(expected); + } + + [Theory] + [MemberData(nameof(TestCases))] + public async void ReturnUrlParser_ParseAsync(string returnUrl, bool expected) + { + var parser = GetReturnUrlParser(); + var actual = await parser.ParseAsync(returnUrl); + if (expected) + { + actual.Should().NotBeNull(); + } + else + { + actual.Should().BeNull(); + } + } + + private static ReturnUrlParser GetReturnUrlParser() + { + var oidcParser = GetOidcReturnUrlParser(); + var parser = new ReturnUrlParser(new IReturnUrlParser[] { oidcParser }); + return parser; + } + + private static OidcReturnUrlParser GetOidcReturnUrlParser() + { + return new OidcReturnUrlParser( + new IdentityServerOptions(), + new StubAuthorizeRequestValidator + { + Result = new AuthorizeRequestValidationResult + ( + new ValidatedAuthorizeRequest() + ) + }, + new MockUserSession(), + new MockServerUrls(), + new LoggerFactory().CreateLogger()); + } + + +} \ No newline at end of file