fix: fixes the bucket/object tagging key/value name validation

Fixes #1579 S3 enforces a specific rule for validating bucket and object tag key/value names. This PR integrates the regexp pattern used by S3 for tag validation. Official S3 documentation for tag validation rules: [AWS S3 Tag](https://docs.aws.amazon.com/AmazonS3/latest/API/API_control_Tag.html) There are two types of tagging inputs for buckets and objects: 1. **On existing buckets/objects** — used in the `PutObjectTagging` and `PutBucketTagging` actions, where tags are provided in the request body. 2. **On object creation** — used in the `PutObject`, `CreateMultipartUpload`, and `CopyObject` actions, where tags are provided in the request headers and must be URL-encoded. This implementation ensures correct validation for both types of tag inputs.
2026-04-19 12:15:02 +00:00 · 2025-10-20 14:41:33 +04:00
parent 2bf4ccd244
commit ebf7a030cc
4 changed files with 165 additions and 20 deletions
--- a/backend/common.go
+++ b/backend/common.go
@@ -317,14 +317,12 @@ func ParseObjectTags(tagging string) (map[string]string, error) {
 	return tagSet, nil
 }

-var validTagComponent = regexp.MustCompile(`^[a-zA-Z0-9:/_.\-+ ]+$`)
+// tag component (key/value) name rule regexp
+// https://docs.aws.amazon.com/AmazonS3/latest/API/API_control_Tag.html
+var validTagComponent = regexp.MustCompile(`^([\p{L}\p{Z}\p{N}_.:/=+\-@]*)$`)

-// isValidTagComponent matches strings which contain letters, decimal digits,
-// and special chars: '/', '_', '-', '+', '.', ' ' (space)
+// isValidTagComponent validates the tag component(key/value) name
 func isValidTagComponent(str string) bool {
-	if str == "" {
-		return true
-	}
 	return validTagComponent.Match([]byte(str))
 }

--- a/s3api/utils/utils.go
+++ b/s3api/utils/utils.go
@@ -658,6 +658,11 @@ const (
 	TagLimitObject TagLimit = 10
 )

+// The tag key/value validation pattern comes from
+// AWS S3 docs
+// https://docs.aws.amazon.com/AmazonS3/latest/API/API_control_Tag.html
+var tagRule = regexp.MustCompile(`^([\p{L}\p{Z}\p{N}_.:/=+\-@]*)$`)
+
 // Parses and validates tagging
 func ParseTagging(data []byte, limit TagLimit) (map[string]string, error) {
 	var tagging s3response.TaggingInput
@@ -682,18 +687,30 @@ func ParseTagging(data []byte, limit TagLimit) (map[string]string, error) {
 	tagSet := make(map[string]string, tLen)

 	for _, tag := range tagging.TagSet.Tags {
-		// validate tag key
+		// validate tag key length
 		if len(tag.Key) == 0 || len(tag.Key) > 128 {
 			debuglogger.Logf("tag key should 0 < tag.Key <= 128, key: %v", tag.Key)
 			return nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)
 		}

-		// validate tag value
+		// validate tag key string chars
+		if !tagRule.MatchString(tag.Key) {
+			debuglogger.Logf("invalid tag key: %s", tag.Key)
+			return nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)
+		}
+
+		// validate tag value length
 		if len(tag.Value) > 256 {
 			debuglogger.Logf("invalid long tag value: (length): %v, (value): %v", len(tag.Value), tag.Value)
 			return nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)
 		}

+		// validate tag value string chars
+		if !tagRule.MatchString(tag.Value) {
+			debuglogger.Logf("invalid tag value: %s", tag.Value)
+			return nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)
+		}
+
 		// make sure there are no duplicate keys
 		_, ok := tagSet[tag.Key]
 		if ok {
--- a/tests/integration/group-tests.go
+++ b/tests/integration/group-tests.go
@@ -125,6 +125,7 @@ func TestDeleteBucketOwnershipControls(ts *TestState) {
 func TestPutBucketTagging(ts *TestState) {
 	ts.Run(PutBucketTagging_non_existing_bucket)
 	ts.Run(PutBucketTagging_long_tags)
+	ts.Run(PutBucketTagging_invalid_tags)
 	ts.Run(PutBucketTagging_duplicate_keys)
 	ts.Run(PutBucketTagging_tag_count_limit)
 	ts.Run(PutBucketTagging_success)
@@ -337,6 +338,7 @@ func TestPutObjectTagging(ts *TestState) {
 	ts.Run(PutObjectTagging_long_tags)
 	ts.Run(PutObjectTagging_duplicate_keys)
 	ts.Run(PutObjectTagging_tag_count_limit)
+	ts.Run(PutObjectTagging_invalid_tags)
 	ts.Run(PutObjectTagging_success)
 }

@@ -1146,6 +1148,7 @@ func GetIntTests() IntTests {
 		"DeleteBucketOwnershipControls_success":                                   DeleteBucketOwnershipControls_success,
 		"PutBucketTagging_non_existing_bucket":                                    PutBucketTagging_non_existing_bucket,
 		"PutBucketTagging_long_tags":                                              PutBucketTagging_long_tags,
+		"PutBucketTagging_invalid_tags":                                           PutBucketTagging_invalid_tags,
 		"PutBucketTagging_duplicate_keys":                                         PutBucketTagging_duplicate_keys,
 		"PutBucketTagging_tag_count_limit":                                        PutBucketTagging_tag_count_limit,
 		"PutBucketTagging_success":                                                PutBucketTagging_success,
@@ -1277,6 +1280,7 @@ func GetIntTests() IntTests {
 		"PutObjectTagging_long_tags":                                              PutObjectTagging_long_tags,
 		"PutObjectTagging_duplicate_keys":                                         PutObjectTagging_duplicate_keys,
 		"PutObjectTagging_tag_count_limit":                                        PutObjectTagging_tag_count_limit,
+		"PutObjectTagging_invalid_tags":                                           PutObjectTagging_invalid_tags,
 		"PutObjectTagging_success":                                                PutObjectTagging_success,
 		"GetObjectTagging_non_existing_object":                                    GetObjectTagging_non_existing_object,
 		"GetObjectTagging_unset_tags":                                             GetObjectTagging_unset_tags,
--- a/tests/integration/tests.go
+++ b/tests/integration/tests.go
@@ -2573,6 +2573,58 @@ func PutBucketTagging_long_tags(s *S3Conf) error {
 	})
 }

+func PutBucketTagging_invalid_tags(s *S3Conf) error {
+	testName := "PutBucketTagging_invalid_tags"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		for i, test := range []struct {
+			tags []types.Tag
+			err  s3err.APIError
+		}{
+			// invalid tag key tests
+			{[]types.Tag{{Key: getPtr("user!name"), Value: getPtr("value")}}, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
+			{[]types.Tag{{Key: getPtr("foo#bar"), Value: getPtr("value")}}, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
+			{[]types.Tag{
+				{Key: getPtr("validkey"), Value: getPtr("validvalue")},
+				{Key: getPtr("data%20"), Value: getPtr("value")},
+			}, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
+			{[]types.Tag{
+				{Key: getPtr("abcd"), Value: getPtr("xyz123")},
+				{Key: getPtr("a*b"), Value: getPtr("value")},
+			}, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
+			// invalid tag value tests
+			{[]types.Tag{
+				{Key: getPtr("hello"), Value: getPtr("world")},
+				{Key: getPtr("key"), Value: getPtr("name?test")},
+			}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+			{[]types.Tag{
+				{Key: getPtr("foo"), Value: getPtr("bar")},
+				{Key: getPtr("key"), Value: getPtr("`path")},
+			}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+			{[]types.Tag{{Key: getPtr("valid"), Value: getPtr("comma,separated")}}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+			{[]types.Tag{{Key: getPtr("valid"), Value: getPtr("semicolon;test")}}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+			{[]types.Tag{{Key: getPtr("valid"), Value: getPtr("(parentheses)")}}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+		} {
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			_, err := s3client.PutBucketTagging(ctx, &s3.PutBucketTaggingInput{
+				Bucket: &bucket,
+				Tagging: &types.Tagging{
+					TagSet: test.tags,
+				},
+			})
+			cancel()
+			if err == nil {
+				return fmt.Errorf("test %v failed: expected err %w, instead got nil", i+1, test.err)
+			}
+
+			if err := checkApiErr(err, test.err); err != nil {
+				return fmt.Errorf("test %v failed: %w", i+1, err)
+			}
+		}
+
+		return nil
+	})
+}
+
 func PutBucketTagging_duplicate_keys(s *S3Conf) error {
 	testName := "PutBucketTagging_duplicate_keys"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -2935,7 +2987,7 @@ func PutObject_tagging(s *S3Conf) error {
 			return nil
 		}

-		for _, el := range []struct {
+		for i, el := range []struct {
 			tagging     string
 			result      map[string]string
 			expectedErr error
@@ -2949,6 +3001,7 @@ func PutObject_tagging(s *S3Conf) error {
 			{"key=val&", map[string]string{"key": "val"}, nil},
 			{"key1&key2", map[string]string{"key1": "", "key2": ""}, nil},
 			{"key1=val1&key2=val2", map[string]string{"key1": "val1", "key2": "val2"}, nil},
+			{"key@=val@", map[string]string{"key@": "val@"}, nil},
 			// invalid url-encoded
 			{"=", nil, s3err.GetAPIError(s3err.ErrInvalidURLEncodedTagging)},
 			{"key%", nil, s3err.GetAPIError(s3err.ErrInvalidURLEncodedTagging)},
@@ -2960,7 +3013,6 @@ func PutObject_tagging(s *S3Conf) error {
 			{"key*=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key$=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key#=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
-			{"key@=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key!=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			// invalid tag values
 			{"key=val?", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
@@ -2968,7 +3020,6 @@ func PutObject_tagging(s *S3Conf) error {
 			{"key=val*", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val$", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val#", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
-			{"key=val@", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val!", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			// success special chars
 			{"key-key_key.key/key=value-value_value.value/value", map[string]string{"key-key_key.key/key": "value-value_value.value/value"}, nil},
@@ -2979,9 +3030,15 @@ func PutObject_tagging(s *S3Conf) error {
 			{"key%20key=value%20value", map[string]string{"key key": "value value"}, nil},
 			{"key%5Fkey=value%5Fvalue", map[string]string{"key_key": "value_value"}, nil},
 		} {
+			if s.azureTests {
+				// azure doesn't support '@' character
+				if strings.Contains(el.tagging, "@") {
+					continue
+				}
+			}
 			err := testTagging(el.tagging, el.result, el.expectedErr)
 			if err != nil {
-				return err
+				return fmt.Errorf("test case %v failed: %w", i+1, err)
 			}
 		}
 		return nil
@@ -7627,7 +7684,7 @@ func CopyObject_should_replace_tagging(s *S3Conf) error {
 			return nil
 		}

-		for _, el := range []struct {
+		for i, el := range []struct {
 			tagging     string
 			result      map[string]string
 			expectedErr error
@@ -7641,6 +7698,7 @@ func CopyObject_should_replace_tagging(s *S3Conf) error {
 			{"key=val&", map[string]string{"key": "val"}, nil},
 			{"key1&key2", map[string]string{"key1": "", "key2": ""}, nil},
 			{"key1=val1&key2=val2", map[string]string{"key1": "val1", "key2": "val2"}, nil},
+			{"key@=val@", map[string]string{"key@": "val@"}, nil},
 			// invalid url-encoded
 			{"=", nil, s3err.GetAPIError(s3err.ErrInvalidURLEncodedTagging)},
 			{"key%", nil, s3err.GetAPIError(s3err.ErrInvalidURLEncodedTagging)},
@@ -7652,7 +7710,6 @@ func CopyObject_should_replace_tagging(s *S3Conf) error {
 			{"key*=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key$=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key#=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
-			{"key@=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key!=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			// invalid tag values
 			{"key=val?", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
@@ -7660,7 +7717,6 @@ func CopyObject_should_replace_tagging(s *S3Conf) error {
 			{"key=val*", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val$", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val#", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
-			{"key=val@", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val!", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			// success special chars
 			{"key-key_key.key/key=value-value_value.value/value",
@@ -7673,9 +7729,15 @@ func CopyObject_should_replace_tagging(s *S3Conf) error {
 			{"key%20key=value%20value", map[string]string{"key key": "value value"}, nil},
 			{"key%5Fkey=value%5Fvalue", map[string]string{"key_key": "value_value"}, nil},
 		} {
+			if s.azureTests {
+				// azure doesn't support '@' character
+				if strings.Contains(el.tagging, "@") {
+					continue
+				}
+			}
 			err := testTagging(el.tagging, el.result, el.expectedErr)
 			if err != nil {
-				return err
+				return fmt.Errorf("test case %v failed: %w", i+1, err)
 			}
 		}
 		return nil
@@ -8794,6 +8856,65 @@ func PutObjectTagging_tag_count_limit(s *S3Conf) error {
 	})
 }

+func PutObjectTagging_invalid_tags(s *S3Conf) error {
+	testName := "PutObjectTagging_invalid_tags"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-object"
+		_, err := putObjects(s3client, []string{obj}, bucket)
+		if err != nil {
+			return err
+		}
+
+		for i, test := range []struct {
+			tags []types.Tag
+			err  s3err.APIError
+		}{
+			// invalid tag key tests
+			{[]types.Tag{{Key: getPtr("user!name"), Value: getPtr("value")}}, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
+			{[]types.Tag{{Key: getPtr("foo#bar"), Value: getPtr("value")}}, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
+			{[]types.Tag{
+				{Key: getPtr("validkey"), Value: getPtr("validvalue")},
+				{Key: getPtr("data%20"), Value: getPtr("value")},
+			}, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
+			{[]types.Tag{
+				{Key: getPtr("abcd"), Value: getPtr("xyz123")},
+				{Key: getPtr("a*b"), Value: getPtr("value")},
+			}, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
+			// invalid tag value tests
+			{[]types.Tag{
+				{Key: getPtr("hello"), Value: getPtr("world")},
+				{Key: getPtr("key"), Value: getPtr("name?test")},
+			}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+			{[]types.Tag{
+				{Key: getPtr("foo"), Value: getPtr("bar")},
+				{Key: getPtr("key"), Value: getPtr("`path")},
+			}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+			{[]types.Tag{{Key: getPtr("valid"), Value: getPtr("comma,separated")}}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+			{[]types.Tag{{Key: getPtr("valid"), Value: getPtr("semicolon;test")}}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+			{[]types.Tag{{Key: getPtr("valid"), Value: getPtr("(parentheses)")}}, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
+		} {
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			_, err := s3client.PutObjectTagging(ctx, &s3.PutObjectTaggingInput{
+				Bucket: &bucket,
+				Key:    &obj,
+				Tagging: &types.Tagging{
+					TagSet: test.tags,
+				},
+			})
+			cancel()
+			if err == nil {
+				return fmt.Errorf("test %v failed: expected err %w, instead got nil", i+1, test.err)
+			}
+
+			if err := checkApiErr(err, test.err); err != nil {
+				return fmt.Errorf("test %v failed: %w", i+1, err)
+			}
+		}
+
+		return nil
+	})
+}
+
 func PutObjectTagging_success(s *S3Conf) error {
 	testName := "PutObjectTagging_success"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -9561,7 +9682,7 @@ func CreateMultipartUpload_with_tagging(s *S3Conf) error {
 			return nil
 		}

-		for _, el := range []struct {
+		for i, el := range []struct {
 			tagging     string
 			result      map[string]string
 			expectedErr error
@@ -9575,6 +9696,7 @@ func CreateMultipartUpload_with_tagging(s *S3Conf) error {
 			{"key=val&", map[string]string{"key": "val"}, nil},
 			{"key1&key2", map[string]string{"key1": "", "key2": ""}, nil},
 			{"key1=val1&key2=val2", map[string]string{"key1": "val1", "key2": "val2"}, nil},
+			{"key@=val@", map[string]string{"key@": "val@"}, nil},
 			// invalid url-encoded
 			{"=", nil, s3err.GetAPIError(s3err.ErrInvalidURLEncodedTagging)},
 			{"key%", nil, s3err.GetAPIError(s3err.ErrInvalidURLEncodedTagging)},
@@ -9586,7 +9708,6 @@ func CreateMultipartUpload_with_tagging(s *S3Conf) error {
 			{"key*=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key$=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key#=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
-			{"key@=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			{"key!=val", nil, s3err.GetAPIError(s3err.ErrInvalidTagKey)},
 			// invalid tag values
 			{"key=val?", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
@@ -9594,7 +9715,6 @@ func CreateMultipartUpload_with_tagging(s *S3Conf) error {
 			{"key=val*", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val$", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val#", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
-			{"key=val@", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			{"key=val!", nil, s3err.GetAPIError(s3err.ErrInvalidTagValue)},
 			// success special chars
 			{"key-key_key.key/key=value-value_value.value/value",
@@ -9607,9 +9727,15 @@ func CreateMultipartUpload_with_tagging(s *S3Conf) error {
 			{"key%20key=value%20value", map[string]string{"key key": "value value"}, nil},
 			{"key%5Fkey=value%5Fvalue", map[string]string{"key_key": "value_value"}, nil},
 		} {
+			if s.azureTests {
+				// azure doesn't support '@' character
+				if strings.Contains(el.tagging, "@") {
+					continue
+				}
+			}
 			err := testTagging(el.tagging, el.result, el.expectedErr)
 			if err != nil {
-				return err
+				return fmt.Errorf("test case %v faild: %w", i+1, err)
 			}
 		}
 		return nil