Skip to content

Commit

Permalink
Issue #2074 Do not merge query strings
Browse files Browse the repository at this point in the history 
Do not merge the query string itself, only the parameter map.
The query string is either the original or replaced by the one from the dispatch.

Signed-off-by: Greg Wilkins <[email protected]>
  • Loading branch information
@gregw
gregw committed Mar 30, 2020
1 parent 0f5b47b commit 239b7c5
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 58 deletions.
35 changes: 2 additions & 33 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2368,7 +2368,7 @@ public void mergeQueryParameters(String oldQuery, String newQuery, boolean updat
if (newQueryParams == null || newQueryParams.size() == 0)
mergedQueryParams = oldQueryParams == null ? NO_PARAMS : oldQueryParams;
else if (oldQueryParams == null || oldQueryParams.size() == 0)
mergedQueryParams = newQueryParams == null ? NO_PARAMS : newQueryParams;
mergedQueryParams = newQueryParams;
else
{
// Parameters values are accumulated.
Expand All @@ -2380,38 +2380,7 @@ else if (oldQueryParams == null || oldQueryParams.size() == 0)
resetParameters();

if (updateQueryString)
{
if (newQuery == null)
setQueryString(oldQuery);
else if (oldQuery == null)
setQueryString(newQuery);
else if (oldQueryParams.keySet().stream().anyMatch(newQueryParams.keySet()::contains))
{
// Build the new merged query string, parameters in the
// new query string hide parameters in the old query string.
StringBuilder mergedQuery = new StringBuilder();
if (newQuery != null)
mergedQuery.append(newQuery);
for (Map.Entry<String, List<String>> entry : mergedQueryParams.entrySet())
{
if (newQueryParams != null && newQueryParams.containsKey(entry.getKey()))
continue;
for (String value : entry.getValue())
{
if (mergedQuery.length() > 0)
mergedQuery.append("&");
URIUtil.encodePath(mergedQuery, entry.getKey());
mergedQuery.append('=');
URIUtil.encodePath(mergedQuery, value);
}
}
setQueryString(mergedQuery.toString());
}
else
{
setQueryString(newQuery + '&' + oldQuery);
}
}
setQueryString(newQuery == null ? oldQuery : newQuery);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextDispatchWithQueryStrings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,14 @@ else if ("/firstDispatchWithNewQueryString".equals(uri))
{
AsyncContext async = request.startAsync();
async.dispatch("/secondDispatchNewValueForExistingQueryString?newQueryString=newValue");
assertEquals("newQueryString=initialValue&initialParam=right", queryString);
assertEquals("newQueryString=initialValue", queryString);
}
else
{
response.setContentType("text/html");
response.setStatus(HttpServletResponse.SC_OK);
response.getWriter().println("<h1>woohhooooo</h1>");
assertEquals("newQueryString=newValue&initialParam=right", queryString);
assertEquals("newQueryString=newValue", queryString);
}
}
}
Expand Down
14 changes: 7 additions & 7 deletions jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncServletTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -533,12 +533,12 @@ public void testFwdStartDispatch() throws Exception
assertThat(response, startsWith("HTTP/1.1 200 OK"));
assertThat(__history, contains(
"FWD REQUEST /ctx/fwd/info?start=200&dispatch=20",
"FORWARD /ctx/path1?forward=true&start=200&dispatch=20",
"FORWARD /ctx/path1?forward=true",
"initial",
"start",
"dispatch",
"FWD ASYNC /ctx/fwd/info?start=200&dispatch=20",
"FORWARD /ctx/path1?forward=true&start=200&dispatch=20",
"FORWARD /ctx/path1?forward=true",
"!initial",
"onComplete"));
assertContains("DISPATCHED", response);
Expand All @@ -551,7 +551,7 @@ public void testFwdStartDispatchPath() throws Exception
assertThat(response, startsWith("HTTP/1.1 200 OK"));
assertThat(__history, contains(
"FWD REQUEST /ctx/fwd/info?start=200&dispatch=20&path=/path2",
"FORWARD /ctx/path1?forward=true&start=200&dispatch=20&path=/path2",
"FORWARD /ctx/path1?forward=true",
"initial",
"start",
"dispatch",
Expand All @@ -568,11 +568,11 @@ public void testFwdWrapStartDispatch() throws Exception
assertThat(response, startsWith("HTTP/1.1 200 OK"));
assertThat(__history, contains(
"FWD REQUEST /ctx/fwd/info?wrap=true&start=200&dispatch=20",
"FORWARD /ctx/path1?forward=true&wrap=true&start=200&dispatch=20",
"FORWARD /ctx/path1?forward=true",
"initial",
"start",
"dispatch",
"ASYNC /ctx/path1?forward=true&wrap=true&start=200&dispatch=20",
"ASYNC /ctx/path1?forward=true",
"wrapped REQ RSP",
"!initial",
"onComplete"));
Expand All @@ -586,11 +586,11 @@ public void testFwdWrapStartDispatchPath() throws Exception
assertThat(response, startsWith("HTTP/1.1 200 OK"));
assertThat(__history, contains(
"FWD REQUEST /ctx/fwd/info?wrap=true&start=200&dispatch=20&path=/path2",
"FORWARD /ctx/path1?forward=true&wrap=true&start=200&dispatch=20&path=/path2",
"FORWARD /ctx/path1?forward=true",
"initial",
"start",
"dispatch",
"ASYNC /ctx/path2?forward=true&wrap=true&start=200&dispatch=20&path=/path2",
"ASYNC /ctx/path2?forward=true",
"wrapped REQ RSP",
"!initial",
"onComplete"));
Expand Down
21 changes: 8 additions & 13 deletions jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherForwardTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ public void testQueryReplacedByForwardWithQuery() throws Exception
CountDownLatch latch = new CountDownLatch(1);
final String query1 = "a=1%20one&b=2%20two";
final String query2 = "a=3%20three";
final String query3 = "a=3%20three&b=2%20two";
servlet1 = new HttpServlet()
{
@Override
Expand All @@ -163,7 +162,7 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
{
checkThat(req.getQueryString(), Matchers.equalTo(query3));
checkThat(req.getQueryString(), Matchers.equalTo(query2));
checkThat(req.getParameter("a"), Matchers.equalTo("3 three"));
checkThat(req.getParameter("b"), Matchers.equalTo("2 two"));
}
Expand All @@ -186,13 +185,12 @@ public void testQueryMergedByForwardWithQuery() throws Exception
{
// 1. request /one?a=1
// 1. forward /two?b=2
// 2. assert query => a=1&b=2
// 2. assert query => b=2
// 1. assert query => a=1

CountDownLatch latch = new CountDownLatch(1);
final String query1 = "a=1%20one";
final String query2 = "b=2%20two";
final String query3 = "b=2%20two&a=1%20one";
servlet1 = new HttpServlet()
{
@Override
Expand All @@ -212,7 +210,7 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
{
checkThat(req.getQueryString(), Matchers.equalTo(query3));
checkThat(req.getQueryString(), Matchers.equalTo(query2));
checkThat(req.getParameter("a"), Matchers.equalTo("1 one"));
checkThat(req.getParameter("b"), Matchers.equalTo("2 two"));
}
Expand Down Expand Up @@ -348,13 +346,12 @@ public void testQueryAggregatesWithFormMergedByForwardWithQuery() throws Excepti
{
// 1. request /one?a=1 + content b=2
// 1. forward /two?c=3
// 2. assert query => a=1&c=3 + params => a=1&b=2&c=3
// 2. assert query => c=3 + params => a=1&b=2&c=3
// 1. assert query => a=1 + params => a=1&b=2

CountDownLatch latch = new CountDownLatch(1);
final String query1 = "a=1%20one";
final String query2 = "c=3%20three";
final String query3 = "c=3%20three&a=1%20one";
final String form = "b=2%20two";
servlet1 = new HttpServlet()
{
Expand All @@ -377,7 +374,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws S
@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
{
checkThat(req.getQueryString(), Matchers.equalTo(query3));
checkThat(req.getQueryString(), Matchers.equalTo(query2));
checkThat(req.getParameter("a"), Matchers.equalTo("1 one"));
checkThat(req.getParameter("b"), Matchers.equalTo("2 two"));
checkThat(req.getParameter("c"), Matchers.equalTo("3 three"));
Expand Down Expand Up @@ -405,13 +402,12 @@ public void testQueryAggregatesWithFormBeforeAndAfterForward() throws Exception
// 1. request /one?a=1 + content b=2
// 1. assert params => a=1&b=2
// 1. forward /two?c=3
// 2. assert query => a=1&c=3 + params => a=1&b=2&c=3
// 2. assert query => c=3 + params => a=1&b=2&c=3
// 1. assert query => a=1 + params => a=1&b=2

CountDownLatch latch = new CountDownLatch(1);
final String query1 = "a=1%20one";
final String query2 = "c=3%20three";
final String query3 = "c=3%20three&a=1%20one";
final String form = "b=2%20two";
servlet1 = new HttpServlet()
{
Expand All @@ -436,7 +432,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws S
@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
{
checkThat(req.getQueryString(), Matchers.equalTo(query3));
checkThat(req.getQueryString(), Matchers.equalTo(query2));
checkThat(req.getParameter("a"), Matchers.equalTo("1 one"));
checkThat(req.getParameter("b"), Matchers.equalTo("2 two"));
checkThat(req.getParameter("c"), Matchers.equalTo("3 three"));
Expand Down Expand Up @@ -513,7 +509,6 @@ public void testContentCanBeReadViaInputStreamAfterForwardWithQuery() throws Exc
CountDownLatch latch = new CountDownLatch(1);
final String query1 = "a=1%20one";
final String query2 = "b=2%20two";
final String query3 = "b=2%20two&a=1%20one";
final String form = "c=3%20three";
servlet1 = new HttpServlet()
{
Expand All @@ -534,7 +529,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws S
@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException
{
checkThat(req.getQueryString(), Matchers.equalTo(query3));
checkThat(req.getQueryString(), Matchers.equalTo(query2));
ServletInputStream input = req.getInputStream();
for (int i = 0; i < form.length(); ++i)
{
Expand Down
6 changes: 3 additions & 3 deletions jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t

assertEquals(null, request.getPathInfo());
assertEquals(null, request.getPathTranslated());
assertEquals("do=end&do=the&test=1", request.getQueryString());
assertEquals("do=end&do=the", request.getQueryString());
assertEquals("/context/AssertForwardServlet", request.getRequestURI());
assertEquals("/context", request.getContextPath());
assertEquals("/AssertForwardServlet", request.getServletPath());
Expand Down Expand Up @@ -788,8 +788,8 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
q2.decode(query.getString("else"));
String russian = q2.encode();
assertThat(russian, is("%D0%B2%D1%8B%D0%B1%D1%80%D0%B0%D0%BD%D0%BE=%D0%A2%D0%B5%D0%BC%D0%BF%D0%B5%D1%80%D0%B0%D1%82%D1%83%D1%80%D0%B0"));
assertThat(query.getString("test"), is("1"));
assertThat(query.containsKey("foreign"), is(true));
assertThat(query.containsKey("test"), is(false));
assertThat(query.containsKey("foreign"), is(false));

String[] vals = request.getParameterValues("foreign");
assertTrue(vals != null);
Expand Down

0 comments on commit 239b7c5

Please sign in to comment.