diff --git a/weed/s3api/s3api_multipart_list_args_test.go b/weed/s3api/s3api_multipart_list_args_test.go new file mode 100644 index 0000000000..0bf62573e2 --- /dev/null +++ b/weed/s3api/s3api_multipart_list_args_test.go @@ -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(), "InvalidArgument") { + 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(), "2") || strings.Contains(rr.Body.String(), "3") { + t.Fatalf("got %d %s, want 200 listing only part 2", rr.Code, rr.Body.String()) + } +} diff --git a/weed/s3api/s3api_object_handlers_multipart.go b/weed/s3api/s3api_object_handlers_multipart.go index c817027f60..19b802be57 100644 --- a/weed/s3api/s3api_object_handlers_multipart.go +++ b/weed/s3api/s3api_object_handlers_multipart.go @@ -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 }