From cdfc12caf52754ea4026d5338a56ad4e6f822105 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 22 Nov 2024 15:26:11 -0500 Subject: [PATCH] Expand logic and tests to handle edge cases This commit expands filepathDescendsFrom(string, string) to handle edge cases such as mixing absolute and relative paths or artifact name edge cases. Additionally, tests for filepathDescendsFrom() and downloadrun() have been expanded to verify additional use cases. --- pkg/cmd/run/download/download.go | 11 +- pkg/cmd/run/download/download_test.go | 189 +++++++++++++++++++++++++- pkg/cmd/run/download/zip.go | 21 ++- pkg/cmd/run/download/zip_test.go | 80 +++++++++++ 4 files changed, 297 insertions(+), 4 deletions(-) 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) {