From 365bab153c14c5895e7fab284a6e3182c432e816 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 2 Nov 2020 10:19:08 +0100 Subject: [PATCH 1/2] Fixes #5555 NPE if Filter of named servlet 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() + { + } + } + } From 4c49ca5184dfc45a4e670c9d740c07e1b08792d9 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 2 Nov 2020 14:08:46 +0100 Subject: [PATCH 2/2] More cleanup for #5555 Some more cleanup for #5555 --- .../eclipse/jetty/servlet/ServletHandler.java | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 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 bd06cb7bd1e..4f29e4e76d8 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 @@ -521,21 +521,8 @@ public class ServletHandler extends ScopedHandler FilterChain chain = null; // find the servlet - if (target.startsWith("/")) - { - if (servletHolder != null && _filterMappings != null && _filterMappings.length > 0) - chain = getFilterChain(baseRequest, target, servletHolder); - } - else - { - if (servletHolder != null) - { - if (_filterMappings != null && _filterMappings.length > 0) - { - chain = getFilterChain(baseRequest, null, servletHolder); - } - } - } + if (servletHolder != null && _filterMappings != null && _filterMappings.length > 0) + chain = getFilterChain(baseRequest, target.startsWith("/") ? target : null, servletHolder); if (LOG.isDebugEnabled()) LOG.debug("chain={}", chain); @@ -593,6 +580,7 @@ public class ServletHandler extends ScopedHandler protected FilterChain getFilterChain(Request baseRequest, String pathInContext, ServletHolder servletHolder) { + Objects.requireNonNull(servletHolder); String key = pathInContext == null ? servletHolder.getName() : pathInContext; int dispatch = FilterMapping.dispatch(baseRequest.getDispatcherType()); @@ -608,7 +596,7 @@ public class ServletHandler extends ScopedHandler // The mappings lists have been reversed to make this simple and fast. FilterChain chain = null; - if (servletHolder != null && _filterNameMappings != null && !_filterNameMappings.isEmpty()) + if (_filterNameMappings != null && !_filterNameMappings.isEmpty()) { if (_wildFilterNameMappings != null) for (FilterMapping mapping : _wildFilterNameMappings) @@ -1626,6 +1614,7 @@ public class ServletHandler extends ScopedHandler ChainEnd(ServletHolder holder) { + Objects.requireNonNull(holder); _servletHolder = holder; }