Skip to content

Commit 50b4c72

Browse files
ilanggarydgregory
andauthored
Fix FtpFileObject.exists() returning true for root-level folders after connection drop (#757)
* [VFS] Fix FtpFileObject.exists() returning true for root-level folders after connection drop setFTPFile() blindly assumed that root-level directories (where getParent() returns null) always exist by creating a synthetic FTPFile with DIRECTORY_TYPE. This caused exists() to return true even after the FTP connection was lost. Fix: add verifyRootDirectory() which uses CWD "." to verify the directory exists on the server. CWD "." checks the current directory (the logical VFS root) which is correct for both configurations: - userDirIsRoot=true: current dir is the user's login directory - userDirIsRoot=false: current dir is "/" (set by the factory CWD) Using "." rather than "/" avoids a semantic mismatch on non-chroot servers with userDirIsRoot=true, where "/" would go to the actual server root instead of the user's login directory. Add FtpClient.changeDirectory() default method and FTPClientWrapper implementation to support CWD from FtpFileObject. * Fix Javadoc since tag. --------- Co-authored-by: Gary Gregory <garydgregory@users.noreply.github.com>
1 parent 3a3febb commit 50b4c72

4 files changed

Lines changed: 162 additions & 3 deletions

File tree

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,17 @@ protected FTPClientWrapper(final GenericFileName rootFileName, final FileSystemO
6464
getFtpClient(); // fail-fast
6565
}
6666

67+
/** {@inheritDoc} */
68+
@Override
69+
public boolean changeDirectory(final String relPath) throws IOException {
70+
try {
71+
return getFtpClient().changeWorkingDirectory(relPath);
72+
} catch (final IOException e) {
73+
disconnect();
74+
return getFtpClient().changeWorkingDirectory(relPath);
75+
}
76+
}
77+
6778
@Override
6879
public boolean abort() throws IOException {
6980
try {

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,18 @@ public interface FtpClient {
5858
*/
5959
boolean completePendingCommand() throws IOException;
6060

61+
/**
62+
* Changes the current working directory of the FTP session.
63+
*
64+
* @param relPath The pathname of the directory to change to.
65+
* @return true if successfully completed, false if not.
66+
* @throws IOException If an I/O error occurs.
67+
* @since 2.12.0
68+
*/
69+
default boolean changeDirectory(String relPath) throws IOException {
70+
return false;
71+
}
72+
6173
/**
6274
* Deletes a file on the FTP server.
6375
*

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

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -631,11 +631,39 @@ private void setFTPFile(final boolean flush) throws IOException {
631631
if (parent != null) {
632632
newFileInfo = parent.getChildFile(UriParser.decode(getName().getBaseName()), flush);
633633
} else {
634-
// Assume the root is a directory and exists
635-
newFileInfo = new FTPFile();
636-
newFileInfo.setType(FTPFile.DIRECTORY_TYPE);
634+
// Root-level resource: no parent to query via getChildFile().
635+
// Verify the directory exists using CWD, which is a lightweight
636+
// control-channel command (no data transfer). Previously this
637+
// assumed the root always exists, causing exists() to return true
638+
// for non-existent FTP folders.
639+
newFileInfo = verifyRootDirectory();
637640
}
638641
ftpFile = newFileInfo == null ? UNKNOWN : newFileInfo;
639642
}
640643
}
644+
645+
/**
646+
* Verifies a root-level FTP directory exists by attempting to CWD into it.
647+
* Returns an FTPFile with DIRECTORY_TYPE if CWD succeeds, or {@code null} if it fails.
648+
* <p>
649+
* Uses CWD "." (the current directory) rather than CWD "/" to verify
650+
* the logical root. With {@code userDirIsRoot=true}, the logical root
651+
* is the user's login directory, not the server root "/".
652+
* </p>
653+
*/
654+
private FTPFile verifyRootDirectory() throws IOException {
655+
final FtpClient client = getAbstractFileSystem().getClient();
656+
try {
657+
// relPath is always null for the root (constructor maps "." to null),
658+
// so this always resolves to CWD ".". The relPath check is defensive.
659+
if (client.changeDirectory(relPath != null ? relPath : ".")) {
660+
final FTPFile result = new FTPFile();
661+
result.setType(FTPFile.DIRECTORY_TYPE);
662+
return result;
663+
}
664+
return null;
665+
} finally {
666+
getAbstractFileSystem().putClient(client);
667+
}
668+
}
641669
}
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* https://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.commons.vfs2.provider.ftp;
18+
19+
import java.time.Duration;
20+
21+
import org.apache.commons.vfs2.FileObject;
22+
import org.apache.commons.vfs2.FileSystemException;
23+
import org.apache.commons.vfs2.FileSystemOptions;
24+
import org.apache.commons.vfs2.VfsTestUtils;
25+
import org.apache.commons.vfs2.impl.DefaultFileSystemManager;
26+
import org.junit.jupiter.api.AfterEach;
27+
import org.junit.jupiter.api.Assertions;
28+
import org.junit.jupiter.api.BeforeEach;
29+
import org.junit.jupiter.api.Test;
30+
31+
/**
32+
* Tests {@link FtpFileObject#exists()} for root-level FTP folders where {@code getParent()} returns {@code null}.
33+
* <p>
34+
* Regression test for the bug in {@link FtpFileObject} where {@code setFTPFile()} blindly assumed that root-level
35+
* directories exist ({@code setType(DIRECTORY_TYPE)}) without verifying on the server. This caused {@code exists()} to
36+
* return {@code true} even after the FTP connection was lost, while non-root folders correctly reported the connection
37+
* failure via {@link FileSystemException}.
38+
* </p>
39+
*/
40+
public class FtpRootExistsOnDisconnectTest {
41+
42+
@BeforeEach
43+
public void setUp() throws Exception {
44+
FtpProviderTest.setUpClass(VfsTestUtils.getTestDirectory(), null, null);
45+
}
46+
47+
@AfterEach
48+
public void tearDown() {
49+
FtpProviderTest.tearDownClass();
50+
}
51+
52+
private static FileSystemOptions createOptions() {
53+
final FileSystemOptions options = new FileSystemOptions();
54+
final FtpFileSystemConfigBuilder builder = FtpFileSystemConfigBuilder.getInstance();
55+
builder.setUserDirIsRoot(options, true);
56+
builder.setPassiveMode(options, true);
57+
builder.setConnectTimeout(options, Duration.ofSeconds(10));
58+
return options;
59+
}
60+
61+
/**
62+
* Tests that {@code exists()} returns {@code true} when the server is running, and does not silently return
63+
* {@code true} after the FTP connection is lost.
64+
* <p>
65+
* With {@code userDirIsRoot=true}, the root's {@code getParent()} returns {@code null}, which triggers the
66+
* {@code verifyRootDirectory()} code path in {@code setFTPFile()}.
67+
* </p>
68+
* <p>
69+
* Before the fix, {@code setFTPFile()} set {@code type=DIRECTORY} when {@code getParent()} returned {@code null},
70+
* without contacting the server. After the fix, {@code setFTPFile()} uses CWD to verify, which fails on a dead
71+
* connection.
72+
* </p>
73+
*/
74+
@Test
75+
public void testRootExistsFailsWhenConnectionDropped() throws Exception {
76+
try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
77+
manager.addProvider("ftp", new FtpFileProvider());
78+
manager.init();
79+
final FileObject root = manager.resolveFile(FtpProviderTest.getConnectionUri(), createOptions());
80+
81+
// Verify precondition: with userDirIsRoot=true, getParent() returns null,
82+
// which is the code path this test exercises.
83+
Assertions.assertNull(root.getParent(),
84+
"Root folder's getParent() should return null with userDirIsRoot=true");
85+
86+
// Verify the root exists initially.
87+
Assertions.assertTrue(root.exists(), "Root should exist while server is running");
88+
89+
// Stop the server to simulate a connection drop.
90+
FtpProviderTest.tearDownClass();
91+
92+
// exists() must throw FileSystemException on the dead connection.
93+
// The first call may still succeed if the MINA server processes one last
94+
// CWD during graceful shutdown, but the second call must fail.
95+
boolean threwException = false;
96+
for (int i = 0; i < 2 && !threwException; i++) {
97+
root.refresh();
98+
try {
99+
root.exists();
100+
} catch (final FileSystemException expected) {
101+
threwException = true;
102+
}
103+
}
104+
Assertions.assertTrue(threwException,
105+
"exists() must throw FileSystemException after FTP connection is lost");
106+
}
107+
}
108+
}

0 commit comments

Comments
 (0)