Use the copy-data extension for cp_file when the server supports it - #74
Closed
shcheklein wants to merge 21 commits into
Closed
Use the copy-data extension for cp_file when the server supports it#74shcheklein wants to merge 21 commits into
shcheklein wants to merge 21 commits into
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds SFTP server-side copying when copy-data is supported, retaining shell fallback otherwise.
Changes:
- Caches remote-copy capability and uses
remote_only=True. - Adds remote-copy and fallback tests.
- Verifies copied file contents.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
sshfs/spec.py |
Implements capability-gated SFTP copying. |
tests/test_sshfs.py |
Adds copy-path tests and content verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
shcheklein
force-pushed
the
cp-file-copy-data
branch
from
August 6, 2026 23:22
7e8c0f7 to
6b5492c
Compare
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…h 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Collaborator
Author
|
It is getting too complicated, not worth it atm I think. Happy to discuss if someone can get back to it at some point. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
cp_file(and themovefallback) shells out tocpover an SSH exec channel. SFTP-only servers that deny exec (chrootedinternal-sftp, transfer appliances) can't copy at all, and the remote must have a POSIX shell withcp.Change
When the channel advertises the
copy-dataextension (supports_remote_copy, asyncssh >= 2.19 talking to e.g. OpenSSH >= 9.0) and the destination does not exist, the file is copied server-side: the source is opened, the destination is created withFXF_WRITE|FXF_CREAT|FXF_EXCL(mode = the source's mode without special bits, filtered by the server's umask; a directory destination resolves todir/basename(src)first), and a single copy-data request copies until the source's end of file. No shell, no data through the client, and no size assumptions — sources whose stat lies (procfs-style) copy correctly. A failed or interrupted copy may leave a partial destination, exactly like an interruptedcp; it is deliberately never unlinked, since over SFTP a pathname cannot be re-verified to still name the file this client created (an unrelated file could race onto the same name).Everything else goes to the shell
cpfallback, byte-for-byte as before this PR: existing destinations — which include every alias of the source (same path, symlink, or hardlink, the latter undetectable over SFTP) — dangling destination symlinks (FXF_EXCLrefuses to create through them), missing parents. In-place overwrite over SFTP is deliberately not implemented: a stale pre-copy size corrupts changing or stat-lying sources, a post-copy truncate can be denied by fsetstat policy after the destination was already modified, a pre-copy truncate destroys hardlink-aliased sources, and replacing the directory entry loses the inode.cpowns those semantics.Guards and fixes along the way:
getattr— no version-pin change.fstat(OpenSSH-P fstat) is tolerated and fails closed: an unknown source mode creates the destination0600rather than the server's world-readable default. A known mode is copied exactly.moveno longer copies and deletes. It tries posix-rename, then the standard SFTP rename, then the remotemv. A copy only proves that bytes reached an open handle, not that the destination path still names it, so deleting the source afterwards could destroy the only remaining copy of the data. On a server without shell access a move that neither rename can perform now fails with the source untouched. A denied rename propagates instead of being downgraded to something with different semantics.info()["type"],isdir()andisfile()are fixed for SFTP v4+ servers, which report the file type in its own field rather than in the permission bits (pre-existing bug, surfaced by the new v4 fixture; can be split out if you prefer).SFTPPermissionDeniedis now mapped toPermissionError(previously leaked the asyncssh exception through the public API).movefallback removes the source only after a successful copy (previously afinallydeleted it even when the copy failed).--so a path starting with a dash is not parsed as acpoption.Known divergences and operational notes on the copy-data path:
cpoverwrites it on the next attempt, but on an SFTP-only server a retry hits the existing-destination path and fails until the partial file is removed — automatic retries cannot clear it themselves.Tests
asyncssh-based server fixture (implements copy-data natively, unlike paramiko-based mockssh): chrooted to a fresh directory, key-authenticated, cleanly shut down (no leaked tasks). Functional coverage of the create-only contract: content and umask-filtered mode, copy-into-directory, existing/alias destinations fail untouched when no shell is available, dangling destination symlinks refused. Skipped on asyncssh < 2.19; per-test directories and names are rerun-safe; the suite runs once per SFTP protocol generation with the server offering v3 or v4 (v4+ separates file type from permissions) and a test asserting the negotiated version, so that coverage claim cannot silently regress. Policy denial is unit-tested (falls back for that copy, extension stays enabled) and an unsupported request separately (extension disabled); gating/caching and the move fallback's source preservation are pinned.