From 0ce31daf90dc32ec38adf15fe456e6e50f356232 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 8 Oct 2025 14:24:10 -0700 Subject: [PATCH] Fix #7305: Return 400 BadDigest instead of 500 InternalError for MD5 mismatch (#7306) When an S3 upload has a mismatched Content-MD5 header, SeaweedFS was incorrectly returning a 500 Internal Server Error instead of the proper 400 Bad Request with error code BadDigest (per AWS S3 specification). Changes: - Created weed/util/constants/filer.go with error message constants - Added ErrMsgBadDigest constant for MD5 mismatch errors - Added ErrMsgOperationNotPermitted constant for WORM permission errors - Added ErrBadDigest error code with proper 400 status code mapping - Updated filerErrorToS3Error() to detect MD5 mismatch and return ErrBadDigest - Updated filer autoChunk() to return 400 Bad Request for MD5 mismatch - Refactored error handling to use switch statement for better readability - Ordered error checks with exact matches first for better maintainability - Updated all error handling to use centralized constants - Added comprehensive unit tests All error messages now use constants from a single location for better maintainability and consistency. Constants placed in util package to avoid architectural dependency issues. Fixes #7305 --- weed/s3api/s3api_object_handlers_put.go | 3 ++ weed/s3api/s3api_object_handlers_put_test.go | 46 +++++++++++++++++++ weed/s3api/s3err/s3-error.go | 4 +- weed/s3api/s3err/s3api_errors.go | 8 ++++ weed/server/filer_server_handlers_write.go | 5 +- .../filer_server_handlers_write_autochunk.go | 19 +++++--- weed/util/constants/filer.go | 7 +++ 7 files changed, 82 insertions(+), 10 deletions(-) create mode 100644 weed/s3api/s3api_object_handlers_put_test.go create mode 100644 weed/util/constants/filer.go diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 17fceb8d2..cbd8da54f 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -20,6 +20,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" "github.com/seaweedfs/seaweedfs/weed/security" weed_server "github.com/seaweedfs/seaweedfs/weed/server" + "github.com/seaweedfs/seaweedfs/weed/util/constants" stats_collect "github.com/seaweedfs/seaweedfs/weed/stats" ) @@ -380,6 +381,8 @@ func setEtag(w http.ResponseWriter, etag string) { func filerErrorToS3Error(errString string) s3err.ErrorCode { switch { + case errString == constants.ErrMsgBadDigest: + return s3err.ErrBadDigest case strings.HasPrefix(errString, "existing ") && strings.HasSuffix(errString, "is a directory"): return s3err.ErrExistingObjectIsDirectory case strings.HasSuffix(errString, "is a file"): diff --git a/weed/s3api/s3api_object_handlers_put_test.go b/weed/s3api/s3api_object_handlers_put_test.go new file mode 100644 index 000000000..87b874e1f --- /dev/null +++ b/weed/s3api/s3api_object_handlers_put_test.go @@ -0,0 +1,46 @@ +package s3api + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" + "github.com/seaweedfs/seaweedfs/weed/util/constants" +) + +func TestFilerErrorToS3Error(t *testing.T) { + tests := []struct { + name string + errString string + expectedErr s3err.ErrorCode + }{ + { + name: "MD5 mismatch error", + errString: constants.ErrMsgBadDigest, + expectedErr: s3err.ErrBadDigest, + }, + { + name: "Directory exists error", + errString: "existing /path/to/file is a directory", + expectedErr: s3err.ErrExistingObjectIsDirectory, + }, + { + name: "File exists error", + errString: "/path/to/file is a file", + expectedErr: s3err.ErrExistingObjectIsFile, + }, + { + name: "Unknown error", + errString: "some random error", + expectedErr: s3err.ErrInternalError, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := filerErrorToS3Error(tt.errString) + if result != tt.expectedErr { + t.Errorf("filerErrorToS3Error(%q) = %v, want %v", tt.errString, result, tt.expectedErr) + } + }) + } +} diff --git a/weed/s3api/s3err/s3-error.go b/weed/s3api/s3err/s3-error.go index b87764742..c5e515abd 100644 --- a/weed/s3api/s3err/s3-error.go +++ b/weed/s3api/s3err/s3-error.go @@ -1,5 +1,7 @@ package s3err +import "github.com/seaweedfs/seaweedfs/weed/util/constants" + /* * MinIO Go Library for Amazon S3 Compatible Cloud Storage * Copyright 2015-2017 MinIO, Inc. @@ -21,7 +23,7 @@ package s3err // http://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html var s3ErrorResponseMap = map[string]string{ "AccessDenied": "Access Denied.", - "BadDigest": "The Content-Md5 you specified did not match what we received.", + "BadDigest": constants.ErrMsgBadDigest, "EntityTooSmall": "Your proposed upload is smaller than the minimum allowed object size.", "EntityTooLarge": "Your proposed upload exceeds the maximum allowed object size.", "IncompleteBody": "You did not provide the number of bytes specified by the Content-Length HTTP header.", diff --git a/weed/s3api/s3err/s3api_errors.go b/weed/s3api/s3err/s3api_errors.go index 3da79e817..24f8e1b56 100644 --- a/weed/s3api/s3err/s3api_errors.go +++ b/weed/s3api/s3err/s3api_errors.go @@ -4,6 +4,8 @@ import ( "encoding/xml" "fmt" "net/http" + + "github.com/seaweedfs/seaweedfs/weed/util/constants" ) // APIError structure @@ -59,6 +61,7 @@ const ( ErrInvalidBucketName ErrInvalidBucketState ErrInvalidDigest + ErrBadDigest ErrInvalidMaxKeys ErrInvalidMaxUploads ErrInvalidMaxParts @@ -187,6 +190,11 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "The Content-Md5 you specified is not valid.", HTTPStatusCode: http.StatusBadRequest, }, + ErrBadDigest: { + Code: "BadDigest", + Description: constants.ErrMsgBadDigest, + HTTPStatusCode: http.StatusBadRequest, + }, ErrInvalidMaxUploads: { Code: "InvalidArgument", Description: "Argument max-uploads must be an integer between 0 and 2147483647", diff --git a/weed/server/filer_server_handlers_write.go b/weed/server/filer_server_handlers_write.go index 923f2c0eb..1535ba207 100644 --- a/weed/server/filer_server_handlers_write.go +++ b/weed/server/filer_server_handlers_write.go @@ -15,6 +15,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/operation" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/security" + "github.com/seaweedfs/seaweedfs/weed/util/constants" "github.com/seaweedfs/seaweedfs/weed/stats" "github.com/seaweedfs/seaweedfs/weed/storage/needle" "github.com/seaweedfs/seaweedfs/weed/util" @@ -168,7 +169,7 @@ func (fs *FilerServer) move(ctx context.Context, w http.ResponseWriter, r *http. return } else if wormEnforced { // you cannot move a worm file or directory - err = fmt.Errorf("cannot move write-once entry from '%s' to '%s': operation not permitted", src, dst) + err = fmt.Errorf("cannot move write-once entry from '%s' to '%s': %s", src, dst, constants.ErrMsgOperationNotPermitted) writeJsonError(w, r, http.StatusForbidden, err) return } @@ -228,7 +229,7 @@ func (fs *FilerServer) DeleteHandler(w http.ResponseWriter, r *http.Request) { writeJsonError(w, r, http.StatusInternalServerError, err) return } else if wormEnforced { - writeJsonError(w, r, http.StatusForbidden, errors.New("operation not permitted")) + writeJsonError(w, r, http.StatusForbidden, errors.New(constants.ErrMsgOperationNotPermitted)) return } diff --git a/weed/server/filer_server_handlers_write_autochunk.go b/weed/server/filer_server_handlers_write_autochunk.go index 0d6462c11..46fa2519d 100644 --- a/weed/server/filer_server_handlers_write_autochunk.go +++ b/weed/server/filer_server_handlers_write_autochunk.go @@ -20,6 +20,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/operation" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/util/constants" "github.com/seaweedfs/seaweedfs/weed/storage/needle" "github.com/seaweedfs/seaweedfs/weed/util" ) @@ -50,13 +51,17 @@ func (fs *FilerServer) autoChunk(ctx context.Context, w http.ResponseWriter, r * reply, md5bytes, err = fs.doPutAutoChunk(ctx, w, r, chunkSize, contentLength, so) } if err != nil { - if err.Error() == "operation not permitted" { + errStr := err.Error() + switch { + case errStr == constants.ErrMsgOperationNotPermitted: writeJsonError(w, r, http.StatusForbidden, err) - } else if strings.HasPrefix(err.Error(), "read input:") || err.Error() == io.ErrUnexpectedEOF.Error() { + case strings.HasPrefix(errStr, "read input:") || errStr == io.ErrUnexpectedEOF.Error(): writeJsonError(w, r, util.HttpStatusCancelled, err) - } else if strings.HasSuffix(err.Error(), "is a file") || strings.HasSuffix(err.Error(), "already exists") { + case strings.HasSuffix(errStr, "is a file") || strings.HasSuffix(errStr, "already exists"): writeJsonError(w, r, http.StatusConflict, err) - } else { + case errStr == constants.ErrMsgBadDigest: + writeJsonError(w, r, http.StatusBadRequest, err) + default: writeJsonError(w, r, http.StatusInternalServerError, err) } } else if reply != nil { @@ -110,7 +115,7 @@ func (fs *FilerServer) doPostAutoChunk(ctx context.Context, w http.ResponseWrite headerMd5 := r.Header.Get("Content-Md5") if headerMd5 != "" && !(util.Base64Encode(md5bytes) == headerMd5 || fmt.Sprintf("%x", md5bytes) == headerMd5) { fs.filer.DeleteUncommittedChunks(ctx, fileChunks) - return nil, nil, errors.New("The Content-Md5 you specified did not match what we received.") + return nil, nil, errors.New(constants.ErrMsgBadDigest) } filerResult, replyerr = fs.saveMetaData(ctx, r, fileName, contentType, so, md5bytes, fileChunks, chunkOffset, smallContent) if replyerr != nil { @@ -142,7 +147,7 @@ func (fs *FilerServer) doPutAutoChunk(ctx context.Context, w http.ResponseWriter headerMd5 := r.Header.Get("Content-Md5") if headerMd5 != "" && !(util.Base64Encode(md5bytes) == headerMd5 || fmt.Sprintf("%x", md5bytes) == headerMd5) { fs.filer.DeleteUncommittedChunks(ctx, fileChunks) - return nil, nil, errors.New("The Content-Md5 you specified did not match what we received.") + return nil, nil, errors.New(constants.ErrMsgBadDigest) } filerResult, replyerr = fs.saveMetaData(ctx, r, fileName, contentType, so, md5bytes, fileChunks, chunkOffset, smallContent) if replyerr != nil { @@ -171,7 +176,7 @@ func (fs *FilerServer) checkPermissions(ctx context.Context, r *http.Request, fi return err } else if enforced { // you cannot change a worm file - return errors.New("operation not permitted") + return errors.New(constants.ErrMsgOperationNotPermitted) } return nil diff --git a/weed/util/constants/filer.go b/weed/util/constants/filer.go new file mode 100644 index 000000000..f5f240e76 --- /dev/null +++ b/weed/util/constants/filer.go @@ -0,0 +1,7 @@ +package constants + +// Filer error messages +const ( + ErrMsgOperationNotPermitted = "operation not permitted" + ErrMsgBadDigest = "The Content-Md5 you specified did not match what we received." +)