Merge pull request cli#4564 from cli/e-flag

adonovan · web-flow · commit fc1de3aaa0c3 · 2021-10-19T11:38:19.000-04:00
gh cs cp: disable remote shell expansion unless -e flag
diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go
@@ -27,8 +27,10 @@ func Shell(ctx context.Context, log logger, sshArgs []string, port int, destinat
 
 // Copy runs an scp command over the specified port. The arguments may
 // include flags and non-flags, optionally separated by "--".
-// Remote files are indicated by a "remote:" prefix, and are resolved
-// relative to the remote user's home directory.
+//
+// Remote files indicated by a "remote:" prefix are resolved relative
+// to the remote user's home directory, and are subject to shell expansion
+// on the remote host; see https://lwn.net/Articles/835962/.
 func Copy(ctx context.Context, scpArgs []string, port int, destination string) error {
 	// Beware: invalid syntax causes scp to exit 1 with
 	// no error message, so don't let that happen.
diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go
@@ -145,26 +145,39 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e
 type cpOptions struct {
 	sshOptions
 	recursive bool // -r
+	expand    bool // -e
 }
 
 func newCpCmd(app *App) *cobra.Command {
 	var opts cpOptions
 
 	cpCmd := &cobra.Command{
-		Use:   "cp [-r] srcs... dest",
+		Use:   "cp [-e] [-r] srcs... dest",
 		Short: "Copy files between local and remote file systems",
 		Long: `
 The cp command copies files between the local and remote file systems.
 
-A 'remote:' prefix on any file name argument indicates that it refers to
-the file system of the remote (Codespace) machine. It is resolved relative
-to the home directory of the remote user.
-
 As with the UNIX cp command, the first argument specifies the source and the last
 specifies the destination; additional sources may be specified after the first,
 if the destination is a directory.
 
 The -r (recursive) flag is required if any source is a directory.
+
+A 'remote:' prefix on any file name argument indicates that it refers to
+the file system of the remote (Codespace) machine. It is resolved relative
+to the home directory of the remote user.
+
+By default, remote file names are interpreted literally. With the -e flag,
+each such argument is treated in the manner of scp, as a Bash expression to
+be evaluated on the remote machine, subject to expansion of tildes, braces,
+globs, environment variables, and backticks, as in these examples:
+
+ $ gh codespace cp -e README.md 'remote:/workspace/$RepositoryName/'
+ $ gh codespace cp -e 'remote:~/*.go' ./gofiles/
+ $ gh codespace cp -e 'remote:/workspace/myproj/go.{mod,sum}' ./gofiles/
+
+For security, do not use the -e flag with arguments provided by untrusted
+users; see https://lwn.net/Articles/835962/ for discussion.
 `,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return app.Copy(cmd.Context(), args, opts)
@@ -173,6 +186,7 @@ The -r (recursive) flag is required if any source is a directory.
 
 	// We don't expose all sshOptions.
 	cpCmd.Flags().BoolVarP(&opts.recursive, "recursive", "r", false, "Recursively copy directories")
+	cpCmd.Flags().BoolVarP(&opts.expand, "expand", "e", false, "Expand remote file names on remote shell")
 	cpCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace")
 	return cpCmd
 }
@@ -188,7 +202,18 @@ func (a *App) Copy(ctx context.Context, args []string, opts cpOptions) (err erro
 	}
 	opts.scpArgs = append(opts.scpArgs, "--")
 	for _, arg := range args {
-		if !filepath.IsAbs(arg) && !strings.HasPrefix(arg, "remote:") {
+		if rest := strings.TrimPrefix(arg, "remote:"); rest != arg {
+			// scp treats each filename argument as a shell expression,
+			// subjecting it to expansion of environment variables, braces,
+			// tilde, backticks, globs and so on. Because these present a
+			// security risk (see https://lwn.net/Articles/835962/), we
+			// disable them by shell-escaping the argument unless the user
+			// provided the -e flag.
+			if !opts.expand {
+				arg = `remote:'` + strings.Replace(rest, `'`, `'\''`, -1) + `'`
+			}
+
+		} else if !filepath.IsAbs(arg) {
 			// scp treats a colon in the first path segment as a host identifier.
 			// Escape it by prepending "./".
 			// TODO(adonovan): test on Windows, including with a c:\\foo path.
