From 53e1609f3d3f517c352d3a24e46bd2fa67d8a6e8 Mon Sep 17 00:00:00 2001 From: crawld Date: Thu, 6 Aug 2026 12:27:22 -0700 Subject: [PATCH 01/21] Use the copy-data extension for cp_file when the server supports it Server-side copy over SFTP (asyncssh >= 2.19 with an OpenSSH >= 9.0 server) needs no shell access and keeps the data on the server, which also unblocks copy/move on SFTP-only servers that deny exec. The branch is gated on supports_remote_copy: without it asyncssh would silently copy through the client (download + re-upload) on servers lacking the extension, and asyncssh <= 2.18 has no server-side copy at all. remote_only=True keeps that guarantee even if the gate is ever bypassed. Everything else falls back to the shell cp as before. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sshfs/spec.py b/sshfs/spec.py index f8f5940..177675c 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -261,6 +261,15 @@ async def _get_file( @wrap_exceptions async def _cp_file(self, lpath, rpath, **kwargs): + # Server-side copy through the copy-data extension (asyncssh >= + # 2.19 with an OpenSSH >= 9.0 server) needs no shell access and + # keeps the data on the server. remote_only guards against + # asyncssh silently copying through the client instead. Without + # the extension, fall back to a shell cp. + async with self._pool.get() as channel: + if getattr(channel, "supports_remote_copy", False): + return await channel.copy(lpath, rpath, remote_only=True) + cmd = f"cp {shlex.quote(lpath)} {shlex.quote(rpath)}" await self._execute(cmd) From 4d6116aa6ee3a8ed3c61b4d30b505361d586eed7 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 12:27:22 -0700 Subject: [PATCH 02/21] tests: pin cp_file copy-data gating A channel advertising supports_remote_copy must be used with remote_only=True and no shell; a channel without it (or an asyncssh without the attribute) must use the shell fallback. test_copy keeps covering the fallback end to end against mockssh. Co-Authored-By: Claude Fable 5 --- tests/test_sshfs.py | 60 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 141baf7..1359412 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -211,6 +211,66 @@ def test_copy(fs, remote_dir): assert strip_keys(initial_info) == strip_keys(secondary_info) +class _FakeChannelPool: + def __init__(self, channel): + self.channel = channel + + def get(self): + pool = self + + class _Ctx: + async def __aenter__(self): + return pool.channel + + async def __aexit__(self, *exc): + return False + + return _Ctx() + + +def test_cp_file_remote_copy(fs, monkeypatch): + # A channel advertising copy-data must be used with remote_only=True + # (otherwise asyncssh silently copies through the client) and the + # shell fallback must not run. + calls = {} + + class Channel: + supports_remote_copy = True + + async def copy(self, lpath, rpath, remote_only=False): + calls["copy"] = (lpath, rpath, remote_only) + + async def no_shell(*args, **kwargs): + raise AssertionError("shell fallback must not run") + + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + monkeypatch.setattr(fs, "_execute", no_shell) + + fs.cp_file("/src", "/dst") + assert calls["copy"] == ("/src", "/dst", True) + + +def test_cp_file_shell_fallback(fs, monkeypatch): + # Without copy-data support (or on asyncssh < 2.19, where the + # attribute does not exist), the shell cp path is used. + calls = {} + + class Channel: + supports_remote_copy = False + + async def copy(self, *args, **kwargs): + raise AssertionError("copy-data must not be attempted") + + async def record_shell(cmd, **kwargs): + calls["cmd"] = cmd + + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + monkeypatch.setattr(fs, "_execute", record_shell) + + fs.cp_file("/src", "/dst") + assert calls["cmd"] == "cp /src /dst" + + def test_rm(fs, remote_dir): fs.touch(remote_dir + "/a.txt") fs.rm(remote_dir + "/a.txt") From fa854c63b499c48a2bee1588c4e6c36bfe099ee5 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 14:50:23 -0700 Subject: [PATCH 03/21] tests: verify copied content in test_copy The test previously copied an empty file and compared metadata only, so a copy that produced wrong or no content would still pass. Co-Authored-By: Claude Fable 5 --- tests/test_sshfs.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 1359412..0d40cae 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -199,7 +199,9 @@ def test_move(fs, remote_dir): def test_copy(fs, remote_dir): - fs.touch(remote_dir + "/a.txt") + data = b"data to copy" + with fs.open(remote_dir + "/a.txt", "wb") as stream: + stream.write(data) initial_info = fs.info(remote_dir + "/a.txt") fs.copy(remote_dir + "/a.txt", remote_dir + "/b.txt") @@ -207,6 +209,7 @@ def test_copy(fs, remote_dir): assert fs.exists(remote_dir + "/a.txt") assert fs.exists(remote_dir + "/b.txt") + assert fs.cat_file(remote_dir + "/b.txt") == data assert strip_keys(initial_info) == strip_keys(secondary_info) From bf8fee194f080cc5fcba61c00d694f9ddb3b9f37 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 15:07:53 -0700 Subject: [PATCH 04/21] Cache the copy-data capability; cover legacy channels in tests Both from review: the capability is per-connection, so probe it once and let the shell fallback skip the channel pool afterwards (under a saturated hard pool a shell-only copy could otherwise block waiting for an SFTP channel it never uses), and actually exercise the asyncssh < 2.19 case where supports_remote_copy does not exist, which the fallback test claimed but did not cover. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 19 ++++++++++++++----- tests/test_sshfs.py | 41 +++++++++++++++++++++++++++++------------ 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 177675c..9c43312 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -79,6 +79,8 @@ def __init__( self._stack = AsyncExitStack() self.active_executors = 0 + # None means "not probed yet"; resolved on first _cp_file call. + self._supports_remote_copy = None self._client, self._pool = self.connect( host, pool_type, @@ -264,11 +266,18 @@ async def _cp_file(self, lpath, rpath, **kwargs): # Server-side copy through the copy-data extension (asyncssh >= # 2.19 with an OpenSSH >= 9.0 server) needs no shell access and # keeps the data on the server. remote_only guards against - # asyncssh silently copying through the client instead. Without - # the extension, fall back to a shell cp. - async with self._pool.get() as channel: - if getattr(channel, "supports_remote_copy", False): - return await channel.copy(lpath, rpath, remote_only=True) + # asyncssh silently copying through the client instead. The + # capability is per-connection, so it is cached after the first + # probe and the shell fallback never touches the channel pool + # again. Without the extension, fall back to a shell cp. + if self._supports_remote_copy is not False: + async with self._pool.get() as channel: + if self._supports_remote_copy is None: + self._supports_remote_copy = getattr( + channel, "supports_remote_copy", False + ) + if self._supports_remote_copy: + return await channel.copy(lpath, rpath, remote_only=True) cmd = f"cp {shlex.quote(lpath)} {shlex.quote(rpath)}" await self._execute(cmd) diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 0d40cae..6a7a1c0 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -235,43 +235,60 @@ def test_cp_file_remote_copy(fs, monkeypatch): # A channel advertising copy-data must be used with remote_only=True # (otherwise asyncssh silently copies through the client) and the # shell fallback must not run. - calls = {} + calls = [] class Channel: supports_remote_copy = True async def copy(self, lpath, rpath, remote_only=False): - calls["copy"] = (lpath, rpath, remote_only) + calls.append((lpath, rpath, remote_only)) async def no_shell(*args, **kwargs): raise AssertionError("shell fallback must not run") + monkeypatch.setattr(fs, "_supports_remote_copy", None) monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) monkeypatch.setattr(fs, "_execute", no_shell) fs.cp_file("/src", "/dst") - assert calls["copy"] == ("/src", "/dst", True) + assert calls == [("/src", "/dst", True)] + # The probed capability is cached and reused. + fs.cp_file("/src2", "/dst2") + assert calls[-1] == ("/src2", "/dst2", True) -def test_cp_file_shell_fallback(fs, monkeypatch): - # Without copy-data support (or on asyncssh < 2.19, where the - # attribute does not exist), the shell cp path is used. - calls = {} - class Channel: - supports_remote_copy = False +@pytest.mark.parametrize("legacy_asyncssh", [False, True]) +def test_cp_file_shell_fallback(fs, monkeypatch, legacy_asyncssh): + # Channels without copy-data support -- and asyncssh < 2.19 + # channels, which lack the supports_remote_copy attribute entirely + # -- must use the shell cp path, and only the first call may touch + # the channel pool (the probed capability is cached). + calls = [] + pool_uses = [] + class Channel: async def copy(self, *args, **kwargs): raise AssertionError("copy-data must not be attempted") + if not legacy_asyncssh: + Channel.supports_remote_copy = False + async def record_shell(cmd, **kwargs): - calls["cmd"] = cmd + calls.append(cmd) - monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + pool = _FakeChannelPool(Channel()) + _orig_get = pool.get + pool.get = lambda: pool_uses.append(1) or _orig_get() + + monkeypatch.setattr(fs, "_supports_remote_copy", None) + monkeypatch.setattr(fs, "_pool", pool) monkeypatch.setattr(fs, "_execute", record_shell) fs.cp_file("/src", "/dst") - assert calls["cmd"] == "cp /src /dst" + fs.cp_file("/src2", "/dst2") + assert calls == ["cp /src /dst", "cp /src2 /dst2"] + assert len(pool_uses) == 1 def test_rm(fs, remote_dir): From 71245e6cd72835a3f6bf645e585e263eeae356e3 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 15:40:57 -0700 Subject: [PATCH 05/21] Match shell cp semantics on the copy-data path All three from review. asyncssh's copy() defaults diverge from the shell fallback, so the same operation behaved differently depending on whether the server advertises copy-data: - it opens the destination with truncation and no same-file check, so aliased paths (same path or a symlink to the source) would destroy the source where shell cp refuses the copy -- guard with realpath comparison and raise shutil.SameFileError (hardlink aliases stay undetectable over SFTP: no inode in the attrs) - follow_symlinks=False copied the link itself; the shell path copies the target's content -- pass follow_symlinks=True - preserve=False created the destination with default permissions; the shell path keeps the source mode -- pass preserve=True Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 24 ++++++++++++++++++++++- tests/test_sshfs.py | 47 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 9c43312..4dc8a0b 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -1,6 +1,7 @@ import asyncio import posixpath import shlex +import shutil import stat import weakref from contextlib import AsyncExitStack, suppress @@ -277,7 +278,28 @@ async def _cp_file(self, lpath, rpath, **kwargs): channel, "supports_remote_copy", False ) if self._supports_remote_copy: - return await channel.copy(lpath, rpath, remote_only=True) + # asyncssh opens the destination with truncation and + # no same-file check, so aliased paths (same path or + # a symlink to the source) would destroy the source; + # shell cp refuses them instead. Hardlink aliases + # cannot be detected over SFTP (no inode in attrs). + src, dst = await asyncio.gather( + channel.realpath(lpath), channel.realpath(rpath) + ) + if src == dst: + raise shutil.SameFileError( + f"{lpath!r} and {rpath!r} are the same file" + ) + # preserve and follow_symlinks match what the shell + # cp fallback does: copy the link target's content + # and keep the source permissions. + return await channel.copy( + lpath, + rpath, + preserve=True, + follow_symlinks=True, + remote_only=True, + ) cmd = f"cp {shlex.quote(lpath)} {shlex.quote(rpath)}" await self._execute(cmd) diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 6a7a1c0..9440d42 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -1,6 +1,7 @@ import hashlib import posixpath import secrets +import shutil import tempfile import warnings from concurrent import futures @@ -233,15 +234,19 @@ async def __aexit__(self, *exc): def test_cp_file_remote_copy(fs, monkeypatch): # A channel advertising copy-data must be used with remote_only=True - # (otherwise asyncssh silently copies through the client) and the - # shell fallback must not run. + # (otherwise asyncssh silently copies through the client), matching + # the shell fallback's semantics (content of the link target, + # source permissions), and the shell fallback must not run. calls = [] class Channel: supports_remote_copy = True - async def copy(self, lpath, rpath, remote_only=False): - calls.append((lpath, rpath, remote_only)) + async def realpath(self, path): + return "/real" + path + + async def copy(self, lpath, rpath, **kwargs): + calls.append((lpath, rpath, kwargs)) async def no_shell(*args, **kwargs): raise AssertionError("shell fallback must not run") @@ -250,12 +255,42 @@ async def no_shell(*args, **kwargs): monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) monkeypatch.setattr(fs, "_execute", no_shell) + expected = { + "preserve": True, + "follow_symlinks": True, + "remote_only": True, + } fs.cp_file("/src", "/dst") - assert calls == [("/src", "/dst", True)] + assert calls == [("/src", "/dst", expected)] # The probed capability is cached and reused. fs.cp_file("/src2", "/dst2") - assert calls[-1] == ("/src2", "/dst2", True) + assert calls[-1] == ("/src2", "/dst2", expected) + + +def test_cp_file_same_file(fs, monkeypatch): + # Aliased source and destination (same path or a symlink to the + # source) must fail before any data is touched: asyncssh's copy + # opens the destination with truncation and would destroy the + # source, while shell cp refuses the copy. + class Channel: + supports_remote_copy = True + + async def realpath(self, path): + return "/real/same" + + async def copy(self, *args, **kwargs): + raise AssertionError("copy must not run on aliased paths") + + async def no_shell(*args, **kwargs): + raise AssertionError("shell fallback must not run") + + monkeypatch.setattr(fs, "_supports_remote_copy", None) + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + monkeypatch.setattr(fs, "_execute", no_shell) + + with pytest.raises(shutil.SameFileError): + fs.cp_file("/a", "/link-to-a") @pytest.mark.parametrize("legacy_asyncssh", [False, True]) From 6b5492cde9da5b1687467e80bac93f465fd818bb Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 16:21:38 -0700 Subject: [PATCH 06/21] Delegate directory destinations to the shell fallback From review: asyncssh's copy() resolves a directory destination as "copy into" (dst/basename(src)), so cp_file("/dir/file", "/dir") passed the same-file guard while the effective destination was the source itself, which the truncating open would destroy. Rather than replicate cp's into-directory resolution (symlink leaf names and re-canonicalization included), the remote-copy path now handles only the plain file-to-file form; directory destinations take the shell path, whose cp implements those rules and protections natively -- matching the pre-copy-data behavior for that form exactly. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 58 ++++++++++++++++++++++++++++----------------- tests/test_sshfs.py | 38 ++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 23 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 4dc8a0b..574fe54 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -9,7 +9,7 @@ from typing import Optional import asyncssh -from asyncssh.sftp import SFTPOpUnsupported +from asyncssh.sftp import SFTPNoSuchFile, SFTPOpUnsupported from fsspec.asyn import ( AsyncFileSystem, FSTimeoutError, @@ -278,28 +278,42 @@ async def _cp_file(self, lpath, rpath, **kwargs): channel, "supports_remote_copy", False ) if self._supports_remote_copy: - # asyncssh opens the destination with truncation and - # no same-file check, so aliased paths (same path or - # a symlink to the source) would destroy the source; - # shell cp refuses them instead. Hardlink aliases - # cannot be detected over SFTP (no inode in attrs). - src, dst = await asyncio.gather( - channel.realpath(lpath), channel.realpath(rpath) - ) - if src == dst: - raise shutil.SameFileError( - f"{lpath!r} and {rpath!r} are the same file" + # The remote copy handles only the plain + # file-to-file form. A directory destination means + # "copy into" with cp's resolution rules (including + # its same-file protections), so that form is + # delegated to the shell fallback below. + dst_attrs = None + with suppress(SFTPNoSuchFile): + dst_attrs = await channel.stat(rpath) + if dst_attrs is None or not stat.S_ISDIR( + dst_attrs.permissions + ): + # asyncssh opens the destination with truncation + # and no same-file check, so aliased paths (same + # path or a symlink to the source) would destroy + # the source; shell cp refuses them instead. + # Hardlink aliases cannot be detected over SFTP + # (no inode in attrs). + src, dst = await asyncio.gather( + channel.realpath(lpath), + channel.realpath(rpath), + ) + if src == dst: + raise shutil.SameFileError( + f"{lpath!r} and {rpath!r} " "are the same file" + ) + # preserve and follow_symlinks match what the + # shell cp fallback does: copy the link + # target's content and keep the source + # permissions. + return await channel.copy( + lpath, + rpath, + preserve=True, + follow_symlinks=True, + remote_only=True, ) - # preserve and follow_symlinks match what the shell - # cp fallback does: copy the link target's content - # and keep the source permissions. - return await channel.copy( - lpath, - rpath, - preserve=True, - follow_symlinks=True, - remote_only=True, - ) cmd = f"cp {shlex.quote(lpath)} {shlex.quote(rpath)}" await self._execute(cmd) diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 9440d42..d0ec9d8 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -11,7 +11,7 @@ import fsspec import pytest -from asyncssh.sftp import SFTPAttrs, SFTPFailure +from asyncssh.sftp import SFTPAttrs, SFTPFailure, SFTPNoSuchFile from importlib_metadata import entry_points from sshfs import SSHFileSystem @@ -242,6 +242,9 @@ def test_cp_file_remote_copy(fs, monkeypatch): class Channel: supports_remote_copy = True + async def stat(self, path): + raise SFTPNoSuchFile("destination does not exist") + async def realpath(self, path): return "/real" + path @@ -276,6 +279,9 @@ def test_cp_file_same_file(fs, monkeypatch): class Channel: supports_remote_copy = True + async def stat(self, path): + return SFTPAttrs(permissions=0o100644) + async def realpath(self, path): return "/real/same" @@ -293,6 +299,36 @@ async def no_shell(*args, **kwargs): fs.cp_file("/a", "/link-to-a") +def test_cp_file_directory_destination(fs, monkeypatch): + # A directory destination means "copy into" with cp's resolution + # rules (and its own same-file protections, e.g. + # cp_file("/dir/file", "/dir")), so it must take the shell path + # even when copy-data is available. + calls = [] + + class Channel: + supports_remote_copy = True + + async def stat(self, path): + return SFTPAttrs(permissions=0o040755) + + async def realpath(self, path): + raise AssertionError("realpath not needed for dir targets") + + async def copy(self, *args, **kwargs): + raise AssertionError("copy must not run for dir targets") + + async def record_shell(cmd, **kwargs): + calls.append(cmd) + + monkeypatch.setattr(fs, "_supports_remote_copy", None) + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + monkeypatch.setattr(fs, "_execute", record_shell) + + fs.cp_file("/dir/file", "/dir") + assert calls == ["cp /dir/file /dir"] + + @pytest.mark.parametrize("legacy_asyncssh", [False, True]) def test_cp_file_shell_fallback(fs, monkeypatch, legacy_asyncssh): # Channels without copy-data support -- and asyncssh < 2.19 From de13b73d6316d5d421d12b8e81d5248794466977 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 17:41:07 -0700 Subject: [PATCH 07/21] Harden the copy-data path and the move fallback The remote copy now writes to a temporary name, sets the destination mode there and renames over the destination: - the directory check uses isdir(), which inspects the file type on every SFTP version; v4+ attributes carry no type bits in permissions, so the previous S_ISDIR(permissions) never matched and a directory destination could resolve back onto the source - realpath results are normalized to bytes before the same-file comparison (realpath preserves its argument's str/bytes type, so bytes and str spellings of one path compared unequal) - hardlink aliases carry no inode over SFTP and cannot be detected; the temporary-name strategy keeps them (and check/open races) safe instead of truncating the source - directory destinations resolve against the source's basename like cp, so SFTP-only servers support the copy-into form as well - an existing destination keeps its own mode and a new one gets the source's, like cp; the mode is set before the rename so a failure never leaves the destination changed, and timestamps are not preserved, also like cp The move fallback deleted the source in a finally block, so a failed copy still destroyed it; the source is now removed only after the copy fully succeeded. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 117 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 46 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 574fe54..30b079f 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -1,5 +1,6 @@ import asyncio import posixpath +import secrets import shlex import shutil import stat @@ -225,12 +226,11 @@ async def _mv(self, lpath, rpath, **kwargs): # Some systems doesn't natively support posix_rename # which is an extension to the original SFTP protocol. # In that case we are going to copy the file and delete - # it. + # it. The source must only be removed after the copy fully + # succeeded. - try: - await self._cp_file(lpath, rpath) - finally: - await self._rm_file(lpath) + await self._cp_file(lpath, rpath) + await self._rm_file(lpath) @wrap_exceptions async def _put_file( @@ -262,15 +262,75 @@ async def _get_file( progress_handler=as_progress_handler(callback), ) + async def _remote_copy_file(self, channel, lpath, rpath): + # copy-data must never be pointed at an aliased destination: + # asyncssh opens it with truncation and would destroy the + # source, where shell cp refuses the copy. The realpath + # comparison (normalized to bytes: realpath preserves the + # str/bytes type of its argument) catches path and symlink + # aliases; hardlinks carry no inode over SFTP and cannot be + # detected, so the data is written to a temporary name and + # renamed over the destination, which is safe for any alias. + src, dst = await asyncio.gather( + channel.realpath(lpath), channel.realpath(rpath) + ) + src, dst = channel.encode(src), channel.encode(dst) + + # A directory destination means "copy into": like cp, resolve + # it against the source's basename. isdir() is False for + # missing paths and checks the file type on every SFTP version + # (v4+ attributes carry no type bits in `permissions`). + if await channel.isdir(dst): + dst = channel.encode( + await channel.realpath( + posixpath.join( + dst, posixpath.basename(channel.encode(lpath)) + ) + ) + ) + + if src == dst: + raise shutil.SameFileError( + f"{lpath!r} and {rpath!r} are the same file" + ) + + # cp keeps an existing destination's mode and gives a new file + # the source's mode (the remote umask is unknowable over SFTP + # and cannot be applied). The mode is set on the temporary file + # before the rename, so a failure never leaves the destination + # changed. Timestamps are deliberately not preserved, like cp. + src_attrs = await channel.stat(lpath) + dst_attrs = None + with suppress(SFTPNoSuchFile): + dst_attrs = await channel.stat(dst) + mode = (dst_attrs or src_attrs).permissions & 0o7777 + + tmp = dst + f".{secrets.token_hex(8)}.part".encode() + try: + await channel.copy( + lpath, tmp, follow_symlinks=True, remote_only=True + ) + await channel.setstat(tmp, asyncssh.SFTPAttrs(permissions=mode)) + try: + await channel.posix_rename(tmp, dst) + except SFTPOpUnsupported: + # SFTPv3 RENAME refuses existing destinations. + with suppress(SFTPNoSuchFile): + await channel.remove(dst) + await channel.rename(tmp, dst) + except BaseException: + with suppress(Exception): + await channel.remove(tmp) + raise + @wrap_exceptions async def _cp_file(self, lpath, rpath, **kwargs): # Server-side copy through the copy-data extension (asyncssh >= # 2.19 with an OpenSSH >= 9.0 server) needs no shell access and - # keeps the data on the server. remote_only guards against - # asyncssh silently copying through the client instead. The - # capability is per-connection, so it is cached after the first - # probe and the shell fallback never touches the channel pool - # again. Without the extension, fall back to a shell cp. + # keeps the data on the server. The capability is + # per-connection, so it is cached after the first probe and the + # shell fallback never touches the channel pool again. Without + # the extension, fall back to a shell cp. if self._supports_remote_copy is not False: async with self._pool.get() as channel: if self._supports_remote_copy is None: @@ -278,42 +338,7 @@ async def _cp_file(self, lpath, rpath, **kwargs): channel, "supports_remote_copy", False ) if self._supports_remote_copy: - # The remote copy handles only the plain - # file-to-file form. A directory destination means - # "copy into" with cp's resolution rules (including - # its same-file protections), so that form is - # delegated to the shell fallback below. - dst_attrs = None - with suppress(SFTPNoSuchFile): - dst_attrs = await channel.stat(rpath) - if dst_attrs is None or not stat.S_ISDIR( - dst_attrs.permissions - ): - # asyncssh opens the destination with truncation - # and no same-file check, so aliased paths (same - # path or a symlink to the source) would destroy - # the source; shell cp refuses them instead. - # Hardlink aliases cannot be detected over SFTP - # (no inode in attrs). - src, dst = await asyncio.gather( - channel.realpath(lpath), - channel.realpath(rpath), - ) - if src == dst: - raise shutil.SameFileError( - f"{lpath!r} and {rpath!r} " "are the same file" - ) - # preserve and follow_symlinks match what the - # shell cp fallback does: copy the link - # target's content and keep the source - # permissions. - return await channel.copy( - lpath, - rpath, - preserve=True, - follow_symlinks=True, - remote_only=True, - ) + return await self._remote_copy_file(channel, lpath, rpath) cmd = f"cp {shlex.quote(lpath)} {shlex.quote(rpath)}" await self._execute(cmd) From f975d8f4802f6ef892ba832ea0f2ec5d9ad8c849 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 17:41:07 -0700 Subject: [PATCH 08/21] tests: functional copy-data coverage via an asyncssh server The paramiko-based mockssh fixture cannot speak copy-data, so the semantics of the remote-copy path were only pinned through fakes. An asyncssh-based server fixture (which implements copy-data natively) now covers them end to end: content, mode rules for new and existing destinations, same-file refusal for str/bytes and symlink aliases, hardlink safety, and copy-into-directory resolution. The move fallback's source-preservation contract is pinned as well. strip_keys() returned None, so test_copy's metadata assertion compared None == None; it now returns the stripped dict. Co-Authored-By: Claude Fable 5 --- tests/conftest.py | 38 +++++++++++ tests/test_sshfs.py | 149 +++++++++++++++++++++++--------------------- 2 files changed, 115 insertions(+), 72 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index cd5eba3..ec385dd 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,45 @@ +import asyncio import threading +from pathlib import Path from queue import Queue +import asyncssh import mockssh.server +import pytest + +_STATIC = (Path(__file__).parent / "static").resolve() + + +class _NoAuthSSHServer(asyncssh.SSHServer): + def begin_auth(self, username): + return False + + +@pytest.fixture(scope="session") +def asyncssh_server(): + """SFTP server that, unlike the paramiko-based mockssh fixture, + implements the copy-data and limits extensions. Serves the local + filesystem as the current user; yields (host, port).""" + loop = asyncio.new_event_loop() + thread = threading.Thread(target=loop.run_forever, daemon=True) + thread.start() + + async def _listen(): + return await asyncssh.listen( + "127.0.0.1", + 0, + server_host_keys=[str(_STATIC / "user.key")], + server_factory=_NoAuthSSHServer, + sftp_factory=asyncssh.SFTPServer, + ) + + server = asyncio.run_coroutine_threadsafe(_listen(), loop).result(30) + try: + yield "127.0.0.1", server.get_port() + finally: + loop.call_soon_threadsafe(server.close) + loop.call_soon_threadsafe(loop.stop) + thread.join(timeout=5) def _handler_run(self): diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index d0ec9d8..7c188a0 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -1,4 +1,5 @@ import hashlib +import os import posixpath import secrets import shutil @@ -11,7 +12,7 @@ import fsspec import pytest -from asyncssh.sftp import SFTPAttrs, SFTPFailure, SFTPNoSuchFile +from asyncssh.sftp import SFTPAttrs, SFTPFailure, SFTPOpUnsupported from importlib_metadata import entry_points from sshfs import SSHFileSystem @@ -75,6 +76,7 @@ def fs_hard_queue(ssh_server, user="user"): def strip_keys(info): for key in ["name", "time", "mtime", "atime"]: info.pop(key, None) + return info def test_fsspec_registration(ssh_server): @@ -232,101 +234,104 @@ async def __aexit__(self, *exc): return _Ctx() -def test_cp_file_remote_copy(fs, monkeypatch): - # A channel advertising copy-data must be used with remote_only=True - # (otherwise asyncssh silently copies through the client), matching - # the shell fallback's semantics (content of the link target, - # source permissions), and the shell fallback must not run. - calls = [] - - class Channel: - supports_remote_copy = True - - async def stat(self, path): - raise SFTPNoSuchFile("destination does not exist") +@pytest.fixture +def copydata_fs(asyncssh_server): + host, port = asyncssh_server + yield SSHFileSystem(host=host, port=port, username="user") - async def realpath(self, path): - return "/real" + path - async def copy(self, lpath, rpath, **kwargs): - calls.append((lpath, rpath, kwargs)) +def test_cp_file_copy_data(copydata_fs, tmp_path): + fs = copydata_fs + src = tmp_path / "src" + src.write_bytes(b"payload") + src.chmod(0o640) - async def no_shell(*args, **kwargs): - raise AssertionError("shell fallback must not run") + dst = tmp_path / "dst" + fs.cp_file(str(src), str(dst)) + # the copy-data path was actually taken, not the shell fallback + assert fs._supports_remote_copy is True + assert dst.read_bytes() == b"payload" + # a new destination gets the source's mode + assert (dst.stat().st_mode & 0o7777) == 0o640 - monkeypatch.setattr(fs, "_supports_remote_copy", None) - monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) - monkeypatch.setattr(fs, "_execute", no_shell) + # an existing destination keeps its own mode, like cp + dst.chmod(0o600) + fs.cp_file(str(src), str(dst)) + assert dst.read_bytes() == b"payload" + assert (dst.stat().st_mode & 0o7777) == 0o600 - expected = { - "preserve": True, - "follow_symlinks": True, - "remote_only": True, - } - fs.cp_file("/src", "/dst") - assert calls == [("/src", "/dst", expected)] - # The probed capability is cached and reused. - fs.cp_file("/src2", "/dst2") - assert calls[-1] == ("/src2", "/dst2", expected) +def test_cp_file_copy_data_aliases(copydata_fs, tmp_path): + fs = copydata_fs + src = tmp_path / "src" + src.write_bytes(b"payload") + with pytest.raises(shutil.SameFileError): + fs.cp_file(str(src), str(src)) -def test_cp_file_same_file(fs, monkeypatch): - # Aliased source and destination (same path or a symlink to the - # source) must fail before any data is touched: asyncssh's copy - # opens the destination with truncation and would destroy the - # source, while shell cp refuses the copy. - class Channel: - supports_remote_copy = True + # bytes and str spellings of the same path are still aliases + with pytest.raises(shutil.SameFileError): + fs.cp_file(str(src).encode(), str(src)) - async def stat(self, path): - return SFTPAttrs(permissions=0o100644) + link = tmp_path / "link" + link.symlink_to(src) + with pytest.raises(shutil.SameFileError): + fs.cp_file(str(src), str(link)) - async def realpath(self, path): - return "/real/same" + # hardlink aliases cannot be detected over SFTP; the copy must + # still never destroy the source + hard = tmp_path / "hard" + os.link(src, hard) + fs.cp_file(str(src), str(hard)) + assert src.read_bytes() == b"payload" + assert hard.read_bytes() == b"payload" - async def copy(self, *args, **kwargs): - raise AssertionError("copy must not run on aliased paths") - async def no_shell(*args, **kwargs): - raise AssertionError("shell fallback must not run") +def test_cp_file_copy_data_directory_destination(copydata_fs, tmp_path): + fs = copydata_fs + src = tmp_path / "src" + src.write_bytes(b"payload") - monkeypatch.setattr(fs, "_supports_remote_copy", None) - monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) - monkeypatch.setattr(fs, "_execute", no_shell) + directory = tmp_path / "directory" + directory.mkdir() + fs.cp_file(str(src), str(directory)) + assert (directory / "src").read_bytes() == b"payload" + # "copy into" resolving to the source itself is an alias with pytest.raises(shutil.SameFileError): - fs.cp_file("/a", "/link-to-a") + fs.cp_file(str(directory / "src"), str(directory)) + assert (directory / "src").read_bytes() == b"payload" -def test_cp_file_directory_destination(fs, monkeypatch): - # A directory destination means "copy into" with cp's resolution - # rules (and its own same-file protections, e.g. - # cp_file("/dir/file", "/dir")), so it must take the shell path - # even when copy-data is available. - calls = [] - +def test_mv_fallback_keeps_source_on_copy_failure(fs, monkeypatch): + # When posix_rename is unsupported and the copy fails, the source + # must survive: it may only be removed after a successful copy. class Channel: - supports_remote_copy = True - - async def stat(self, path): - return SFTPAttrs(permissions=0o040755) + async def posix_rename(self, lpath, rpath): + raise SFTPOpUnsupported("posix-rename not supported") - async def realpath(self, path): - raise AssertionError("realpath not needed for dir targets") + removed = [] - async def copy(self, *args, **kwargs): - raise AssertionError("copy must not run for dir targets") + async def failing_cp(*args, **kwargs): + raise OSError("copy failed") - async def record_shell(cmd, **kwargs): - calls.append(cmd) + async def record_rm(path, **kwargs): + removed.append(path) - monkeypatch.setattr(fs, "_supports_remote_copy", None) monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) - monkeypatch.setattr(fs, "_execute", record_shell) + monkeypatch.setattr(fs, "_cp_file", failing_cp) + monkeypatch.setattr(fs, "_rm_file", record_rm) + + with pytest.raises(OSError): + fs.mv("/src", "/dst") + assert removed == [] + + async def ok_cp(*args, **kwargs): + pass - fs.cp_file("/dir/file", "/dir") - assert calls == ["cp /dir/file /dir"] + monkeypatch.setattr(fs, "_cp_file", ok_cp) + fs.mv("/src", "/dst") + assert removed == ["/src"] @pytest.mark.parametrize("legacy_asyncssh", [False, True]) From 90c03f8d6eaaf820d8c173af4b45ae595ad98e7a Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 18:27:50 -0700 Subject: [PATCH 09/21] Copy through the destination file instead of replacing it The temporary-name-and-rename strategy replaced the destination's directory entry, which is not what cp does and cannot be made safe: the temporary was created world-readable before its mode was set, the SFTPv3 rename fallback could destroy the old destination on failure, replacing the inode broke hardlink peers and dropped owner/xattrs, chmod to the full source mode bypassed the umask, canonicalized paths erased trailing-slash and dangling-symlink meaning, and the appended suffix could exceed the name limit. The copy-data request is now issued between open handles (FXF_WRITE|FXF_CREAT without FXF_TRUNC, truncated to the source length afterwards), which is cp's own contract: the destination inode with its mode, owner, xattrs and hardlink peers survives, a read-only destination is refused at open, a new file's mode is the source's (special bits stripped) filtered by the server's umask, and the requested path is used as given. It is also alias-safe by construction: copying a file onto itself through any alias -- path, symlink, or a hardlink, which carries no inode over SFTP and cannot be detected -- writes its bytes over themselves and the truncation to its own length changes nothing. Detectable aliases still raise SameFileError like cp refuses them. Known divergence: sparse sources are materialized densely; OpenSSH's copy-data reads and rewrites the whole range. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 100 ++++++++++++++++++++++++-------------------------- 1 file changed, 47 insertions(+), 53 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 30b079f..8a2c5a2 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -1,6 +1,5 @@ import asyncio import posixpath -import secrets import shlex import shutil import stat @@ -10,7 +9,7 @@ from typing import Optional import asyncssh -from asyncssh.sftp import SFTPNoSuchFile, SFTPOpUnsupported +from asyncssh.sftp import SFTPOpUnsupported from fsspec.asyn import ( AsyncFileSystem, FSTimeoutError, @@ -263,65 +262,60 @@ async def _get_file( ) async def _remote_copy_file(self, channel, lpath, rpath): - # copy-data must never be pointed at an aliased destination: - # asyncssh opens it with truncation and would destroy the - # source, where shell cp refuses the copy. The realpath - # comparison (normalized to bytes: realpath preserves the - # str/bytes type of its argument) catches path and symlink - # aliases; hardlinks carry no inode over SFTP and cannot be - # detected, so the data is written to a temporary name and - # renamed over the destination, which is safe for any alias. + # The data is written through the destination's existing file + # (FXF_CREAT without FXF_TRUNC, truncated to the source length + # afterwards), never by replacing its directory entry. That is + # cp's own contract: the destination inode with its mode, + # owner, xattrs and hardlink peers survives, a read-only + # destination is refused at open, and a new file's requested + # mode passes through the server's umask. It also makes + # aliases safe by construction -- copying a file onto itself + # writes its bytes over themselves and the final truncation to + # its own length changes nothing -- which matters because + # hardlink aliases carry no inode over SFTP and cannot be + # detected. Detectable aliases (path and symlink, compared via + # realpath normalized to bytes: realpath preserves the + # str/bytes type of its argument) are refused like cp refuses + # them. src, dst = await asyncio.gather( channel.realpath(lpath), channel.realpath(rpath) ) - src, dst = channel.encode(src), channel.encode(dst) - - # A directory destination means "copy into": like cp, resolve - # it against the source's basename. isdir() is False for - # missing paths and checks the file type on every SFTP version - # (v4+ attributes carry no type bits in `permissions`). - if await channel.isdir(dst): - dst = channel.encode( - await channel.realpath( - posixpath.join( - dst, posixpath.basename(channel.encode(lpath)) - ) - ) - ) - - if src == dst: + if channel.encode(src) == channel.encode(dst): raise shutil.SameFileError( f"{lpath!r} and {rpath!r} are the same file" ) - # cp keeps an existing destination's mode and gives a new file - # the source's mode (the remote umask is unknowable over SFTP - # and cannot be applied). The mode is set on the temporary file - # before the rename, so a failure never leaves the destination - # changed. Timestamps are deliberately not preserved, like cp. - src_attrs = await channel.stat(lpath) - dst_attrs = None - with suppress(SFTPNoSuchFile): - dst_attrs = await channel.stat(dst) - mode = (dst_attrs or src_attrs).permissions & 0o7777 - - tmp = dst + f".{secrets.token_hex(8)}.part".encode() - try: - await channel.copy( - lpath, tmp, follow_symlinks=True, remote_only=True + # A directory destination means "copy into": like cp, resolve + # it against the source's basename. The copy itself keeps the + # requested path -- canonicalizing it would change meaning for + # trailing slashes and symlinks. isdir() checks the file type + # on every SFTP version (v4+ attributes carry no type bits in + # `permissions`) and is False for missing paths. + if await channel.isdir(rpath): + rpath = posixpath.join( + channel.encode(rpath), + posixpath.basename(channel.encode(lpath)), ) - await channel.setstat(tmp, asyncssh.SFTPAttrs(permissions=mode)) - try: - await channel.posix_rename(tmp, dst) - except SFTPOpUnsupported: - # SFTPv3 RENAME refuses existing destinations. - with suppress(SFTPNoSuchFile): - await channel.remove(dst) - await channel.rename(tmp, dst) - except BaseException: - with suppress(Exception): - await channel.remove(tmp) - raise + resolved = await channel.realpath(rpath) + if channel.encode(src) == channel.encode(resolved): + raise shutil.SameFileError( + f"{lpath!r} and {rpath!r} are the same file" + ) + + async with channel.open(lpath, "rb", block_size=0) as src_file: + src_attrs = await src_file.stat() + # Like cp for new files: special bits stripped, and the + # server applies its umask to the requested mode. Existing + # destinations keep their attributes untouched. + mode = (src_attrs.permissions or 0o666) & 0o777 + async with channel.open( + rpath, + asyncssh.FXF_WRITE | asyncssh.FXF_CREAT, + asyncssh.SFTPAttrs(permissions=mode), + block_size=0, + ) as dst_file: + await channel.remote_copy(src_file, dst_file) + await dst_file.truncate(src_attrs.size) @wrap_exceptions async def _cp_file(self, lpath, rpath, **kwargs): From 8715bd1118cee3c13603d53d5201d95cc7c59564 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 18:27:50 -0700 Subject: [PATCH 10/21] Map SFTPPermissionDenied to PermissionError Only the auth-level PermissionDenied was translated; SFTP-level permission errors (e.g. opening a read-only file for write) leaked the asyncssh exception through the public API. Co-Authored-By: Claude Fable 5 --- sshfs/utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sshfs/utils.py b/sshfs/utils.py index fb2b02d..e504961 100644 --- a/sshfs/utils.py +++ b/sshfs/utils.py @@ -4,7 +4,7 @@ from asyncssh import ProcessError from asyncssh.misc import PermissionDenied -from asyncssh.sftp import SFTPFailure, SFTPNoSuchFile +from asyncssh.sftp import SFTPFailure, SFTPNoSuchFile, SFTPPermissionDenied from fsspec.asyn import sync_wrapper _NOT_FOUND = os.strerror(errno.ENOENT) @@ -27,6 +27,8 @@ async def wrapper(*args, **kwargs): return await func(*args, **kwargs) except PermissionDenied as exc: raise PermissionError(exc.reason) from exc + except SFTPPermissionDenied as exc: + raise PermissionError(errno.EACCES, exc.reason) from exc except SFTPNoSuchFile as exc: raise FileNotFoundError(errno.ENOENT, _NOT_FOUND) from exc except ProcessError as exc: From 415128c4eba6d76cc9002e314fa7e611953e1362 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 18:27:50 -0700 Subject: [PATCH 11/21] tests: chroot and authenticate the asyncssh fixture; pin write-through semantics The fixture no longer serves the whole filesystem without authentication: it is chrooted to a fresh directory, requires the test user key, and shuts down by closing the client connection and draining the loop's tasks (the suite previously ended with 'Task was destroyed but it is pending!'). The functional tests now pin the write-through contract: hardlink peers of the destination see updates and source hardlinks keep their inode, an existing destination keeps its mode, a new file's mode is umask-filtered, a read-only destination is refused and unchanged, and a trailing slash on a file destination is rejected. Co-Authored-By: Claude Fable 5 --- tests/conftest.py | 48 ++++++++++++---- tests/test_sshfs.py | 136 ++++++++++++++++++++++++++++++-------------- 2 files changed, 131 insertions(+), 53 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index ec385dd..3f244c1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ import asyncio import threading +from contextlib import suppress from pathlib import Path from queue import Queue @@ -8,18 +9,27 @@ import pytest _STATIC = (Path(__file__).parent / "static").resolve() +_USER_KEY = asyncssh.read_private_key(str(_STATIC / "user.key")) -class _NoAuthSSHServer(asyncssh.SSHServer): +class _TestSSHServer(asyncssh.SSHServer): def begin_auth(self, username): - return False + return True + + def public_key_auth_supported(self): + return True + + def validate_public_key(self, username, key): + return key == _USER_KEY.convert_to_public() @pytest.fixture(scope="session") -def asyncssh_server(): +def asyncssh_server(tmp_path_factory): """SFTP server that, unlike the paramiko-based mockssh fixture, - implements the copy-data and limits extensions. Serves the local - filesystem as the current user; yields (host, port).""" + implements the copy-data and limits extensions. Authenticated with + the test user key and chrooted to a fresh directory; yields + (host, port, root) where the remote "/" maps to root.""" + root = tmp_path_factory.mktemp("asyncssh-root") loop = asyncio.new_event_loop() thread = threading.Thread(target=loop.run_forever, daemon=True) thread.start() @@ -28,18 +38,36 @@ async def _listen(): return await asyncssh.listen( "127.0.0.1", 0, - server_host_keys=[str(_STATIC / "user.key")], - server_factory=_NoAuthSSHServer, - sftp_factory=asyncssh.SFTPServer, + server_host_keys=[_USER_KEY], + server_factory=_TestSSHServer, + sftp_factory=lambda chan: asyncssh.SFTPServer( + chan, chroot=str(root) + ), ) server = asyncio.run_coroutine_threadsafe(_listen(), loop).result(30) try: - yield "127.0.0.1", server.get_port() + yield "127.0.0.1", server.get_port(), root finally: - loop.call_soon_threadsafe(server.close) + + async def _shutdown(): + server.close() + await server.wait_closed() + tasks = [ + task + for task in asyncio.all_tasks() + if task is not asyncio.current_task() + ] + for task in tasks: + task.cancel() + if tasks: + await asyncio.wait(tasks, timeout=5) + + with suppress(Exception): + asyncio.run_coroutine_threadsafe(_shutdown(), loop).result(30) loop.call_soon_threadsafe(loop.stop) thread.join(timeout=5) + loop.close() def _handler_run(self): diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 7c188a0..0507564 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -6,13 +6,15 @@ import tempfile import warnings from concurrent import futures +from contextlib import suppress from datetime import datetime, timedelta, timezone from pathlib import Path from types import SimpleNamespace import fsspec import pytest -from asyncssh.sftp import SFTPAttrs, SFTPFailure, SFTPOpUnsupported +from asyncssh.sftp import SFTPAttrs, SFTPError, SFTPFailure, SFTPOpUnsupported +from fsspec.asyn import sync from importlib_metadata import entry_points from sshfs import SSHFileSystem @@ -234,73 +236,121 @@ async def __aexit__(self, *exc): return _Ctx() -@pytest.fixture +@pytest.fixture(scope="session") def copydata_fs(asyncssh_server): - host, port = asyncssh_server - yield SSHFileSystem(host=host, port=port, username="user") + host, port, _root = asyncssh_server + fs = SSHFileSystem( + host=host, + port=port, + username="user", + client_keys=[USERS["user"]], + ) + yield fs + # Close the connection so the server fixture can shut its loop + # down without cancelling live connection tasks. + with suppress(Exception): + sync(fs.loop, fs._stack.aclose, timeout=5) -def test_cp_file_copy_data(copydata_fs, tmp_path): - fs = copydata_fs - src = tmp_path / "src" - src.write_bytes(b"payload") - src.chmod(0o640) +@pytest.fixture +def copydata_dir(asyncssh_server, request): + _host, _port, root = asyncssh_server + local = root / request.node.name + local.mkdir() + # the server is chrooted to `root`, so `local` is served as this + # remote path + yield local, "/" + local.name - dst = tmp_path / "dst" - fs.cp_file(str(src), str(dst)) - # the copy-data path was actually taken, not the shell fallback - assert fs._supports_remote_copy is True - assert dst.read_bytes() == b"payload" - # a new destination gets the source's mode - assert (dst.stat().st_mode & 0o7777) == 0o640 - # an existing destination keeps its own mode, like cp - dst.chmod(0o600) - fs.cp_file(str(src), str(dst)) - assert dst.read_bytes() == b"payload" - assert (dst.stat().st_mode & 0o7777) == 0o600 +def test_cp_file_copy_data(copydata_fs, copydata_dir): + fs = copydata_fs + local, remote = copydata_dir + (local / "src").write_bytes(b"payload") + (local / "src").chmod(0o666) + umask = os.umask(0) + os.umask(umask) -def test_cp_file_copy_data_aliases(copydata_fs, tmp_path): + fs.cp_file(remote + "/src", remote + "/dst") + # the copy-data path was actually taken, not the shell fallback + assert fs._supports_remote_copy is True + assert (local / "dst").read_bytes() == b"payload" + # a new file gets the source's mode filtered by the server's + # umask, like cp + assert ((local / "dst").stat().st_mode & 0o7777) == 0o666 & ~umask + + # an existing destination keeps its inode: its own mode survives + # and hardlink peers see the update, like cp writing through the + # file + (local / "dst").chmod(0o600) + os.link(local / "dst", local / "peer") + (local / "src").write_bytes(b"new payload") + fs.cp_file(remote + "/src", remote + "/dst") + assert (local / "dst").read_bytes() == b"new payload" + assert ((local / "dst").stat().st_mode & 0o7777) == 0o600 + assert (local / "peer").read_bytes() == b"new payload" + + +def test_cp_file_copy_data_aliases(copydata_fs, copydata_dir): fs = copydata_fs - src = tmp_path / "src" + local, remote = copydata_dir + src = local / "src" src.write_bytes(b"payload") with pytest.raises(shutil.SameFileError): - fs.cp_file(str(src), str(src)) + fs.cp_file(remote + "/src", remote + "/src") # bytes and str spellings of the same path are still aliases with pytest.raises(shutil.SameFileError): - fs.cp_file(str(src).encode(), str(src)) + fs.cp_file((remote + "/src").encode(), remote + "/src") - link = tmp_path / "link" - link.symlink_to(src) + (local / "link").symlink_to("src") with pytest.raises(shutil.SameFileError): - fs.cp_file(str(src), str(link)) + fs.cp_file(remote + "/src", remote + "/link") - # hardlink aliases cannot be detected over SFTP; the copy must - # still never destroy the source - hard = tmp_path / "hard" - os.link(src, hard) - fs.cp_file(str(src), str(hard)) + # hardlink aliases cannot be detected over SFTP: the write-through + # copy puts the bytes over themselves and must leave the file, + # its content and the link intact + os.link(src, local / "hard") + fs.cp_file(remote + "/src", remote + "/hard") assert src.read_bytes() == b"payload" - assert hard.read_bytes() == b"payload" + assert (local / "hard").stat().st_ino == src.stat().st_ino -def test_cp_file_copy_data_directory_destination(copydata_fs, tmp_path): +def test_cp_file_copy_data_directory_destination(copydata_fs, copydata_dir): fs = copydata_fs - src = tmp_path / "src" - src.write_bytes(b"payload") + local, remote = copydata_dir + (local / "src").write_bytes(b"payload") + (local / "d").mkdir() - directory = tmp_path / "directory" - directory.mkdir() - fs.cp_file(str(src), str(directory)) - assert (directory / "src").read_bytes() == b"payload" + fs.cp_file(remote + "/src", remote + "/d") + assert (local / "d" / "src").read_bytes() == b"payload" # "copy into" resolving to the source itself is an alias with pytest.raises(shutil.SameFileError): - fs.cp_file(str(directory / "src"), str(directory)) - assert (directory / "src").read_bytes() == b"payload" + fs.cp_file(remote + "/d/src", remote + "/d") + assert (local / "d" / "src").read_bytes() == b"payload" + + +def test_cp_file_copy_data_destination_errors(copydata_fs, copydata_dir): + fs = copydata_fs + local, remote = copydata_dir + (local / "src").write_bytes(b"payload") + + # a read-only destination is refused at open, like cp, and stays + # untouched + ro = local / "ro" + ro.write_bytes(b"old") + ro.chmod(0o444) + with pytest.raises(PermissionError): + fs.cp_file(remote + "/src", remote + "/ro") + assert ro.read_bytes() == b"old" + + # a trailing slash on a file destination is not a directory (the + # server rejects it; like mkdir, the SFTP error is passed through) + with pytest.raises((OSError, SFTPError)): + fs.cp_file(remote + "/src", remote + "/ro/") + assert ro.read_bytes() == b"old" def test_mv_fallback_keeps_source_on_copy_failure(fs, monkeypatch): From a8e25125f227c5ad0a2f1ee8ebab349358c654f1 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 19:11:07 -0700 Subject: [PATCH 12/21] Only create new destinations over copy-data In-place overwrite over SFTP is unsolvable and is deliberately not implemented: a stale pre-copy size corrupts sources whose stat lies (procfs) or that change while copying, a post-copy truncate can be denied by fsetstat policy after the destination was already modified, a pre-copy truncate destroys sources aliased behind an undetectable hardlink, and replacing the directory entry loses the inode. The extension path now opens the source, creates the destination with FXF_EXCL (source mode without special bits, filtered by the server's umask; a valid mode of 0 is kept -- only a missing mode falls back to the server default) and copies until the source's end of file, with no size assumptions and no post-write fix-ups. FXF_EXCL also refuses to create through a dangling symlink and guarantees a failed copy is cleaned up completely: the file it made is ours to remove. Existing destinations -- which include every alias of the source -- and every other form go to the shell fallback, whose cp implements those semantics natively. When the server advertises copy-data but denies the request (an OpenSSH allow/deny policy), the created file is removed, the capability is re-cached as unsupported and the copy falls back to the shell. The shell command now also accepts bytes paths by decoding them. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 127 +++++++++++++++++++++++++++++++------------------- 1 file changed, 78 insertions(+), 49 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 8a2c5a2..51afac8 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -1,7 +1,6 @@ import asyncio import posixpath import shlex -import shutil import stat import weakref from contextlib import AsyncExitStack, suppress @@ -9,7 +8,7 @@ from typing import Optional import asyncssh -from asyncssh.sftp import SFTPOpUnsupported +from asyncssh.sftp import SFTPError, SFTPOpUnsupported, SFTPPermissionDenied from fsspec.asyn import ( AsyncFileSystem, FSTimeoutError, @@ -262,60 +261,80 @@ async def _get_file( ) async def _remote_copy_file(self, channel, lpath, rpath): - # The data is written through the destination's existing file - # (FXF_CREAT without FXF_TRUNC, truncated to the source length - # afterwards), never by replacing its directory entry. That is - # cp's own contract: the destination inode with its mode, - # owner, xattrs and hardlink peers survives, a read-only - # destination is refused at open, and a new file's requested - # mode passes through the server's umask. It also makes - # aliases safe by construction -- copying a file onto itself - # writes its bytes over themselves and the final truncation to - # its own length changes nothing -- which matters because - # hardlink aliases carry no inode over SFTP and cannot be - # detected. Detectable aliases (path and symlink, compared via - # realpath normalized to bytes: realpath preserves the - # str/bytes type of its argument) are refused like cp refuses - # them. - src, dst = await asyncio.gather( - channel.realpath(lpath), channel.realpath(rpath) - ) - if channel.encode(src) == channel.encode(dst): - raise shutil.SameFileError( - f"{lpath!r} and {rpath!r} are the same file" - ) + """Copy over the copy-data extension. Returns False when the + copy must be handled by the shell fallback instead.""" + # The remote copy only ever CREATES the destination (FXF_EXCL) + # and copies until the source's end of file. Everything an + # in-place overwrite would need is unsolvable over SFTP: a + # stale pre-copy size corrupts sources whose stat lies (procfs) + # or that change while copying, a post-copy truncate can be + # denied (fsetstat policy) after the destination was already + # modified, a pre-copy truncate destroys sources aliased behind + # an undetectable hardlink, and replacing the directory entry + # loses the inode. Existing destinations therefore go to the + # shell fallback, whose cp implements those semantics natively. + # FXF_EXCL also refuses to create through a dangling symlink, + # and guarantees a failed copy can be cleaned up completely -- + # the file it made is ours. # A directory destination means "copy into": like cp, resolve - # it against the source's basename. The copy itself keeps the - # requested path -- canonicalizing it would change meaning for - # trailing slashes and symlinks. isdir() checks the file type - # on every SFTP version (v4+ attributes carry no type bits in - # `permissions`) and is False for missing paths. + # it against the source's basename. isdir() checks the file + # type on every SFTP version (v4+ attributes carry no type bits + # in `permissions`) and is False for missing paths. if await channel.isdir(rpath): rpath = posixpath.join( channel.encode(rpath), posixpath.basename(channel.encode(lpath)), ) - resolved = await channel.realpath(rpath) - if channel.encode(src) == channel.encode(resolved): - raise shutil.SameFileError( - f"{lpath!r} and {rpath!r} are the same file" - ) + # The source is opened before the destination is created so + # that a missing source cannot leave an empty destination. async with channel.open(lpath, "rb", block_size=0) as src_file: src_attrs = await src_file.stat() # Like cp for new files: special bits stripped, and the - # server applies its umask to the requested mode. Existing - # destinations keep their attributes untouched. - mode = (src_attrs.permissions or 0o666) & 0o777 - async with channel.open( - rpath, - asyncssh.FXF_WRITE | asyncssh.FXF_CREAT, - asyncssh.SFTPAttrs(permissions=mode), - block_size=0, - ) as dst_file: + # server applies its umask to the requested mode. A mode of + # 0 is a valid mode, only a missing one falls back to the + # server default. + if src_attrs.permissions is None: + attrs = asyncssh.SFTPAttrs() + else: + attrs = asyncssh.SFTPAttrs( + permissions=src_attrs.permissions & 0o777 + ) + try: + dst_file = await channel.open( + rpath, + asyncssh.FXF_WRITE + | asyncssh.FXF_CREAT + | asyncssh.FXF_EXCL, + attrs, + block_size=0, + ) + except (OSError, SFTPError): + # Existing destination (any alias of the source is one), + # dangling symlink, missing parent, trailing slash on a + # file: the shell fallback owns these forms. + return False + + try: await channel.remote_copy(src_file, dst_file) - await dst_file.truncate(src_attrs.size) + except (SFTPOpUnsupported, SFTPPermissionDenied): + # Advertised but denied (e.g. an OpenSSH allow/deny + # policy): remove the file we created and stop trying + # the extension on this connection. + await dst_file.close() + with suppress(OSError, SFTPError): + await channel.remove(rpath) + self._supports_remote_copy = False + return False + except BaseException: + await dst_file.close() + with suppress(OSError, SFTPError): + await channel.remove(rpath) + raise + else: + await dst_file.close() + return True @wrap_exceptions async def _cp_file(self, lpath, rpath, **kwargs): @@ -323,17 +342,27 @@ async def _cp_file(self, lpath, rpath, **kwargs): # 2.19 with an OpenSSH >= 9.0 server) needs no shell access and # keeps the data on the server. The capability is # per-connection, so it is cached after the first probe and the - # shell fallback never touches the channel pool again. Without - # the extension, fall back to a shell cp. + # shell fallback never touches the channel pool again. The + # extension path only creates new destinations; everything else + # falls back to a shell cp. if self._supports_remote_copy is not False: async with self._pool.get() as channel: if self._supports_remote_copy is None: self._supports_remote_copy = getattr( channel, "supports_remote_copy", False ) - if self._supports_remote_copy: - return await self._remote_copy_file(channel, lpath, rpath) - + if ( + self._supports_remote_copy + and await self._remote_copy_file(channel, lpath, rpath) + ): + return + + # The shell command needs text; bytes paths (accepted by the + # SFTP operations) are decoded with the SFTP default encoding. + if isinstance(lpath, bytes): + lpath = lpath.decode("utf-8") + if isinstance(rpath, bytes): + rpath = rpath.decode("utf-8") cmd = f"cp {shlex.quote(lpath)} {shlex.quote(rpath)}" await self._execute(cmd) From d1eae3184c81a75a9a67f926bf4dc372e2bf6877 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 19:11:07 -0700 Subject: [PATCH 13/21] tests: pin the create-only copy-data contract Functional coverage against the chrooted asyncssh server: created files carry the umask-filtered source mode, copy-into-directory creates the source's basename, existing destinations and every alias of the source fail untouched when no shell is available, and a dangling destination symlink is refused instead of creating its target. The tests are capability-skipped on asyncssh < 2.19 (the runtime correctly uses the shell there, which this fixture does not offer) and the per-test directories are unique so pytest-rerunfailures retries never collide. Policy-denied copy-data (advertised but refused) is unit-tested: artifact removed, capability re-cached, shell attempted. Co-Authored-By: Claude Fable 5 --- tests/test_sshfs.py | 164 +++++++++++++++++++++++++++++--------------- 1 file changed, 110 insertions(+), 54 deletions(-) diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 0507564..806f075 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -2,7 +2,6 @@ import os import posixpath import secrets -import shutil import tempfile import warnings from concurrent import futures @@ -13,7 +12,14 @@ import fsspec import pytest -from asyncssh.sftp import SFTPAttrs, SFTPError, SFTPFailure, SFTPOpUnsupported +from asyncssh.misc import ChannelOpenError +from asyncssh.sftp import ( + SFTPAttrs, + SFTPClient, + SFTPFailure, + SFTPOpUnsupported, + SFTPPermissionDenied, +) from fsspec.asyn import sync from importlib_metadata import entry_points @@ -255,14 +261,23 @@ def copydata_fs(asyncssh_server): @pytest.fixture def copydata_dir(asyncssh_server, request): _host, _port, root = asyncssh_server - local = root / request.node.name + # unique per invocation so pytest-rerunfailures retries get a + # fresh directory + local = root / f"{request.node.name}-{secrets.token_hex(4)}" local.mkdir() # the server is chrooted to `root`, so `local` is served as this # remote path yield local, "/" + local.name -def test_cp_file_copy_data(copydata_fs, copydata_dir): +requires_copy_data = pytest.mark.skipif( + not hasattr(SFTPClient, "supports_remote_copy"), + reason="asyncssh without copy-data support (< 2.19)", +) + + +@requires_copy_data +def test_cp_file_copy_data_creates(copydata_fs, copydata_dir): fs = copydata_fs local, remote = copydata_dir (local / "src").write_bytes(b"payload") @@ -279,78 +294,119 @@ def test_cp_file_copy_data(copydata_fs, copydata_dir): # umask, like cp assert ((local / "dst").stat().st_mode & 0o7777) == 0o666 & ~umask - # an existing destination keeps its inode: its own mode survives - # and hardlink peers see the update, like cp writing through the - # file - (local / "dst").chmod(0o600) - os.link(local / "dst", local / "peer") - (local / "src").write_bytes(b"new payload") - fs.cp_file(remote + "/src", remote + "/dst") - assert (local / "dst").read_bytes() == b"new payload" - assert ((local / "dst").stat().st_mode & 0o7777) == 0o600 - assert (local / "peer").read_bytes() == b"new payload" + # "copy into" an existing directory creates the source's basename + (local / "d").mkdir() + fs.cp_file(remote + "/src", remote + "/d") + assert (local / "d" / "src").read_bytes() == b"payload" -def test_cp_file_copy_data_aliases(copydata_fs, copydata_dir): +@requires_copy_data +def test_cp_file_copy_data_existing_destinations(copydata_fs, copydata_dir): + # The extension path only creates destinations. Anything existing + # -- including every alias of the source -- is left to the shell + # fallback, which this server does not offer: the copy must fail + # without touching a single byte. fs = copydata_fs local, remote = copydata_dir src = local / "src" src.write_bytes(b"payload") - with pytest.raises(shutil.SameFileError): - fs.cp_file(remote + "/src", remote + "/src") + (local / "existing").write_bytes(b"old") + (local / "link").symlink_to("src") + os.link(src, local / "hard") - # bytes and str spellings of the same path are still aliases - with pytest.raises(shutil.SameFileError): + for dst in ["/src", "/existing", "/link", "/hard"]: + with pytest.raises((OSError, ChannelOpenError)): + fs.cp_file(remote + "/src", remote + dst) + with pytest.raises((OSError, ChannelOpenError)): fs.cp_file((remote + "/src").encode(), remote + "/src") - (local / "link").symlink_to("src") - with pytest.raises(shutil.SameFileError): - fs.cp_file(remote + "/src", remote + "/link") - - # hardlink aliases cannot be detected over SFTP: the write-through - # copy puts the bytes over themselves and must leave the file, - # its content and the link intact - os.link(src, local / "hard") - fs.cp_file(remote + "/src", remote + "/hard") assert src.read_bytes() == b"payload" + assert (local / "existing").read_bytes() == b"old" assert (local / "hard").stat().st_ino == src.stat().st_ino + # "copy into" resolving to an existing file is refused the same way + (local / "d").mkdir() + os.link(src, local / "d" / "src") + with pytest.raises((OSError, ChannelOpenError)): + fs.cp_file(remote + "/src", remote + "/d") + assert (local / "d" / "src").read_bytes() == b"payload" -def test_cp_file_copy_data_directory_destination(copydata_fs, copydata_dir): +@requires_copy_data +def test_cp_file_copy_data_never_redirects(copydata_fs, copydata_dir): + # FXF_EXCL refuses to create through a dangling symlink, so the + # copy cannot be redirected to the link's target. (Trailing-slash + # handling is the server's path resolution and is not asserted: + # this server normalizes it away, OpenSSH rejects it.) fs = copydata_fs local, remote = copydata_dir (local / "src").write_bytes(b"payload") - (local / "d").mkdir() + (local / "dangling").symlink_to("missing") - fs.cp_file(remote + "/src", remote + "/d") - assert (local / "d" / "src").read_bytes() == b"payload" + with pytest.raises((OSError, ChannelOpenError)): + fs.cp_file(remote + "/src", remote + "/dangling") + assert not (local / "missing").exists() - # "copy into" resolving to the source itself is an alias - with pytest.raises(shutil.SameFileError): - fs.cp_file(remote + "/d/src", remote + "/d") - assert (local / "d" / "src").read_bytes() == b"payload" +def test_cp_file_copy_data_denied(fs, monkeypatch): + # copy-data advertised but denied by server policy: the created + # destination is removed, the capability is re-cached as + # unsupported, and the copy falls back to the shell. + events = [] -def test_cp_file_copy_data_destination_errors(copydata_fs, copydata_dir): - fs = copydata_fs - local, remote = copydata_dir - (local / "src").write_bytes(b"payload") + class _File: + async def stat(self): + return SFTPAttrs(permissions=0o100644) - # a read-only destination is refused at open, like cp, and stays - # untouched - ro = local / "ro" - ro.write_bytes(b"old") - ro.chmod(0o444) - with pytest.raises(PermissionError): - fs.cp_file(remote + "/src", remote + "/ro") - assert ro.read_bytes() == b"old" - - # a trailing slash on a file destination is not a directory (the - # server rejects it; like mkdir, the SFTP error is passed through) - with pytest.raises((OSError, SFTPError)): - fs.cp_file(remote + "/src", remote + "/ro/") - assert ro.read_bytes() == b"old" + async def close(self): + events.append("close") + + class _OpenResult: + def __init__(self): + self.file = _File() + + def __await__(self): + async def _result(): + return self.file + + return _result().__await__() + + async def __aenter__(self): + return self.file + + async def __aexit__(self, *exc): + return False + + class Channel: + supports_remote_copy = True + + def encode(self, path): + return path.encode() if isinstance(path, str) else path + + async def isdir(self, path): + return False + + def open(self, path, *args, **kwargs): + events.append(("open", path)) + return _OpenResult() + + async def remote_copy(self, src, dst): + raise SFTPPermissionDenied("denied by policy") + + async def remove(self, path): + events.append(("remove", path)) + + async def record_shell(cmd, **kwargs): + events.append(("shell", cmd)) + + monkeypatch.setattr(fs, "_supports_remote_copy", None) + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + monkeypatch.setattr(fs, "_execute", record_shell) + + fs.cp_file("/src", "/dst") + assert ("remove", "/dst") in events + assert ("shell", "cp /src /dst") in events + assert fs._supports_remote_copy is False def test_mv_fallback_keeps_source_on_copy_failure(fs, monkeypatch): From 18f1cbd2ef2808052f8d187d9bf429faee604340 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 21:06:45 -0700 Subject: [PATCH 14/21] Never unlink on failure; tolerate fstat denial FXF_EXCL proves this client created the inode, but over SFTP the pathname cannot be re-verified to still name that inode at cleanup time: another actor can rename the created file away and put an unrelated file on the same name, and the failure cleanup would delete it. Failure cleanup by unlink is therefore removed entirely -- a failed or interrupted copy may leave a partial destination behind, exactly like an interrupted cp, and a disconnect could never have cleaned up anyway. The policy-denial fallback no longer needs removal either: the shell cp simply overwrites the empty exclusively-created file. The source fstat is now best-effort: servers can deny individual requests (OpenSSH sftp-server -P fstat) while still allowing copy-data, so a denied mode lookup falls back to the server's default creation attributes instead of failing the copy. Destination close failures after writing surface (the data may not be durable) but can no longer mask the copy error, and source close failures never fail a completed copy. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 58 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 51afac8..214be79 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -273,9 +273,14 @@ async def _remote_copy_file(self, channel, lpath, rpath): # an undetectable hardlink, and replacing the directory entry # loses the inode. Existing destinations therefore go to the # shell fallback, whose cp implements those semantics natively. - # FXF_EXCL also refuses to create through a dangling symlink, - # and guarantees a failed copy can be cleaned up completely -- - # the file it made is ours. + # FXF_EXCL also refuses to create through a dangling symlink. + # + # A failed or interrupted copy may leave a partial destination + # behind, exactly like an interrupted cp. It is deliberately + # never unlinked: FXF_EXCL proves this client created the + # inode, but over SFTP the pathname cannot be re-verified to + # still name that inode at cleanup time, so removing it could + # delete an unrelated file that raced onto the same name. # A directory destination means "copy into": like cp, resolve # it against the source's basename. isdir() checks the file @@ -289,18 +294,19 @@ async def _remote_copy_file(self, channel, lpath, rpath): # The source is opened before the destination is created so # that a missing source cannot leave an empty destination. - async with channel.open(lpath, "rb", block_size=0) as src_file: - src_attrs = await src_file.stat() + src_file = await channel.open(lpath, "rb", block_size=0) + try: # Like cp for new files: special bits stripped, and the # server applies its umask to the requested mode. A mode of - # 0 is a valid mode, only a missing one falls back to the - # server default. - if src_attrs.permissions is None: - attrs = asyncssh.SFTPAttrs() - else: - attrs = asyncssh.SFTPAttrs( - permissions=src_attrs.permissions & 0o777 - ) + # 0 is a valid mode; a missing one -- or a server that + # denies fstat -- falls back to the server default. + attrs = asyncssh.SFTPAttrs() + with suppress(OSError, SFTPError): + src_attrs = await src_file.stat() + if src_attrs.permissions is not None: + attrs = asyncssh.SFTPAttrs( + permissions=src_attrs.permissions & 0o777 + ) try: dst_file = await channel.open( rpath, @@ -320,20 +326,23 @@ async def _remote_copy_file(self, channel, lpath, rpath): await channel.remote_copy(src_file, dst_file) except (SFTPOpUnsupported, SFTPPermissionDenied): # Advertised but denied (e.g. an OpenSSH allow/deny - # policy): remove the file we created and stop trying - # the extension on this connection. - await dst_file.close() - with suppress(OSError, SFTPError): - await channel.remove(rpath) + # policy): stop trying the extension on this connection + # and let the shell cp overwrite the empty file just + # created. self._supports_remote_copy = False + with suppress(OSError, SFTPError): + await dst_file.close() return False except BaseException: - await dst_file.close() with suppress(OSError, SFTPError): - await channel.remove(rpath) + await dst_file.close() raise - else: - await dst_file.close() + # A close failure after writing must surface: the data may + # not be durable. + await dst_file.close() + finally: + with suppress(OSError, SFTPError): + await src_file.close() return True @wrap_exceptions @@ -357,8 +366,9 @@ async def _cp_file(self, lpath, rpath, **kwargs): ): return - # The shell command needs text; bytes paths (accepted by the - # SFTP operations) are decoded with the SFTP default encoding. + # The shell command needs text: bytes paths are decoded as + # UTF-8. Non-UTF-8 byte paths cannot ride a shell command and + # only work on operations that stay on the SFTP channel. if isinstance(lpath, bytes): lpath = lpath.decode("utf-8") if isinstance(rpath, bytes): From eac44d38084c4fb5b6ce6fd844580f1ba106b241 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 21:06:45 -0700 Subject: [PATCH 15/21] tests: run the copy-data contract on SFTP v3 and v4 v4+ separates the file type from permissions, so the functional suite now runs against both protocol generations. The denial test reflects that the created empty file is left for the shell to overwrite rather than removed, and a functional test pins that a move between two hardlinks of the same inode is a POSIX rename no-op with both names surviving. Co-Authored-By: Claude Fable 5 --- tests/test_sshfs.py | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 806f075..b581cc9 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -242,14 +242,20 @@ async def __aexit__(self, *exc): return _Ctx() -@pytest.fixture(scope="session") -def copydata_fs(asyncssh_server): +@pytest.fixture(scope="session", params=[None, 4], ids=["sftpv3", "sftpv4"]) +def copydata_fs(asyncssh_server, request): + # v4+ separates the file type from `permissions`, so both protocol + # generations must satisfy the same contract. host, port, _root = asyncssh_server + extra = {} + if request.param is not None: + extra["sftp_client_kwargs"] = {"sftp_version": request.param} fs = SSHFileSystem( host=host, port=port, username="user", client_keys=[USERS["user"]], + **extra, ) yield fs # Close the connection so the server fixture can shut its loop @@ -348,10 +354,24 @@ def test_cp_file_copy_data_never_redirects(copydata_fs, copydata_dir): assert not (local / "missing").exists() +def test_mv_hardlink_alias(copydata_fs, copydata_dir): + # POSIX rename between two names of the same inode is a no-op: + # the move succeeds with both names surviving and no data lost. + fs = copydata_fs + local, remote = copydata_dir + src = local / "src" + src.write_bytes(b"payload") + os.link(src, local / "hard") + + fs.mv(remote + "/src", remote + "/hard") + assert src.read_bytes() == b"payload" + assert (local / "hard").read_bytes() == b"payload" + + def test_cp_file_copy_data_denied(fs, monkeypatch): - # copy-data advertised but denied by server policy: the created - # destination is removed, the capability is re-cached as - # unsupported, and the copy falls back to the shell. + # copy-data advertised but denied by server policy: the capability + # is re-cached as unsupported and the copy falls back to the shell, + # which overwrites the empty file created by the exclusive open. events = [] class _File: @@ -393,9 +413,6 @@ def open(self, path, *args, **kwargs): async def remote_copy(self, src, dst): raise SFTPPermissionDenied("denied by policy") - async def remove(self, path): - events.append(("remove", path)) - async def record_shell(cmd, **kwargs): events.append(("shell", cmd)) @@ -404,7 +421,6 @@ async def record_shell(cmd, **kwargs): monkeypatch.setattr(fs, "_execute", record_shell) fs.cp_file("/src", "/dst") - assert ("remove", "/dst") in events assert ("shell", "cp /src /dst") in events assert fs._supports_remote_copy is False From 0c2c7c786b2dcfe530ef74415f5d2af63feb422d Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 21:11:37 -0700 Subject: [PATCH 16/21] Give non-UTF-8 byte paths a clear shell-fallback error Byte paths are decoded as UTF-8 for the shell command; a path that is not valid UTF-8 now explains that it can only be used with the operations that stay on the SFTP channel, instead of surfacing a bare UnicodeDecodeError from shlex. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 214be79..897734b 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -260,6 +260,24 @@ async def _get_file( progress_handler=as_progress_handler(callback), ) + @staticmethod + def _shell_paths(*paths): + # Shell commands are text. Byte paths are decoded as UTF-8; + # paths that are not valid UTF-8 can only be used with the + # operations that stay on the SFTP channel. + decoded = [] + for path in paths: + if isinstance(path, bytes): + try: + path = path.decode("utf-8") + except UnicodeDecodeError as exc: + raise ValueError( + f"{path!r} is not valid UTF-8 and cannot be used " + "in a shell command" + ) from exc + decoded.append(path) + return decoded + async def _remote_copy_file(self, channel, lpath, rpath): """Copy over the copy-data extension. Returns False when the copy must be handled by the shell fallback instead.""" @@ -366,13 +384,7 @@ async def _cp_file(self, lpath, rpath, **kwargs): ): return - # The shell command needs text: bytes paths are decoded as - # UTF-8. Non-UTF-8 byte paths cannot ride a shell command and - # only work on operations that stay on the SFTP channel. - if isinstance(lpath, bytes): - lpath = lpath.decode("utf-8") - if isinstance(rpath, bytes): - rpath = rpath.decode("utf-8") + lpath, rpath = self._shell_paths(lpath, rpath) cmd = f"cp {shlex.quote(lpath)} {shlex.quote(rpath)}" await self._execute(cmd) From 19713dbf6708610bf8202357e8cd426a7b22ee5e Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 21:11:37 -0700 Subject: [PATCH 17/21] tests: pin lying stat sizes and a preserved mode of 0 Two regressions the suite could not have caught. A server reporting every file as empty (procfs and sysfs do this) now serves a copy test: the destination must carry the full content, which fails against a destination sized from a stat snapshot. And a source reporting permissions == 0 -- a valid mode, and how SFTP v4+ reports mode 000 since the file type lives elsewhere -- must be requested as-is rather than treated as a missing mode. Co-Authored-By: Claude Fable 5 --- tests/conftest.py | 50 ++++++++++++++++++++++++++++++-------- tests/test_sshfs.py | 59 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 10 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 3f244c1..2ac1e60 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -23,13 +23,28 @@ def validate_public_key(self, username, key): return key == _USER_KEY.convert_to_public() -@pytest.fixture(scope="session") -def asyncssh_server(tmp_path_factory): - """SFTP server that, unlike the paramiko-based mockssh fixture, - implements the copy-data and limits extensions. Authenticated with - the test user key and chrooted to a fresh directory; yields - (host, port, root) where the remote "/" maps to root.""" - root = tmp_path_factory.mktemp("asyncssh-root") +class _ZeroSizeSFTPServer(asyncssh.SFTPServer): + """Reports every file as empty, like procfs and sysfs do, while + still serving the real content.""" + + def _zero(self, result): + attrs = asyncssh.SFTPAttrs.from_local(result) + attrs.size = 0 + return attrs + + def stat(self, path): + return self._zero(super().stat(path)) + + def lstat(self, path): + return self._zero(super().lstat(path)) + + def fstat(self, file_obj): + return self._zero(super().fstat(file_obj)) + + +def _serve(root, sftp_server=asyncssh.SFTPServer): + """Start an authenticated SFTP server chrooted to `root` on its own + event loop thread. Yields (host, port, root).""" loop = asyncio.new_event_loop() thread = threading.Thread(target=loop.run_forever, daemon=True) thread.start() @@ -40,9 +55,7 @@ async def _listen(): 0, server_host_keys=[_USER_KEY], server_factory=_TestSSHServer, - sftp_factory=lambda chan: asyncssh.SFTPServer( - chan, chroot=str(root) - ), + sftp_factory=lambda chan: sftp_server(chan, chroot=str(root)), ) server = asyncio.run_coroutine_threadsafe(_listen(), loop).result(30) @@ -70,6 +83,23 @@ async def _shutdown(): loop.close() +@pytest.fixture(scope="session") +def asyncssh_server(tmp_path_factory): + """SFTP server that, unlike the paramiko-based mockssh fixture, + implements the copy-data and limits extensions. Authenticated with + the test user key and chrooted to a fresh directory; yields + (host, port, root) where the remote "/" maps to root.""" + yield from _serve(tmp_path_factory.mktemp("asyncssh-root")) + + +@pytest.fixture(scope="session") +def zero_size_server(tmp_path_factory): + """Like asyncssh_server, but every stat reports size 0.""" + yield from _serve( + tmp_path_factory.mktemp("zero-size-root"), _ZeroSizeSFTPServer + ) + + def _handler_run(self): # Identical to mockssh.server.Handler.run except that the command # queue is created atomically. Upstream checks `chanid not in diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index b581cc9..7ca01f3 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -306,6 +306,65 @@ def test_cp_file_copy_data_creates(copydata_fs, copydata_dir): assert (local / "d" / "src").read_bytes() == b"payload" +@requires_copy_data +def test_cp_file_copy_data_ignores_reported_size(zero_size_server): + # Sources whose stat lies about the size (procfs, sysfs) must be + # copied whole: the copy runs to the source's real end of file and + # never sizes the destination from a stat snapshot. + host, port, root = zero_size_server + fs = SSHFileSystem( + host=host, port=port, username="user", client_keys=[USERS["user"]] + ) + try: + (root / "src").write_bytes(b"payload" * 1000) + assert fs.info("/src")["size"] == 0 + + fs.cp_file("/src", "/dst") + assert fs._supports_remote_copy is True + assert (root / "dst").read_bytes() == b"payload" * 1000 + finally: + with suppress(Exception): + sync(fs.loop, fs._stack.aclose, timeout=5) + + +def test_remote_copy_keeps_mode_zero(fs, monkeypatch): + # A mode of 0 is a valid mode, not a missing one: it must be + # requested as-is instead of falling back to the server's default + # (SFTP v4+ reports it as permissions == 0, since the file type + # lives in a separate field). + opened = [] + + class _File: + async def stat(self): + return SFTPAttrs(permissions=0) + + async def close(self): + pass + + class Channel: + supports_remote_copy = True + + def encode(self, path): + return path.encode() if isinstance(path, str) else path + + async def isdir(self, path): + return False + + async def open(self, path, *args, **kwargs): + opened.append((path, args)) + return _File() + + async def remote_copy(self, src, dst): + pass + + monkeypatch.setattr(fs, "_supports_remote_copy", True) + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + + fs.cp_file("/src", "/dst") + _dst_path, dst_args = opened[-1] + assert dst_args[1].permissions == 0 + + @requires_copy_data def test_cp_file_copy_data_existing_destinations(copydata_fs, copydata_dir): # The extension path only creates destinations. Anything existing From 64ed8122bb6ee6630e0806f14e6930a58b6f635f Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 22:41:35 -0700 Subject: [PATCH 18/21] Fail closed on unknown modes, try the standard rename, scope denials Three problems with the previous revision: An unreadable source mode is no longer a licence to publish the file. Servers can deny fstat (OpenSSH -P fstat) while allowing the copy, and falling back to the server's default creation mode turned a 0400 source into a 0644 destination. An unknown mode is now 0600. Owner write is also always requested, so that a source without it does not produce a destination the shell fallback cannot write if the copy is denied; such a source gains that single bit, and no group or other bit is ever added. _mv() went straight from an unsupported posix-rename to copy-and- delete, which turns a symlink into a copy of its target, gives a hardlink a new inode and drops its special bits. The standard SFTP rename, which every version has, is now tried in between: it keeps the object's identity and cannot lose data. Only when neither rename applies does the copy run. A denied copy-data request no longer disables the extension for the whole connection: a policy can allow or deny it per path, so denial falls back for that copy alone. Only an unsupported request, which is a property of the server, is cached. The shell fallback also passes -- so that a relative path starting with a dash is not parsed as a cp option. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 61 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index 897734b..f00b6e1 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -218,15 +218,23 @@ async def _modified(self, path: str, **kwargs) -> datetime: @wrap_exceptions async def _mv(self, lpath, rpath, **kwargs): async with self._pool.get() as channel: + # posix-rename is an extension to the original SFTP + # protocol, but it is the only form that can replace an + # existing destination atomically. with suppress(SFTPOpUnsupported): return await channel.posix_rename(lpath, rpath) - # Some systems doesn't natively support posix_rename - # which is an extension to the original SFTP protocol. - # In that case we are going to copy the file and delete - # it. The source must only be removed after the copy fully - # succeeded. + # The standard rename is still a rename: it keeps the + # object's identity (symlinks stay symlinks, hardlinks keep + # their inode and their special bits) and cannot lose data. + # It refuses an existing destination, which is the case the + # copy below has to handle. + with suppress(OSError, SFTPError): + return await channel.rename(lpath, rpath) + # Neither rename is available for these operands, so fall back + # to copying and removing. The source must only be removed + # after the copy fully succeeded. await self._cp_file(lpath, rpath) await self._rm_file(lpath) @@ -314,17 +322,23 @@ async def _remote_copy_file(self, channel, lpath, rpath): # that a missing source cannot leave an empty destination. src_file = await channel.open(lpath, "rb", block_size=0) try: - # Like cp for new files: special bits stripped, and the - # server applies its umask to the requested mode. A mode of - # 0 is a valid mode; a missing one -- or a server that - # denies fstat -- falls back to the server default. - attrs = asyncssh.SFTPAttrs() + # Like cp for new files: the source's mode without the + # special bits, which the server then filters through its + # umask. A mode of 0 is a valid mode, so only a genuinely + # missing one is replaced -- and it is replaced with 0600, + # never with the server's default: servers may deny fstat + # (OpenSSH -P fstat) while allowing the copy, and defaulting + # to a world-readable mode would publish the contents of a + # private source. Owner-write is always requested so that + # the shell fallback can still write the file if the copy + # is denied; a source without owner-write therefore gains + # that single bit. + mode = 0o600 with suppress(OSError, SFTPError): src_attrs = await src_file.stat() if src_attrs.permissions is not None: - attrs = asyncssh.SFTPAttrs( - permissions=src_attrs.permissions & 0o777 - ) + mode = (src_attrs.permissions & 0o777) | 0o200 + attrs = asyncssh.SFTPAttrs(permissions=mode) try: dst_file = await channel.open( rpath, @@ -342,15 +356,22 @@ async def _remote_copy_file(self, channel, lpath, rpath): try: await channel.remote_copy(src_file, dst_file) - except (SFTPOpUnsupported, SFTPPermissionDenied): - # Advertised but denied (e.g. an OpenSSH allow/deny - # policy): stop trying the extension on this connection - # and let the shell cp overwrite the empty file just - # created. + except SFTPOpUnsupported: + # Advertised but not actually implemented: stop trying + # the extension on this connection and let the shell cp + # overwrite the empty file just created. self._supports_remote_copy = False with suppress(OSError, SFTPError): await dst_file.close() return False + except SFTPPermissionDenied: + # Denied for these operands (an OpenSSH allow/deny + # policy can depend on the paths involved), which says + # nothing about the next copy: fall back for this one + # without disabling the extension connection-wide. + with suppress(OSError, SFTPError): + await dst_file.close() + return False except BaseException: with suppress(OSError, SFTPError): await dst_file.close() @@ -385,7 +406,9 @@ async def _cp_file(self, lpath, rpath, **kwargs): return lpath, rpath = self._shell_paths(lpath, rpath) - cmd = f"cp {shlex.quote(lpath)} {shlex.quote(rpath)}" + # `--` keeps a relative path starting with a dash from being + # parsed as an option. + cmd = f"cp -- {shlex.quote(lpath)} {shlex.quote(rpath)}" await self._execute(cmd) @wrap_exceptions From bd78fa9d46cc078e3d90265899ea23990ebc8633 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Thu, 6 Aug 2026 22:41:35 -0700 Subject: [PATCH 19/21] tests: negotiate SFTP v4 for real and cover the new contracts The v4 coverage was false: the client asked for v4 while the test server only offered v3, so both parameters ran v3. The server fixture now runs the suite once per protocol generation and a test asserts the negotiated version, so the claim cannot silently regress again. New coverage: an unknown source mode creates a private destination, a source without owner-write keeps its group and other bits, a move falls back to the standard rename and keeps a symlink a symlink, and a denied copy-data request does not disable the extension for later copies. The fixtures skip on asyncssh < 2.19 instead of erroring, and the zero-size test uses unique names so reruns cannot collide. Co-Authored-By: Claude Fable 5 --- tests/conftest.py | 23 ++++-- tests/test_sshfs.py | 178 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 171 insertions(+), 30 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 2ac1e60..9bff731 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -42,9 +42,11 @@ def fstat(self, file_obj): return self._zero(super().fstat(file_obj)) -def _serve(root, sftp_server=asyncssh.SFTPServer): +def _serve(root, sftp_server=asyncssh.SFTPServer, sftp_version=3): """Start an authenticated SFTP server chrooted to `root` on its own - event loop thread. Yields (host, port, root).""" + event loop thread. Yields (host, port, root, sftp_version).""" + if not hasattr(asyncssh.SFTPClient, "supports_remote_copy"): + pytest.skip("asyncssh without copy-data support (< 2.19)") loop = asyncio.new_event_loop() thread = threading.Thread(target=loop.run_forever, daemon=True) thread.start() @@ -56,11 +58,12 @@ async def _listen(): server_host_keys=[_USER_KEY], server_factory=_TestSSHServer, sftp_factory=lambda chan: sftp_server(chan, chroot=str(root)), + sftp_version=sftp_version, ) server = asyncio.run_coroutine_threadsafe(_listen(), loop).result(30) try: - yield "127.0.0.1", server.get_port(), root + yield "127.0.0.1", server.get_port(), root, sftp_version finally: async def _shutdown(): @@ -83,13 +86,19 @@ async def _shutdown(): loop.close() -@pytest.fixture(scope="session") -def asyncssh_server(tmp_path_factory): +@pytest.fixture(scope="session", params=[3, 4], ids=["sftpv3", "sftpv4"]) +def asyncssh_server(tmp_path_factory, request): """SFTP server that, unlike the paramiko-based mockssh fixture, implements the copy-data and limits extensions. Authenticated with the test user key and chrooted to a fresh directory; yields - (host, port, root) where the remote "/" maps to root.""" - yield from _serve(tmp_path_factory.mktemp("asyncssh-root")) + (host, port, root, version) where the remote "/" maps to root. + Runs once per + SFTP protocol generation: v4+ moves the file type out of the + permission bits.""" + yield from _serve( + tmp_path_factory.mktemp(f"asyncssh-root-v{request.param}"), + sftp_version=request.param, + ) @pytest.fixture(scope="session") diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index 7ca01f3..d3bdb54 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -9,6 +9,7 @@ from datetime import datetime, timedelta, timezone from pathlib import Path from types import SimpleNamespace +from unittest import mock import fsspec import pytest @@ -81,6 +82,11 @@ def fs_hard_queue(ssh_server, user="user"): ) +async def _channel_version(fs): + async with fs._pool.get() as channel: + return channel.version + + def strip_keys(info): for key in ["name", "time", "mtime", "atime"]: info.pop(key, None) @@ -242,20 +248,17 @@ async def __aexit__(self, *exc): return _Ctx() -@pytest.fixture(scope="session", params=[None, 4], ids=["sftpv3", "sftpv4"]) -def copydata_fs(asyncssh_server, request): - # v4+ separates the file type from `permissions`, so both protocol - # generations must satisfy the same contract. - host, port, _root = asyncssh_server - extra = {} - if request.param is not None: - extra["sftp_client_kwargs"] = {"sftp_version": request.param} +@pytest.fixture(scope="session") +def copydata_fs(asyncssh_server): + # The server fixture runs the whole suite once per SFTP protocol + # generation; the client negotiates up to whatever it offers. + host, port, _root, version = asyncssh_server fs = SSHFileSystem( host=host, port=port, username="user", client_keys=[USERS["user"]], - **extra, + sftp_client_kwargs={"sftp_version": version}, ) yield fs # Close the connection so the server fixture can shut its loop @@ -266,7 +269,7 @@ def copydata_fs(asyncssh_server, request): @pytest.fixture def copydata_dir(asyncssh_server, request): - _host, _port, root = asyncssh_server + _host, _port, root, _version = asyncssh_server # unique per invocation so pytest-rerunfailures retries get a # fresh directory local = root / f"{request.node.name}-{secrets.token_hex(4)}" @@ -311,17 +314,18 @@ def test_cp_file_copy_data_ignores_reported_size(zero_size_server): # Sources whose stat lies about the size (procfs, sysfs) must be # copied whole: the copy runs to the source's real end of file and # never sizes the destination from a stat snapshot. - host, port, root = zero_size_server + host, port, root, _version = zero_size_server fs = SSHFileSystem( host=host, port=port, username="user", client_keys=[USERS["user"]] ) try: - (root / "src").write_bytes(b"payload" * 1000) - assert fs.info("/src")["size"] == 0 + name = secrets.token_hex(4) + (root / f"src-{name}").write_bytes(b"payload" * 1000) + assert fs.info(f"/src-{name}")["size"] == 0 - fs.cp_file("/src", "/dst") + fs.cp_file(f"/src-{name}", f"/dst-{name}") assert fs._supports_remote_copy is True - assert (root / "dst").read_bytes() == b"payload" * 1000 + assert (root / f"dst-{name}").read_bytes() == b"payload" * 1000 finally: with suppress(Exception): sync(fs.loop, fs._stack.aclose, timeout=5) @@ -329,9 +333,10 @@ def test_cp_file_copy_data_ignores_reported_size(zero_size_server): def test_remote_copy_keeps_mode_zero(fs, monkeypatch): # A mode of 0 is a valid mode, not a missing one: it must be - # requested as-is instead of falling back to the server's default + # carried over instead of falling back to the server's default # (SFTP v4+ reports it as permissions == 0, since the file type - # lives in a separate field). + # lives in a separate field). Only owner-write is added, so no + # group or other bit appears. opened = [] class _File: @@ -362,7 +367,7 @@ async def remote_copy(self, src, dst): fs.cp_file("/src", "/dst") _dst_path, dst_args = opened[-1] - assert dst_args[1].permissions == 0 + assert dst_args[1].permissions == 0o200 @requires_copy_data @@ -413,6 +418,7 @@ def test_cp_file_copy_data_never_redirects(copydata_fs, copydata_dir): assert not (local / "missing").exists() +@requires_copy_data def test_mv_hardlink_alias(copydata_fs, copydata_dir): # POSIX rename between two names of the same inode is a no-op: # the move succeeds with both names surviving and no data lost. @@ -427,9 +433,132 @@ def test_mv_hardlink_alias(copydata_fs, copydata_dir): assert (local / "hard").read_bytes() == b"payload" +@requires_copy_data +def test_copydata_server_negotiates_expected_version( + copydata_fs, asyncssh_server +): + # The functional suite claims to cover both protocol generations, + # so the negotiated version must actually be the server's. + _host, _port, _root, version = asyncssh_server + negotiated = sync( + copydata_fs.loop, _channel_version, copydata_fs, timeout=10 + ) + assert negotiated == version + + +@requires_copy_data +def test_cp_file_copy_data_unreadable_source_mode(copydata_fs, copydata_dir): + # A source without owner-write keeps its group/other bits and + # gains only owner-write, so the shell fallback could still write + # the file if the copy were denied. + fs = copydata_fs + local, remote = copydata_dir + (local / "src").write_bytes(b"payload") + (local / "src").chmod(0o400) + + fs.cp_file(remote + "/src", remote + "/dst") + assert (local / "dst").read_bytes() == b"payload" + assert ((local / "dst").stat().st_mode & 0o077) == 0 + + +@requires_copy_data +def test_mv_uses_standard_rename(copydata_fs, copydata_dir): + # Without posix-rename, a plain rename still keeps the object's + # identity: a symlink must stay a symlink instead of being + # flattened into a copy of its target. + fs = copydata_fs + local, remote = copydata_dir + (local / "target").write_bytes(b"payload") + (local / "link").symlink_to("target") + + async def _no_posix_rename(*args, **kwargs): + raise SFTPOpUnsupported("posix-rename not supported") + + with mock.patch.object(SFTPClient, "posix_rename", _no_posix_rename): + fs.mv(remote + "/link", remote + "/moved") + + assert (local / "moved").is_symlink() + assert os.readlink(local / "moved") == "target" + + +def test_remote_copy_unknown_mode_is_private(fs, monkeypatch): + # A server that denies fstat must not cause the destination to be + # created with the server's default (world-readable) mode. + opened = [] + + class _File: + async def stat(self): + raise SFTPPermissionDenied("fstat denied") + + async def close(self): + pass + + class Channel: + supports_remote_copy = True + + def encode(self, path): + return path.encode() if isinstance(path, str) else path + + async def isdir(self, path): + return False + + async def open(self, path, *args, **kwargs): + opened.append((path, args)) + return _File() + + async def remote_copy(self, src, dst): + pass + + monkeypatch.setattr(fs, "_supports_remote_copy", True) + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + + fs.cp_file("/src", "/dst") + _path, args = opened[-1] + assert args[1].permissions == 0o600 + + +def test_cp_file_copy_data_denied_is_not_cached(fs, monkeypatch): + # A denial can depend on the operands, so it must not disable the + # extension for every later copy on the connection. + events = [] + + class _File: + async def stat(self): + return SFTPAttrs(permissions=0o100644) + + async def close(self): + pass + + class Channel: + supports_remote_copy = True + + def encode(self, path): + return path.encode() if isinstance(path, str) else path + + async def isdir(self, path): + return False + + async def open(self, path, *args, **kwargs): + return _File() + + async def remote_copy(self, src, dst): + raise SFTPPermissionDenied("denied for these operands") + + async def record_shell(cmd, **kwargs): + events.append(cmd) + + monkeypatch.setattr(fs, "_supports_remote_copy", None) + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + monkeypatch.setattr(fs, "_execute", record_shell) + + fs.cp_file("/denied", "/dst") + assert events == ["cp -- /denied /dst"] + assert fs._supports_remote_copy is True + + def test_cp_file_copy_data_denied(fs, monkeypatch): - # copy-data advertised but denied by server policy: the capability - # is re-cached as unsupported and the copy falls back to the shell, + # copy-data advertised but not implemented: the capability is + # re-cached as unsupported and the copy falls back to the shell, # which overwrites the empty file created by the exclusive open. events = [] @@ -470,7 +599,7 @@ def open(self, path, *args, **kwargs): return _OpenResult() async def remote_copy(self, src, dst): - raise SFTPPermissionDenied("denied by policy") + raise SFTPOpUnsupported("advertised but not implemented") async def record_shell(cmd, **kwargs): events.append(("shell", cmd)) @@ -480,7 +609,7 @@ async def record_shell(cmd, **kwargs): monkeypatch.setattr(fs, "_execute", record_shell) fs.cp_file("/src", "/dst") - assert ("shell", "cp /src /dst") in events + assert ("shell", "cp -- /src /dst") in events assert fs._supports_remote_copy is False @@ -491,6 +620,9 @@ class Channel: async def posix_rename(self, lpath, rpath): raise SFTPOpUnsupported("posix-rename not supported") + async def rename(self, lpath, rpath): + raise SFTPFailure("destination exists") + removed = [] async def failing_cp(*args, **kwargs): @@ -544,7 +676,7 @@ async def record_shell(cmd, **kwargs): fs.cp_file("/src", "/dst") fs.cp_file("/src2", "/dst2") - assert calls == ["cp /src /dst", "cp /src2 /dst2"] + assert calls == ["cp -- /src /dst", "cp -- /src2 /dst2"] assert len(pool_uses) == 1 From 4580b7ef92f54d416a124e12b2719623d47af11b Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Fri, 7 Aug 2026 11:31:15 -0700 Subject: [PATCH 20/21] Move with a rename or the remote mv, never copy-and-delete A successful copy only proves that the bytes reached an open destination handle, not that the destination path still names it, so removing the source afterwards could destroy the only remaining copy of the data -- the same identity problem that already rules out unlinking on failure. When neither rename applies, the move is handed to the remote mv instead; on a server without shell access the move now fails with the source untouched, which is the honest outcome. The standard rename also no longer swallows every error: only the statuses that mean "these operands cannot be renamed" (existing destination, cross-device, unimplemented) fall through. A denied rename used to be quietly downgraded to copy-and-delete, bypassing the policy and flattening symlinks into regular files. The copy no longer widens read-only source modes. Requesting owner write made every successful copy of a 0400 source produce 0600, and it could not even guarantee the recovery it was meant to enable, since the server filters the requested mode through its umask anyway. An unknown mode still falls back to 0600 rather than the server default. Co-Authored-By: Claude Fable 5 --- sshfs/spec.py | 65 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 18 deletions(-) diff --git a/sshfs/spec.py b/sshfs/spec.py index f00b6e1..25f484d 100644 --- a/sshfs/spec.py +++ b/sshfs/spec.py @@ -8,7 +8,17 @@ from typing import Optional import asyncssh -from asyncssh.sftp import SFTPError, SFTPOpUnsupported, SFTPPermissionDenied +from asyncssh.sftp import ( + FILEXFER_TYPE_DIRECTORY, + FILEXFER_TYPE_REGULAR, + FILEXFER_TYPE_SYMLINK, + FILEXFER_TYPE_UNKNOWN, + SFTPError, + SFTPFailure, + SFTPFileAlreadyExists, + SFTPOpUnsupported, + SFTPPermissionDenied, +) from fsspec.asyn import ( AsyncFileSystem, FSTimeoutError, @@ -29,6 +39,12 @@ async_methods.append("_mv") +_FILE_TYPES = { + FILEXFER_TYPE_REGULAR: "file", + FILEXFER_TYPE_DIRECTORY: "directory", + FILEXFER_TYPE_SYMLINK: "link", +} + # Always allocate 2 channels for shell operations # and the rest (generally 8) for SFTP. _SHELL_CHANNELS = 2 @@ -168,7 +184,13 @@ def client(self): return self._client def _decode_attributes(self, attributes): - if stat.S_ISDIR(attributes.permissions): + # SFTP v4 and later carry the file type in its own field and + # leave only the permission bits in `permissions`, so the type + # has to be read from there when the server reports one. + file_type = getattr(attributes, "type", FILEXFER_TYPE_UNKNOWN) + if file_type != FILEXFER_TYPE_UNKNOWN: + kind = _FILE_TYPES.get(file_type, "unknown") + elif stat.S_ISDIR(attributes.permissions): kind = "directory" elif stat.S_ISREG(attributes.permissions): kind = "file" @@ -224,19 +246,29 @@ async def _mv(self, lpath, rpath, **kwargs): with suppress(SFTPOpUnsupported): return await channel.posix_rename(lpath, rpath) - # The standard rename is still a rename: it keeps the - # object's identity (symlinks stay symlinks, hardlinks keep - # their inode and their special bits) and cannot lose data. - # It refuses an existing destination, which is the case the - # copy below has to handle. - with suppress(OSError, SFTPError): + # The standard rename keeps the object's identity + # (symlinks stay symlinks, hardlinks keep their inode and + # their special bits) and cannot lose data. Only the + # statuses that a rename returns for "cannot rename these + # operands" -- an existing destination, a cross-device + # move, an unimplemented request -- fall through to the + # shell below; permission, missing-file and transport + # errors are the caller's answer. + with suppress( + SFTPFailure, SFTPFileAlreadyExists, SFTPOpUnsupported + ): return await channel.rename(lpath, rpath) - # Neither rename is available for these operands, so fall back - # to copying and removing. The source must only be removed - # after the copy fully succeeded. - await self._cp_file(lpath, rpath) - await self._rm_file(lpath) + # Neither rename applies, so let the remote mv do it. Copying + # and deleting the source here would be unsafe: a successful + # copy only proves that the bytes reached an open handle, not + # that the destination path still names it, so the source could + # be removed after its data ended up somewhere unreachable. On + # a server without shell access this raises instead, leaving + # the source untouched. + lpath, rpath = self._shell_paths(lpath, rpath) + cmd = f"mv -- {shlex.quote(lpath)} {shlex.quote(rpath)}" + await self._execute(cmd) @wrap_exceptions async def _put_file( @@ -329,15 +361,12 @@ async def _remote_copy_file(self, channel, lpath, rpath): # never with the server's default: servers may deny fstat # (OpenSSH -P fstat) while allowing the copy, and defaulting # to a world-readable mode would publish the contents of a - # private source. Owner-write is always requested so that - # the shell fallback can still write the file if the copy - # is denied; a source without owner-write therefore gains - # that single bit. + # private source. mode = 0o600 with suppress(OSError, SFTPError): src_attrs = await src_file.stat() if src_attrs.permissions is not None: - mode = (src_attrs.permissions & 0o777) | 0o200 + mode = src_attrs.permissions & 0o777 attrs = asyncssh.SFTPAttrs(permissions=mode) try: dst_file = await channel.open( From f6fe1fbf9795da5824ab14d3a5ed2f227b009d3e Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Fri, 7 Aug 2026 11:31:30 -0700 Subject: [PATCH 21/21] tests: cover the move ladder, exact modes and v4 metadata The move fallback is pinned to the remote mv with copy-and-delete asserted never to run, and a denied rename is asserted to propagate instead of silently changing semantics. The read-only-source test now asserts the exact mode, and file types are checked on every protocol generation. The zero-size test skips the instance cache so a rerun no longer reuses the closed filesystem it just built. Co-Authored-By: Claude Fable 5 --- tests/test_sshfs.py | 96 ++++++++++++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 27 deletions(-) diff --git a/tests/test_sshfs.py b/tests/test_sshfs.py index d3bdb54..2e1b1a6 100644 --- a/tests/test_sshfs.py +++ b/tests/test_sshfs.py @@ -316,7 +316,11 @@ def test_cp_file_copy_data_ignores_reported_size(zero_size_server): # never sizes the destination from a stat snapshot. host, port, root, _version = zero_size_server fs = SSHFileSystem( - host=host, port=port, username="user", client_keys=[USERS["user"]] + host=host, + port=port, + username="user", + client_keys=[USERS["user"]], + skip_instance_cache=True, ) try: name = secrets.token_hex(4) @@ -335,8 +339,7 @@ def test_remote_copy_keeps_mode_zero(fs, monkeypatch): # A mode of 0 is a valid mode, not a missing one: it must be # carried over instead of falling back to the server's default # (SFTP v4+ reports it as permissions == 0, since the file type - # lives in a separate field). Only owner-write is added, so no - # group or other bit appears. + # lives in a separate field). opened = [] class _File: @@ -367,7 +370,7 @@ async def remote_copy(self, src, dst): fs.cp_file("/src", "/dst") _dst_path, dst_args = opened[-1] - assert dst_args[1].permissions == 0o200 + assert dst_args[1].permissions == 0 @requires_copy_data @@ -447,10 +450,31 @@ def test_copydata_server_negotiates_expected_version( @requires_copy_data -def test_cp_file_copy_data_unreadable_source_mode(copydata_fs, copydata_dir): - # A source without owner-write keeps its group/other bits and - # gains only owner-write, so the shell fallback could still write - # the file if the copy were denied. +def test_info_reports_type_on_every_version(copydata_fs, copydata_dir): + # SFTP v4+ reports the file type in its own field and leaves only + # the permission bits in `permissions`, so the type must not be + # decoded from the mode alone. + fs = copydata_fs + local, remote = copydata_dir + (local / "file").write_bytes(b"payload") + (local / "dir").mkdir() + (local / "link").symlink_to("file") + + assert fs.info(remote + "/file")["type"] == "file" + assert fs.info(remote + "/dir")["type"] == "directory" + assert fs.isdir(remote + "/dir") + assert fs.isfile(remote + "/file") + assert {i["type"] for i in fs.ls(remote)} == { + "file", + "directory", + "link", + } + + +@requires_copy_data +def test_cp_file_copy_data_read_only_source_mode(copydata_fs, copydata_dir): + # A read-only source keeps its exact mode, like cp: the copy must + # not widen it just because the extension was used. fs = copydata_fs local, remote = copydata_dir (local / "src").write_bytes(b"payload") @@ -458,7 +482,7 @@ def test_cp_file_copy_data_unreadable_source_mode(copydata_fs, copydata_dir): fs.cp_file(remote + "/src", remote + "/dst") assert (local / "dst").read_bytes() == b"payload" - assert ((local / "dst").stat().st_mode & 0o077) == 0 + assert ((local / "dst").stat().st_mode & 0o7777) == 0o400 @requires_copy_data @@ -613,9 +637,13 @@ async def record_shell(cmd, **kwargs): assert fs._supports_remote_copy is False -def test_mv_fallback_keeps_source_on_copy_failure(fs, monkeypatch): - # When posix_rename is unsupported and the copy fails, the source - # must survive: it may only be removed after a successful copy. +def test_mv_falls_back_to_remote_mv(fs, monkeypatch): + # When neither rename applies, the move is handed to the remote mv + # rather than copied and deleted: a copy only proves that bytes + # reached an open handle, so deleting the source afterwards could + # destroy the only remaining copy of the data. + events = [] + class Channel: async def posix_rename(self, lpath, rpath): raise SFTPOpUnsupported("posix-rename not supported") @@ -623,28 +651,42 @@ async def posix_rename(self, lpath, rpath): async def rename(self, lpath, rpath): raise SFTPFailure("destination exists") - removed = [] + async def fail_cp(*args, **kwargs): + raise AssertionError("mv must not copy and delete") - async def failing_cp(*args, **kwargs): - raise OSError("copy failed") + async def fail_rm(*args, **kwargs): + raise AssertionError("mv must not remove the source itself") - async def record_rm(path, **kwargs): - removed.append(path) + async def record_shell(cmd, **kwargs): + events.append(cmd) monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) - monkeypatch.setattr(fs, "_cp_file", failing_cp) - monkeypatch.setattr(fs, "_rm_file", record_rm) + monkeypatch.setattr(fs, "_cp_file", fail_cp) + monkeypatch.setattr(fs, "_rm_file", fail_rm) + monkeypatch.setattr(fs, "_execute", record_shell) - with pytest.raises(OSError): - fs.mv("/src", "/dst") - assert removed == [] + fs.mv("/src", "/dst") + assert events == ["mv -- /src /dst"] - async def ok_cp(*args, **kwargs): - pass - monkeypatch.setattr(fs, "_cp_file", ok_cp) - fs.mv("/src", "/dst") - assert removed == ["/src"] +def test_mv_propagates_rename_errors(fs, monkeypatch): + # A denied rename is an answer, not a reason to try something with + # different semantics. + class Channel: + async def posix_rename(self, lpath, rpath): + raise SFTPOpUnsupported("posix-rename not supported") + + async def rename(self, lpath, rpath): + raise SFTPPermissionDenied("rename denied by policy") + + async def fail_shell(*args, **kwargs): + raise AssertionError("a denied rename must not reach the shell") + + monkeypatch.setattr(fs, "_pool", _FakeChannelPool(Channel())) + monkeypatch.setattr(fs, "_execute", fail_shell) + + with pytest.raises(PermissionError): + fs.mv("/src", "/dst") @pytest.mark.parametrize("legacy_asyncssh", [False, True])