From ebf7a030cc3ea1fbfeeb07fce39bc69fdce71c18 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Mon, 20 Oct 2025 14:41:33 +0400 Subject: [PATCH] fix: fixes the bucket/object tagging key/value name validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/common.go | 10 +-- s3api/utils/utils.go | 21 ++++- tests/integration/group-tests.go | 4 + tests/integration/tests.go | 150 ++++++++++++++++++++++++++++--- 4 files changed, 165 insertions(+), 20 deletions(-) diff --git a/backend/common.go b/backend/common.go index f3d55f9..9092a17 100644 --- 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)) } diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index 9b2072d..e156fb8 100644 --- 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 { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index afde517..d888904 100644 --- 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, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index 2054375..0259078 100644 --- 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