fix(s3): reject non-integer max-parts, part-number-marker and max-uploads (#12270)

getObjectResources and getBucketMultipartResources dropped the strconv
error, so a value that does not parse was read as 0. ListParts with
max-parts=abc then reached the filer with limit 0, which the filer reads
as its default listing limit, and returned every part with
<MaxParts>0</MaxParts>; part-number-marker=abc restarted the listing
from part 1; and ListMultipartUploads with max-uploads=abc answered an
empty, untruncated list even with uploads in progress.

Return the existing InvalidArgument errors (ErrInvalidMaxParts,
ErrInvalidPartNumberMarker, ErrInvalidMaxUploads) instead, the way
max-keys is validated. max-parts and max-uploads are parsed as 32-bit
integers to match the documented 0..2147483647 range.
This commit is contained in:
hsdfat authored and GitHub committed 2026-10-31 05:10:50 -07:00
1 parent 6cf6ce81d7
commit 9df2125bb0
2 files changed
+112 -9

No files matched your search

@@ -0,0 +1,81 @@
package s3api
import (
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
"github.com/gorilla/mux"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
)
// uploadsListingFiler also lists the .uploads folder itself, which
// ListMultipartUploads reads.
type uploadsListingFiler struct {
*fakePartsFiler
}
func (f uploadsListingFiler) ListEntries(req *filer_pb.ListEntriesRequest, stream filer_pb.SeaweedFiler_ListEntriesServer) error {
if req.Directory == f.uploadsDir {
return stream.Send(&filer_pb.ListEntriesResponse{Entry: f.uploadEntry})
}
return f.fakePartsFiler.ListEntries(req, stream)
}
// max-parts, part-number-marker and max-uploads must be integers, as their
// InvalidArgument errors say and as max-keys is already validated. A value that
// does not parse used to be read as 0: a max-parts of 0 reaches the filer as
// "no limit" and lists every part, a part-number-marker of 0 restarts the
// listing from the first part, and a max-uploads of 0 lists no uploads.
func TestMultipartListingsRejectNonIntegerArguments(t *testing.T) {
const bucket, object = "b", "a.bin"
f := &fakePartsFiler{parts: []*filer_pb.Entry{
partEntry("0001.part", 5),
partEntry("0002.part", 5),
partEntry("0003.part", 5),
}}
f.uploadsDir = (&S3ApiServer{option: &S3ApiServerOption{}}).genUploadsFolder(bucket)
s3a := newFailoverTestServer(t, startFakeFiler(t, uploadsListingFiler{f}))
s3a.iam = &IdentityAccessManagement{isAuthEnabled: true}
s3a.bucketConfigCache = NewBucketConfigCache(time.Minute)
s3a.bucketConfigCache.Set(bucket, &BucketConfig{Name: bucket})
uploadID := s3a.generateUploadID(object)
f.uploadEntry = uploadRecordEntry(uploadID)
cases := []struct {
name string
query string
handler http.HandlerFunc
object string
}{
{"ListParts max-parts", "uploadId=" + uploadID + "&max-parts=abc", s3a.ListObjectPartsHandler, object},
{"ListParts max-parts out of range", "uploadId=" + uploadID + "&max-parts=2147483648", s3a.ListObjectPartsHandler, object},
{"ListParts part-number-marker", "uploadId=" + uploadID + "&part-number-marker=abc", s3a.ListObjectPartsHandler, object},
{"ListParts part-number-marker out of range", "uploadId=" + uploadID + "&part-number-marker=2147483648", s3a.ListObjectPartsHandler, object},
{"ListMultipartUploads max-uploads", "uploads&max-uploads=abc", s3a.ListMultipartUploadsHandler, ""},
{"ListMultipartUploads max-uploads out of range", "uploads&max-uploads=2147483648", s3a.ListMultipartUploadsHandler, ""},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
req := httptest.NewRequest(http.MethodGet, "/"+bucket+"/"+tc.object+"?"+tc.query, nil)
req = mux.SetURLVars(req, map[string]string{"bucket": bucket, "object": tc.object})
rr := httptest.NewRecorder()
tc.handler(rr, req)
if rr.Code != http.StatusBadRequest || !strings.Contains(rr.Body.String(), "<Code>InvalidArgument</Code>") {
t.Fatalf("got %d %s, want 400 InvalidArgument", rr.Code, rr.Body.String())
}
})
}
// well-formed arguments still page as before
req := httptest.NewRequest(http.MethodGet, "/"+bucket+"/"+object+"?uploadId="+uploadID+"&part-number-marker=1&max-parts=1", nil)
req = mux.SetURLVars(req, map[string]string{"bucket": bucket, "object": object})
rr := httptest.NewRecorder()
s3a.ListObjectPartsHandler(rr, req)
if rr.Code != http.StatusOK || !strings.Contains(rr.Body.String(), "<PartNumber>2</PartNumber>") || strings.Contains(rr.Body.String(), "<PartNumber>3</PartNumber>") {
t.Fatalf("got %d %s, want 200 listing only part 2", rr.Code, rr.Body.String())
}
}
+31 -9
View File
@@ -169,7 +169,7 @@ func (s3a *S3ApiServer) CompleteMultipartUploadHandler(w http.ResponseWriter, r
}
// Get upload id.
uploadID, _, _, _ := getObjectResources(r.URL.Query())
uploadID, _, _, _, _ := getObjectResources(r.URL.Query())
err := s3a.checkUploadId(object, uploadID)
if err != nil {
s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchUpload)
@@ -224,7 +224,7 @@ func (s3a *S3ApiServer) AbortMultipartUploadHandler(w http.ResponseWriter, r *ht
}
// Get upload id.
uploadID, _, _, _ := getObjectResources(r.URL.Query())
uploadID, _, _, _, _ := getObjectResources(r.URL.Query())
err := s3a.checkUploadId(object, uploadID)
if err != nil {
s3err.WriteErrorResponse(w, r, s3err.ErrNoSuchUpload)
@@ -260,7 +260,11 @@ func (s3a *S3ApiServer) ListMultipartUploadsHandler(w http.ResponseWriter, r *ht
return
}
prefix, multipartKeyMarker, uploadIDMarker, delimiter, maxUploads, encodingType := getBucketMultipartResources(r.URL.Query())
prefix, multipartKeyMarker, uploadIDMarker, delimiter, maxUploads, encodingType, errCode := getBucketMultipartResources(r.URL.Query())
if errCode != s3err.ErrNone {
s3err.WriteErrorResponse(w, r, errCode)
return
}
if maxUploads < 0 {
s3err.WriteErrorResponse(w, r, s3err.ErrInvalidMaxUploads)
return
@@ -309,7 +313,11 @@ func (s3a *S3ApiServer) ListObjectPartsHandler(w http.ResponseWriter, r *http.Re
return
}
uploadID, partNumberMarker, maxParts, _ := getObjectResources(r.URL.Query())
uploadID, partNumberMarker, maxParts, _, errCode := getObjectResources(r.URL.Query())
if errCode != s3err.ErrNone {
s3err.WriteErrorResponse(w, r, errCode)
return
}
if partNumberMarker < 0 {
s3err.WriteErrorResponse(w, r, s3err.ErrInvalidPartNumberMarker)
return
@@ -620,13 +628,17 @@ func isLowerHex(value string) bool {
}
// Parse bucket url queries for ?uploads
func getBucketMultipartResources(values url.Values) (prefix, keyMarker, uploadIDMarker, delimiter string, maxUploads int, encodingType string) {
func getBucketMultipartResources(values url.Values) (prefix, keyMarker, uploadIDMarker, delimiter string, maxUploads int, encodingType string, errCode s3err.ErrorCode) {
prefix = values.Get("prefix")
keyMarker = values.Get("key-marker")
uploadIDMarker = values.Get("upload-id-marker")
delimiter = values.Get("delimiter")
if values.Get("max-uploads") != "" {
maxUploads, _ = strconv.Atoi(values.Get("max-uploads"))
parsed, err := strconv.ParseInt(values.Get("max-uploads"), 10, 32)
if err != nil {
errCode = s3err.ErrInvalidMaxUploads
}
maxUploads = int(parsed)
} else {
maxUploads = maxUploadsList
}
@@ -635,11 +647,21 @@ func getBucketMultipartResources(values url.Values) (prefix, keyMarker, uploadID
}
// Parse object url queries
func getObjectResources(values url.Values) (uploadID string, partNumberMarker, maxParts int, encodingType string) {
func getObjectResources(values url.Values) (uploadID string, partNumberMarker, maxParts int, encodingType string, errCode s3err.ErrorCode) {
uploadID = values.Get("uploadId")
partNumberMarker, _ = strconv.Atoi(values.Get("part-number-marker"))
if values.Get("part-number-marker") != "" {
parsed, err := strconv.ParseInt(values.Get("part-number-marker"), 10, 32)
if err != nil {
errCode = s3err.ErrInvalidPartNumberMarker
}
partNumberMarker = int(parsed)
}
if values.Get("max-parts") != "" {
maxParts, _ = strconv.Atoi(values.Get("max-parts"))
parsed, err := strconv.ParseInt(values.Get("max-parts"), 10, 32)
if err != nil {
errCode = s3err.ErrInvalidMaxParts
}
maxParts = int(parsed)
} else {
maxParts = maxPartsList
}