From 2dfeb5c26abd76e54e9b3be5b89e658b71755a75 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sun, 19 Mar 2023 22:50:49 +0100 Subject: [PATCH] GH-325: SFTP: fix deleting remote symbolic links 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: https://github.com/apache/mina-sshd/issues/325 --- CHANGES.md | 1 + .../client/fs/SftpFileSystemProvider.java | 7 +- .../sftp/client/fs/SftpFileSystemTest.java | 156 ++++++++++++++++-- 3 files changed, 151 insertions(+), 13 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index a9250a543..6b1efbca5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -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. diff --git a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java index ca8ae9306..9638ee9c8 100644 --- a/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java +++ b/sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java @@ -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 { diff --git a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/SftpFileSystemTest.java b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/SftpFileSystemTest.java index 29065470e..aa43cfa1f 100644 --- a/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/SftpFileSystemTest.java +++ b/sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/SftpFileSystemTest.java @@ -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; @@ -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; @@ -111,11 +114,7 @@ public void setUp() throws Exception { @Test public void testFileSystem() throws Exception { - try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(), - MapBuilder. 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()); } @@ -128,11 +127,7 @@ public void testFileSystemWriteAppend() throws Exception { getCurrentTestName()); CommonTestSupportUtils.deleteRecursive(lclSftp); - try (FileSystem fs = FileSystems.newFileSystem(createDefaultFileSystemURI(), - MapBuilder. 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"); @@ -161,6 +156,147 @@ MapBuilder. 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 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 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 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 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 defaultOptions() { return MapBuilder. builder() .put(SftpModuleProperties.READ_BUFFER_SIZE.getName(), IoUtils.DEFAULT_COPY_SIZE)