From 1c5b84e5942795d3f8a0c95cd2e6da5ed2d529da Mon Sep 17 00:00:00 2001 From: Manfred Riem Date: Mon, 15 Jan 2024 16:12:37 -0600 Subject: [PATCH] Fixes #3628 - Make sure RequestDispatcher.include honors parameter precendence --- .../impl/DefaultServletRequestDispatcher.java | 49 +++++++++++++--- .../core/impl/RequestDispatcherTest.java | 57 ++++++++++++++++++- 2 files changed, 96 insertions(+), 10 deletions(-) diff --git a/core/impl/src/main/java/cloud/piranha/core/impl/DefaultServletRequestDispatcher.java b/core/impl/src/main/java/cloud/piranha/core/impl/DefaultServletRequestDispatcher.java index dea6a89579..609f9ec50e 100644 --- a/core/impl/src/main/java/cloud/piranha/core/impl/DefaultServletRequestDispatcher.java +++ b/core/impl/src/main/java/cloud/piranha/core/impl/DefaultServletRequestDispatcher.java @@ -263,7 +263,7 @@ public void include(ServletRequest servletRequest, ServletResponse servletRespon * Unwrap the passed ServletRequest to the underlying WebApplicationRequest. */ WebApplicationRequest request = unwrap(servletRequest, WebApplicationRequest.class); - + /** * Set the include attributes. */ @@ -272,13 +272,17 @@ public void include(ServletRequest servletRequest, ServletResponse servletRespon request.setAttribute(INCLUDE_QUERY_STRING, request.getQueryString()); request.setAttribute(INCLUDE_REQUEST_URI, request.getRequestURI()); request.setAttribute(INCLUDE_SERVLET_PATH, request.getServletPath()); - + /** * Set the new dispatcher type. */ request.setDispatcherType(INCLUDE); request.setServletPath(path == null ? "/" + servletEnvironment.getServletName() : getServletPath(path)); request.setPathInfo(null); + + if (path != null) { + request.setQueryString(getQueryString(path)); + } invocationFinder.addFilters(INCLUDE, servletInvocation, request.getServletPath(), ""); @@ -286,6 +290,33 @@ public void include(ServletRequest servletRequest, ServletResponse servletRespon request.setAsyncSupported(request.isAsyncSupported() && isAsyncSupportedInChain()); } + /* + * Aggregate the request parameters with giving the new parameter values + * precedence over the old ones. + */ + if (request.getQueryString() != null) { + String queryString = request.getQueryString(); + Map parameters = request.getModifiableParameterMap(); + for (String param : queryString.split("&")) { + String[] pair = param.split("="); + String key = URLDecoder.decode(pair[0], StandardCharsets.UTF_8); + String value = ""; + if (pair.length > 1) { + value = URLDecoder.decode(pair[1], StandardCharsets.UTF_8); + } + String[] values = parameters.get(key); + if (values == null) { + values = new String[]{value}; + parameters.put(key, values); + } else { + String[] newValues = new String[values.length + 1]; + System.arraycopy(values, 0, newValues, 1, values.length); + newValues[0] = value; + parameters.put(key, newValues); + } + } + } + /* * Execute the filter chain. */ @@ -293,7 +324,7 @@ public void include(ServletRequest servletRequest, ServletResponse servletRespon servletEnvironment.getWebApplication().linkRequestAndResponse(servletRequest, servletResponse); servletInvocation.getFilterChain().doFilter(servletRequest, servletResponse); servletEnvironment.getWebApplication().unlinkRequestAndResponse(servletRequest, servletResponse); - } catch(Exception e) { + } catch (Exception e) { rethrow(e); } finally { /* @@ -301,7 +332,7 @@ public void include(ServletRequest servletRequest, ServletResponse servletRespon */ request.setServletPath((String) request.getAttribute(INCLUDE_SERVLET_PATH)); request.setPathInfo((String) request.getAttribute(INCLUDE_PATH_INFO)); - + /* * Remove include attributes. */ @@ -699,7 +730,7 @@ private void dispatchSyncForward( /* * If response is already committed we need to throw an IllegalStateException. - */ + */ if (servletResponse.isCommitted()) { throw new IllegalStateException("Response already committed"); } @@ -723,7 +754,7 @@ private void dispatchSyncForward( * Unwrap the passed ServletRequest to the underlying WebApplicationRequest. */ WebApplicationRequest request = unwrap(servletRequest, WebApplicationRequest.class); - + /* * Set the forward attributes. */ @@ -732,7 +763,7 @@ private void dispatchSyncForward( request.setAttribute(FORWARD_QUERY_STRING, request.getQueryString()); request.setAttribute(FORWARD_REQUEST_URI, request.getRequestURI()); request.setAttribute(FORWARD_SERVLET_PATH, request.getServletPath()); - + /* * Set the new dispatcher type. */ @@ -748,7 +779,7 @@ private void dispatchSyncForward( request.setServletPath("/" + servletEnvironment.getServletName()); request.setQueryString(request.getQueryString()); } - + /* * Aggregate the request parameters with giving the new parameter values * precedence over the old ones. @@ -775,7 +806,7 @@ private void dispatchSyncForward( } } } - + invocationFinder.addFilters(FORWARD, servletInvocation, request.getServletPath(), ""); if (servletInvocation.getServletEnvironment() != null) { diff --git a/core/impl/src/test/java/cloud/piranha/core/impl/RequestDispatcherTest.java b/core/impl/src/test/java/cloud/piranha/core/impl/RequestDispatcherTest.java index 57427aeb09..30e143e618 100644 --- a/core/impl/src/test/java/cloud/piranha/core/impl/RequestDispatcherTest.java +++ b/core/impl/src/test/java/cloud/piranha/core/impl/RequestDispatcherTest.java @@ -141,7 +141,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t .build(); webApplication.initialize(); webApplication.start(); - + WebApplicationRequest request = new DefaultWebApplicationRequestBuilder() .webApplication(webApplication) .build(); @@ -739,6 +739,61 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t assertEquals("123456", new String(byteOutput.toByteArray())); } + /** + * Test an include with a query string. + * + * @throws Exception when an error occurs. + */ + @Test + void testIncludeWithQueryString() throws Exception { + WebApplication webApplication = new DefaultWebApplicationBuilder() + .servlet("Include", new HttpServlet() { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { + /* + * Force the original parameter to be parsed out of the query string. + */ + req.getParameter("p"); + /* + * Dispatch with a new parameter value. + */ + getServletContext() + .getRequestDispatcher("/include2?p=New") + .include(req, resp); + } + }) + .servletMapping("Include", "/include") + .servlet("Include2", new HttpServlet() { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { + /* + * We should be getting the new parameter value here as it takes precendence. + */ + resp.getWriter().print(req.getParameterMap().get("p")[0]); + } + }) + .servletMapping("Include2", "/include2") + .build(); + webApplication.initialize(); + webApplication.start(); + + WebApplicationRequest request = new DefaultWebApplicationRequestBuilder() + .webApplication(webApplication) + .servletPath("/include") + .queryString("p=Original") + .build(); + + WebApplicationResponse response = new DefaultWebApplicationResponseBuilder() + .webApplication(webApplication) + .bodyOnly(true) + .build(); + ByteArrayOutputStream byteOutput = new ByteArrayOutputStream(); + response.getWebApplicationOutputStream().setOutputStream(byteOutput); + + webApplication.service(request, response); + assertEquals("New", byteOutput.toString()); + } + @Test void testErrorDispatcher() throws Exception { DefaultWebApplication webApp = new DefaultWebApplication();