fix: Fixes the bucket policy json document validation errors for invalid jsons.

Fixes #965

Changes the returned error description to `Policies must be valid JSON and the first byte must be '{'` for invalid bucket policy json documents, which doesn't start with `{`.
The gateway returns `This policy contains invalid Json` error description, if the document starts with `{`, but still isn't valid json.

Implements the `policyErr` string type which implements the `error` interface, to handle the policy json document validation errors, by avoiding staticchecker warnings.

auth/bucket_policy.go

@@ -22,14 +22,20 @@ import (
 	"github.com/versity/versitygw/s3err"
 )
 
-var (
-	errResourceMismatch = errors.New("Action does not apply to any resource(s) in statement")
-	//lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations
-	errInvalidResource = errors.New("Policy has invalid resource")
-	//lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations
-	errInvalidPrincipal = errors.New("Invalid principal in policy")
-	//lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations
-	errInvalidAction = errors.New("Policy has invalid action")
+type policyErr string
+
+func (p policyErr) Error() string {
+	return string(p)
+}
+
+const (
+	policyErrResourceMismatch = policyErr("Action does not apply to any resource(s) in statement")
+	policyErrInvalidResource  = policyErr("Policy has invalid resource")
+	policyErrInvalidPrincipal = policyErr("Invalid principal in policy")
+	policyErrInvalidAction    = policyErr("Policy has invalid action")
+	policyErrInvalidPolicy    = policyErr("This policy contains invalid Json")
+	policyErrInvalidFirstChar = policyErr("Policies must be valid JSON and the first byte must be '{'")
+	policyErrEmptyStatement   = policyErr("Could not parse the policy: Statement is empty!")
 )
 
 type BucketPolicy struct {
@@ -90,10 +96,10 @@ func (bpi *BucketPolicyItem) Validate(bucket string, iam IAMService) error {
 			break
 		}
 		if *isObjectAction && !containsObjectAction {
-			return errResourceMismatch
+			return policyErrResourceMismatch
 		}
 		if !*isObjectAction && !containsBucketAction {
-			return errResourceMismatch
+			return policyErrResourceMismatch
 		}
 	}
 
@@ -117,14 +123,20 @@ func getMalformedPolicyError(err error) error {
 }
 
 func ValidatePolicyDocument(policyBin []byte, bucket string, iam IAMService) error {
+	if len(policyBin) == 0 || policyBin[0] != '{' {
+		return getMalformedPolicyError(policyErrInvalidFirstChar)
+	}
 	var policy BucketPolicy
 	if err := json.Unmarshal(policyBin, &policy); err != nil {
-		return getMalformedPolicyError(err)
+		var pe policyErr
+		if errors.As(err, &pe) {
+			return getMalformedPolicyError(err)
+		}
+		return getMalformedPolicyError(policyErrInvalidPolicy)
 	}
 
 	if len(policy.Statement) == 0 {
-		//lint:ignore ST1005 Reason: This error message is intended for end-user clarity and follows their expectations
-		return getMalformedPolicyError(errors.New("Could not parse the policy: Statement is empty!"))
+		return getMalformedPolicyError(policyErrEmptyStatement)
 	}
 
 	if err := policy.Validate(bucket, iam); err != nil {

auth/bucket_policy_actions.go

@@ -125,7 +125,7 @@ var supportedObjectActionList = map[Action]struct{}{
 // Validates Action: it should either wildcard match with supported actions list or be in it
 func (a Action) IsValid() error {
 	if !strings.HasPrefix(string(a), "s3:") {
-		return errInvalidAction
+		return policyErrInvalidAction
 	}
 
 	if a == AllActions {
@@ -140,12 +140,12 @@ func (a Action) IsValid() error {
 			}
 		}
 
-		return errInvalidAction
+		return policyErrInvalidAction
 	}
 
 	_, found := supportedActionList[a]
 	if !found {
-		return errInvalidAction
+		return policyErrInvalidAction
 	}
 	return nil
 }
@@ -191,7 +191,7 @@ func (a *Actions) UnmarshalJSON(data []byte) error {
 	var err error
 	if err = json.Unmarshal(data, &ss); err == nil {
 		if len(ss) == 0 {
-			return errInvalidAction
+			return policyErrInvalidAction
 		}
 		*a = make(Actions)
 		for _, s := range ss {
@@ -204,7 +204,7 @@ func (a *Actions) UnmarshalJSON(data []byte) error {
 		var s string
 		if err = json.Unmarshal(data, &s); err == nil {
 			if s == "" {
-				return errInvalidAction
+				return policyErrInvalidAction
 			}
 			*a = make(Actions)
 			err = a.Add(s)

auth/bucket_policy_principals.go

@@ -36,7 +36,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error {
 
 	if err = json.Unmarshal(data, &ss); err == nil {
 		if len(ss) == 0 {
-			return errInvalidPrincipal
+			return policyErrInvalidPrincipal
 		}
 		*p = make(Principals)
 		for _, s := range ss {
@@ -45,7 +45,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error {
 		return nil
 	} else if err = json.Unmarshal(data, &s); err == nil {
 		if s == "" {
-			return errInvalidPrincipal
+			return policyErrInvalidPrincipal
 		}
 		*p = make(Principals)
 		p.Add(s)
@@ -53,7 +53,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error {
 		return nil
 	} else if err = json.Unmarshal(data, &k); err == nil {
 		if k.AWS == "" {
-			return errInvalidPrincipal
+			return policyErrInvalidPrincipal
 		}
 		*p = make(Principals)
 		p.Add(k.AWS)
@@ -65,7 +65,7 @@ func (p *Principals) UnmarshalJSON(data []byte) error {
 		}
 		if err = json.Unmarshal(data, &sk); err == nil {
 			if len(sk.AWS) == 0 {
-				return errInvalidPrincipal
+				return policyErrInvalidPrincipal
 			}
 			*p = make(Principals)
 			for _, s := range sk.AWS {
@@ -97,7 +97,7 @@ func (p Principals) Validate(iam IAMService) error {
 		if len(p) == 1 {
 			return nil
 		}
-		return errInvalidPrincipal
+		return policyErrInvalidPrincipal
 	}
 
 	accs, err := CheckIfAccountsExist(p.ToSlice(), iam)
@@ -105,7 +105,7 @@ func (p Principals) Validate(iam IAMService) error {
 		return err
 	}
 	if len(accs) > 0 {
-		return errInvalidPrincipal
+		return policyErrInvalidPrincipal
 	}
 
 	return nil

auth/bucket_policy_resources.go

@@ -29,7 +29,7 @@ func (r *Resources) UnmarshalJSON(data []byte) error {
 	var err error
 	if err = json.Unmarshal(data, &ss); err == nil {
 		if len(ss) == 0 {
-			return errInvalidResource
+			return policyErrInvalidResource
 		}
 		*r = make(Resources)
 		for _, s := range ss {
@@ -42,7 +42,7 @@ func (r *Resources) UnmarshalJSON(data []byte) error {
 		var s string
 		if err = json.Unmarshal(data, &s); err == nil {
 			if s == "" {
-				return errInvalidResource
+				return policyErrInvalidResource
 			}
 			*r = make(Resources)
 			err = r.Add(s)
@@ -59,7 +59,7 @@ func (r *Resources) UnmarshalJSON(data []byte) error {
 func (r Resources) Add(rc string) error {
 	ok, pattern := isValidResource(rc)
 	if !ok {
-		return errInvalidResource
+		return policyErrInvalidResource
 	}
 
 	r[pattern] = struct{}{}
@@ -93,7 +93,7 @@ func (r Resources) ContainsBucketPattern() bool {
 func (r Resources) Validate(bucket string) error {
 	for resource := range r {
 		if !strings.HasPrefix(resource, bucket) {
-			return errInvalidResource
+			return policyErrInvalidResource
 		}
 	}
 

s3api/controllers/base_test.go

@@ -634,8 +634,7 @@ func TestS3ApiController_PutBucketActions(t *testing.T) {
 	</VersioningConfiguration>
 	`
 
-	policyBody := `
-	{
+	policyBody := `{
 		"Statement": [
 			{
 				"Effect": "Allow",

tests/integration/group-tests.go

@@ -466,6 +466,7 @@ func TestGetBucketAcl(s *S3Conf) {
 
 func TestPutBucketPolicy(s *S3Conf) {
 	PutBucketPolicy_non_existing_bucket(s)
+	PutBucketPolicy_invalid_json(s)
 	PutBucketPolicy_empty_statement(s)
 	PutBucketPolicy_invalid_effect(s)
 	PutBucketPolicy_empty_actions_string(s)
@@ -1055,6 +1056,7 @@ func GetIntTests() IntTests {
 		"GetBucketAcl_access_denied":                                              GetBucketAcl_access_denied,
 		"GetBucketAcl_success":                                                    GetBucketAcl_success,
 		"PutBucketPolicy_non_existing_bucket":                                     PutBucketPolicy_non_existing_bucket,
+		"PutBucketPolicy_invalid_json":                                            PutBucketPolicy_invalid_json,
 		"PutBucketPolicy_empty_statement":                                         PutBucketPolicy_empty_statement,
 		"PutBucketPolicy_invalid_effect":                                          PutBucketPolicy_invalid_effect,
 		"PutBucketPolicy_empty_actions_string":                                    PutBucketPolicy_empty_actions_string,

tests/integration/tests.go

@@ -11402,14 +11402,50 @@ func PutBucketPolicy_non_existing_bucket(s *S3Conf) error {
 	})
 }
 
+func PutBucketPolicy_invalid_json(s *S3Conf) error {
+	testName := "PutBucketPolicy_invalid_json"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		for _, doc := range []string{
+			"{true}",
+			"{asdfsdaf",
+			`{"Principal": "*" `,
+		} {
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			_, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+				Bucket: &bucket,
+				Policy: &doc,
+			})
+			cancel()
+			if err := checkApiErr(err, getMalformedPolicyError("This policy contains invalid Json")); err != nil {
+				return err
+			}
+		}
+
+		for _, doc := range []string{
+			"false",
+			"invalid_json",
+			"bucketPolicy",
+			`"Statement": []}`,
+		} {
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			_, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+				Bucket: &bucket,
+				Policy: &doc,
+			})
+			cancel()
+			if err := checkApiErr(err, getMalformedPolicyError("Policies must be valid JSON and the first byte must be '{'")); err != nil {
+				return err
+			}
+		}
+
+		return nil
+	})
+}
+
 func PutBucketPolicy_empty_statement(s *S3Conf) error {
 	testName := "PutBucketPolicy_empty_statement"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
-		doc := `
-		{
-			"Statement": []
-		}
-		`
+		doc := `{"Statement": []}`
 
 		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
 		_, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
@@ -11783,16 +11819,16 @@ func PutBucketPolicy_object_action_on_bucket_resource(s *S3Conf) error {
 			Policy: &doc,
 		})
 		cancel()
-
 		if err := checkApiErr(err, getMalformedPolicyError("Action does not apply to any resource(s) in statement")); err != nil {
 			return err
 		}
+
 		return nil
 	})
 }
 
 func PutBucketPolicy_bucket_action_on_object_resource(s *S3Conf) error {
-	testName := "PutBucketPolicy_object_action_on_bucket_resource"
+	testName := "PutBucketPolicy_bucket_action_on_object_resource"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
 		resource := fmt.Sprintf(`"arn:aws:s3:::%v/*"`, bucket)
 		doc := genPolicyDoc("Allow", `["*"]`, `"s3:DeleteBucket"`, resource)
@@ -11811,7 +11847,7 @@ func PutBucketPolicy_bucket_action_on_object_resource(s *S3Conf) error {
 	})
 }
 func PutBucketPolicy_explicit_deny(s *S3Conf) error {
-	testName := "PutBucketPolicy_object_action_on_bucket_resource"
+	testName := "PutBucketPolicy_explicit_deny"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
 		user2 := user{"grt2", "grt2secret", "user"}
 		err := createUsers(s, []user{
@@ -11826,8 +11862,7 @@ func PutBucketPolicy_explicit_deny(s *S3Conf) error {
 		resourceWildCard := fmt.Sprintf("%v/*", resource)
 		resourcePrefix := fmt.Sprintf("%v/someprefix/*", resource)
 
-		policy := fmt.Sprintf(`
-			{
+		policy := fmt.Sprintf(`{
 				"Statement": [
 					{
 						"Action": [
@@ -11866,8 +11901,7 @@ func PutBucketPolicy_explicit_deny(s *S3Conf) error {
 						"Resource": "%v"
 					}
 				]
-			}
-		`, resourcePrefix, resource, resourceWildCard, resource, resourcePrefix)
+			}`, resourcePrefix, resource, resourceWildCard, resource, resourcePrefix)
 
 		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
 		_, err = s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
@@ -14032,8 +14066,7 @@ func AccessControl_single_object_resource_actions(s *S3Conf) error {
 func AccessControl_multi_statement_policy(s *S3Conf) error {
 	testName := "AccessControl_multi_statement_policy"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
-		policy := fmt.Sprintf(`
-		{
+		policy := fmt.Sprintf(`{
 			"Statement": [
 				{
 					"Effect": "Deny",
@@ -14048,8 +14081,7 @@ func AccessControl_multi_statement_policy(s *S3Conf) error {
 					"Resource": ["arn:aws:s3:::%s", "arn:aws:s3:::%s/*"]
 				}
 			]
-		}	
-		`, bucket, bucket, bucket)
+		}`, bucket, bucket, bucket)
 
 		usr := user{
 			access: "grt1",

tests/integration/utils.go

@@ -973,8 +973,7 @@ func changeAuthCred(uri, newVal string, index int) (string, error) {
 }
 
 func genPolicyDoc(effect, principal, action, resource string) string {
-	jsonTemplate := `
-	{
+	jsonTemplate := `{
 		"Statement": [
 			{
 				"Effect":  "%s",