Fix IsLocalUrl in EF

Note that in 6.1, IsLocalUrl got copied into the EF package
DuendeSoftware · Jun 12, 2024 · 4a861f2 · 4a861f2
1 parent 5e69ed0
commit 4a861f2
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 3 deletions.
diff --git 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<char> 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
@@ -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<object[]> TestCases =>
+        new List<object[]>
+        {
+            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);
+    }
+}
diff --git 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);