From 2877febd7321aba373c642a5e7d15a0614e88148 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 24 Mar 2026 13:35:28 -0700 Subject: [PATCH] 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. --- weed/filer/filer.go | 9 +- weed/s3api/s3_constants/header.go | 5 + weed/s3api/s3api_object_handlers_copy.go | 8 +- weed/s3api/s3api_object_handlers_multipart.go | 5 + weed/s3api/s3api_object_handlers_put.go | 22 +-- weed/s3api/s3api_object_handlers_put_test.go | 126 ++++++++++++++++++ weed/s3api/s3err/s3api_errors.go | 9 ++ weed/util/constants/filer.go | 10 +- 8 files changed, 177 insertions(+), 17 deletions(-) diff --git a/weed/filer/filer.go b/weed/filer/filer.go index 537483fd4..25fce8bf3 100644 --- a/weed/filer/filer.go +++ b/weed/filer/filer.go @@ -23,6 +23,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/util" + "github.com/seaweedfs/seaweedfs/weed/util/constants" "github.com/seaweedfs/seaweedfs/weed/util/log_buffer" "github.com/seaweedfs/seaweedfs/weed/wdclient" "golang.org/x/sync/singleflight" @@ -203,7 +204,7 @@ func (f *Filer) CreateEntry(ctx context.Context, entry *Entry, o_excl bool, isFr } if entry.FullPath.IsLongerFileName(maxFilenameLength) { - return fmt.Errorf("entry name too long") + return fmt.Errorf(constants.ErrMsgEntryNameTooLong) } if entry.IsDirectory() { @@ -324,7 +325,7 @@ func (f *Filer) ensureParentDirectoryEntry(ctx context.Context, entry *Entry, di } else if !dirEntry.IsDirectory() { glog.ErrorfCtx(ctx, "CreateEntry %s: %s should be a directory", entry.FullPath, dirPath) - return fmt.Errorf("%s is a file", dirPath) + return fmt.Errorf("%s%s", dirPath, constants.ErrMsgIsAFile) } return nil @@ -335,11 +336,11 @@ func (f *Filer) UpdateEntry(ctx context.Context, oldEntry, entry *Entry) (err er entry.Attr.Crtime = oldEntry.Attr.Crtime if oldEntry.IsDirectory() && !entry.IsDirectory() { glog.ErrorfCtx(ctx, "existing %s is a directory", oldEntry.FullPath) - return fmt.Errorf("existing %s is a directory", oldEntry.FullPath) + return fmt.Errorf("%s%s%s", constants.ErrMsgExistingPrefix, oldEntry.FullPath, constants.ErrMsgIsADirectory) } if !oldEntry.IsDirectory() && entry.IsDirectory() { glog.ErrorfCtx(ctx, "existing %s is a file", oldEntry.FullPath) - return fmt.Errorf("existing %s is a file", oldEntry.FullPath) + return fmt.Errorf("%s%s%s", constants.ErrMsgExistingPrefix, oldEntry.FullPath, constants.ErrMsgIsAFile) } } return f.Store.UpdateEntry(ctx, entry) diff --git a/weed/s3api/s3_constants/header.go b/weed/s3api/s3_constants/header.go index 39529f7bc..6f5974b17 100644 --- a/weed/s3api/s3_constants/header.go +++ b/weed/s3api/s3_constants/header.go @@ -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 diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 818caff40..f4e446ec1 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -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 } diff --git a/weed/s3api/s3api_object_handlers_multipart.go b/weed/s3api/s3api_object_handlers_multipart.go index 52dc00817..ceb1d4eb8 100644 --- a/weed/s3api/s3api_object_handlers_multipart.go +++ b/weed/s3api/s3api_object_handlers_multipart.go @@ -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) diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index d944f496c..b0e2ac703 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -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 } diff --git a/weed/s3api/s3api_object_handlers_put_test.go b/weed/s3api/s3api_object_handlers_put_test.go index 011b73578..ef6b9a757 100644 --- a/weed/s3api/s3api_object_handlers_put_test.go +++ b/weed/s3api/s3api_object_handlers_put_test.go @@ -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) + } +} diff --git a/weed/s3api/s3err/s3api_errors.go b/weed/s3api/s3err/s3api_errors.go index 309f543fb..d46e7673b 100644 --- a/weed/s3api/s3err/s3api_errors.go +++ b/weed/s3api/s3err/s3api_errors.go @@ -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. diff --git a/weed/util/constants/filer.go b/weed/util/constants/filer.go index f5f240e76..559154c91 100644 --- a/weed/util/constants/filer.go +++ b/weed/util/constants/filer.go @@ -1,7 +1,15 @@ package constants -// Filer error messages +// Filer error messages — shared between filer (producing) and S3 API (matching). +// These cross a gRPC boundary as strings in CreateEntryResponse.Error, so typed +// errors cannot be used. Keeping the strings in one place prevents mismatches. const ( ErrMsgOperationNotPermitted = "operation not permitted" ErrMsgBadDigest = "The Content-Md5 you specified did not match what we received." + ErrMsgEntryNameTooLong = "entry name too long" + // Suffix appended to a path, e.g. "/buckets/b/foo/bar is a file" + ErrMsgIsAFile = " is a file" + // Prefix + suffix, e.g. "existing /buckets/b/foo is a directory" + ErrMsgExistingPrefix = "existing " + ErrMsgIsADirectory = " is a directory" )