Repository: minio/minio (language: go)

Issue:
`The TagValue you have provided is invalid` breaks Console and tag deletion
We have a minio instance with a couple of buckets that seem to have invalid tag values. I'm not sure how we managed to even write these values (has there recently been a problem with the validation in the tagging API?). These few affected buckets completely break the console login. Additionally, using mcli, I cannot remove the tags from the affected buckets.

It is always the same issue: `The TagValue you have provided is invalid`. The UI stays white after login, but the error can be found in the log.

## Expected Behavior

My expectation would have been, that invalid values can't even be added to objects. However assuming this could happen (e.g. through bugs), it should at least be possible to delete the broken tags. And in any case the console should not completely break, just because some of the buckets are invalid. It should either discard the invalid information, discard the invalid buckets/objects or at least display a useful message in the UI instead of a white blank page.

## Current Behavior
It is probably not currently possible to add invalid tags (not sure, since we remove these in out system), however if an invalid tag ends up on a bucket/project you won't be able to remove it and you won't be able to login to the console.

## Possible Solution
The login should handle the HTTP 400 response properly and continue with the remaining useful data and/or display a message indicating this issue.
The API should _at least_ allow to delete invalid tags, but I guess other operations could work too.

## Steps to Reproduce (for bugs)
<!--- Provide a link to a live example, or an unambiguous set of steps to -->
<!--- reproduce this bug. Include code to reproduce, if relevant -->
<!--- and make sure you have followed https://github.com/minio/minio/tree/release/docs/debugging to capture relevant logs -->

1. Given a minio instance that has invalid tags on a bucket
2. try logging in to the console
3. try deleting the tags by running `mcli tag remove --debug your-minio/the-bucket`

## Your Environment
<!--- Include as many relevant details about the environment you experienced the bug in -->
* Version used (`minio --version`): https://github.com/minio/minio/releases/tag/RELEASE.2022-10-05T14-58-27Z



Discussion / hints:
Any existing tags that were incompatible is not supported anymore.

We used to support a wider array of tag values which was wrong so we moved the tags to be more in line with S3 compatible behavior.

You can write an API call directly to wipe them. 
We are using the regex `[^\p{L}\p{Z}\p{N}_.:/=+\-@]` from https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html to replace invalid characters with underscores.
> We are using the regex `[^\p{L}\p{Z}\p{N}_.:/=+\-@]` from https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html to replace invalid characters with underscores.

This not true for all services, AWS S3 doesn't support this. 
oh. is there a correct regex or list of characters that is supported?
https://github.com/minio/minio-go/blob/master/pkg/tags/tags.go#L72 @pschichtel 
> 3\. try deleting the tags by running `mcli tag remove --debug your-minio/the-bucket`

is this not allowed you say? @pschichtel can provide the full output?
```
mcli: <DEBUG> GET /8a38cb42-a396-4b97-a1cb-7d68411370f5/?location= HTTP/1.1
Host: some-host
User-Agent: MinIO (linux; amd64) minio-go/v7.0.34 mcli/RELEASE.2022-08-23T05-45-20Z
Authorization: 
X-Amz-Content-Sha256: UNSIGNED-PAYLOAD
X-Amz-Date: 20221019T164633Z
Accept-Encoding: gzip

mcli: <DEBUG> HTTP/1.1 200 OK
Content-Length: 128
Accept-Ranges: bytes
Access-Control-Allow-Origin: *
Content-Security-Policy: block-all-mixed-content
Content-Type: application/xml
Date: Wed, 19 Oct 2022 16:46:33 GMT
Server: MinIO
Strict-Transport-Security: max-age=31536000; includeSubDomains
Vary: Origin
Vary: Accept-Encoding
X-Amz-Request-Id: 171F863F42BBD3DD
X-Content-Type-Options: nosniff
X-Xss-Protection: 1; mode=block

mcli: <DEBUG> TLS Certificate found: 
mcli: <DEBUG>  >> Country: US
mcli: <DEBUG>  >> Organization: Let's Encrypt
mcli: <DEBUG>  >> Expires: 2022-12-26 19:42:58 +0000 UTC
mcli: <DEBUG> TLS Certificate found: 
mcli: <DEBUG>  >> Country: US
mcli: <DEBUG>  >> Organization: Internet Security Research Group
mcli: <DEBUG>  >> Expires: 2025-09-15 16:00:00 +0000 UTC
mcli: <DEBUG> TLS Certificate found: 
mcli: <DEBUG>  >> Organization: Digital Signature Trust Co.
mcli: <DEBUG>  >> Expires: 2024-09-30 18:14:03 +0000 UTC
mcli: <DEBUG> Response Time:  114.46329ms

mcli: <DEBUG> DELETE /8a38cb42-a396-4b97-a1cb-7d68411370f5/?tagging= HTTP/1.1
Host: some-host
User-Agent: MinIO (linux; amd64) minio-go/v7.0.34 mcli/RELEASE.2022-08-23T05-45-20Z
Authorization: 
X-Amz-Content-Sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
X-Amz-Date: 20221019T164633Z
Accept-Encoding: gzip

mcli: <DEBUG> HTTP/1.1 400 Bad Request
Content-Length: 349
Accept-Ranges: bytes
Access-Control-Allow-Origin: *
Content-Security-Policy: block-all-mixed-content
Content-Type: application/xml
Date: Wed, 19 Oct 2022 16:46:33 GMT
Server: MinIO
Strict-Transport-Security: max-age=31536000; includeSubDomains
Vary: Origin
Vary: Accept-Encoding
X-Amz-Request-Id: 171F863F438DCB76
X-Content-Type-Options: nosniff
X-Xss-Protection: 1; mode=block

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>InvalidTag</Code><Message>The TagValue you have provided is invalid</Message><BucketName>8a38cb42-a396-4b97-a1cb-7d68411370f5</BucketName><Resource>/8a38cb42-a396-4b97-a1cb-7d68411370f5/</Resource><RequestId>171F863F438DCB76</RequestId><HostId>076af6dd-a831-4ead-bfaa-d3530535b849</HostId></Error>mcli: <DEBUG> TLS Certificate found: 
mcli: <DEBUG>  >> Country: US
mcli: <DEBUG>  >> Organization: Let's Encrypt
mcli: <DEBUG>  >> Expires: 2022-12-26 19:42:58 +0000 UTC
mcli: <DEBUG> TLS Certificate found: 
mcli: <DEBUG>  >> Country: US
mcli: <DEBUG>  >> Organization: Internet Security Research Group
mcli: <DEBUG>  >> Expires: 2025-09-15 16:00:00 +0000 UTC
mcli: <DEBUG> TLS Certificate found: 
mcli: <DEBUG>  >> Organization: Digital Signature Trust Co.
mcli: <DEBUG>  >> Expires: 2024-09-30 18:14:03 +0000 UTC
mcli: <DEBUG> Response Time:  14.464088ms

mcli: <ERROR> Unable to remove tags for https://some-host/8a38cb42-a396-4b97-a1cb-7d68411370f5. The TagValue you have provided is invalid
 (0) client-s3.go:2536 cmd.(*S3Client).DeleteTags(..)
 Release-Tag:RELEASE.2022-08-23T05-45-20Z | Commit:07fffc3bc88f70a73c3714d9af23122f611b76cf | Host:cln888 | OS:linux | Arch:amd64 | Lang:go1.19 | Mem:2.6 MB/25 MB | Heap:2.6 MB/12 MB.
 ```
this is what the server logged:

```json
{
  "deploymentid": "076af6dd-a831-4ead-bfaa-d3530535b849",
  "level": "ERROR",
  "errKind": "ALL",
  "time": "2022-10-19T16:46:33.425914139Z",
  "api": {
    "name": "DeleteBucketTagging",
    "args": {
      "bucket": "8a38cb42-a396-4b97-a1cb-7d68411370f5"
    }
  },
  "remotehost": "some-ip",
  "host": "some-host",
  "requestID": "171F863F438DCB76",
  "userAgent": "MinIO (linux; amd64) minio-go/v7.0.34 mcli/RELEASE.2022-08-23T05-45-20Z",
  "error": {
    "message": "The TagValue you have provided is invalid (*tags.errTag)",
    "source": [
      "internal/logger/logger.go:259:logger.LogIf()",
      "cmd/api-errors.go:2164:cmd.toAPIErrorCode()",
      "cmd/api-errors.go:2189:cmd.toAPIError()",
      "cmd/bucket-handlers.go:1623:cmd.objectAPIHandlers.DeleteBucketTaggingHandler()",
      "net/http/server.go:2084:http.HandlerFunc.ServeHTTP()"
    ]
  }
}
```
Yeah I think during delete we can avoid parsing things, let me send a fix for this.

Fix patch:
diff --git a/cmd/bucket-encryption-handlers.go b/cmd/bucket-encryption-handlers.go
index ec47f43a283e9..aa5999485808c 100644
--- a/cmd/bucket-encryption-handlers.go
+++ b/cmd/bucket-encryption-handlers.go
@@ -204,7 +204,7 @@ func (api objectAPIHandlers) DeleteBucketEncryptionHandler(w http.ResponseWriter
 	}
 
 	// Delete bucket encryption config from object layer
-	updatedAt, err := globalBucketMetadataSys.Update(ctx, bucket, bucketSSEConfig, nil)
+	updatedAt, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketSSEConfig)
 	if err != nil {
 		writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
 		return
diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go
index b0afca206ec70..18ff802d97623 100644
--- a/cmd/bucket-handlers.go
+++ b/cmd/bucket-handlers.go
@@ -1618,7 +1618,7 @@ func (api objectAPIHandlers) DeleteBucketTaggingHandler(w http.ResponseWriter, r
 		return
 	}
 
-	updatedAt, err := globalBucketMetadataSys.Update(ctx, bucket, bucketTaggingConfig, nil)
+	updatedAt, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketTaggingConfig)
 	if err != nil {
 		writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
 		return
diff --git a/cmd/bucket-lifecycle-handlers.go b/cmd/bucket-lifecycle-handlers.go
index e7a8943cc071f..77d6432f7c0c8 100644
--- a/cmd/bucket-lifecycle-handlers.go
+++ b/cmd/bucket-lifecycle-handlers.go
@@ -168,7 +168,7 @@ func (api objectAPIHandlers) DeleteBucketLifecycleHandler(w http.ResponseWriter,
 		return
 	}
 
-	if _, err := globalBucketMetadataSys.Update(ctx, bucket, bucketLifecycleConfig, nil); err != nil {
+	if _, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketLifecycleConfig); err != nil {
 		writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
 		return
 	}
diff --git a/cmd/bucket-metadata-sys.go b/cmd/bucket-metadata-sys.go
index b4c2c9b8efbed..b7bc78eed0d95 100644
--- a/cmd/bucket-metadata-sys.go
+++ b/cmd/bucket-metadata-sys.go
@@ -81,9 +81,7 @@ func (sys *BucketMetadataSys) Set(bucket string, meta BucketMetadata) {
 	}
 }
 
-// Update update bucket metadata for the specified config file.
-// The configData data should not be modified after being sent here.
-func (sys *BucketMetadataSys) Update(ctx context.Context, bucket string, configFile string, configData []byte) (updatedAt time.Time, err error) {
+func (sys *BucketMetadataSys) updateAndParse(ctx context.Context, bucket string, configFile string, configData []byte, parse bool) (updatedAt time.Time, err error) {
 	objAPI := newObjectLayerFn()
 	if objAPI == nil {
 		return updatedAt, errServerNotInitialized
@@ -107,7 +105,7 @@ func (sys *BucketMetadataSys) Update(ctx context.Context, bucket string, configF
 		return updatedAt, errInvalidArgument
 	}
 
-	meta, err := loadBucketMetadata(ctx, objAPI, bucket)
+	meta, err := loadBucketMetadataParse(ctx, objAPI, bucket, parse)
 	if err != nil {
 		if !globalIsErasure && !globalIsDistErasure && errors.Is(err, errVolumeNotFound) {
 			// Only single drive mode needs this fallback.
@@ -165,6 +163,18 @@ func (sys *BucketMetadataSys) Update(ctx context.Context, bucket string, configF
 	return updatedAt, nil
 }
 
+// Delete delete the bucket metadata for the specified bucket.
+// must be used by all callers instead of using Update() with nil configData.
+func (sys *BucketMetadataSys) Delete(ctx context.Context, bucket string, configFile string) (updatedAt time.Time, err error) {
+	return sys.updateAndParse(ctx, bucket, configFile, nil, false)
+}
+
+// Update update bucket metadata for the specified bucket.
+// The configData data should not be modified after being sent here.
+func (sys *BucketMetadataSys) Update(ctx context.Context, bucket string, configFile string, configData []byte) (updatedAt time.Time, err error) {
+	return sys.updateAndParse(ctx, bucket, configFile, configData, true)
+}
+
 // Get metadata for a bucket.
 // If no metadata exists errConfigNotFound is returned and a new metadata is returned.
 // Only a shallow copy is returned, so referenced data should not be modified,
diff --git a/cmd/bucket-metadata.go b/cmd/bucket-metadata.go
index 2d5376babd047..2f565d6b59dce 100644
--- a/cmd/bucket-metadata.go
+++ b/cmd/bucket-metadata.go
@@ -162,8 +162,7 @@ func (b *BucketMetadata) Load(ctx context.Context, api ObjectLayer, name string)
 	return err
 }
 
-// loadBucketMetadata loads and migrates to bucket metadata.
-func loadBucketMetadata(ctx context.Context, objectAPI ObjectLayer, bucket string) (BucketMetadata, error) {
+func loadBucketMetadataParse(ctx context.Context, objectAPI ObjectLayer, bucket string, parse bool) (BucketMetadata, error) {
 	b := newBucketMetadata(bucket)
 	err := b.Load(ctx, objectAPI, b.Name)
 	if err != nil && !errors.Is(err, errConfigNotFound) {
@@ -172,8 +171,22 @@ func loadBucketMetadata(ctx context.Context, objectAPI ObjectLayer, bucket strin
 	if err == nil {
 		b.defaultTimestamps()
 	}
-	// Old bucket without bucket metadata. Hence we migrate existing settings.
-	if err := b.convertLegacyConfigs(ctx, objectAPI); err != nil {
+
+	configs, err := b.getAllLegacyConfigs(ctx, objectAPI)
+	if err != nil {
+		return b, err
+	}
+
+	if len(configs) == 0 {
+		if parse {
+			// nothing to update, parse and proceed.
+			err = b.parseAllConfigs(ctx, objectAPI)
+		}
+	} else {
+		// Old bucket without bucket metadata. Hence we migrate existing settings.
+		err = b.convertLegacyConfigs(ctx, objectAPI, configs)
+	}
+	if err != nil {
 		return b, err
 	}
 
@@ -185,6 +198,11 @@ func loadBucketMetadata(ctx context.Context, objectAPI ObjectLayer, bucket strin
 	return b, nil
 }
 
+// loadBucketMetadata loads and migrates to bucket metadata.
+func loadBucketMetadata(ctx context.Context, objectAPI ObjectLayer, bucket string) (BucketMetadata, error) {
+	return loadBucketMetadataParse(ctx, objectAPI, bucket, true)
+}
+
 // parseAllConfigs will parse all configs and populate the private fields.
 // The first error encountered is returned.
 func (b *BucketMetadata) parseAllConfigs(ctx context.Context, objectAPI ObjectLayer) (err error) {
@@ -277,7 +295,7 @@ func (b *BucketMetadata) parseAllConfigs(ctx context.Context, objectAPI ObjectLa
 	return nil
 }
 
-func (b *BucketMetadata) convertLegacyConfigs(ctx context.Context, objectAPI ObjectLayer) error {
+func (b *BucketMetadata) getAllLegacyConfigs(ctx context.Context, objectAPI ObjectLayer) (map[string][]byte, error) {
 	legacyConfigs := []string{
 		legacyBucketObjectLockEnabledConfigFile,
 		bucketPolicyConfig,
@@ -291,7 +309,7 @@ func (b *BucketMetadata) convertLegacyConfigs(ctx context.Context, objectAPI Obj
 		objectLockConfig,
 	}
 
-	configs := make(map[string][]byte)
+	configs := make(map[string][]byte, len(legacyConfigs))
 
 	// Handle migration from lockEnabled to newer format.
 	if b.LockEnabled {
@@ -316,16 +334,15 @@ func (b *BucketMetadata) convertLegacyConfigs(ctx context.Context, objectAPI Obj
 				continue
 			}
 
-			return err
+			return nil, err
 		}
 		configs[legacyFile] = configData
 	}
 
-	if len(configs) == 0 {
-		// nothing to update, return right away.
-		return b.parseAllConfigs(ctx, objectAPI)
-	}
+	return configs, nil
+}
 
+func (b *BucketMetadata) convertLegacyConfigs(ctx context.Context, objectAPI ObjectLayer, configs map[string][]byte) error {
 	for legacyFile, configData := range configs {
 		switch legacyFile {
 		case legacyBucketObjectLockEnabledConfigFile:
diff --git a/cmd/bucket-policy-handlers.go b/cmd/bucket-policy-handlers.go
index 3b6b5c3df401c..c1b2cd18598e4 100644
--- a/cmd/bucket-policy-handlers.go
+++ b/cmd/bucket-policy-handlers.go
@@ -149,7 +149,7 @@ func (api objectAPIHandlers) DeleteBucketPolicyHandler(w http.ResponseWriter, r
 		return
 	}
 
-	updatedAt, err := globalBucketMetadataSys.Update(ctx, bucket, bucketPolicyConfig, nil)
+	updatedAt, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketPolicyConfig)
 	if err != nil {
 		writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
 		return
diff --git a/cmd/bucket-replication-handlers.go b/cmd/bucket-replication-handlers.go
index 68d96f6410768..bb0e8b3327c7e 100644
--- a/cmd/bucket-replication-handlers.go
+++ b/cmd/bucket-replication-handlers.go
@@ -165,7 +165,7 @@ func (api objectAPIHandlers) DeleteBucketReplicationConfigHandler(w http.Respons
 		writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrReplicationDenyEditError), r.URL)
 		return
 	}
-	if _, err := globalBucketMetadataSys.Update(ctx, bucket, bucketReplicationConfig, nil); err != nil {
+	if _, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketReplicationConfig); err != nil {
 		writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
 		return
 	}
diff --git a/cmd/site-replication.go b/cmd/site-replication.go
index 966c2ea537b9a..b52e7ae505f67 100644
--- a/cmd/site-replication.go
+++ b/cmd/site-replication.go
@@ -1361,7 +1361,7 @@ func (c *SiteReplicationSys) PeerBucketPolicyHandler(ctx context.Context, bucket
 	}
 
 	// Delete the bucket policy
-	_, err := globalBucketMetadataSys.Update(ctx, bucket, bucketPolicyConfig, nil)
+	_, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketPolicyConfig)
 	if err != nil {
 		return wrapSRErr(err)
 	}
@@ -1391,7 +1391,7 @@ func (c *SiteReplicationSys) PeerBucketTaggingHandler(ctx context.Context, bucke
 	}
 
 	// Delete the tags
-	_, err := globalBucketMetadataSys.Update(ctx, bucket, bucketTaggingConfig, nil)
+	_, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketTaggingConfig)
 	if err != nil {
 		return wrapSRErr(err)
 	}
@@ -1445,7 +1445,7 @@ func (c *SiteReplicationSys) PeerBucketSSEConfigHandler(ctx context.Context, buc
 	}
 
 	// Delete sse config
-	_, err := globalBucketMetadataSys.Update(ctx, bucket, bucketSSEConfig, nil)
+	_, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketSSEConfig)
 	if err != nil {
 		return wrapSRErr(err)
 	}
@@ -1475,7 +1475,7 @@ func (c *SiteReplicationSys) PeerBucketQuotaConfigHandler(ctx context.Context, b
 	}
 
 	// Delete the bucket policy
-	_, err := globalBucketMetadataSys.Update(ctx, bucket, bucketQuotaConfigFile, nil)
+	_, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketQuotaConfigFile)
 	if err != nil {
 		return wrapSRErr(err)
 	}
@@ -2234,7 +2234,7 @@ func (c *SiteReplicationSys) RemoveRemoteTargetsForEndpoint(ctx context.Context,
 				return err
 			}
 		} else {
-			if _, err := globalBucketMetadataSys.Update(ctx, b.Name, bucketReplicationConfig, nil); err != nil {
+			if _, err := globalBucketMetadataSys.Delete(ctx, b.Name, bucketReplicationConfig); err != nil {
 				return err
 			}
 		}


Test patch: