From f1347139fa7e7866356cbec685e61ee74a12944c Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 5 Sep 2019 11:01:04 +0100 Subject: [PATCH] config: check config names more carefully and report errors - fixes #3506 Before this change it was possible to make a remote with an invalid name in the config file, either manually or with `rclone config create` (but not with `rclone config`). When this remote was used, because it was invalid, rclone would presume this remote name was a local directory for a very suprising user experience! This change checks remote names more carefully and returns errors - when the user tries to use an invalid remote name on the command line - when an invalid remote name is used in `rclone config create/update/password` - when the user tries to enter an invalid remote name in `rclone config` This does not prevent the user entering a remote name with invalid characters in the config manually, but such a remote will fail immediately when it is used on the command line. --- cmd/cmd.go | 11 +++- fs/config/config.go | 18 +++++- fs/fs.go | 5 +- fs/fspath/path.go | 58 +++++++++++++++--- fs/fspath/path_test.go | 126 +++++++++++++++++++++++++++++--------- fstest/fstests/fstests.go | 3 +- 6 files changed, 177 insertions(+), 44 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index c5d2d388b..1e772f0e2 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -174,7 +174,11 @@ func NewFsSrcDstFiles(args []string) (fsrc fs.Fs, srcFileName string, fdst fs.Fs // If file exists then srcFileName != "", however if the file // doesn't exist then we assume it is a directory... if srcFileName != "" { - dstRemote, dstFileName = fspath.Split(dstRemote) + var err error + dstRemote, dstFileName, err = fspath.Split(dstRemote) + if err != nil { + log.Fatalf("Parsing %q failed: %v", args[1], err) + } if dstRemote == "" { dstRemote = "." } @@ -197,7 +201,10 @@ func NewFsSrcDstFiles(args []string) (fsrc fs.Fs, srcFileName string, fdst fs.Fs // NewFsDstFile creates a new dst fs with a destination file name from the arguments func NewFsDstFile(args []string) (fdst fs.Fs, dstFileName string) { - dstRemote, dstFileName := fspath.Split(args[0]) + dstRemote, dstFileName, err := fspath.Split(args[0]) + if err != nil { + log.Fatalf("Parsing %q failed: %v", args[0], err) + } if dstRemote == "" { dstRemote = "." } diff --git a/fs/config/config.go b/fs/config/config.go index 676a9143f..bb636b0e4 100644 --- a/fs/config/config.go +++ b/fs/config/config.go @@ -944,6 +944,10 @@ func suppressConfirm() func() { // UpdateRemote adds the keyValues passed in to the remote of name. // keyValues should be key, value pairs. func UpdateRemote(name string, keyValues rc.Params) error { + err := fspath.CheckConfigName(name) + if err != nil { + return err + } defer suppressConfirm()() // Work out which options need to be obscured @@ -987,6 +991,10 @@ func UpdateRemote(name string, keyValues rc.Params) error { // parameters which are key, value pairs. If update is set then it // adds the new keys rather than replacing all of them. func CreateRemote(name string, provider string, keyValues rc.Params) error { + err := fspath.CheckConfigName(name) + if err != nil { + return err + } // Delete the old config if it exists getConfigData().DeleteSection(name) // Set the type @@ -998,6 +1006,10 @@ func CreateRemote(name string, provider string, keyValues rc.Params) error { // PasswordRemote adds the keyValues passed in to the remote of name. // keyValues should be key, value pairs. func PasswordRemote(name string, keyValues rc.Params) error { + err := fspath.CheckConfigName(name) + if err != nil { + return err + } defer suppressConfirm()() for k, v := range keyValues { keyValues[k] = obscure.MustObscure(fmt.Sprint(v)) @@ -1041,14 +1053,14 @@ func NewRemoteName() (name string) { for { fmt.Printf("name> ") name = ReadLine() - parts := fspath.Matcher.FindStringSubmatch(name + ":") + err := fspath.CheckConfigName(name) switch { case name == "": fmt.Printf("Can't use empty name.\n") case driveletter.IsDriveLetter(name): fmt.Printf("Can't use %q as it can be confused with a drive letter.\n", name) - case parts == nil: - fmt.Printf("Can't use %q as it has invalid characters in it.\n", name) + case err != nil: + fmt.Printf("Can't use %q as %v.\n", name, err) default: return name } diff --git a/fs/fs.go b/fs/fs.go index 1138f33cf..22c295690 100644 --- a/fs/fs.go +++ b/fs/fs.go @@ -1087,7 +1087,10 @@ func MustFind(name string) *RegInfo { // ParseRemote deconstructs a path into configName, fsPath, looking up // the fsName in the config file (returning NotFoundInConfigFile if not found) func ParseRemote(path string) (fsInfo *RegInfo, configName, fsPath string, err error) { - configName, fsPath = fspath.Parse(path) + configName, fsPath, err = fspath.Parse(path) + if err != nil { + return nil, "", "", err + } var fsName string var ok bool if configName != "" { diff --git a/fs/fspath/path.go b/fs/fspath/path.go index fa992cf04..d0684f941 100644 --- a/fs/fspath/path.go +++ b/fs/fspath/path.go @@ -2,6 +2,7 @@ package fspath import ( + "errors" "path" "path/filepath" "regexp" @@ -10,8 +11,40 @@ import ( "github.com/rclone/rclone/fs/driveletter" ) -// Matcher is a pattern to match an rclone URL -var Matcher = regexp.MustCompile(`^(:?[\w_ -]+):(.*)$`) +const ( + configNameRe = `[\w_ -]+` + remoteNameRe = `^(:?` + configNameRe + `):` +) + +var ( + errInvalidCharacters = errors.New("config name contains invalid characters - may only contain 0-9, A-Z ,a-z ,_ , - and space ") + + // urlMatcher is a pattern to match an rclone URL + // note that this matches invalid remoteNames + urlMatcher = regexp.MustCompile(`^(:?[^\\/:]*):(.*)$`) + + // configNameMatcher is a pattern to match an rclone config name + configNameMatcher = regexp.MustCompile(`^` + configNameRe + `$`) + + // remoteNameMatcher is a pattern to match an rclone remote name + remoteNameMatcher = regexp.MustCompile(remoteNameRe + `$`) +) + +// CheckConfigName returns an error if configName is invalid +func CheckConfigName(configName string) error { + if !configNameMatcher.MatchString(configName) { + return errInvalidCharacters + } + return nil +} + +// CheckRemoteName returns an error if remoteName is invalid +func CheckRemoteName(remoteName string) error { + if !remoteNameMatcher.MatchString(remoteName) { + return errInvalidCharacters + } + return nil +} // Parse deconstructs a remote path into configName and fsPath // @@ -21,15 +54,21 @@ var Matcher = regexp.MustCompile(`^(:?[\w_ -]+):(.*)$`) // and "/path/to/local" will return ("", "/path/to/local") // // Note that this will turn \ into / in the fsPath on Windows -func Parse(path string) (configName, fsPath string) { - parts := Matcher.FindStringSubmatch(path) +// +// An error may be returned if the remote name has invalid characters in it. +func Parse(path string) (configName, fsPath string, err error) { + parts := urlMatcher.FindStringSubmatch(path) configName, fsPath = "", path if parts != nil && !driveletter.IsDriveLetter(parts[1]) { configName, fsPath = parts[1], parts[2] + err = CheckRemoteName(configName + ":") + if err != nil { + return configName, fsPath, errInvalidCharacters + } } // change native directory separators to / if there are any fsPath = filepath.ToSlash(fsPath) - return configName, fsPath + return configName, fsPath, nil } // Split splits a remote into a parent and a leaf @@ -40,14 +79,17 @@ func Parse(path string) (configName, fsPath string) { // // The returned values have the property that parent + leaf == remote // (except under Windows where \ will be translated into /) -func Split(remote string) (parent string, leaf string) { - remoteName, remotePath := Parse(remote) +func Split(remote string) (parent string, leaf string, err error) { + remoteName, remotePath, err := Parse(remote) + if err != nil { + return "", "", err + } if remoteName != "" { remoteName += ":" } // Construct new remote name without last segment parent, leaf = path.Split(remotePath) - return remoteName + parent, leaf + return remoteName + parent, leaf, nil } // JoinRootPath joins any number of path elements into a single path, adding a diff --git a/fs/fspath/path_test.go b/fs/fspath/path_test.go index f900d62be..5b85df215 100644 --- a/fs/fspath/path_test.go +++ b/fs/fspath/path_test.go @@ -2,23 +2,85 @@ package fspath import ( "fmt" + "runtime" + "strings" "testing" "github.com/stretchr/testify/assert" ) +func TestCheckConfigName(t *testing.T) { + for _, test := range []struct { + in string + want error + }{ + {"remote", nil}, + {"", errInvalidCharacters}, + {":remote:", errInvalidCharacters}, + {"remote:", errInvalidCharacters}, + {"rem:ote", errInvalidCharacters}, + {"rem/ote", errInvalidCharacters}, + {"rem\\ote", errInvalidCharacters}, + {"[remote", errInvalidCharacters}, + {"*", errInvalidCharacters}, + } { + got := CheckConfigName(test.in) + assert.Equal(t, test.want, got, test.in) + } +} + +func TestCheckRemoteName(t *testing.T) { + for _, test := range []struct { + in string + want error + }{ + {":remote:", nil}, + {"remote:", nil}, + {"", errInvalidCharacters}, + {"rem:ote", errInvalidCharacters}, + {"rem:ote:", errInvalidCharacters}, + {"remote", errInvalidCharacters}, + {"rem/ote:", errInvalidCharacters}, + {"rem\\ote:", errInvalidCharacters}, + {"[remote:", errInvalidCharacters}, + {"*:", errInvalidCharacters}, + } { + got := CheckRemoteName(test.in) + assert.Equal(t, test.want, got, test.in) + } +} + func TestParse(t *testing.T) { for _, test := range []struct { in, wantConfigName, wantFsPath string + wantErr error }{ - {"", "", ""}, - {"/path/to/file", "", "/path/to/file"}, - {"path/to/file", "", "path/to/file"}, - {"remote:path/to/file", "remote", "path/to/file"}, - {"remote:/path/to/file", "remote", "/path/to/file"}, - {":backend:/path/to/file", ":backend", "/path/to/file"}, + {"", "", "", nil}, + {":", "", "", errInvalidCharacters}, + {"::", ":", "", errInvalidCharacters}, + {":/:", "", "/:", errInvalidCharacters}, + {"/:", "", "/:", nil}, + {"\\backslash:", "", "\\backslash:", nil}, + {"/slash:", "", "/slash:", nil}, + {"with\\backslash:", "", "with\\backslash:", nil}, + {"with/slash:", "", "with/slash:", nil}, + {"/path/to/file", "", "/path/to/file", nil}, + {"/path:/to/file", "", "/path:/to/file", nil}, + {"./path:/to/file", "", "./path:/to/file", nil}, + {"./:colon.txt", "", "./:colon.txt", nil}, + {"path/to/file", "", "path/to/file", nil}, + {"remote:path/to/file", "remote", "path/to/file", nil}, + {"rem*ote:path/to/file", "rem*ote", "path/to/file", errInvalidCharacters}, + {"remote:/path/to/file", "remote", "/path/to/file", nil}, + {"rem.ote:/path/to/file", "rem.ote", "/path/to/file", errInvalidCharacters}, + {":backend:/path/to/file", ":backend", "/path/to/file", nil}, + {":bac*kend:/path/to/file", ":bac*kend", "/path/to/file", errInvalidCharacters}, } { - gotConfigName, gotFsPath := Parse(test.in) + gotConfigName, gotFsPath, gotErr := Parse(test.in) + if runtime.GOOS == "windows" { + test.wantFsPath = strings.Replace(test.wantFsPath, `\`, `/`, -1) + } + assert.Equal(t, test.wantErr, gotErr) assert.Equal(t, test.wantConfigName, gotConfigName) assert.Equal(t, test.wantFsPath, gotFsPath) } @@ -27,35 +89,41 @@ func TestParse(t *testing.T) { func TestSplit(t *testing.T) { for _, test := range []struct { remote, wantParent, wantLeaf string + wantErr error }{ - {"", "", ""}, + {"", "", "", nil}, - {"remote:", "remote:", ""}, - {"remote:potato", "remote:", "potato"}, - {"remote:/", "remote:/", ""}, - {"remote:/potato", "remote:/", "potato"}, - {"remote:/potato/potato", "remote:/potato/", "potato"}, - {"remote:potato/sausage", "remote:potato/", "sausage"}, + {"remote:", "remote:", "", nil}, + {"remote:potato", "remote:", "potato", nil}, + {"remote:/", "remote:/", "", nil}, + {"remote:/potato", "remote:/", "potato", nil}, + {"remote:/potato/potato", "remote:/potato/", "potato", nil}, + {"remote:potato/sausage", "remote:potato/", "sausage", nil}, + {"rem.ote:potato/sausage", "", "", errInvalidCharacters}, - {":remote:", ":remote:", ""}, - {":remote:potato", ":remote:", "potato"}, - {":remote:/", ":remote:/", ""}, - {":remote:/potato", ":remote:/", "potato"}, - {":remote:/potato/potato", ":remote:/potato/", "potato"}, - {":remote:potato/sausage", ":remote:potato/", "sausage"}, + {":remote:", ":remote:", "", nil}, + {":remote:potato", ":remote:", "potato", nil}, + {":remote:/", ":remote:/", "", nil}, + {":remote:/potato", ":remote:/", "potato", nil}, + {":remote:/potato/potato", ":remote:/potato/", "potato", nil}, + {":remote:potato/sausage", ":remote:potato/", "sausage", nil}, + {":rem[ote:potato/sausage", "", "", errInvalidCharacters}, - {"/", "/", ""}, - {"/root", "/", "root"}, - {"/a/b", "/a/", "b"}, - {"root", "", "root"}, - {"a/b", "a/", "b"}, - {"root/", "root/", ""}, - {"a/b/", "a/b/", ""}, + {"/", "/", "", nil}, + {"/root", "/", "root", nil}, + {"/a/b", "/a/", "b", nil}, + {"root", "", "root", nil}, + {"a/b", "a/", "b", nil}, + {"root/", "root/", "", nil}, + {"a/b/", "a/b/", "", nil}, } { - gotParent, gotLeaf := Split(test.remote) + gotParent, gotLeaf, gotErr := Split(test.remote) + assert.Equal(t, test.wantErr, gotErr) assert.Equal(t, test.wantParent, gotParent, test.remote) assert.Equal(t, test.wantLeaf, gotLeaf, test.remote) - assert.Equal(t, test.remote, gotParent+gotLeaf, fmt.Sprintf("%s: %q + %q != %q", test.remote, gotParent, gotLeaf, test.remote)) + if gotErr == nil { + assert.Equal(t, test.remote, gotParent+gotLeaf, fmt.Sprintf("%s: %q + %q != %q", test.remote, gotParent, gotLeaf, test.remote)) + } } } func TestJoinRootPath(t *testing.T) { diff --git a/fstest/fstests/fstests.go b/fstest/fstests/fstests.go index 677a73ec0..7a1fc6c1e 100644 --- a/fstest/fstests/fstests.go +++ b/fstest/fstests/fstests.go @@ -1384,7 +1384,8 @@ func Run(t *testing.T, opt *Opt) { t.Skip("Can't list from root on this remote") } - configName, configLeaf := fspath.Parse(subRemoteName) + configName, configLeaf, err := fspath.Parse(subRemoteName) + require.NoError(t, err) if configName == "" { configName, configLeaf = path.Split(subRemoteName) } else {