diff --git a/openam-core/src/main/java/com/sun/identity/jaxrpc/JAXRPCRequestFilter.java b/openam-core/src/main/java/com/sun/identity/jaxrpc/JAXRPCRequestFilter.java index 9004639495..1926343e33 100644 --- a/openam-core/src/main/java/com/sun/identity/jaxrpc/JAXRPCRequestFilter.java +++ b/openam-core/src/main/java/com/sun/identity/jaxrpc/JAXRPCRequestFilter.java @@ -126,16 +126,22 @@ public void init(FilterConfig filterConfig) throws ServletException { @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - boolean bound = false; - if (request instanceof HttpServletRequest) { - CURRENT_REQUEST.set((HttpServletRequest) request); - bound = true; + if (!(request instanceof HttpServletRequest)) { + chain.doFilter(request, response); + return; } + // The filter is mapped for FORWARD and INCLUDE dispatches as well, so a nested + // dispatch into /jaxrpc/* re-enters it on the same thread: restore the outer binding + // on exit rather than clearing it. + HttpServletRequest previous = CURRENT_REQUEST.get(); + CURRENT_REQUEST.set((HttpServletRequest) request); try { chain.doFilter(request, response); } finally { - if (bound) { + if (previous == null) { CURRENT_REQUEST.remove(); + } else { + CURRENT_REQUEST.set(previous); } } } diff --git a/openam-core/src/test/java/com/sun/identity/jaxrpc/JAXRPCRequestFilterTest.java b/openam-core/src/test/java/com/sun/identity/jaxrpc/JAXRPCRequestFilterTest.java index a75c9ffce9..1e18b171ff 100644 --- a/openam-core/src/test/java/com/sun/identity/jaxrpc/JAXRPCRequestFilterTest.java +++ b/openam-core/src/test/java/com/sun/identity/jaxrpc/JAXRPCRequestFilterTest.java @@ -72,4 +72,27 @@ public void filterBindsRequestForTheChainAndClearsItAfterwards() throws Exceptio assertNull(JAXRPCRequestFilter.getCurrentRequest(), "request must be cleared once the filter chain completes"); } + + @Test + public void nestedDispatchRestoresTheOuterRequestOnExit() throws Exception { + // The filter is mapped for FORWARD and INCLUDE dispatches as well, so a nested + // dispatch into /jaxrpc/* re-enters it on the same thread; the outer request + // must be bound again once the nested chain returns. + HttpServletRequest outer = mock(HttpServletRequest.class); + HttpServletRequest inner = mock(HttpServletRequest.class); + HttpServletResponse response = mock(HttpServletResponse.class); + JAXRPCRequestFilter filter = new JAXRPCRequestFilter(); + final HttpServletRequest[] seen = new HttpServletRequest[2]; + FilterChain outerChain = (req, res) -> { + filter.doFilter(inner, res, (r, s) -> seen[0] = JAXRPCRequestFilter.getCurrentRequest()); + seen[1] = JAXRPCRequestFilter.getCurrentRequest(); + }; + + filter.doFilter(outer, response, outerChain); + + assertSame(seen[0], inner, "the nested request must be bound during the nested chain"); + assertSame(seen[1], outer, "the outer request must be bound again after the nested chain"); + assertNull(JAXRPCRequestFilter.getCurrentRequest(), + "nothing must stay bound once the outer chain completes"); + } } diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java index 1cf064a2f2..e5e701920b 100755 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/FSUtils.java @@ -361,8 +361,10 @@ public static void forwardRequest( index + deploymentURI.length()); // The target comes from the request (goto/RelayState). A forward // bypasses the web.xml filters and can reach /WEB-INF, so refuse - // anything that is not a plain in-app path. - if (!ForwardPathValidator.isSafeForwardPath(resource)) { + // anything that is not a plain in-app path and dispatch the form + // the container maps. + String dispatchPath = ForwardPathValidator.forwardTarget(resource); + if (dispatchPath == null) { FSUtils.debug.warning("FSUtils.forwardRequest: refusing to " + "forward to a path with traversal or a reserved " + "directory"); @@ -371,16 +373,16 @@ public static void forwardRequest( } if (FSUtils.debug.messageEnabled()) { FSUtils.debug.message( - "FSUtils.forwardRequest: Forwarding to :" + resource); - } - RequestDispatcher dispatcher = - request.getRequestDispatcher(resource); + "FSUtils.forwardRequest: Forwarding to :" + dispatchPath); + } + RequestDispatcher dispatcher = + request.getRequestDispatcher(dispatchPath); try { dispatcher.forward(request, response); } catch (Exception e) { FSUtils.debug.error("FSUtils.forwardRequest: Exception " + "occured while trying to forward to resource:" + - resource , e); + dispatchPath, e); } } } catch (Exception ex) { diff --git a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java index 5f23cd896a..a35a1ff741 100644 --- a/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java +++ b/openam-federation/openam-federation-library/src/main/java/com/sun/identity/federation/common/ForwardPathValidator.java @@ -16,6 +16,7 @@ package com.sun.identity.federation.common; import java.io.ByteArrayOutputStream; +import java.net.URLDecoder; import java.nio.ByteBuffer; import java.nio.charset.CharacterCodingException; import java.nio.charset.CodingErrorAction; @@ -78,6 +79,46 @@ public static boolean isSafeForwardPath(String path) { return true; } + /** + * The path to hand to a {@code RequestDispatcher} for {@code path}, or {@code null} when + * {@code path} must not be dispatched to. The result is the form the container maps - + * path parameters stripped and percent-escapes decoded once, {@code +} kept literal - with + * the query string as it was given. + *

+ * Beyond {@link #isSafeForwardPath} the dispatcher form is refused when it carries a + * {@code WEB-INF} segment anywhere, not only at the root: no in-app path of the product + * does. The decoded form is derived and re-checked here in the shape static analysis + * recognises as a sanitised forward target, so the checks read as a repetition of what + * {@code isSafeForwardPath} established. + * + * @param path a context-relative path, optionally with a query string + * @return the path to dispatch to, or {@code null} if it is not a plain in-app path + */ + public static String forwardTarget(String path) { + if (!isSafeForwardPath(path)) { + return null; + } + int query = path.indexOf('?'); + String uri = query == -1 ? path : path.substring(0, query); + // isSafeForwardPath refused every escape that decodes to a delimiter, %25 included, + // so one round decodes everything; '+' is protected from URLDecoder's form decoding. + String decoded = stripPathParams(uri).replace("+", "%2B"); + try { + while (decoded.contains("%")) { + decoded = URLDecoder.decode(decoded, StandardCharsets.UTF_8); + } + } catch (IllegalArgumentException e) { + return null; + } + // Only a segment that is exactly ".." is traversal; every other segment becomes "x". + String dotSegments = decoded.replaceAll("(?<=^|/)(?!\\.\\.(?=/|$))[^/]+", "x"); + if ((decoded + "/").toUpperCase(Locale.ROOT).contains("/WEB-INF/") + || dotSegments.contains("..")) { + return null; + } + return query == -1 ? decoded : decoded + "?" + path.substring(query + 1); + } + private static boolean isReserved(String collapsedPath) { String lower = collapsedPath.toLowerCase(Locale.ROOT); return lower.startsWith("/web-inf/") || lower.equals("/web-inf") diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java index 3b670c9112..ffe55b24fd 100644 --- a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/FSUtilsForwardPathTest.java @@ -70,6 +70,20 @@ public void forwardsAnInAppTargetOnTheSameHost() throws Exception { verify(response, never()).sendError(anyInt()); } + @Test + public void dispatchesTheFormTheContainerMaps() throws Exception { + // Path parameters stripped and escapes decoded once, the query string as given. + RequestDispatcher dispatcher = mock(RequestDispatcher.class); + when(request.getRequestDispatcher("/console/my page.jsp?goto=%2Fopenam%2Fconsole")) + .thenReturn(dispatcher); + + FSUtils.forwardRequest(request, response, + BASE + "/console;x%2Fy/my%20page.jsp?goto=%2Fopenam%2Fconsole"); + + verify(dispatcher).forward(request, response); + verify(response, never()).sendError(anyInt()); + } + @DataProvider public Object[][] traversingTargets() { return new Object[][] { diff --git a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java index b6dfb2bbd5..0922ff8aa6 100644 --- a/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java +++ b/openam-federation/openam-federation-library/src/test/java/com/sun/identity/federation/common/ForwardPathValidatorTest.java @@ -121,6 +121,56 @@ public void rejectsTraversalAndReservedDirectories(String path) { assertThat(ForwardPathValidator.isSafeForwardPath(path)).as(path).isFalse(); } + @DataProvider + public Object[][] forwardTargets() { + return new Object[][] { + // {path as received, the form handed to the RequestDispatcher} + {"/idpSSOInit.jsp", "/idpSSOInit.jsp"}, + {"/console/my%20page.jsp", "/console/my page.jsp"}, // decoded once, as the container maps it + {"/console/a+b.jsp", "/console/a+b.jsp"}, // '+' stays literal + {"/console/a+b%20c.jsp", "/console/a+b c.jsp"}, + {"/console/base;jsessionid=1/AMAdminFrame", "/console/base/AMAdminFrame"}, // parameter stripped + {"/console;x%2Fy/base/AMAdminFrame", "/console/base/AMAdminFrame"}, // encoded slash goes with it + {"/console//base/./AMAdminFrame", "/console//base/./AMAdminFrame"}, // the container collapses + {"/console/base/", "/console/base/"}, // a trailing slash is a different mapping + {"/a/b..c/d", "/a/b..c/d"}, // ".." inside a segment is an ordinary name + {"/x/.../y", "/x/.../y"}, + {"/web-info/page.jsp", "/web-info/page.jsp"}, + {"/UI/Login?goto=%2Fopenam%2Fconsole", "/UI/Login?goto=%2Fopenam%2Fconsole"}, // query kept as given + {"/saml2/jsp/idpSSOInit.jsp?metaAlias=%2Fidp&goto=%2F..%2FWEB-INF", + "/saml2/jsp/idpSSOInit.jsp?metaAlias=%2Fidp&goto=%2F..%2FWEB-INF"}, + {"/console/my%20page.jsp?", "/console/my page.jsp?"}, + {"/XUI/#login/", "/XUI/#login/"}, + }; + } + + @Test(dataProvider = "forwardTargets") + public void forwardTargetIsTheFormTheContainerMaps(String path, String expected) { + assertThat(ForwardPathValidator.forwardTarget(path)).as(path).isEqualTo(expected); + } + + @Test(dataProvider = "unsafeForwardPaths") + public void forwardTargetIsNullForAnUnsafePath(String path) { + assertThat(ForwardPathValidator.forwardTarget(path)).as(path).isNull(); + } + + @DataProvider + public Object[][] reservedSegmentsBelowTheRoot() { + return new Object[][] { + {"/x/WEB-INF/y.jsp"}, + {"/x/web-inf/"}, + {"/x/WEB-INF"}, + {"/x;a/%57EB-INF/y.jsp"}, + }; + } + + @Test(dataProvider = "reservedSegmentsBelowTheRoot") + public void forwardTargetRefusesAReservedSegmentAnywhere(String path) { + // isSafeForwardPath reserves the directory at the root only; no in-app path carries + // the name below it either, and the dispatcher form is refused when one does. + assertThat(ForwardPathValidator.forwardTarget(path)).as(path).isNull(); + } + @DataProvider public Object[][] safeMetaAliases() { return new Object[][] { diff --git a/openam-server-only/pom.xml b/openam-server-only/pom.xml index b33dc1f9d1..855fe650b9 100644 --- a/openam-server-only/pom.xml +++ b/openam-server-only/pom.xml @@ -973,5 +973,12 @@ org.openidentityplatform.openam openam-cassandra-cts + + + + org.testng + testng + test + diff --git a/openam-server-only/src/main/webapp/WEB-INF/web.xml b/openam-server-only/src/main/webapp/WEB-INF/web.xml index 6274bbff6a..b4253b2231 100644 --- a/openam-server-only/src/main/webapp/WEB-INF/web.xml +++ b/openam-server-only/src/main/webapp/WEB-INF/web.xml @@ -1,4 +1,20 @@ + OpenAM Services @@ -226,21 +242,45 @@ /oauth2/* + AuthNFilter /ws/* + REQUEST + FORWARD + INCLUDE + ASYNC + ERROR JAXRPCRequestFilter /jaxrpc/* + REQUEST + FORWARD + INCLUDE + ASYNC + ERROR AuthZFilter /ws/* + REQUEST + FORWARD + INCLUDE + ASYNC + ERROR NotificationsWebSocketFilter /notifications + REQUEST + FORWARD + INCLUDE + ASYNC + ERROR diff --git a/openam-server-only/src/test/java/org/openidentityplatform/openam/server/SecurityFilterDispatchersTest.java b/openam-server-only/src/test/java/org/openidentityplatform/openam/server/SecurityFilterDispatchersTest.java new file mode 100644 index 0000000000..70f8b4f7cc --- /dev/null +++ b/openam-server-only/src/test/java/org/openidentityplatform/openam/server/SecurityFilterDispatchersTest.java @@ -0,0 +1,109 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package org.openidentityplatform.openam.server; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; + +import java.io.File; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilderFactory; + +import org.testng.annotations.BeforeClass; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.Node; +import org.w3c.dom.NodeList; + +/** + * Pins the {@code } declarations of the security filters in the shipped + * {@code web.xml}. A {@code } that names no {@code } applies to + * REQUEST only, so a {@code RequestDispatcher} forward or include, an async dispatch or an + * error-page dispatch reaches the mapped endpoint with the filter skipped; naming any type + * replaces that default, so every type has to be listed. + */ +public class SecurityFilterDispatchersTest { + + private static final Set EVERY_DISPATCHER_TYPE = new HashSet<>( + Arrays.asList("REQUEST", "FORWARD", "INCLUDE", "ASYNC", "ERROR")); + + private Document webXml; + + @BeforeClass + public void parseWebXml() throws Exception { + File file = new File(System.getProperty("basedir", System.getProperty("user.dir")), + "src/main/webapp/WEB-INF/web.xml"); + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setNamespaceAware(true); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + webXml = factory.newDocumentBuilder().parse(file); + } + + /** + * Filters that authenticate or authorize the caller of a narrowly mapped endpoint. + */ + @DataProvider + public Object[][] securityFilters() { + return new Object[][] { + {"AuthNFilter"}, + {"AuthZFilter"}, + {"JAXRPCRequestFilter"}, + {"NotificationsWebSocketFilter"}, + }; + } + + @Test(dataProvider = "securityFilters") + public void securityFilterRunsOnEveryDispatchType(String filterName) { + List mappings = filterMappings(filterName); + assertFalse(mappings.isEmpty(), filterName + " must have a "); + for (Element mapping : mappings) { + assertEquals(childTexts(mapping, "dispatcher"), EVERY_DISPATCHER_TYPE, + filterName + " on " + childTexts(mapping, "url-pattern") + ": every " + + "type must be declared, or a forward, include, async or error dispatch reaches " + + "the endpoint with the filter skipped"); + } + } + + private List filterMappings(String filterName) { + List result = new ArrayList<>(); + NodeList mappings = webXml.getElementsByTagNameNS("*", "filter-mapping"); + for (int i = 0; i < mappings.getLength(); i++) { + Element mapping = (Element) mappings.item(i); + if (childTexts(mapping, "filter-name").contains(filterName)) { + result.add(mapping); + } + } + return result; + } + + private static Set childTexts(Element parent, String childName) { + Set texts = new HashSet<>(); + for (Node child = parent.getFirstChild(); child != null; child = child.getNextSibling()) { + if (child instanceof Element && childName.equals(child.getLocalName())) { + texts.add(child.getTextContent().trim()); + } + } + return texts; + } +}