From b0e6f7a096609aa21d9a4eaa954e3f042d1ade78 Mon Sep 17 00:00:00 2001 From: Wleeaf Date: Sat, 24 Oct 2026 17:04:01 +0300 Subject: [PATCH] s3: allow up to 50 tags in a bucket tag set (#12030) * s3: allow up to 50 tags in a bucket tag set PutBucketTagging validated the tag set with the object tag rules, which allow 10 tags. AWS allows a bucket up to 50 tags, so tag sets of 11 to 50 tags were rejected with InvalidTag. Validate bucket tags against 50 and keep the limit of 10 for objects and S3 Tables. A tag set that repeats a key is rejected with InvalidTag, as AWS does, instead of keeping one of the values, so 51 tags with a repeated key no longer pass as 50. * s3: reject duplicate tag keys in PutObjectTagging The tag set collapses to a map, so a repeated key was silently kept once.\nMatch the bucket tagging check and answer InvalidTag instead. * s3: reject empty tag keys AWS requires a tag key to be at least one character; the shared\nvalidator only checked the 128-char upper bound. * s3: reject duplicate tag keys in the tagging header parser parseTagsHeader (the x-amz-tagging path used by CopyObject with TaggingDirective=REPLACE) silently kept the last value when a key repeated, while the PutObject and multipart header paths and the new XML checks return InvalidTag for the same input. A repeated key after URL decoding now fails the same way. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: validate x-amz-tagging tags on PutObject before storing the entry The header path only rejected duplicate keys, so an upload could store more than ten tags or keys and values that fail S3 tag rules. Run the parsed set through ValidateTags like the XML tagging handlers do. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: validate x-amz-tagging tags on multipart initiate ParseS3Metadata stored header tags with no tag-set validation, the same gap putToFiler had. Multipart uploads now reject oversize or malformed tag sets with InvalidTag. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * s3: count tag length in characters, not bytes * s3: validate object tags before uploading chunks * s3: reuse parseTagsHeader and drop raw header from warnings --------- Co-authored-by: Wleeaf <179125857+wleeaf@users.noreply.github.com> Co-authored-by: user.email Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/s3api/s3_metadata_util.go | 24 ++--- weed/s3api/s3api_bucket_tagging_handlers.go | 9 +- .../s3api_bucket_tagging_handlers_test.go | 91 +++++++++++++++++++ weed/s3api/s3api_object_handlers_put.go | 42 +++++---- weed/s3api/s3api_object_handlers_tagging.go | 6 ++ .../s3api_object_handlers_tagging_test.go | 29 ++++++ weed/s3api/s3tables/utils.go | 17 +++- weed/s3api/tags.go | 11 +++ weed/s3api/tags_test.go | 29 ++++++ 9 files changed, 215 insertions(+), 43 deletions(-) create mode 100644 weed/s3api/s3api_bucket_tagging_handlers_test.go create mode 100644 weed/s3api/s3api_object_handlers_tagging_test.go diff --git a/weed/s3api/s3_metadata_util.go b/weed/s3api/s3_metadata_util.go index bcd62b0e9d..01e0adc194 100644 --- a/weed/s3api/s3_metadata_util.go +++ b/weed/s3api/s3_metadata_util.go @@ -2,7 +2,6 @@ package s3api import ( "net/http" - "net/url" "strings" "github.com/seaweedfs/seaweedfs/weed/glog" @@ -52,25 +51,16 @@ func ParseS3Metadata(r *http.Request, existing map[string][]byte, isReplace bool // Object tagging if tags := r.Header.Get(s3_constants.AmzObjectTagging); tags != "" { - // Use url.ParseQuery for robust parsing and automatic URL decoding - parsedTags, err := url.ParseQuery(tags) + tagSet, err := parseTagsHeader(tags) if err != nil { - // Return proper S3 error instead of silently dropping tags - glog.Warningf("Invalid S3 tag format in header '%s': %v", tags, err) + glog.Warningf("Invalid S3 tagging header: %v", err) return nil, s3err.ErrInvalidTag } - - // Validate: S3 spec does not allow duplicate tag keys - for key, values := range parsedTags { - if len(values) > 1 { - glog.Warningf("Duplicate tag key '%s' in header '%s'", key, tags) - return nil, s3err.ErrInvalidTag - } - // Tag value can be an empty string but not nil - value := "" - if len(values) > 0 { - value = values[0] - } + if err := ValidateTags(tagSet); err != nil { + glog.Warningf("Invalid tags (%d): %v", len(tagSet), err) + return nil, s3err.ErrInvalidTag + } + for key, value := range tagSet { metadata[s3_constants.AmzObjectTagging+"-"+key] = []byte(value) } } diff --git a/weed/s3api/s3api_bucket_tagging_handlers.go b/weed/s3api/s3api_bucket_tagging_handlers.go index a1b116fd28..9541f38cb1 100644 --- a/weed/s3api/s3api_bucket_tagging_handlers.go +++ b/weed/s3api/s3api_bucket_tagging_handlers.go @@ -68,9 +68,14 @@ func (s3a *S3ApiServer) PutBucketTaggingHandler(w http.ResponseWriter, r *http.R } tags := tagging.ToTags() + // A key may appear only once in a tag set; the map keeps one of them. + if len(tags) != len(tagging.TagSet.Tag) { + glog.Errorf("PutBucketTagging duplicate tag keys %s", r.URL) + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidTag) + return + } - // Validate tags using existing validation - err = ValidateTags(tags) + err = ValidateBucketTags(tags) if err != nil { glog.Errorf("PutBucketTagging ValidateTags error %s: %v", r.URL, err) s3err.WriteErrorResponse(w, r, s3err.ErrInvalidTag) diff --git a/weed/s3api/s3api_bucket_tagging_handlers_test.go b/weed/s3api/s3api_bucket_tagging_handlers_test.go new file mode 100644 index 0000000000..a1cc26b165 --- /dev/null +++ b/weed/s3api/s3api_bucket_tagging_handlers_test.go @@ -0,0 +1,91 @@ +package s3api + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gorilla/mux" + "github.com/seaweedfs/seaweedfs/weed/pb/s3_pb" + "google.golang.org/protobuf/proto" +) + +// A bucket's tag set holds up to 50 tags, unlike an object's 10, and no key +// twice. +func TestPutBucketTaggingTagCountLimit(t *testing.T) { + const bucket = "tag-limit-bucket" + for _, tc := range []struct { + tags int + repeatKey bool // the last tag repeats the first tag's key + emptyKey bool // the first tag has an empty key + wantCode int + }{ + {tags: 10, wantCode: http.StatusOK}, + {tags: 11, wantCode: http.StatusOK}, + {tags: 50, wantCode: http.StatusOK}, + {tags: 51, wantCode: http.StatusBadRequest}, + {tags: 2, repeatKey: true, wantCode: http.StatusBadRequest}, + {tags: 51, repeatKey: true, wantCode: http.StatusBadRequest}, + {tags: 1, emptyKey: true, wantCode: http.StatusBadRequest}, + } { + name := fmt.Sprintf("%d tags", tc.tags) + if tc.repeatKey { + name += " with a repeated key" + } + if tc.emptyKey { + name += " with an empty key" + } + t.Run(name, func(t *testing.T) { + filer := &fakeTxnFiler{} + s3a := newHeadBucketTestServer(t, filer) + s3a.iam = &IdentityAccessManagement{isAuthEnabled: true} + s3a.bucketConfigCache = NewBucketConfigCache(time.Minute) + s3a.bucketConfigCache.Set(bucket, &BucketConfig{Name: bucket}) + + var body strings.Builder + body.WriteString("") + for i := 0; i < tc.tags; i++ { + key := fmt.Sprintf("key%d", i) + if tc.repeatKey && i == tc.tags-1 { + key = "key0" + } + if tc.emptyKey && i == 0 { + key = "" + } + fmt.Fprintf(&body, "%svalue%d", key, i) + } + body.WriteString("") + + req := httptest.NewRequest(http.MethodPut, "/"+bucket+"?tagging", strings.NewReader(body.String())) + req = mux.SetURLVars(req, map[string]string{"bucket": bucket}) + rr := httptest.NewRecorder() + s3a.PutBucketTaggingHandler(rr, req) + + if rr.Code != tc.wantCode { + t.Fatalf("status = %d, want %d; body: %s", rr.Code, tc.wantCode, rr.Body.String()) + } + if tc.wantCode != http.StatusOK { + if !strings.Contains(rr.Body.String(), "InvalidTag") { + t.Fatalf("body = %s, want InvalidTag", rr.Body.String()) + } + if filer.lastReq != nil { + t.Fatalf("rejected tag set was written to the filer") + } + return + } + if filer.lastReq == nil || len(filer.lastReq.Mutations) != 1 { + t.Fatalf("expected one bucket metadata write, got %+v", filer.lastReq) + } + var stored s3_pb.BucketMetadata + if err := proto.Unmarshal(filer.lastReq.Mutations[0].Content, &stored); err != nil { + t.Fatalf("decode stored bucket metadata: %v", err) + } + if len(stored.Tags) != tc.tags { + t.Fatalf("stored %d tags, want %d", len(stored.Tags), tc.tags) + } + }) + } +} diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index b38a205f16..aa6682712a 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -527,6 +527,22 @@ func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader if !s3_constants.IsValidBucketName(bucket) || (object != "" && !s3_constants.IsValidObjectKey(object)) { return "", s3err.ErrInvalidRequest, SSEResponseMetadata{} } + + // Reject invalid tags before any chunks are uploaded so a failed request + // leaves nothing behind on the volume servers. + var tagSet map[string]string + if tagging := r.Header.Get(s3_constants.AmzObjectTagging); tagging != "" { + parsed, err := parseTagsHeader(tagging) + if err != nil { + glog.Warningf("putToFiler: invalid tagging header for %s: %v", filePath, err) + return "", s3err.ErrInvalidTag, SSEResponseMetadata{} + } + if err := ValidateTags(parsed); err != nil { + glog.Warningf("putToFiler: invalid tags (%d) for %s: %v", len(parsed), filePath, err) + return "", s3err.ErrInvalidTag, SSEResponseMetadata{} + } + tagSet = parsed + } // NEW OPTIMIZATION: Write directly to volume servers, bypassing filer proxy // This eliminates the filer proxy overhead for PUT operations // Note: filePath is now passed directly instead of URL (no parsing needed) @@ -938,26 +954,12 @@ func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader entry.Extended[s3_constants.AmzStorageClass] = []byte(sc) } - // Parse and store object tags from X-Amz-Tagging header - // Fix for GitHub issue #7589: Tags sent during object upload were not being stored - if tagging := r.Header.Get(s3_constants.AmzObjectTagging); tagging != "" { - parsedTags, err := url.ParseQuery(tagging) - if err != nil { - glog.Warningf("putToFiler: Invalid S3 tag format in header '%s': %v", tagging, err) - return "", s3err.ErrInvalidTag, SSEResponseMetadata{} - } - for key, values := range parsedTags { - if len(values) > 1 { - glog.Warningf("putToFiler: Duplicate tag key '%s' in header", key) - return "", s3err.ErrInvalidTag, SSEResponseMetadata{} - } - value := "" - if len(values) > 0 { - value = values[0] - } - entry.Extended[s3_constants.AmzObjectTagging+"-"+key] = []byte(value) - } - glog.V(3).Infof("putToFiler: stored %d tags from X-Amz-Tagging header", len(parsedTags)) + // Store the tags validated before the chunk upload + for key, value := range tagSet { + entry.Extended[s3_constants.AmzObjectTagging+"-"+key] = []byte(value) + } + if len(tagSet) > 0 { + glog.V(3).Infof("putToFiler: stored %d tags from X-Amz-Tagging header", len(tagSet)) } storeSSEMetadata(entry, sseResult) diff --git a/weed/s3api/s3api_object_handlers_tagging.go b/weed/s3api/s3api_object_handlers_tagging.go index 466d8bcfd4..c55b38c09c 100644 --- a/weed/s3api/s3api_object_handlers_tagging.go +++ b/weed/s3api/s3api_object_handlers_tagging.go @@ -139,6 +139,12 @@ func (s3a *S3ApiServer) PutObjectTaggingHandler(w http.ResponseWriter, r *http.R return } tags := tagging.ToTags() + // A key may appear only once in a tag set; the map keeps one of them. + if len(tags) != len(tagging.TagSet.Tag) { + glog.Errorf("PutObjectTaggingHandler duplicate tag keys %s", r.URL) + s3err.WriteErrorResponse(w, r, s3err.ErrInvalidTag) + return + } err = ValidateTags(tags) if err != nil { glog.Errorf("PutObjectTaggingHandler ValidateTags error %s: %v", r.URL, err) diff --git a/weed/s3api/s3api_object_handlers_tagging_test.go b/weed/s3api/s3api_object_handlers_tagging_test.go new file mode 100644 index 0000000000..05dec5a833 --- /dev/null +++ b/weed/s3api/s3api_object_handlers_tagging_test.go @@ -0,0 +1,29 @@ +package s3api + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gorilla/mux" +) + +func TestPutObjectTaggingRejectsDuplicateKeys(t *testing.T) { + s3a := newHeadBucketTestServer(t, &fakeTxnFiler{}) + body := "" + + "a1" + + "a2" + + "" + req := httptest.NewRequest(http.MethodPut, "/b/o?tagging", strings.NewReader(body)) + req = mux.SetURLVars(req, map[string]string{"bucket": "b", "object": "o"}) + rr := httptest.NewRecorder() + s3a.PutObjectTaggingHandler(rr, req) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body: %s", rr.Code, http.StatusBadRequest, rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), "InvalidTag") { + t.Fatalf("body = %s, want InvalidTag", rr.Body.String()) + } +} diff --git a/weed/s3api/s3tables/utils.go b/weed/s3api/s3tables/utils.go index d83e498830..8c76b189af 100644 --- a/weed/s3api/s3tables/utils.go +++ b/weed/s3api/s3tables/utils.go @@ -9,6 +9,7 @@ import ( "regexp" "strings" "time" + "unicode/utf8" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" ) @@ -372,17 +373,25 @@ func arnPartitionForRegion(region string) string { // ValidateTags validates tags for S3 Tables. func ValidateTags(tags map[string]string) error { - if len(tags) > 10 { - return fmt.Errorf("validate tags: %d tags more than 10", len(tags)) + return ValidateTagsWithLimit(tags, 10) +} + +// ValidateTagsWithLimit validates a tag set that may hold up to maxTags tags. +func ValidateTagsWithLimit(tags map[string]string, maxTags int) error { + if len(tags) > maxTags { + return fmt.Errorf("validate tags: %d tags more than %d", len(tags), maxTags) } for k, v := range tags { - if len(k) > 128 { + if len(k) == 0 { + return fmt.Errorf("validate tags: empty tag key") + } + if utf8.RuneCountInString(k) > 128 { return fmt.Errorf("validate tags: tag key longer than 128") } if !tagPattern.MatchString(k) { return fmt.Errorf("validate tags key %s error, incorrect key", k) } - if len(v) > 256 { + if utf8.RuneCountInString(v) > 256 { return fmt.Errorf("validate tags: tag value longer than 256") } if !tagPattern.MatchString(v) { diff --git a/weed/s3api/tags.go b/weed/s3api/tags.go index 76830ccf14..49cde79f59 100644 --- a/weed/s3api/tags.go +++ b/weed/s3api/tags.go @@ -63,6 +63,9 @@ func parseTagsHeader(tags string) (map[string]string, error) { if err != nil { return nil, fmt.Errorf("failed to decode tag key '%s': %w", key, err) } + if _, exists := parsedTags[decodedKey]; exists { + return nil, fmt.Errorf("duplicate tag key '%s'", decodedKey) + } if !hasValue { parsedTags[decodedKey] = "" continue @@ -79,3 +82,11 @@ func parseTagsHeader(tags string) (map[string]string, error) { func ValidateTags(tags map[string]string) error { return s3tables.ValidateTags(tags) } + +// maxBucketTags is how many tags a bucket's tag set can hold; an object's +// tag set holds 10. +const maxBucketTags = 50 + +func ValidateBucketTags(tags map[string]string) error { + return s3tables.ValidateTagsWithLimit(tags, maxBucketTags) +} diff --git a/weed/s3api/tags_test.go b/weed/s3api/tags_test.go index a7dd1b806e..f313806ae5 100644 --- a/weed/s3api/tags_test.go +++ b/weed/s3api/tags_test.go @@ -1,6 +1,7 @@ package s3api import ( + "strings" "testing" ) @@ -62,6 +63,18 @@ func TestParseTagsHeader(t *testing.T) { expected: nil, expectError: true, }, + { + name: "duplicate key", + input: "key1=value1&key1=value2", + expected: nil, + expectError: true, + }, + { + name: "encoded duplicate key", + input: "key1=value1&%6bey1=value2", + expected: nil, + expectError: true, + }, { name: "plus signs and equals in values", input: "formula=a%2Bb%3Dc&normal=test", @@ -120,3 +133,19 @@ func TestParseTagsHeader(t *testing.T) { }) } } + +func TestValidateTagsUnicodeLength(t *testing.T) { + // S3 counts tag length in characters, not bytes + multibyteKey := strings.Repeat("中", 128) + multibyteValue := strings.Repeat("中", 256) + + if err := ValidateTags(map[string]string{multibyteKey: multibyteValue}); err != nil { + t.Errorf("Expected 128-char key and 256-char value to pass, got %v", err) + } + if err := ValidateTags(map[string]string{strings.Repeat("中", 129): "v"}); err == nil { + t.Error("Expected 129-char key to fail") + } + if err := ValidateTags(map[string]string{"k": strings.Repeat("中", 257)}); err == nil { + t.Error("Expected 257-char value to fail") + } +}