Honor parameters order when parsing query and form parameters (#7599) (…

…#7605)

* Honor parameters order when parsing query and form parameters

When parsing the query or form parameters in Request, the values are stored in a MultiMap. This class extends HashMap which does not preserve the order of insertion so a request with parameters "first=1&second=2" might end up in a map where "second" will come first when iterating on the entry set. 

The order is necessary in some case where the request is signed off the body and/or the query parameters. When the order is not preserved, it is impossible to reconstruct the original request sent, unless using the Request::getInputStream which consumes the stream and makes subsequent calls to Request::getParameters to don't return the form parameters which can be misleading. The same behavior applied to query parameters, by using Request::getQueryString, you get the correct order but Request::getParameters will not.

Moreoever, if the application is behind a reverse proxy using Jetty that is proxying using Request::getParameters which consume the request InputStream, it will be completely impossible to reconstruct the original request.

* Added a test with parameter merging

Co-authored-by: Jacques-Etienne Beaudet <[email protected]>

jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

@@ -34,6 +34,7 @@
 import java.util.Enumeration;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
@@ -253,6 +254,64 @@ public void testParamExtraction() throws Exception
         assertTrue(responses.startsWith("HTTP/1.1 200"));
     }
 
+    @Test
+    public void testParameterExtractionKeepOrderingIntact() throws Exception
+    {
+        AtomicReference<Map<String, String[]>> reference = new AtomicReference<>();
+        _handler._checker = new RequestTester()
+        {
+            @Override
+            public boolean check(HttpServletRequest request, HttpServletResponse response)
+            {
+                reference.set(request.getParameterMap());
+                return true;
+            }
+        };
+
+        String request = "POST /?first=1&second=2&third=3&fourth=4 HTTP/1.1\r\n" +
+            "Host: whatever\r\n" +
+            "Content-Type: application/x-www-form-urlencoded\n" +
+            "Connection: close\n" +
+            "Content-Length: 34\n" +
+            "\n" +
+            "fifth=5&sixth=6&seventh=7&eighth=8";
+
+        String responses = _connector.getResponse(request);
+        assertTrue(responses.startsWith("HTTP/1.1 200"));
+        assertThat(new ArrayList<>(reference.get().keySet()), is(Arrays.asList("first", "second", "third", "fourth", "fifth", "sixth", "seventh", "eighth")));
+    }
+
+    @Test
+    public void testParameterExtractionOrderingWithMerge() throws Exception
+    {
+        AtomicReference<Map<String, String[]>> reference = new AtomicReference<>();
+        _handler._checker = new RequestTester()
+        {
+            @Override
+            public boolean check(HttpServletRequest request, HttpServletResponse response)
+            {
+                reference.set(request.getParameterMap());
+                return true;
+            }
+        };
+
+        String request = "POST /?a=1&b=2&c=3&a=4 HTTP/1.1\r\n" +
+            "Host: whatever\r\n" +
+            "Content-Type: application/x-www-form-urlencoded\n" +
+            "Connection: close\n" +
+            "Content-Length: 11\n" +
+            "\n" +
+            "c=5&b=6&a=7";
+
+        String responses = _connector.getResponse(request);
+        Map<String, String[]> returnedMap = reference.get();
+        assertTrue(responses.startsWith("HTTP/1.1 200"));
+        assertThat(new ArrayList<>(returnedMap.keySet()), is(Arrays.asList("a", "b", "c")));
+        assertTrue(Arrays.equals(returnedMap.get("a"), new String[]{"1", "4", "7"}));
+        assertTrue(Arrays.equals(returnedMap.get("b"), new String[]{"2", "6"}));
+        assertTrue(Arrays.equals(returnedMap.get("c"), new String[]{"3", "5"}));
+    }
+
     @Test
     public void testParamExtractionBadSequence() throws Exception
     {

jetty-util/src/main/java/org/eclipse/jetty/util/MultiMap.java

@@ -15,8 +15,8 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.HashMap;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -26,7 +26,7 @@
  * @param <V> the entry type for multimap values
  */
 @SuppressWarnings("serial")
-public class MultiMap<V> extends HashMap<String, List<V>>
+public class MultiMap<V> extends LinkedHashMap<String, List<V>>
 {
     public MultiMap()
     {
@@ -316,13 +316,13 @@ public boolean containsSimpleValue(V value)
     @Override
     public String toString()
     {
-        Iterator<Entry<String, List<V>>> iter = entrySet().iterator();
+        Iterator<Map.Entry<String, List<V>>> iter = entrySet().iterator();
         StringBuilder sb = new StringBuilder();
         sb.append('{');
         boolean delim = false;
         while (iter.hasNext())
         {
-            Entry<String, List<V>> e = iter.next();
+            Map.Entry<String, List<V>> e = iter.next();
             if (delim)
             {
                 sb.append(", ");
@@ -350,7 +350,7 @@ public String toString()
      */
     public Map<String, String[]> toStringArrayMap()
     {
-        HashMap<String, String[]> map = new HashMap<String, String[]>(size() * 3 / 2)
+        Map<String, String[]> map = new LinkedHashMap<String, String[]>(size() * 3 / 2)
         {
             @Override
             public String toString()