From c2abe9131c5a26368283d6d712dd3552b17fa5c2 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 18 Aug 2025 13:16:21 +0200 Subject: [PATCH 1/8] use isEmpty() instead of length check --- opengrok-web/src/main/webapp/list.jsp | 5 ++--- opengrok-web/src/main/webapp/mast.jsp | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/opengrok-web/src/main/webapp/list.jsp b/opengrok-web/src/main/webapp/list.jsp index be8ac04f2e4..c13c16e284b 100644 --- a/opengrok-web/src/main/webapp/list.jsp +++ b/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) { diff --git a/opengrok-web/src/main/webapp/mast.jsp b/opengrok-web/src/main/webapp/mast.jsp index 6d9020f3208..55213134fbd 100644 --- a/opengrok-web/src/main/webapp/mast.jsp +++ b/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 . @@ -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 { From cd12a675f4f9db9681a794db62ee4a94958cf4f8 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 18 Aug 2025 14:27:08 +0200 Subject: [PATCH 2/8] sanitize revision parameter --- .../org/opengrok/indexer/web/Laundromat.java | 10 ++++ .../java/org/opengrok/web/PageConfig.java | 8 ++- .../java/org/opengrok/web/PageConfigTest.java | 55 ++++++++++++------- 3 files changed, 50 insertions(+), 23 deletions(-) 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 edfa7665cb1..1519ec4dccb 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 @@ -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 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 821858d7d52..f06e7107f17 100644 --- a/opengrok-web/src/main/java/org/opengrok/web/PageConfig.java +++ b/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; } diff --git a/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java b/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java index 8b4eaba42f4..a1d0ffb211b 100644 --- a/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java +++ b/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java @@ -26,6 +26,7 @@ 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; @@ -34,13 +35,17 @@ import java.util.List; import java.util.Map; 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,7 +84,9 @@ 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()); } @@ -425,30 +432,36 @@ 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> getParamsForTestGetRequestedRevision() { + return Stream.of(Pair.of("6c5588de", "6c5588de"), + Pair.of("10013:cb02e4e3d492", "10013:cb02e4e3d492"), + Pair.of("", ""), + Pair.of("(foo)\n", "foo")); + } + + @MethodSource("getParamsForTestGetRequestedRevision") + @ParameterizedTest + void testGetRequestedRevision(Pair 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 From 2d99def7250bde5d5356bc1048cd7b2b33b88181 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 18 Aug 2025 14:56:05 +0200 Subject: [PATCH 3/8] no need to strip the second part of the revision string also fix style --- .../indexer/history/MercurialRepository.java | 39 ++++++++----------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java b/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java index d6baee4d954..172d8113cb2 100644 --- a/opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java +++ b/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 {rev}:{node|short}) * @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 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); } } From 64bab648030b925ae3488b73e64556a184cb6549 Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 18 Aug 2025 15:22:41 +0200 Subject: [PATCH 4/8] encode error output --- opengrok-web/src/main/webapp/xref.jspf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opengrok-web/src/main/webapp/xref.jspf b/opengrok-web/src/main/webapp/xref.jspf index ef626679d5c..b158d78ee60 100644 --- a/opengrok-web/src/main/webapp/xref.jspf +++ b/opengrok-web/src/main/webapp/xref.jspf @@ -170,7 +170,7 @@ org.opengrok.indexer.web.QueryParameters"

Error reading file

<% if (error != null) { %> -

<%= error %>

<% +

<%= Util.htmlize(error) %>

<% } } } else if (g == AbstractAnalyzer.Genre.IMAGE) { From 21c8c8ed2fb5b5ed6e2051da1502796d5768695d Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 18 Aug 2025 15:23:20 +0200 Subject: [PATCH 5/8] bump copyright year --- opengrok-web/src/main/webapp/xref.jspf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opengrok-web/src/main/webapp/xref.jspf b/opengrok-web/src/main/webapp/xref.jspf index b158d78ee60..d42753d0900 100644 --- a/opengrok-web/src/main/webapp/xref.jspf +++ b/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 . --%> From 7043f43737e1989692dd815145de86eb83c89d4f Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 18 Aug 2025 16:27:10 +0200 Subject: [PATCH 6/8] test null revision --- opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java b/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java index a1d0ffb211b..b738195362e 100644 --- a/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java +++ b/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java @@ -436,6 +436,7 @@ private static Stream> getParamsForTestGetRequestedRevision return Stream.of(Pair.of("6c5588de", "6c5588de"), Pair.of("10013:cb02e4e3d492", "10013:cb02e4e3d492"), Pair.of("", ""), + Pair.of(null, ""), Pair.of("(foo)\n", "foo")); } From 05c900c50c8dd568e1122831d10d3716d0c0e9ac Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 18 Aug 2025 16:31:24 +0200 Subject: [PATCH 7/8] cleanup --- .../java/org/opengrok/web/PageConfigTest.java | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java b/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java index b738195362e..178e6f2b11a 100644 --- a/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java +++ b/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java @@ -91,7 +91,7 @@ public static void setUpClass() throws Exception { } @AfterAll - public static void tearDownClass() throws Exception { + public static void tearDownClass() { repository.destroy(); repository = null; } @@ -242,9 +242,6 @@ 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()); @@ -352,15 +349,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); @@ -568,7 +556,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); @@ -590,7 +578,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); From c3297ab859951ed0498de27a6133894da58ae88e Mon Sep 17 00:00:00 2001 From: Vladimir Kotal Date: Mon, 18 Aug 2025 16:33:16 +0200 Subject: [PATCH 8/8] ensure list of files is non-null --- .../src/test/java/org/opengrok/web/PageConfigTest.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java b/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java index 178e6f2b11a..04760f8c0af 100644 --- a/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java +++ b/opengrok-web/src/test/java/org/opengrok/web/PageConfigTest.java @@ -34,6 +34,7 @@ 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; @@ -182,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); @@ -245,7 +246,8 @@ void testGetSortedFilesDirsFirst() throws IOException { // 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.