diff --git a/internal/storage/url.go b/internal/storage/url.go index 3e41198..53750a0 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,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 -// 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) { 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 diff --git a/internal/storage/url_parse_test.go b/internal/storage/url_parse_test.go index 2151249..64062f2 100644 --- a/internal/storage/url_parse_test.go +++ b/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.