From b77ee515a5b14435260ad914b29d2bf309694685 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 2 Nov 2020 13:30:52 +0100 Subject: [PATCH] Fixes #5555 NPE if Filter of named servlet (#5556) Fixed #5555 NPE if there is a filter with a servlet name mapping, but a request is received for a servlet without a name match. Added more simple tests for servlet and filter mappings --- .../eclipse/jetty/servlet/ServletHandler.java | 10 +- .../jetty/servlet/ServletHandlerTest.java | 118 ++++++++++++++++++ 2 files changed, 125 insertions(+), 3 deletions(-) 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 ba41caf3285..bd06cb7bd1e 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 @@ -614,10 +614,14 @@ public class ServletHandler extends ScopedHandler for (FilterMapping mapping : _wildFilterNameMappings) chain = newFilterChain(mapping.getFilterHolder(), chain == null ? new ChainEnd(servletHolder) : chain); - for (FilterMapping mapping : _filterNameMappings.get(servletHolder.getName())) + List nameMappings = _filterNameMappings.get(servletHolder.getName()); + if (nameMappings != null) { - if (mapping.appliesTo(dispatch)) - chain = newFilterChain(mapping.getFilterHolder(), chain == null ? new ChainEnd(servletHolder) : chain); + for (FilterMapping mapping : nameMappings) + { + if (mapping.appliesTo(dispatch)) + chain = newFilterChain(mapping.getFilterHolder(), chain == null ? new ChainEnd(servletHolder) : chain); + } } } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHandlerTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHandlerTest.java index 29dc90ad1c1..5bddd101f30 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHandlerTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletHandlerTest.java @@ -18,18 +18,29 @@ package org.eclipse.jetty.servlet; +import java.io.IOException; import java.util.ArrayList; import java.util.EnumSet; import java.util.List; import javax.servlet.DispatcherType; +import javax.servlet.Filter; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSessionEvent; import javax.servlet.http.HttpSessionListener; import org.eclipse.jetty.http.pathmap.MappedResource; +import org.eclipse.jetty.server.LocalConnector; +import org.eclipse.jetty.server.Server; import org.eclipse.jetty.util.component.Container; 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; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -730,4 +741,111 @@ public class ServletHandlerTest assertTrue(removeResults.contains(sh1)); assertTrue(removeResults.contains(lh1)); } + + @Test + public void testServletMappings() throws Exception + { + Server server = new Server(); + ServletHandler handler = new ServletHandler(); + server.setHandler(handler); + for (final String mapping : new String[] {"/", "/foo", "/bar/*", "*.bob"}) + { + handler.addServletWithMapping(new ServletHolder(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + resp.getOutputStream().println("mapping='" + mapping + "'"); + } + }), mapping); + } + // add servlet with no mapping + handler.addServlet(new ServletHolder(new HttpServlet() {})); + + LocalConnector connector = new LocalConnector(server); + server.addConnector(connector); + + server.start(); + + assertThat(connector.getResponse("GET /default HTTP/1.0\r\n\r\n"), containsString("mapping='/'")); + assertThat(connector.getResponse("GET /foo HTTP/1.0\r\n\r\n"), containsString("mapping='/foo'")); + assertThat(connector.getResponse("GET /bar HTTP/1.0\r\n\r\n"), containsString("mapping='/bar/*'")); + assertThat(connector.getResponse("GET /bar/bob HTTP/1.0\r\n\r\n"), containsString("mapping='/bar/*'")); + assertThat(connector.getResponse("GET /bar/foo.bob HTTP/1.0\r\n\r\n"), containsString("mapping='/bar/*'")); + assertThat(connector.getResponse("GET /other/foo.bob HTTP/1.0\r\n\r\n"), containsString("mapping='*.bob'")); + } + + @Test + public void testFilterMappings() throws Exception + { + Server server = new Server(); + ServletHandler handler = new ServletHandler(); + server.setHandler(handler); + + ServletHolder foo = new ServletHolder(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + resp.getOutputStream().println("FOO"); + } + }); + foo.setName("foo"); + handler.addServletWithMapping(foo, "/foo/*"); + + ServletHolder def = new ServletHolder(new HttpServlet() + { + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + resp.getOutputStream().println("default"); + } + }); + def.setName("default"); + handler.addServletWithMapping(def, "/"); + + for (final String mapping : new String[]{"/*", "/foo", "/bar/*", "*.bob"}) + { + handler.addFilterWithMapping(new FilterHolder((TestFilter)(request, response, chain) -> + { + response.getOutputStream().print("path-" + mapping + "-"); + chain.doFilter(request, response); + }), mapping, EnumSet.of(DispatcherType.REQUEST)); + } + + FilterHolder fooFilter = new FilterHolder((TestFilter)(request, response, chain) -> + { + response.getOutputStream().print("name-foo-"); + chain.doFilter(request, response); + }); + fooFilter.setName("fooFilter"); + FilterMapping named = new FilterMapping(); + named.setFilterHolder(fooFilter); + named.setServletName("foo"); + handler.addFilter(fooFilter, named); + + LocalConnector connector = new LocalConnector(server); + server.addConnector(connector); + + server.start(); + + assertThat(connector.getResponse("GET /default HTTP/1.0\r\n\r\n"), containsString("path-/*-default")); + assertThat(connector.getResponse("GET /foo HTTP/1.0\r\n\r\n"), containsString("path-/*-path-/foo-name-foo-FOO")); + assertThat(connector.getResponse("GET /foo/bar HTTP/1.0\r\n\r\n"), containsString("path-/*-name-foo-FOO")); + assertThat(connector.getResponse("GET /foo/bar.bob HTTP/1.0\r\n\r\n"), containsString("path-/*-path-*.bob-name-foo-FOO")); + assertThat(connector.getResponse("GET /other.bob HTTP/1.0\r\n\r\n"), containsString("path-/*-path-*.bob-default")); + } + + private interface TestFilter extends Filter + { + default void init(FilterConfig filterConfig) throws ServletException + { + } + + @Override + default void destroy() + { + } + } + }