From aa2d79daf2dc7497c23aab382a757196b900288b Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 9 Nov 2026 19:13:21 +0800 Subject: [PATCH] fix(object-lock): check a directory object's lock before a bulk delete A directory object always has the null version. DeleteObjects without a version ID chose whether to read the object before it turned the version into null, so with Object Lock and no replication or tiering it checked retention against empty metadata and removed a directory object under legal hold or retention. Choose the null version before the lookup. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Feng Ruohang --- .github/workflows/go.yml | 2 +- cmd/bucket-handlers.go | 12 +++--- cmd/bulk-delete-object-lock_test.go | 57 +++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 cmd/bulk-delete-object-lock_test.go diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 5ad58dfd2..0d06cf211 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -127,7 +127,7 @@ jobs: - name: Run Object Lock authorization and overwrite tests under race detector env: MINIO_API_REQUESTS_MAX: "10000" - run: go test -race -tags kqueue,dev ./cmd -run '^Test(APIPutObjectRetention|APIVersionAddressedWrite|PutObjectRetention|DirectoryObjectLock|ObjectLockOverwrite|APICopyObjectPreservesVersionAfterSuspension|MultipartNullObjectLock|PoolsObjectLockOverwrite|ReplicaDeleteObjectLock|PoolsReplicaDeleteObjectLock|MultipartVersioningTransitions|PlanReview|RecoveredMultipartUpload)' -count=1 -timeout=5m + run: go test -race -tags kqueue,dev ./cmd -run '^Test(APIPutObjectRetention|APIVersionAddressedWrite|PutObjectRetention|DirectoryObjectLock|ObjectLockOverwrite|APICopyObjectPreservesVersionAfterSuspension|MultipartNullObjectLock|PoolsObjectLockOverwrite|ReplicaDeleteObjectLock|PoolsReplicaDeleteObjectLock|BulkDeleteDirectoryObjectLock|MultipartVersioningTransitions|PlanReview|RecoveredMultipartUpload)' -count=1 -timeout=5m - name: Run maintenance storage timeout and compatibility tests under race detector env: diff --git a/cmd/bucket-handlers.go b/cmd/bucket-handlers.go index 0df95f057..4bb7cec0f 100644 --- a/cmd/bucket-handlers.go +++ b/cmd/bucket-handlers.go @@ -523,6 +523,13 @@ func (api objectAPIHandlers) DeleteMultipleObjectsHandler(w http.ResponseWriter, } } + // All deletes on directory objects needs to be for `nullVersionID`. + // Decide this before the lookup below, so that the retention check + // reads the version the delete removes. + if isDirObject(object.ObjectName) && object.VersionID == "" { + object.VersionID = nullVersionID + } + opts := ObjectOptions{ VersionID: object.VersionID, Versioned: vc.PrefixEnabled(object.ObjectName), @@ -541,11 +548,6 @@ func (api objectAPIHandlers) DeleteMultipleObjectsHandler(w http.ResponseWriter, oss[index].SetTransitionState(goi.TransitionedObject) } - // All deletes on directory objects needs to be for `nullVersionID` - if isDirObject(object.ObjectName) && object.VersionID == "" { - object.VersionID = nullVersionID - } - if replicateDeletes { dsc = checkReplicateDelete(ctx, bucket, ObjectToDelete{ ObjectV: ObjectV{ diff --git a/cmd/bulk-delete-object-lock_test.go b/cmd/bulk-delete-object-lock_test.go new file mode 100644 index 000000000..342137e25 --- /dev/null +++ b/cmd/bulk-delete-object-lock_test.go @@ -0,0 +1,57 @@ +// Copyright (c) 2026 PGSTY +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cmd + +import ( + "bytes" + "encoding/xml" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/minio/minio/internal/auth" +) + +// A directory object always has the null version, and a bulk delete without a +// version ID removes it. The retention check must read that version's lock. +func TestBulkDeleteDirectoryObjectLock(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, endpoints: []string{"DeleteMultipleObjects"}, + objAPITest: func(obj ObjectLayer, instance, bucket string, router http.Handler, cred auth.Credentials, t *testing.T) { + enableBucketObjectLock(t, bucket) + for _, kind := range append([]string{"unlocked"}, batchLockKinds...) { + key := "dir-" + kind + "/" + if _, err := obj.PutObject(t.Context(), bucket, key, mustGetPutObjReader(t, bytes.NewReader(nil), 0, "", ""), + ObjectOptions{Versioned: true, VersionID: nullVersionID, UserDefined: batchLockMeta(kind)}); err != nil { + t.Fatal(err) + } + body := encodeResponse(DeleteObjectsRequest{Objects: []ObjectToDelete{{ObjectV: ObjectV{ObjectName: key}}}}) + req, err := newTestSignedRequestV4(http.MethodPost, getDeleteMultipleObjectsURL("", bucket), int64(len(body)), bytes.NewReader(body), + cred.AccessKey, cred.SecretKey, nil) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + var resp DeleteObjectsResponse + if rec.Code != http.StatusOK || xml.Unmarshal(rec.Body.Bytes(), &resp) != nil { + t.Fatalf("%s: %s: status %d: %s", instance, key, rec.Code, rec.Body.String()) + } + exists := batchVersionExists(t, obj, bucket, key, nullVersionID) + if kind == "unlocked" { + if exists || len(resp.DeletedObjects) != 1 { + t.Errorf("%s: %s: unlocked directory object was not deleted: %s", instance, key, rec.Body.String()) + } + continue + } + if !exists || len(resp.Errors) != 1 || len(resp.DeletedObjects) != 0 || + resp.Errors[0].Key != key || !strings.Contains(resp.Errors[0].Message, "WORM") { + t.Errorf("%s: %s: want the WORM error and the object kept (exists=%v): %s", instance, key, exists, rec.Body.String()) + } + } + }, + }) +}