Copy ServletHolder class/instance properly during startWebapp (#6214)

 ServletHolder.copyClassServlet() method added to correctly copy held class.


Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/Holder.java

@@ -186,7 +186,6 @@ public String toString()
 
     protected class HolderConfig
     {
-
         public ServletContext getServletContext()
         {
             return getServletHandler().getServletContext();

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<String, String> entry : jsp.getInitParameters().entrySet())
                         {
@@ -927,7 +935,6 @@ protected void appendPath(StringBuffer path, String element)
 
     protected class Config extends HolderConfig implements ServletConfig
     {
-
         @Override
         public String getServletName()
         {
@@ -1185,9 +1192,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

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<Path> 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.
+     * <p>
+     * Think of this as a precompiled jsp entry, but the class doesn't exist.
+     * </p>
+     */
+    @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<? extends HttpServlet> 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");
+        }
+    }
+}

jetty-webapp/src/test/webapp-alt-jsp/WEB-INF/web.xml

@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="utf-8"?>
+
+<web-app xmlns="http://xmlns.jcp.org/xml/ns/javaee"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee
+         http://xmlns.jcp.org/xml/ns/javaee/web-app_3_1.xsd"
+  version="3.1">
+
+  <servlet>
+    <servlet-name>zedName</servlet-name>
+    <jsp-file>/does/not/exist/index.jsp</jsp-file>
+  </servlet>
+  <servlet-mapping>
+    <servlet-name>zedName</servlet-name>
+    <url-pattern>/zed</url-pattern>
+  </servlet-mapping>
+
+  <servlet>
+    <servlet-name>precompiledName</servlet-name>
+    <servlet-class>org.eclipse.jetty.webapp.ForcedServletTest$FakePrecompiledJSP</servlet-class>
+  </servlet>
+  <servlet-mapping>
+    <servlet-name>precompiledName</servlet-name>
+    <url-pattern>/precompiled/world.jsp</url-pattern>
+  </servlet-mapping>
+
+</web-app>

jetty-webapp/src/test/webapp-alt-jsp/hello.jsp

@@ -0,0 +1 @@
+This shouldn't be returned