Replace the shared vfs_cache temp dir in DefaultFileReplicator - #775
Replace the shared vfs_cache temp dir in DefaultFileReplicator#775rootvector2 wants to merge 2 commits into
Conversation
The replica directory was pinned to <java.io.tmpdir>/vfs_cache and left to mkdirs under the default umask, so replicated files landed world-readable in a directory any local user can predict and pre-create. Allocate it with Files.createTempDirectory on first use, which picks an unguessable name and creates it rwx------ on POSIX file systems in one step.
There was a problem hiding this comment.
Pull request overview
Hardens DefaultFileReplicator’s replica-directory handling by replacing the predictable, shared <java.io.tmpdir>/vfs_cache directory with a per-instance directory created via Files.createTempDirectory, reducing the risk of local symlink/pre-creation attacks and improving default permissions on POSIX file systems.
Changes:
- Lazily allocates the replica directory using
Files.createTempDirectory("vfs_cache")instead of a fixed temp path. - Moves temp-dir creation behind a synchronized accessor and keeps
init()focused onfileCountinitialization. - Adds unit tests asserting the temp directory is not the shared
vfs_cachename and isrwx------on POSIX.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| commons-vfs2/src/main/java/org/apache/commons/vfs2/impl/DefaultFileReplicator.java | Replaces fixed vfs_cache temp directory with a lazily-created createTempDirectory location. |
| commons-vfs2/src/test/java/org/apache/commons/vfs2/impl/DefaultFileReplicatorTest.java | Adds tests ensuring the temp dir isn’t a shared/predictable name and is owner-only on POSIX. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected File createAndAddFile(final File parent, final String baseName) throws FileSystemException { | ||
| final File file = createFile(tempDir, baseName); | ||
| final File file = createFile(getTempDir(), baseName); |
There was a problem hiding this comment.
good catch, createAndAddFile now uses parent as passed in and the javadoc no longer claims it is ignored. allocateFile already supplies the temp dir, so behavior is unchanged.
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
@rootvector2 Please review Copilot's comment.
TY!
createAndAddFile ignored its parent argument and always used the temp directory, making the protected API misleading. Use parent as passed in; allocateFile already supplies the temp directory, so behavior is unchanged.
DefaultFileReplicatorpins its replica directory to<java.io.tmpdir>/vfs_cache, a fixed name under a directory every local user can write to, and leaves creation tomkdirsunder the default umask. Everything the library replicates lands there world-readable, including the whole remote archive thatZipFileSystemandTarFileSystemcopy throughreplicateFilebefore opening it. Because the name is known up front, another local user can also pre-createvfs_cacheas a symlink and collect the replicas, or swap a replicated jar beforeVFSClassLoaderreads it; found while auditing where the library writes files on the caller's behalf.Allocate the directory with
Files.createTempDirectoryon first use instead. It picks an unguessable name and creates itrwx------on POSIX file systems in a single step, so there is no window between creation and a chmod. Keeping this in the replicator puts the guarantee where the directory's lifetime is already managed, and an application that wants a specific location still passes one to theDefaultFileReplicator(File)constructor.mvn; that'smvnon the command line by itself.