Add PathFence by garydgregory · Pull Request #805 · apache/commons-io

garydgregory · 2025-10-19T12:53:19Z

A Path fence guards against using paths outside of a "fence" of made of root paths.

This was extracted from Apache Commons Text's private PathFence.

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

Read the contribution guidelines for this project.
Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
I used AI to create any part of, or all of, this pull request: The initial unit test was created and then heavily and manually modified using Co-Pilot.
Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best-practice.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

A Path fence guards against using paths outside of a "fence" of made of root paths.

garydgregory · 2025-10-19T12:53:41Z

@ppkarwasz
Any thoughts?

ppkarwasz · 2025-10-19T15:58:18Z

This API needs a clear security model: is it intended for reading existing files, creating new files or both?

The path resolution strategy should change depending on the use case:

Reading trusted configuration files
When reading from trusted system locations (e.g. configuration directories managed by sysadmins), we typically must allow symlinks. Example: On Debian, CATALINA_BASE is /var/lib/tomcat, but /var/lib/tomcat/conf is a symlink to /etc/tomcat, which should be allowed.
Reading files from untrusted user input
For user-supplied paths, following symlinks can enable path traversal attacks. In this case, symlinks should generally not be followed.

At the moment, the class performs only syntactic path validation using path.toAbsolutePath().normalize(). This is sufficient for trusted environments (e.g. system-managed configuration directories), but it does not call Path.toRealPath(), which resolves symlinks. As a result, when used with a root directory that may contain untrusted content, it can be bypassed by symlink-based path traversal attacks.

garydgregory · 2025-10-21T12:27:42Z

Hi @ppkarwasz
Thank you for the review. I've addressed the relative path issues. I'll look into symbolic links next.

Marcono1234

Hopefully these comments are useful, and not disruptive. Though I understand that this PR is still a draft and the design and implementation decisions on this PR are not yet final.

Marcono1234 · 2026-07-07T13:43:22Z

+     */
+    // Cannot implement both UnaryOperator<Path> and Function<String, Path>
+    public Path apply(final String fileName) {
+        return getPath(fileName, Paths.get(fileName));


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)

Marcono1234 · 2026-07-07T13:45:32Z

+        final Optional<Path> first = roots.stream().filter(pathAbs::startsWith).findFirst();
+        if (first.isPresent()) {
+            return path;
+        }


Use Stream#anyMatch instead of stream.findFirst().isPresent()?

Marcono1234 · 2026-07-07T13:47:47Z

+        if (roots.isEmpty()) {
+            return path;
+        }


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.

Agreed, this should fail securely

Marcono1234 · 2026-07-07T13:58:48Z

+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "/a/b", "/a/b/c", "/a/b/c/d", "a", "a/b", "a/b/c", "a/b/c/d" })


Should this also cover "" or "/" to ensure #805 (comment) is fixed?

ecki · 2026-07-07T15:33:46Z

+ *
+ * @since 2.21.0
+ */
+// Cannot implement both UnaryOperator<Path> and Function<String, Path>, so don't pick one over the other


What about a predicate instead?

ecki · 2026-07-07T15:35:54Z

+     * @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) {


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.

ecki · 2026-07-07T15:37:21Z

+        return getPath(fileName, Paths.get(fileName));
+    }
+
+    private Path getPath(final String fileName, final Path path) {


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?

ecki · 2026-07-07T15:38:43Z

+        if (first.isPresent()) {
+            return path;
+        }
+        throw new IllegalArgumentException(String.format("[%s] -> [%s] not in the fence %s", fileName, pathAbs, roots));


It’s typically frowned upon to disclose system infos (like resolved absolute path) in exceptions, at least in the toString/getMessage

ecki · 2026-07-07T15:43:48Z

+        final Path relPath = relPathTop.resolve("foo/bar");
+        assertSame(relPath, fence.apply(relPath));
+    }
+


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?

ecki · 2026-07-07T15:47:58Z

+        final Path relPath = relPathTop.resolve("foo/bar");
+        assertSame(relPath, fence.apply(relPath));
+    }
+


And it should probably also describe if or if not how syml8nks are handled (toRealPath?)

ecki · 2026-07-07T15:49:01Z

+     * @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>


This comment is pretty confusing

garydgregory added 3 commits November 10, 2024 08:07

[IO-856] Try test on all OSs for GitHub CI

ed20f2a

Merge remote-tracking branch 'upstream/master'

cb5359f

Add PathFence

2692adf

A Path fence guards against using paths outside of a "fence" of made of root paths.

ppkarwasz reviewed Oct 19, 2025

View reviewed changes

Comment thread src/main/java/org/apache/commons/io/file/PathFence.java Outdated

garydgregory force-pushed the path_fence branch from 69b9e0a to 065b5b3 Compare October 21, 2025 13:15

Address relative path issues

e459071

garydgregory force-pushed the path_fence branch from 065b5b3 to e459071 Compare October 21, 2025 13:48

garydgregory marked this pull request as draft November 10, 2025 11:12

garydgregory mentioned this pull request Jun 29, 2026

[COMPRESS-727] Provide a symlink-resistant and concurrency-safe archive Extractor apache/commons-compress#776

Open

Marcono1234 reviewed Jul 7, 2026

View reviewed changes

ecki requested changes Jul 7, 2026

View reviewed changes

ecki reviewed Jul 7, 2026

View reviewed changes

Uh oh!

Conversation

garydgregory commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garydgregory commented Oct 19, 2025

Uh oh!

ppkarwasz commented Oct 19, 2025

Uh oh!

Uh oh!

garydgregory commented Oct 21, 2025

Uh oh!

Marcono1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

garydgregory commented Oct 19, 2025 •

edited

Loading