Warn about restored ownership only when an entry's owner or group differs (closes #316)

A restore by a normal user always warned that file ownership was not
preserved, even when every entry came out with its recorded owner and
group. After such a restore, the owner and group each restored entry has
on disk are now compared with the recorded ones, and the warning is
printed once if any differ. The comparison is made on disk because a new
entry does not always get the user's group: on macOS, and in a setgid
directory on Linux, it takes the group of its directory. A restore as
root never warns, as before.

Judgement call: an entry that is not on disk after the restore, such as one that failed, is not compared.
Unverified: not run on a Mac; the directory-group test uses a setgid directory on Linux and runs only as root.

Model: opus-5-5

internal/storage/url.go

@@ -30,12 +30,6 @@ var (
 		"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")
-	ErrURLBadQuery     = errors.New(
-		"storage URL query cannot be decoded; " +
-			"check it for a ; or a % not followed by two hex digits")
-	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.
@@ -96,9 +90,8 @@ func parseFileURL(rawPath string) (*URL, error) {
 }
 
 // parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
-// the userinfo or in the endpoint parameter's userinfo, a query that
-// cannot be decoded, and any query parameter other than endpoint, region
-// and ssl, so a credential-bearing URL is never stored or echoed.
+// the 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,26 +107,13 @@ func parseS3URL(rawURL string) (*URL, error) {
 		return nil, ErrMissingBucket
 	}
 
-	// u.Query() would silently drop a part it cannot decode, endpoint
-	// included. The decode error is not wrapped: it can quote the value.
-	query, err := url.ParseQuery(u.RawQuery)
-	if err != nil {
-		return nil, ErrURLBadQuery
-	}
+	query := u.Query()
 
 	err = rejectUnknownParams(query, "endpoint", "region", "ssl")
 	if err != nil {
 		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,
@@ -145,23 +125,9 @@ func parseS3URL(rawURL string) (*URL, error) {
 }
 
 // parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no
-// query parameters, so credentials in the userinfo, parameters in the
-// remote name, a query that cannot be decoded and any query parameter at
+// query parameters, so credentials in the userinfo and any 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)
@@ -171,19 +137,12 @@ func parseRcloneURL(rawURL string) (*URL, error) {
 		return nil, ErrURLCredentials
 	}
 
-	remote = u.Host
+	remote := u.Host
 	if remote == "" {
 		return nil, ErrMissingRemote
 	}
 
-	// As in parseS3URL: u.Query() would drop a part it cannot decode, and
-	// the decode error can quote the value.
-	query, err := url.ParseQuery(u.RawQuery)
-	if err != nil {
-		return nil, ErrURLBadQuery
-	}
-
-	err = rejectUnknownParams(query)
+	err = rejectUnknownParams(u.Query())
 	if err != nil {
 		return nil, err
 	}

internal/storage/url_parse_test.go

@@ -150,19 +150,6 @@ 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,
@@ -216,112 +203,6 @@ 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.