From 30f3fac4e17dcb2cf648dbd2848f1aae6459d7a9 Mon Sep 17 00:00:00 2001
From: niksis02 <sisnikoyan@gmail.com>
Date: Sat, 1 Mar 2025 01:14:12 +0400
Subject: [PATCH] fix: Prioritize explicit deny in bucket policy statements

---
 auth/bucket_policy.go            |  5 +-
 tests/integration/group-tests.go |  2 +
 tests/integration/tests.go       | 85 ++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 2 deletions(-)

diff --git a/auth/bucket_policy.go b/auth/bucket_policy.go
index 0765d272..a29e6fa6 100644
--- a/auth/bucket_policy.go
+++ b/auth/bucket_policy.go
@@ -48,18 +48,19 @@ func (bp *BucketPolicy) Validate(bucket string, iam IAMService) error {
 }
 
 func (bp *BucketPolicy) isAllowed(principal string, action Action, resource string) bool {
+	var isAllowed bool
 	for _, statement := range bp.Statement {
 		if statement.findMatch(principal, action, resource) {
 			switch statement.Effect {
 			case BucketPolicyAccessTypeAllow:
-				return true
+				isAllowed = true
 			case BucketPolicyAccessTypeDeny:
 				return false
 			}
 		}
 	}
 
-	return false
+	return isAllowed
 }
 
 type BucketPolicyItem struct {
diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go
index a81de847..e6f15a87 100644
--- a/tests/integration/group-tests.go
+++ b/tests/integration/group-tests.go
@@ -471,6 +471,7 @@ func TestPutBucketPolicy(s *S3Conf) {
 	PutBucketPolicy_incorrect_bucket_name(s)
 	PutBucketPolicy_object_action_on_bucket_resource(s)
 	PutBucketPolicy_bucket_action_on_object_resource(s)
+	PutBucketPolicy_explicit_deny(s)
 	PutBucketPolicy_success(s)
 }
 
@@ -1042,6 +1043,7 @@ func GetIntTests() IntTests {
 		"PutBucketPolicy_duplicate_resource":                                      PutBucketPolicy_duplicate_resource,
 		"PutBucketPolicy_incorrect_bucket_name":                                   PutBucketPolicy_incorrect_bucket_name,
 		"PutBucketPolicy_object_action_on_bucket_resource":                        PutBucketPolicy_object_action_on_bucket_resource,
+		"PutBucketPolicy_explicit_deny":                                           PutBucketPolicy_explicit_deny,
 		"PutBucketPolicy_bucket_action_on_object_resource":                        PutBucketPolicy_bucket_action_on_object_resource,
 		"PutBucketPolicy_success":                                                 PutBucketPolicy_success,
 		"GetBucketPolicy_non_existing_bucket":                                     GetBucketPolicy_non_existing_bucket,
diff --git a/tests/integration/tests.go b/tests/integration/tests.go
index 9e86a6d0..7fbe5c41 100644
--- a/tests/integration/tests.go
+++ b/tests/integration/tests.go
@@ -11196,6 +11196,91 @@ func PutBucketPolicy_bucket_action_on_object_resource(s *S3Conf) error {
 	})
 }
 
+func PutBucketPolicy_explicit_deny(s *S3Conf) error {
+	testName := "PutBucketPolicy_object_action_on_bucket_resource"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		user2 := user{"grt2", "grt2secret", "user"}
+		err := createUsers(s, []user{
+			{"grt1", "grt1secret", "user"},
+			user2,
+		})
+		if err != nil {
+			return err
+		}
+
+		resource := fmt.Sprintf("arn:aws:s3:::%v", bucket)
+		resourceWildCard := fmt.Sprintf("%v/*", resource)
+		resourcePrefix := fmt.Sprintf("%v/someprefix/*", resource)
+
+		policy := fmt.Sprintf(`
+			{
+				"Statement": [
+					{
+						"Action": [
+							"s3:*"
+						],
+						"Effect": "Allow",
+						"Principal": [
+							"grt1"
+						],
+						"Resource": [
+							"%v",
+							"%v"
+						]
+					},
+					{
+						"Action": [
+							"s3:*"
+						],
+						"Effect": "Allow",
+						"Principal": [
+							"grt2"
+						],
+						"Resource": [
+							"%v",
+							"%v"
+						]
+					},
+					{
+						"Action": [
+							"s3:*"
+						],
+						"Effect": "Deny",
+						"Principal": [
+							"grt2"
+						],
+						"Resource": "%v"
+					}
+				]
+			}
+		`, resourcePrefix, resource, resourceWildCard, resource, resourcePrefix)
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: &bucket,
+			Policy: &policy,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		userClient := getUserS3Client(user2, s)
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = userClient.PutObject(ctx, &s3.PutObjectInput{
+			Bucket: &bucket,
+			Key:    getPtr("someprefix/hello"),
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
 func PutBucketPolicy_success(s *S3Conf) error {
 	testName := "PutBucketPolicy_success"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {