Disallow invalid characters in filepaths (#287)

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
+}

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())
+	}
+}

upload/upload.go

@@ -71,6 +71,14 @@ 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 {
+		err := helpers.CheckValidChars(filename)
+		if err != nil {
+			return err
+		}
+	}
+
 	// 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 {
@@ -225,7 +233,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)
 		}
 
@@ -239,20 +247,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 {
@@ -267,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 errors.New(*targetDir + " is not a valid target directory")
+	}
+
+	// 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")
 	}
@@ -328,7 +327,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)))
 		}
 	}
 

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
@@ -438,3 +423,103 @@ func (suite *TestSuite) TestRecursiveToDifferentTarget() {
 
 	log.SetOutput(os.Stdout)
 }
+
+func (suite *TestSuite) TestUploadInvalidCharacters() {
+
+	// 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.Panic(err)
+	}
+
+	// Create temp dir with file
+	dir, err := os.MkdirTemp(os.TempDir(), "test")
+	if err != nil {
+		log.Panic(err)
+	}
+	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(), targetDir+" is not a valid target directory", 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)
+		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.Panic(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())
+	}
+
+}