S3: fix silent PutObject failure and enforce 1024-byte key limit (#8764)
* S3: add KeyTooLongError error code Add ErrKeyTooLongError (HTTP 400, code "KeyTooLongError") to match the standard AWS S3 error for object keys that exceed length limits. * S3: fix silent PutObject failure when entry name exceeds max_file_name_length putToFiler called client.CreateEntry() directly and discarded the gRPC response. The filer embeds application errors like "entry name too long" in resp.Error (not as gRPC transport errors), so the error was silently swallowed and clients received HTTP 200 with an ETag for objects that were never stored. Switch to the filer_pb.CreateEntry() helper which properly checks resp.Error, and map "entry name too long" to KeyTooLongError (HTTP 400). To avoid fragile string parsing across the gRPC boundary, define shared error message constants in weed/util/constants and use them in both the filer (producing errors) and S3 API (matching errors). Switch filerErrorToS3Error to use strings.Contains/HasSuffix with these constants so matches work regardless of any wrapper prefix. Apply filerErrorToS3Error to the mkdir path for directory markers. Fixes #8759 * S3: enforce 1024-byte maximum object key length AWS S3 limits object keys to 1024 bytes. Add early validation on write paths (PutObject, CopyObject, CreateMultipartUpload) to reject keys exceeding the limit with the standard KeyTooLongError (HTTP 400). The key length check runs before bucket auto-creation to prevent overlong keys from triggering unnecessary side effects. Also use filerErrorToS3Error for CopyObject's mkFile error paths so name-too-long errors from the filer return KeyTooLongError instead of InternalError. Ref #8758 * S3: add handler-level tests for key length validation and error mapping Add tests for filerErrorToS3Error mapping "entry name too long" to KeyTooLongError, including a regression test for the CreateEntry-prefixed "existing ... is a directory" form. Add handler-level integration tests that exercise PutObjectHandler, CopyObjectHandler, and NewMultipartUploadHandler via httptest, verifying HTTP 400 and KeyTooLongError XML response for overlong keys and acceptance of keys at the 1024-byte limit.
This commit is contained in:
@@ -29,6 +29,11 @@ const (
|
||||
S3Namespace = "http://s3.amazonaws.com/doc/2006-03-01/"
|
||||
)
|
||||
|
||||
// S3 object key limits
|
||||
const (
|
||||
MaxS3ObjectKeyLength = 1024
|
||||
)
|
||||
|
||||
// Standard S3 HTTP request constants
|
||||
const (
|
||||
// S3 storage class
|
||||
|
||||
@@ -49,6 +49,10 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request
|
||||
srcBucket, srcObject, srcVersionId := pathToBucketObjectAndVersion(rawCopySource, cpSrcPath)
|
||||
|
||||
glog.V(3).Infof("CopyObjectHandler %s %s (version: %s) => %s %s", srcBucket, srcObject, srcVersionId, dstBucket, dstObject)
|
||||
if len(dstObject) > s3_constants.MaxS3ObjectKeyLength {
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrKeyTooLongError)
|
||||
return
|
||||
}
|
||||
if err := s3a.validateTableBucketObjectPath(dstBucket, dstObject); err != nil {
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied)
|
||||
return
|
||||
@@ -347,7 +351,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request
|
||||
entry.Attributes = dstEntry.Attributes
|
||||
entry.Extended = dstEntry.Extended
|
||||
}); err != nil {
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrInternalError)
|
||||
s3err.WriteErrorResponse(w, r, filerErrorToS3Error(err))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -383,7 +387,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request
|
||||
entry.Attributes = dstEntry.Attributes
|
||||
entry.Extended = dstEntry.Extended
|
||||
}); err != nil {
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrInternalError)
|
||||
s3err.WriteErrorResponse(w, r, filerErrorToS3Error(err))
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -35,6 +35,11 @@ const (
|
||||
func (s3a *S3ApiServer) NewMultipartUploadHandler(w http.ResponseWriter, r *http.Request) {
|
||||
bucket, object := s3_constants.GetBucketAndObject(r)
|
||||
|
||||
if len(object) > s3_constants.MaxS3ObjectKeyLength {
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrKeyTooLongError)
|
||||
return
|
||||
}
|
||||
|
||||
// Check if bucket exists, and create it if it doesn't (auto-create bucket)
|
||||
if err := s3a.checkBucket(r, bucket); err == s3err.ErrNoSuchBucket {
|
||||
// Auto-create bucket if it doesn't exist (requires Admin permission)
|
||||
|
||||
@@ -83,6 +83,10 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request)
|
||||
return
|
||||
}
|
||||
glog.V(2).Infof("PutObjectHandler bucket=%s object=%s size=%d", bucket, object, r.ContentLength)
|
||||
if len(object) > s3_constants.MaxS3ObjectKeyLength {
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrKeyTooLongError)
|
||||
return
|
||||
}
|
||||
if err := s3a.validateTableBucketObjectPath(bucket, object); err != nil {
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied)
|
||||
return
|
||||
@@ -178,7 +182,7 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request)
|
||||
// Set object owner for directory objects (same as regular objects)
|
||||
s3a.setObjectOwnerFromRequest(r, bucket, entry)
|
||||
}); err != nil {
|
||||
s3err.WriteErrorResponse(w, r, s3err.ErrInternalError)
|
||||
s3err.WriteErrorResponse(w, r, filerErrorToS3Error(err))
|
||||
return
|
||||
}
|
||||
setEtag(w, dirEtag)
|
||||
@@ -695,11 +699,11 @@ func (s3a *S3ApiServer) putToFiler(r *http.Request, filePath string, dataReader
|
||||
Entry: entry,
|
||||
}
|
||||
glog.V(3).Infof("putToFiler: Calling CreateEntry for %s", filePath)
|
||||
_, err := client.CreateEntry(context.Background(), req)
|
||||
if err != nil {
|
||||
if err := filer_pb.CreateEntry(context.Background(), client, req); err != nil {
|
||||
glog.Errorf("putToFiler: CreateEntry returned error: %v", err)
|
||||
return err
|
||||
}
|
||||
return err
|
||||
return nil
|
||||
})
|
||||
if createErr != nil {
|
||||
glog.Errorf("putToFiler: failed to create entry for %s: %v", filePath, createErr)
|
||||
@@ -795,17 +799,15 @@ func filerErrorToS3Error(err error) s3err.ErrorCode {
|
||||
case errString == constants.ErrMsgBadDigest:
|
||||
return s3err.ErrBadDigest
|
||||
case errors.Is(err, weed_server.ErrReadOnly):
|
||||
// Bucket is read-only due to quota enforcement or other configuration
|
||||
// Return 403 Forbidden per S3 semantics (similar to MinIO's quota enforcement)
|
||||
// Uses errors.Is() to properly detect wrapped errors
|
||||
return s3err.ErrAccessDenied
|
||||
case strings.Contains(errString, "context canceled") || strings.Contains(errString, "code = Canceled"):
|
||||
// Client canceled the request, return client error not server error
|
||||
return s3err.ErrInvalidRequest
|
||||
case strings.HasPrefix(errString, "existing ") && strings.HasSuffix(errString, "is a directory"):
|
||||
case strings.Contains(errString, constants.ErrMsgExistingPrefix) && strings.HasSuffix(errString, constants.ErrMsgIsADirectory):
|
||||
return s3err.ErrExistingObjectIsDirectory
|
||||
case strings.HasSuffix(errString, "is a file"):
|
||||
case strings.HasSuffix(errString, constants.ErrMsgIsAFile):
|
||||
return s3err.ErrExistingObjectIsFile
|
||||
case strings.Contains(errString, constants.ErrMsgEntryNameTooLong):
|
||||
return s3err.ErrKeyTooLongError
|
||||
default:
|
||||
return s3err.ErrInternalError
|
||||
}
|
||||
|
||||
@@ -1,10 +1,16 @@
|
||||
package s3api
|
||||
|
||||
import (
|
||||
"encoding/xml"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/gorilla/mux"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
|
||||
"github.com/seaweedfs/seaweedfs/weed/s3api/s3err"
|
||||
weed_server "github.com/seaweedfs/seaweedfs/weed/server"
|
||||
"github.com/seaweedfs/seaweedfs/weed/util/constants"
|
||||
@@ -51,11 +57,26 @@ func TestFilerErrorToS3Error(t *testing.T) {
|
||||
err: errors.New("existing /path/to/file is a directory"),
|
||||
expectedErr: s3err.ErrExistingObjectIsDirectory,
|
||||
},
|
||||
{
|
||||
name: "Directory exists error (CreateEntry-wrapped)",
|
||||
err: errors.New("CreateEntry: existing /path/to/file is a directory"),
|
||||
expectedErr: s3err.ErrExistingObjectIsDirectory,
|
||||
},
|
||||
{
|
||||
name: "File exists error",
|
||||
err: errors.New("/path/to/file is a file"),
|
||||
expectedErr: s3err.ErrExistingObjectIsFile,
|
||||
},
|
||||
{
|
||||
name: "Entry name too long error",
|
||||
err: errors.New("CreateEntry: entry name too long"),
|
||||
expectedErr: s3err.ErrKeyTooLongError,
|
||||
},
|
||||
{
|
||||
name: "Entry name too long error (unwrapped)",
|
||||
err: errors.New("entry name too long"),
|
||||
expectedErr: s3err.ErrKeyTooLongError,
|
||||
},
|
||||
{
|
||||
name: "Unknown error",
|
||||
err: errors.New("some random error"),
|
||||
@@ -72,3 +93,108 @@ func TestFilerErrorToS3Error(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// setupKeyLengthTestRouter creates a minimal router that maps requests directly
|
||||
// to the given handler with {bucket} and {object} mux vars, bypassing auth.
|
||||
func setupKeyLengthTestRouter(handler http.HandlerFunc) *mux.Router {
|
||||
router := mux.NewRouter()
|
||||
bucket := router.PathPrefix("/{bucket}").Subrouter()
|
||||
bucket.Path("/{object:.+}").HandlerFunc(handler)
|
||||
return router
|
||||
}
|
||||
|
||||
func TestPutObjectHandler_KeyTooLong(t *testing.T) {
|
||||
s3a := &S3ApiServer{}
|
||||
router := setupKeyLengthTestRouter(s3a.PutObjectHandler)
|
||||
|
||||
longKey := strings.Repeat("a", s3_constants.MaxS3ObjectKeyLength+1)
|
||||
req := httptest.NewRequest(http.MethodPut, "/bucket/"+longKey, nil)
|
||||
rr := httptest.NewRecorder()
|
||||
router.ServeHTTP(rr, req)
|
||||
|
||||
if rr.Code != http.StatusBadRequest {
|
||||
t.Errorf("expected status %d, got %d", http.StatusBadRequest, rr.Code)
|
||||
}
|
||||
var errResp s3err.RESTErrorResponse
|
||||
if err := xml.Unmarshal(rr.Body.Bytes(), &errResp); err != nil {
|
||||
t.Fatalf("failed to parse error XML: %v", err)
|
||||
}
|
||||
if errResp.Code != "KeyTooLongError" {
|
||||
t.Errorf("expected error code KeyTooLongError, got %s", errResp.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPutObjectHandler_KeyAtLimit(t *testing.T) {
|
||||
s3a := &S3ApiServer{}
|
||||
|
||||
// Wrap handler to convert panics from uninitialized server state into 500
|
||||
// responses. The key length check runs early and writes 400 KeyTooLongError
|
||||
// before reaching any code that needs a fully initialized server. A panic
|
||||
// means the handler accepted the key and continued past the check.
|
||||
panicSafe := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
defer func() {
|
||||
if p := recover(); p != nil {
|
||||
w.WriteHeader(http.StatusInternalServerError)
|
||||
}
|
||||
}()
|
||||
s3a.PutObjectHandler(w, r)
|
||||
})
|
||||
router := setupKeyLengthTestRouter(panicSafe)
|
||||
|
||||
atLimitKey := strings.Repeat("a", s3_constants.MaxS3ObjectKeyLength)
|
||||
req := httptest.NewRequest(http.MethodPut, "/bucket/"+atLimitKey, nil)
|
||||
rr := httptest.NewRecorder()
|
||||
router.ServeHTTP(rr, req)
|
||||
|
||||
// Must NOT be KeyTooLongError — any other response (including 500 from
|
||||
// the minimal server hitting uninitialized state) proves the key passed.
|
||||
var errResp s3err.RESTErrorResponse
|
||||
if rr.Code == http.StatusBadRequest {
|
||||
if err := xml.Unmarshal(rr.Body.Bytes(), &errResp); err == nil && errResp.Code == "KeyTooLongError" {
|
||||
t.Errorf("key at exactly %d bytes should not be rejected as too long", s3_constants.MaxS3ObjectKeyLength)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestCopyObjectHandler_KeyTooLong(t *testing.T) {
|
||||
s3a := &S3ApiServer{}
|
||||
router := setupKeyLengthTestRouter(s3a.CopyObjectHandler)
|
||||
|
||||
longKey := strings.Repeat("a", s3_constants.MaxS3ObjectKeyLength+1)
|
||||
req := httptest.NewRequest(http.MethodPut, "/bucket/"+longKey, nil)
|
||||
req.Header.Set("X-Amz-Copy-Source", "/src-bucket/src-object")
|
||||
rr := httptest.NewRecorder()
|
||||
router.ServeHTTP(rr, req)
|
||||
|
||||
if rr.Code != http.StatusBadRequest {
|
||||
t.Errorf("expected status %d, got %d", http.StatusBadRequest, rr.Code)
|
||||
}
|
||||
var errResp s3err.RESTErrorResponse
|
||||
if err := xml.Unmarshal(rr.Body.Bytes(), &errResp); err != nil {
|
||||
t.Fatalf("failed to parse error XML: %v", err)
|
||||
}
|
||||
if errResp.Code != "KeyTooLongError" {
|
||||
t.Errorf("expected error code KeyTooLongError, got %s", errResp.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewMultipartUploadHandler_KeyTooLong(t *testing.T) {
|
||||
s3a := &S3ApiServer{}
|
||||
router := setupKeyLengthTestRouter(s3a.NewMultipartUploadHandler)
|
||||
|
||||
longKey := strings.Repeat("a", s3_constants.MaxS3ObjectKeyLength+1)
|
||||
req := httptest.NewRequest(http.MethodPost, "/bucket/"+longKey+"?uploads", nil)
|
||||
rr := httptest.NewRecorder()
|
||||
router.ServeHTTP(rr, req)
|
||||
|
||||
if rr.Code != http.StatusBadRequest {
|
||||
t.Errorf("expected status %d, got %d", http.StatusBadRequest, rr.Code)
|
||||
}
|
||||
var errResp s3err.RESTErrorResponse
|
||||
if err := xml.Unmarshal(rr.Body.Bytes(), &errResp); err != nil {
|
||||
t.Fatalf("failed to parse error XML: %v", err)
|
||||
}
|
||||
if errResp.Code != "KeyTooLongError" {
|
||||
t.Errorf("expected error code KeyTooLongError, got %s", errResp.Code)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -145,6 +145,9 @@ const (
|
||||
ErrInvalidStorageClass
|
||||
|
||||
ErrInvalidAttributeName
|
||||
|
||||
// Object key length errors
|
||||
ErrKeyTooLongError
|
||||
)
|
||||
|
||||
// Error message constants for checksum validation
|
||||
@@ -608,6 +611,12 @@ var errorCodeResponse = map[ErrorCode]APIError{
|
||||
Description: "Invalid attribute name specified",
|
||||
HTTPStatusCode: http.StatusBadRequest,
|
||||
},
|
||||
|
||||
ErrKeyTooLongError: {
|
||||
Code: "KeyTooLongError",
|
||||
Description: "Your key is too long.",
|
||||
HTTPStatusCode: http.StatusBadRequest,
|
||||
},
|
||||
}
|
||||
|
||||
// GetAPIError provides API Error for input API error code.
|
||||
|
||||
Reference in New Issue
Block a user