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); } } 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/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 { diff --git a/opengrok-web/src/main/webapp/xref.jspf b/opengrok-web/src/main/webapp/xref.jspf index ef626679d5c..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 . --%> @@ -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) { 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..04760f8c0af 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; @@ -33,14 +34,19 @@ 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> 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 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);