Refuse credentials in a storage URL's endpoint or rclone remote name (closes #322)

storage_url refused only the outer URL's userinfo and unknown query
parameters. Userinfo inside the s3 endpoint= value, and rclone's
remote,param=value parameters in the remote name, were accepted and
then printed by info, remote info and snapshot create. Both are now
refused without echoing the value: a host name never contains "@", and
an rclone remote name never contains a comma. An s3 query that cannot
be decoded (a ";" or a bad "%" escape) is refused too, because decoding
drops that part, endpoint included, before it can be checked. The comma
is checked before url.Parse, whose error for such a name prints part of
it. The rclone case has its own error pointing at rclone config.

Model: opus-5-5

README.md

@@ -271,8 +271,17 @@ existing file. Created with mode `0600` since it will contain credentials.
 **`config set`**: Set a scalar config value by dotted YAML path
 (e.g. `vaultik config set compression_level 9`,
 `vaultik config set storage_url "file:///mnt/backups"`). Comments and
-formatting in the file are preserved; intermediate maps are created as
-needed.
+formatting in the file are preserved. Missing keys along the path are
+created: as a list when the next path element is a number (so
+`vaultik config set age_recipients.0 age1...` works on a file without
+`age_recipients`), otherwise as a map. If the config loads, a change
+after which it would not, such as `vaultik config set compression_level 99`,
+is refused and the file is left unchanged. A config that does not load,
+such as the one `config init` writes before `storage_url` is set or one
+with a `${ENV:...}` reference to a variable not set in this shell, is
+checked one setting at a time: only a change after which the changed
+setting cannot be read, such as one giving it the wrong type (a single
+value where a list belongs or a word where a number belongs), is refused.
 
 **`snapshot create`**: Perform incremental backup of configured snapshots.
 * Optional snapshot names argument to create specific snapshots (default: all)

internal/cli/config.go

@@ -8,6 +8,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"reflect"
+	"slices"
 	"strconv"
 	"strings"
 	"unicode/utf8"
@@ -44,6 +45,9 @@ var (
 	errNeedNumericIndex = errors.New("key is a list; use a numeric index")
 	errIndexOutOfRange  = errors.New("index out of range")
 	errNotMapOrList     = errors.New("key is not a map or list")
+	errWouldNotLoad     = errors.New(
+		"vaultik would not load the config after this change, " +
+			"so the file was not changed")
 )
 
 const defaultConfigTemplate = `# vaultik configuration
@@ -391,8 +395,17 @@ func newConfigSetCommand() *cobra.Command {
 		Use:   "set <key> <value>",
 		Short: "Set a config value by dotted path (e.g. compression_level 5)",
 		Long: `Sets a scalar config value addressed by dotted YAML path and writes
-the file back, preserving comments and formatting. Intermediate maps
-are created as needed.
+the file back, preserving comments and formatting. Missing keys along
+the path are created: as a list when the next path element is a
+number, otherwise as a map.
+
+If the config loads, a change after which it would not, such as
+compression_level 99, is refused and the file is left unchanged. A
+config that does not load, such as the one config init writes before
+storage_url is set or one with a ${ENV:...} reference to a variable
+not set here, is checked one setting at a time: only a change after
+which the changed setting cannot be read, such as one giving it the
+wrong type, is refused.
 
 Examples:
   vaultik config set storage_url "file:///mnt/backups"
@@ -422,7 +435,14 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 		return err
 	}
 
-	err = yamlPathSet(root, strings.Split(key, "."), value)
+	oldData, err := marshalConfigYAML(root)
+	if err != nil {
+		return fmt.Errorf("marshaling config: %w", err)
+	}
+
+	keys := strings.Split(key, ".")
+
+	err = yamlPathSet(root, keys, value)
 	if err != nil {
 		return err
 	}
@@ -432,6 +452,21 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 		return fmt.Errorf("marshaling config: %w", err)
 	}
 
+	setting, err := yamlPathOnly(root, keys)
+	if err != nil {
+		return err
+	}
+
+	settingData, err := marshalConfigYAML(setting)
+	if err != nil {
+		return fmt.Errorf("marshaling config: %w", err)
+	}
+
+	err = checkConfigChange(oldData, data, settingData)
+	if err != nil {
+		return fmt.Errorf("%w: %w", errWouldNotLoad, err)
+	}
+
 	err = os.WriteFile(path, data, configFileMode)
 	if err != nil {
 		return fmt.Errorf("writing config file: %w", err)
@@ -453,6 +488,58 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 	return nil
 }
 
+// checkConfigChange returns the reason vaultik would not load newData, the
+// config after a change, if the change brings that failure in. If oldData,
+// the config before it, loads, the new one must load too. A config that
+// does not load, such as one still being filled in or one with a ${...}
+// reference that cannot be filled in here, is checked one setting at a
+// time: settingData, the changed setting on its own, must decode into the
+// config type.
+func checkConfigChange(oldData, newData, settingData []byte) error {
+	oldCfg, err := config.Parse(oldData)
+	if err == nil && oldCfg.Validate() == nil {
+		newCfg, err := config.Parse(newData)
+		if err != nil {
+			return err
+		}
+
+		return newCfg.Validate()
+	}
+
+	_, err = config.Parse(settingData)
+
+	return err
+}
+
+// yamlPathOnly returns a document holding only the value at the dotted key
+// path in root, inside the maps and lists that lead to it there.
+func yamlPathOnly(root *yaml.Node, keys []string) (*yaml.Node, error) {
+	node, err := yamlPathGet(root, keys)
+	if err != nil {
+		return nil, err
+	}
+
+	for i, key := range slices.Backward(keys) {
+		parent, err := yamlPathGet(root, keys[:i])
+		if err != nil {
+			return nil, err
+		}
+
+		if parent.Kind == yaml.SequenceNode {
+			node = &yaml.Node{
+				Kind: yaml.SequenceNode, Content: []*yaml.Node{node},
+			}
+		} else {
+			keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: key}
+			node = &yaml.Node{
+				Kind: yaml.MappingNode, Content: []*yaml.Node{keyNode, node},
+			}
+		}
+	}
+
+	return &yaml.Node{Kind: yaml.DocumentNode, Content: []*yaml.Node{node}}, nil
+}
+
 // marshalConfigYAML renders a config document tree with 2-space indentation,
 // matching defaultConfigTemplate. yaml.Marshal defaults to 4 spaces, which
 // would reindent the whole file on the first `config set` despite the promise
@@ -559,8 +646,8 @@ func yamlPathGet(root *yaml.Node, keys []string) (*yaml.Node, error) {
 	return node, nil
 }
 
-// yamlPathSet navigates a dotted key path, creating intermediate maps as
-// needed, and sets the final key to the given scalar value. Numeric path
+// yamlPathSet navigates a dotted key path, creating missing keys as needed,
+// and sets the final key to the given scalar value. Numeric path
 // components index into sequences; an index equal to the sequence length
 // appends a new element (e.g. "age_recipients.1" on a 1-element list).
 func yamlPathSet(root *yaml.Node, keys []string, value string) error {
@@ -573,12 +660,12 @@ func yamlPathSet(root *yaml.Node, keys []string, value string) error {
 		node = node.Content[0]
 	}
 
-	for i, key := range keys {
+	for i := range keys {
 		last := i == len(keys)-1
 
 		switch node.Kind {
 		case yaml.MappingNode:
-			node = yamlSetInMapping(node, key, value, last)
+			node = yamlSetInMapping(node, keys, i, value, last)
 		case yaml.SequenceNode:
 			next, err := yamlSetInSequence(node, keys, i, value, last)
 			if err != nil {
@@ -647,14 +734,16 @@ func yamlField(typ reflect.Type, key string) (reflect.StructField, bool) {
 	return reflect.StructField{}, false
 }
 
-// yamlSetInMapping resolves (creating if needed) the value node for key
-// within a mapping node, setting it to value when it is the final path
-// element, and returns the node to descend into.
-func yamlSetInMapping(node *yaml.Node, key, value string, last bool) *yaml.Node {
+// yamlSetInMapping resolves (creating if needed) the value node for the
+// path element keys[i] within a mapping node, setting it to value when it
+// is the final path element, and returns the node to descend into.
+func yamlSetInMapping(
+	node *yaml.Node, keys []string, i int, value string, last bool,
+) *yaml.Node {
 	var valueNode *yaml.Node
 
 	for j := 0; j+1 < len(node.Content); j += 2 {
-		if node.Content[j].Value == key {
+		if node.Content[j].Value == keys[i] {
 			valueNode = node.Content[j+1]
 
 			break
@@ -662,13 +751,8 @@ func yamlSetInMapping(node *yaml.Node, key, value string, last bool) *yaml.Node
 	}
 
 	if valueNode == nil {
-		keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: key}
-
-		valueNode = &yaml.Node{Kind: yaml.MappingNode}
-		if last {
-			valueNode = &yaml.Node{Kind: yaml.ScalarNode, Value: value}
-		}
-
+		keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: keys[i]}
+		valueNode = newPathNode(keys, i, value)
 		node.Content = append(node.Content, keyNode, valueNode)
 	} else if last {
 		setScalar(valueNode, value)
@@ -696,12 +780,7 @@ func yamlSetInSequence(
 	}
 
 	if idx == len(node.Content) {
-		newNode := &yaml.Node{Kind: yaml.MappingNode}
-		if last {
-			newNode = &yaml.Node{Kind: yaml.ScalarNode, Value: value}
-		}
-
-		node.Content = append(node.Content, newNode)
+		node.Content = append(node.Content, newPathNode(keys, i, value))
 	} else if last {
 		setScalar(node.Content[idx], value)
 	}
@@ -709,6 +788,22 @@ func yamlSetInSequence(
 	return node.Content[idx], nil
 }
 
+// newPathNode returns the node to create for the missing path element
+// keys[i]: the value when it is the final path element, otherwise an empty
+// list when the next path element is a number and an empty map when not.
+func newPathNode(keys []string, i int, value string) *yaml.Node {
+	if i == len(keys)-1 {
+		return &yaml.Node{Kind: yaml.ScalarNode, Value: value}
+	}
+
+	_, err := strconv.Atoi(keys[i+1])
+	if err == nil {
+		return &yaml.Node{Kind: yaml.SequenceNode}
+	}
+
+	return &yaml.Node{Kind: yaml.MappingNode}
+}
+
 // setScalar overwrites a node in place with a plain scalar value.
 func setScalar(n *yaml.Node, value string) {
 	n.Kind = yaml.ScalarNode

internal/cli/config_test.go

@@ -58,14 +58,17 @@ func TestDefaultConfigTemplateParses(t *testing.T) {
 	}
 }
 
+// testRecipient is a valid age public key.
+const testRecipient = "age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5dqhfj"
+
 // TestConfigSetRecipientOnFreshConfig follows the README quickstart: on the
 // file `config init` writes, `config set age_recipients.0` and
 // `config set storage_url` give a config that loads with that recipient.
+// Neither is refused, although the config does not load until both are set;
+// once it loads, a recipient that is not a valid key is.
 func TestConfigSetRecipientOnFreshConfig(t *testing.T) {
 	t.Parallel()
 
-	const recipient = "age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5dqhfj"
-
 	path := filepath.Join(t.TempDir(), "config.yml")
 
 	err := os.WriteFile(path, []byte(defaultConfigTemplate), configFileMode)
@@ -75,7 +78,7 @@ func TestConfigSetRecipientOnFreshConfig(t *testing.T) {
 
 	out := ui.NewWithColor(&bytes.Buffer{}, false)
 
-	err = writeConfigSet(out, path, "age_recipients.0", recipient)
+	err = writeConfigSet(out, path, "age_recipients.0", testRecipient)
 	if err != nil {
 		t.Fatalf("config set age_recipients.0: %v", err)
 	}
@@ -90,8 +93,211 @@ func TestConfigSetRecipientOnFreshConfig(t *testing.T) {
 		t.Fatalf("config.Load: %v", err)
 	}
 
-	if len(cfg.AgeRecipients) != 1 || cfg.AgeRecipients[0] != recipient {
-		t.Errorf("age_recipients = %v, want [%s]", cfg.AgeRecipients, recipient)
+	if len(cfg.AgeRecipients) != 1 || cfg.AgeRecipients[0] != testRecipient {
+		t.Errorf("age_recipients = %v, want [%s]", cfg.AgeRecipients, testRecipient)
+	}
+
+	err = writeConfigSet(out, path, "age_recipients.0", "notakey")
+	if err == nil || !strings.Contains(err.Error(), "not a valid recipient") {
+		t.Errorf("config set age_recipients.0 notakey gives error %v, "+
+			"want one saying it is not a valid recipient", err)
+	}
+
+	_, err = config.Load(path)
+	if err != nil {
+		t.Errorf("config.Load after the refused change: %v", err)
+	}
+}
+
+// configWithoutRecipients loads, but has no age_recipients key and no
+// snapshot named work.
+const configWithoutRecipients = `storage_url: file:///mnt/backups
+snapshots:
+  home:
+    paths:
+      - "~"
+`
+
+// configWithUnsetReference loads only where VAULTIK_TEST_UNSET_SECRET is
+// set, as a backup job's environment might set it. No test sets it.
+const configWithUnsetReference = configWithoutRecipients + `s3:
+  secret_access_key: ${ENV:VAULTIK_TEST_UNSET_SECRET}
+`
+
+// configWithLevelReference returns configWithoutRecipients with a ${FILE:...}
+// reference on compression_level to a file holding 9, so that the config
+// loads once the reference is filled in.
+func configWithLevelReference(t *testing.T) string {
+	t.Helper()
+
+	levelPath := filepath.Join(t.TempDir(), "level")
+
+	err := os.WriteFile(levelPath, []byte("9"), configFileMode)
+	if err != nil {
+		t.Fatalf("write level: %v", err)
+	}
+
+	return configWithoutRecipients +
+		"compression_level: ${FILE:" + levelPath + "}\n"
+}
+
+// TestConfigSetIndexUnderMissingKey checks that a list index under a key
+// the file does not have creates a list, and that the result loads.
+func TestConfigSetIndexUnderMissingKey(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		key   string
+		value string
+		field func(cfg *config.Config) []string
+	}{
+		{"age_recipients.0", testRecipient,
+			func(cfg *config.Config) []string { return cfg.AgeRecipients }},
+		{"snapshots.work.paths.0", "/srv/work",
+			func(cfg *config.Config) []string { return cfg.Snapshots["work"].Paths }},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.key, func(t *testing.T) {
+			t.Parallel()
+
+			path := filepath.Join(t.TempDir(), "config.yml")
+
+			err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode)
+			if err != nil {
+				t.Fatalf("write config: %v", err)
+			}
+
+			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
+				path, tt.key, tt.value)
+			if err != nil {
+				t.Fatalf("config set %s: %v", tt.key, err)
+			}
+
+			cfg, err := config.Load(path)
+			if err != nil {
+				t.Fatalf("config.Load: %v", err)
+			}
+
+			got := tt.field(cfg)
+			if len(got) != 1 || got[0] != tt.value {
+				t.Errorf("%s gives %v, want [%s]", tt.key, got, tt.value)
+			}
+		})
+	}
+}
+
+// TestConfigSetKeepsFileThatWouldNotLoad checks that config set refuses a
+// change after which the config would not load, with an error giving the
+// reason, and leaves the file as it was. On the file `config init` writes,
+// which does not load until storage_url is set, and on one that cannot be
+// read here, only a value of the wrong type is refused.
+func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
+	t.Parallel()
+
+	levelReference := configWithLevelReference(t)
+
+	tests := []struct {
+		config string
+		key    string
+		value  string
+		reason string
+	}{
+		// Index 1 of the empty list the missing key gives.
+		{configWithoutRecipients, "age_recipients.1", testRecipient,
+			"index out of range"},
+		// A list set to a single value.
+		{configWithoutRecipients, "snapshots.home.paths", "/srv",
+			"into []string"},
+		{configWithoutRecipients, "chunk_size", "fast",
+			"invalid size format"},
+		// Values of the right type that fail validation.
+		{configWithoutRecipients, "compression_level", "99",
+			"compression_level must be between 1 and 19"},
+		{configWithoutRecipients, "chunk_size", "1KB",
+			"chunk_size must be at least 1MB"},
+		// With its reference filled in, the config loads.
+		{levelReference, "blob_size_limit", "1MB",
+			"blob_size_limit must be at least"},
+		// The type check on a config that does not load yet.
+		{defaultConfigTemplate, "snapshots.apps.paths", "/srv",
+			"into []string"},
+		// The type check on a config that cannot be read here.
+		{configWithUnsetReference, "snapshots.home.paths", "/x",
+			"cannot unmarshal !!str `/x`"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.key+" "+tt.value, func(t *testing.T) {
+			t.Parallel()
+
+			path := filepath.Join(t.TempDir(), "config.yml")
+
+			err := os.WriteFile(path, []byte(tt.config), configFileMode)
+			if err != nil {
+				t.Fatalf("write config: %v", err)
+			}
+
+			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
+				path, tt.key, tt.value)
+			if err == nil || !strings.Contains(err.Error(), tt.reason) {
+				t.Errorf("config set %s %s gives error %v, want one saying %q",
+					tt.key, tt.value, err, tt.reason)
+			}
+
+			data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
+			if err != nil {
+				t.Fatalf("read config: %v", err)
+			}
+
+			if string(data) != tt.config {
+				t.Errorf("config set %s changed the file to:\n%s",
+					tt.key, data)
+			}
+		})
+	}
+}
+
+// TestConfigSetRefusesOnlyItsOwnFailure checks that setting another key
+// works on a config with a ${...} reference on compression_level, which
+// loads because loading fills the reference in, on one whose
+// compression_level already fails to load, and on one with a reference
+// that cannot be filled in here.
+func TestConfigSetRefusesOnlyItsOwnFailure(t *testing.T) {
+	t.Parallel()
+
+	configs := map[string]string{
+		"reference":       configWithLevelReference(t),
+		"failing":         configWithoutRecipients + "compression_level: fast\n",
+		"unset reference": configWithUnsetReference,
+	}
+
+	for name, configYAML := range configs {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			path := filepath.Join(t.TempDir(), "config.yml")
+
+			err := os.WriteFile(path, []byte(configYAML), configFileMode)
+			if err != nil {
+				t.Fatalf("write config: %v", err)
+			}
+
+			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
+				path, "hostname", "myhost")
+			if err != nil {
+				t.Fatalf("config set hostname: %v", err)
+			}
+
+			data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
+			if err != nil {
+				t.Fatalf("read config: %v", err)
+			}
+
+			if !strings.Contains(string(data), "hostname: myhost") {
+				t.Errorf("config set hostname wrote:\n%s", data)
+			}
+		})
 	}
 }
 

internal/config/config.go

@@ -4,6 +4,7 @@
 package config
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"os"
@@ -233,14 +234,48 @@ func New(path Path) (*Config, error) {
 	return cfg, nil
 }
 
-// Load reads and parses the configuration file from the specified path.
-// It applies default values for optional fields, performs environment variable
-// substitution using smartconfig, and validates the configuration.
-// The configuration file should be in YAML format. Returns an error if the file
-// cannot be read, parsed, or if validation fails.
+// Load reads the configuration file at path, parses it with Parse, warns
+// when others can read the file, and validates the configuration.
+// Returns an error if the file cannot be read, parsed, or if validation
+// fails.
 func Load(path string) (*Config, error) {
+	data, err := os.ReadFile(path) //nolint:gosec // G304: config path is operator-supplied
+	if err != nil {
+		return nil, fmt.Errorf("failed to load config file: %w", err)
+	}
+
+	cfg, err := Parse(data)
+	if err != nil {
+		return nil, err
+	}
+
+	// Check config file permissions (warn if world or group readable)
+	//nolint:gosec // G703: config path is operator-supplied by design
+	info, statErr := os.Stat(path)
+	if statErr == nil {
+		mode := info.Mode().Perm()
+		if mode&0044 != 0 { // group or world readable
+			log.Warn(cfg.readableByOthersWarning(),
+				"path", path,
+				"mode", fmt.Sprintf("%04o", mode),
+				"recommendation", "chmod 600 "+path)
+		}
+	}
+
+	err = cfg.Validate()
+	if err != nil {
+		return nil, fmt.Errorf("invalid config: %w", err)
+	}
+
+	return cfg, nil
+}
+
+// Parse reads YAML config data as Load does, without validating it. It
+// applies default values for optional fields, fills in ${...} references
+// using smartconfig, and expands ~ in paths.
+func Parse(data []byte) (*Config, error) {
 	// Load config using smartconfig for interpolation
-	sc, err := smartconfig.NewFromConfigPath(path)
+	sc, err := smartconfig.NewFromReader(bytes.NewReader(data))
 	if err != nil {
 		return nil, fmt.Errorf("failed to load config file: %w", err)
 	}
@@ -302,24 +337,6 @@ func Load(path string) (*Config, error) {
 		cfg.S3.Region = "us-east-1"
 	}
 
-	// Check config file permissions (warn if world or group readable)
-	//nolint:gosec // G703: config path is operator-supplied by design
-	info, statErr := os.Stat(path)
-	if statErr == nil {
-		mode := info.Mode().Perm()
-		if mode&0044 != 0 { // group or world readable
-			log.Warn(cfg.readableByOthersWarning(),
-				"path", path,
-				"mode", fmt.Sprintf("%04o", mode),
-				"recommendation", "chmod 600 "+path)
-		}
-	}
-
-	err = cfg.Validate()
-	if err != nil {
-		return nil, fmt.Errorf("invalid config: %w", err)
-	}
-
 	return cfg, nil
 }
 

internal/storage/url.go

@@ -30,6 +30,12 @@ var (
 		"storage URL must not carry credentials; " +
 			"set s3.access_key_id and s3.secret_access_key in the config instead")
 	ErrURLUnknownParam = errors.New("unknown query parameter in storage URL")
+	ErrURLBadQuery     = errors.New(
+		"storage URL query cannot be decoded; " +
+			"check it for a ; or a % not followed by two hex digits")
+	ErrRcloneRemoteParams = errors.New(
+		"rclone URL must not carry parameters in the remote name; " +
+			"set them on the remote with rclone config instead")
 )
 
 // URL represents a parsed storage URL.
@@ -90,8 +96,9 @@ func parseFileURL(rawPath string) (*URL, error) {
 }
 
 // parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
-// the userinfo and any query parameter other than endpoint, region and
-// ssl, so a credential-bearing URL is never stored or echoed.
+// the userinfo or in the endpoint parameter's userinfo, a query that
+// cannot be decoded, and any query parameter other than endpoint, region
+// and ssl, so a credential-bearing URL is never stored or echoed.
 func parseS3URL(rawURL string) (*URL, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -107,13 +114,26 @@ func parseS3URL(rawURL string) (*URL, error) {
 		return nil, ErrMissingBucket
 	}
 
-	query := u.Query()
+	// u.Query() would silently drop a part it cannot decode, endpoint
+	// included. The decode error is not wrapped: it can quote the value.
+	query, err := url.ParseQuery(u.RawQuery)
+	if err != nil {
+		return nil, ErrURLBadQuery
+	}
 
 	err = rejectUnknownParams(query, "endpoint", "region", "ssl")
 	if err != nil {
 		return nil, err
 	}
 
+	// A host name never contains "@", so one in an endpoint, with or
+	// without a scheme, can only introduce userinfo.
+	for _, endpoint := range query["endpoint"] {
+		if strings.Contains(endpoint, "@") {
+			return nil, ErrURLCredentials
+		}
+	}
+
 	return &URL{
 		Scheme:   schemeS3,
 		Bucket:   bucket,
@@ -125,9 +145,23 @@ func parseS3URL(rawURL string) (*URL, error) {
 }
 
 // parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no
-// query parameters, so credentials in the userinfo and any parameter at
-// all are rejected rather than silently ignored.
+// query parameters, so credentials in the userinfo, parameters in the
+// remote name and any query parameter at all are rejected rather than
+// silently ignored.
 func parseRcloneURL(rawURL string) (*URL, error) {
+	// The remote name runs up to the first "/", "?" or "#". It never
+	// contains a comma; one starts rclone's remote,param=value form,
+	// whose values can be credentials. This is checked before url.Parse,
+	// whose error for such a name can print part of it.
+	remote := strings.TrimPrefix(rawURL, "rclone://")
+	if end := strings.IndexAny(remote, "/?#"); end >= 0 {
+		remote = remote[:end]
+	}
+
+	if strings.Contains(remote, ",") {
+		return nil, ErrRcloneRemoteParams
+	}
+
 	u, err := url.Parse(rawURL)
 	if err != nil {
 		return nil, wrapParseError(err)
@@ -137,7 +171,7 @@ func parseRcloneURL(rawURL string) (*URL, error) {
 		return nil, ErrURLCredentials
 	}
 
-	remote := u.Host
+	remote = u.Host
 	if remote == "" {
 		return nil, ErrMissingRemote
 	}

internal/storage/url_parse_test.go

@@ -150,6 +150,19 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 			wantErr: storage.ErrURLCredentials,
 			secrets: []string{key, secret, "mybucket"},
 		},
+		{
+			name: "s3 userinfo in endpoint",
+			raw: "s3://mybucket?endpoint=https://" +
+				key + ":" + secret + "@minio.example.com",
+			wantErr: storage.ErrURLCredentials,
+			secrets: []string{key, secret},
+		},
+		{
+			name:    "s3 userinfo in endpoint without scheme",
+			raw:     "s3://mybucket?endpoint=" + key + ":" + secret + "@minio.example.com",
+			wantErr: storage.ErrURLCredentials,
+			secrets: []string{key, secret},
+		},
 		{
 			name:    "s3 unknown query param",
 			raw:     "s3://mybucket?access_key=" + key + "&secret=" + secret,
@@ -203,6 +216,104 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 	}
 }
 
+// TestParseStorageURLRejectsRcloneRemoteParams checks that an rclone
+// remote name carrying parameters (rclone's "remote,param=value" form,
+// which can hold a credential) is rejected without echoing the value,
+// including the URLs that url.Parse itself refuses.
+func TestParseStorageURLRejectsRcloneRemoteParams(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name    string
+		raw     string
+		secrets []string // must not appear in the error message
+	}{
+		{
+			name:    "configured remote",
+			raw:     "rclone://gdrive,client_secret=topsecret/backups",
+			secrets: []string{"topsecret"},
+		},
+		{
+			name:    "remote defined inline",
+			raw:     "rclone://:s3,access_key_id=KEY,secret_access_key=SECRET/bucket",
+			secrets: []string{"KEY", "SECRET"},
+		},
+		{
+			name:    "colon in a parameter value",
+			raw:     "rclone://gdrive,client_secret=ab:cd/backups",
+			secrets: []string{"cd"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := storage.ParseStorageURL(tc.raw)
+			if !errors.Is(err, storage.ErrRcloneRemoteParams) {
+				t.Fatalf("ParseStorageURL(%q) error = %v, want %v",
+					tc.raw, err, storage.ErrRcloneRemoteParams)
+			}
+
+			for _, secret := range tc.secrets {
+				if strings.Contains(err.Error(), secret) {
+					t.Errorf("error message leaked %q: %v", secret, err.Error())
+				}
+			}
+		})
+	}
+}
+
+// TestParseStorageURLRejectsUndecodableQuery checks that an s3 URL whose
+// query cannot be decoded is rejected, not read with that part of the
+// query left out, and that the error does not echo it.
+func TestParseStorageURLRejectsUndecodableQuery(t *testing.T) {
+	t.Parallel()
+
+	const (
+		key    = "AKIAKEY"
+		secret = "topsecret"
+	)
+
+	cases := []struct {
+		name string
+		raw  string
+	}{
+		{
+			name: "semicolon",
+			raw: "s3://mybucket/prefix?endpoint=https://" + key + ":" + secret +
+				"@minio.example.com:9000;region=us-east-1",
+		},
+		{
+			name: "bad percent escape",
+			raw: "s3://mybucket/prefix?endpoint=https://" + key + ":" + secret +
+				"%zz@minio.example.com:9000",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := storage.ParseStorageURL(tc.raw)
+			if err == nil {
+				t.Fatalf("ParseStorageURL(%q) returned no error", tc.raw)
+			}
+
+			if !errors.Is(err, storage.ErrURLBadQuery) {
+				t.Errorf("ParseStorageURL(%q) error = %v, want %v",
+					tc.raw, err, storage.ErrURLBadQuery)
+			}
+
+			for _, s := range []string{key, secret, "minio.example.com"} {
+				if strings.Contains(err.Error(), s) {
+					t.Errorf("error message leaked %q: %v", s, err.Error())
+				}
+			}
+		})
+	}
+}
+
 // TestParseStorageURLParseFailureHidesURL checks that when url.Parse
 // itself fails, the wrapped error carries only the inner cause, not the
 // *url.Error whose text embeds the raw (possibly credential-bearing) URL.