From bd143a674ba7fbb3c7e7e450c8102e9a4bd95172 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 22 Aug 2018 08:29:12 +1000 Subject: [PATCH] Issue #2787 BadMessage if bad query detected in dispatcher (#2827) Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/server/Request.java | 11 +++- .../eclipse/jetty/servlet/ServletHandler.java | 8 +++ .../eclipse/jetty/servlet/DispatcherTest.java | 64 ++++++++++++++++++- 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 6068f7c34d0..b75d0e6548e 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -2390,8 +2390,6 @@ public class Request implements HttpServletRequest /* ------------------------------------------------------------ */ public void mergeQueryParameters(String oldQuery,String newQuery, boolean updateQueryString) { - // TODO This is seriously ugly - MultiMap newQueryParams = null; // Have to assume ENCODING because we can't know otherwise. if (newQuery!=null) @@ -2404,7 +2402,14 @@ public class Request implements HttpServletRequest if (oldQueryParams == null && oldQuery != null) { oldQueryParams = new MultiMap<>(); - UrlEncoded.decodeTo(oldQuery, oldQueryParams, getQueryEncoding()); + try + { + UrlEncoded.decodeTo(oldQuery, oldQueryParams, getQueryEncoding()); + } + catch(Throwable th) + { + throw new BadMessageException(400,"Bad query encoding",th); + } } MultiMap mergedQueryParams; diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java index 269ad2b44c1..ee2f0e01d0c 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java @@ -50,6 +50,7 @@ import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.http.PathMap; @@ -647,8 +648,15 @@ public class ServletHandler extends ScopedHandler else response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); } + else if (th instanceof BadMessageException) + { + BadMessageException bme = (BadMessageException)th; + response.sendError(bme.getCode(),bme.getMessage()); + } else + { response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } } else { diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java index e7fb37e280c..c2657eba638 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.servlet; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.assertEquals; @@ -59,6 +60,8 @@ import org.eclipse.jetty.server.handler.ResourceHandler; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.UrlEncoded; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.StacklessLogging; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -156,6 +159,42 @@ public class DispatcherTest assertEquals(expected, responses); } + @Test + public void testForwardWithBadParams() throws Exception + { + try(StacklessLogging nostack = new StacklessLogging(ServletHandler.class)) + { + Log.getLogger(ServletHandler.class).info("Expect Not valid UTF8 warnings..."); + _contextHandler.addServlet(AlwaysForwardServlet.class, "/forward/*"); + _contextHandler.addServlet(EchoServlet.class, "/echo/*"); + + String response; + + response = _connector.getResponse("GET /context/forward/?echo=allgood HTTP/1.0\n\n"); + assertThat(response,containsString(" 200 OK")); + assertThat(response,containsString("allgood")); + + response = _connector.getResponse("GET /context/forward/params?echo=allgood HTTP/1.0\n\n"); + assertThat(response,containsString(" 200 OK")); + assertThat(response,containsString("allgood")); + assertThat(response,containsString("forward")); + + response = _connector.getResponse("GET /context/forward/badparams?echo=badparams HTTP/1.0\n\n"); + assertThat(response,containsString(" 500 ")); + + response = _connector.getResponse("GET /context/forward/?echo=badclient&bad=%88%A4 HTTP/1.0\n\n"); + assertThat(response,containsString(" 400 ")); + + response = _connector.getResponse("GET /context/forward/params?echo=badclient&bad=%88%A4 HTTP/1.0\n\n"); + assertThat(response,containsString(" 400 ")); + + response = _connector.getResponse("GET /context/forward/badparams?echo=badclientandparam&bad=%88%A4 HTTP/1.0\n\n"); + assertThat(response,containsString(" 500 ")); + } + } + + + @Test public void testInclude() throws Exception { @@ -358,6 +397,20 @@ public class DispatcherTest dispatcher.forward(request, response); } } + + public static class AlwaysForwardServlet extends HttpServlet implements Servlet + { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + if ("/params".equals(request.getPathInfo())) + getServletContext().getRequestDispatcher("/echo?echo=forward").forward(request, response); + else if ("/badparams".equals(request.getPathInfo())) + getServletContext().getRequestDispatcher("/echo?echo=forward&fbad=%88%A4").forward(request, response); + else + getServletContext().getRequestDispatcher("/echo").forward(request, response); + } + } public static class ForwardNonUTF8Servlet extends HttpServlet implements Servlet @@ -480,15 +533,20 @@ public class DispatcherTest @Override public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException { - String echoText = req.getParameter("echo"); + String[] echoText = req.getParameterValues("echo"); - if ( echoText == null ) + if ( echoText == null || echoText.length==0) { throw new ServletException("echo is a required parameter"); } + else if (echoText.length==1) + { + res.getWriter().print(echoText[0]); + } else { - res.getWriter().print(echoText); + for (String text:echoText) + res.getWriter().print(text); } } }