fix: bind s3:versionid conditions to the effective object version

A bucket policy that allows s3:DeleteObject only when s3:versionid is null
-- Condition {"Null": {"s3:versionid": "true"}}, the idiom for "let clients
delete current objects but not roll back versions" -- denied every delete,
including the version-less ones it was meant to permit (upstream issue
minio/minio#21735).

getConditionValues wrote "versionid": {""} unconditionally. The condition
engine decides Null by slice length (nullfunc.evaluate), so a present-but-
empty value reads as "key present": Null:true never matched and Null:false
always did. Absent and empty were indistinguishable.

Writing the key only when the request names a version fixes the reported
case but, alone, opens a worse one. DeleteObjects carries each object's
version in the XML body, which getConditionValues -- reading only r.Form --
never sees. A body version would then vanish from the map, read as null,
and a policy meant to protect old versions would authorize deleting a
specific one. So authorization also rebinds versionid to the effective,
server-resolved reqInfo.VersionID for DeleteObjectAction: the per-entry
body value that checkRequestAuthTypeWithVID already sets in the
DeleteObjects loop, deleting the key when that value is empty. A
query-level ?versionId on a DeleteObjects POST no longer leaks into any
entry's decision.

Finally, trim the version the condition builder reads. newContext and
getOpts both TrimSpace it before the object layer acts, so an untrimmed
value here let a padded ?versionId=V%20 present a different s3:versionid
than the version actually operated on, sidestepping a Deny keyed on
StringEquals s3:versionid. DeleteObjectAction was already immune via the
trimmed reqInfo value; this covers GetObject, tagging, retention, and the
copy-source read.

Tests: an end-to-end DeleteObjects against a Null:{s3:versionid:true}
policy over versioned objects (with a decoy query versionId proving the
per-entry body value wins), and a unit test asserting key presence,
trimming, and the copy-source fallback.

Co-authored-by: ChatGPT <noreply@openai.com>
Co-authored-by: Claude <noreply@anthropic.com>

cmd/bucket-handlers_test.go

@@ -944,3 +944,130 @@ func testAPIDeleteMultipleObjectsHandler(obj ObjectLayer, instanceType, bucketNa
 	// `ExecObjectLayerAPINilTest` manages the operation.
 	ExecObjectLayerAPINilTest(t, nilBucket, nilObject, instanceType, apiRouter, nilReq)
 }
+
+func TestAPIDeleteMultipleObjectsVersionIDNullCondition(t *testing.T) {
+	ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{
+		t:                 t,
+		objAPITest:        testAPIDeleteMultipleObjectsVersionIDNullCondition,
+		endpoints:         []string{"DeleteMultipleObjects", "PutBucketPolicy"},
+		makeBucketOptions: MakeBucketOptions{VersioningEnabled: true},
+	})
+}
+
+func testAPIDeleteMultipleObjectsVersionIDNullCondition(obj ObjectLayer, instanceType, bucketName string, apiRouter http.Handler,
+	credentials auth.Credentials, t *testing.T,
+) {
+	versionIDs := make(map[string]string, 4)
+	for _, objectName := range []string{
+		"without-version-id-before",
+		"with-version-id",
+		"without-version-id-after",
+		"with-null-version-id",
+	} {
+		data := []byte(objectName)
+		info, err := obj.PutObject(t.Context(), bucketName, objectName,
+			mustGetPutObjReader(t, bytes.NewReader(data), int64(len(data)), "", ""), ObjectOptions{Versioned: true})
+		if err != nil {
+			t.Fatalf("%s: put %q: %v", instanceType, objectName, err)
+		}
+		if info.VersionID == "" {
+			t.Fatalf("%s: put %q did not create a version ID", instanceType, objectName)
+		}
+		versionIDs[objectName] = info.VersionID
+	}
+
+	policyBytes := fmt.Appendf(nil, `{
+		"Version":"2012-10-17",
+		"Statement":[{
+			"Effect":"Allow",
+			"Principal":"*",
+			"Action":"s3:DeleteObject",
+			"Resource":"arn:aws:s3:::%s/*",
+			"Condition":{"Null":{"s3:versionid":"true"}}
+		}]
+	}`, bucketName)
+	policyReq, err := newTestSignedRequestV4(http.MethodPut, getPutPolicyURL("", bucketName), int64(len(policyBytes)),
+		bytes.NewReader(policyBytes), credentials.AccessKey, credentials.SecretKey, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	policyRec := httptest.NewRecorder()
+	apiRouter.ServeHTTP(policyRec, policyReq)
+	if policyRec.Code != http.StatusNoContent {
+		t.Fatalf("%s: put policy returned %d: %s", instanceType, policyRec.Code, policyRec.Body.String())
+	}
+
+	deleteBody := encodeResponse(DeleteObjectsRequest{Objects: []ObjectToDelete{
+		{ObjectV: ObjectV{ObjectName: "without-version-id-before"}},
+		{ObjectV: ObjectV{ObjectName: "with-version-id", VersionID: versionIDs["with-version-id"]}},
+		{ObjectV: ObjectV{ObjectName: "without-version-id-after"}},
+		{ObjectV: ObjectV{ObjectName: "with-null-version-id", VersionID: nullVersionID}},
+	}})
+	// A query-level versionId is not the version of every XML entry. Each
+	// object's optional VersionId remains the effective authorization value.
+	deleteURL := getDeleteMultipleObjectsURL("", bucketName) + "&versionId=query-level-decoy"
+	deleteReq, err := newTestRequest(http.MethodPost, deleteURL,
+		int64(len(deleteBody)), bytes.NewReader(deleteBody))
+	if err != nil {
+		t.Fatal(err)
+	}
+	deleteRec := httptest.NewRecorder()
+	apiRouter.ServeHTTP(deleteRec, deleteReq)
+	if deleteRec.Code != http.StatusOK {
+		t.Fatalf("%s: delete returned %d: %s", instanceType, deleteRec.Code, deleteRec.Body.String())
+	}
+
+	var response DeleteObjectsResponse
+	if err = xml.Unmarshal(deleteRec.Body.Bytes(), &response); err != nil {
+		t.Fatalf("%s: decode response: %v: %s", instanceType, err, deleteRec.Body.String())
+	}
+	deleted := make(map[string]DeletedObject, len(response.DeletedObjects))
+	for _, object := range response.DeletedObjects {
+		deleted[object.ObjectName] = object
+	}
+	for _, objectName := range []string{"without-version-id-before", "without-version-id-after"} {
+		object, ok := deleted[objectName]
+		if !ok || !object.DeleteMarker || object.DeleteMarkerVersionID == "" {
+			t.Errorf("%s: %q was not a successful delete-marker creation: %+v", instanceType, objectName, response.DeletedObjects)
+		}
+	}
+	if len(deleted) != 2 {
+		t.Errorf("%s: unexpected deleted objects: %+v", instanceType, response.DeletedObjects)
+	}
+	errorsByKey := make(map[string]DeleteError, len(response.Errors))
+	for _, deleteErr := range response.Errors {
+		errorsByKey[deleteErr.Key] = deleteErr
+	}
+	for objectName, versionID := range map[string]string{
+		"with-version-id":      versionIDs["with-version-id"],
+		"with-null-version-id": nullVersionID,
+	} {
+		deleteErr, ok := errorsByKey[objectName]
+		if !ok || deleteErr.VersionID != versionID || deleteErr.Code != errorCodes[ErrAccessDenied].Code {
+			t.Errorf("%s: %q did not return AccessDenied for version %q: %+v", instanceType, objectName, versionID, response.Errors)
+		}
+	}
+	if len(errorsByKey) != 2 {
+		t.Errorf("%s: unexpected delete errors: %+v", instanceType, response.Errors)
+	}
+
+	// A simple delete adds a marker and keeps the old version. The explicitly
+	// named version must also remain because its policy condition did not match.
+	for objectName, versionID := range versionIDs {
+		if _, err = obj.GetObjectInfo(t.Context(), bucketName, objectName, ObjectOptions{VersionID: versionID}); err != nil {
+			t.Errorf("%s: version %s of %q was not preserved: %v", instanceType, versionID, objectName, err)
+		}
+	}
+	for _, objectName := range []string{"without-version-id-before", "without-version-id-after"} {
+		if _, err = obj.GetObjectInfo(t.Context(), bucketName, objectName, ObjectOptions{}); !isErrObjectNotFound(err) {
+			t.Errorf("%s: simple delete of %q did not hide the latest object behind a delete marker: %v", instanceType, objectName, err)
+		}
+	}
+	for _, objectName := range []string{"with-version-id", "with-null-version-id"} {
+		if info, err := obj.GetObjectInfo(t.Context(), bucketName, objectName, ObjectOptions{}); err != nil {
+			t.Errorf("%s: denied version delete removed latest %q: %v", instanceType, objectName, err)
+		} else if info.VersionID != versionIDs[objectName] {
+			t.Errorf("%s: latest version of %q changed from %s to %s", instanceType, objectName, versionIDs[objectName], info.VersionID)
+		}
+	}
+}