Address relative path issues

Author: garydgregory

src/main/java/org/apache/commons/io/file/PathFence.java

@@ -120,27 +120,37 @@ 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<Path> and Function<String, Path>
     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.
+     * 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<Path> and Function<String, Path>
     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<Path> first = roots.stream().filter(pathAbs::startsWith).findFirst();
         if (first.isPresent()) {
             return path;

src/test/java/org/apache/commons/io/file/PathFenceTest.java

@@ -28,6 +28,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 +43,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();
+        assertEquals("/", rootPath.toString());
+        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 +78,16 @@ void testEmpty(final String test) {
         assertSame(path, fence.apply(path));
     }
 
+    private 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 +105,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 +121,27 @@ 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));
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "/a", "/a/b", "/a/b/c", "/a/b/c/d", "a", "a/b", "a/b/c", "a/b/c/d" })
+    void testUserHomeDirEscapeAttempt(final String path) throws Exception {
+        testEscapeAttempt(Paths.get(path));
+    }
+
 }