This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 16d9dbd3a9ecc27b665bc17aef836df770c24c93 Author: opencode <[email protected]> AuthorDate: Mon Oct 12 11:11:29 2026 +0200 Fix NPE in LoadBalancerDrainingValve with no requested session id The cookie comparison in the draining branch assumed a non-null requested session id, but the id is not parsed at all when the effective session tracking modes of the context do not include cookies, and nothing is parsed before the request maps to a context when a valve attached to the Engine or a Host sees a request that matched no context. In both cases a client that still sent a session cookie triggered a NullPointerException, turning the draining redirect into a 500 error. The valve now only compares cookie values against the requested session id when one was parsed, and the cookie purge block requires a context for its path and secure configuration, which it always had when an id was parsed. A stale cookie that belongs to another context is not purged now, instead of the request failing. The test gained a session state dimension covering an absent requested session id and an absent context. --- .../catalina/valves/LoadBalancerDrainingValve.java | 27 ++++++++--- .../valves/TestLoadBalancerDrainingValve.java | 55 ++++++++++++++-------- 2 files changed, 56 insertions(+), 26 deletions(-) diff --git a/java/org/apache/catalina/valves/LoadBalancerDrainingValve.java b/java/org/apache/catalina/valves/LoadBalancerDrainingValve.java index 49c1463157..568ab6314e 100644 --- a/java/org/apache/catalina/valves/LoadBalancerDrainingValve.java +++ b/java/org/apache/catalina/valves/LoadBalancerDrainingValve.java @@ -23,6 +23,7 @@ import jakarta.servlet.SessionCookieConfig; import jakarta.servlet.http.Cookie; import jakarta.servlet.http.HttpServletResponse; +import org.apache.catalina.Context; import org.apache.catalina.connector.Request; import org.apache.catalina.connector.Response; import org.apache.catalina.util.SessionConfig; @@ -168,7 +169,16 @@ public class LoadBalancerDrainingValve extends ValveBase { final Cookie[] cookies = request.getCookies(); - final String sessionCookieName = SessionConfig.getSessionCookieName(request.getContext()); + // The context may be null, for example for a valve attached to the + // Engine receiving a request that matched no context + final Context context = request.getContext(); + + // The requested session id may be null, for example when session + // tracking via cookies is disabled for the context while the + // client still sends a session cookie + final String requestedSessionId = request.getRequestedSessionId(); + + final String sessionCookieName = SessionConfig.getSessionCookieName(context); if (null != cookies) { for (Cookie cookie : cookies) { @@ -177,8 +187,8 @@ public class LoadBalancerDrainingValve extends ValveBase { containerLog.trace("Checking cookie " + cookieName + "=" + cookie.getValue()); } - if (sessionCookieName.equals(cookieName) && - request.getRequestedSessionId().equals(cookie.getValue())) { + if (sessionCookieName.equals(cookieName) && requestedSessionId != null && + requestedSessionId.equals(cookie.getValue())) { sessionCookie = cookie; } else if (null != _ignoreCookieName && _ignoreCookieName.equals(cookieName) && null != _ignoreCookieValue && _ignoreCookieValue.equals(cookie.getValue())) { @@ -200,20 +210,23 @@ public class LoadBalancerDrainingValve extends ValveBase { // Kill any session cookie that was found // TODO: Consider implications of SSO cookies - if (null != sessionCookie) { - sessionCookie.setPath(SessionConfig.getSessionCookiePath(request.getContext())); + // A context is needed for the cookie path and secure configuration + // and, when the context is null, no requested session id can have + // been parsed so no session cookie would have been found anyway + if (null != sessionCookie && context != null) { + sessionCookie.setPath(SessionConfig.getSessionCookiePath(context)); sessionCookie.setMaxAge(0); // Delete sessionCookie.setValue(""); // Purge the cookie's value // Replicate logic used to set secure attribute for session cookies SessionCookieConfig sessionCookieConfig = - request.getContext().getServletContext().getSessionCookieConfig(); + context.getServletContext().getSessionCookieConfig(); sessionCookie.setSecure(request.isSecure() || sessionCookieConfig.isSecure()); response.addCookie(sessionCookie); } String uri = collapseLeadingSlashes(request.getRequestURI()); // Re-write the URI if it contains a ;jsessionid parameter - String sessionURIParamName = SessionConfig.getSessionUriParamName(request.getContext()); + String sessionURIParamName = SessionConfig.getSessionUriParamName(context); if (uri.contains(";" + sessionURIParamName + "=")) { uri = uri.replaceFirst(";" + sessionURIParamName + "=[^;/]*", ""); } diff --git a/test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java b/test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java index 08588199f5..fe6002e180 100644 --- a/test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java +++ b/test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java @@ -45,13 +45,17 @@ import org.easymock.IMocksControl; public class TestLoadBalancerDrainingValve { @Parameters(name = "{index}: activation[{0}], validSessionID[{1}], expectInvokeNext[{2}], enableIgnore[{3}], " + - "leadingSlashes[{4}], queryString[{5}]]") + "leadingSlashes[{4}], queryString[{5}], sessionState[{8}]]") public static Collection<Object[]> parameters() { String[] jkActivations = new String[] { "ACT", "DIS" }; Boolean[] booleans = new Boolean[] { Boolean.TRUE, Boolean.FALSE }; Integer[] integers = new Integer[] { Integer.valueOf(0), Integer.valueOf(1), Integer.valueOf(2) }; String[] queryStrings = new String[] { null, "foo=bar" }; + // The states with a missing requested session id or a missing context + // are only meaningful for the draining of a request with an invalid + // session + String[] sessionStates = new String[] { "idPresent", "idAbsent", "contextAbsent" }; List<Object[]> parameterSets = new ArrayList<>(); for (String jkActivation : jkActivations) { @@ -63,9 +67,13 @@ public class TestLoadBalancerDrainingValve { for (String queryString : queryStrings) { for (Boolean secureRequest : booleans) { for (Boolean secureSessionConfig : booleans) { - parameterSets.add(new Object[] { jkActivation, validSessionId, expectInvokeNext, - enableIgnore, leadingSlashes, queryString, secureRequest, - secureSessionConfig }); + for (String sessionState : "DIS".equals(jkActivation) && + !validSessionId.booleanValue() ? sessionStates : new String[] { + "idPresent" }) { + parameterSets.add(new Object[] { jkActivation, validSessionId, + expectInvokeNext, enableIgnore, leadingSlashes, queryString, + secureRequest, secureSessionConfig, sessionState }); + } } } } @@ -101,6 +109,9 @@ public class TestLoadBalancerDrainingValve { @Parameter(7) public boolean secureSessionConfig; + @Parameter(8) + public String sessionState; + @Test public void runValve() throws Exception { @@ -146,33 +157,39 @@ public class TestLoadBalancerDrainingValve { cookies.add(cookie); - EasyMock.expect(request.getRequestedSessionId()).andStubReturn(sessionId); + EasyMock.expect(request.getRequestedSessionId()) + .andStubReturn("idPresent".equals(sessionState) ? sessionId : null); EasyMock.expect(request.getRequestURI()).andStubReturn("/".repeat(leadingSlashes.intValue()) + requestURI); EasyMock.expect(request.getCookies()).andStubReturn(cookies.toArray(new Cookie[0])); - EasyMock.expect(request.getContext()).andStubReturn(ctx); + EasyMock.expect(request.getContext()) + .andStubReturn("contextAbsent".equals(sessionState) ? null : ctx); EasyMock.expect(ctx.getSessionCookieName()).andStubReturn(sessionCookieName); EasyMock.expect(servletContext.getSessionCookieConfig()).andStubReturn(cookieConfig); EasyMock.expect(request.getQueryString()).andStubReturn(queryString); EasyMock.expect(ctx.getSessionCookiePath()).andStubReturn("/"); if (!enableIgnore) { - EasyMock.expect(Boolean.valueOf(ctx.getSessionCookiePathUsesTrailingSlash())) - .andStubReturn(Boolean.TRUE); - EasyMock.expect(request.getQueryString()).andStubReturn(queryString); - // Response will have cookie deleted - MyCookie expectedCookie = new MyCookie(cookieConfig.getName(), ""); - expectedCookie.setPath(cookieConfig.getPath()); - expectedCookie.setMaxAge(0); - - EasyMock.expect(Boolean.valueOf(request.isSecure())).andReturn(secureRequest); - - // These two lines just mean EasyMock.expect(response.addCookie) but for a void method - response.addCookie(expectedCookie); - EasyMock.expect(ctx.getSessionCookieName()).andReturn(sessionCookieName); // Indirect call String expectedRequestURI = requestURI; if (null != queryString) { expectedRequestURI = expectedRequestURI + '?' + queryString; } + if ("idPresent".equals(sessionState)) { + EasyMock.expect(Boolean.valueOf(ctx.getSessionCookiePathUsesTrailingSlash())) + .andStubReturn(Boolean.TRUE); + EasyMock.expect(request.getQueryString()).andStubReturn(queryString); + // Response will have cookie deleted + MyCookie expectedCookie = new MyCookie(cookieConfig.getName(), ""); + expectedCookie.setPath(cookieConfig.getPath()); + expectedCookie.setMaxAge(0); + + EasyMock.expect(Boolean.valueOf(request.isSecure())).andReturn(secureRequest); + + // These two lines just mean EasyMock.expect(response.addCookie) but for a void method + response.addCookie(expectedCookie); + EasyMock.expect(ctx.getSessionCookieName()).andReturn(sessionCookieName); // Indirect call + } + // Without a requested session id the cookie cannot match, so it + // is not deleted, but the redirect is still expected response.sendRedirect(expectedRequestURI, 307); } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
