Browse Source

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
pull/7301/head
Chris Lu 3 days ago
committed by GitHub
parent
commit
0ce31daf90
No known key found for this signature in database GPG Key ID: B5690EEEBB952194
  1. 3
      weed/s3api/s3api_object_handlers_put.go
  2. 46
      weed/s3api/s3api_object_handlers_put_test.go
  3. 4
      weed/s3api/s3err/s3-error.go
  4. 8
      weed/s3api/s3err/s3api_errors.go
  5. 5
      weed/server/filer_server_handlers_write.go
  6. 19
      weed/server/filer_server_handlers_write_autochunk.go
  7. 7
      weed/util/constants/filer.go

3
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"):

46
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)
}
})
}
}

4
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.",

8
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",

5
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
}

19
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

7
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."
)
Loading…
Cancel
Save