From a93991fd30168192890701af43bb5760aed74ad8 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 12:19:42 +0300 Subject: [PATCH 1/3] Fix partial path traversal in UI ResourceServlet The containment check compared canonical paths as string prefixes, so a sibling directory sharing the configured directory's name prefix (e.g. ui/admin/default-old next to ui/admin/default) was reachable via "..". Resolve requests with java.nio.file.Path and check containment on path components, resolving symlinks on both sides. Serve files through Files/Path instead of file: URLs. Resolves CodeQL alerts #2, #3, #8, #9 (java/partial-path-traversal) and #5, #6, #7 (java/ssrf) in openidm-servlet. --- openidm-servlet/pom.xml | 17 +- .../ui/internal/service/ResourceServlet.java | 132 ++++---- .../internal/service/ResourceServletTest.java | 319 ++++++++++++++++++ 3 files changed, 393 insertions(+), 75 deletions(-) create mode 100644 openidm-servlet/src/test/java/org/forgerock/openidm/ui/internal/service/ResourceServletTest.java diff --git a/openidm-servlet/pom.xml b/openidm-servlet/pom.xml index c576385a8..423b54f54 100644 --- a/openidm-servlet/pom.xml +++ b/openidm-servlet/pom.xml @@ -22,7 +22,7 @@ ~ your own identifying information: ~ "Portions Copyrighted [year] [name of copyright owner]" ~ - ~ Portions Copyrighted 2024-2025 3A Systems LLC. + ~ Portions Copyrighted 2024-2026 3A Systems LLC. --> 4.0.0 @@ -80,6 +80,21 @@ org.apache.felix.framework provided + + org.testng + testng + test + + + org.mockito + mockito-all + test + + + org.slf4j + slf4j-simple + test + diff --git a/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java b/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java index 0b1bf0763..5eb2f0468 100644 --- a/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java +++ b/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java @@ -19,13 +19,13 @@ */ package org.forgerock.openidm.ui.internal.service; -import java.io.File; import java.io.IOException; -import java.io.InputStream; import java.io.OutputStream; -import java.net.URL; -import java.net.URLConnection; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.InvalidPathException; +import java.nio.file.Path; +import java.nio.file.Paths; import java.util.Dictionary; import java.util.Hashtable; import java.util.Map; @@ -144,37 +144,62 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) } // Locate the file in extension dir first, fall back to default dir - URL url = null; - String loadDir = (String) PropertyUtil.substVars(extensionDir, IdentityServer.getInstance(), false); - File file = new File(loadDir + target); - if (file.getCanonicalPath().startsWith(new File(loadDir).getCanonicalPath()) - && file.exists() && !file.isDirectory()) { - url = file.getCanonicalFile().toURI().toURL(); - } else { - loadDir = (String) PropertyUtil.substVars(defaultDir, IdentityServer.getInstance(), false); - file = new File(loadDir + target); - if (file.getCanonicalPath().startsWith(new File(loadDir).getCanonicalPath()) - && file.exists() && !file.isDirectory()) { - url = file.getCanonicalFile().toURI().toURL(); - } + Path file = locate(extensionDir, target); + if (file == null) { + file = locate(defaultDir, target); } - if (url == null) { + if (file == null) { res.sendError(HttpServletResponse.SC_NOT_FOUND); } else if (target.equals("/index.html")) { - handleIndexHtml(res, url); + handleIndexHtml(res, file); } else { - handle(req, res, url, target); + handle(req, res, file, target); } } } + /** + * Resolves a request path against one of the configured resource directories. + *

+ * Containment is checked on path components ({@link Path#startsWith(Path)}), never on a + * string prefix, so a sibling directory such as {@code default-old} is not reachable from + * {@code default}. Symbolic links are resolved on both sides before the final check. + * + * @param dir the configured directory, possibly containing {@code &{...}} property references + * @param target the request path, always starting with {@code /} + * @return the real path of the regular file denoted by {@code target} inside {@code dir}, + * or {@code null} if the directory does not exist, the file does not exist, is not a + * regular file, or lies outside the directory + */ + private Path locate(String dir, String target) { + String loadDir = (String) PropertyUtil.substVars(dir, IdentityServer.getInstance(), false); + Path base; + Path file; + try { + base = Paths.get(loadDir).toAbsolutePath().normalize(); + file = base.resolve(target.substring(1)).normalize(); + } catch (InvalidPathException e) { + return null; + } + if (!file.startsWith(base) || !Files.isDirectory(base) || !Files.isRegularFile(file)) { + return null; + } + try { + Path realFile = file.toRealPath(); + return realFile.startsWith(base.toRealPath()) ? realFile : null; + } catch (IOException e) { + // vanished or unreadable between the check above and here + return null; + } + } + /** * Serves index.html with the openidm.context.path injected as a global JS variable. * Replaces {@code } with a small inline script that sets * {@code window.__openidm_context_path} before RequireJS boots. */ - private void handleIndexHtml(HttpServletResponse res, URL url) throws IOException { + private void handleIndexHtml(HttpServletResponse res, Path file) throws IOException { res.setContentType("text/html"); res.setHeader("Cache-Control", "no-cache"); @@ -185,11 +210,7 @@ private void handleIndexHtml(HttpServletResponse res, URL url) throws IOExceptio // Strip leading slash — the UI Constants.context value does not include it String contextValue = contextPath.substring(1); - byte[] raw; - try (InputStream is = url.openStream()) { - raw = is.readAllBytes(); - } - String html = new String(raw, StandardCharsets.UTF_8); + String html = new String(Files.readAllBytes(file), StandardCharsets.UTF_8); // Inject a tiny script right before so it is available before RequireJS loads. // Escape characters that could break out of the JS string or the script tag. @@ -259,7 +280,7 @@ private void clear() { logger.debug("Unregistered UI servlet at {}", contextRoot); } - private void handle(HttpServletRequest req, HttpServletResponse res, URL url, String resName) + private void handle(HttpServletRequest req, HttpServletResponse res, Path file, String resName) throws IOException { String contentType = getServletContext().getMimeType(resName); if (contentType != null) { @@ -268,7 +289,7 @@ private void handle(HttpServletRequest req, HttpServletResponse res, URL url, St res.setContentType(getMimeType(resName)); } - long lastModified = getLastModified(url); + long lastModified = getLastModified(file); if (lastModified != 0) { res.setDateHeader("Last-Modified", lastModified); } @@ -276,31 +297,16 @@ private void handle(HttpServletRequest req, HttpServletResponse res, URL url, St if (!resourceModified(lastModified, req.getDateHeader("If-Modified-Since"))) { res.setStatus(HttpServletResponse.SC_NOT_MODIFIED); } else { - copyResource(url, res); + copyResource(file, res); } } - private long getLastModified(URL url) { - long lastModified = 0; - + private long getLastModified(Path file) { try { - URLConnection conn = url.openConnection(); - lastModified = conn.getLastModified(); - } catch (Exception e) { - // Do nothing + return Files.getLastModifiedTime(file).toMillis(); + } catch (IOException e) { + return 0; } - - if (lastModified == 0) { - String filepath = url.getPath(); - if (filepath != null) { - File f = new File(filepath); - if (f.exists()) { - lastModified = f.lastModified(); - } - } - } - - return lastModified; } private String getMimeType(String fileName) { @@ -324,33 +330,11 @@ private boolean resourceModified(long resTimestamp, long modSince) { return resTimestamp == 0 || modSince == -1 || resTimestamp > modSince; } - private void copyResource(URL url, HttpServletResponse res) + private void copyResource(Path file, HttpServletResponse res) throws IOException { - OutputStream os = null; - InputStream is = null; - - try { - os = res.getOutputStream(); - is = url.openStream(); - - int len = 0; - byte[] buf = new byte[1024]; - int n; - - while ((n = is.read(buf, 0, buf.length)) >= 0) { - os.write(buf, 0, n); - len += n; - } - - res.setContentLength(len); - } finally { - if (is != null) { - is.close(); - } - - if (os != null) { - os.close(); - } + res.setContentLengthLong(Files.size(file)); + try (OutputStream os = res.getOutputStream()) { + Files.copy(file, os); } } diff --git a/openidm-servlet/src/test/java/org/forgerock/openidm/ui/internal/service/ResourceServletTest.java b/openidm-servlet/src/test/java/org/forgerock/openidm/ui/internal/service/ResourceServletTest.java new file mode 100644 index 000000000..b1ab2f136 --- /dev/null +++ b/openidm-servlet/src/test/java/org/forgerock/openidm/ui/internal/service/ResourceServletTest.java @@ -0,0 +1,319 @@ +/* + * 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.forgerock.openidm.ui.internal.service; + +import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.lang.reflect.Field; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; + +import jakarta.servlet.ServletConfig; +import jakarta.servlet.ServletContext; +import jakarta.servlet.ServletOutputStream; +import jakarta.servlet.WriteListener; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import org.testng.SkipException; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +/** + * Tests for the static-resource lookup in {@link ResourceServlet}. + * + *

The servlet is driven through its real {@link ResourceServlet#doGet} with a mocked + * request/response pair. {@code defaultDir} / {@code extensionDir} are plain absolute paths + * (no {@code &{...}} variables), so {@code PropertyUtil.substVars} returns them untouched and + * no {@code IdentityServer} set-up is needed. + * + *

The fixture is a real directory tree, because the lookup resolves paths against the + * live filesystem: + * 

+ *   tmpDir/
+ *     secret.txt                ← above the UI root, must never be reachable
+ *     ui/
+ *       default/
+ *         index.html
+ *         shared.txt
+ *         js/app.js
+ *       default-old/            ← sibling sharing the "default" name prefix
+ *         leak.txt
+ *       extension/
+ *         shared.txt            ← overrides default/shared.txt
+ *       extension.bak/          ← sibling sharing the "extension" name prefix
+ *         leak.txt
+ *
+ */ +public class ResourceServletTest { + + private static final String INDEX_HTML = + "tindex"; + + /** Captures everything the servlet writes to the response body. */ + private static final class CapturingOutputStream extends ServletOutputStream { + private final ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + + @Override + public void write(int b) { + buffer.write(b); + } + + @Override + public boolean isReady() { + return true; + } + + @Override + public void setWriteListener(WriteListener writeListener) { + // not used + } + + String asString() { + return new String(buffer.toByteArray(), StandardCharsets.UTF_8); + } + } + + private Path tmpDir; + private Path defaultDir; + private Path extensionDir; + + private ResourceServlet servlet; + private HttpServletResponse response; + private CapturingOutputStream body; + + @BeforeMethod + public void setUp() throws Exception { + tmpDir = Files.createTempDirectory("openidm-ui-test-"); + Files.writeString(tmpDir.resolve("secret.txt"), "top-secret"); + + Path ui = tmpDir.resolve("ui"); + defaultDir = ui.resolve("default"); + extensionDir = ui.resolve("extension"); + + Files.createDirectories(defaultDir.resolve("js")); + Files.writeString(defaultDir.resolve("index.html"), INDEX_HTML); + Files.writeString(defaultDir.resolve("shared.txt"), "from-default"); + Files.writeString(defaultDir.resolve("js/app.js"), "default-js"); + + Files.createDirectories(ui.resolve("default-old")); + Files.writeString(ui.resolve("default-old/leak.txt"), "LEAKED"); + + Files.createDirectories(extensionDir); + Files.writeString(extensionDir.resolve("shared.txt"), "from-extension"); + + Files.createDirectories(ui.resolve("extension.bak")); + Files.writeString(ui.resolve("extension.bak/leak.txt"), "LEAKED"); + + servlet = newServlet(defaultDir.toString(), extensionDir.toString()); + + body = new CapturingOutputStream(); + response = mock(HttpServletResponse.class); + when(response.getOutputStream()).thenReturn(body); + } + + @AfterMethod + public void tearDown() { + deleteRecursively(tmpDir.toFile()); + } + + // ----------------------------------------------------------------------- + // Regular lookups + // ----------------------------------------------------------------------- + + @Test(description = "A file below the default dir is served with its MIME type") + public void testServesFileFromDefaultDir() throws Exception { + servlet.doGet(request("/js/app.js"), response); + + assertEquals(body.asString(), "default-js"); + verify(response).setContentType("application/javascript"); + verify(response, never()).sendError(anyInt()); + } + + @Test(description = "A file present in both dirs is taken from the extension dir") + public void testExtensionDirTakesPrecedence() throws Exception { + servlet.doGet(request("/shared.txt"), response); + + assertEquals(body.asString(), "from-extension"); + verify(response, never()).sendError(anyInt()); + } + + @Test(description = "index.html is served with the context path injected before ") + public void testIndexHtmlInjectsContextPath() throws Exception { + servlet.doGet(request("/index.html"), response); + + String html = body.asString(); + assertTrue(html.contains("window.__openidm_context_path="), html); + assertTrue(html.contains("\n"), html); + verify(response).setContentType("text/html"); + // set once in doGet() and once more in handleIndexHtml() + verify(response, atLeastOnce()).setHeader("Cache-Control", "no-cache"); + } + + @Test(description = "The context root '/' is answered with index.html") + public void testRootPathServesIndexHtml() throws Exception { + servlet.doGet(request("/"), response); + + assertTrue(body.asString().contains("index"), body.asString()); + verify(response, never()).sendError(anyInt()); + } + + @Test(description = "A non-existent extension dir is skipped and the default dir is used") + public void testMissingExtensionDirFallsBackToDefaultDir() throws Exception { + servlet = newServlet(defaultDir.toString(), tmpDir.resolve("ui/no-such-dir").toString()); + + servlet.doGet(request("/js/app.js"), response); + + assertEquals(body.asString(), "default-js"); + verify(response, never()).sendError(anyInt()); + } + + @Test(description = "An unchanged resource is answered with 304 and no body") + public void testNotModifiedSinceIsHonoured() throws Exception { + HttpServletRequest request = request("/js/app.js"); + when(request.getDateHeader("If-Modified-Since")) + .thenReturn(System.currentTimeMillis() + 60_000L); + + servlet.doGet(request, response); + + verify(response).setStatus(HttpServletResponse.SC_NOT_MODIFIED); + assertEquals(body.asString(), ""); + } + + // ----------------------------------------------------------------------- + // Rejections + // ----------------------------------------------------------------------- + + @Test(description = "A directory is never served") + public void testDirectoryIsNotServed() throws Exception { + servlet.doGet(request("/js"), response); + + verify(response).sendError(HttpServletResponse.SC_NOT_FOUND); + assertEquals(body.asString(), ""); + } + + @Test(description = "An unknown file yields 404") + public void testUnknownFileYields404() throws Exception { + servlet.doGet(request("/nope.txt"), response); + + verify(response).sendError(HttpServletResponse.SC_NOT_FOUND); + assertEquals(body.asString(), ""); + } + + @Test(description = "A symlink inside the dir that points outside it is rejected") + public void testSymlinkEscapingTheDirIsRejected() throws Exception { + createSymbolicLink(defaultDir.resolve("link.txt"), tmpDir.resolve("secret.txt")); + + servlet.doGet(request("/link.txt"), response); + + verify(response).sendError(HttpServletResponse.SC_NOT_FOUND); + assertEquals(body.asString(), ""); + } + + @Test(description = "A configured dir that is itself a symlink still serves its files") + public void testSymlinkedDirIsServed() throws Exception { + Path link = tmpDir.resolve("ui/extension-link"); + createSymbolicLink(link, extensionDir); + servlet = newServlet(defaultDir.toString(), link.toString()); + + servlet.doGet(request("/shared.txt"), response); + + assertEquals(body.asString(), "from-extension"); + verify(response, never()).sendError(anyInt()); + } + + @DataProvider(name = "traversalPaths") + public Object[][] traversalPaths() { + return new Object[][] { + { "/../default-old/leak.txt", "sibling of the default dir sharing its name prefix" }, + { "/js/../../default-old/leak.txt", "dot-dot climbing out of a real sub-directory" }, + { "/../extension.bak/leak.txt", "sibling of the extension dir sharing its name prefix" }, + { "/../../secret.txt", "file above the UI root" }, + }; + } + + @Test(dataProvider = "traversalPaths", + description = "Paths escaping the configured dirs are rejected with 404") + public void testPathTraversalIsRejected(String target, String scenario) throws Exception { + servlet.doGet(request(target), response); + + verify(response).sendError(HttpServletResponse.SC_NOT_FOUND); + assertEquals(body.asString(), "", scenario); + } + + // ----------------------------------------------------------------------- + // Helpers + // ----------------------------------------------------------------------- + + private static ResourceServlet newServlet(String defaultDir, String extensionDir) throws Exception { + ResourceServlet servlet = new ResourceServlet(); + injectField(servlet, "defaultDir", defaultDir); + injectField(servlet, "extensionDir", extensionDir); + + ServletContext servletContext = mock(ServletContext.class); + when(servletContext.getMimeType(anyString())).thenReturn(null); + ServletConfig servletConfig = mock(ServletConfig.class); + when(servletConfig.getServletContext()).thenReturn(servletContext); + servlet.init(servletConfig); + return servlet; + } + + private static HttpServletRequest request(String pathInfo) { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getServletPath()).thenReturn("/ui"); + when(request.getPathInfo()).thenReturn(pathInfo); + when(request.getDateHeader("If-Modified-Since")).thenReturn(-1L); + return request; + } + + private static void createSymbolicLink(Path link, Path target) throws IOException { + try { + Files.createSymbolicLink(link, target); + } catch (UnsupportedOperationException | SecurityException e) { + throw new SkipException("symbolic links not supported here: " + e); + } + } + + private static void injectField(Object target, String name, Object value) throws Exception { + Field field = target.getClass().getDeclaredField(name); + field.setAccessible(true); + field.set(target, value); + } + + private static void deleteRecursively(File file) { + File[] children = file.listFiles(); + if (children != null) { + for (File child : children) { + deleteRecursively(child); + } + } + file.delete(); + } +} From e735b4bb090c38756f77154f5a58133480b7568b Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 14:57:20 +0300 Subject: [PATCH 2/3] Redirect to the configured context root instead of the request's servlet path The bare-context-root redirect built its target from req.getServletPath(); use the configured urlContextRoot, which is the same value for the Pax Web alias registration but does not depend on anything in the request. Resolves CodeQL alert #4 (java/unvalidated-url-redirection). --- .../ui/internal/service/ResourceServlet.java | 5 +++-- .../internal/service/ResourceServletTest.java | 21 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java b/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java index 5eb2f0468..3619e84df 100644 --- a/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java +++ b/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java @@ -122,10 +122,11 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) throws ServletException, IOException { logger.debug("GET call on {}", req); - // the request pathInfo is always null for root contexts + // path info is null when the request names the context root itself without a trailing slash String target = req.getPathInfo(); if (target == null || "".equals(target)) { - res.sendRedirect(req.getServletPath() + "/"); + // redirect to the configured context root rather than anything taken from the request + res.sendRedirect("/".equals(contextRoot) ? "/" : contextRoot + "/"); } else { if ("/".equals(target)) { target = "/index.html"; diff --git a/openidm-servlet/src/test/java/org/forgerock/openidm/ui/internal/service/ResourceServletTest.java b/openidm-servlet/src/test/java/org/forgerock/openidm/ui/internal/service/ResourceServletTest.java index b1ab2f136..c017cc786 100644 --- a/openidm-servlet/src/test/java/org/forgerock/openidm/ui/internal/service/ResourceServletTest.java +++ b/openidm-servlet/src/test/java/org/forgerock/openidm/ui/internal/service/ResourceServletTest.java @@ -185,6 +185,27 @@ public void testRootPathServesIndexHtml() throws Exception { verify(response, never()).sendError(anyInt()); } + @Test(description = "A request without path info is redirected to the configured context root") + public void testMissingPathInfoRedirectsToContextRoot() throws Exception { + injectField(servlet, "contextRoot", "/admin"); + HttpServletRequest request = request(null); + when(request.getServletPath()).thenReturn("/somewhere-else"); + + servlet.doGet(request, response); + + verify(response).sendRedirect("/admin/"); + verify(response, never()).sendError(anyInt()); + } + + @Test(description = "The redirect for the root context root does not double the slash") + public void testMissingPathInfoRedirectsForRootContext() throws Exception { + injectField(servlet, "contextRoot", "/"); + + servlet.doGet(request(null), response); + + verify(response).sendRedirect("/"); + } + @Test(description = "A non-existent extension dir is skipped and the default dir is used") public void testMissingExtensionDirFallsBackToDefaultDir() throws Exception { servlet = newServlet(defaultDir.toString(), tmpDir.resolve("ui/no-such-dir").toString()); From 3d032fdacc408c855d0e2f85bd52f004f9692d5f Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 18 Sep 2026 18:05:25 +0300 Subject: [PATCH 3/3] Use String.isEmpty() for the empty path-info check Resolves CodeQL alert #437 (java/inefficient-empty-string-test). --- .../forgerock/openidm/ui/internal/service/ResourceServlet.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java b/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java index 3619e84df..53d210116 100644 --- a/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java +++ b/openidm-servlet/src/main/java/org/forgerock/openidm/ui/internal/service/ResourceServlet.java @@ -124,7 +124,7 @@ protected void doGet(HttpServletRequest req, HttpServletResponse res) // path info is null when the request names the context root itself without a trailing slash String target = req.getPathInfo(); - if (target == null || "".equals(target)) { + if (target == null || target.isEmpty()) { // redirect to the configured context root rather than anything taken from the request res.sendRedirect("/".equals(contextRoot) ? "/" : contextRoot + "/"); } else {