Skip to content

Commit

Permalink
apacheGH-325: SFTP: fix deleting remote symbolic links
Browse files Browse the repository at this point in the history 
Files.delete() is specified *not* to follow symbolic links. The
implementation in SftpFileSystemProvider checked for write access
and file existence via a method that does resolve symbolic links.
This lead to general inconsistency and various kinds of failures.

Fix this by making sure that the code does not resolve symbolic
links in Files.delete().

Bug: apache#325
  • Loading branch information
@tomaswolf
tomaswolf committed Mar 22, 2023
1 parent a87697b commit 2dfeb5c
Show file tree
Hide file tree
Showing 3 changed files with 151 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
* [GH-298](https://github.com/apache/mina-sshd/issues/298) Server side heartbeat not working.
* [GH-300](https://github.com/apache/mina-sshd/issues/300) Read the channel id in `SSH_MSG_CHANNEL_OPEN_CONFIRMATION` as unsigned int.
* [GH-313](https://github.com/apache/mina-sshd/issues/313) Log exceptions in the SFTP subsystem before sending a failure status reply.
* [GH-325](https://github.com/apache/mina-sshd/issues/325) SftpFileSystemProvider: fix deletions of symlinks through Files.delete().


* [SSHD-1295](https://issues.apache.org/jira/browse/SSHD-1295) Fix cancellation of futures and add options to cancel futures on time-outs.
Expand Down
7 changes: 4 additions & 3 deletions sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,15 +601,16 @@ public void createDirectory(Path dir, FileAttribute<?>... attrs) throws IOExcept
@Override
public void delete(Path path) throws IOException {
SftpPath p = toSftpPath(path);
checkAccess(p, AccessMode.WRITE);

SftpFileSystem fs = p.getFileSystem();
if (log.isDebugEnabled()) {
log.debug("delete({}) {}", fs, path);
}

if (fs.isReadOnly()) {
throw new AccessDeniedException("Filesystem is read-only: " + path.toString());
}
BasicFileAttributes attributes = readAttributes(path, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS);
try (SftpClient sftp = fs.getClient()) {
BasicFileAttributes attributes = readAttributes(path, BasicFileAttributes.class);
if (attributes.isDirectory()) {
sftp.rmdir(path.toString());
} else {
Expand Down
156 changes: 146 additions & 10 deletions sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/SftpFileSystemTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.nio.file.attribute.BasicFileAttributes;
Expand Down Expand Up @@ -84,6 +86,7 @@
import org.hamcrest.BaseMatcher;
import org.hamcrest.Description;
import org.hamcrest.MatcherAssert;
import org.junit.Assume;
import org.junit.Before;
import org.junit.FixMethodOrder;
import org.junit.Test;
Expand Down Expand Up @@ -111,11 +114,7 @@ public void setUp() throws Exception {

@Test
public void testFileSystem() throws Exception {
try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(),
MapBuilder.<String, Object> builder()
.put(SftpModuleProperties.READ_BUFFER_SIZE.getName(), IoUtils.DEFAULT_COPY_SIZE)
.put(SftpModuleProperties.WRITE_BUFFER_SIZE.getName(), IoUtils.DEFAULT_COPY_SIZE)
.build())) {
try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(), defaultOptions())) {
assertTrue("Not an SftpFileSystem", fs instanceof SftpFileSystem);
testFileSystem(fs, ((SftpFileSystem) fs).getVersion());
}
Expand All @@ -128,11 +127,7 @@ public void testFileSystemWriteAppend() throws Exception {
getCurrentTestName());
CommonTestSupportUtils.deleteRecursive(lclSftp);

try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(),
MapBuilder.<String, Object> builder()
.put(SftpModuleProperties.READ_BUFFER_SIZE.getName(), IoUtils.DEFAULT_COPY_SIZE)
.put(SftpModuleProperties.WRITE_BUFFER_SIZE.getName(), IoUtils.DEFAULT_COPY_SIZE)
.build())) {
try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(), defaultOptions())) {
assertTrue("Not an SftpFileSystem", fs instanceof SftpFileSystem);
Path parentPath = targetPath.getParent();
Path clientFolder = lclSftp.resolve("client");
Expand Down Expand Up @@ -161,6 +156,147 @@ MapBuilder.<String, Object> builder()
}
}

@Test // See GH-325
public void testDeleteLink() throws Exception {
// This test creates symbolic links.
Assume.assumeFalse(OsUtils.isWin32());
Path targetPath = detectTargetFolder();
Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME, getClass().getSimpleName(),
getCurrentTestName());
CommonTestSupportUtils.deleteRecursive(lclSftp);

List<Path> toRemove = new ArrayList<>();
try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(), defaultOptions())) {
assertTrue("Not an SftpFileSystem", fs instanceof SftpFileSystem);
Path parentPath = targetPath.getParent();
Path clientFolder = lclSftp.resolve("client");
assertHierarchyTargetFolderExists(clientFolder);
Path localFile = clientFolder.resolve("file.txt");
Files.write(localFile, "Hello".getBytes(StandardCharsets.UTF_8));
toRemove.add(localFile);
Path existingSymlink = clientFolder.resolve("existing.txt");
Files.createSymbolicLink(existingSymlink, localFile);
toRemove.add(existingSymlink);

String remExistingLink = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, existingSymlink);
Path remoteExistingLink = fs.getPath(remExistingLink);
assertHierarchyTargetFolderExists(remoteExistingLink.getParent());
assertTrue(Files.exists(remoteExistingLink));
assertTrue(Files.exists(remoteExistingLink, LinkOption.NOFOLLOW_LINKS));
assertTrue(Files.isSymbolicLink(remoteExistingLink));
Files.delete(remoteExistingLink);
assertTrue(Files.exists(localFile));
assertFalse(Files.exists(existingSymlink, LinkOption.NOFOLLOW_LINKS));
assertFalse(Files.exists(remoteExistingLink, LinkOption.NOFOLLOW_LINKS));
} finally {
for (Path p : toRemove) {
Files.deleteIfExists(p);
}
}
}

@Test // See GH-325
public void testDeleteNonexistingLink() throws Exception {
// This test creates symbolic links.
Assume.assumeFalse(OsUtils.isWin32());
Path targetPath = detectTargetFolder();
Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME, getClass().getSimpleName(),
getCurrentTestName());
CommonTestSupportUtils.deleteRecursive(lclSftp);

List<Path> toRemove = new ArrayList<>();
try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(), defaultOptions())) {
assertTrue("Not an SftpFileSystem", fs instanceof SftpFileSystem);
Path parentPath = targetPath.getParent();
Path clientFolder = lclSftp.resolve("client");
assertHierarchyTargetFolderExists(clientFolder);
Path nonExistingSymlink = clientFolder.resolve("nonexisting.txt");
Files.createSymbolicLink(nonExistingSymlink, clientFolder.resolve("gone.txt"));
toRemove.add(nonExistingSymlink);

String remNonExistingLink = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, nonExistingSymlink);
Path remoteNonExistingLink = fs.getPath(remNonExistingLink);
assertFalse(Files.exists(remoteNonExistingLink));
assertTrue(Files.exists(remoteNonExistingLink, LinkOption.NOFOLLOW_LINKS));
assertTrue(Files.isSymbolicLink(remoteNonExistingLink));
Files.delete(remoteNonExistingLink);
assertFalse(Files.exists(nonExistingSymlink, LinkOption.NOFOLLOW_LINKS));
assertFalse(Files.exists(remoteNonExistingLink, LinkOption.NOFOLLOW_LINKS));
} finally {
for (Path p : toRemove) {
Files.deleteIfExists(p);
}
}
}

@Test // See GH-325
public void testDeleteDirectoryLink() throws Exception {
// This test creates symbolic links.
Assume.assumeFalse(OsUtils.isWin32());
Path targetPath = detectTargetFolder();
Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME, getClass().getSimpleName(),
getCurrentTestName());
CommonTestSupportUtils.deleteRecursive(lclSftp);

List<Path> toRemove = new ArrayList<>();
try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(), defaultOptions())) {
assertTrue("Not an SftpFileSystem", fs instanceof SftpFileSystem);
Path parentPath = targetPath.getParent();
Path clientFolder = lclSftp.resolve("client");
assertHierarchyTargetFolderExists(clientFolder);
Path directory = clientFolder.resolve("subdir");
Files.createDirectory(directory);
toRemove.add(directory);
Path directorySymlink = clientFolder.resolve("dirlink");
Files.createSymbolicLink(directorySymlink, directory);
toRemove.add(directorySymlink);

String remDirectoryLink = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, directorySymlink);
Path remoteDirectoryLink = fs.getPath(remDirectoryLink);
assertTrue(Files.isDirectory(remoteDirectoryLink));
assertTrue(Files.exists(remoteDirectoryLink, LinkOption.NOFOLLOW_LINKS));
assertFalse(Files.isDirectory(remoteDirectoryLink, LinkOption.NOFOLLOW_LINKS));
assertTrue(Files.isSymbolicLink(remoteDirectoryLink));
Files.delete(remoteDirectoryLink);
assertTrue(Files.isDirectory(directory));
assertFalse(Files.exists(directorySymlink, LinkOption.NOFOLLOW_LINKS));
assertFalse(Files.exists(remoteDirectoryLink, LinkOption.NOFOLLOW_LINKS));
} finally {
for (Path p : toRemove) {
Files.deleteIfExists(p);
}
}
}

@Test // See GH-325
public void testDeleteNeverExistingLink() throws Exception {
// This test creates symbolic links.
Assume.assumeFalse(OsUtils.isWin32());
Path targetPath = detectTargetFolder();
Path lclSftp = CommonTestSupportUtils.resolve(targetPath, SftpConstants.SFTP_SUBSYSTEM_NAME, getClass().getSimpleName(),
getCurrentTestName());
CommonTestSupportUtils.deleteRecursive(lclSftp);

List<Path> toRemove = new ArrayList<>();
try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(), defaultOptions())) {
assertTrue("Not an SftpFileSystem", fs instanceof SftpFileSystem);
Path parentPath = targetPath.getParent();
Path clientFolder = lclSftp.resolve("client");
assertHierarchyTargetFolderExists(clientFolder);

String doesNotExist = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath,
clientFolder.resolve("neverExisted.txt"));
Path neverExisted = fs.getPath(doesNotExist);
assertFalse(Files.exists(neverExisted));
assertFalse(Files.deleteIfExists(neverExisted));
assertThrows(NoSuchFileException.class, () -> Files.delete(neverExisted));
} finally {
for (Path p : toRemove) {
Files.deleteIfExists(p);
}
}
}

private Map<String, Object> defaultOptions() {
return MapBuilder.<String, Object> builder()
.put(SftpModuleProperties.READ_BUFFER_SIZE.getName(), IoUtils.DEFAULT_COPY_SIZE)
Expand Down

0 comments on commit 2dfeb5c

Please sign in to comment.