#2346 Fix problem with downstream URLs where query parameter names get messed up if they contain a placeholder that has a non-empty value

src/Ocelot/DownstreamUrlCreator/DownstreamUrlCreatorMiddleware.cs

@@ -1,4 +1,5 @@
 using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.WebUtilities;
 using Ocelot.Configuration;
 using Ocelot.DownstreamRouteFinder.UrlMatcher;
 using Ocelot.Infrastructure;
@@ -126,27 +127,64 @@ protected static string MergeQueryStringsWithoutDuplicateValues(string queryStri
     /// Added support for query string parameters in upstream path template.
     /// </summary>
     protected static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(DownstreamRequest downstreamRequest, List<PlaceholderNameAndValue> templatePlaceholders)
-    {
-        var builder = new StringBuilder();
-        foreach (var nAndV in templatePlaceholders)
         {
-            var name = nAndV.Name.Trim(OpeningBrace, ClosingBrace);
-            var parameter = $"{name}={nAndV.Value}";
-            if (!downstreamRequest.Query.Contains(parameter))
+            // Normalize legacy malformed query (?a=1?b=2)
+            var rawQuery = downstreamRequest.Query;
+
+            int firstQuestionMark = rawQuery.IndexOf('?');
+            if (firstQuestionMark != -1 &&
+                rawQuery.IndexOf('?', firstQuestionMark + 1) != -1)
             {
-                continue;
+                rawQuery =
+                    rawQuery.Substring(0, firstQuestionMark + 1) +
+                    rawQuery.Substring(firstQuestionMark + 1).Replace("?", "&");
             }
 
-            int questionMarkOrAmpersand = downstreamRequest.Query.IndexOf(name, StringComparison.Ordinal);
-            builder.Clear()
-                .Append(downstreamRequest.Query)
-                .Replace(parameter, string.Empty)
-                .Remove(--questionMarkOrAmpersand, 1);
-            downstreamRequest.Query = builder.Length > 0
-                ? builder.Remove(0, 1).Insert(0, QuestionMark).ToString()
+            // Parse the NORMALIZED query
+            var parsedQuery = QueryHelpers.ParseQuery(rawQuery);
+
+            // Create a case-insensitive lookup for template placeholders
+            var placeholderLookup = templatePlaceholders
+                .ToDictionary(p => p.Name.Trim(OpeningBrace, ClosingBrace), p => p.Value, StringComparer.OrdinalIgnoreCase);
+
+            // Build new query string by filtering out matching placeholders
+            var remainingParameters = new List<string>();
+
+            foreach (var kvp in parsedQuery)
+            {
+                var queryKey = kvp.Key;
+                var queryValues = kvp.Value;
+
+                // Check if this query parameter matches any placeholder (case-insensitive)
+                if (placeholderLookup.TryGetValue(queryKey, out var placeholderValue))
+                {
+                    // Remove only the values that match the placeholder value exactly
+                    var filteredValues = queryValues.Where(qv => !string.Equals(qv, placeholderValue, StringComparison.Ordinal)).ToList();
+
+                    // If there are remaining values, add them back
+                    if (filteredValues.Count > 0)
+                    {
+                        foreach (var value in filteredValues)
+                        {
+                            remainingParameters.Add($"{queryKey}={HttpUtility.UrlEncode(value)}");
+                        }
+                    }
+                }
+                else
+                {
+                    // This query parameter doesn't match any placeholder, keep all values
+                    foreach (var value in queryValues)
+                    {
+                        remainingParameters.Add($"{queryKey}={HttpUtility.UrlEncode(value)}");
+                    }
+                }
+            }
+
+            // Rebuild the query string
+            downstreamRequest.Query = remainingParameters.Count > 0 
+                ? QuestionMark + string.Join(Ampersand, remainingParameters)
                 : string.Empty;
         }
-    }
 
     protected static ReadOnlySpan<char> GetPath(ReadOnlySpan<char> downstreamPath)
     {

test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs

@@ -662,6 +662,192 @@ public async Task ShouldNotFailToHandleUrlWithSpecialRegexChars(string urlPath)
         Assert.Equal((int)HttpStatusCode.OK, _httpContext.Response.StatusCode);
     }
 
+    [Fact]
+    [Trait("Bug", "2346")]
+    public async Task Should_not_corrupt_query_parameter_names_containing_id_when_route_has_id_placeholder()
+    {
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithDownstreamPathTemplate("/v1/payment-methods")
+            .WithUpstreamHttpMethod(new List<string> { "Get" })
+            .WithDownstreamScheme("http")
+            .Build();
+        var config = new ServiceProviderConfigurationBuilder()
+            .Build();
+
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            new List<PlaceholderNameAndValue>
+            {
+                new("{id}", "123"), // This {id} placeholder should not affect customer_id query parameter
+            },
+            new Route(downstreamRoute, HttpMethod.Get)));
+
+        GivenTheDownstreamRequestUriIs("http://localhost:5003/v1/payment-methods?customer_id=12345");
+        GivenTheServiceProviderConfigIs(config);
+        GivenTheUrlReplacerWillReturn("/v1/payment-methods");
+
+        // Act
+        await _middleware.Invoke(_httpContext);
+
+        // Assert
+        ThenTheDownstreamRequestUriIs("http://localhost:5003/v1/payment-methods?customer_id=12345");
+        ThenTheQueryStringIs("?customer_id=12345");
+    }
+
+    [Fact]
+    [Trait("Bug", "2346")]
+    public async Task Should_not_remove_query_when_placeholder_is_suffix_of_query_name()
+    {
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithDownstreamPathTemplate("/orders")
+            .WithUpstreamHttpMethod(["Get"])
+            .WithDownstreamScheme("http")
+            .Build();
+
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            [ new("{id}", "1") ],
+            new Route(downstreamRoute, HttpMethod.Get)));
+
+        GivenTheDownstreamRequestUriIs("http://localhost/orders?orderid=99");
+        GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
+        GivenTheUrlReplacerWillReturn("/orders");
+
+        await _middleware.Invoke(_httpContext);
+
+        ThenTheQueryStringIs("?orderid=99");
+    }
+
+    [Fact]
+    public async Task Should_not_remove_query_when_placeholder_value_does_not_match()
+    {
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithDownstreamPathTemplate("/items/{id}")
+            .WithUpstreamHttpMethod(["Get"])
+            .WithDownstreamScheme("http")
+            .Build();
+
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            [ new("{id}", "10") ],
+            new Route(downstreamRoute, HttpMethod.Get)));
+
+        GivenTheDownstreamRequestUriIs("http://localhost/items?id=11");
+        GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
+        GivenTheUrlReplacerWillReturn("/items/10");
+
+        await _middleware.Invoke(_httpContext);
+
+        ThenTheQueryStringIs("?id=11");
+    }
+
+    [Fact]
+    public async Task Should_remove_all_occurrences_of_placeholder_query()
+    {
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithDownstreamPathTemplate("/items/{id}")
+            .WithUpstreamHttpMethod(["Get"])
+            .WithDownstreamScheme("http")
+            .Build();
+
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            [ new("{id}", "10") ],
+            new Route(downstreamRoute, HttpMethod.Get)));
+
+        GivenTheDownstreamRequestUriIs("http://localhost/items?id=10&id=10");
+        GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
+        GivenTheUrlReplacerWillReturn("/items/10");
+
+        await _middleware.Invoke(_httpContext);
+
+        ThenTheQueryStringIs(string.Empty);
+    }
+
+    [Fact]
+    public async Task Should_match_query_keys_case_insensitively()
+    {
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithDownstreamPathTemplate("/items/{id}")
+            .WithUpstreamHttpMethod(["Get"])
+            .WithDownstreamScheme("http")
+            .Build();
+
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            [ new("{id}", "10") ],
+            new Route(downstreamRoute, HttpMethod.Get)));
+
+        GivenTheDownstreamRequestUriIs("http://localhost/items?ID=10");
+        GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
+        GivenTheUrlReplacerWillReturn("/items/10");
+
+        await _middleware.Invoke(_httpContext);
+
+        ThenTheQueryStringIs(string.Empty);
+    }
+
+    [Fact]
+    public async Task Should_remove_placeholder_query_with_url_encoded_value()
+    {
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithDownstreamPathTemplate("/users/{name}")
+            .WithUpstreamHttpMethod(["Get"])
+            .WithDownstreamScheme("http")
+            .Build();
+
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            [ new("{name}", "john doe") ],
+            new Route(downstreamRoute, HttpMethod.Get)));
+
+        GivenTheDownstreamRequestUriIs("http://localhost/users?name=john%20doe");
+        GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
+        GivenTheUrlReplacerWillReturn("/users/john%20doe");
+
+        await _middleware.Invoke(_httpContext);
+
+        ThenTheQueryStringIs(string.Empty);
+    }
+
+    [Fact]
+    public async Task Should_handle_empty_query_value_gracefully()
+    {
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithDownstreamPathTemplate("/items/{id}")
+            .WithUpstreamHttpMethod(["Get"])
+            .WithDownstreamScheme("http")
+            .Build();
+
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            [ new("{id}", "") ],
+            new Route(downstreamRoute, HttpMethod.Get)));
+
+        GivenTheDownstreamRequestUriIs("http://localhost/items?id=");
+        GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
+        GivenTheUrlReplacerWillReturn("/items");
+
+        await _middleware.Invoke(_httpContext);
+
+        ThenTheQueryStringIs(string.Empty);
+    }
+
+    [Fact]
+    public async Task Should_not_remove_query_when_name_partially_matches_with_different_casing()
+    {
+        var downstreamRoute = new DownstreamRouteBuilder()
+            .WithDownstreamPathTemplate("/items")
+            .WithUpstreamHttpMethod(["Get"])
+            .WithDownstreamScheme("http")
+            .Build();
+
+        GivenTheDownStreamRouteIs(new DownstreamRouteHolder(
+            [ new("{id}", "1") ],
+            new Route(downstreamRoute, HttpMethod.Get)));
+
+        GivenTheDownstreamRequestUriIs("http://localhost/items?IdValue=1");
+        GivenTheServiceProviderConfigIs(new ServiceProviderConfigurationBuilder().Build());
+        GivenTheUrlReplacerWillReturn("/items");
+
+        await _middleware.Invoke(_httpContext);
+
+        ThenTheQueryStringIs("?IdValue=1");
+    }
+
     private static ReadOnlySpan<char> GetPath(string downstreamPath)
         => DownstreamUrlCreatorMiddlewareTestWrapper.GetPath(downstreamPath);