ecki commented on code in PR #805: URL: https://github.com/apache/commons-io/pull/805#discussion_r3537746225
########## src/main/java/org/apache/commons/io/file/PathFence.java: ########## @@ -0,0 +1,171 @@ +/* + * 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. + * <p> + * For example, to prevent an application from using paths outside of its configuration folder: + * </p> + * <pre> + * Path config = Paths.get("path/to/config"); + * PathFence fence = PathFence.builder().setRoots(config).get(); + * </pre> + * <p> + * You call one of the {@code apply} methods to validate paths in your application: + * </p> + * <pre> + * // Gets a Path or throws IllegalArgumentException + * Path file1 = fence.{@link #apply(String) apply}("someName"); + * Path file2 = fence.{@link #apply(Path) apply}(somePath); + * </pre> + * <p> + * You can also use multiple roots as the path fence: + * </p> + * <pre> + * 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(); + * </pre> + * <p> + * To use the current directory as the path fence: + * </p> + * <pre> + * PathFence fence = PathFence.builder().setRoots(PathUtils.current()).get(); + * </pre> + * + * @since 2.21.0 + */ +// Cannot implement both UnaryOperator<Path> and Function<String, Path>, so don't pick one over the other +public final class PathFence { + + /** + * Builds {@link PathFence} instances. + */ + public static final class Builder implements Supplier<PathFence> { + + /** 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<Path> roots; + + /** + * Constructs a new instance. + * + * @param builder A builder. + */ + private PathFence(final Builder builder) { + this.roots = Arrays.stream(builder.roots).map(this::absoluteNormalize).collect(Collectors.toList()); + } + + /** + * 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 path is not within our fence. + */ + // Cannot implement both UnaryOperator<Path> and Function<String, Path> + public Path apply(final Path path) { Review Comment: I find apply() a bit non descriptive, and why does it return a Path instead of a boolean? Not sure if IAE should be thrown as a result of a policy check. ########## src/main/java/org/apache/commons/io/file/PathFence.java: ########## @@ -0,0 +1,171 @@ +/* + * 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. + * <p> + * For example, to prevent an application from using paths outside of its configuration folder: + * </p> + * <pre> + * Path config = Paths.get("path/to/config"); + * PathFence fence = PathFence.builder().setRoots(config).get(); + * </pre> + * <p> + * You call one of the {@code apply} methods to validate paths in your application: + * </p> + * <pre> + * // Gets a Path or throws IllegalArgumentException + * Path file1 = fence.{@link #apply(String) apply}("someName"); + * Path file2 = fence.{@link #apply(Path) apply}(somePath); + * </pre> + * <p> + * You can also use multiple roots as the path fence: + * </p> + * <pre> + * 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(); + * </pre> + * <p> + * To use the current directory as the path fence: + * </p> + * <pre> + * PathFence fence = PathFence.builder().setRoots(PathUtils.current()).get(); + * </pre> + * + * @since 2.21.0 + */ +// Cannot implement both UnaryOperator<Path> and Function<String, Path>, so don't pick one over the other Review Comment: What about a predicate instead? ########## src/main/java/org/apache/commons/io/file/PathFence.java: ########## @@ -0,0 +1,171 @@ +/* + * 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. + * <p> + * For example, to prevent an application from using paths outside of its configuration folder: + * </p> + * <pre> + * Path config = Paths.get("path/to/config"); + * PathFence fence = PathFence.builder().setRoots(config).get(); + * </pre> + * <p> + * You call one of the {@code apply} methods to validate paths in your application: + * </p> + * <pre> + * // Gets a Path or throws IllegalArgumentException + * Path file1 = fence.{@link #apply(String) apply}("someName"); + * Path file2 = fence.{@link #apply(Path) apply}(somePath); + * </pre> + * <p> + * You can also use multiple roots as the path fence: + * </p> + * <pre> + * 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(); + * </pre> + * <p> + * To use the current directory as the path fence: + * </p> + * <pre> + * PathFence fence = PathFence.builder().setRoots(PathUtils.current()).get(); + * </pre> + * + * @since 2.21.0 + */ +// Cannot implement both UnaryOperator<Path> and Function<String, Path>, so don't pick one over the other +public final class PathFence { + + /** + * Builds {@link PathFence} instances. + */ + public static final class Builder implements Supplier<PathFence> { + + /** 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<Path> roots; + + /** + * Constructs a new instance. + * + * @param builder A builder. + */ + private PathFence(final Builder builder) { + this.roots = Arrays.stream(builder.roots).map(this::absoluteNormalize).collect(Collectors.toList()); + } + + /** + * 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 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 is within 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) { + return getPath(fileName, Paths.get(fileName)); + } + + private Path getPath(final String fileName, final Path path) { Review Comment: GetPath is a strange name for this, how about “isInsideFence” or something like that? Also why does it have two argumente, one of it unused? ########## src/test/java/org/apache/commons/io/file/PathFenceTest.java: ########## @@ -0,0 +1,144 @@ +/* + * 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.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; + +import java.io.IOException; +import java.nio.file.Files; +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; +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)); + } + + 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 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(resolved.toString()); + assertEquals(resolved.toAbsolutePath().normalize(), childOk.toAbsolutePath().normalize()); + // check with a Path instance should return the same instance when allowed + assertSame(resolved, fence.apply(resolved)); + } + + @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)); + } + + @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 + 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 resolved = fenceRootPath.resolve("subdir/../other.txt"); + final PathFence fence = PathFence.builder().setRoots(fenceRootPath).get(); + assertSame(resolved, fence.apply(resolved)); + } + + @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); + } + + @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)); + } + Review Comment: And it should probably also describe if or if not how syml8nks are handled (toRealPath?) ########## src/main/java/org/apache/commons/io/file/PathFence.java: ########## @@ -0,0 +1,171 @@ +/* + * 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. + * <p> + * For example, to prevent an application from using paths outside of its configuration folder: + * </p> + * <pre> + * Path config = Paths.get("path/to/config"); + * PathFence fence = PathFence.builder().setRoots(config).get(); + * </pre> + * <p> + * You call one of the {@code apply} methods to validate paths in your application: + * </p> + * <pre> + * // Gets a Path or throws IllegalArgumentException + * Path file1 = fence.{@link #apply(String) apply}("someName"); + * Path file2 = fence.{@link #apply(Path) apply}(somePath); + * </pre> + * <p> + * You can also use multiple roots as the path fence: + * </p> + * <pre> + * 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(); + * </pre> + * <p> + * To use the current directory as the path fence: + * </p> + * <pre> + * PathFence fence = PathFence.builder().setRoots(PathUtils.current()).get(); + * </pre> + * + * @since 2.21.0 + */ +// Cannot implement both UnaryOperator<Path> and Function<String, Path>, so don't pick one over the other +public final class PathFence { + + /** + * Builds {@link PathFence} instances. + */ + public static final class Builder implements Supplier<PathFence> { + + /** 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<Path> roots; + + /** + * Constructs a new instance. + * + * @param builder A builder. + */ + private PathFence(final Builder builder) { + this.roots = Arrays.stream(builder.roots).map(this::absoluteNormalize).collect(Collectors.toList()); + } + + /** + * 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 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 is within 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) { + return getPath(fileName, Paths.get(fileName)); + } + + private Path getPath(final String fileName, final Path path) { + if (roots.isEmpty()) { + return path; + } + final Path pathAbs = absoluteNormalize(path); + final Optional<Path> 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)); Review Comment: It’s typically frowned upon to disclose system infos (like resolved absolute path) in exceptions, at least in the toString/getMessage ########## src/test/java/org/apache/commons/io/file/PathFenceTest.java: ########## @@ -0,0 +1,144 @@ +/* + * 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.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; + +import java.io.IOException; +import java.nio.file.Files; +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; +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)); + } + + 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 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(resolved.toString()); + assertEquals(resolved.toAbsolutePath().normalize(), childOk.toAbsolutePath().normalize()); + // check with a Path instance should return the same instance when allowed + assertSame(resolved, fence.apply(resolved)); + } + + @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)); + } + + @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 + 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 resolved = fenceRootPath.resolve("subdir/../other.txt"); + final PathFence fence = PathFence.builder().setRoots(fenceRootPath).get(); + assertSame(resolved, fence.apply(resolved)); + } + + @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); + } + + @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)); + } + Review Comment: Should probably also define what to do with \\ separator and test for windows ans linux machines. Should it also deny \0 bytes before using Path API? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
