#2212 Incorrect calculation of placeholders, only the last placeholde…

…r should be allowed to match several segments (#2213) * #2212 Incorrect calculation of placeholders, fixing the issue and adding unit and acceptance tests verifying the behavior. * Code review by @raman-m: Convert anonimous delegate to local function. --------- Co-authored-by: Raman Maksimchuk <[email protected]>
ThreeMammals · Nov 22, 2024 · 7a3771b · 7a3771b
1 parent e9a8bfc
commit 7a3771b
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 1 deletion.
diff --git a/src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs b/src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs
@@ -67,10 +67,12 @@ public Response<List<PlaceholderNameAndValue>> Find(string path, string query, s
     private static List<Group> FindGroups(string path, string query, string template)
     {
         template = EscapeExceptBraces(template);
-        var regexPattern = $"^{RegexPlaceholders().Replace(template, match => $"(?<{match.Groups[1].Value}>[^&]*)")}";
+        var regexPattern = GenerateRegexPattern(template);
         var testedPath = ShouldSkipQuery(query, template) ? path : $"{path}{query}";
+
         var match = Regex.Match(testedPath, regexPattern);
         var foundGroups = match.Groups.Cast<Group>().Skip(1).ToList();
+
         if (foundGroups.Count > 0 || !IsCatchAllPath(template))
         {
             return foundGroups;
@@ -80,6 +82,29 @@ private static List<Group> FindGroups(string path, string query, string template
         match = Regex.Match($"{testedPath}/", regexPattern);
         return match.Groups.Cast<Group>().Skip(1).ToList();
     }
+
+    /// <summary>
+    /// The placeholders that are not placed at the end of the template are delimited by forward slashes, only the last one, the catch-all can match more segments.
+    /// </summary>
+    /// <param name="escapedTemplate">The escaped path template.</param>
+    /// <returns>The pattern for values replacement.</returns>
+    private static string GenerateRegexPattern(string escapedTemplate)
+    {
+        // First we count the matches
+        var placeHoldersCountMatch = RegexPlaceholders().Matches(escapedTemplate);
+        int index = 0, placeHoldersCount = placeHoldersCountMatch.Count;
+
+        // We know that the replace process will be started from the beginning of the url,
+        // so we can use a simple counter to determine the last placeholder
+        string MatchEvaluator(Match match)
+        {
+            var groupName = match.Groups[1].Value;
+            index++;
+            return index == placeHoldersCount ? $"(?<{groupName}>[^&]*)" : $"(?<{groupName}>[^/|&]*)";
+        }
+
+        return $@"^{RegexPlaceholders().Replace(escapedTemplate, MatchEvaluator)}";
+    }
 
     private const int CatchAllQueryMilliseconds = 300;
 #if NET7_0_OR_GREATER

diff --git a/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs b/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs
@@ -501,6 +501,30 @@ public void ShouldNotMatchComplexQueriesCaseSensitive(string downstream, string
             .BDDfy();
     }
 
+    [Theory]
+    [Trait("Bug", "2212")]
+    [InlineData("/data-registers/{version}/it/{everything}", "/dati-registri/{version}/{everything}", "/dati-registri/v1.0/operatore/R80QQ5J9600/valida", "/data-registers/v1.0/it/operatore/R80QQ5J9600/valida")]
+    [InlineData("/files/{version}/uploads/{everything}", "/data/{version}/storage/{everything}", "/data/v2.0/storage/images/photos/nature", "/files/v2.0/uploads/images/photos/nature")]
+    [InlineData("/resources/{area}/details/{everything}", "/api/resources/{area}/info/{everything}", "/api/resources/global/info/stats/2024/data", "/resources/global/details/stats/2024/data")]
+    [InlineData("/users/{userId}/logs/{everything}", "/data/users/{userId}/activity/{everything}", "/data/users/12345/activity/session/login/2024", "/users/12345/logs/session/login/2024")]
+    [InlineData("/orders/{orderId}/items/{everything}", "/ecommerce/{orderId}/details/{everything}", "/ecommerce/98765/details/category/electronics/phone", "/orders/98765/items/category/electronics/phone")]
+    [InlineData("/tasks/{taskId}/subtasks/{everything}", "/work/{taskId}/breakdown/{everything}", "/work/56789/breakdown/phase/3/step/2", "/tasks/56789/subtasks/phase/3/step/2")]
+    [InlineData("/configs/{env}/overrides/{everything}", "/settings/{env}/{everything}", "/settings/prod/feature/toggles", "/configs/prod/overrides/feature/toggles")]
+    public void OnlyTheLastPlaceholderShouldMatchSeveralSegments(string downstream, string upstream, string requestUrl, string downstreamPath)
+    {
+        var port = PortFinder.GetRandomPort();
+        var route = GivenRoute(port, upstream, downstream);
+        var configuration = GivenConfiguration(route);
+        this.Given(x => GivenThereIsAServiceRunningOn(port, downstreamPath, HttpStatusCode.OK, "Hello from Guillaume"))
+            .And(x => GivenThereIsAConfiguration(configuration))
+            .And(x => GivenOcelotIsRunning())
+            .When(x => WhenIGetUrlOnTheApiGateway(requestUrl))
+            .Then(x => ThenTheDownstreamUrlPathShouldBe(downstreamPath))
+            .And(x => ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
+            .And(x => ThenTheResponseBodyShouldBe("Hello from Guillaume"))
+            .BDDfy();
+    }
+
     [Fact]
     [Trait("Feat", "91, 94")]
     public void Should_return_response_201_with_simple_url_and_multiple_upstream_http_method()

diff --git a/...t.UnitTests/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinderTests.cs b/...t.UnitTests/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinderTests.cs
@@ -416,6 +416,41 @@ public void Find_CaseInsensitive_CannotMatchPlaceholders(string template, string
         // Assert;
         ThenTheExpectedVariablesCantBeFound(expectedTemplates.ToArray());
     }
+
+    [Theory]
+    [Trait("Bug", "2212")]
+    [InlineData("/dati-registri/{version}/{everything}", "/dati-registri/v1.0/operatore/R80QQ5J9600/valida", "{version}", "v1.0", "{everything}", "operatore/R80QQ5J9600/valida")]
+    [InlineData("/api/invoices/{invoiceId}/{url}", "/api/invoices/1", "{invoiceId}", "1", "{url}", "")]
+    [InlineData("/api/{version}/{type}/{everything}", "/api/v1.0/items/details/12345", "{version}", "v1.0", "{type}", "items", "{everything}", "details/12345")]
+    [InlineData("/resources/{area}/{id}/{details}", "/resources/europe/56789/info/about", "{area}", "europe", "{id}", "56789", "{details}", "info/about")]
+    [InlineData("/data/{version}/{category}/{subcategory}/{rest}", "/data/2.1/sales/reports/weekly/summary", "{version}", "2.1", "{category}", "sales", "{subcategory}", "reports", "{rest}", "weekly/summary")]
+    [InlineData("/users/{region}/{team}/{userId}/{details}", "/users/north/eu/12345/activities/list", "{region}", "north", "{team}", "eu", "{userId}", "12345", "{details}", "activities/list")]
+    public void Find_HasCatchAll_OnlyTheLastPlaceholderCanContainSlashes(string template, string path,
+        string placeholderName1, string placeholderValue1, string placeholderName2, string placeholderValue2,
+        string placeholderName3 = null, string placeholderValue3 = null, string placeholderName4 = null, string placeholderValue4 = null)
+    {
+        var expectedTemplates = new List<PlaceholderNameAndValue>
+        {
+            new(placeholderName1, placeholderValue1),
+            new(placeholderName2, placeholderValue2),
+        };
+
+        if (!string.IsNullOrEmpty(placeholderName3))
+        {
+            expectedTemplates.Add(new(placeholderName3, placeholderValue3));
+        }
+
+        if (!string.IsNullOrEmpty(placeholderName4))
+        {
+            expectedTemplates.Add(new(placeholderName4, placeholderValue4));
+        }
+
+        // Act
+        _result = _finder.Find(path, Empty, template);
+
+        // Assert
+        ThenTheTemplatesVariablesAre(expectedTemplates.ToArray());
+    }
 
     private void ThenSinglePlaceholderIs(string expectedName, string expectedValue)
     {