cli
`cs cp`: inappropriate quoting breaks most use cases
Describe the bug
gh cs cp incorrectly wraps single quotes around all remote paths which are interpreted literally by sshd. This means that copying from the codespace is impossible because it looks for e.g. the file with the literal name '/path/to/my/file' instead of /path/to/my/file. Copying to the codespace only works if you use relative paths and accept the fact that it will always create a file with a mangled quoted wrapped path like 'file' that you need to fix after copying.
This appears to be due to an attempt to prevent argument injection or mishandling for paths with spaces but the fix doesn't work. I suspect this feature may have been developed on client/server ssh versions which behave differently with respect to quoting rules.
tested with versions:
gh: version 2.14.4 (2022-08-10)
server: OpenSSH_8.4p1 Debian-5+deb11u1, OpenSSL 1.1.1n 15 Mar 2022
client: OpenSSH_9.0p1, OpenSSL 1.1.1q 5 Jul 2022
Steps to reproduce the behavior
Users/carl $ gh --version
gh version 2.14.4 (2022-08-10)
https://github.com/cli/cli/releases/tag/v2.14.4
# demonstrate this file is indeed on the remote server at path /home/vscode/testfile
Users/carl $ gh cs ssh --codespace $CODESPACE_NAME 'ls -la /home/vscode | grep testfile'
-rw-r--r-- 1 vscode vscode 6 Aug 26 09:32 testfile
# cp fails, it's looking for testfile' inside the directory '/home/vscode
Users/carl $ gh cs cp --codespace $CODESPACE_NAME remote:/home/vscode/testfile .
/usr/local/bin/scp: '/home/vscode/testfile': No such file or directory
shell closed: exit status 1
# Other variants where I manually quote don't work either
Users/carl $ gh cs cp --codespace $CODESPACE_NAME remote:'/home/vscode/testfile' .
/usr/local/bin/scp: '/home/vscode/testfile': No such file or directory
shell closed: exit status 1
Users/carl $ gh cs cp --codespace $CODESPACE_NAME remote:"/home/vscode/testfile" .
/usr/local/bin/scp: '/home/vscode/testfile': No such file or directory
shell closed: exit status 1
# Demonstrate again the file is indeed present
Users/carl $ gh cs ssh --codespace $CODESPACE_NAME 'cat /home/vscode/testfile'
hello
# try to send a local file to the codespace using an absolute target path.
Users/carl $ touch blah > localfile && gh cs cp --codespace $CODESPACE_NAME localfile remote:/home/vscode/localfile
/usr/local/bin/scp: dest open "'/home/vscode/localfile'": No such file or directory
/usr/local/bin/scp: failed to upload file localfile to '/home/vscode/localfile'
shell closed: exit status 1
# copy works using relative paths but it mangles the target by wrapping it in literal single quotes
Users/carl $ gh cs cp --codespace $CODESPACE_NAME localfile remote:localfile
localfile
Users/carl $ gh cs ssh --codespace $CODESPACE_NAME 'ls -la /home/vscode | grep file'
-rw-r--r-- 1 vscode vscode 0 Aug 26 09:50 'localfile'
-rw-r--r-- 1 vscode vscode 6 Aug 26 09:32 testfile
### Expected vs actual behavior
file names that don't include spaces are left alone. file names that include spaces are quoted in a way that allows them to be copied with their full name preserved.
### Logs
see above
For anyone affected by this, one workaround is just to pipe the data yourself through ssh, e.g.
# copy from codespace
gh cs ssh --codespace $CODESPACE_NAME 'gzip < /path/to/some_file' | gunzip > some_file
# copy to codespace
gzip < some_file | gh cs ssh --codespace $CODESPACE_NAME 'gunzip > /path/to/some_file'
Looking into this a bit, the ' is added to prevent shell expansions and that behavior can be disabled by adding the -e flag:
https://github.com/cli/cli/blob/e2f344fef4b34356004c9c0f3adf252f1cc0af2b/pkg/cmd/codespace/ssh.go#L683-L691
That said, it's really just a better workaround to add -e, not a solution. We should fix the default behavior so that the automatic paths with ' actually do work.
I'll try to open a fix for this, since the command is almost entirely broken. It's not just that we shell-escape, some code also wraps the whole argument in single quotes, which breaks the use of absolute paths:
$ touch 123
$ gh cs cp --codespace $CODESPACE 123 remote:/home/vscode/123
/usr/local/bin/scp: dest open "'/home/vscode/123'": No such file or directory
/usr/local/bin/scp: failed to upload file 123 to '/home/vscode/123'
Ah, I see what's happened. OpenSSH 9.0 was released last year, and with it the move to deprecate the legacy RCP protocol which relies on the remote shell in favor of SFTP. This is the initiative mentioned in the article linked in the source. As the release notes mention, this is a breaking change since that protocol does not involve the shell in any way, so paths are interpreted literally instead of using shell quoting rules. The 8.9 release notes make it clear that they're not going to do anything to mitigate this:
Legacy scp/rcp performs wildcard expansion of remote filenames (e.g. "scp host:* .") through the remote shell. This has the side effect of requiring double quoting of shell meta-characters in file names included on scp(1) command-lines, otherwise they could be interpreted as shell commands on the remote side.
This creates one area of potential incompatibility: scp(1) when using the SFTP protocol no longer requires this finicky and brittle quoting, and attempts to use it may cause transfers to fail. We consider the removal of the need for double-quoting shell characters in file names to be a benefit and do not intend to introduce bug-compatibility for legacy scp/rcp in scp(1) when using the SFTP protocol.
So under post-9.0 SSH versions, the quoting we do mangles the paths 100% of the time, for example, if I send a file called a it gets written as having literal single quotes in its destination file name, ie 'a':
$ touch a && gh cs cp --codespace $CODESPACE a remote:a
a 100% 0 0.0KB/s 00:00
$ gh cs ssh --codespace $CODESPACE 'ls -l | grep a'
total 148
-rw-r--r-- 1 vscode vscode 0 Jan 11 20:39 'a'
I hope we can agree that's broken behavior (yes, it can be disabled with -e). Unfortunately openssh didn't provide great tools for managing this breaking change -- the -s flag introduced in 8.7 to force SFTP protocol mode is marked as unstable and short-lived. -O forces legacy SCP mode and will likely be available for a while but was added pretty recently. So there's no stable set of scp flags that you can rely on to get consistent behavior across a wide variety of versions.
The options I can think of are:
- stop shelling out and use a pure go sftp client like https://github.com/pkg/sftp
- stop shelling out and copy files directly in RCP mode using
golang.org/x/crypto/ssh - parse the output of
ssh -Vand disable escaping if the system ssh is both OpenSSH and a version that defaults to the SFTP protocol (9.0+) - parse the output of
ssh -Vand opt in to legacy mode (-O) if it's an SFTP-by-default OpenSSH version
Personally I prefer a solution that lets us continue to shell out - the reason being to maintain the compatibility to pass scp args directly via gh cs cp ... -- <scp args>. If I understand 1 and 2, we would need to fully implement those arguments ourselves to continue supporting that.
For options 3 and 4, it seems not terrible to check ssh -V, but my concern is that there is another ssh implementation out there that also has this same behavior and then we get into a game of whack-a-mole. Is there an approach we could use that tells us if the ssh app does or does not quote already and react to that instead?
I'm not very hip with the times in terms of which ssh versions are common or not. If we believe that this OpenSSH 9.0+ is more common than not, a 5th option is to make not quoting the default and add a new flag to re-enable quoting.
@cmbrose yep, 1 or 2 would definitely be a breaking change, but it's ultimately just a propagation of the underlying breakage that OpenSSH has created by deprecating RCP and promoting SFTP to the default transport. That decision was intentionally weighed against the security problems of the previous approach.
To be clear, SFTP does support the most common bit of shell-style syntax, * for globbing files.
I think that 3 and 4 would require a fairly granular allowlist of scp implementations. Not sure what the status quo on windows is but I know putty also adopted SFTP as its scp transport quite some time ago.
Is there an approach we could use that tells us if the ssh app does or does not quote already and react to that instead?
Not that I'm aware of. You could determine it empirically by transferring a quoted file before executing the user's command and inspecting the result but that has lots of issues...
In terms of uptake, the latest versions of macos and Ubuntu are both on 9.0+ right now, Debian stable and fedora will be by July (bookworm promotion to stable) and May (fedora 38) respectively. WSL2 uses Debian stable, so I think it's likely that today SFTP-by-default clients are a sizeable minority which will become a majority by mid year.