STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Expand logic and tests to handle edge cases

Browse source 
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.
This commit is contained in:
Andy Feller 2024-11-22 15:26:11 -05:00
parent e7c5706336
commit cdfc12caf5
4 changed files with 297 additions and 4 deletions
Download patch file Download diff file Expand all files Collapse all files

11
pkg/cmd/run/download/download.go
View file

@ -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)
}

189
pkg/cmd/run/download/download_test.go
View file

@ -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{

21
pkg/cmd/run/download/zip.go
View file

@ -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, "..")
}

80
pkg/cmd/run/download/zip_test.go
View file

@ -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) {