Skip to content

Commit

Permalink
Forbid using '..' in paths passed to DirectoryPathTree
Browse files Browse the repository at this point in the history
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 quarkusio#23692 (comment)

(cherry picked from commit b33f815)
  • Loading branch information
yrodiere authored and gsmet committed Feb 28, 2022
1 parent 523f396 commit 955c621
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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> T apply(String relativePath, Function<PathVisit, T> func, boolean manifestEnabled) {
relativePath = normalize(relativePath);
ensureResourcePath(relativePath);
if (!PathFilter.isVisible(pathFilter, relativePath)) {
return func.apply(null);
}
Expand All @@ -106,7 +105,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;
Expand All @@ -121,7 +120,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;
}
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 955c621

Please sign in to comment.