Merge pull request #23862 from yrodiere/pathtree-forbid-dotdot

Forbid using '..' in paths passed to PathTree
quarkusio · Feb 22, 2022 · 2239a38 · 2239a38
2 parents 95cc838 + f654089
commit 2239a38
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 34 deletions.
diff --git a/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/ArchivePathTree.java b/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/ArchivePathTree.java
@@ -49,9 +49,13 @@ public void walk(PathVisitor visitor) {
         }
     }
 
+    private void ensureResourcePath(String path) {
+        DirectoryPathTree.ensureResourcePath(archive.getFileSystem(), path);
+    }
+
     @Override
     protected <T> T apply(String relativePath, Function<PathVisit, T> func, boolean manifestEnabled) {
-        DirectoryPathTree.ensureRelativePath(relativePath);
+        ensureResourcePath(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return func.apply(null);
         }
@@ -74,7 +78,7 @@ protected <T> T apply(String relativePath, Function<PathVisit, T> func, boolean
 
     @Override
     public void accept(String relativePath, Consumer<PathVisit> consumer) {
-        DirectoryPathTree.ensureRelativePath(relativePath);
+        ensureResourcePath(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             consumer.accept(null);
             return;
@@ -99,7 +103,7 @@ public void accept(String relativePath, Consumer<PathVisit> consumer) {
 
     @Override
     public boolean contains(String relativePath) {
-        DirectoryPathTree.ensureRelativePath(relativePath);
+        ensureResourcePath(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return false;
         }

diff --git a/...endent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java b/...endent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java
@@ -2,6 +2,7 @@
 
 import java.io.IOException;
 import java.io.Serializable;
+import java.nio.file.FileSystem;
 import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -34,10 +35,18 @@ static boolean isAbsolutePath(String path) {
                         || path.startsWith(FileSystems.getDefault().getSeparator()));
     }
 
-    static void ensureRelativePath(String path) {
+    static void ensureResourcePath(FileSystem fs, String path) {
         if (isAbsolutePath(path)) {
             throw new IllegalArgumentException("Expected a path relative to the root of the path tree but got " + path);
         }
+        // this is to disallow reading outside the path tree root
+        if (path != null && path.contains("..")) {
+            for (Path pathElement : fs.getPath(path)) {
+                if (pathElement.toString().equals("..")) {
+                    throw new IllegalArgumentException("'..' cannot be used in resource paths, but got " + path);
+                }
+            }
+        }
     }
 
     private Path dir;
@@ -80,22 +89,13 @@ public void walk(PathVisitor visitor) {
         PathTreeVisit.walk(dir, dir, pathFilter, getMultiReleaseMapping(), visitor);
     }
 
-    private String normalize(String path) {
-        ensureRelativePath(path);
-        // this is to disallow reading outside the path tree root
-        if (path != null && path.contains("..")) {
-            final Path absolutePath = dir.resolve(path).normalize().toAbsolutePath();
-            if (absolutePath.startsWith(dir)) {
-                return dir.relativize(absolutePath).toString();
-            }
-            return null;
-        }
-        return path;
+    private void ensureResourcePath(String path) {
+        ensureResourcePath(dir.getFileSystem(), path);
     }
 
     @Override
     protected <T> T apply(String relativePath, Function<PathVisit, T> func, boolean manifestEnabled) {
-        relativePath = normalize(relativePath);
+        ensureResourcePath(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return func.apply(null);
         }
@@ -108,7 +108,7 @@ protected <T> T apply(String relativePath, Function<PathVisit, T> func, boolean
 
     @Override
     public void accept(String relativePath, Consumer<PathVisit> consumer) {
-        relativePath = normalize(relativePath);
+        ensureResourcePath(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             consumer.accept(null);
             return;
@@ -123,7 +123,7 @@ public void accept(String relativePath, Consumer<PathVisit> consumer) {
 
     @Override
     public boolean contains(String relativePath) {
-        relativePath = normalize(relativePath);
+        ensureResourcePath(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return false;
         }
@@ -133,7 +133,7 @@ public boolean contains(String relativePath) {
 
     @Override
     public Path getPath(String relativePath) {
-        relativePath = normalize(relativePath);
+        ensureResourcePath(relativePath);
         if (!PathFilter.isVisible(pathFilter, relativePath)) {
             return null;
         }

diff --git a/...nt-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/DirectoryPathTreeTest.java b/...nt-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/DirectoryPathTreeTest.java
@@ -1,6 +1,7 @@
 package io.quarkus.paths;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.jupiter.api.Assertions.fail;
 
 import java.io.IOException;
@@ -89,26 +90,39 @@ public void acceptIllegalAbsolutePathOutsideTree() throws Exception {
     public void acceptExistingRelativeNonNormalizedPath() throws Exception {
         final Path root = resolveTreeRoot("root");
         final PathTree tree = PathTree.ofDirectoryOrArchive(root);
-        tree.accept("../root/./other/../README.md", visit -> {
-            assertThat(visit).isNotNull();
-            assertThat(visit.getRelativePath("/")).isEqualTo("README.md");
-            assertThat(visit.getPath()).exists();
-            assertThat(visit.getRoot()).isEqualTo(root);
-            try {
-                assertThat(Files.readString(visit.getPath())).isEqualTo("test readme");
-            } catch (IOException e) {
-                throw new UncheckedIOException(e);
-            }
-        });
+        assertThatThrownBy(() -> {
+            tree.accept("other/../README.md", visit -> {
+                fail("'..' should lead to an exception that prevents the visitor from being called");
+            });
+        })
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("'..' cannot be used in resource paths");
     }
 
     @Test
-    public void acceptNonExistentRelativeNonNormalizedPath() throws Exception {
+    public void acceptExistingRelativeNonNormalizedPathOutsideTree() throws Exception {
         final Path root = resolveTreeRoot("root");
         final PathTree tree = PathTree.ofDirectoryOrArchive(root);
-        tree.accept("../root/./README.md/../non-existent.txt", visit -> {
-            assertThat(visit).isNull();
-        });
+        assertThatThrownBy(() -> {
+            tree.accept("../root/./other/../README.md", visit -> {
+                fail("'..' should lead to an exception that prevents the visitor from being called");
+            });
+        })
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("'..' cannot be used in resource paths");
+    }
+
+    @Test
+    public void acceptNonExistentRelativeNonNormalizedPathOutsideTree() throws Exception {
+        final Path root = resolveTreeRoot("root");
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        assertThatThrownBy(() -> {
+            tree.accept("../root/./README.md/../non-existent.txt", visit -> {
+                fail("'..' should lead to an exception that prevents the visitor from being called");
+            });
+        })
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("'..' cannot be used in resource paths");
     }
 
     @Test

diff --git a/independent-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/JarPathTreeTest.java b/independent-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/JarPathTreeTest.java
@@ -1,6 +1,7 @@
 package io.quarkus.paths;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.jupiter.api.Assertions.fail;
 
 import io.quarkus.fs.util.ZipUtils;
@@ -85,6 +86,42 @@ public void acceptAbsolutePathOutsideTree() throws Exception {
         }
     }
 
+    @Test
+    public void acceptExistingRelativeNonNormalizedPath() throws Exception {
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        assertThatThrownBy(() -> {
+            tree.accept("other/../README.md", visit -> {
+                fail("'..' should lead to an exception that prevents the visitor from being called");
+            });
+        })
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("'..' cannot be used in resource paths");
+    }
+
+    @Test
+    public void acceptExistingRelativeNonNormalizedPathOutsideTree() throws Exception {
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        assertThatThrownBy(() -> {
+            tree.accept("../root/./other/../README.md", visit -> {
+                fail("'..' should lead to an exception that prevents the visitor from being called");
+            });
+        })
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("'..' cannot be used in resource paths");
+    }
+
+    @Test
+    public void acceptNonExistentRelativeNonNormalizedPathOutsideTree() throws Exception {
+        final PathTree tree = PathTree.ofDirectoryOrArchive(root);
+        assertThatThrownBy(() -> {
+            tree.accept("../root/./README.md/../non-existent.txt", visit -> {
+                fail("'..' should lead to an exception that prevents the visitor from being called");
+            });
+        })
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("'..' cannot be used in resource paths");
+    }
+
     @Test
     public void walk() throws Exception {
         final PathTree tree = PathTree.ofDirectoryOrArchive(root);