Route parameter with urlencoded slash inconsistency between TestServer and Kestrel behavior (#33364)

Author: ladeak

src/Hosting/TestHost/test/ClientHandlerTests.cs

@@ -24,6 +24,29 @@ namespace Microsoft.AspNetCore.TestHost
 {
     public class ClientHandlerTests
     {
+        [Fact]
+        public async Task SlashUrlEncodedDoesNotGetDecoded()
+        {
+            var handler = new ClientHandler(new PathString(), new InspectingApplication(features =>
+            {
+                Assert.True(features.Get<IHttpRequestLifetimeFeature>().RequestAborted.CanBeCanceled);
+                Assert.Equal(HttpProtocol.Http11, features.Get<IHttpRequestFeature>().Protocol);
+                Assert.Equal("GET", features.Get<IHttpRequestFeature>().Method);
+                Assert.Equal("https", features.Get<IHttpRequestFeature>().Scheme);
+                Assert.Equal("/api/a%2Fb c", features.Get<IHttpRequestFeature>().Path);
+                Assert.NotNull(features.Get<IHttpRequestFeature>().Body);
+                Assert.NotNull(features.Get<IHttpRequestFeature>().Headers);
+                Assert.NotNull(features.Get<IHttpResponseFeature>().Headers);
+                Assert.NotNull(features.Get<IHttpResponseBodyFeature>().Stream);
+                Assert.Equal(200, features.Get<IHttpResponseFeature>().StatusCode);
+                Assert.Null(features.Get<IHttpResponseFeature>().ReasonPhrase);
+                Assert.Equal("example.com", features.Get<IHttpRequestFeature>().Headers["host"]);
+                Assert.NotNull(features.Get<IHttpRequestLifetimeFeature>());
+            }));
+            var httpClient = new HttpClient(handler);
+            await httpClient.GetAsync("https://example.com/api/a%2Fb%20c");
+        }
+
         [Fact]
         public Task ExpectedKeysAreAvailable()
         {

src/Http/Http.Abstractions/perf/Microbenchmarks/PathStringBenchmark.cs

@@ -0,0 +1,33 @@
+using System;
+using System.Collections.Generic;
+using BenchmarkDotNet.Attributes;
+
+namespace Microsoft.AspNetCore.Http.Abstractions.Microbenchmarks
+{
+    public class PathStringBenchmark
+    {
+        private const string TestPath = "/api/a%2Fb/c";
+        private const string LongTestPath = "/thisMustBeAVeryLongPath/SoLongThatItCouldActuallyBeLargerToTheStackAllocThresholdValue/PathsShorterToThisAllocateLessOnHeapByUsingStackAllocation/api/a%20b";
+        private const string LongTestPathEarlyPercent = "/t%20hisMustBeAVeryLongPath/SoLongButStillShorterToTheStackAllocThresholdValue/PathsShorterToThisAllocateLessOnHeap/api/a%20b";
+
+        public IEnumerable<object> TestPaths => new[] { TestPath, LongTestPath, LongTestPathEarlyPercent };
+
+        public IEnumerable<object> TestUris => new[] { new Uri($"https://localhost:5001/{TestPath}"), new Uri($"https://localhost:5001/{LongTestPath}"), new Uri($"https://localhost:5001/{LongTestPathEarlyPercent}") };
+
+        [Benchmark]
+        [ArgumentsSource(nameof(TestPaths))]
+        public string OnPathFromUriComponent(string testPath)
+        {
+            var pathString = PathString.FromUriComponent(testPath);
+            return pathString.Value;
+        }
+
+        [Benchmark]
+        [ArgumentsSource(nameof(TestUris))]
+        public string OnUriFromUriComponent(Uri testUri)
+        {
+            var pathString = PathString.FromUriComponent(testUri);
+            return pathString.Value;
+        }
+    }
+}

src/Http/Http.Abstractions/src/Microsoft.AspNetCore.Http.Abstractions.csproj

@@ -24,6 +24,7 @@ Microsoft.AspNetCore.Http.HttpResponse</Description>
     <Compile Include="$(SharedSourceRoot)ActivatorUtilities\*.cs" />
     <Compile Include="$(SharedSourceRoot)ParameterDefaultValue\*.cs" />
     <Compile Include="$(SharedSourceRoot)PropertyHelper\**\*.cs" />
+    <Compile Include="$(SharedSourceRoot)\UrlDecoder\UrlDecoder.cs" Link="UrlDecoder.cs" />
   </ItemGroup>
 
   <ItemGroup>

src/Http/Http.Abstractions/src/PathString.cs

@@ -7,6 +7,7 @@
 using System.Globalization;
 using System.Text;
 using Microsoft.AspNetCore.Http.Abstractions;
+using Microsoft.AspNetCore.Internal;
 
 namespace Microsoft.AspNetCore.Http
 {
@@ -16,6 +17,8 @@ namespace Microsoft.AspNetCore.Http
     [TypeConverter(typeof(PathStringConverter))]
     public readonly struct PathString : IEquatable<PathString>
     {
+        internal const int StackAllocThreshold = 128;
+
         /// <summary>
         /// Represents the empty path. This field is read-only.
         /// </summary>
@@ -172,8 +175,16 @@ private static string ToEscapedUriComponent(string value, int i)
         /// <returns>The resulting PathString</returns>
         public static PathString FromUriComponent(string uriComponent)
         {
-            // REVIEW: what is the exactly correct thing to do?
-            return new PathString(Uri.UnescapeDataString(uriComponent));
+            int position = uriComponent.IndexOf('%');
+            if (position == -1)
+            {
+                return new PathString(uriComponent);
+            }
+            Span<char> pathBuffer = uriComponent.Length <= StackAllocThreshold ? stackalloc char[StackAllocThreshold] : new char[uriComponent.Length];
+            uriComponent.CopyTo(pathBuffer);
+            var length = UrlDecoder.DecodeInPlace(pathBuffer.Slice(position, uriComponent.Length - position));
+            pathBuffer = pathBuffer.Slice(0, position + length);
+            return new PathString(pathBuffer.ToString());
         }
 
         /// <summary>
@@ -187,9 +198,12 @@ public static PathString FromUriComponent(Uri uri)
             {
                 throw new ArgumentNullException(nameof(uri));
             }
-
-            // REVIEW: what is the exactly correct thing to do?
-            return new PathString("/" + uri.GetComponents(UriComponents.Path, UriFormat.Unescaped));
+            var uriComponent = uri.GetComponents(UriComponents.Path, UriFormat.UriEscaped);
+            Span<char> pathBuffer = uriComponent.Length < StackAllocThreshold ? stackalloc char[StackAllocThreshold] : new char[uriComponent.Length + 1];
+            pathBuffer[0] = '/';
+            var length = UrlDecoder.DecodeRequestLine(uriComponent.AsSpan(), pathBuffer.Slice(1));
+            pathBuffer = pathBuffer.Slice(0, length + 1);
+            return new PathString(pathBuffer.ToString());
         }
 
         /// <summary>

src/Http/Http.Abstractions/test/PathStringTests.cs

@@ -2,7 +2,10 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections.Generic;
 using System.ComponentModel;
+using System.Globalization;
+using System.Linq;
 using Microsoft.AspNetCore.Testing;
 using Xunit;
 
@@ -236,5 +239,114 @@ public void PathStringStaysEqualAfterAssignments()
             PathString p2 = s1;
             Assert.Equal(p1, p2);
         }
+
+        [Theory]
+        [InlineData("/a%2Fb")]
+        [InlineData("/a%2F")]
+        [InlineData("/%2fb")]
+        [InlineData("/a%2Fb/c%2Fd/e")]
+        public void StringFromUriComponentLeavesForwardSlashEscaped(string input)
+        {
+            var sut = PathString.FromUriComponent(input);
+            Assert.Equal(input, sut.Value);
+        }
+
+        [Theory]
+        [InlineData("/a%2Fb")]
+        [InlineData("/a%2F")]
+        [InlineData("/%2fb")]
+        [InlineData("/a%2Fb/c%2Fd/e")]
+        public void UriFromUriComponentLeavesForwardSlashEscaped(string input)
+        {
+            var uri = new Uri($"https://localhost:5001{input}");
+            var sut = PathString.FromUriComponent(uri);
+            Assert.Equal(input, sut.Value);
+        }
+
+        [Theory]
+        [InlineData("/a%20b", "/a b")]
+        [InlineData("/thisMustBeAVeryLongPath/SoLongThatItCouldActuallyBeLargerToTheStackAllocThresholdValue/PathsShorterToThisAllocateLessOnHeapByUsingStackAllocation/api/a%20b",
+            "/thisMustBeAVeryLongPath/SoLongThatItCouldActuallyBeLargerToTheStackAllocThresholdValue/PathsShorterToThisAllocateLessOnHeapByUsingStackAllocation/api/a b")]
+        public void StringFromUriComponentUnescapes(string input, string expected)
+        {
+            var sut = PathString.FromUriComponent(input);
+            Assert.Equal(expected, sut.Value);
+        }
+
+        [Theory]
+        [InlineData("/a%20b", "/a b")]
+        [InlineData("/thisMustBeAVeryLongPath/SoLongThatItCouldActuallyBeLargerToTheStackAllocThresholdValue/PathsShorterToThisAllocateLessOnHeapByUsingStackAllocation/api/a%20b",
+    "/thisMustBeAVeryLongPath/SoLongThatItCouldActuallyBeLargerToTheStackAllocThresholdValue/PathsShorterToThisAllocateLessOnHeapByUsingStackAllocation/api/a b")]
+        public void UriFromUriComponentUnescapes(string input, string expected)
+        {
+            var uri = new Uri($"https://localhost:5001{input}");
+            var sut = PathString.FromUriComponent(uri);
+            Assert.Equal(expected, sut.Value);
+        }
+
+        [Theory]
+        [InlineData("/a%2Fb")]
+        [InlineData("/a%2F")]
+        [InlineData("/%2fb")]
+        [InlineData("/%2Fb%20c")]
+        [InlineData("/a%2Fb%20c")]
+        [InlineData("/a%20b")]
+        [InlineData("/a%2Fb/c%2Fd/e%20f")]
+        [InlineData("/%E4%BD%A0%E5%A5%BD")]
+        public void FromUriComponentToUriComponent(string input)
+        {
+            var sut = PathString.FromUriComponent(input);
+            Assert.Equal(input, sut.ToUriComponent());
+        }
+
+        [Theory]
+        [MemberData(nameof(CharsToUnescape))]
+        [InlineData("/%E4%BD%A0%E5%A5%BD", "/你好")]
+        public void FromUriComponentUnescapesAllExceptForwardSlash(string input, string expected)
+        {
+            var sut = PathString.FromUriComponent(input);
+            Assert.Equal(expected, sut.Value);
+        }
+
+        [Theory]
+        [InlineData(-1)]
+        [InlineData(0)]
+        [InlineData(1)]
+        public void ExercisingStringFromUriComponentOnStackAllocLimit(int offset)
+        {
+            var path = "/";
+            var testString = new string('a', PathString.StackAllocThreshold + offset - path.Length);
+            var sut = PathString.FromUriComponent(path + testString);
+            Assert.Equal(PathString.StackAllocThreshold + offset, sut.Value!.Length);
+        }
+
+        [Theory]
+        [InlineData(-1)]
+        [InlineData(0)]
+        [InlineData(1)]
+        public void ExercisingUriFromUriComponentOnStackAllocLimit(int offset)
+        {
+            var localhost = "https://localhost:5001/";
+            var testString = new string('a', PathString.StackAllocThreshold + offset);
+            var sut = PathString.FromUriComponent(new Uri(localhost + testString));
+            Assert.Equal(PathString.StackAllocThreshold + offset + 1, sut.Value!.Length);
+        }
+
+        public static IEnumerable<object[]> CharsToUnescape
+        {
+            get
+            {
+                foreach (var item in Enumerable.Range(1, 127))
+                {
+                    // %2F is '/' not escaped for paths
+                    if (item != 0x2f)
+                    {
+                        var hexEscapedValue = "%" + item.ToString("x2", CultureInfo.InvariantCulture);
+                        var expected = Uri.UnescapeDataString(hexEscapedValue);
+                        yield return new object[] { "/a" + hexEscapedValue, "/a" + expected };
+                    }
+                }
+            }
+        }
     }
 }

src/Mvc/test/WebSites/RoutingWebSite/StartupForDynamic.cs

@@ -61,7 +61,7 @@ public override ValueTask<RouteValueDictionary> TransformAsync(HttpContext httpC
                 var results = new RouteValueDictionary();
                 foreach (var kvp in kvps)
                 {
-                    var split = kvp.Split("=");
+                    var split = kvp.Replace("%2F", "/", StringComparison.OrdinalIgnoreCase).Split("=");
                     results[split[0]] = split[1];
                 }
 

src/Mvc/test/WebSites/RoutingWebSite/StartupForDynamicAndRazorPages.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Http;
@@ -50,7 +51,7 @@ public override ValueTask<RouteValueDictionary> TransformAsync(HttpContext httpC
                 var results = new RouteValueDictionary();
                 foreach (var kvp in kvps)
                 {
-                    var split = kvp.Split("=");
+                    var split = kvp.Replace("%2F", "/", StringComparison.OrdinalIgnoreCase).Split("=");
                     results[split[0]] = split[1];
                 }
 

src/Mvc/test/WebSites/RoutingWebSite/StartupForDynamicOrder.cs

@@ -115,7 +115,7 @@ public override ValueTask<RouteValueDictionary> TransformAsync(HttpContext httpC
 
                 foreach (var kvp in kvps)
                 {
-                    var split = kvp.Split("=");
+                    var split = kvp.Replace("%2F", "/", StringComparison.OrdinalIgnoreCase).Split("=");
                     if (split.Length == 2)
                     {
                         results[split[0]] = split[1];