Address run download feedback

- With no arguments in TTY mode, prompt which artifacts to download
- Change `--pattern` argument to be just `--name` and only do exact
  matching
- For multi-archive downloads, prefix the destination path with the name
  of the artifact
- Add tests exercising HTTP functionality
- Avoid "zipslip" path injection when extracting ZIP files
- Add tests for ZIP extraction

pkg/cmd/run/download/download.go

@@ -5,25 +5,33 @@ import (
 	"fmt"
 	"path/filepath"
 
+	"github.com/AlecAivazis/survey/v2"
 	"github.com/MakeNowJust/heredoc"
 	"github.com/cli/cli/pkg/cmdutil"
 	"github.com/cli/cli/pkg/iostreams"
+	"github.com/cli/cli/pkg/prompt"
+	"github.com/cli/cli/pkg/set"
 	"github.com/spf13/cobra"
 )
 
 type DownloadOptions struct {
 	IO       *iostreams.IOStreams
 	Platform platform
+	Prompter prompter
 
+	DoPrompt       bool
 	RunID          string
 	DestinationDir string
-	FilePatterns   []string
+	Names          []string
 }
 
 type platform interface {
 	List(runID string) ([]Artifact, error)
 	Download(url string, dir string) error
 }
+type prompter interface {
+	Prompt(message string, options []string, result interface{}) error
+}
 
 func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobra.Command {
 	opts := &DownloadOptions{
@@ -33,22 +41,29 @@ func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobr
 	cmd := &cobra.Command{
 		Use:   "download [<run-id>]",
 		Short: "Download artifacts generated by a workflow run",
-		Args:  cobra.MaximumNArgs(1),
+		Long: heredoc.Doc(`
+			Download artifacts generated by a GitHub Actions workflow run.
+
+			The contents of each artifact will be extracted under separate directories based on
+			the artifact name. If only a single artifact is specified, it will be extracted into
+			the current directory.
+		`),
+		Args: cobra.MaximumNArgs(1),
 		Example: heredoc.Doc(`
 		  # Download all artifacts generated by a workflow run
 		  $ gh run download <run-id>
 
 		  # Download a specific artifact within a run
-		  $ gh run download <run-id> -p <name>
+		  $ gh run download <run-id> -n <name>
 
 		  # Download specific artifacts across all runs in a repository
-		  $ gh run download -p <name1> -p <name2>
+		  $ gh run download -n <name1> -n <name2>
 		`),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if len(args) > 0 {
 				opts.RunID = args[0]
-			} else if len(opts.FilePatterns) == 0 {
-				return &cmdutil.FlagError{Err: errors.New("either run ID or `--pattern` is required")}
+			} else if len(opts.Names) == 0 && opts.IO.CanPrompt() {
+				opts.DoPrompt = true
 			}
 
 			// support `-R, --repo` override
@@ -64,6 +79,7 @@ func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobr
 				client: httpClient,
 				repo:   baseRepo,
 			}
+			opts.Prompter = &surveyPrompter{}
 
 			if runF != nil {
 				return runF(opts)
@@ -73,47 +89,79 @@ func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobr
 	}
 
 	cmd.Flags().StringVarP(&opts.DestinationDir, "dir", "D", ".", "The directory to download artifacts into")
-	cmd.Flags().StringArrayVarP(&opts.FilePatterns, "pattern", "p", nil, "Download only artifacts that match a glob pattern")
+	cmd.Flags().StringArrayVarP(&opts.Names, "name", "n", nil, "Only download artifacts that match any of the given names")
 
 	return cmd
 }
 
 func runDownload(opts *DownloadOptions) error {
 	opts.IO.StartProgressIndicator()
-	defer opts.IO.StopProgressIndicator()
-
 	artifacts, err := opts.Platform.List(opts.RunID)
+	opts.IO.StopProgressIndicator()
 	if err != nil {
 		return fmt.Errorf("error fetching artifacts: %w", err)
 	}
 
-	// track downloaded artifacts and avoid re-downloading any of the same name
-	downloaded := map[string]struct{}{}
-	numArtifacts := 0
-
+	numValidArtifacts := 0
 	for _, a := range artifacts {
 		if a.Expired {
 			continue
 		}
-		numArtifacts++
-		if _, found := downloaded[a.Name]; found {
+		numValidArtifacts++
+	}
+	if numValidArtifacts == 0 {
+		return errors.New("no valid artifacts found to download")
+	}
+
+	wantNames := opts.Names
+	if opts.DoPrompt {
+		artifactNames := set.NewStringSet()
+		for _, a := range artifacts {
+			if !a.Expired {
+				artifactNames.Add(a.Name)
+			}
+		}
+		options := artifactNames.ToSlice()
+		if len(options) > 10 {
+			options = options[:10]
+		}
+		err := opts.Prompter.Prompt("Select artifacts to download:", options, &wantNames)
+		if err != nil {
+			return err
+		}
+		if len(wantNames) == 0 {
+			return errors.New("no artifacts selected")
+		}
+	}
+
+	opts.IO.StartProgressIndicator()
+	defer opts.IO.StopProgressIndicator()
+
+	// track downloaded artifacts and avoid re-downloading any of the same name
+	downloaded := set.NewStringSet()
+	for _, a := range artifacts {
+		if a.Expired {
 			continue
 		}
-		if len(opts.FilePatterns) > 0 && !matchAny(opts.FilePatterns, a.Name) {
+		if downloaded.Contains(a.Name) {
 			continue
 		}
-		err := opts.Platform.Download(a.DownloadURL, opts.DestinationDir)
+		if len(wantNames) > 0 && !matchAny(wantNames, a.Name) {
+			continue
+		}
+		destDir := opts.DestinationDir
+		if len(wantNames) != 1 {
+			destDir = filepath.Join(destDir, a.Name)
+		}
+		err := opts.Platform.Download(a.DownloadURL, destDir)
 		if err != nil {
 			return fmt.Errorf("error downloading %s: %w", a.Name, err)
 		}
-		downloaded[a.Name] = struct{}{}
+		downloaded.Add(a.Name)
 	}
 
-	if numArtifacts == 0 {
-		return errors.New("no valid artifacts found to download")
-	}
-	if len(downloaded) == 0 {
-		return errors.New("no artifact matches any of the patterns provided")
+	if downloaded.Len() == 0 {
+		return errors.New("no artifact matches any of the names provided")
 	}
 
 	return nil
@@ -121,9 +169,18 @@ func runDownload(opts *DownloadOptions) error {
 
 func matchAny(patterns []string, name string) bool {
 	for _, p := range patterns {
-		if isMatch, err := filepath.Match(p, name); err == nil && isMatch {
+		if name == p {
 			return true
 		}
 	}
 	return false
 }
+
+type surveyPrompter struct{}
+
+func (sp *surveyPrompter) Prompt(message string, options []string, result interface{}) error {
+	return prompt.SurveyAskOne(&survey.MultiSelect{
+		Message: message,
+		Options: options,
+	}, result)
+}

pkg/cmd/run/download/download_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"io/ioutil"
 	"net/http"
+	"path/filepath"
 	"testing"
 
 	"github.com/cli/cli/internal/ghrepo"
@@ -11,6 +12,7 @@ import (
 	"github.com/cli/cli/pkg/iostreams"
 	"github.com/google/shlex"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
@@ -23,10 +25,15 @@ func Test_NewCmdDownload(t *testing.T) {
 		wantErr string
 	}{
 		{
-			name:    "empty",
-			args:    "",
-			isTTY:   true,
-			wantErr: "either run ID or `--pattern` is required",
+			name:  "empty",
+			args:  "",
+			isTTY: true,
+			want: DownloadOptions{
+				RunID:          "",
+				DoPrompt:       true,
+				Names:          []string(nil),
+				DestinationDir: ".",
+			},
 		},
 		{
 			name:  "with run ID",
@@ -34,7 +41,8 @@ func Test_NewCmdDownload(t *testing.T) {
 			isTTY: true,
 			want: DownloadOptions{
 				RunID:          "2345",
-				FilePatterns:   []string(nil),
+				DoPrompt:       false,
+				Names:          []string(nil),
 				DestinationDir: ".",
 			},
 		},
@@ -44,17 +52,19 @@ func Test_NewCmdDownload(t *testing.T) {
 			isTTY: true,
 			want: DownloadOptions{
 				RunID:          "2345",
-				FilePatterns:   []string(nil),
+				DoPrompt:       false,
+				Names:          []string(nil),
 				DestinationDir: "tmp/dest",
 			},
 		},
 		{
-			name:  "repo level with patterns",
-			args:  "-p one -p two",
+			name:  "repo level with names",
+			args:  "-n one -n two",
 			isTTY: true,
 			want: DownloadOptions{
 				RunID:          "",
-				FilePatterns:   []string{"one", "two"},
+				DoPrompt:       false,
+				Names:          []string{"one", "two"},
 				DestinationDir: ".",
 			},
 		},
@@ -100,47 +110,136 @@ func Test_NewCmdDownload(t *testing.T) {
 			}
 
 			assert.Equal(t, tt.want.RunID, opts.RunID)
-			assert.Equal(t, tt.want.FilePatterns, opts.FilePatterns)
+			assert.Equal(t, tt.want.Names, opts.Names)
 			assert.Equal(t, tt.want.DestinationDir, opts.DestinationDir)
+			assert.Equal(t, tt.want.DoPrompt, opts.DoPrompt)
 		})
 	}
 }
 
 func Test_runDownload(t *testing.T) {
 	tests := []struct {
-		name           string
-		opts           DownloadOptions
-		artifacts      []Artifact
-		wantDownloaded []string
-		wantErr        string
+		name       string
+		opts       DownloadOptions
+		mockAPI    func(*mockPlatform)
+		mockPrompt func(*mockPrompter)
+		wantErr    string
 	}{
 		{
 			name: "download non-expired",
+			opts: DownloadOptions{
+				RunID:          "2345",
+				DestinationDir: "./tmp",
+				Names:          []string(nil),
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]Artifact{
+					{
+						Name:        "artifact-1",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+					{
+						Name:        "expired-artifact",
+						DownloadURL: "http://download.com/expired.zip",
+						Expired:     true,
+					},
+					{
+						Name:        "artifact-2",
+						DownloadURL: "http://download.com/artifact2.zip",
+						Expired:     false,
+					},
+				}, nil)
+				p.On("Download", "http://download.com/artifact1.zip", filepath.FromSlash("tmp/artifact-1")).Return(nil)
+				p.On("Download", "http://download.com/artifact2.zip", filepath.FromSlash("tmp/artifact-2")).Return(nil)
+			},
+		},
+		{
+			name: "no valid artifacts",
 			opts: DownloadOptions{
 				RunID:          "2345",
 				DestinationDir: ".",
-				FilePatterns:   []string(nil),
+				Names:          []string(nil),
 			},
-			artifacts: []Artifact{
-				{
-					Name:        "artifact-1",
-					DownloadURL: "http://download.com/artifact-1.zip",
-					Expired:     false,
-				},
-				{
-					Name:        "expired-artifact",
-					DownloadURL: "http://download.com/expired.zip",
-					Expired:     true,
-				},
-				{
-					Name:        "artifact-2",
-					DownloadURL: "http://download.com/artifact-2.zip",
-					Expired:     false,
-				},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]Artifact{
+					{
+						Name:        "artifact-1",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     true,
+					},
+					{
+						Name:        "artifact-2",
+						DownloadURL: "http://download.com/artifact2.zip",
+						Expired:     true,
+					},
+				}, nil)
 			},
-			wantDownloaded: []string{
-				"http://download.com/artifact-1.zip",
-				"http://download.com/artifact-2.zip",
+			wantErr: "no valid artifacts found to download",
+		},
+		{
+			name: "no matches",
+			opts: DownloadOptions{
+				RunID:          "2345",
+				DestinationDir: ".",
+				Names:          []string{"artifact-3"},
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "2345").Return([]Artifact{
+					{
+						Name:        "artifact-1",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+					{
+						Name:        "artifact-2",
+						DownloadURL: "http://download.com/artifact2.zip",
+						Expired:     false,
+					},
+				}, nil)
+			},
+			wantErr: "no artifact matches any of the names provided",
+		},
+		{
+			name: "prompt to select artifact",
+			opts: DownloadOptions{
+				RunID:          "",
+				DoPrompt:       true,
+				DestinationDir: ".",
+				Names:          []string(nil),
+			},
+			mockAPI: func(p *mockPlatform) {
+				p.On("List", "").Return([]Artifact{
+					{
+						Name:        "artifact-1",
+						DownloadURL: "http://download.com/artifact1.zip",
+						Expired:     false,
+					},
+					{
+						Name:        "expired-artifact",
+						DownloadURL: "http://download.com/expired.zip",
+						Expired:     true,
+					},
+					{
+						Name:        "artifact-2",
+						DownloadURL: "http://download.com/artifact2.zip",
+						Expired:     false,
+					},
+					{
+						Name:        "artifact-2",
+						DownloadURL: "http://download.com/artifact2.also.zip",
+						Expired:     false,
+					},
+				}, nil)
+				p.On("Download", "http://download.com/artifact2.zip", ".").Return(nil)
+			},
+			mockPrompt: func(p *mockPrompter) {
+				p.On("Prompt", "Select artifacts to download:", []string{"artifact-1", "artifact-2"}, mock.AnythingOfType("*[]string")).
+					Run(func(args mock.Arguments) {
+						result := args.Get(2).(*[]string)
+						*result = []string{"artifact-2"}
+					}).
+					Return(nil)
 			},
 		},
 	}
@@ -149,8 +248,8 @@ func Test_runDownload(t *testing.T) {
 			opts := &tt.opts
 			io, _, stdout, stderr := iostreams.Test()
 			opts.IO = io
-			platform := &stubPlatform{listResults: tt.artifacts}
-			opts.Platform = platform
+			opts.Platform = newMockPlatform(t, tt.mockAPI)
+			opts.Prompter = newMockPrompter(t, tt.mockPrompt)
 
 			err := runDownload(opts)
 			if tt.wantErr != "" {
@@ -159,23 +258,55 @@ func Test_runDownload(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			assert.Equal(t, tt.wantDownloaded, platform.downloaded)
 			assert.Equal(t, "", stdout.String())
 			assert.Equal(t, "", stderr.String())
 		})
 	}
 }
 
-type stubPlatform struct {
-	listResults []Artifact
-	downloaded  []string
+type mockPlatform struct {
+	mock.Mock
 }
 
-func (p *stubPlatform) List(runID string) ([]Artifact, error) {
-	return p.listResults, nil
+func newMockPlatform(t *testing.T, config func(*mockPlatform)) *mockPlatform {
+	m := &mockPlatform{}
+	m.Test(t)
+	t.Cleanup(func() {
+		m.AssertExpectations(t)
+	})
+	if config != nil {
+		config(m)
+	}
+	return m
 }
 
-func (p *stubPlatform) Download(url string, dir string) error {
-	p.downloaded = append(p.downloaded, url)
-	return nil
+func (p *mockPlatform) List(runID string) ([]Artifact, error) {
+	args := p.Called(runID)
+	return args.Get(0).([]Artifact), args.Error(1)
+}
+
+func (p *mockPlatform) Download(url string, dir string) error {
+	args := p.Called(url, dir)
+	return args.Error(0)
+}
+
+type mockPrompter struct {
+	mock.Mock
+}
+
+func newMockPrompter(t *testing.T, config func(*mockPrompter)) *mockPrompter {
+	m := &mockPrompter{}
+	m.Test(t)
+	t.Cleanup(func() {
+		m.AssertExpectations(t)
+	})
+	if config != nil {
+		config(m)
+	}
+	return m
+}
+
+func (p *mockPrompter) Prompt(msg string, opts []string, res interface{}) error {
+	args := p.Called(msg, opts, res)
+	return args.Error(0)
 }

pkg/cmd/run/download/http.go

@@ -1,6 +1,7 @@
 package download
 
 import (
+	"archive/zip"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -62,7 +63,7 @@ func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string)
 
 	dec := json.NewDecoder(resp.Body)
 	if err := dec.Decode(&response); err != nil {
-		return response.Artifacts, fmt.Errorf("error reading JSON: %w", err)
+		return response.Artifacts, fmt.Errorf("error parsing JSON: %w", err)
 	}
 
 	return response.Artifacts, nil
@@ -73,8 +74,8 @@ func downloadArtifact(httpClient *http.Client, url, destDir string) error {
 	if err != nil {
 		return err
 	}
-	// The API rejects requesting the "application/octet-stream" type :(
-	req.Header.Set("Accept", "application/json")
+	// The server rejects this :(
+	//req.Header.Set("Accept", "application/zip")
 
 	resp, err := httpClient.Do(req)
 	if err != nil {
@@ -90,19 +91,22 @@ func downloadArtifact(httpClient *http.Client, url, destDir string) error {
 	if err != nil {
 		return fmt.Errorf("error initializing temporary file: %w", err)
 	}
-	defer tmpfile.Close()
+	defer func() {
+		_ = tmpfile.Close()
+		_ = os.Remove(tmpfile.Name())
+	}()
 
-	_, err = io.Copy(tmpfile, resp.Body)
+	size, err := io.Copy(tmpfile, resp.Body)
 	if err != nil {
 		return fmt.Errorf("error writing zip archive: %w", err)
 	}
 
-	tmpfile.Close()
-	if err := extractZip(tmpfile, destDir); err != nil {
+	zipfile, err := zip.NewReader(tmpfile, size)
+	if err != nil {
 		return fmt.Errorf("error extracting zip archive: %w", err)
 	}
-	if err := os.Remove(tmpfile.Name()); err != nil {
-		return err
+	if err := extractZip(zipfile, destDir); err != nil {
+		return fmt.Errorf("error extracting zip archive: %w", err)
 	}
 
 	return nil

pkg/cmd/run/download/http_test.go

@@ -0,0 +1,106 @@
+package download
+
+import (
+	"net/http"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/cli/cli/internal/ghrepo"
+	"github.com/cli/cli/pkg/httpmock"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_List(t *testing.T) {
+	reg := &httpmock.Registry{}
+	defer reg.Verify(t)
+
+	reg.Register(
+		httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/123/artifacts"),
+		httpmock.StringResponse(`{
+			"total_count": 2,
+			"artifacts": [
+				{"name": "artifact-1"},
+				{"name": "artifact-2"}
+			]
+		}`))
+
+	api := &apiPlatform{
+		client: &http.Client{Transport: reg},
+		repo:   ghrepo.New("OWNER", "REPO"),
+	}
+	artifacts, err := api.List("123")
+	require.NoError(t, err)
+
+	require.Equal(t, 2, len(artifacts))
+	assert.Equal(t, "artifact-1", artifacts[0].Name)
+	assert.Equal(t, "artifact-2", artifacts[1].Name)
+}
+
+func Test_List_perRepository(t *testing.T) {
+	reg := &httpmock.Registry{}
+	defer reg.Verify(t)
+
+	reg.Register(
+		httpmock.REST("GET", "repos/OWNER/REPO/actions/artifacts"),
+		httpmock.StringResponse(`{}`))
+
+	api := &apiPlatform{
+		client: &http.Client{Transport: reg},
+		repo:   ghrepo.New("OWNER", "REPO"),
+	}
+	_, err := api.List("")
+	require.NoError(t, err)
+}
+
+func Test_Download(t *testing.T) {
+	tmpDir := t.TempDir()
+	destDir := filepath.Join(tmpDir, "artifact")
+
+	reg := &httpmock.Registry{}
+	defer reg.Verify(t)
+
+	reg.Register(
+		httpmock.REST("GET", "repos/OWNER/REPO/actions/artifacts/12345/zip"),
+		httpmock.FileResponse("./fixtures/myproject.zip"))
+
+	api := &apiPlatform{
+		client: &http.Client{Transport: reg},
+	}
+	err := api.Download("https://api.github.com/repos/OWNER/REPO/actions/artifacts/12345/zip", destDir)
+	require.NoError(t, err)
+
+	var paths []string
+	parentPrefix := tmpDir + string(filepath.Separator)
+	err = filepath.Walk(tmpDir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if path == tmpDir {
+			return nil
+		}
+		entry := strings.TrimPrefix(path, parentPrefix)
+		if info.IsDir() {
+			entry += "/"
+		} else if info.Mode()&0111 != 0 {
+			entry += "(X)"
+		}
+		paths = append(paths, entry)
+		return nil
+	})
+	require.NoError(t, err)
+
+	sort.Strings(paths)
+	assert.Equal(t, []string{
+		"artifact/",
+		"artifact/bin/",
+		"artifact/bin/myexe",
+		"artifact/readme.md",
+		"artifact/src/",
+		"artifact/src/main.go",
+		"artifact/src/util.go",
+	}, paths)
+}

pkg/cmd/run/download/zip.go

@@ -6,29 +6,33 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 )
 
-func extractZip(zipfile *os.File, destDir string) error {
-	zr, err := zip.OpenReader(zipfile.Name())
-	if err != nil {
-		return err
-	}
-	defer zr.Close()
+const (
+	dirMode  os.FileMode = 0755
+	fileMode os.FileMode = 0644
+	execMode os.FileMode = 0755
+)
 
+func extractZip(zr *zip.Reader, destDir string) error {
+	pathPrefix := filepath.Clean(destDir) + string(filepath.Separator)
 	for _, zf := range zr.File {
-		fpath := filepath.Join(destDir, zf.Name)
+		fpath := filepath.Join(destDir, filepath.FromSlash(zf.Name))
+		if !strings.HasPrefix(fpath, pathPrefix) {
+			continue
+		}
 		if err := extractZipFile(zf, fpath); err != nil {
 			return fmt.Errorf("error extracting %q: %w", zf.Name, err)
 		}
 	}
-
 	return nil
 }
 
 func extractZipFile(zf *zip.File, dest string) error {
 	zm := zf.Mode()
 	if zm.IsDir() {
-		return os.MkdirAll(dest, 0755)
+		return os.MkdirAll(dest, dirMode)
 	}
 
 	f, err := zf.Open()
@@ -37,15 +41,13 @@ func extractZipFile(zf *zip.File, dest string) error {
 	}
 	defer f.Close()
 
-	if err := os.MkdirAll(filepath.Dir(dest), 0755); err != nil {
-		return err
+	if dir := filepath.Dir(dest); dir != "." {
+		if err := os.MkdirAll(dir, dirMode); err != nil {
+			return err
+		}
 	}
 
-	var mode os.FileMode = 0644
-	if isBinary(zm) {
-		mode = 0755
-	}
-	df, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, mode)
+	df, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, getPerm(zm))
 	if err != nil {
 		return err
 	}
@@ -55,6 +57,9 @@ func extractZipFile(zf *zip.File, dest string) error {
 	return err
 }
 
-func isBinary(m os.FileMode) bool {
-	return m&0111 != 0
+func getPerm(m os.FileMode) os.FileMode {
+	if m&0111 == 0 {
+		return fileMode
+	}
+	return execMode
 }