Merge branch 'entitlements/fix-fileaccesstree-ordering' of github.com:ldematte/elasticsearch into entitlements/fix-fileaccesstree-ordering

Author: ldematte

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileUtils.java

@@ -50,23 +50,18 @@ private FileUtils() {}
         return len1 - len2;
     };
 
-    /**
-     * Java allows to use both forward and backslash on Windows systems as path separators;
-     * On posix filesystems however '\' is a valid character for file or directory names, so we stick to just the
-     * standard platform separator for anything that is not Windows.
-     */
     @SuppressForbidden(reason = "we need the separator as a char, not a string")
     private static boolean isPathSeparator(char c) {
-        if (Platform.WINDOWS.isCurrent()) {
-            return isSlash(c);
-        }
         return c == File.separatorChar;
     }
 
     /**
      * Tests if a path is absolute or relative, taking into consideration both Unix and Windows conventions.
      * Note that this leads to a conflict, resolved in favor of Unix rules: `/foo` can be either a Unix absolute path, or a Windows
-     * relative path with "wrong" directory separator (using non-canonical slash in Windows).
+     * relative path with "wrong" directory separator (using non-canonical / in Windows).
+     * This method is intended to be used as validation for different file entitlements format: therefore it is preferable to reject a
+     * relative path that is definitely absolute on Unix, rather than accept it as a possible relative path on Windows (if that is the case,
+     * the developer can easily fix the path by using the correct platform separators).
      */
     public static boolean isAbsolutePath(String path) {
         if (path.isEmpty()) {
@@ -80,6 +75,10 @@ public static boolean isAbsolutePath(String path) {
         return isWindowsAbsolutePath(path);
     }
 
+    /**
+     * When testing for path separators in a platform-agnostic way, we may encounter both kinds of slashes, especially when
+     * processing windows paths. The JDK parses paths the same way under Windows.
+     */
     static boolean isSlash(char c) {
         return (c == '\\') || (c == '/');
     }

libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java

@@ -26,6 +26,7 @@
 import java.util.Map;
 
 import static org.elasticsearch.core.PathUtils.getDefaultFileSystem;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 
 @ESTestCase.WithoutSecurityManager
@@ -193,6 +194,48 @@ public void testNormalizePath() {
         assertThat(tree.canRead(path("")), is(false));
     }
 
+    public void testNormalizeDirectorySeparatorWindows() {
+        assumeTrue("normalization of windows paths", Platform.WINDOWS.isCurrent());
+
+        assertThat(FileAccessTree.normalizePath(Path.of("C:\\a\\b")), equalTo("C:\\a\\b"));
+        assertThat(FileAccessTree.normalizePath(Path.of("C:/a.xml")), equalTo("C:\\a.xml"));
+        assertThat(FileAccessTree.normalizePath(Path.of("C:/a/b.txt")), equalTo("C:\\a\\b.txt"));
+        assertThat(FileAccessTree.normalizePath(Path.of("C:/a/c\\foo.txt")), equalTo("C:\\a\\c\\foo.txt"));
+
+        var tree = accessTree(
+            entitlement("C:\\a\\b", "read", "C:/a.xml", "read", "C:/a/b.txt", "read", "C:/a/c\\foo.txt", "read"),
+            List.of()
+        );
+
+        assertThat(tree.canRead(Path.of("C:/a.xml")), is(true));
+        assertThat(tree.canRead(Path.of("C:\\a.xml")), is(true));
+        assertThat(tree.canRead(Path.of("C:/a/")), is(false));
+        assertThat(tree.canRead(Path.of("C:/a/b.txt")), is(true));
+        assertThat(tree.canRead(Path.of("C:/a/b/c.txt")), is(true));
+        assertThat(tree.canRead(Path.of("C:\\a\\b\\c.txt")), is(true));
+        assertThat(tree.canRead(Path.of("C:\\a\\c\\")), is(false));
+        assertThat(tree.canRead(Path.of("C:\\a\\c\\foo.txt")), is(true));
+    }
+
+    public void testNormalizeDirectorySeparatorPosix() {
+        assumeFalse("normalization of posix paths", Platform.WINDOWS.isCurrent());
+
+        assertThat(FileAccessTree.normalizePath(Path.of("\\a\\b")), equalTo("/a/b"));
+        assertThat(FileAccessTree.normalizePath(Path.of("/a.xml")), equalTo("/a.xml"));
+        assertThat(FileAccessTree.normalizePath(Path.of("/a/c\\foo.txt")), equalTo("/a/c/foo.txt"));
+
+        var tree = accessTree(entitlement("\\a\\b", "read", "/a.xml", "read", "/a/b.txt", "read", "/a/c\\foo.txt", "read"), List.of());
+
+        assertThat(tree.canRead(Path.of("/a.xml")), is(true));
+        assertThat(tree.canRead(Path.of("\\a.xml")), is(true));
+        assertThat(tree.canRead(Path.of("/a/")), is(false));
+        assertThat(tree.canRead(Path.of("/a/b.txt")), is(true));
+        assertThat(tree.canRead(Path.of("/a/b/c.txt")), is(true));
+        assertThat(tree.canRead(Path.of("\\a\\b\\c.txt")), is(true));
+        assertThat(tree.canRead(Path.of("\\a\\c\\")), is(false));
+        assertThat(tree.canRead(Path.of("\\a\\c\\foo.txt")), is(true));
+    }
+
     public void testNormalizeTrailingSlashes() {
         var tree = accessTree(entitlement("/trailing/slash/", "read", "/no/trailing/slash", "read"), List.of());
         assertThat(tree.canRead(path("/trailing/slash")), is(true));

libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileUtilsTests.java

@@ -9,6 +9,7 @@
 
 package org.elasticsearch.entitlement.runtime.policy;
 
+import org.elasticsearch.core.PathUtils;
 import org.elasticsearch.test.ESTestCase;
 
 import static org.elasticsearch.entitlement.runtime.policy.FileUtils.PATH_ORDER;
@@ -42,7 +43,9 @@ public void testPathIsAbsolute() {
         assertThat(isAbsolutePath(""), is(false));
     }
 
-    public void testPathOrder() {
+    public void testPathOrderPosix() {
+        assumeFalse("path ordering rules specific to non-Windows path styles", Platform.WINDOWS.isCurrent());
+
         // Unix-style
         // Directories come BEFORE files; note that this differs from natural lexicographical order
         assertThat(PATH_ORDER.compare("/a/b", "/a.xml"), lessThan(0));
@@ -60,27 +63,30 @@ public void testPathOrder() {
         assertThat(PATH_ORDER.compare("C:/a/b", "C:/a/b.txt"), lessThan(0));
         assertThat(PATH_ORDER.compare("C:/a/c", "C:/a/b.txt"), greaterThan(0));
         assertThat(PATH_ORDER.compare("C:/a/b", "C:/a/b/foo.txt"), lessThan(0));
-    }
-
-    public void testPathOrderPosix() {
-        assumeFalse("path ordering rules specific to non-Windows platforms", Platform.WINDOWS.isCurrent());
 
+        // "\" is a valid file name character on Posix, test we treat it like that
         assertThat(PATH_ORDER.compare("/a\\b", "/a/b.txt"), greaterThan(0));
     }
 
     public void testPathOrderWindows() {
         assumeTrue("path ordering rules specific to Windows", Platform.WINDOWS.isCurrent());
 
-        // Windows-style
+        // Directories come BEFORE files; note that this differs from natural lexicographical order
         assertThat(PATH_ORDER.compare("C:\\a\\b", "C:\\a.xml"), lessThan(0));
+
+        // Natural lexicographical order is respected in all the other cases
         assertThat(PATH_ORDER.compare("C:\\a\\b", "C:\\a\\b.txt"), lessThan(0));
         assertThat(PATH_ORDER.compare("C:\\a\\b", "C:\\a\\b\\foo.txt"), lessThan(0));
         assertThat(PATH_ORDER.compare("C:\\a\\c", "C:\\a\\b.txt"), greaterThan(0));
+    }
+
+    public void testPathOrderingSpecialCharacters() {
+        assertThat(PATH_ORDER.compare("aa\uD801\uDC28", "aa\uD801\uDC28"), is(0));
+        assertThat(PATH_ORDER.compare("aa\uD801\uDC28", "aa\uD801\uDC28a"), lessThan(0));
 
-        // Mixed-style
-        assertThat(PATH_ORDER.compare("C:\\a\\b", "C:/a.xml"), lessThan(0));
-        assertThat(PATH_ORDER.compare("C:\\a\\b", "C:/a/b.txt"), lessThan(0));
-        assertThat(PATH_ORDER.compare("C:\\a\\b", "C:/a/b\\foo.txt"), lessThan(0));
-        assertThat(PATH_ORDER.compare("C:/a\\b", "C:\\a\\b.txt"), lessThan(0));
+        var s = PathUtils.getDefaultFileSystem().getSeparator();
+        // Similarly to the other tests, we assert that Directories come BEFORE files, even when names are special characters
+        assertThat(PATH_ORDER.compare(s + "\uD801\uDC28" + s + "b", s + "\uD801\uDC28.xml"), lessThan(0));
+        assertThat(PATH_ORDER.compare(s + "\uD801\uDC28" + s + "b", s + "b.xml"), greaterThan(0));
     }
 }