From f2e96804317131972cf6641e4ce82bd6c4e55c45 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 16 Feb 2022 11:34:30 -0600 Subject: [PATCH] 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 --- .../org/eclipse/jetty/server/RequestTest.java | 59 +++++++++++++++++++ .../java/org/eclipse/jetty/util/MultiMap.java | 10 ++-- 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 21a41ea964a..73269c2d738 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -34,6 +34,7 @@ import java.util.EnumSet; 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 class RequestTest assertTrue(responses.startsWith("HTTP/1.1 200")); } + @Test + public void testParameterExtractionKeepOrderingIntact() throws Exception + { + AtomicReference> 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> 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 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 { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiMap.java b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiMap.java index fc2d818af9d..481b28f350d 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiMap.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiMap.java @@ -15,8 +15,8 @@ package org.eclipse.jetty.util; 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 @@ import java.util.Map; * @param the entry type for multimap values */ @SuppressWarnings("serial") -public class MultiMap extends HashMap> +public class MultiMap extends LinkedHashMap> { public MultiMap() { @@ -316,13 +316,13 @@ public class MultiMap extends HashMap> @Override public String toString() { - Iterator>> iter = entrySet().iterator(); + Iterator>> iter = entrySet().iterator(); StringBuilder sb = new StringBuilder(); sb.append('{'); boolean delim = false; while (iter.hasNext()) { - Entry> e = iter.next(); + Map.Entry> e = iter.next(); if (delim) { sb.append(", "); @@ -350,7 +350,7 @@ public class MultiMap extends HashMap> */ public Map toStringArrayMap() { - HashMap map = new HashMap(size() * 3 / 2) + Map map = new LinkedHashMap(size() * 3 / 2) { @Override public String toString()