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

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

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:
+		case err != nil:
-			fmt.Printf("Can't use %q as it has invalid characters in it.\n", name)
+			fmt.Printf("Can't use %q as %v.\n", name, err)
 		default:
 			return name
 		}

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 != "" {

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
+const (
-var Matcher = regexp.MustCompile(`^(:?[\w_ -]+):(.*)$`)
+	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) {
+func Split(remote string) (parent string, leaf string, err error) {
-	remoteName, remotePath := Parse(remote)
+	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

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
 	}{
-		{"", "", ""},
+		{"", "", "", nil},
-		{"/path/to/file", "", "/path/to/file"},
+		{":", "", "", errInvalidCharacters},
-		{"path/to/file", "", "path/to/file"},
+		{"::", ":", "", errInvalidCharacters},
-		{"remote:path/to/file", "remote", "path/to/file"},
+		{":/:", "", "/:", errInvalidCharacters},
-		{"remote:/path/to/file", "remote", "/path/to/file"},
+		{"/:", "", "/:", nil},
-		{":backend:/path/to/file", ":backend", "/path/to/file"},
+		{"\\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:", "remote:", "", nil},
-		{"remote:potato", "remote:", "potato"},
+		{"remote:potato", "remote:", "potato", nil},
-		{"remote:/", "remote:/", ""},
+		{"remote:/", "remote:/", "", nil},
-		{"remote:/potato", "remote:/", "potato"},
+		{"remote:/potato", "remote:/", "potato", nil},
-		{"remote:/potato/potato", "remote:/potato/", "potato"},
+		{"remote:/potato/potato", "remote:/potato/", "potato", nil},
-		{"remote:potato/sausage", "remote:potato/", "sausage"},
+		{"remote:potato/sausage", "remote:potato/", "sausage", nil},
+		{"rem.ote:potato/sausage", "", "", errInvalidCharacters},
 
-		{":remote:", ":remote:", ""},
+		{":remote:", ":remote:", "", nil},
-		{":remote:potato", ":remote:", "potato"},
+		{":remote:potato", ":remote:", "potato", nil},
-		{":remote:/", ":remote:/", ""},
+		{":remote:/", ":remote:/", "", nil},
-		{":remote:/potato", ":remote:/", "potato"},
+		{":remote:/potato", ":remote:/", "potato", nil},
-		{":remote:/potato/potato", ":remote:/potato/", "potato"},
+		{":remote:/potato/potato", ":remote:/potato/", "potato", nil},
-		{":remote:potato/sausage", ":remote:potato/", "sausage"},
+		{":remote:potato/sausage", ":remote:potato/", "sausage", nil},
+		{":rem[ote:potato/sausage", "", "", errInvalidCharacters},
 
-		{"/", "/", ""},
+		{"/", "/", "", nil},
-		{"/root", "/", "root"},
+		{"/root", "/", "root", nil},
-		{"/a/b", "/a/", "b"},
+		{"/a/b", "/a/", "b", nil},
-		{"root", "", "root"},
+		{"root", "", "root", nil},
-		{"a/b", "a/", "b"},
+		{"a/b", "a/", "b", nil},
-		{"root/", "root/", ""},
+		{"root/", "root/", "", nil},
-		{"a/b/", "a/b/", ""},
+		{"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) {

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 {