Merge commit from fork
The fix
This commit is contained in:
Tyler McGoffin
GitHub
commit
1136764c36
4 changed files with 629 additions and 133 deletions
|
|
@ -151,8 +151,10 @@ func runDownload(opts *DownloadOptions) error {
|
|||
opts.IO.StartProgressIndicator()
|
||||
defer opts.IO.StopProgressIndicator()
|
||||
|
||||
// track downloaded artifacts and avoid re-downloading any of the same name
|
||||
// track downloaded artifacts and avoid re-downloading any of the same name, isolate if multiple artifacts
|
||||
downloaded := set.NewStringSet()
|
||||
isolateArtifacts := isolateArtifacts(wantNames, wantPatterns)
|
||||
|
||||
for _, a := range artifacts {
|
||||
if a.Expired {
|
||||
continue
|
||||
|
|
@ -165,10 +167,16 @@ func runDownload(opts *DownloadOptions) error {
|
|||
continue
|
||||
}
|
||||
}
|
||||
|
||||
destDir := opts.DestinationDir
|
||||
if len(wantPatterns) != 0 || len(wantNames) != 1 {
|
||||
if isolateArtifacts {
|
||||
destDir = filepath.Join(destDir, a.Name)
|
||||
}
|
||||
|
||||
if !filepathDescendsFrom(destDir, opts.DestinationDir) {
|
||||
return fmt.Errorf("error downloading %s: would result in path traversal", a.Name)
|
||||
}
|
||||
|
||||
err := opts.Platform.Download(a.DownloadURL, destDir)
|
||||
if err != nil {
|
||||
return fmt.Errorf("error downloading %s: %w", a.Name, err)
|
||||
|
|
@ -183,6 +191,25 @@ func runDownload(opts *DownloadOptions) error {
|
|||
return nil
|
||||
}
|
||||
|
||||
func isolateArtifacts(wantNames []string, wantPatterns []string) bool {
|
||||
if len(wantPatterns) > 0 {
|
||||
// Patterns can match multiple artifacts
|
||||
return true
|
||||
}
|
||||
|
||||
if len(wantNames) == 0 {
|
||||
// All artifacts wanted regardless what they are named
|
||||
return true
|
||||
}
|
||||
|
||||
if len(wantNames) > 1 {
|
||||
// Multiple, specific artifacts wanted
|
||||
return true
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
func matchAnyName(names []string, name string) bool {
|
||||
for _, n := range names {
|
||||
if name == n {
|
||||
|
|
|
|||
|
|
@ -2,8 +2,11 @@ package download
|
|||
|
||||
import (
|
||||
"bytes"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
|
||||
|
|
@ -14,7 +17,6 @@ import (
|
|||
"github.com/cli/cli/v2/pkg/iostreams"
|
||||
"github.com/google/shlex"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/mock"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
|
|
@ -143,159 +145,537 @@ func Test_NewCmdDownload(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
type testArtifact struct {
|
||||
artifact shared.Artifact
|
||||
files []string
|
||||
}
|
||||
|
||||
type fakePlatform struct {
|
||||
runArtifacts map[string][]testArtifact
|
||||
}
|
||||
|
||||
func (f *fakePlatform) List(runID string) ([]shared.Artifact, error) {
|
||||
var runIds []string
|
||||
if runID != "" {
|
||||
runIds = []string{runID}
|
||||
} else {
|
||||
for k := range f.runArtifacts {
|
||||
runIds = append(runIds, k)
|
||||
}
|
||||
}
|
||||
|
||||
var artifacts []shared.Artifact
|
||||
for _, id := range runIds {
|
||||
for _, a := range f.runArtifacts[id] {
|
||||
artifacts = append(artifacts, a.artifact)
|
||||
}
|
||||
}
|
||||
|
||||
return artifacts, nil
|
||||
}
|
||||
|
||||
func (f *fakePlatform) Download(url string, dir string) error {
|
||||
if err := os.MkdirAll(dir, 0755); err != nil {
|
||||
return err
|
||||
}
|
||||
// Now to be consistent, we find the artifact with the provided URL.
|
||||
// It's a bit janky to iterate the runs, to find the right artifact
|
||||
// rather than keying directly to it, but it allows the setup of the
|
||||
// fake platform to be declarative rather than imperative.
|
||||
// Think fakePlatform { artifacts: ... } rather than fakePlatform.makeArtifactAvailable()
|
||||
for _, testArtifacts := range f.runArtifacts {
|
||||
for _, testArtifact := range testArtifacts {
|
||||
if testArtifact.artifact.DownloadURL == url {
|
||||
for _, file := range testArtifact.files {
|
||||
path := filepath.Join(dir, file)
|
||||
return os.WriteFile(path, []byte{}, 0600)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return errors.New("no artifact matches the provided URL")
|
||||
}
|
||||
|
||||
func Test_runDownload(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
opts DownloadOptions
|
||||
mockAPI func(*mockPlatform)
|
||||
promptStubs func(*prompter.MockPrompter)
|
||||
wantErr string
|
||||
name string
|
||||
opts DownloadOptions
|
||||
platform *fakePlatform
|
||||
promptStubs func(*prompter.MockPrompter)
|
||||
expectedFiles []string
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "download non-expired",
|
||||
name: "download non-expired to relative directory",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
DestinationDir: "./tmp",
|
||||
Names: []string(nil),
|
||||
},
|
||||
mockAPI: func(p *mockPlatform) {
|
||||
p.On("List", "2345").Return([]shared.Artifact{
|
||||
{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "expired-artifact",
|
||||
DownloadURL: "http://download.com/expired.zip",
|
||||
Expired: true,
|
||||
},
|
||||
files: []string{
|
||||
"expired",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-2-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
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)
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{
|
||||
filepath.Join("artifact-1", "artifact-1-file"),
|
||||
filepath.Join("artifact-2", "artifact-2-file"),
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "no valid artifacts",
|
||||
name: "download non-expired to absolute directory",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
DestinationDir: ".",
|
||||
Names: []string(nil),
|
||||
DestinationDir: "/tmp",
|
||||
},
|
||||
mockAPI: func(p *mockPlatform) {
|
||||
p.On("List", "2345").Return([]shared.Artifact{
|
||||
{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: true,
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "expired-artifact",
|
||||
DownloadURL: "http://download.com/expired.zip",
|
||||
Expired: true,
|
||||
},
|
||||
files: []string{
|
||||
"expired",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-2-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: true,
|
||||
},
|
||||
}, nil)
|
||||
},
|
||||
},
|
||||
wantErr: "no valid artifacts found to download",
|
||||
expectedFiles: []string{
|
||||
filepath.Join("artifact-1", "artifact-1-file"),
|
||||
filepath.Join("artifact-2", "artifact-2-file"),
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "all artifacts are expired",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
},
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: true,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: true,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-2-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{},
|
||||
wantErr: "no valid artifacts found to download",
|
||||
},
|
||||
{
|
||||
name: "no name matches",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
DestinationDir: ".",
|
||||
Names: []string{"artifact-3"},
|
||||
RunID: "2345",
|
||||
Names: []string{"artifact-3"},
|
||||
},
|
||||
mockAPI: func(p *mockPlatform) {
|
||||
p.On("List", "2345").Return([]shared.Artifact{
|
||||
{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-2-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
}, nil)
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{},
|
||||
wantErr: "no artifact matches any of the names or patterns provided",
|
||||
},
|
||||
{
|
||||
name: "pattern matches",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
FilePatterns: []string{"artifact-*"},
|
||||
},
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "non-artifact-2",
|
||||
DownloadURL: "http://download.com/non-artifact-2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"non-artifact-2-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-3",
|
||||
DownloadURL: "http://download.com/artifact3.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-3-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{
|
||||
filepath.Join("artifact-1", "artifact-1-file"),
|
||||
filepath.Join("artifact-3", "artifact-3-file"),
|
||||
},
|
||||
wantErr: "no artifact matches any of the names or patterns provided",
|
||||
},
|
||||
{
|
||||
name: "no pattern matches",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
DestinationDir: ".",
|
||||
FilePatterns: []string{"artifiction-*"},
|
||||
RunID: "2345",
|
||||
FilePatterns: []string{"artifiction-*"},
|
||||
},
|
||||
mockAPI: func(p *mockPlatform) {
|
||||
p.On("List", "2345").Return([]shared.Artifact{
|
||||
{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-2-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
}, nil)
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{},
|
||||
wantErr: "no artifact matches any of the names or patterns provided",
|
||||
},
|
||||
{
|
||||
name: "want specific single artifact",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
Names: []string{"non-artifact-2"},
|
||||
},
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "non-artifact-2",
|
||||
DownloadURL: "http://download.com/non-artifact-2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"non-artifact-2-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-3",
|
||||
DownloadURL: "http://download.com/artifact3.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-3-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{
|
||||
filepath.Join("non-artifact-2-file"),
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "want specific multiple artifacts",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
Names: []string{"artifact-1", "artifact-3"},
|
||||
},
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "non-artifact-2",
|
||||
DownloadURL: "http://download.com/non-artifact-2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"non-artifact-2-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-3",
|
||||
DownloadURL: "http://download.com/artifact3.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-3-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{
|
||||
filepath.Join("artifact-1", "artifact-1-file"),
|
||||
filepath.Join("artifact-3", "artifact-3-file"),
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "avoid redownloading files of the same name",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
},
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-2-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{
|
||||
filepath.Join("artifact-1", "artifact-1-file"),
|
||||
},
|
||||
wantErr: "no artifact matches any of the names or patterns provided",
|
||||
},
|
||||
{
|
||||
name: "prompt to select artifact",
|
||||
opts: DownloadOptions{
|
||||
RunID: "",
|
||||
DoPrompt: true,
|
||||
DestinationDir: ".",
|
||||
Names: []string(nil),
|
||||
RunID: "",
|
||||
DoPrompt: true,
|
||||
Names: []string(nil),
|
||||
},
|
||||
mockAPI: func(p *mockPlatform) {
|
||||
p.On("List", "").Return([]shared.Artifact{
|
||||
{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-1",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-1-file",
|
||||
},
|
||||
},
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "expired-artifact",
|
||||
DownloadURL: "http://download.com/expired.zip",
|
||||
Expired: true,
|
||||
},
|
||||
files: []string{
|
||||
"expired",
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
Name: "expired-artifact",
|
||||
DownloadURL: "http://download.com/expired.zip",
|
||||
Expired: true,
|
||||
"6789": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "artifact-2",
|
||||
DownloadURL: "http://download.com/artifact2.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"artifact-2-file",
|
||||
},
|
||||
},
|
||||
},
|
||||
{
|
||||
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)
|
||||
},
|
||||
},
|
||||
promptStubs: func(pm *prompter.MockPrompter) {
|
||||
pm.RegisterMultiSelect("Select artifacts to download:", nil, []string{"artifact-1", "artifact-2"},
|
||||
func(_ string, _, opts []string) ([]int, error) {
|
||||
return []int{1}, nil
|
||||
for i, o := range opts {
|
||||
if o == "artifact-2" {
|
||||
return []int{i}, nil
|
||||
}
|
||||
}
|
||||
return nil, fmt.Errorf("no artifact-2 found in %v", opts)
|
||||
})
|
||||
},
|
||||
expectedFiles: []string{
|
||||
filepath.Join("artifact-2-file"),
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "handling artifact name with path traversal exploit",
|
||||
opts: DownloadOptions{
|
||||
RunID: "2345",
|
||||
},
|
||||
platform: &fakePlatform{
|
||||
runArtifacts: map[string][]testArtifact{
|
||||
"2345": {
|
||||
{
|
||||
artifact: shared.Artifact{
|
||||
Name: "..",
|
||||
DownloadURL: "http://download.com/artifact1.zip",
|
||||
Expired: false,
|
||||
},
|
||||
files: []string{
|
||||
"etc/passwd",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
expectedFiles: []string{},
|
||||
wantErr: "error downloading ..: would result in path traversal",
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
opts := &tt.opts
|
||||
if opts.DestinationDir == "" {
|
||||
opts.DestinationDir = t.TempDir()
|
||||
} else {
|
||||
opts.DestinationDir = filepath.Join(t.TempDir(), opts.DestinationDir)
|
||||
}
|
||||
|
||||
ios, _, stdout, stderr := iostreams.Test()
|
||||
opts.IO = ios
|
||||
opts.Platform = newMockPlatform(t, tt.mockAPI)
|
||||
opts.Platform = tt.platform
|
||||
|
||||
pm := prompter.NewMockPrompter(t)
|
||||
opts.Prompter = pm
|
||||
|
|
@ -310,34 +690,31 @@ func Test_runDownload(t *testing.T) {
|
|||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
// Check that the exact number of files exist
|
||||
require.Equal(t, len(tt.expectedFiles), countFilesInDirRecursively(t, opts.DestinationDir))
|
||||
|
||||
// Then check that the exact files are correct
|
||||
for _, name := range tt.expectedFiles {
|
||||
require.FileExists(t, filepath.Join(opts.DestinationDir, name))
|
||||
}
|
||||
|
||||
assert.Equal(t, "", stdout.String())
|
||||
assert.Equal(t, "", stderr.String())
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
type mockPlatform struct {
|
||||
mock.Mock
|
||||
}
|
||||
func countFilesInDirRecursively(t *testing.T, dir string) int {
|
||||
t.Helper()
|
||||
|
||||
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
|
||||
}
|
||||
count := 0
|
||||
require.NoError(t, filepath.Walk(dir, func(_ string, info os.FileInfo, err error) error {
|
||||
require.NoError(t, err)
|
||||
if !info.IsDir() {
|
||||
count++
|
||||
}
|
||||
return nil
|
||||
}))
|
||||
|
||||
func (p *mockPlatform) List(runID string) ([]shared.Artifact, error) {
|
||||
args := p.Called(runID)
|
||||
return args.Get(0).([]shared.Artifact), args.Error(1)
|
||||
}
|
||||
|
||||
func (p *mockPlatform) Download(url string, dir string) error {
|
||||
args := p.Called(url, dir)
|
||||
return args.Error(0)
|
||||
return count
|
||||
}
|
||||
|
|
|
|||
|
|
@ -71,13 +71,25 @@ func getPerm(m os.FileMode) os.FileMode {
|
|||
}
|
||||
|
||||
func filepathDescendsFrom(p, dir string) bool {
|
||||
// Regardless of the logic below, `p` is never allowed to be current directory `.` or parent directory `..`
|
||||
// however we check explicitly here before filepath.Rel() which doesn't cover all cases.
|
||||
p = filepath.Clean(p)
|
||||
dir = filepath.Clean(dir)
|
||||
if dir == "." && !filepath.IsAbs(p) {
|
||||
return !strings.HasPrefix(p, ".."+string(filepath.Separator))
|
||||
|
||||
if p == "." || p == ".." {
|
||||
return false
|
||||
}
|
||||
if !strings.HasSuffix(dir, string(filepath.Separator)) {
|
||||
dir += string(filepath.Separator)
|
||||
|
||||
// filepathDescendsFrom() takes advantage of filepath.Rel() to determine if `p` is descended from `dir`:
|
||||
//
|
||||
// 1. filepath.Rel() calculates a path to traversal from fictious `dir` to `p`.
|
||||
// 2. filepath.Rel() errors in a handful of cases where absolute and relative paths are compared as well as certain traversal edge cases
|
||||
// For more information, https://github.com/golang/go/blob/00709919d09904b17cfe3bfeb35521cbd3fb04f8/src/path/filepath/path_test.go#L1510-L1515
|
||||
// 3. If the path to traverse `dir` to `p` requires `..`, then we know it is not descend from / contained in `dir`
|
||||
//
|
||||
// As-is, this function requires the caller to ensure `p` and `dir` are either 1) both relative or 2) both absolute.
|
||||
relativePath, err := filepath.Rel(dir, p)
|
||||
if err != nil {
|
||||
return false
|
||||
}
|
||||
return strings.HasPrefix(p, dir)
|
||||
return !strings.HasPrefix(relativePath, "..")
|
||||
}
|
||||
|
|
|
|||
|
|
@ -130,6 +130,86 @@ func Test_filepathDescendsFrom(t *testing.T) {
|
|||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "deny parent directory filename (`..`) escaping absolute directory",
|
||||
args: args{
|
||||
p: filepath.FromSlash(".."),
|
||||
dir: filepath.FromSlash("/var/logs/"),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "deny parent directory filename (`..`) escaping current directory",
|
||||
args: args{
|
||||
p: filepath.FromSlash(".."),
|
||||
dir: filepath.FromSlash("."),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "deny parent directory filename (`..`) escaping parent directory",
|
||||
args: args{
|
||||
p: filepath.FromSlash(".."),
|
||||
dir: filepath.FromSlash(".."),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "deny parent directory filename (`..`) escaping relative directory",
|
||||
args: args{
|
||||
p: filepath.FromSlash(".."),
|
||||
dir: filepath.FromSlash("relative-dir"),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "deny current directory filename (`.`) in absolute directory",
|
||||
args: args{
|
||||
p: filepath.FromSlash("."),
|
||||
dir: filepath.FromSlash("/var/logs/"),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "deny current directory filename (`.`) in current directory",
|
||||
args: args{
|
||||
p: filepath.FromSlash("."),
|
||||
dir: filepath.FromSlash("."),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "deny current directory filename (`.`) in parent directory",
|
||||
args: args{
|
||||
p: filepath.FromSlash("."),
|
||||
dir: filepath.FromSlash(".."),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "deny current directory filename (`.`) in relative directory",
|
||||
args: args{
|
||||
p: filepath.FromSlash("."),
|
||||
dir: filepath.FromSlash("relative-dir"),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "relative path, absolute dir",
|
||||
args: args{
|
||||
p: filepath.FromSlash("whatever"),
|
||||
dir: filepath.FromSlash("/a/b/c"),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
{
|
||||
name: "absolute path, relative dir",
|
||||
args: args{
|
||||
p: filepath.FromSlash("/a/b/c"),
|
||||
dir: filepath.FromSlash("whatever"),
|
||||
},
|
||||
want: false,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue