fix reflective XSS of the revision parameter handling

opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java

@@ -280,24 +280,18 @@ Executor getHistoryLogExecutor(File file, String sinceRevision, String tillRevis
      * Try to get file contents for given revision.
      *
      * @param sink a required target sink
-     * @param fullpath full pathname of the file
-     * @param rev revision
+     * @param fullPath full pathname of the file
+     * @param revision revision
      * @return a defined instance with {@code success} == {@code true} if no
      * error occurred and with non-zero {@code iterations} if some data was
      * transferred
      */
-    private HistoryRevResult getHistoryRev(BufferSink sink, String fullpath, String rev) {
-
+    private HistoryRevResult getHistoryRev(BufferSink sink, String fullPath, String revision) {
         HistoryRevResult result = new HistoryRevResult();
         File directory = new File(getDirectoryName());
 
-        String revision = rev;
-        if (rev.indexOf(':') != -1) {
-            revision = rev.substring(0, rev.indexOf(':'));
-        }
-
         try {
-            String filename = fullpath.substring(getDirectoryName().length() + 1);
+            String filename = fullPath.substring(getDirectoryName().length() + 1);
             ensureCommand(CMD_PROPERTY_KEY, CMD_FALLBACK);
             String[] argv = {RepoCommand, "cat", "-r", revision, filename};
             Executor executor = new Executor(Arrays.asList(argv), directory,
@@ -307,7 +301,7 @@ private HistoryRevResult getHistoryRev(BufferSink sink, String fullpath, String
 
             /*
              * If exit value of the process was not 0 then the file did
-             * not exist or internal hg error occured.
+             * not exist or internal hg error occurred.
              */
             result.success = (status == 0);
         } catch (Exception exp) {
@@ -321,13 +315,13 @@ private HistoryRevResult getHistoryRev(BufferSink sink, String fullpath, String
      * Get the name of file in given revision. This is used to get contents
      * of a file in historical revision.
      *
-     * @param fullpath file path
+     * @param fullPath file path
      * @param fullRevToFind revision number (in the form of <code>{rev}:{node|short}</code>)
      * @return original filename
      */
-    private String findOriginalName(String fullpath, String fullRevToFind) throws IOException {
+    private String findOriginalName(String fullPath, String fullRevToFind) throws IOException {
         Matcher matcher = LOG_COPIES_PATTERN.matcher("");
-        String file = fullpath.substring(getDirectoryName().length() + 1);
+        String file = fullPath.substring(getDirectoryName().length() + 1);
         ArrayList<String> argv = new ArrayList<>();
         File directory = new File(getDirectoryName());
 
@@ -359,7 +353,7 @@ private String findOriginalName(String fullpath, String fullRevToFind) throws IO
         // argv.add("reverse(" + rev_to_find + ":)");
         argv.add("--template");
         argv.add("{rev}:{file_copies}\\n");
-        argv.add(fullpath);
+        argv.add(fullPath);
 
         Executor executor = new Executor(argv, directory,
                 RuntimeEnvironment.getInstance().getInteractiveCommandTimeout());
@@ -405,26 +399,25 @@ private String findOriginalName(String fullpath, String fullRevToFind) throws IO
         if (status != 0) {
             LOGGER.log(Level.WARNING,
                     "Failed to get original name in revision {2} for: \"{0}\" Exit code: {1}",
-                    new Object[]{fullpath, String.valueOf(status), fullRevToFind});
+                    new Object[]{fullPath, String.valueOf(status), fullRevToFind});
             return null;
         }
 
-        return (fullpath.substring(0, getDirectoryName().length() + 1) + file);
+        return (fullPath.substring(0, getDirectoryName().length() + 1) + file);
     }
 
     @Override
     boolean getHistoryGet(OutputStream out, String parent, String basename, String rev) {
-
-        String fullpath;
+        String fullPath;
         try {
-            fullpath = new File(parent, basename).getCanonicalPath();
+            fullPath = new File(parent, basename).getCanonicalPath();
         } catch (IOException exp) {
             LOGGER.log(Level.SEVERE,
                     "Failed to get canonical path: {0}", exp.getClass().toString());
             return false;
         }
 
-        HistoryRevResult result = getHistoryRev(out::write, fullpath, rev);
+        HistoryRevResult result = getHistoryRev(out::write, fullPath, rev);
         if (!result.success && result.iterations < 1) {
             /*
              * If we failed to get the contents it might be that the file was
@@ -433,14 +426,14 @@ boolean getHistoryGet(OutputStream out, String parent, String basename, String r
              */
             String origpath;
             try {
-                origpath = findOriginalName(fullpath, rev);
+                origpath = findOriginalName(fullPath, rev);
             } catch (IOException exp) {
                 LOGGER.log(Level.SEVERE,
                         "Failed to get original revision: {0}",
                         exp.getClass().toString());
                 return false;
             }
-            if (origpath != null && !origpath.equals(fullpath)) {
+            if (origpath != null && !origpath.equals(fullPath)) {
                 result = getHistoryRev(out::write, origpath, rev);
             }
         }

opengrok-indexer/src/main/java/org/opengrok/indexer/web/Laundromat.java

@@ -51,6 +51,16 @@ public static String launderInput(String value) {
         return replaceAll(value, ESC_N_R_T_F, "_");
     }
 
+    /**
+     * Sanitize {@code value} where it will be used in subsequent OpenGrok
+     * (non-logging) processing.
+     * @return {@code null} if null or else {@code value} with anything besides
+     * alphanumeric or {@code :} characters removed.
+     */
+    public static String launderRevision(String value) {
+        return replaceAll(value, "[^a-zA-Z0-9:]", "");
+    }
+
     /**
      * Sanitize {@code value} where it will be used in a Lucene query.
      * @return {@code null} if null or else {@code value} with "pattern-breaking

opengrok-web/src/main/java/org/opengrok/web/PageConfig.java

@@ -711,13 +711,17 @@ public String getDefineTagsIndex() {
 
     /**
      * Get the revision parameter {@code r} from the request.
+     * Anything besides alphanumeric and {@code :} is removed via {@link Laundromat#launderInput(String)}.
      *
      * @return revision if found, an empty string otherwise.
      */
     public String getRequestedRevision() {
         if (rev == null) {
-            String tmp = Laundromat.launderInput(req.getParameter(QueryParameters.REVISION_PARAM));
-            rev = (tmp != null && !tmp.isEmpty()) ? tmp : "";
+            String tmp = Laundromat.launderRevision(req.getParameter(QueryParameters.REVISION_PARAM));
+            rev = Optional.ofNullable(tmp)
+                    .filter(s -> !s.isEmpty())
+                    .orElse("");
+
         }
         return rev;
     }

opengrok-web/src/main/webapp/list.jsp

@@ -146,8 +146,7 @@ document.pageReady.push(function() { pageReadyList();});
             if (!projects.contains(projectName)) {
                 projects.add(projectName);
                 // update cookie
-                cookieValue = cookieValue.length() == 0 ? projectName :
-                        projectName + ',' + cookieValue;
+                cookieValue = cookieValue.isEmpty() ? projectName : projectName + ',' + cookieValue;
                 Cookie cookie = new Cookie(PageConfig.OPEN_GROK_PROJECT, URLEncoder.encode(cookieValue, StandardCharsets.UTF_8));
                 // TODO hmmm, projects.jspf doesn't set a path
                 cookie.setPath(request.getContextPath() + '/');
@@ -210,7 +209,7 @@ document.pageReady.push(function() { pageReadyList();});
         }
 
         statistics.report(LOGGER, Level.FINE, "directory listing done", "dir.list.latency");
-    } else if (rev.length() != 0) {
+    } else if (!rev.isEmpty()) {
         // requesting a revision
         File xrefFile;
         if (cfg.isLatestRevision(rev) && (xrefFile = cfg.findDataFile()) != null) {

opengrok-web/src/main/webapp/mast.jsp

@@ -18,7 +18,7 @@ information: Portions Copyright [yyyy] [name of copyright owner]
 
 CDDL HEADER END
 
-Copyright (c) 2005, 2022, Oracle and/or its affiliates. All rights reserved.
+Copyright (c) 2005, 2025, Oracle and/or its affiliates. All rights reserved.
 Portions Copyright 2011 Jens Elkner.
 Portions Copyright (c) 2018, Chris Fraire <cfraire@me.com>.
 
@@ -45,7 +45,7 @@ org.opengrok.indexer.web.Util"%>
     }
 
     String redir = cfg.canProcess();
-    if (redir == null || redir.length() > 0) {
+    if (redir == null || !redir.isEmpty()) {
         if (redir == null) {            
             response.sendError(HttpServletResponse.SC_NOT_FOUND);
         } else {

opengrok-web/src/main/webapp/xref.jspf

@@ -16,7 +16,7 @@ information: Portions Copyright [yyyy] [name of copyright owner]
 
 CDDL HEADER END
 
-Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved.
+Copyright (c) 2015, 2025, Oracle and/or its affiliates. All rights reserved.
 Portions Copyright 2011 Jens Elkner.
 Portions Copyright (c) 2017-2020, Chris Fraire <cfraire@me.com>.
 --%>
@@ -170,7 +170,7 @@ org.opengrok.indexer.web.QueryParameters"
 <h3 class="error">Error reading file</h3><%
             if (error != null) {
 %>
-<p class="error"><%= error %></p><%
+<p class="error"><%= Util.htmlize(error) %></p><%
             }
         }
     } else if (g == AbstractAnalyzer.Genre.IMAGE) {

opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java

@@ -26,21 +26,27 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.net.URL;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import jakarta.servlet.http.HttpServletRequest;
+import org.apache.commons.lang3.tuple.Pair;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.condition.EnabledOnOs;
 import org.junit.jupiter.api.condition.OS;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.opengrok.indexer.authorization.AuthControlFlag;
 import org.opengrok.indexer.authorization.AuthorizationFramework;
 import org.opengrok.indexer.authorization.AuthorizationPlugin;
@@ -79,12 +85,14 @@ public static void setUpClass() throws Exception {
         env.setHistoryEnabled(true);
 
         repository = new TestRepository();
-        repository.create(PageConfigTest.class.getResource("/repositories"));
+        URL repositoryURL = PageConfigTest.class.getResource("/repositories");
+        assertNotNull(repositoryURL);
+        repository.create(repositoryURL);
         env.setRepositories(repository.getSourceRoot());
     }
 
     @AfterAll
-    public static void tearDownClass() throws Exception {
+    public static void tearDownClass() {
         repository.destroy();
         repository = null;
     }
@@ -175,7 +183,7 @@ void testGetResourceFileList() {
         env.setProjectsEnabled(true);
 
         // Enable projects.
-        for (String file : new File(repository.getSourceRoot()).list()) {
+        for (String file : Objects.requireNonNull(new File(repository.getSourceRoot()).list())) {
             Project proj = new Project(file);
             proj.setIndexed(true);
             env.getProjects().put(file, proj);
@@ -235,13 +243,11 @@ public boolean isAllowed(HttpServletRequest request, Project project) {
     void testGetSortedFilesDirsFirst() throws IOException {
         RuntimeEnvironment env = RuntimeEnvironment.getInstance();
         env.setListDirsFirst(true);
-        // Cannot spy/mock final class.
-        HttpServletRequest req = createRequest("/source", "/xref", "");
-        PageConfig pageConfig = PageConfig.get(req);
 
         // Make sure the source root has just directories.
         File sourceRootFile = new File(repository.getSourceRoot());
-        assertTrue(Arrays.stream(sourceRootFile.listFiles()).filter(File::isFile).
+        assertTrue(Arrays.stream(Objects.requireNonNull(sourceRootFile.listFiles())).
+                filter(File::isFile).
                 collect(Collectors.toSet()).isEmpty());
 
         // Create regular file under source root.
@@ -345,15 +351,6 @@ void testGetLatestRevisionViaIndex() throws Exception {
         indexer.doIndexerExecution(null, null);
 
         final String filePath = "/git/main.c";
-
-        DummyHttpServletRequest req1 = new DummyHttpServletRequest() {
-            @Override
-            public String getPathInfo() {
-                return filePath;
-            }
-        };
-
-        PageConfig cfg = PageConfig.get(req1);
         String rev = LatestRevisionUtil.getLastRevFromIndex(new File(repository.getSourceRoot(), filePath));
         assertNotNull(rev);
         assertEquals(HASH_AA35C258, rev);
@@ -425,30 +422,37 @@ public String getPathInfo() {
         assertNull(rev);
     }
 
-    @Test
-    void testGetRequestedRevision() {
-        final String[] revisions = {"6c5588de", "", "6c5588de", "6c5588de", "6c5588de"};
-        for (int i = 0; i < revisions.length; i++) {
-            final int index = i;
-            DummyHttpServletRequest req = new DummyHttpServletRequest() {
-                @Override
-                public String getParameter(String name) {
-                    if (name.equals("r")) {
-                        return revisions[index];
-                    }
-                    return null;
+    private static Stream<Pair<String, String>> getParamsForTestGetRequestedRevision() {
+        return Stream.of(Pair.of("6c5588de", "6c5588de"),
+                Pair.of("10013:cb02e4e3d492", "10013:cb02e4e3d492"),
+                Pair.of("", ""),
+                Pair.of(null, ""),
+                Pair.of("(foo)\n", "foo"));
+    }
+
+    @MethodSource("getParamsForTestGetRequestedRevision")
+    @ParameterizedTest
+    void testGetRequestedRevision(Pair<String, String> revisionParam) {
+        final String actualRevision = revisionParam.getLeft();
+        final String expectedRevision = revisionParam.getRight();
+        DummyHttpServletRequest req = new DummyHttpServletRequest() {
+            @Override
+            public String getParameter(String name) {
+                if (name.equals("r")) {
+                    return actualRevision;
                 }
-            };
+                return null;
+            }
+        };
 
-            PageConfig cfg = PageConfig.get(req);
-            String rev = cfg.getRequestedRevision();
+        PageConfig cfg = PageConfig.get(req);
+        String rev = cfg.getRequestedRevision();
 
-            assertNotNull(rev);
-            assertEquals(revisions[i], rev);
-            assertFalse(rev.contains("r="));
+        assertNotNull(rev);
+        assertEquals(expectedRevision, rev);
+        assertFalse(rev.contains("r="));
 
-            PageConfig.cleanup(req);
-        }
+        PageConfig.cleanup(req);
     }
 
     @Test
@@ -554,7 +558,7 @@ void testCheckSourceRootExistence3() throws IOException {
         File temp = File.createTempFile("opengrok", "-test-file.tmp");
         Files.delete(temp.toPath());
         RuntimeEnvironment.getInstance().setSourceRoot(temp.getAbsolutePath());
-        assertThrows(IOException.class, () -> cfg.checkSourceRootExistence(),
+        assertThrows(IOException.class, cfg::checkSourceRootExistence,
                 "This should throw an exception when the file does not exist");
         RuntimeEnvironment.getInstance().setSourceRoot(path);
         PageConfig.cleanup(req);
@@ -576,7 +580,7 @@ void testCheckSourceRootExistence4() throws IOException {
         // skip the test if the implementation does not permit setting permissions
         assumeTrue(temp.setReadable(false));
         RuntimeEnvironment.getInstance().setSourceRoot(temp.getAbsolutePath());
-        assertThrows(IOException.class, () -> cfg.checkSourceRootExistence(),
+        assertThrows(IOException.class, cfg::checkSourceRootExistence,
                 "This should throw an exception when the file is not readable");
         RuntimeEnvironment.getInstance().setSourceRoot(path);