From 955c6210cd86709da94c8e20b7ea75487bbd1ec4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yoann=20Rodi=C3=A8re?= Date: Tue, 22 Feb 2022 09:46:54 +0100 Subject: [PATCH] Forbid using '..' in paths passed to DirectoryPathTree Because we want the paths to be interpreted in a way that's as similar to ClassLoader#getResource as possible. With the previous behavior, if I pass foo/bar/.. to DirectoryPathTree, it will automatically be resolved to foo/. So in a build step for example, we might end up accepting the path as valid. Later at runtime, we might pass the same string (foo/bar/..) to ClassLoader#getRessource. And then it will fail, because .. has no special meaning for ClassLoader#getResource... See https://github.com/quarkusio/quarkus/pull/23692#discussion_r807717452 (cherry picked from commit b33f815d57687ce776d2ea08493ff8699a74ce5b) --- .../io/quarkus/paths/DirectoryPathTree.java | 19 ++++---- .../quarkus/paths/DirectoryPathTreeTest.java | 44 ++++++++++++------- 2 files changed, 38 insertions(+), 25 deletions(-) diff --git a/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java b/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java index 6800d3a6da856..8d87c2ccef900 100644 --- a/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java +++ b/independent-projects/bootstrap/app-model/src/main/java/io/quarkus/paths/DirectoryPathTree.java @@ -78,22 +78,21 @@ public void walk(PathVisitor visitor) { PathTreeVisit.walk(dir, dir, pathFilter, getMultiReleaseMapping(), visitor); } - private String normalize(String path) { + private void ensureResourcePath(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(); + for (Path pathElement : dir.getFileSystem().getPath(path)) { + if (pathElement.toString().equals("..")) { + throw new IllegalArgumentException("'..' cannot be used in resource paths, but got " + path); + } } - return null; } - return path; } @Override protected T apply(String relativePath, Function func, boolean manifestEnabled) { - relativePath = normalize(relativePath); + ensureResourcePath(relativePath); if (!PathFilter.isVisible(pathFilter, relativePath)) { return func.apply(null); } @@ -106,7 +105,7 @@ protected T apply(String relativePath, Function func, boolean @Override public void accept(String relativePath, Consumer consumer) { - relativePath = normalize(relativePath); + ensureResourcePath(relativePath); if (!PathFilter.isVisible(pathFilter, relativePath)) { consumer.accept(null); return; @@ -121,7 +120,7 @@ public void accept(String relativePath, Consumer consumer) { @Override public boolean contains(String relativePath) { - relativePath = normalize(relativePath); + ensureResourcePath(relativePath); if (!PathFilter.isVisible(pathFilter, relativePath)) { return false; } @@ -131,7 +130,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/independent-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/DirectoryPathTreeTest.java b/independent-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/DirectoryPathTreeTest.java index e9d252def04a9..59c70a39bee7b 100644 --- a/independent-projects/bootstrap/app-model/src/test/java/io/quarkus/paths/DirectoryPathTreeTest.java +++ b/independent-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