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