diff --git a/pkg/cmd/run/download/download.go b/pkg/cmd/run/download/download.go index 5bda2ba3d..04ce74340 100644 --- a/pkg/cmd/run/download/download.go +++ b/pkg/cmd/run/download/download.go @@ -166,8 +166,15 @@ func runDownload(opts *DownloadOptions) error { } } destDir := opts.DestinationDir - // Why do we only include the artifact name in the destination directory if there are multiple? - if len(wantPatterns) != 0 || len(wantNames) != 1 { + + // Isolate the downloaded artifact file to avoid potential conflicts from other downloaded artifacts when: + // + // 1. len(wantPatterns) > 0: Any pattern can result in 2+ artifacts + // 2. len(wantNames) == 0: User wants all artifacts regardless what they are named + // 3. len(wantNames) > 1: User wants multiple, specific artifacts + // + // Otherwise if a single artifact is wanted, then the protective subdirectory is an unnecessary inconvenience. + if len(wantPatterns) > 0 || len(wantNames) != 1 { destDir = filepath.Join(destDir, a.Name) } diff --git a/pkg/cmd/run/download/download_test.go b/pkg/cmd/run/download/download_test.go index 0df94ccf4..aeab20278 100644 --- a/pkg/cmd/run/download/download_test.go +++ b/pkg/cmd/run/download/download_test.go @@ -207,7 +207,7 @@ func Test_runDownload(t *testing.T) { wantErr string }{ { - name: "download non-expired", + name: "download non-expired to relative directory", opts: DownloadOptions{ RunID: "2345", DestinationDir: "./tmp", @@ -253,6 +253,53 @@ func Test_runDownload(t *testing.T) { filepath.Join("artifact-2", "artifact-2-file"), }, }, + { + name: "download non-expired to absolute directory", + opts: DownloadOptions{ + RunID: "2345", + DestinationDir: "/tmp", + }, + 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", + }, + }, + }, + }, + }, + expectedFiles: []string{ + filepath.Join("artifact-1", "artifact-1-file"), + filepath.Join("artifact-2", "artifact-2-file"), + }, + }, { name: "all artifacts are expired", opts: DownloadOptions{ @@ -322,6 +369,53 @@ func Test_runDownload(t *testing.T) { 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"), + }, + }, { name: "no pattern matches", opts: DownloadOptions{ @@ -357,6 +451,99 @@ func Test_runDownload(t *testing.T) { 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{ diff --git a/pkg/cmd/run/download/zip.go b/pkg/cmd/run/download/zip.go index 52994199a..a68b75fd6 100644 --- a/pkg/cmd/run/download/zip.go +++ b/pkg/cmd/run/download/zip.go @@ -71,6 +71,25 @@ func getPerm(m os.FileMode) os.FileMode { } func filepathDescendsFrom(p, dir string) bool { - relativePath, _ := filepath.Rel(dir, p) + // 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) + + if p == "." || p == ".." { + return false + } + + // 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(relativePath, "..") } diff --git a/pkg/cmd/run/download/zip_test.go b/pkg/cmd/run/download/zip_test.go index ca401cdb9..b85122ec5 100644 --- a/pkg/cmd/run/download/zip_test.go +++ b/pkg/cmd/run/download/zip_test.go @@ -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) {