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 when the storage URL is parsed, without echoing the value: a
host name never contains "@", and an rclone remote name never contains
a comma. The rclone case has its own error pointing at rclone config,
since the existing credentials error names the s3.* keys.

Model: opus-5-5

internal/storage/url.go

@@ -29,7 +29,10 @@ var (
 	ErrURLCredentials     = errors.New(
 		"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")
+	ErrURLUnknownParam    = errors.New("unknown query parameter in storage URL")
+	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 +93,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
+// the userinfo or in the endpoint parameter's userinfo, and any query
-// ssl, so a credential-bearing URL is never stored or echoed.
+// 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 {
@@ -114,6 +118,14 @@ func parseS3URL(rawURL string) (*URL, error) {
 		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,8 +137,9 @@ 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
+// query parameters, so credentials in the userinfo, parameters in the
-// all are rejected rather than silently ignored.
+// remote name and any query parameter at all are rejected rather than
+// silently ignored.
 func parseRcloneURL(rawURL string) (*URL, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -142,6 +155,12 @@ func parseRcloneURL(rawURL string) (*URL, error) {
 		return nil, ErrMissingRemote
 	}
 
+	// An rclone remote name never contains a comma; one starts rclone's
+	// remote,param=value form, whose values can be credentials.
+	if strings.Contains(remote, ",") {
+		return nil, ErrRcloneRemoteParams
+	}
+
 	err = rejectUnknownParams(u.Query())
 	if err != nil {
 		return nil, err

internal/storage/url_parse_test.go

@@ -150,6 +150,18 @@ 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 +215,27 @@ 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.
+func TestParseStorageURLRejectsRcloneRemoteParams(t *testing.T) {
+	t.Parallel()
+
+	const secret = "topsecret"
+
+	raw := "rclone://gdrive,client_secret=" + secret + "/backups"
+
+	_, err := storage.ParseStorageURL(raw)
+	if !errors.Is(err, storage.ErrRcloneRemoteParams) {
+		t.Fatalf("ParseStorageURL(%q) error = %v, want %v",
+			raw, err, storage.ErrRcloneRemoteParams)
+	}
+
+	if strings.Contains(err.Error(), secret) {
+		t.Errorf("error message leaked %q: %v", secret, 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.