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 or rclone query
that cannot be decoded (a ";" or a bad "%" escape) is refused too,
because decoding drops that part before it can be checked. The comma
is checked before url.Parse, whose error for such a name prints part of
it. The rclone remote name case has its own error pointing at rclone
config.

Model: opus-5-5

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,112 @@ 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 or rclone
+// 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",
+		},
+		{
+			name: "rclone semicolon",
+			raw:  "rclone://gdrive/backups?token=" + secret + ";x=1",
+		},
+		{
+			name: "rclone bad percent escape",
+			raw:  "rclone://gdrive/backups?token=" + secret + "%zz",
+		},
+	}
+
+	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.