From 1ad3faa8ee0ba17b1a0204ab93ba5d41b878cf07 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 11:01:40 +0000 Subject: [PATCH] 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 | 38 ++++++++++++++++--- internal/storage/url_parse_test.go | 61 ++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 6 deletions(-) diff --git a/internal/storage/url.go b/internal/storage/url.go index 3e41198..d0773e4 100644 --- a/internal/storage/url.go +++ b/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 -// ssl, so a credential-bearing URL is never stored or echoed. +// the userinfo or in the endpoint parameter's userinfo, 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 { @@ -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 -// 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 +163,7 @@ func parseRcloneURL(rawURL string) (*URL, error) { return nil, ErrURLCredentials } - remote := u.Host + remote = u.Host if remote == "" { return nil, ErrMissingRemote } diff --git a/internal/storage/url_parse_test.go b/internal/storage/url_parse_test.go index 2151249..2b64a71 100644 --- a/internal/storage/url_parse_test.go +++ b/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.