sanitize path with revision for diff.jsp (#4834)

Author: vladak

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

@@ -25,20 +25,21 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 /**
- * Represents a container for sanitizing methods for avoiding classifications as
- * taint bugs.
+ * Represents a container for sanitizing methods for avoiding classifications as taint bugs.
  */
 public class Laundromat {
 
-    private static final String ESC_N_R_T_F = "[\\n\\r\\t\\f]";
+    private static final String ESC_N_R_T_F = "[\\n\\r\\t\\f\\u0000]";
     private static final String ESG_N_R_T_F_1_N = ESC_N_R_T_F + "+";
 
     /**
@@ -62,14 +63,35 @@ public static String launderServerName(String value) {
 
     /**
      * Sanitize {@code value} where it will be used in subsequent OpenGrok
-     * (non-logging) processing.
+     * (non-logging) processing. The value is assumed to represent a revision string,
+     * not including file path.
      * @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 subsequent OpenGrok
+     * (non-logging) processing. The value is assumed to represent URI path,
+     * not necessarily existent on the file system. Further, it assumes that the URI path
+     * is already decoded, e.g. {@code %2e%2e} turned into {@code ..}.
+     * @return {@code value} with path traversal path components {@code /../} removed
+     * and null characters replaced with {@code _}.
+     */
+    public static String launderUriPath(@NotNull String value) {
+        List<String> pathElements = new ArrayList<>();
+        String uriPath = Laundromat.launderInput(value);
+        for (String pathElement : uriPath.split("/")) {
+            if (pathElement.isEmpty() || pathElement.equals("..")) {
+                continue;
+            }
+            pathElements.add(pathElement);
+        }
+        return (uriPath.startsWith("/") ? "/" : "") + String.join("/", pathElements);
+    }
+
     /**
      * 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-indexer/src/test/java/org/opengrok/indexer/web/LaundromatTest.java

@@ -70,6 +70,22 @@ void testLaunderServerName(Pair<String, String> param) {
         assertEquals(param.getLeft(), param.getRight());
     }
 
+    private static Stream<Pair<String, String>> getParamsForTestLaunderUriPath() {
+        return Stream.of(Pair.of("foo", Laundromat.launderUriPath("../../../foo")),
+                Pair.of("/foo/bar", Laundromat.launderUriPath("/foo/../../bar")),
+                Pair.of("/foo/bar..", Laundromat.launderUriPath("/foo/bar..")),
+                Pair.of("/foo/bar", Laundromat.launderUriPath("/foo//bar")),
+                Pair.of("..foo/bar", Laundromat.launderUriPath("..foo/bar")),
+                Pair.of("/foo/bar.txt", Laundromat.launderUriPath("/foo/bar.txt")),
+                Pair.of("/foo/bar_.txt", Laundromat.launderUriPath("/foo/bar\u0000.txt")));
+    }
+
+    @ParameterizedTest
+    @MethodSource("getParamsForTestLaunderUriPath")
+    void testLaunderUriPath(Pair<String, String> param) {
+        assertEquals(param.getLeft(), param.getRight());
+    }
+
     @Test
     void launderLogMap() {
         HashMap<String, String[]> testMap = new HashMap<>();

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

@@ -232,34 +232,31 @@ public String getHeaderData() {
     }
 
     /**
-     * Extract file path and revision strings from the URL.
-     * @param data DiffData object
+     * Extract file path and revision strings from the URI. Basically the request URI looks like this:
+     * {@code http://$site/$webapp/diff/$resourceFile?r1=$fileA@$revA&r2=$fileB@$revB}
+     * The code extracts file path and revision from the URI.
+     * @param data DiffData object (output parameter)
      * @param context context path
      * @param filepath file path array (output parameter)
      * @return true if the extraction was successful, false otherwise
      * (in which case {@link DiffData#errorMsg} will be set)
      */
     private boolean getFileRevision(DiffData data, String context, String[] filepath) {
-        /*
-         * Basically the request URI looks like this:
-         * http://$site/$webapp/diff/$resourceFile?r1=$fileA@$revA&r2=$fileB@$revB
-         * The code below extracts file path and revision from the URI.
-         */
         for (int i = 1; i <= 2; i++) {
             String p = req.getParameter(QueryParameters.REVISION_PARAM + i);
             if (p != null) {
                 int j = p.lastIndexOf("@");
                 if (j != -1) {
-                    filepath[i - 1] = p.substring(0, j);
-                    data.rev[i - 1] = p.substring(j + 1);
+                    filepath[i - 1] = Laundromat.launderUriPath(p.substring(0, j));
+                    data.rev[i - 1] = Laundromat.launderRevision(p.substring(j + 1));
                 }
             }
         }
 
         if (data.rev[0] == null || data.rev[1] == null
                 || data.rev[0].isEmpty() || data.rev[1].isEmpty()
                 || data.rev[0].equals(data.rev[1])) {
-            data.errorMsg = "Please pick two revisions to compare the changed "
+            data.errorMsg = "Please pick two revisions to compare the changes "
                     + "from the <a href=\"" + context + Prefix.HIST_L
                     + getUriEncodedPath() + "\">history</a>";
             return false;

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

@@ -18,7 +18,7 @@
  */
 
 /*
- * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, 2025, Oracle and/or its affiliates. All rights reserved.
  */
 package org.opengrok.web;
 
@@ -32,6 +32,8 @@
 import org.opengrok.indexer.util.TestRepository;
 import org.opengrok.indexer.web.QueryParameters;
 
+import java.net.URL;
+
 import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -50,7 +52,9 @@ class DiffTest {
     @BeforeAll
     static void setUp() throws Exception {
         repository = new TestRepository();
-        repository.create(DiffTest.class.getResource("/repositories"));
+        URL resource = DiffTest.class.getResource("/repositories");
+        assertNotNull(resource);
+        repository.create(resource);
 
         env.setSourceRoot(repository.getSourceRoot());
         env.setDataRoot(repository.getDataRoot());