From edcaf70d9a11186771c9decf25fe3575fb717638 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Sat, 15 May 2021 20:28:22 -0500 Subject: [PATCH] Copy ServletHolder class/instance properly during startWebapp (#6214) ServletHolder.copyClassServlet() method added to correctly copy held class. Signed-off-by: Joakim Erdfelt Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/servlet/Holder.java | 1 - .../eclipse/jetty/servlet/ServletHolder.java | 19 +- .../jetty/webapp/ForcedServletTest.java | 259 ++++++++++++++++++ .../src/test/webapp-alt-jsp/WEB-INF/web.xml | 27 ++ .../src/test/webapp-alt-jsp/hello.jsp | 1 + 5 files changed, 300 insertions(+), 7 deletions(-) create mode 100644 jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ForcedServletTest.java create mode 100644 jetty-webapp/src/test/webapp-alt-jsp/WEB-INF/web.xml create mode 100644 jetty-webapp/src/test/webapp-alt-jsp/hello.jsp diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java index 2b410aa0f0c6..9656534a04cc 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java @@ -187,7 +187,6 @@ public String toString() protected class HolderConfig { - public ServletContext getServletContext() { return getServletHandler().getServletContext(); diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java index 7400aff9bbfa..928b937b5537 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java @@ -294,6 +294,14 @@ public void setForcedPath(String forcedPath) _forcedPath = forcedPath; } + private void setClassFrom(ServletHolder holder) throws ServletException + { + if (_servlet != null || getInstance() != null) + throw new IllegalStateException(); + this.setClassName(holder.getClassName()); + this.setHeldClass(holder.getHeldClass()); + } + public boolean isEnabled() { return _enabled; @@ -325,8 +333,8 @@ public void doStart() { if (LOG.isDebugEnabled()) LOG.debug("JSP file {} for {} mapped to Servlet {}", _forcedPath, getName(), jsp.getClassName()); - // set the className for this servlet to the precompiled one - setClassName(jsp.getClassName()); + // set the className/servlet/instance for this servlet to the precompiled one + setClassFrom(jsp); } else { @@ -336,7 +344,7 @@ public void doStart() { if (LOG.isDebugEnabled()) LOG.debug("JSP file {} for {} mapped to JspServlet class {}", _forcedPath, getName(), jsp.getClassName()); - setClassName(jsp.getClassName()); + setClassFrom(jsp); //copy jsp init params that don't exist for this servlet for (Map.Entry entry : jsp.getInitParameters().entrySet()) { @@ -964,7 +972,6 @@ protected void appendPath(StringBuffer path, String element) protected class Config extends HolderConfig implements ServletConfig { - @Override public String getServletName() { @@ -1225,9 +1232,9 @@ public void dump(Appendable out, String indent) throws IOException @Override public String toString() { - return String.format("%s==%s@%x{jsp=%s,order=%d,inst=%b,async=%b,src=%s}", + return String.format("%s==%s@%x{jsp=%s,order=%d,inst=%b,async=%b,src=%s,%s}", getName(), getClassName(), hashCode(), - _forcedPath, _initOrder, _servlet != null, isAsyncSupported(), getSource()); + _forcedPath, _initOrder, _servlet != null, isAsyncSupported(), getSource(), getState()); } private class UnavailableServlet extends Wrapper diff --git a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ForcedServletTest.java b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ForcedServletTest.java new file mode 100644 index 000000000000..c17d695a7b16 --- /dev/null +++ b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ForcedServletTest.java @@ -0,0 +1,259 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.webapp; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.Iterator; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.server.LocalConnector; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.servlet.ServletHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.eclipse.jetty.servlet.ServletMapping; +import org.eclipse.jetty.toolchain.test.FS; +import org.eclipse.jetty.toolchain.test.MavenTestingUtils; +import org.eclipse.jetty.util.component.LifeCycle; +import org.eclipse.jetty.util.resource.PathResource; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class ForcedServletTest +{ + private Server server; + private LocalConnector connector; + + @BeforeEach + public void setup() throws Exception + { + server = new Server(); + connector = new LocalConnector(server); + server.addConnector(connector); + + WebAppContext context = new ForcedWebAppContext(); + + // Lets setup the Webapp base resource properly + Path basePath = MavenTestingUtils.getTargetTestingPath(ForcedServletTest.class.getName()).resolve("webapp"); + FS.ensureEmpty(basePath); + Path srcWebApp = MavenTestingUtils.getProjectDirPath("src/test/webapp-alt-jsp"); + copyDir(srcWebApp, basePath); + copyClass(FakePrecompiledJSP.class, basePath.resolve("WEB-INF/classes")); + + // Use the new base + context.setWarResource(new PathResource(basePath)); + + server.setHandler(context); + // server.setDumpAfterStart(true); + server.start(); + } + + private void copyClass(Class clazz, Path destClasses) throws IOException + { + String classRelativeFilename = clazz.getName().replace('.', '/') + ".class"; + Path destFile = destClasses.resolve(classRelativeFilename); + FS.ensureDirExists(destFile.getParent()); + + Path srcFile = MavenTestingUtils.getTargetPath("test-classes/" + classRelativeFilename); + Files.copy(srcFile, destFile); + } + + private void copyDir(Path src, Path dest) throws IOException + { + FS.ensureDirExists(dest); + for (Iterator it = Files.list(src).iterator(); it.hasNext(); ) + { + Path path = it.next(); + if (Files.isRegularFile(path)) + Files.copy(path, dest.resolve(path.getFileName())); + else if (Files.isDirectory(path)) + copyDir(path, dest.resolve(path.getFileName())); + } + } + + @AfterEach + public void teardown() + { + LifeCycle.stop(server); + } + + /** + * Test access to a jsp resource defined in the web.xml, but doesn't actually exist. + *

+ * Think of this as a precompiled jsp entry, but the class doesn't exist. + *

+ */ + @Test + public void testAccessBadDescriptorEntry() throws Exception + { + StringBuilder rawRequest = new StringBuilder(); + rawRequest.append("GET /does/not/exist/index.jsp HTTP/1.1\r\n"); + rawRequest.append("Host: local\r\n"); + rawRequest.append("Connection: close\r\n"); + rawRequest.append("\r\n"); + + String rawResponse = connector.getResponse(rawRequest.toString()); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + // Since this was a request to a resource ending in `*.jsp`, the RejectUncompiledJspServlet responded + assertEquals(555, response.getStatus()); + } + + /** + * Test access of a jsp resource that doesn't exist in the base resource or the descriptor. + */ + @Test + public void testAccessNonExistentEntry() throws Exception + { + StringBuilder rawRequest = new StringBuilder(); + rawRequest.append("GET /bogus.jsp HTTP/1.1\r\n"); + rawRequest.append("Host: local\r\n"); + rawRequest.append("Connection: close\r\n"); + rawRequest.append("\r\n"); + + String rawResponse = connector.getResponse(rawRequest.toString()); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + // Since this was a request to a resource ending in `*.jsp`, the RejectUncompiledJspServlet responded + assertEquals(555, response.getStatus()); + } + + /** + * Test access of a jsp resource that exist in the base resource, but not in the descriptor. + */ + @Test + public void testAccessUncompiledEntry() throws Exception + { + StringBuilder rawRequest = new StringBuilder(); + rawRequest.append("GET /hello.jsp HTTP/1.1\r\n"); + rawRequest.append("Host: local\r\n"); + rawRequest.append("Connection: close\r\n"); + rawRequest.append("\r\n"); + + String rawResponse = connector.getResponse(rawRequest.toString()); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + // status code 555 is from RejectUncompiledJspServlet + assertEquals(555, response.getStatus()); + } + + /** + * Test access of a jsp resource that does not exist in the base resource, but in the descriptor, as a precompiled jsp entry + */ + @Test + public void testPrecompiledEntry() throws Exception + { + StringBuilder rawRequest = new StringBuilder(); + rawRequest.append("GET /precompiled/world.jsp HTTP/1.1\r\n"); + rawRequest.append("Host: local\r\n"); + rawRequest.append("Connection: close\r\n"); + rawRequest.append("\r\n"); + + String rawResponse = connector.getResponse(rawRequest.toString()); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + assertEquals(200, response.getStatus()); + + String responseBody = response.getContent(); + assertThat(responseBody, containsString("This is the FakePrecompiledJSP")); + } + + public static class ForcedWebAppContext extends WebAppContext + { + @Override + protected void startWebapp() throws Exception + { + // This will result in a 404 for all requests that don't belong to a more precise servlet + forceServlet("default", ServletHandler.Default404Servlet.class); + addServletMapping("default", "/"); + + // This will result in any attempt to use an JSP that isn't precompiled and in the descriptor with status code 555 + forceServlet("jsp", RejectUncompiledJspServlet.class); + addServletMapping("jsp", "*.jsp"); + + super.startWebapp(); + } + + private void addServletMapping(String name, String pathSpec) + { + ServletMapping mapping = new ServletMapping(); + mapping.setServletName(name); + mapping.setPathSpec(pathSpec); + getServletHandler().addServletMapping(mapping); + } + + private void forceServlet(String name, Class servlet) throws Exception + { + ServletHandler handler = getServletHandler(); + + // Remove existing holder + handler.setServlets(Arrays.stream(handler.getServlets()) + .filter(h -> !h.getName().equals(name)) + .toArray(ServletHolder[]::new)); + + // add the forced servlet + ServletHolder holder = new ServletHolder(servlet.getConstructor().newInstance()); + holder.setInitOrder(1); + holder.setName(name); + holder.setAsyncSupported(true); + handler.addServlet(holder); + } + } + + public static class RejectUncompiledJspServlet extends HttpServlet + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + log(String.format("Uncompiled JSPs not supported by %s", request.getRequestURI())); + response.sendError(555); + } + } + + public static class FakeJspServlet extends HttpServlet + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + resp.setCharacterEncoding("utf-8"); + resp.setContentType("text/plain"); + resp.setStatus(HttpServletResponse.SC_OK); + resp.getWriter().println("This is the FakeJspServlet"); + } + } + + public static class FakePrecompiledJSP extends HttpServlet + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + resp.setCharacterEncoding("utf-8"); + resp.setContentType("text/plain"); + resp.setStatus(HttpServletResponse.SC_OK); + resp.getWriter().println("This is the FakePrecompiledJSP"); + } + } +} diff --git a/jetty-webapp/src/test/webapp-alt-jsp/WEB-INF/web.xml b/jetty-webapp/src/test/webapp-alt-jsp/WEB-INF/web.xml new file mode 100644 index 000000000000..2b260c9e62a8 --- /dev/null +++ b/jetty-webapp/src/test/webapp-alt-jsp/WEB-INF/web.xml @@ -0,0 +1,27 @@ + + + + + + zedName + /does/not/exist/index.jsp + + + zedName + /zed + + + + precompiledName + org.eclipse.jetty.webapp.ForcedServletTest$FakePrecompiledJSP + + + precompiledName + /precompiled/world.jsp + + + diff --git a/jetty-webapp/src/test/webapp-alt-jsp/hello.jsp b/jetty-webapp/src/test/webapp-alt-jsp/hello.jsp new file mode 100644 index 000000000000..b7f12aaa847d --- /dev/null +++ b/jetty-webapp/src/test/webapp-alt-jsp/hello.jsp @@ -0,0 +1 @@ +This shouldn't be returned \ No newline at end of file