#2116 Escaping unsafe pattern values of Regex constructor derived…

… from URL query parameter values containing special `Regex` chars (#2150) * regex escape handling for url templates * refactored regex method to lamda version * Quick code review by @raman-m * added acceptance test for url regex bug * moved acceptance test to routing tests * Convert to theory: define 2 test cases --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
ThreeMammals · Sep 20, 2024 · 58d87c9 · 58d87c9
1 parent 8e66be7
commit 58d87c9
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 2 deletions.
diff --git a/src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs b/src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs
@@ -124,8 +124,8 @@ private static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(Downst
             foreach (var nAndV in templatePlaceholderNameAndValues)
             {
                 var name = nAndV.Name.Trim(OpeningBrace, ClosingBrace);
-
-                var rgx = new Regex($@"\b{name}={nAndV.Value}\b");
+                var value = Regex.Escape(nAndV.Value); // to ensure a placeholder value containing special Regex characters from URL query parameters is safely used in a Regex constructor, it's necessary to escape the value
+                var rgx = new Regex($@"\b{name}={value}\b");
 
                 if (rgx.IsMatch(downstreamRequest.Query))
                 {

diff --git a/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs b/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs
@@ -1,5 +1,6 @@
 using Microsoft.AspNetCore.Http;
 using Ocelot.Configuration.File;
+using System.Web;
 
 namespace Ocelot.AcceptanceTests.Routing
 {
@@ -1167,6 +1168,24 @@ public void should_fix_issue_271()
                 .BDDfy();
         }
 
+        [Theory]
+        [Trait("Bug", "2116")]
+        [InlineData("debug()")] // no query
+        [InlineData("debug%28%29")] // debug()
+        public void Should_change_downstream_path_by_upstream_path_when_path_contains_malicious_characters(string path)
+        {
+            var port = PortFinder.GetRandomPort();
+            var configuration = GivenDefaultConfiguration(port, "/api/{path}", "/routed/api/{path}");
+            var decodedDownstreamUrlPath = $"/routed/api/{HttpUtility.UrlDecode(path)}";
+            this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}", decodedDownstreamUrlPath, HttpStatusCode.OK, string.Empty))
+                .And(x => _steps.GivenThereIsAConfiguration(configuration))
+                .And(x => _steps.GivenOcelotIsRunning())
+                .When(x => _steps.WhenIGetUrlOnTheApiGateway($"/api/{path}")) // should be encoded
+                .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
+                .And(x => ThenTheDownstreamUrlPathShouldBe(decodedDownstreamUrlPath))
+                .BDDfy();
+        }
+
         private void GivenThereIsAServiceRunningOn(string baseUrl, string basePath, HttpStatusCode statusCode, string responseBody)
         {
             _serviceHandler.GivenThereIsAServiceRunningOn(baseUrl, basePath, async context =>

diff --git a/test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs b/test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs
@@ -611,6 +611,43 @@ public void Should_map_when_query_parameters_has_same_names_with_placeholder()
         ThenTheQueryStringIs($"?roleId={roleid}&{everything}");
     }
 
+    [Theory]
+    [Trait("Bug", "2116")]
+    [InlineData("api/debug()")] // no query
+    [InlineData("api/debug%28%29")] // debug()
+    public void ShouldNotFailToHandleUrlWithSpecialRegexChars(string urlPath)
+    {
+        // Arrange
+        var withGetMethod = new List<string> { "Get" };
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithUpstreamPathTemplate(new UpstreamPathTemplateBuilder()
+                .WithOriginalValue("/routed/api/{path}")
+                .Build())
+            .WithDownstreamPathTemplate("/api/{path}")
+            .WithUpstreamHttpMethod(withGetMethod)
+            .WithDownstreamScheme(Uri.UriSchemeHttp)
+            .Build();
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            new List<PlaceholderNameAndValue>
+            {
+                new("{path}", urlPath),
+            },
+            new RouteBuilder().WithDownstreamRoute(downstreamRoute)
+                .WithUpstreamHttpMethod(withGetMethod)
+                .Build()
+        ));
+        GivenTheDownstreamRequestUriIs($"http://localhost:5000/{urlPath}");
+        GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
+        GivenTheUrlReplacerWillReturn($"routed/{urlPath}");
+
+        // Act
+        WhenICallTheMiddleware();
+
+        // Assert
+        ThenTheDownstreamRequestUriIs($"http://localhost:5000/routed/{urlPath}");
+        Assert.Equal((int)HttpStatusCode.OK, _httpContext.Response.StatusCode);
+    }
+
     private void GivenTheServiceProviderConfigIs(ServiceProviderConfiguration config)
     {
         var configuration = new InternalConfiguration(null, null, config, null, null, null, null, null, null, null);