Skip to content

Commit 0a9e786

Browse files
committed
[VFS] Fix verifyRootDirectory() to preserve FTP working directory
Add FtpClient.isDirectory() with PWD save/restore in FTPClientWrapper. Update verifyRootDirectory() to use isDirectory() instead of changeDirectory(), which left the working directory changed as a side effect. On non-chroot FTP servers with userDirIsRoot=true, the connection starts at the user's home directory (e.g., /home/ftpuser). verifyRootDirectory() CWDs to "/" which on non-chroot servers goes to the actual server root, not the user's home. All subsequent VFS operations use relative paths that resolve against the working directory, so they would break. The isDirectory() method follows the same save/restore pattern used by the VFS-307 fallback in FTPClientWrapper.listFilesInDirectory().
1 parent 29f5342 commit 0a9e786

4 files changed

Lines changed: 113 additions & 1 deletion

File tree

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FTPClientWrapper.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,28 @@ public boolean changeDirectory(final String relPath) throws IOException {
7575
}
7676
}
7777

78+
/** {@inheritDoc} */
79+
@Override
80+
public boolean isDirectory(final String relPath) throws IOException {
81+
try {
82+
return isDirectoryWithCwd(relPath);
83+
} catch (final IOException e) {
84+
disconnect();
85+
return isDirectoryWithCwd(relPath);
86+
}
87+
}
88+
89+
private boolean isDirectoryWithCwd(final String relPath) throws IOException {
90+
final String saved = getFtpClient().printWorkingDirectory();
91+
try {
92+
return getFtpClient().changeWorkingDirectory(relPath);
93+
} finally {
94+
if (saved != null) {
95+
getFtpClient().changeWorkingDirectory(saved);
96+
}
97+
}
98+
}
99+
78100
@Override
79101
public boolean abort() throws IOException {
80102
try {

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FtpClient.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,20 @@ default boolean changeDirectory(String relPath) throws IOException {
7070
return false;
7171
}
7272

73+
/**
74+
* Tests whether a remote path is an existing directory by attempting to CWD into it.
75+
* Implementations must save and restore the working directory to avoid side effects
76+
* on subsequent operations that use relative paths.
77+
*
78+
* @param relPath The pathname to test.
79+
* @return true if the path is an existing directory, false otherwise.
80+
* @throws IOException If an I/O error occurs.
81+
* @since 2.10.1
82+
*/
83+
default boolean isDirectory(String relPath) throws IOException {
84+
return false;
85+
}
86+
7387
/**
7488
* Deletes a file on the FTP server.
7589
*

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FtpFileObject.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -651,7 +651,7 @@ private void setFTPFile(final boolean flush) throws IOException {
651651
private FTPFile verifyRootDirectory() throws IOException {
652652
final FtpClient client = getAbstractFileSystem().getClient();
653653
try {
654-
if (client.changeDirectory(relPath != null ? relPath : "/")) {
654+
if (client.isDirectory(relPath != null ? relPath : "/")) {
655655
final FTPFile result = new FTPFile();
656656
result.setType(FTPFile.DIRECTORY_TYPE);
657657
return result;

commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/ftp/FtpRootExistsOnDisconnectTest.java

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,10 @@
1616
*/
1717
package org.apache.commons.vfs2.provider.ftp;
1818

19+
import java.io.File;
1920
import java.time.Duration;
2021

22+
import org.apache.commons.net.ftp.FTPClient;
2123
import org.apache.commons.vfs2.FileObject;
2224
import org.apache.commons.vfs2.FileSystemException;
2325
import org.apache.commons.vfs2.FileSystemOptions;
@@ -105,4 +107,78 @@ public void testRootExistsFailsWhenConnectionDropped() throws Exception {
105107
"exists() must throw FileSystemException after FTP connection is lost");
106108
}
107109
}
110+
111+
/**
112+
* Tests that {@link FTPClientWrapper#isDirectory(String)} preserves the FTP working directory.
113+
* <p>
114+
* <b>Background:</b> {@code verifyRootDirectory()} in {@link FtpFileObject} uses CWD to
115+
* check if the root directory exists. It passes {@code "/"} as the path. With
116+
* {@code userDirIsRoot=true}, the FTP factory skips the initial CWD, leaving the connection
117+
* at the user's home directory (e.g., {@code /home/ftpuser}). When
118+
* {@code verifyRootDirectory()} then CWDs to {@code "/"}, the working directory changes to
119+
* the actual server root on non-chroot FTP servers (e.g., vsftpd without
120+
* {@code chroot_local_user}). All subsequent VFS operations use relative paths that resolve
121+
* against the working directory, so they would resolve against {@code "/"} instead of
122+
* {@code /home/ftpuser}, breaking file access.
123+
* </p>
124+
* <p>
125+
* <b>Why this is tested at the wrapper level instead of end-to-end through VFS:</b>
126+
* The embedded Apache MINA FTP server always chroots users to their home directory.
127+
* Within the chroot, {@code "/"} IS the user's home directory, so CWD {@code "/"} at the
128+
* VFS level is effectively a no-op — the working directory does not change to a different
129+
* location. This makes it impossible to reproduce the bug through the VFS API with MINA.
130+
* </p>
131+
* <p>
132+
* To work around this limitation, we test directly at the {@link FTPClientWrapper} level:
133+
* we manually CWD into a subdirectory within the chroot (e.g., {@code /cwdPreserveTest}),
134+
* then call {@code isDirectory("/")}. Even within MINA's chroot, CWD from
135+
* {@code /cwdPreserveTest} to {@code /} is a real directory change that we can detect.
136+
* This simulates the same pattern that would occur on a non-chroot server when
137+
* {@code verifyRootDirectory()} CWDs from the user's home to the server root.
138+
* </p>
139+
*/
140+
@Test
141+
public void testIsDirectoryPreservesWorkingDirectory() throws Exception {
142+
// Create a subdirectory to CWD into.
143+
final File testDir = new File(VfsTestUtils.getTestDirectory());
144+
final File subDir = new File(testDir, "cwdPreserveTest");
145+
subDir.mkdirs();
146+
try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
147+
manager.addProvider("ftp", new FtpFileProvider());
148+
manager.init();
149+
final FileObject root = manager.resolveFile(FtpProviderTest.getConnectionUri(), createOptions());
150+
151+
// Get the FTPClientWrapper from the VFS filesystem.
152+
final FtpFileSystem ftpFs = (FtpFileSystem) root.getFileSystem();
153+
final FTPClientWrapper wrapper = (FTPClientWrapper) ftpFs.getClient();
154+
final FTPClient rawClient = wrapper.getFtpClient();
155+
156+
try {
157+
// Move into a subdirectory so we can detect if CWD "/" changes the position.
158+
// This simulates a non-chroot server where the user's home (/home/ftpuser)
159+
// differs from the server root (/).
160+
rawClient.changeWorkingDirectory("/cwdPreserveTest");
161+
final String before = rawClient.printWorkingDirectory();
162+
Assertions.assertEquals("/cwdPreserveTest", before,
163+
"Precondition: should be in /cwdPreserveTest");
164+
165+
// isDirectory() checks existence via CWD but must save and restore the
166+
// working directory. This is what verifyRootDirectory() uses to avoid
167+
// the CWD side effect that would break subsequent relative path operations.
168+
final boolean isDirResult = wrapper.isDirectory("/");
169+
final String after = rawClient.printWorkingDirectory();
170+
171+
Assertions.assertTrue(isDirResult, "'/' should be recognized as a directory");
172+
Assertions.assertEquals("/cwdPreserveTest", after,
173+
"isDirectory() must preserve the working directory — " +
174+
"without this, verifyRootDirectory() on non-chroot FTP servers would leave " +
175+
"the connection at '/' instead of the original position, breaking all " +
176+
"subsequent operations that use relative paths");
177+
} finally {
178+
ftpFs.putClient(wrapper);
179+
}
180+
} finally {
181+
subDir.delete();
182+
}
183+
}
108184
}

0 commit comments

Comments
 (0)