Merge pull request #2333 from DuendeSoftware/jmdc/7.4/querystring-regression

Fixed a regression where the + character was not treated as a space in query params

Author: adamralph

identity-server/src/Shared/Extensions/StringExtensions.cs

@@ -260,15 +260,19 @@ public static NameValueCollection ReadQueryStringAsNameValueCollection(this stri
             }
 
             var kvp = pair.Split('=', 2);
-            var key = Uri.UnescapeDataString(kvp[0]);
+            var key = kvp[0];
             var value = kvp.Length > 1 ? kvp[1] : null;
-            var unescaped = value.IsPresent() ? Uri.UnescapeDataString(kvp[1]) : null;
-            collection.Add(key, unescaped);
+            if (value.IsPresent())
+            {
+                collection.Add(Decode(key), Decode(value));
+            }
         }
 
         return collection;
     }
 
+    private static string Decode(string value) => Uri.UnescapeDataString(value.Replace('+', ' '));
+
     private static string? QueryString(this string url)
     {
         var queryStringStart = url.IndexOf('?', StringComparison.InvariantCulture);

identity-server/test/IdentityServer.UnitTests/Extensions/StringExtensionsTests.cs

@@ -3,10 +3,11 @@
 
 
 using Duende.IdentityServer.Extensions;
+using Microsoft.AspNetCore.WebUtilities;
 
 namespace UnitTests.Extensions;
 
-public class StringExtensionsTests
+public class StringExtensionsTests()
 {
     private const string Category = "StringExtensions Tests";
 
@@ -230,6 +231,35 @@ public void ReadQueryStringAsNameValueCollection_should_decode_urlencoded_values
         nvc["baz"].ShouldBe("qux+test");
     }
 
+    private IEnumerable<string> AllCharactersAndEncodings()
+    {
+        for (var i = 0; i < 256; i++)
+        {
+            var c = Convert.ToChar(i);
+            var s = Convert.ToString(c);
+            yield return s;
+            yield return Uri.EscapeDataString(s);
+        }
+    }
+
+    [Fact]
+    public void ReadQueryStringAsNameValueCollection_should_decode_urlencoded_values_identically_to_QueryHelpersParseNullableQuery()
+    {
+        foreach (var c in AllCharactersAndEncodings())
+        {
+            var url = $"https://example.com?foo{c}bar=baz{c}quux";
+
+            var queryIndex = url.IndexOf('?');
+            var query = url.Substring(queryIndex + 1);
+            var oldParseResult = QueryHelpers.ParseNullableQuery(query);
+            oldParseResult.ShouldNotBeNull();
+            var oldNvc = oldParseResult.AsNameValueCollection();
+            var newNvc = url.ReadQueryStringAsNameValueCollection();
+
+            newNvc.ShouldBeEquivalentTo(oldNvc, "Failure for character or encoding:" + c);
+        }
+    }
+
     [Fact]
     [Trait("Category", Category)]
     public void ReadQueryStringAsNameValueCollection_should_handle_duplicate_keys()