Marcono1234 commented on code in PR #805: URL: https://github.com/apache/commons-io/pull/805#discussion_r3536927777
########## 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)); Review Comment: Usage of `Paths` ties this to the default file system. Is that intended (if so would be good to document it), or would it make more sense to ensure that all `roots` have the same file system and then use `root.getFileSystem().getPath(...)`? (would only work though if `roots` is non-empty) ########## 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; + } Review Comment: Use `Stream#anyMatch` instead of `stream.findFirst().isPresent()`? ########## 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" }) Review Comment: Should this also cover `""` or `"/"` to ensure https://github.com/apache/commons-io/pull/805#discussion_r2443377598 is fixed? ########## 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; + } Review Comment: So if `roots` is empty, the path fence is effectively disabled? Maybe would be better to add an explicit `Builder` method / static `PathFence` factory method for that instead? Otherwise when accidentally not calling `setRoots` or calling it with an empty array, it might give a false sense of security when `PathFence` is actually ineffective. -- 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]
