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. The comma is checked
before url.Parse, because url.Parse refuses a name such as
":s3,secret_access_key=..." or "x,secret=a:b", and its error prints
the name from the colon on. 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,9 +137,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
+// 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) {
+	// 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 +163,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,54 @@ 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())
+				}
+			}
+		})
+	}
+}
+
 // 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.