From b5409ca11271c62dfed483f6708fab2445140953 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sat, 5 Sep 2026 15:38:33 +0800 Subject: [PATCH 1/2] fix: report logical part sizes in GetObjectAttributes GetObjectAttributes filled ObjectPart.Size from the on-disk part length (cmd/object-handlers.go:715), so every compressed or encrypted multipart object reported transformed sizes that do not sum to the logical ObjectSize the same response returns from objInfo.GetActualSize(). Report each part's uploaded plaintext length instead: a compressed part uses its recorded ActualSize, and a separately encrypted part derives the plaintext length with sio.DecryptedSize, because ActualSize is the ciphertext length for a replicated SSE-C part and zero for parts written before actualSize existed. Only parts of an encrypted multipart object are streams of their own. A legacy encrypted object carries no multipart marker and is one continuous stream that the erasure writer split into storage fragments, so those fragments keep their stored size. Where a part is a stream, one whose length cannot be a valid encrypted stream has no logical length, and the request now fails with XMinioObjectTampered rather than reporting the ciphertext length; DecryptObjectInfo does not catch that case, because ObjectInfo.isMultipart gives up on the first bad part and only the object total is then validated. Tests: TestAPIGetObjectAttributesMultipartLogicalPartSize (plain, compressed, SSE-C and compressed+SSE-C, consecutive and sparse part numbers), TestAPIGetObjectAttributesCompressedEmptyTrailingPart, TestAPIGetObjectAttributesEncryptedPartLengths, and a part-size assertion added to TestAPISSECMultipartReplicationTrust. Compatibility: the XML shape is unchanged and nothing is written to disk, only the value of the existing Size element is corrected. Fixes pgsty/silo#114 Signed-off-by: Feng Ruohang Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01L7qJqWwy8oFA6aCXWRzXQe --- cmd/object-attributes-parts_test.go | 389 ++++++++++++++++++++++++++++ cmd/object-handlers.go | 43 ++- cmd/replication-trust_test.go | 19 ++ 3 files changed, 450 insertions(+), 1 deletion(-) create mode 100644 cmd/object-attributes-parts_test.go diff --git a/cmd/object-attributes-parts_test.go b/cmd/object-attributes-parts_test.go new file mode 100644 index 000000000..61fdb8a53 --- /dev/null +++ b/cmd/object-attributes-parts_test.go @@ -0,0 +1,389 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// Copyright (c) 2026 PGSTY +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "bytes" + "crypto/md5" + "encoding/base64" + "encoding/xml" + "fmt" + "maps" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/minio/minio/internal/auth" + "github.com/minio/minio/internal/crypto" + "github.com/minio/minio/internal/hash" + xhttp "github.com/minio/minio/internal/http" +) + +// attributesPartsResponse is the subset of the GetObjectAttributes response +// the ObjectParts tests assert on. +type attributesPartsResponse struct { + ObjectSize int64 + ObjectParts struct { + IsTruncated bool + NextPartNumberMarker int + PartsCount int + Parts []struct { + PartNumber int + Size int64 + } `xml:"Part"` + } +} + +func attributesPartsSSECHeaders(key []byte) map[string]string { + digest := md5.Sum(key) + return map[string]string{ + xhttp.AmzServerSideEncryptionCustomerAlgorithm: xhttp.AmzEncryptionAES, + xhttp.AmzServerSideEncryptionCustomerKey: base64.StdEncoding.EncodeToString(key), + xhttp.AmzServerSideEncryptionCustomerKeyMD5: base64.StdEncoding.EncodeToString(digest[:]), + } +} + +func attributesPartsSignedRequest(t *testing.T, apiRouter http.Handler, credentials auth.Credentials, + method, target string, body []byte, headers map[string]string, +) *httptest.ResponseRecorder { + t.Helper() + req, err := newTestSignedRequestV4(method, target, int64(len(body)), bytes.NewReader(body), + credentials.AccessKey, credentials.SecretKey, headers) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + apiRouter.ServeHTTP(rec, req) + return rec +} + +// attributesPartsUpload completes a multipart upload of bodies under +// partNumbers and returns the concatenated plaintext. +func attributesPartsUpload(t *testing.T, apiRouter http.Handler, credentials auth.Credentials, + bucketName, object string, headers map[string]string, bodies [][]byte, partNumbers []int, +) []byte { + t.Helper() + initRec := attributesPartsSignedRequest(t, apiRouter, credentials, http.MethodPost, + getNewMultipartURL("", bucketName, object), nil, headers) + if initRec.Code != http.StatusOK { + t.Fatalf("NewMultipart %s: %d %s", object, initRec.Code, initRec.Body.String()) + } + var initiated struct { + UploadID string `xml:"UploadId"` + } + if err := xml.Unmarshal(initRec.Body.Bytes(), &initiated); err != nil { + t.Fatal(err) + } + + var complete bytes.Buffer + complete.WriteString("") + var data []byte + for i, body := range bodies { + data = append(data, body...) + put := attributesPartsSignedRequest(t, apiRouter, credentials, http.MethodPut, + getPutObjectPartURL("", bucketName, object, initiated.UploadID, strconv.Itoa(partNumbers[i])), body, headers) + if put.Code != http.StatusOK { + t.Fatalf("PutObjectPart %s part %d: %d %s", object, partNumbers[i], put.Code, put.Body.String()) + } + fmt.Fprintf(&complete, "%d%s", + partNumbers[i], put.Header()[xhttp.ETag][0]) + } + complete.WriteString("") + + finish := attributesPartsSignedRequest(t, apiRouter, credentials, http.MethodPost, + getCompleteMultipartUploadURL("", bucketName, object, initiated.UploadID), complete.Bytes(), headers) + if finish.Code != http.StatusOK { + t.Fatalf("CompleteMultipartUpload %s: %d %s", object, finish.Code, finish.Body.String()) + } + return data +} + +// attributesPartsFetch issues GetObjectAttributes for ObjectSize and +// ObjectParts and decodes the response. +func attributesPartsFetch(t *testing.T, apiRouter http.Handler, credentials auth.Credentials, + bucketName, object string, headers map[string]string, +) attributesPartsResponse { + t.Helper() + attributeHeaders := maps.Clone(headers) + if attributeHeaders == nil { + attributeHeaders = make(map[string]string) + } + attributeHeaders[xhttp.AmzObjectAttributes] = "ObjectSize,ObjectParts" + rec := attributesPartsSignedRequest(t, apiRouter, credentials, http.MethodGet, + getGetObjectURL("", bucketName, object)+"?attributes", nil, attributeHeaders) + if rec.Code != http.StatusOK { + t.Fatalf("GetObjectAttributes %s: %d %s", object, rec.Code, rec.Body.String()) + } + var response attributesPartsResponse + if err := xml.Unmarshal(rec.Body.Bytes(), &response); err != nil { + t.Fatalf("decode GetObjectAttributes response: %v (%s)", err, rec.Body.String()) + } + return response +} + +// enableAttributesPartsCompression turns on compression for ".txt" objects, +// including encrypted ones, for the duration of the test. +func enableAttributesPartsCompression(t *testing.T) { + t.Helper() + globalCompressConfigMu.Lock() + previous := globalCompressConfig + globalCompressConfig.Enabled = true + globalCompressConfig.Extensions = []string{".txt"} + globalCompressConfig.MimeTypes = nil + globalCompressConfig.AllowEncrypted = true + globalCompressConfigMu.Unlock() + t.Cleanup(func() { + globalCompressConfigMu.Lock() + globalCompressConfig = previous + globalCompressConfigMu.Unlock() + }) +} + +// TestAPIGetObjectAttributesMultipartLogicalPartSize asserts that ObjectPart.Size +// reports the uploaded plaintext length of every part, not the transformed +// length stored on disk. Compressed parts must report the pre-compression +// length and encrypted parts the pre-encryption length, for consecutive as +// well as sparse part numbering. +func TestAPIGetObjectAttributesMultipartLogicalPartSize(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIGetObjectAttributesMultipartLogicalPartSize, + }) +} + +func testAPIGetObjectAttributesMultipartLogicalPartSize(_ ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + previousTLS := globalIsTLS + globalIsTLS = true + defer func() { globalIsTLS = previousTLS }() + enableAttributesPartsCompression(t) + + for _, variant := range []struct { + name string + extension string + encrypted bool + }{ + {name: "plain", extension: ".bin"}, + {name: "compressed", extension: ".txt"}, + {name: "ssec", extension: ".bin", encrypted: true}, + {name: "compressed-ssec", extension: ".txt", encrypted: true}, + } { + for _, numbering := range []struct { + name string + partNumbers []int + }{ + {name: "consecutive", partNumbers: []int{1, 2}}, + {name: "sparse", partNumbers: []int{1, 3}}, + } { + t.Run(variant.name+"/"+numbering.name, func(t *testing.T) { + var headers map[string]string + if variant.encrypted { + headers = attributesPartsSSECHeaders(bytes.Repeat([]byte{0x19}, 32)) + } + object := "attributes/parts-" + variant.name + "-" + numbering.name + variant.extension + bodies := [][]byte{ + bytes.Repeat([]byte("abcd"), 5*1024*1024/4), + bytes.Repeat([]byte("12345"), 103), + } + data := attributesPartsUpload(t, apiRouter, credentials, bucketName, object, headers, bodies, numbering.partNumbers) + + get := attributesPartsSignedRequest(t, apiRouter, credentials, http.MethodGet, + getGetObjectURL("", bucketName, object), nil, headers) + if get.Code != http.StatusOK || !bytes.Equal(get.Body.Bytes(), data) { + t.Fatalf("%s GET: %d bytes=%d want=%d", instanceType, get.Code, get.Body.Len(), len(data)) + } + + response := attributesPartsFetch(t, apiRouter, credentials, bucketName, object, headers) + if response.ObjectSize != int64(len(data)) { + t.Errorf("%s ObjectSize=%d want=%d", instanceType, response.ObjectSize, len(data)) + } + if len(response.ObjectParts.Parts) != len(bodies) { + t.Fatalf("%s part count=%d want=%d", instanceType, len(response.ObjectParts.Parts), len(bodies)) + } + if response.ObjectParts.IsTruncated { + t.Errorf("%s lists all %d parts %v, but IsTruncated=true NextPartNumberMarker=%d", + instanceType, len(bodies), numbering.partNumbers, response.ObjectParts.NextPartNumberMarker) + } + var total int64 + for i, part := range response.ObjectParts.Parts { + if part.PartNumber != numbering.partNumbers[i] { + t.Errorf("%s part %d number=%d want=%d", instanceType, i, part.PartNumber, numbering.partNumbers[i]) + } + if part.Size != int64(len(bodies[i])) { + t.Errorf("%s part %d size=%d want logical size=%d", + instanceType, part.PartNumber, part.Size, len(bodies[i])) + } + total += part.Size + } + if total != response.ObjectSize { + t.Errorf("%s part sizes sum to %d, ObjectSize=%d", instanceType, total, response.ObjectSize) + } + }) + } + } +} + +// TestAPIGetObjectAttributesCompressedEmptyTrailingPart pins the reported +// size of a compressed part carrying no payload. Such a part stores +// Size 0 and ActualSize 0, so it exercises the lower bound of the +// ActualSize guard and must keep reporting 0. +func TestAPIGetObjectAttributesCompressedEmptyTrailingPart(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIGetObjectAttributesCompressedEmptyTrailingPart, + }) +} + +func testAPIGetObjectAttributesCompressedEmptyTrailingPart(_ ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + enableAttributesPartsCompression(t) + + object := "attributes/parts-compressed-empty-tail.txt" + bodies := [][]byte{bytes.Repeat([]byte("abcd"), 5*1024*1024/4), {}} + data := attributesPartsUpload(t, apiRouter, credentials, bucketName, object, nil, bodies, []int{1, 2}) + + response := attributesPartsFetch(t, apiRouter, credentials, bucketName, object, nil) + if response.ObjectSize != int64(len(data)) { + t.Errorf("%s ObjectSize=%d want=%d", instanceType, response.ObjectSize, len(data)) + } + if len(response.ObjectParts.Parts) != len(bodies) { + t.Fatalf("%s part count=%d want=%d", instanceType, len(response.ObjectParts.Parts), len(bodies)) + } + for i, part := range response.ObjectParts.Parts { + if part.Size != int64(len(bodies[i])) { + t.Errorf("%s part %d size=%d want logical size=%d", + instanceType, part.PartNumber, part.Size, len(bodies[i])) + } + } +} + +// TestAPIGetObjectAttributesEncryptedPartLengths pins how a part length that +// cannot be a valid encrypted stream is reported, for both encrypted layouts. +// Parts of an encrypted multipart object are separate streams, so an +// unconvertible one is corrupt and must fail the request. DecryptObjectInfo +// does not catch that: ObjectInfo.isMultipart gives up on the first part that +// fails sio.DecryptedSize, after which ObjectInfo.DecryptedSize validates only +// the object total. A legacy encrypted object carries no multipart marker and +// is one continuous stream that the erasure writer split into storage +// fragments; those fragments are not independently decryptable, so they must +// keep their stored size rather than fail an intact object. Both fixtures use +// part lengths 5245473 and 1, whose sum 5245474 is a valid stream length while +// the second part alone is not. +func TestAPIGetObjectAttributesEncryptedPartLengths(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIGetObjectAttributesEncryptedPartLengths, + }) +} + +func testAPIGetObjectAttributesEncryptedPartLengths(obj ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + ctx := t.Context() + partLengths := []int64{5245473, 1} + + // The marker alone is enough for crypto.IsEncrypted to report an encrypted + // object, which keeps both fixtures free of a sealed key while still + // driving the handler down the encrypted branch. A trusted SSE-C + // replication upload reaches the multipart state with real metadata, + // because it stores whatever part lengths the peer sends. + for _, variant := range []struct { + name string + metadata map[string]string + tampered bool + }{ + { + name: "separately-encrypted-parts", + metadata: map[string]string{crypto.MetaMultipart: ""}, + tampered: true, + }, + { + name: "legacy-single-stream", + metadata: map[string]string{crypto.MetaIV: "legacy"}, + }, + } { + t.Run(variant.name, func(t *testing.T) { + object := "attributes/parts-encrypted-" + variant.name + upload, err := obj.NewMultipartUpload(ctx, bucketName, object, + ObjectOptions{UserDefined: maps.Clone(variant.metadata)}) + if err != nil { + t.Fatal(err) + } + parts := make([]CompletePart, 0, len(partLengths)) + for i, length := range partLengths { + body := bytes.Repeat([]byte("z"), int(length)) + reader, err := hash.NewReader(ctx, bytes.NewReader(body), length, "", "", length) + if err != nil { + t.Fatal(err) + } + info, err := obj.PutObjectPart(ctx, bucketName, object, upload.UploadID, i+1, + NewPutObjReader(reader), ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + parts = append(parts, CompletePart{PartNumber: i + 1, ETag: info.ETag}) + } + if _, err = obj.CompleteMultipartUpload(ctx, bucketName, object, upload.UploadID, parts, ObjectOptions{}); err != nil { + t.Fatal(err) + } + + rec := attributesPartsSignedRequest(t, apiRouter, credentials, http.MethodGet, + getGetObjectURL("", bucketName, object)+"?attributes", nil, + map[string]string{xhttp.AmzObjectAttributes: "ObjectParts"}) + + if variant.tampered { + wantErr := errorCodes.ToAPIErr(ErrObjectTampered) + if rec.Code != wantErr.HTTPStatusCode { + t.Fatalf("%s status %d, want %d: %s", instanceType, rec.Code, wantErr.HTTPStatusCode, rec.Body.String()) + } + var errResp APIErrorResponse + if err = xml.Unmarshal(rec.Body.Bytes(), &errResp); err != nil { + t.Fatalf("decode error response: %v (%s)", err, rec.Body.String()) + } + if errResp.Code != wantErr.Code { + t.Errorf("%s error code %q, want %q", instanceType, errResp.Code, wantErr.Code) + } + return + } + + if rec.Code != http.StatusOK { + t.Fatalf("%s status %d, want %d: %s", instanceType, rec.Code, http.StatusOK, rec.Body.String()) + } + var response attributesPartsResponse + if err = xml.Unmarshal(rec.Body.Bytes(), &response); err != nil { + t.Fatalf("decode GetObjectAttributes response: %v (%s)", err, rec.Body.String()) + } + if len(response.ObjectParts.Parts) != len(partLengths) { + t.Fatalf("%s part count=%d want=%d", instanceType, len(response.ObjectParts.Parts), len(partLengths)) + } + for i, part := range response.ObjectParts.Parts { + if part.Size != partLengths[i] { + t.Errorf("%s part %d size=%d, want the stored fragment size %d", + instanceType, part.PartNumber, part.Size, partLengths[i]) + } + } + }) + } +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index bb2782816..17fa6d838 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -62,6 +62,7 @@ import ( "github.com/minio/minio/internal/logger" "github.com/minio/minio/internal/s3select" "github.com/minio/mux" + "github.com/minio/sio" "github.com/pgsty/silo-pkg/v3/policy" ) @@ -687,6 +688,20 @@ func (api objectAPIHandlers) getObjectAttributesHandler(ctx context.Context, obj objInfo.decryptPartsChecksums(r.Header) if _, ok := opts.ObjectAttributes[xhttp.ObjectParts]; ok { + // Report each part's uploaded plaintext byte length. Parts are stored + // transformed, compressed and/or encrypted, so the stored size is not + // what AWS defines ObjectPart.Size to be. + _, isEncrypted := crypto.IsEncrypted(objInfo.UserDefined) + isCompressed := objInfo.IsCompressed() + // Only a part of an encrypted multipart object is a stream of its + // own. A legacy encrypted object without that marker is one + // continuous stream that the erasure writer split into storage + // fragments, so no fragment has a plaintext length to report and + // each keeps its stored size, as ObjectInfo.DecryptedSize and + // DecryptBlocksRequestR also treat it. + hasEncryptedParts := isEncrypted && + (crypto.IsMultiPart(objInfo.UserDefined) || len(objInfo.Parts) == 1) + OA.ObjectParts = new(objectAttributesParts) OA.ObjectParts.PartNumberMarker = opts.PartNumberMarker @@ -704,6 +719,32 @@ func (api objectAPIHandlers) getObjectAttributesHandler(ctx context.Context, obj break } + partSize := objInfo.Parts[i].Size + switch { + case isCompressed: + // ActualSize is recorded by the same code that compresses, + // so it is always present for a compressed part. + if objInfo.Parts[i].ActualSize >= 0 { + partSize = objInfo.Parts[i].ActualSize + } + case hasEncryptedParts: + // ActualSize cannot be trusted for encrypted parts: a + // replicated SSE-C part records the ciphertext length, and + // parts written before actualSize existed record 0. Derive + // the plaintext length from the ciphertext instead, exactly + // as ObjectInfo.DecryptedSize does. A part whose stored + // length is not a valid encrypted stream has no logical + // length to report, and DecryptObjectInfo above only + // validates the object as a whole in that case, because + // ObjectInfo.isMultipart gives up on the first bad part. + decrypted, err := sio.DecryptedSize(uint64(partSize)) + if err != nil { + writeErrorResponse(ctx, w, toAPIError(ctx, errObjectTampered), r.URL) + return + } + partSize = int64(decrypted) + } + OA.ObjectParts.NextPartNumberMarker = v.Number OA.ObjectParts.Parts = append(OA.ObjectParts.Parts, &objectAttributesPart{ ChecksumSHA1: objInfo.Parts[i].Checksums["SHA1"], @@ -712,7 +753,7 @@ func (api objectAPIHandlers) getObjectAttributesHandler(ctx context.Context, obj ChecksumCRC32C: objInfo.Parts[i].Checksums["CRC32C"], ChecksumCRC64NVME: objInfo.Parts[i].Checksums["CRC64NVME"], PartNumber: objInfo.Parts[i].Number, - Size: objInfo.Parts[i].Size, + Size: partSize, }) } } diff --git a/cmd/replication-trust_test.go b/cmd/replication-trust_test.go index 1d1c309b9..1b0fa9579 100644 --- a/cmd/replication-trust_test.go +++ b/cmd/replication-trust_test.go @@ -829,6 +829,25 @@ func testAPISSECMultipartReplicationTrust(obj ObjectLayer, instanceType, bucketN if !bytes.Equal(getRec.Body.Bytes(), data) { t.Fatal("replicated SSE-C multipart object did not decrypt to source plaintext") } + + // A replicated SSE-C part is written as raw ciphertext, so its stored + // ActualSize is the ciphertext length. GetObjectAttributes must still + // report the part's plaintext length, which for this single-part object + // is the whole object size. + attributes := attributesPartsFetch(t, apiRouter, credentials, bucketName, object, sseHeaders) + if len(attributes.ObjectParts.Parts) != 1 { + t.Fatalf("replica attributes part count %d, want 1", len(attributes.ObjectParts.Parts)) + } + if got := attributes.ObjectParts.Parts[0].Size; got != int64(len(data)) { + t.Errorf("replica attributes part 1 size=%d, want plaintext size %d", got, len(data)) + } + if attributes.ObjectSize != int64(len(data)) { + t.Errorf("replica attributes ObjectSize=%d, want %d", attributes.ObjectSize, len(data)) + } + if attributes.ObjectParts.Parts[0].Size != attributes.ObjectSize { + t.Errorf("replica attributes part 1 size=%d does not match ObjectSize=%d", + attributes.ObjectParts.Parts[0].Size, attributes.ObjectSize) + } } // TestAPIStreamingTrailerWithUntrustedReplicationHeaders verifies that a From 2cbd48a3c3845a252a8457f0ed93dd8c9de8fabb Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Sat, 5 Sep 2026 15:40:55 +0800 Subject: [PATCH 2/2] fix: end GetObjectAttributes part pagination correctly GetObjectAttributes decided truncation by comparing the last returned part number with the part count (cmd/object-handlers.go:720), which is only a coincidence of contiguous numbering. Sparse parts 1/3 reported a complete page as truncated with a marker that loops, and parts 1/3/5 with max-parts=1 stopped after part 3 and silently dropped part 5. Set IsTruncated in the break that proves an eligible part was left unreturned, and zero NextPartNumberMarker when the listing is complete, as ListObjectParts already does. Also reject negative x-amz-max-parts and x-amz-part-number-marker in getAndValidateAttributesOpts with the same API errors ListObjectParts uses, instead of answering an invalid request with an empty parts listing; an absent or zero max-parts still means the default page size. Tests: TestAPIGetObjectAttributesPartsPagination (sparse 1/3/5 and contiguous 1/2 walks on ErasureSD and Erasure), TestGetAndValidateAttributesOptsPartsRange, and the sparse variants of TestAPIGetObjectAttributesMultipartLogicalPartSize. Compatibility: no field is added or removed; IsTruncated and NextPartNumberMarker change only where they were wrong, and negative pagination values that no SDK sends now fail fast. Fixes pgsty/silo#115 Signed-off-by: Feng Ruohang Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01L7qJqWwy8oFA6aCXWRzXQe --- cmd/object-api-options.go | 18 ++ cmd/object-api-options_test.go | 108 ++++++++++ ...object-attributes-parts-pagination_test.go | 185 ++++++++++++++++++ cmd/object-handlers.go | 10 +- 4 files changed, 319 insertions(+), 2 deletions(-) create mode 100644 cmd/object-attributes-parts-pagination_test.go diff --git a/cmd/object-api-options.go b/cmd/object-api-options.go index 6ea1610c4..7efb77a12 100644 --- a/cmd/object-api-options.go +++ b/cmd/object-api-options.go @@ -184,6 +184,16 @@ func getAndValidateAttributesOpts(ctx context.Context, w http.ResponseWriter, r return opts, valid } + // Reject out-of-range page sizes as ListObjectParts does, instead of + // answering an invalid request with an empty parts listing. + if opts.MaxParts < 0 { + apiErr = errorCodes.ToAPIErr(ErrInvalidMaxParts) + argumentName = strings.ToLower(xhttp.AmzMaxParts) + argumentValue = r.Header.Get(xhttp.AmzMaxParts) + valid = false + return opts, valid + } + if opts.MaxParts == 0 { opts.MaxParts = maxPartsList } @@ -196,6 +206,14 @@ func getAndValidateAttributesOpts(ctx context.Context, w http.ResponseWriter, r return opts, valid } + if opts.PartNumberMarker < 0 { + apiErr = errorCodes.ToAPIErr(ErrInvalidPartNumberMarker) + argumentName = strings.ToLower(xhttp.AmzPartNumberMarker) + argumentValue = r.Header.Get(xhttp.AmzPartNumberMarker) + valid = false + return opts, valid + } + opts.ObjectAttributes = parseObjectAttributes(r.Header) if len(opts.ObjectAttributes) < 1 { apiErr = errorCodes.ToAPIErr(ErrInvalidAttributeName) diff --git a/cmd/object-api-options_test.go b/cmd/object-api-options_test.go index 661372cd4..2c229f8ff 100644 --- a/cmd/object-api-options_test.go +++ b/cmd/object-api-options_test.go @@ -18,9 +18,11 @@ package cmd import ( + "encoding/xml" "net/http" "net/http/httptest" "reflect" + "strings" "testing" xhttp "github.com/minio/minio/internal/http" @@ -76,3 +78,109 @@ func TestGetAndValidateAttributesOpts(t *testing.T) { }) } } + +// TestGetAndValidateAttributesOptsPartsRange asserts that GetObjectAttributes +// range checks the ObjectParts pagination headers the way ListObjectParts +// does: a negative value is rejected with the same API error, an absent or +// zero x-amz-max-parts means the default page size, and in-range values are +// passed through unchanged. +func TestGetAndValidateAttributesOptsPartsRange(t *testing.T) { + globalBucketVersioningSys = &BucketVersioningSys{} + bucket := minioMetaBucket + ctx := t.Context() + + testCases := []struct { + name string + maxParts string + marker string + wantValid bool + wantErr APIErrorCode + wantArgument string + wantValue string + wantMaxParts int + wantMarker int + }{ + { + name: "defaults", + wantValid: true, + wantMaxParts: maxPartsList, + }, + { + name: "zero max-parts means the default page size", + maxParts: "0", + wantValid: true, + wantMaxParts: maxPartsList, + }, + { + name: "in range values pass through", + maxParts: "10", + marker: "3", + wantValid: true, + wantMaxParts: 10, + wantMarker: 3, + }, + { + name: "negative max-parts is rejected", + maxParts: "-1", + wantValid: false, + wantErr: ErrInvalidMaxParts, + wantArgument: strings.ToLower(xhttp.AmzMaxParts), + wantValue: "-1", + }, + { + name: "negative part-number-marker is rejected", + marker: "-1", + wantValid: false, + wantErr: ErrInvalidPartNumberMarker, + wantArgument: strings.ToLower(xhttp.AmzPartNumberMarker), + wantValue: "-1", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/testbucket/testobject?attributes", nil) + req.Header.Set(xhttp.AmzObjectAttributes, "ObjectParts") + if testCase.maxParts != "" { + req.Header.Set(xhttp.AmzMaxParts, testCase.maxParts) + } + if testCase.marker != "" { + req.Header.Set(xhttp.AmzPartNumberMarker, testCase.marker) + } + + opts, valid := getAndValidateAttributesOpts(ctx, rec, req, bucket, "testobject") + if valid != testCase.wantValid { + t.Fatalf("want valid %v, got %v (%s)", testCase.wantValid, valid, rec.Body.String()) + } + + if testCase.wantValid { + if opts.MaxParts != testCase.wantMaxParts { + t.Errorf("want MaxParts %d, got %d", testCase.wantMaxParts, opts.MaxParts) + } + if opts.PartNumberMarker != testCase.wantMarker { + t.Errorf("want PartNumberMarker %d, got %d", testCase.wantMarker, opts.PartNumberMarker) + } + return + } + + wantErr := errorCodes.ToAPIErr(testCase.wantErr) + if rec.Code != wantErr.HTTPStatusCode { + t.Errorf("want HTTP status %d, got %d", wantErr.HTTPStatusCode, rec.Code) + } + var errResp objectAttributesErrorResponse + if err := xml.Unmarshal(rec.Body.Bytes(), &errResp); err != nil { + t.Fatalf("decode error response: %v (%s)", err, rec.Body.String()) + } + if errResp.Code != wantErr.Code || errResp.Message != wantErr.Description { + t.Errorf("want error %s/%q, got %s/%q", wantErr.Code, wantErr.Description, errResp.Code, errResp.Message) + } + if errResp.ArgumentName == nil || *errResp.ArgumentName != testCase.wantArgument { + t.Errorf("want ArgumentName %q, got %v", testCase.wantArgument, errResp.ArgumentName) + } + if errResp.ArgumentValue == nil || *errResp.ArgumentValue != testCase.wantValue { + t.Errorf("want ArgumentValue %q, got %v", testCase.wantValue, errResp.ArgumentValue) + } + }) + } +} diff --git a/cmd/object-attributes-parts-pagination_test.go b/cmd/object-attributes-parts-pagination_test.go new file mode 100644 index 000000000..c80a0327d --- /dev/null +++ b/cmd/object-attributes-parts-pagination_test.go @@ -0,0 +1,185 @@ +// Copyright (c) 2015-2026 MinIO, Inc. +// +// This file is part of MinIO Object Storage stack +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "bytes" + "encoding/xml" + "fmt" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/minio/minio/internal/auth" + xhttp "github.com/minio/minio/internal/http" +) + +type attributesPartsPage struct { + ObjectParts struct { + IsTruncated bool + MaxParts int + NextPartNumberMarker int + PartNumberMarker int + PartsCount int + Parts []struct { + PartNumber int + Size int64 + } `xml:"Part"` + } +} + +// TestAPIGetObjectAttributesPartsPagination asserts that ObjectParts pagination +// terminates for sparse part numbers: truncation is decided by whether parts +// remain, not by comparing the last part number with the part count. +func TestAPIGetObjectAttributesPartsPagination(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPIGetObjectAttributesPartsPagination, + }) +} + +func testAPIGetObjectAttributesPartsPagination(_ ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + signedRequest := func(method, target string, body []byte, headers map[string]string) *httptest.ResponseRecorder { + t.Helper() + req, err := newTestSignedRequestV4(method, target, int64(len(body)), bytes.NewReader(body), + credentials.AccessKey, credentials.SecretKey, headers) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + apiRouter.ServeHTTP(rec, req) + return rec + } + + const partSize = 5 * 1024 * 1024 + + // upload completes a multipart object with the given part numbers. Part + // numbers must be strictly increasing, but gaps are allowed, so the last + // part number need not equal the number of parts. + upload := func(object string, partNumbers []int) { + t.Helper() + initRec := signedRequest(http.MethodPost, getNewMultipartURL("", bucketName, object), nil, nil) + if initRec.Code != http.StatusOK { + t.Fatalf("%s INIT: %d %s", instanceType, initRec.Code, initRec.Body.String()) + } + var initiated struct { + UploadID string `xml:"UploadId"` + } + if err := xml.Unmarshal(initRec.Body.Bytes(), &initiated); err != nil { + t.Fatal(err) + } + var complete bytes.Buffer + complete.WriteString("") + for i, number := range partNumbers { + body := bytes.Repeat([]byte("abcd"), partSize/4) + if i == len(partNumbers)-1 { + body = bytes.Repeat([]byte("12345"), 103) + } + put := signedRequest(http.MethodPut, + getPutObjectPartURL("", bucketName, object, initiated.UploadID, strconv.Itoa(number)), body, nil) + if put.Code != http.StatusOK { + t.Fatalf("%s PART %d: %d %s", instanceType, number, put.Code, put.Body.String()) + } + fmt.Fprintf(&complete, "%d%s", + number, put.Header()[xhttp.ETag][0]) + } + complete.WriteString("") + finish := signedRequest(http.MethodPost, + getCompleteMultipartUploadURL("", bucketName, object, initiated.UploadID), complete.Bytes(), nil) + if finish.Code != http.StatusOK { + t.Fatalf("%s COMPLETE: %d %s", instanceType, finish.Code, finish.Body.String()) + } + } + + attributes := func(object string, marker, maxParts int) attributesPartsPage { + t.Helper() + headers := map[string]string{xhttp.AmzObjectAttributes: "ObjectParts"} + if marker > 0 { + headers[xhttp.AmzPartNumberMarker] = strconv.Itoa(marker) + } + if maxParts > 0 { + headers[xhttp.AmzMaxParts] = strconv.Itoa(maxParts) + } + rec := signedRequest(http.MethodGet, getGetObjectURL("", bucketName, object)+"?attributes", nil, headers) + if rec.Code != http.StatusOK { + t.Fatalf("%s ATTRIBUTES marker=%d max=%d: %d %s", instanceType, marker, maxParts, rec.Code, rec.Body.String()) + } + var page attributesPartsPage + if err := xml.Unmarshal(rec.Body.Bytes(), &page); err != nil { + t.Fatal(err) + } + return page + } + + check := func(name string, page attributesPartsPage, wantParts []int, wantTruncated bool, wantNext, wantCount int) { + t.Helper() + got := make([]int, 0, len(page.ObjectParts.Parts)) + for _, p := range page.ObjectParts.Parts { + got = append(got, p.PartNumber) + } + if fmt.Sprint(got) != fmt.Sprint(wantParts) { + t.Errorf("%s %s: parts=%v want=%v", instanceType, name, got, wantParts) + } + if page.ObjectParts.IsTruncated != wantTruncated { + t.Errorf("%s %s: IsTruncated=%v want=%v", instanceType, name, page.ObjectParts.IsTruncated, wantTruncated) + } + if page.ObjectParts.NextPartNumberMarker != wantNext { + t.Errorf("%s %s: NextPartNumberMarker=%d want=%d", instanceType, name, + page.ObjectParts.NextPartNumberMarker, wantNext) + } + if page.ObjectParts.PartsCount != wantCount { + t.Errorf("%s %s: PartsCount=%d want=%d", instanceType, name, page.ObjectParts.PartsCount, wantCount) + } + } + + sparse := "review/attributes-pagination-sparse" + upload(sparse, []int{1, 3, 5}) + + // A single page holding every sparse part is complete. + check("sparse full page", attributes(sparse, 0, 0), []int{1, 3, 5}, false, 0, 3) + + // One part per page walks 1, 3, 5 and stops. Page 2 returns part 3, + // whose number equals the part count, and must still be truncated: + // the old comparison declared that page final and silently dropped + // part 5. + check("sparse max-parts=1 page 1", attributes(sparse, 0, 1), []int{1}, true, 1, 3) + check("sparse max-parts=1 page 2", attributes(sparse, 1, 1), []int{3}, true, 3, 3) + check("sparse max-parts=1 page 3", attributes(sparse, 3, 1), []int{5}, false, 0, 3) + + // A page that exactly holds the remainder is not truncated, so no + // empty trailing page is requested. + check("sparse max-parts=2 page 2", attributes(sparse, 1, 2), []int{3, 5}, false, 0, 3) + + // A marker at or past the last part ends the listing instead of + // looping back through NextPartNumberMarker=0. + check("sparse marker at last part", attributes(sparse, 5, 0), nil, false, 0, 3) + check("sparse marker past last part", attributes(sparse, 9, 0), nil, false, 0, 3) + + // Contiguous numbering is affected too: the empty page past the end + // used to report IsTruncated=true with NextPartNumberMarker=0. + contiguous := "review/attributes-pagination-contiguous" + upload(contiguous, []int{1, 2}) + check("contiguous full page", attributes(contiguous, 0, 0), []int{1, 2}, false, 0, 2) + check("contiguous max-parts=1 page 1", attributes(contiguous, 0, 1), []int{1}, true, 1, 2) + check("contiguous max-parts=1 page 2", attributes(contiguous, 1, 1), []int{2}, false, 0, 2) + check("contiguous marker at last part", attributes(contiguous, 2, 0), nil, false, 0, 2) +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 17fa6d838..ffb283899 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -716,6 +716,9 @@ func (api objectAPIHandlers) getObjectAttributesHandler(ctx context.Context, obj } if len(OA.ObjectParts.Parts) == opts.MaxParts { + // This page is full and at least one more part + // is still pending, so the listing is truncated. + OA.ObjectParts.IsTruncated = true break } @@ -758,8 +761,11 @@ func (api objectAPIHandlers) getObjectAttributesHandler(ctx context.Context, obj } } - if OA.ObjectParts.NextPartNumberMarker != partsLength { - OA.ObjectParts.IsTruncated = true + // Part numbers may be sparse, so they cannot be compared against + // the part count. NextPartNumberMarker only carries a continuation + // token for a truncated listing, as in ListObjectParts. + if !OA.ObjectParts.IsTruncated { + OA.ObjectParts.NextPartNumberMarker = 0 } }