From 4993291af050185295bfe87dadf2f48158f84d31 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 20 Jan 2023 08:16:43 -0600 Subject: [PATCH 1/3] Wrong value of `RequestDispatcher.FORWARD_CONTEXT_PATH` attribute on root context (#9123) * Wrong value of RequestDispatcher.FORWARD_CONTEXT_PATH on root context * Fixes #9119 - uses proper context path that satisfies the root context rules of the servlet spec Signed-off-by: Joakim Erdfelt --- .../org/eclipse/jetty/server/Dispatcher.java | 2 +- .../eclipse/jetty/servlet/DispatcherTest.java | 218 +++++++++++++++--- .../test/resources/jetty-logging.properties | 1 + 3 files changed, 184 insertions(+), 37 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java index 705827bca470..13971f65c5c8 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java @@ -178,7 +178,7 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc if (old_attr.getAttribute(FORWARD_REQUEST_URI) == null) baseRequest.setAttributes(new ForwardAttributes(old_attr, old_uri.getPath(), - old_context == null ? null : old_context.getContextHandler().getContextPathEncoded(), + baseRequest.getContextPath(), baseRequest.getPathInContext(), source_mapping, old_uri.getQuery())); 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 ce0c4abdf9b2..3006569a219b 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 @@ -13,13 +13,21 @@ package org.eclipse.jetty.servlet; +import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.InputStream; import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.PrintWriter; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collections; import java.util.EnumSet; import java.util.List; +import java.util.Objects; +import java.util.Properties; +import java.util.stream.Collectors; import javax.servlet.AsyncContext; import javax.servlet.DispatcherType; import javax.servlet.Filter; @@ -43,6 +51,8 @@ import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponseWrapper; +import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.Dispatcher; import org.eclipse.jetty.server.HttpChannel; @@ -52,10 +62,11 @@ import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ContextHandlerCollection; import org.eclipse.jetty.server.handler.ResourceHandler; -import org.eclipse.jetty.toolchain.test.MavenTestingUtils; +import org.eclipse.jetty.toolchain.test.MavenPaths; import org.eclipse.jetty.util.MultiMap; import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.UrlEncoded; +import org.hamcrest.Matcher; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -93,7 +104,7 @@ public void init() throws Exception _contextHandler.setContextPath("/context"); _contextCollection.addHandler(_contextHandler); _resourceHandler = new ResourceHandler(); - _resourceHandler.setResourceBase(MavenTestingUtils.getTestResourceDir("dispatchResourceTest").getAbsolutePath()); + _resourceHandler.setResourceBase(MavenPaths.findTestResourceDir("dispatchResourceTest").toUri().toASCIIString()); _resourceHandler.setPathInfoOnly(true); ContextHandler resourceContextHandler = new ContextHandler("/resource"); resourceContextHandler.setHandler(_resourceHandler); @@ -112,21 +123,135 @@ public void destroy() throws Exception } @Test - public void testForward() throws Exception + public void testForwardInContext() throws Exception { - _contextHandler.addServlet(ForwardServlet.class, "/ForwardServlet/*"); - _contextHandler.addServlet(AssertForwardServlet.class, "/AssertForwardServlet/*"); + _contextHandler.addServlet(DumpForwardServlet.class, "/DumpForward/*"); - String expected = - "HTTP/1.1 200 OK\r\n" + - "Content-Type: text/html\r\n" + - "Content-Length: 7\r\n" + - "\r\n" + - "FORWARD"; + ServletHolder forwardServlet = new ServletHolder( + new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + RequestDispatcher dispatcher = request.getRequestDispatcher("/DumpForward/foo?a=query"); + dispatcher.forward(request, response); + } + } + ); + _contextHandler.addServlet(forwardServlet, "/DoForward/*"); - String responses = _connector.getResponse("GET /context/ForwardServlet?do=assertforward&do=more&test=1 HTTP/1.0\n\n"); + String rawRequest = "GET /context/DoForward?do=more&test=example HTTP/1.1\r\n" + + "Host: local\r\n" + + "Connection: close\r\n" + + "\r\n"; - assertEquals(expected, responses); + String rawResponse = _connector.getResponse(rawRequest); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + + assertThat(response.getStatus(), is(200)); + assertThat(response.getField(HttpHeader.CONTENT_TYPE).getValue(), startsWith("text/plain")); + + Properties responseProps = new Properties(); + try (InputStream inputStream = new ByteArrayInputStream(response.getContentBytes())) + { + responseProps.load(inputStream); + } + + dumpProperties(responseProps); + + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_REQUEST_URI), is("'/context/DoForward'")); + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_CONTEXT_PATH), is("'/context'")); + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_SERVLET_PATH), is("'/DoForward'")); + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_PATH_INFO), is("")); + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_QUERY_STRING), is("'do=more&test=example'")); + assertPropertyValue(responseProps, String.format("request.attr[%s].mappingMatch", RequestDispatcher.FORWARD_MAPPING), is("PATH")); + assertPropertyValue(responseProps, String.format("request.attr[%s].matchValue", RequestDispatcher.FORWARD_MAPPING), is("'DoForward'")); + assertPropertyValue(responseProps, String.format("request.attr[%s].pattern", RequestDispatcher.FORWARD_MAPPING), is("'/DoForward/*'")); + + assertPropertyValue(responseProps, "request.contextPath", is("'/context'")); + assertPropertyValue(responseProps, "request.dispatcherType", is("FORWARD")); + assertPropertyValue(responseProps, "request.pathInfo", is("'/foo'")); + assertPropertyValue(responseProps, "request.pathTranslated", is("")); + assertPropertyValue(responseProps, "request.queryString", is("'a=query'")); + assertPropertyValue(responseProps, "request.requestURI", is("'/context/DumpForward/foo'")); + assertPropertyValue(responseProps, "request.servletPath", is("'/DumpForward'")); + } + + @Test + public void testForwardInRoot() throws Exception + { + ServletContextHandler rootContextHandler = new ServletContextHandler(); + rootContextHandler.setServer(_server); + rootContextHandler.setContextPath("/"); + rootContextHandler.start(); + + _contextCollection.addHandler(rootContextHandler); + + rootContextHandler.addServlet(DumpForwardServlet.class, "/DumpForward/*"); + + ServletHolder forwardServlet = new ServletHolder( + new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + RequestDispatcher dispatcher = request.getRequestDispatcher("/DumpForward/foo?a=query"); + dispatcher.forward(request, response); + } + } + ); + rootContextHandler.addServlet(forwardServlet, "/DoForward/*"); + + String rawRequest = "GET /DoForward?do=more&test=example HTTP/1.1\r\n" + + "Host: local\r\n" + + "Connection: close\r\n" + + "\r\n"; + + String rawResponse = _connector.getResponse(rawRequest); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + + assertThat(response.getStatus(), is(200)); + assertThat(response.getField(HttpHeader.CONTENT_TYPE).getValue(), startsWith("text/plain")); + + Properties responseProps = new Properties(); + try (InputStream inputStream = new ByteArrayInputStream(response.getContentBytes())) + { + responseProps.load(inputStream); + } + + dumpProperties(responseProps); + + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_REQUEST_URI), is("'/DoForward'")); + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_CONTEXT_PATH), is("''")); + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_SERVLET_PATH), is("'/DoForward'")); + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_PATH_INFO), is("")); + assertPropertyValue(responseProps, String.format("request.attr[%s]", RequestDispatcher.FORWARD_QUERY_STRING), is("'do=more&test=example'")); + assertPropertyValue(responseProps, String.format("request.attr[%s].mappingMatch", RequestDispatcher.FORWARD_MAPPING), is("PATH")); + assertPropertyValue(responseProps, String.format("request.attr[%s].matchValue", RequestDispatcher.FORWARD_MAPPING), is("'DoForward'")); + assertPropertyValue(responseProps, String.format("request.attr[%s].pattern", RequestDispatcher.FORWARD_MAPPING), is("'/DoForward/*'")); + + assertPropertyValue(responseProps, "request.contextPath", is("''")); + assertPropertyValue(responseProps, "request.dispatcherType", is("FORWARD")); + assertPropertyValue(responseProps, "request.pathInfo", is("'/foo'")); + assertPropertyValue(responseProps, "request.pathTranslated", is("")); + assertPropertyValue(responseProps, "request.queryString", is("'a=query'")); + assertPropertyValue(responseProps, "request.requestURI", is("'/DumpForward/foo'")); + assertPropertyValue(responseProps, "request.servletPath", is("'/DumpForward'")); + } + + private void assertPropertyValue(Properties props, String keyName, Matcher matcher) + { + assertThat(keyName, props.getProperty(keyName), matcher); + } + + public static void dumpProperties(Properties props) + { + if (LOG.isDebugEnabled()) + { + List keys = Collections.list(props.keys()).stream().map(Objects::toString).sorted().collect(Collectors.toList()); + LOG.debug("Dump Properties: has {} key(s)", keys.size()); + keys.forEach(keyName -> System.err.printf(" %s=%s%n", keyName, props.getProperty(keyName))); + } } @Test @@ -467,7 +592,7 @@ public void testWrappedForwardCloseIntercepted() throws Exception { // Add filter that wraps response, intercepts close and writes after doChain _contextHandler.addFilter(WrappingFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST)); - testForward(); + testForwardInContext(); } @Test @@ -878,35 +1003,56 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t } } - public static class AssertForwardServlet extends HttpServlet implements Servlet + public static class DumpForwardServlet extends HttpServlet { @Override - protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException { - assertEquals("/context/ForwardServlet", request.getAttribute(Dispatcher.FORWARD_REQUEST_URI)); - assertEquals("/context", request.getAttribute(Dispatcher.FORWARD_CONTEXT_PATH)); - assertEquals("/ForwardServlet", request.getAttribute(Dispatcher.FORWARD_SERVLET_PATH)); - assertEquals(null, request.getAttribute(Dispatcher.FORWARD_PATH_INFO)); - assertEquals("do=assertforward&do=more&test=1", request.getAttribute(Dispatcher.FORWARD_QUERY_STRING)); - HttpServletMapping fwdMapping = (HttpServletMapping)request.getAttribute(Dispatcher.FORWARD_MAPPING); - assertNotNull(fwdMapping); - assertEquals("ForwardServlet", fwdMapping.getMatchValue()); + response.setCharacterEncoding("utf-8"); + response.setContentType("text/plain"); + PrintWriter writer = new PrintWriter(new OutputStreamWriter(response.getOutputStream(), StandardCharsets.UTF_8)); - List expectedAttributeNames = Arrays.asList(Dispatcher.FORWARD_REQUEST_URI, Dispatcher.FORWARD_CONTEXT_PATH, - Dispatcher.FORWARD_SERVLET_PATH, Dispatcher.FORWARD_QUERY_STRING, Dispatcher.FORWARD_MAPPING); - List requestAttributeNames = Collections.list(request.getAttributeNames()); - assertTrue(requestAttributeNames.containsAll(expectedAttributeNames)); + List attrNames = List.of(RequestDispatcher.FORWARD_REQUEST_URI, + RequestDispatcher.FORWARD_CONTEXT_PATH, + RequestDispatcher.FORWARD_SERVLET_PATH, + RequestDispatcher.FORWARD_PATH_INFO, + RequestDispatcher.FORWARD_QUERY_STRING, + RequestDispatcher.FORWARD_MAPPING); - assertEquals(null, request.getPathInfo()); - assertEquals(null, request.getPathTranslated()); - assertEquals("do=end&do=the", request.getQueryString()); - assertEquals("/context/AssertForwardServlet", request.getRequestURI()); - assertEquals("/context", request.getContextPath()); - assertEquals("/AssertForwardServlet", request.getServletPath()); + for (String attrName : attrNames) + { + Object value = request.getAttribute(attrName); + writer.printf("request.attr[%s]=%s%n", attrName, as(value)); + } - response.setContentType("text/html"); + HttpServletMapping fwdMapping = (HttpServletMapping)request.getAttribute(RequestDispatcher.FORWARD_MAPPING); + if (fwdMapping != null) + { + writer.printf("request.attr[%s].mappingMatch=%s%n", RequestDispatcher.FORWARD_MAPPING, as(fwdMapping.getMappingMatch())); + writer.printf("request.attr[%s].matchValue=%s%n", RequestDispatcher.FORWARD_MAPPING, as(fwdMapping.getMatchValue())); + writer.printf("request.attr[%s].pattern=%s%n", RequestDispatcher.FORWARD_MAPPING, as(fwdMapping.getPattern())); + writer.printf("request.attr[%s].servletName=%s%n", RequestDispatcher.FORWARD_MAPPING, as(fwdMapping.getServletName())); + } + + writer.printf("request.requestURI=%s%n", as(request.getRequestURI())); + writer.printf("request.contextPath=%s%n", as(request.getContextPath())); + writer.printf("request.pathTranslated=%s%n", as(request.getPathTranslated())); + writer.printf("request.servletPath=%s%n", as(request.getServletPath())); + writer.printf("request.pathInfo=%s%n", as(request.getPathInfo())); + writer.printf("request.queryString=%s%n", as(request.getQueryString())); + writer.printf("request.dispatcherType=%s%n", request.getDispatcherType().name()); + + writer.flush(); response.setStatus(HttpServletResponse.SC_OK); - response.getOutputStream().print(request.getDispatcherType().toString()); + } + + private String as(Object obj) + { + if (obj == null) + return ""; + if (obj instanceof String) + return String.format("'%s'", obj); + return Objects.toString(obj); } } diff --git a/jetty-servlet/src/test/resources/jetty-logging.properties b/jetty-servlet/src/test/resources/jetty-logging.properties index bd3b391a3dc3..4b390d5066a1 100644 --- a/jetty-servlet/src/test/resources/jetty-logging.properties +++ b/jetty-servlet/src/test/resources/jetty-logging.properties @@ -5,3 +5,4 @@ #org.eclipse.jetty.io.SocketChannelEndPoint.LEVEL=DEBUG #org.eclipse.jetty.server.DebugListener.LEVEL=DEBUG #org.eclipse.jetty.server.HttpChannelState.LEVEL=DEBUG +#org.eclipse.jetty.servlet.DispatcherTest.LEVEL=DEBUG From 039503c8e95e513adf194bd98df270664f704877 Mon Sep 17 00:00:00 2001 From: Olivier Lamy Date: Mon, 23 Jan 2023 11:44:31 +1000 Subject: [PATCH 2/3] no need to use an agent here before the real build Signed-off-by: Olivier Lamy --- Jenkinsfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Jenkinsfile b/Jenkinsfile index fe861da90667..b3e036bbc7e2 100644 --- a/Jenkinsfile +++ b/Jenkinsfile @@ -1,7 +1,7 @@ #!groovy pipeline { - agent any + agent none // save some io during the build options { skipDefaultCheckout() From d46f6f78346333d66f155a4eca85cb21cd5cb6bf Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 25 Jan 2023 17:03:04 +1100 Subject: [PATCH 3/3] Log as info exceptions from server after sending stop with StopMojo. (#9188) * Log as info exceptions from server after sending stop with StopMojo. --- .../jetty/maven/plugin/JettyStopMojo.java | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java index 4d329e0c8952..96536a06ba6d 100644 --- a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java +++ b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java @@ -220,9 +220,19 @@ private String send(String command, int wait) } else { - //Wait only a small amount of time to ensure TCP has sent the message - s.setSoTimeout(1000); - s.getInputStream().read(); + try + { + //Wait only a small amount of time to ensure TCP has sent the message + s.setSoTimeout(1000); + s.getInputStream().read(); + } + catch (Exception e) + { + if (getLog().isDebugEnabled()) + getLog().error("Error after sending command: " + command + ". Check the server state.", e); + else + getLog().info(e.getMessage() + " after sending command: " + command + ". Check the server state."); + } } return response;