diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/web/Laundromat.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/web/Laundromat.java index c335cd6c057..9ca8979effb 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/web/Laundromat.java +++ b/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,7 +63,8 @@ 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. */ @@ -70,6 +72,26 @@ 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 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 diff --git a/opengrok-indexer/src/test/java/org/opengrok/indexer/web/LaundromatTest.java b/opengrok-indexer/src/test/java/org/opengrok/indexer/web/LaundromatTest.java index 2d2429dcb5e..0ff05731a94 100644 --- a/opengrok-indexer/src/test/java/org/opengrok/indexer/web/LaundromatTest.java +++ b/opengrok-indexer/src/test/java/org/opengrok/indexer/web/LaundromatTest.java @@ -70,6 +70,22 @@ void testLaunderServerName(Pair param) { assertEquals(param.getLeft(), param.getRight()); } + private static Stream> 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 param) { + assertEquals(param.getLeft(), param.getRight()); + } + @Test void launderLogMap() { HashMap testMap = new HashMap<>(); diff --git a/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java b/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java index da1e5670c22..d79f7d52cd4 100644 --- a/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java +++ b/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java @@ -232,26 +232,23 @@ 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)); } } } @@ -259,7 +256,7 @@ private boolean getFileRevision(DiffData data, String context, String[] filepath 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 history"; return false; diff --git a/opengrok-web/src/test/java/org/opengrok/web/DiffTest.java b/opengrok-web/src/test/java/org/opengrok/web/DiffTest.java index de395f0d342..ca9e6399c7d 100644 --- a/opengrok-web/src/test/java/org/opengrok/web/DiffTest.java +++ b/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());