From fa0d82a4f420258c7886a284a23cada5b4aeff4d Mon Sep 17 00:00:00 2001 From: MalinAhlberg Date: Thu, 16 Nov 2023 15:21:58 +0100 Subject: [PATCH 1/8] feat: validate filepaths characters --- upload/upload.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/upload/upload.go b/upload/upload.go index 6ad249a6..892f11f5 100644 --- a/upload/upload.go +++ b/upload/upload.go @@ -7,6 +7,7 @@ import ( "os" "path" "path/filepath" + "regexp" "strings" "github.com/NBISweden/sda-cli/encrypt" @@ -71,6 +72,15 @@ func uploadFiles(files, outFiles []string, targetDir string, config *helpers.Con return errors.New("no files to upload") } + // Loop through the list of file paths and check if their names are valid + for _, filename := range outFiles { + re := regexp.MustCompile(`[\\:\*\?"<>\|\x00-\x1F\x7F]`) + dissallowedChars := re.FindAllString(filename, -1) + if dissallowedChars != nil { + return fmt.Errorf("filepath %v contains disallowed characters: %+v", filename, strings.Join(dissallowedChars, ", ")) + } + } + // Loop through the list of files and check if they are encrypted // If we run into an unencrypted file and the flag force-unencrypted is not set, we stop the upload for _, filename := range files { From 3682c854266d2ae4470c9bfddd4e9e13049a90b3 Mon Sep 17 00:00:00 2001 From: MalinAhlberg Date: Thu, 16 Nov 2023 15:22:31 +0100 Subject: [PATCH 2/8] fix: remove old character replacments in filepaths --- upload/upload.go | 18 ++---------------- upload/upload_test.go | 15 --------------- 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/upload/upload.go b/upload/upload.go index 892f11f5..02b4f471 100644 --- a/upload/upload.go +++ b/upload/upload.go @@ -235,7 +235,7 @@ func createFilePaths(dirPath string) ([]string, []string, error) { // Remove possible trailing "/" so that "path" and "path/" behave the same dirPath = strings.TrimSuffix(dirPath, string(os.PathSeparator)) pathToTrim := strings.TrimSuffix(dirPath, filepath.Base(dirPath)) - outPath := formatUploadFilePath(strings.TrimPrefix(path, pathToTrim)) + outPath := filepath.ToSlash(strings.TrimPrefix(path, pathToTrim)) outFiles = append(outFiles, outPath) } @@ -249,20 +249,6 @@ func createFilePaths(dirPath string) ([]string, []string, error) { return files, outFiles, nil } -// formatUploadFilePath ensures that path separators are "/", and that special -// characters are replaced with safe characters. -func formatUploadFilePath(filePath string) string { - - outPath := filepath.ToSlash(filePath) - - for _, char := range []string{":", ";"} { - outPath = strings.ReplaceAll(outPath, char, "_") - } - log.Debugf("Converted filepath %v to %v", filePath, outPath) - - return outPath -} - // Upload function uploads files to the s3 bucket. Input can be files or // directories to be uploaded recursively func Upload(args []string) error { @@ -338,7 +324,7 @@ func Upload(args []string) error { outFiles = append(outFiles, upFilePaths...) } else { files = append(files, filePath) - outFiles = append(outFiles, formatUploadFilePath(filepath.Base(filePath))) + outFiles = append(outFiles, filepath.ToSlash(filepath.Base(filePath))) } } diff --git a/upload/upload_test.go b/upload/upload_test.go index 7df3ee61..7f7350d1 100644 --- a/upload/upload_test.go +++ b/upload/upload_test.go @@ -135,21 +135,6 @@ func (suite *TestSuite) TestcreateFilePaths() { assert.ErrorContains(suite.T(), err, msg) } -func (suite *TestSuite) TestFormatUploadFilePath() { - - unixPath := "a/b/c.c4gh" - testPath := filepath.Join("a", "b", "c.c4gh") - uploadPath := formatUploadFilePath(testPath) - - assert.Equal(suite.T(), unixPath, uploadPath) - - weirdPath := "a/b:/c;.c4gh" - goodPath := "a/b_/c_.c4gh" - formattedPath := formatUploadFilePath(weirdPath) - - assert.Equal(suite.T(), goodPath, formattedPath) -} - func (suite *TestSuite) TestFunctionality() { // Create a fake s3 backend From af8cb2351ff27696c12677e9e62eef7034f26d7a Mon Sep 17 00:00:00 2001 From: MalinAhlberg Date: Thu, 23 Nov 2023 13:42:05 +0100 Subject: [PATCH 3/8] add test for invalid chars --- upload/upload_test.go | 71 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/upload/upload_test.go b/upload/upload_test.go index 7f7350d1..5bed4303 100644 --- a/upload/upload_test.go +++ b/upload/upload_test.go @@ -423,3 +423,74 @@ func (suite *TestSuite) TestRecursiveToDifferentTarget() { log.SetOutput(os.Stdout) } + +func (suite *TestSuite) TestUploadInvalidCharacters() { + // Filenames with :\?* can not be created on windows + if runtime.GOOS == "windows" { + return + } + + // Create a fake s3 backend + backend := s3mem.New() + faker := gofakes3.New(backend) + ts := httptest.NewServer(faker.Server()) + defer ts.Close() + + // Create conf file for sda-cli + var confFile = fmt.Sprintf(` + access_token = "eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxNzA3NDgzOTQ0IiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWUsImlhdCI6MTUxNjIzOTAyMiwiZXhwIjoxNzA3NDgzOTQ0fQ.D7hrpd3ROXp53NnXa0PL9js2Oi1KqpKpkVMic1B23X84ksX9kbbtn4Ad4BkhO8Tm35a5hBu95CGgw5b06sd3LQ" + host_base = %[1]s + encoding = UTF-8 + host_bucket = %[1]s + multipart_chunk_size_mb = 50 + secret_key = dummy + access_key = dummy + use_https = False + check_ssl_certificate = False + check_ssl_hostname = False + socket_timeout = 30 + human_readable_sizes = True + guess_mime_type = True + encrypt = False + `, strings.TrimPrefix(ts.URL, "http://")) + + configPath, err := os.CreateTemp(os.TempDir(), "s3cmd.conf") + if err != nil { + log.Panic(err) + } + defer os.Remove(configPath.Name()) + + err = os.WriteFile(configPath.Name(), []byte(confFile), 0600) + if err != nil { + log.Printf("failed to write temp config file, %v", err) + } + + // Create temp dir with file + dir, err := os.MkdirTemp(os.TempDir(), "test") + if err != nil { + log.Panic(err) + } + defer os.RemoveAll(dir) + + // Test that no files with invalid characters can be uploaded + for _, badc := range "\\:*?" { + badchar := string(badc) + testfilepath := "test" + badchar + "file" + var testfile *os.File + testfile, err := os.Create(filepath.Join(dir, testfilepath)) + if err != nil { + log.Panic(err) + } + err = os.WriteFile(testfile.Name(), []byte("content"), 0600) + if err != nil { + log.Printf("failed to write temp file, %v", err) + } + defer os.Remove(testfile.Name()) + + os.Args = []string{"upload", "--force-unencrypted", "-config", configPath.Name(), "-r", testfile.Name()} + err = Upload(os.Args) + assert.Error(suite.T(), err) + assert.Equal(suite.T(), fmt.Sprintf("filepath %v contains disallowed characters: %+v", testfilepath, badchar), err.Error()) + } + +} From 78d0d111ddd8b751f061b4fa9a5af7cac3a288c0 Mon Sep 17 00:00:00 2001 From: MalinAhlberg Date: Mon, 27 Nov 2023 15:38:59 +0100 Subject: [PATCH 4/8] refactor: move invalid chars check to helpers --- helpers/helpers.go | 12 ++++++++++++ helpers/helpers_test.go | 12 ++++++++++++ upload/upload.go | 8 +++----- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/helpers/helpers.go b/helpers/helpers.go index dc74867d..25d3a623 100644 --- a/helpers/helpers.go +++ b/helpers/helpers.go @@ -9,6 +9,7 @@ import ( "io" "os" "path/filepath" + "regexp" "strings" "sync" "time" @@ -396,3 +397,14 @@ func ListFiles(config Config, prefix string) (result *s3.ListObjectsV2Output, er return result, nil } + +// Check for invalid characters +func CheckValidChars(filename string) error { + re := regexp.MustCompile(`[\\:\*\?"<>\|\x00-\x1F\x7F]`) + dissallowedChars := re.FindAllString(filename, -1) + if dissallowedChars != nil { + return fmt.Errorf("filepath %v contains disallowed characters: %+v", filename, strings.Join(dissallowedChars, ", ")) + } + + return nil +} diff --git a/helpers/helpers_test.go b/helpers/helpers_test.go index 5d025802..455c9df1 100644 --- a/helpers/helpers_test.go +++ b/helpers/helpers_test.go @@ -377,3 +377,15 @@ public_key = 27be42445fd9e39c9be39e6b36a55e61e3801fc845f63781a813d3fe9977e17a os.Remove("key-from-oidc.pub.pem") } } + +func (suite *HelperTests) TestInvalidCharacters() { + // Test that file paths with invalid characters trigger errors + for _, badc := range "\x00\x7F\x1A:*?\\" { + badchar := string(badc) + testfilepath := "test" + badchar + "file" + + err := CheckValidChars(testfilepath) + assert.Error(suite.T(), err) + assert.Equal(suite.T(), fmt.Sprintf("filepath %v contains disallowed characters: %+v", testfilepath, badchar), err.Error()) + } +} diff --git a/upload/upload.go b/upload/upload.go index 02b4f471..60e968d9 100644 --- a/upload/upload.go +++ b/upload/upload.go @@ -7,7 +7,6 @@ import ( "os" "path" "path/filepath" - "regexp" "strings" "github.com/NBISweden/sda-cli/encrypt" @@ -74,10 +73,9 @@ func uploadFiles(files, outFiles []string, targetDir string, config *helpers.Con // Loop through the list of file paths and check if their names are valid for _, filename := range outFiles { - re := regexp.MustCompile(`[\\:\*\?"<>\|\x00-\x1F\x7F]`) - dissallowedChars := re.FindAllString(filename, -1) - if dissallowedChars != nil { - return fmt.Errorf("filepath %v contains disallowed characters: %+v", filename, strings.Join(dissallowedChars, ", ")) + err := helpers.CheckValidChars(filename) + if err != nil { + return err } } From 260f1d8cd075cdf3ce1b097a041048d2b9b2897d Mon Sep 17 00:00:00 2001 From: MalinAhlberg Date: Tue, 28 Nov 2023 16:01:48 +0100 Subject: [PATCH 5/8] refactor: use Skip() for skipped tests --- upload/upload_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/upload/upload_test.go b/upload/upload_test.go index 5bed4303..a176db00 100644 --- a/upload/upload_test.go +++ b/upload/upload_test.go @@ -427,7 +427,7 @@ func (suite *TestSuite) TestRecursiveToDifferentTarget() { func (suite *TestSuite) TestUploadInvalidCharacters() { // Filenames with :\?* can not be created on windows if runtime.GOOS == "windows" { - return + suite.T().Skip("Skipping. Cannot create filenames with invalid characters on windows") } // Create a fake s3 backend From d75b50ccaf8a4110b7a9abafb48ed54875eb648b Mon Sep 17 00:00:00 2001 From: MalinAhlberg Date: Thu, 30 Nov 2023 10:02:47 +0100 Subject: [PATCH 6/8] refactor: panic if test file creation fails --- upload/upload_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/upload/upload_test.go b/upload/upload_test.go index a176db00..a12cfac6 100644 --- a/upload/upload_test.go +++ b/upload/upload_test.go @@ -462,7 +462,7 @@ func (suite *TestSuite) TestUploadInvalidCharacters() { err = os.WriteFile(configPath.Name(), []byte(confFile), 0600) if err != nil { - log.Printf("failed to write temp config file, %v", err) + log.Panic(err) } // Create temp dir with file @@ -483,7 +483,7 @@ func (suite *TestSuite) TestUploadInvalidCharacters() { } err = os.WriteFile(testfile.Name(), []byte("content"), 0600) if err != nil { - log.Printf("failed to write temp file, %v", err) + log.Panic(err) } defer os.Remove(testfile.Name()) From c7b49869bbd098f497522e527eea45a2d7e11cc9 Mon Sep 17 00:00:00 2001 From: MalinAhlberg Date: Thu, 30 Nov 2023 10:16:41 +0100 Subject: [PATCH 7/8] add checks to target dir --- upload/upload.go | 11 ++++++++--- upload/upload_test.go | 37 +++++++++++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/upload/upload.go b/upload/upload.go index 60e968d9..9fa42d50 100644 --- a/upload/upload.go +++ b/upload/upload.go @@ -261,15 +261,20 @@ func Upload(args []string) error { return fmt.Errorf("failed parsing arguments, reason: %v", err) } - // Check that specified target directory is valid, i.e. not a filepath or a flag - info, err := os.Stat(*targetDir) - // Dereference the pointer to a string var targetDirString string if targetDir != nil { targetDirString = *targetDir } + err = helpers.CheckValidChars(filepath.ToSlash(targetDirString)) + if err != nil { + return err + } + + // Check that specified target directory is valid, i.e. not a filepath or a flag + info, err := os.Stat(*targetDir) + if (!os.IsNotExist(err) && !info.IsDir()) || (targetDirString != "" && targetDirString[0:1] == "-") { return errors.New(*targetDir + " is not a valid target directory") } diff --git a/upload/upload_test.go b/upload/upload_test.go index a12cfac6..ff446b5a 100644 --- a/upload/upload_test.go +++ b/upload/upload_test.go @@ -425,10 +425,6 @@ func (suite *TestSuite) TestRecursiveToDifferentTarget() { } func (suite *TestSuite) TestUploadInvalidCharacters() { - // Filenames with :\?* can not be created on windows - if runtime.GOOS == "windows" { - suite.T().Skip("Skipping. Cannot create filenames with invalid characters on windows") - } // Create a fake s3 backend backend := s3mem.New() @@ -472,6 +468,39 @@ func (suite *TestSuite) TestUploadInvalidCharacters() { } defer os.RemoveAll(dir) + // Create a test file + testfilepath := "testfile" + var testfile *os.File + testfile, err = os.Create(filepath.Join(dir, testfilepath)) + if err != nil { + log.Panic(err) + } + err = os.WriteFile(testfile.Name(), []byte("content"), 0600) + if err != nil { + log.Panic(err) + } + defer os.Remove(testfile.Name()) + + // Check that target dir names with invalid characters will not be accepted + badchars := ":*?" + // backlash is only allowed on windows + if runtime.GOOS != "windows" { + badchars += "\\" + } + for _, badc := range badchars { + badchar := string(badc) + targetDir := "test" + badchar + "dir" + os.Args = []string{"upload", "--force-unencrypted", "-config", configPath.Name(), "-targetDir", targetDir, "-r", testfile.Name()} + err = Upload(os.Args) + assert.Error(suite.T(), err) + assert.Equal(suite.T(), fmt.Sprintf("filepath %v contains disallowed characters: %+v", targetDir, badchar), err.Error()) + } + + // Filenames with :\?* can not be created on windows, skip the following tests + if runtime.GOOS == "windows" { + suite.T().Skip("Skipping. Cannot create filenames with invalid characters on windows") + } + // Test that no files with invalid characters can be uploaded for _, badc := range "\\:*?" { badchar := string(badc) From e6b6639b5934bfc0fc88c3a7562f94d076efc4ee Mon Sep 17 00:00:00 2001 From: MalinAhlberg Date: Thu, 30 Nov 2023 11:44:33 +0100 Subject: [PATCH 8/8] refactor: update error message for invalid target dir names --- upload/upload.go | 2 +- upload/upload_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/upload/upload.go b/upload/upload.go index 9fa42d50..20239cc1 100644 --- a/upload/upload.go +++ b/upload/upload.go @@ -269,7 +269,7 @@ func Upload(args []string) error { err = helpers.CheckValidChars(filepath.ToSlash(targetDirString)) if err != nil { - return err + return errors.New(*targetDir + " is not a valid target directory") } // Check that specified target directory is valid, i.e. not a filepath or a flag diff --git a/upload/upload_test.go b/upload/upload_test.go index ff446b5a..7f47d9ac 100644 --- a/upload/upload_test.go +++ b/upload/upload_test.go @@ -493,7 +493,7 @@ func (suite *TestSuite) TestUploadInvalidCharacters() { os.Args = []string{"upload", "--force-unencrypted", "-config", configPath.Name(), "-targetDir", targetDir, "-r", testfile.Name()} err = Upload(os.Args) assert.Error(suite.T(), err) - assert.Equal(suite.T(), fmt.Sprintf("filepath %v contains disallowed characters: %+v", targetDir, badchar), err.Error()) + assert.Equal(suite.T(), targetDir+" is not a valid target directory", err.Error()) } // Filenames with :\?* can not be created on windows, skip the following tests