Also validate canonicalRoot when deserializing Configuration

Author: idodeclare

opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/CanonicalRootValidator.java

@@ -0,0 +1,66 @@
+/*
+ * CDDL HEADER START
+ *
+ * The contents of this file are subject to the terms of the
+ * Common Development and Distribution License (the "License").
+ * You may not use this file except in compliance with the License.
+ *
+ * See LICENSE.txt included in this distribution for the specific
+ * language governing permissions and limitations under the License.
+ *
+ * When distributing Covered Code, include this CDDL HEADER in each
+ * file and include the License file at LICENSE.txt.
+ * If applicable, add the following below this CDDL HEADER, with the
+ * fields enclosed by brackets "[]" replaced with your own identifying
+ * information: Portions Copyright [yyyy] [name of copyright owner]
+ *
+ * CDDL HEADER END
+ */
+
+/*
+ * Copyright (c) 2019, Chris Fraire <cfraire@me.com>.
+ */
+
+package org.opengrok.indexer.configuration;
+
+import java.io.File;
+
+/**
+ * Represents a validator of a --canonicalRoot value.
+ */
+public class CanonicalRootValidator {
+
+    /**
+     * Validates the specified setting, returning an error message if validation
+     * fails.
+     * @return a defined instance if not valid or {@code null} otherwise
+     */
+    public static String validate(String setting, String elementDescription) {
+        // Test that the value ends with the system separator.
+        if (!setting.endsWith(File.separator)) {
+            return elementDescription + " must end with a separator";
+        }
+        // Test that the value is not like a Windows root (e.g. C:\ or Z:/).
+        if (setting.matches("\\w:(?:[/\\\\]*)")) {
+            return elementDescription + " cannot be a root directory";
+        }
+        // Test that some character other than separators is in the value.
+        if (!setting.matches(".*[^/\\\\].*")) {
+            return elementDescription + " cannot be the root directory";
+        }
+
+        /*
+         * There is no need to validate that the caller has not specified e.g.
+         * "/./" (i.e. an alias to the root "/") because --canonicalRoot values
+         * are matched via string comparison against true canonical values got
+         * via File.getCanonicalPath(). An alias like "/./" is therefore a
+         * never-matching value and hence inactive.
+         */
+        return null;
+    }
+
+    /** Private to enforce static. */
+    private CanonicalRootValidator() {
+
+    }
+}

opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/Configuration.java

@@ -53,6 +53,8 @@
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
 import java.util.regex.PatternSyntaxException;
+import java.util.stream.Collectors;
+
 import org.opengrok.indexer.authorization.AuthControlFlag;
 import org.opengrok.indexer.authorization.AuthorizationStack;
 import org.opengrok.indexer.history.RepositoryInfo;
@@ -1359,6 +1361,22 @@ public boolean test(Group g) {
         }
         conf.setGroups(copy);
 
+        /*
+         * Validate any defined canonicalRoot entries, and only include where
+         * validation succeeds.
+         */
+        if (conf.canonicalRoots != null) {
+            conf.canonicalRoots = conf.canonicalRoots.stream().filter(s -> {
+                String problem = CanonicalRootValidator.validate(s, "canonicalRoot element");
+                if (problem == null) {
+                    return true;
+                } else {
+                    LOGGER.warning(problem);
+                    return false;
+                }
+            }).collect(Collectors.toCollection(HashSet::new));
+        }
+
         return conf;
     }
 }

opengrok-indexer/src/main/java/org/opengrok/indexer/index/Indexer.java

@@ -53,6 +53,7 @@
 import org.opengrok.indexer.analysis.AnalyzerGuru;
 import org.opengrok.indexer.analysis.AnalyzerGuruHelp;
 import org.opengrok.indexer.analysis.Ctags;
+import org.opengrok.indexer.configuration.CanonicalRootValidator;
 import org.opengrok.indexer.configuration.Configuration;
 import org.opengrok.indexer.configuration.ConfigurationHelp;
 import org.opengrok.indexer.configuration.LuceneLockName;
@@ -494,11 +495,9 @@ public static String[] parseOptions(String[] argv) throws ParseException {
                     "canonical root must end with a file separator. For security, a canonical",
                     "root cannot be the root directory. Option may be repeated.").Do(v -> {
                 String root = (String) v;
-                if (!root.endsWith("/") && !root.endsWith("\\")) {
-                    die("--canonicalRoot must end with a separator");
-                }
-                if (root.equals("/") || root.equals("\\")) {
-                    die("--canonicalRoot cannot be the root directory");
+                String problem = CanonicalRootValidator.validate(root, "--canonicalRoot");
+                if (problem != null) {
+                    die(problem);
                 }
                 canonicalRoots.add(root);
             });

opengrok-indexer/src/test/java/org/opengrok/indexer/configuration/CanonicalRootValidatorTest.java

@@ -0,0 +1,59 @@
+/*
+ * CDDL HEADER START
+ *
+ * The contents of this file are subject to the terms of the
+ * Common Development and Distribution License (the "License").
+ * You may not use this file except in compliance with the License.
+ *
+ * See LICENSE.txt included in this distribution for the specific
+ * language governing permissions and limitations under the License.
+ *
+ * When distributing Covered Code, include this CDDL HEADER in each
+ * file and include the License file at LICENSE.txt.
+ * If applicable, add the following below this CDDL HEADER, with the
+ * fields enclosed by brackets "[]" replaced with your own identifying
+ * information: Portions Copyright [yyyy] [name of copyright owner]
+ *
+ * CDDL HEADER END
+ */
+
+/*
+ * Copyright (c) 2019, Chris Fraire <cfraire@me.com>.
+ */
+
+package org.opengrok.indexer.configuration;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+import org.junit.Test;
+
+import java.io.File;
+
+public class CanonicalRootValidatorTest {
+
+    @Test
+    public void testRejectUnseparated() {
+        assertEquals("test value must end with a separator",
+                CanonicalRootValidator.validate("test", "test value"));
+    }
+
+    @Test
+    public void testRejectRoot() {
+        assertEquals("should reject root", "test value cannot be the root directory",
+                CanonicalRootValidator.validate("/", "test value"));
+    }
+
+    @Test
+    public void testRejectWindowsRoot() {
+        assertEquals("should reject Windows root", "--canonicalRoot cannot be a root directory",
+                CanonicalRootValidator.validate("C:" + File.separator, "--canonicalRoot"));
+    }
+
+    @Test
+    public void testSlashVar() {
+        assertNull("should allow /var/",
+                CanonicalRootValidator.validate(File.separator + "var" + File.separator,
+                        "--canonicalRoot"));
+    }
+}