From 3bd990d2eac86bc1c2e6cef478ba7e21b0386b71 Mon Sep 17 00:00:00 2001
From: Chris Lu <chris.lu@gmail.com>
Date: Mon, 16 Feb 2026 02:00:30 -0800
Subject: [PATCH] s3api: address object lock delete review comments

- Remove versioningConfigured guard: object lock protections must apply to all buckets
- Combine identical switch cases for clarity
- Change default case to fail-closed (true) for safety
- Add test case for suspended versioning with specific versionId
- Update test expectations to reflect fail-closed default

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 weed/s3api/s3api_object_handlers_delete.go    | 36 +++++++++----------
 .../s3api_object_handlers_delete_test.go      | 11 ++++--
 2 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/weed/s3api/s3api_object_handlers_delete.go b/weed/s3api/s3api_object_handlers_delete.go
index 7e0f1c2c0..975990b5d 100644
--- a/weed/s3api/s3api_object_handlers_delete.go
+++ b/weed/s3api/s3api_object_handlers_delete.go
@@ -28,14 +28,12 @@ func objectLockVersionToCheckForDelete(versioningState, requestedVersionID strin
 	}
 
 	switch versioningState {
-	case s3_constants.VersioningEnabled:
+	case s3_constants.VersioningEnabled, "":
 		return "", true
 	case s3_constants.VersioningSuspended:
 		return "null", true
-	case "":
-		return "", true
 	default:
-		return "", false
+		return "", true
 	}
 }
 
@@ -253,22 +251,20 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h
 				continue
 			}
 
-			// Check object lock permissions before deletion
-			if versioningConfigured {
-				lockCheckVersionID, shouldCheckObjectLock := objectLockVersionToCheckForDelete(versioningState, object.VersionId)
-				if shouldCheckObjectLock {
-					// Validate governance bypass for this specific object
-					governanceBypassAllowed := s3a.evaluateGovernanceBypassRequest(r, bucket, object.Key)
-					if err := s3a.enforceObjectLockProtections(r, bucket, object.Key, lockCheckVersionID, governanceBypassAllowed); err != nil {
-						glog.V(2).Infof("DeleteMultipleObjectsHandler: object lock check failed for %s/%s (version: %s): %v", bucket, object.Key, lockCheckVersionID, err)
-						deleteErrors = append(deleteErrors, DeleteError{
-							Code:      s3err.GetAPIError(s3err.ErrAccessDenied).Code,
-							Message:   s3err.GetAPIError(s3err.ErrAccessDenied).Description,
-							Key:       object.Key,
-							VersionId: object.VersionId,
-						})
-						continue
-					}
+			// Check object lock permissions before deletion (applies to all buckets: versioned or non-versioned)
+			lockCheckVersionID, shouldCheckObjectLock := objectLockVersionToCheckForDelete(versioningState, object.VersionId)
+			if shouldCheckObjectLock {
+				// Validate governance bypass for this specific object
+				governanceBypassAllowed := s3a.evaluateGovernanceBypassRequest(r, bucket, object.Key)
+				if err := s3a.enforceObjectLockProtections(r, bucket, object.Key, lockCheckVersionID, governanceBypassAllowed); err != nil {
+					glog.V(2).Infof("DeleteMultipleObjectsHandler: object lock check failed for %s/%s (version: %s): %v", bucket, object.Key, lockCheckVersionID, err)
+					deleteErrors = append(deleteErrors, DeleteError{
+						Code:      s3err.GetAPIError(s3err.ErrAccessDenied).Code,
+						Message:   s3err.GetAPIError(s3err.ErrAccessDenied).Description,
+						Key:       object.Key,
+						VersionId: object.VersionId,
+					})
+					continue
 				}
 			}
 
diff --git a/weed/s3api/s3api_object_handlers_delete_test.go b/weed/s3api/s3api_object_handlers_delete_test.go
index ed5717aaa..b85dab157 100644
--- a/weed/s3api/s3api_object_handlers_delete_test.go
+++ b/weed/s3api/s3api_object_handlers_delete_test.go
@@ -44,11 +44,18 @@ func TestObjectLockVersionToCheckForDelete(t *testing.T) {
 			expectedShouldCheck: true,
 		},
 		{
-			name:                "unknown versioning state without version id is skipped",
+			name:                "unknown versioning state without version id is checked (fail-closed for safety)",
 			versioningState:     "UnexpectedState",
 			requestedVersionID:  "",
 			expectedVersionID:   "",
-			expectedShouldCheck: false,
+			expectedShouldCheck: true,
+		},
+		{
+			name:                "suspended versioning with specific version id checks that version",
+			versioningState:     s3_constants.VersioningSuspended,
+			requestedVersionID:  "abc123",
+			expectedVersionID:   "abc123",
+			expectedShouldCheck: true,
 		},
 	}