From ed20f2aa57a8494782e362decb48a3561936c194 Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Sun, 10 Nov 2024 08:07:48 -0500 Subject: [PATCH 1/3] [IO-856] Try test on all OSs for GitHub CI --- src/test/java/org/apache/commons/io/FileUtilsListFilesTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/apache/commons/io/FileUtilsListFilesTest.java b/src/test/java/org/apache/commons/io/FileUtilsListFilesTest.java index 20e3c8d77a1..6def0148670 100644 --- a/src/test/java/org/apache/commons/io/FileUtilsListFilesTest.java +++ b/src/test/java/org/apache/commons/io/FileUtilsListFilesTest.java @@ -238,7 +238,7 @@ public void testListFilesWithDeletion() throws IOException { * Tests IO-856 ListFiles should not fail on vanishing files. */ @Test - @EnabledOnOs(value = OS.WINDOWS) + // @EnabledOnOs(value = OS.WINDOWS) public void testListFilesWithDeletionThreaded() throws ExecutionException, InterruptedException { // test for IO-856 // create random directory in tmp, create the directory if it does not exist From 2692adf563437503c8c58d8ee6638d667349daf0 Mon Sep 17 00:00:00 2001 From: "Gary D. Gregory" Date: Sun, 19 Oct 2025 08:50:13 -0400 Subject: [PATCH 2/3] Add PathFence A Path fence guards against using paths outside of a "fence" of made of root paths. --- .../org/apache/commons/io/file/PathFence.java | 161 ++++++++++++++++++ .../apache/commons/io/file/PathFenceTest.java | 101 +++++++++++ 2 files changed, 262 insertions(+) create mode 100644 src/main/java/org/apache/commons/io/file/PathFence.java create mode 100644 src/test/java/org/apache/commons/io/file/PathFenceTest.java diff --git a/src/main/java/org/apache/commons/io/file/PathFence.java b/src/main/java/org/apache/commons/io/file/PathFence.java new file mode 100644 index 00000000000..ed032ef8e79 --- /dev/null +++ b/src/main/java/org/apache/commons/io/file/PathFence.java @@ -0,0 +1,161 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache license, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the license for the specific language governing permissions and + * limitations under the license. + */ + +package org.apache.commons.io.file; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; +import java.util.List; +import java.util.Optional; +import java.util.function.Supplier; +import java.util.stream.Collectors; + +/** + * A Path fence guards against using paths outside of a "fence" of made of root paths. + *

+ * For example, to prevent an application from using paths outside of its configuration folder: + *

+ * 
+ * Path config = Paths.get("path/to/config");
+ * PathFence fence = PathFence.builder().setRoots(config).get();
+ *
+ *

+ * You call one of the {@code apply} methods to validate paths in your application: + *

+ * 
+ * // Gets a Path or throws IllegalArgumentException
+ * Path file1 = fence.{@link #apply(String) apply}("someName");
+ * Path file2 = fence.{@link #apply(Path) apply}(somePath);
+ *
+ *

+ * You can also use multiple roots as the path fence: + *

+ * 
+ * Path globalConfig = Paths.get("path1/to/global-config");
+ * Path userConfig = Paths.get("path2/to/user-config");
+ * Path localConfig = Paths.get("path3/to/sys-config");
+ * PathFence fence = PathFence.builder().setRoots(globalConfig, userConfig, localConfig).get();
+ *
+ *

+ * To use the current directory as the path fence: + *

+ * 
+ * PathFence fence = PathFence.builder().setRoots(PathUtils.current()).get();
+ *
+ * + * @since 2.21.0 + */ +// Cannot implement both UnaryOperator and Function, so don't pick one over the other +public final class PathFence { + + /** + * Builds {@link PathFence} instances. + */ + public static final class Builder implements Supplier { + + /** The empty Path array. */ + private static final Path[] EMPTY = {}; + + /** + * A fence is made of root Paths. + */ + private Path[] roots = EMPTY; + + /** + * Constructs a new instance. + */ + private Builder() { + // empty + } + + @Override + public PathFence get() { + return new PathFence(this); + } + + /** + * Sets the paths that delineate this fence. + * + * @param roots the paths that delineate this fence. + * @return {@code this} instance. + */ + Builder setRoots(final Path... roots) { + this.roots = roots != null ? roots.clone() : EMPTY; + return this; + } + } + + /** + * Creates a new builder. + * + * @return a new builder. + */ + public static Builder builder() { + return new Builder(); + } + + /** + * A fence is made of Paths guarding Path resolution. + */ + private final List roots; + + /** + * Constructs a new instance. + * + * @param builder A builder. + */ + private PathFence(final Builder builder) { + this.roots = Arrays.stream(builder.roots).map(Path::toAbsolutePath).collect(Collectors.toList()); + } + + /** + * Checks that that a Path resolves within our fence. + * + * @param path The path to test. + * @return The given path. + * @throws IllegalArgumentException Thrown if the file name is not without our fence. + */ + // Cannot implement both UnaryOperator and Function + public Path apply(final Path path) { + return getPath(path.toString(), path); + } + + /** + * Gets a Path for the given file name, checking that it resolves within our fence. + * + * @param fileName the file name to resolve. + * @return a fenced Path. + * @throws IllegalArgumentException Thrown if the file name is not without our fence. + */ + // Cannot implement both UnaryOperator and Function + public Path apply(final String fileName) { + return getPath(fileName, Paths.get(fileName)); + } + + private Path getPath(final String fileName, final Path path) { + if (roots.isEmpty()) { + return path; + } + final Path pathAbs = path.normalize().toAbsolutePath(); + final Optional first = roots.stream().filter(pathAbs::startsWith).findFirst(); + if (first.isPresent()) { + return path; + } + throw new IllegalArgumentException(String.format("[%s] -> [%s] not in the fence %s", fileName, pathAbs, roots)); + } +} diff --git a/src/test/java/org/apache/commons/io/file/PathFenceTest.java b/src/test/java/org/apache/commons/io/file/PathFenceTest.java new file mode 100644 index 00000000000..c16044499e7 --- /dev/null +++ b/src/test/java/org/apache/commons/io/file/PathFenceTest.java @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.io.file; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** + * Tests {@link PathFence}. + */ +public class PathFenceTest { + + private Path createDirectory(final Path tempDir, final String other) throws IOException { + return Files.createDirectory(tempDir.resolve(other)); + } + + @Test + void testAbsolutePath(@TempDir final Path fenceRootPath) throws Exception { + // tempDir is the fence + final Path childTest = fenceRootPath.resolve("child/file.txt"); + final PathFence fence = PathFence.builder().setRoots(fenceRootPath).get(); + // getPath with an absolute string should be allowed + final Path childOk = fence.apply(childTest.toString()); + assertEquals(childTest.toAbsolutePath().normalize(), childOk.toAbsolutePath().normalize()); + // check with a Path instance should return the same instance when allowed + assertSame(childTest, fence.apply(childTest)); + } + + @ParameterizedTest + @ValueSource(strings = { "", ".", "some", "some/relative", "some/relative/path" }) + void testEmpty(final String test) { + // An empty fence accepts anything + final PathFence fence = PathFence.builder().get(); + final Path path = Paths.get(test); + assertEquals(path, fence.apply(test)); + assertSame(path, fence.apply(path)); + } + + @Test + void testMultipleFencePaths(@TempDir final Path tempDir) throws Exception { + // The fence is inside tempDir + final Path fenceRootPath1 = createDirectory(tempDir, "root-one"); + final Path fenceRootPath2 = createDirectory(tempDir, "root-two"); + final PathFence fence = PathFence.builder().setRoots(fenceRootPath1, fenceRootPath2).get(); + // Path under the first path should be allowed + final Path inPath1 = fenceRootPath1.resolve("a/b.txt"); + assertSame(inPath1, fence.apply(inPath1)); + // Path under the second path should be allowed + final Path inPath2 = fenceRootPath2.resolve("a/b.txt"); + assertSame(inPath2, fence.apply(inPath2)); + } + + @Test + void testNormalization(@TempDir final Path tempDir) throws Exception { + final Path fenceRootPath = createDirectory(tempDir, "root-one"); + final Path weird = fenceRootPath.resolve("subdir/../other.txt"); + final PathFence fence = PathFence.builder().setRoots(fenceRootPath).get(); + // Path contains '..' but after normalization it's still inside the base + assertSame(weird, fence.apply(weird)); + } + + @Test + void testOutsideFenceThrows(@TempDir final Path tempDir) throws Exception { + final Path fenceRootPath = createDirectory(tempDir, "root-one"); + final Path other = createDirectory(tempDir, "other"); + final PathFence fence = PathFence.builder().setRoots(fenceRootPath).get(); + final IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> fence.apply(other.toString())); + final String msg = ex.getMessage(); + assertNotNull(msg); + assertTrue(msg.contains("not in the fence"), () -> "Expected message to mention fence: " + msg); + assertTrue(msg.contains(other.toAbsolutePath().toString()), () -> "Expected message to contain the path: " + msg); + } +} From e4590716a696f62869b6df1bfe34a5354ace0342 Mon Sep 17 00:00:00 2001 From: "Gary D. Gregory" Date: Tue, 21 Oct 2025 08:26:13 -0400 Subject: [PATCH 3/3] Address relative path issues --- .../org/apache/commons/io/file/PathFence.java | 26 ++++++--- .../apache/commons/io/file/PathFenceTest.java | 57 ++++++++++++++++--- 2 files changed, 68 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/apache/commons/io/file/PathFence.java b/src/main/java/org/apache/commons/io/file/PathFence.java index ed032ef8e79..2a0554fee14 100644 --- a/src/main/java/org/apache/commons/io/file/PathFence.java +++ b/src/main/java/org/apache/commons/io/file/PathFence.java @@ -120,15 +120,25 @@ public static Builder builder() { * @param builder A builder. */ private PathFence(final Builder builder) { - this.roots = Arrays.stream(builder.roots).map(Path::toAbsolutePath).collect(Collectors.toList()); + this.roots = Arrays.stream(builder.roots).map(this::absoluteNormalize).collect(Collectors.toList()); } /** - * Checks that that a Path resolves within our fence. + * Converts the given path to a normalized absolute path. + * + * @param path The source path. + * @return The result path. + */ + private Path absoluteNormalize(final Path path) { + return path.toAbsolutePath().normalize(); + } + + /** + * Checks that that a Path is within our fence. * * @param path The path to test. * @return The given path. - * @throws IllegalArgumentException Thrown if the file name is not without our fence. + * @throws IllegalArgumentException Thrown if the path is not within our fence. */ // Cannot implement both UnaryOperator and Function public Path apply(final Path path) { @@ -136,11 +146,11 @@ public Path apply(final Path path) { } /** - * Gets a Path for the given file name, checking that it resolves within our fence. + * Gets a Path for the given file name, checking that it is within our fence. * - * @param fileName the file name to resolve. - * @return a fenced Path. - * @throws IllegalArgumentException Thrown if the file name is not without our fence. + * @param fileName the file name to test. + * @return The given path. + * @throws IllegalArgumentException Thrown if the file name is not within our fence. */ // Cannot implement both UnaryOperator and Function public Path apply(final String fileName) { @@ -151,7 +161,7 @@ private Path getPath(final String fileName, final Path path) { if (roots.isEmpty()) { return path; } - final Path pathAbs = path.normalize().toAbsolutePath(); + final Path pathAbs = absoluteNormalize(path); final Optional first = roots.stream().filter(pathAbs::startsWith).findFirst(); if (first.isPresent()) { return path; diff --git a/src/test/java/org/apache/commons/io/file/PathFenceTest.java b/src/test/java/org/apache/commons/io/file/PathFenceTest.java index c16044499e7..0ca0eef0a67 100644 --- a/src/test/java/org/apache/commons/io/file/PathFenceTest.java +++ b/src/test/java/org/apache/commons/io/file/PathFenceTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -28,6 +29,7 @@ import java.nio.file.Path; import java.nio.file.Paths; +import org.apache.commons.lang3.StringUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; @@ -42,16 +44,29 @@ private Path createDirectory(final Path tempDir, final String other) throws IOEx return Files.createDirectory(tempDir.resolve(other)); } + private Path getRelPathToTop() { + final Path startPath = PathUtils.current().toAbsolutePath(); + final Path parent = startPath; + final int nameCount = parent.getNameCount(); + final String relName = StringUtils.repeat("../", nameCount); + final Path relPath = Paths.get(relName); + // sanity checks + final Path rootPath = relPath.toAbsolutePath().normalize(); + assertNull(rootPath.getFileName()); + assertEquals(startPath.getRoot(), rootPath); + return relPath; + } + @Test void testAbsolutePath(@TempDir final Path fenceRootPath) throws Exception { // tempDir is the fence - final Path childTest = fenceRootPath.resolve("child/file.txt"); + final Path resolved = fenceRootPath.resolve("child/file.txt"); final PathFence fence = PathFence.builder().setRoots(fenceRootPath).get(); // getPath with an absolute string should be allowed - final Path childOk = fence.apply(childTest.toString()); - assertEquals(childTest.toAbsolutePath().normalize(), childOk.toAbsolutePath().normalize()); + final Path childOk = fence.apply(resolved.toString()); + assertEquals(resolved.toAbsolutePath().normalize(), childOk.toAbsolutePath().normalize()); // check with a Path instance should return the same instance when allowed - assertSame(childTest, fence.apply(childTest)); + assertSame(resolved, fence.apply(resolved)); } @ParameterizedTest @@ -64,6 +79,18 @@ void testEmpty(final String test) { assertSame(path, fence.apply(path)); } + @ParameterizedTest + @ValueSource(strings = { "/a/b", "/a/b/c", "/a/b/c/d", "a", "a/b", "a/b/c", "a/b/c/d" }) + public void testEscapeAttempt(final Path fenceRootPath) { + final Path resolved = fenceRootPath.resolve("../../etc/passwd"); + final Path relative = Paths.get("../../etc/passwd"); + final PathFence fence = PathFence.builder().setRoots(fenceRootPath).get(); + assertThrows(IllegalArgumentException.class, () -> fence.apply(resolved)); + assertThrows(IllegalArgumentException.class, () -> fence.apply(relative)); + assertThrows(IllegalArgumentException.class, () -> fence.apply(resolved.toString())); + assertThrows(IllegalArgumentException.class, () -> fence.apply(relative.toString())); + } + @Test void testMultipleFencePaths(@TempDir final Path tempDir) throws Exception { // The fence is inside tempDir @@ -81,10 +108,9 @@ void testMultipleFencePaths(@TempDir final Path tempDir) throws Exception { @Test void testNormalization(@TempDir final Path tempDir) throws Exception { final Path fenceRootPath = createDirectory(tempDir, "root-one"); - final Path weird = fenceRootPath.resolve("subdir/../other.txt"); + final Path resolved = fenceRootPath.resolve("subdir/../other.txt"); final PathFence fence = PathFence.builder().setRoots(fenceRootPath).get(); - // Path contains '..' but after normalization it's still inside the base - assertSame(weird, fence.apply(weird)); + assertSame(resolved, fence.apply(resolved)); } @Test @@ -98,4 +124,21 @@ void testOutsideFenceThrows(@TempDir final Path tempDir) throws Exception { assertTrue(msg.contains("not in the fence"), () -> "Expected message to mention fence: " + msg); assertTrue(msg.contains(other.toAbsolutePath().toString()), () -> "Expected message to contain the path: " + msg); } + + @Test + void testResolveRelative() throws Exception { + final PathFence fence = PathFence.builder().setRoots(Paths.get("/foo/bar")).get(); + final Path relPathTop = getRelPathToTop(); + final Path relPath = relPathTop.resolve("foo/bar"); + assertSame(relPath, fence.apply(relPath)); + } + + @Test + void testResolveRelativeRoot() throws Exception { + final Path relPathTop = getRelPathToTop(); + final PathFence fence = PathFence.builder().setRoots(relPathTop.resolve("foo/bar")).get(); + final Path relPath = relPathTop.resolve("foo/bar"); + assertSame(relPath, fence.apply(relPath)); + } + }