From 995b103bbf12c7dc32e306442366ab0fbce05bff Mon Sep 17 00:00:00 2001 From: Finn <26823828+int0x81@users.noreply.github.com> Date: Tue, 10 Dec 2024 12:33:21 +0100 Subject: [PATCH] #2064 2065 #2132 #2169 Ampersand vs Slash workaround in `UpstreamTemplatePatternCreator` (#2225) * fixed bug in UpstreamTemplatePatternCreator * improved regex pattern + added unit tests * fixed upstream template tests * added acceptance test * pr adaptions * adopted regex related pr change requests * Code review by @raman-m --------- Co-authored-by: Raman Maksimchuk --- .../Creator/UpstreamTemplatePatternCreator.cs | 4 +- .../Routing/RoutingTests.cs | 43 ++++++++++++++++++- .../UpstreamTemplatePatternCreatorTests.cs | 43 ++++++++++++++++++- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs b/src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs index abf9c957b..58c992f39 100644 --- a/src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs +++ b/src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs @@ -40,7 +40,9 @@ public UpstreamPathTemplate Create(IRoute route) if (upstreamTemplate.Contains('?')) { containsQueryString = true; - upstreamTemplate = upstreamTemplate.Replace("?", "(|\\?)"); + upstreamTemplate = upstreamTemplate.Replace( + upstreamTemplate.Contains("/?") ? "/?" : "?", + @"(/$|/\?|\?|$)"); } for (var i = 0; i < placeholders.Count; i++) diff --git a/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs b/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs index bd83462dd..58b3beb64 100644 --- a/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs +++ b/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs @@ -666,7 +666,48 @@ public void Should_change_downstream_path_by_upstream_path_when_path_contains_ma .And(x => ThenTheDownstreamUrlPathShouldBe(decodedDownstreamUrlPath)) .BDDfy(); } - + + [Fact] + [Trait("Bug", "2064")] + [Trait("Discus", "2065")] + public void Should_match_correct_route_when_placeholder_appears_after_query_start() + { + const string DownstreamPath = "/1/products/1"; + var port = PortFinder.GetRandomPort(); + var configuration = GivenConfiguration( + GivenRoute(port, "/{tenantId}/products?{everything}", "/{tenantId}/products?{everything}"), // This route should NOT BE matched + GivenRoute(port, "/{tenantId}/products/{everything}", "/{tenantId}/products/{everything}")); // This route should BE matched + this.Given(x => GivenThereIsAServiceRunningOn(port, DownstreamPath, HttpStatusCode.OK, "Hello from Finn")) + .And(x => GivenThereIsAConfiguration(configuration)) + .And(x => GivenOcelotIsRunning()) + .When(x => WhenIGetUrlOnTheApiGateway("/1/products/1")) + .Then(x => ThenTheDownstreamUrlPathShouldBe(DownstreamPath)) + .And(x => ThenTheDownstreamUrlQueryStringShouldBe(string.Empty)) + .And(x => ThenTheStatusCodeShouldBe(HttpStatusCode.OK)) + .And(x => ThenTheResponseBodyShouldBe("Hello from Finn")) + .BDDfy(); + } + + [Fact] + [Trait("Bug", "2132")] + public void Should_match_correct_route_when_a_configuration_exists_with_query_param_wildcard() + { + const string DownstreamPath = "/api/v1/apple"; + var port = PortFinder.GetRandomPort(); + var configuration = GivenConfiguration( + GivenRoute(port, "/api/v1/abc?{everything}", "/api/v1/abc?{everything}"), // This route should NOT be matched + GivenRoute(port, "/api/v1/abc2/{everything}", "/api/v1/{everything}")); // This route should be matched + this.Given(x => GivenThereIsAServiceRunningOn(port, DownstreamPath, HttpStatusCode.OK, "Hello from Finn")) + .And(x => GivenThereIsAConfiguration(configuration)) + .And(x => GivenOcelotIsRunning()) + .When(x => WhenIGetUrlOnTheApiGateway("/api/v1/abc2/apple?isRequired=1")) + .Then(x => ThenTheDownstreamUrlPathShouldBe(DownstreamPath)) + .And(x => ThenTheDownstreamUrlQueryStringShouldBe("?isRequired=1")) + .And(x => ThenTheStatusCodeShouldBe(HttpStatusCode.OK)) + .And(x => ThenTheResponseBodyShouldBe("Hello from Finn")) + .BDDfy(); + } + private void GivenThereIsAServiceRunningOn(int port, string basePath, HttpStatusCode statusCode, string responseBody) { var baseUrl = DownstreamUrl(port); diff --git a/test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs b/test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs index 576c36ebf..db783ca7a 100644 --- a/test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs +++ b/test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs @@ -1,6 +1,7 @@ using Ocelot.Configuration.Creator; using Ocelot.Configuration.File; using Ocelot.Values; +using System.Text.RegularExpressions; namespace Ocelot.UnitTests.Configuration; @@ -215,7 +216,7 @@ public void should_create_template_pattern_that_matches_query_string() this.Given(x => x.GivenTheFollowingFileRoute(fileRoute)) .When(x => x.WhenICreateTheTemplatePattern()) - .Then(x => x.ThenTheFollowingIsReturned($"^(?i)/api/subscriptions/[^/]+/updates(|\\?)unitId={MatchEverything}$")) + .Then(x => x.ThenTheFollowingIsReturned($@"^(?i)/api/subscriptions/[^/]+/updates(/$|/\?|\?|$)unitId={MatchEverything}$")) .And(x => ThenThePriorityIs(1)) .BDDfy(); } @@ -230,11 +231,49 @@ public void should_create_template_pattern_that_matches_query_string_with_multip this.Given(x => x.GivenTheFollowingFileRoute(fileRoute)) .When(x => x.WhenICreateTheTemplatePattern()) - .Then(x => x.ThenTheFollowingIsReturned($"^(?i)/api/subscriptions/[^/]+/updates(|\\?)unitId={MatchEverything}&productId={MatchEverything}$")) + .Then(x => x.ThenTheFollowingIsReturned($@"^(?i)/api/subscriptions/[^/]+/updates(/$|/\?|\?|$)unitId={MatchEverything}&productId={MatchEverything}$")) .And(x => ThenThePriorityIs(1)) .BDDfy(); } + [Theory] + [Trait("Bug", "2064")] + [InlineData("/{tenantId}/products?{everything}", "/1/products/1", false)] + [InlineData("/{tenantId}/products/{everything}", "/1/products/1", true)] + public void Should_not_match_when_placeholder_appears_after_query_start(string urlPathTemplate, string requestPath, bool shouldMatch) + { + // Arrange + GivenTheFollowingFileRoute(new() { UpstreamPathTemplate = urlPathTemplate }); + + // Act + WhenICreateTheTemplatePattern(); + + // Assert + ShouldMatchWithRegex(requestPath, shouldMatch); + } + + [Theory] + [Trait("Bug", "2132")] + [InlineData("/api/v1/abc?{everything}", "/api/v1/abc2/apple", false)] + [InlineData("/api/v1/abc2/{everything}", "/api/v1/abc2/apple", true)] + public void Should_not_match_with_query_param_wildcard(string urlPathTemplate, string requestPath, bool shouldMatch) + { + // Arrange + GivenTheFollowingFileRoute(new() { UpstreamPathTemplate = urlPathTemplate }); + + // Act + WhenICreateTheTemplatePattern(); + + // Assert + ShouldMatchWithRegex(requestPath, shouldMatch); + } + + private void ShouldMatchWithRegex(string requestPath, bool shouldMatch) + { + var match = Regex.Match(requestPath, _result.Template); + Assert.Equal(shouldMatch, match.Success); + } + private void GivenTheFollowingFileRoute(FileRoute fileRoute) { _fileRoute = fileRoute;