internal/verify: ValidIAMPolicyJSON helpful error messages

.changelog/27502.txt

@@ -0,0 +1,3 @@
+```release-note:enhancement
+various IAM resource types: more detailed error messages for invalid policy document JSON
+```

internal/verify/validate.go

@@ -1,6 +1,7 @@
 package verify
 
 import (
+	"encoding/json"
 	"fmt"
 	"net"
 	"regexp"
@@ -162,16 +163,41 @@ func ValidIAMPolicyJSON(v interface{}, k string) (ws []string, errors []error) {
 	// IAM Policy documents need to be valid JSON, and pass legacy parsing
 	value := v.(string)
 	if len(value) < 1 {
-		errors = append(errors, fmt.Errorf("%q contains an invalid JSON policy", k))
-		return
-	}
-	if value[:1] != "{" {
-		errors = append(errors, fmt.Errorf("%q contains an invalid JSON policy", k))
-		return
-	}
-	if _, err := structure.NormalizeJsonString(v); err != nil {
-		errors = append(errors, fmt.Errorf("%q contains an invalid JSON: %s", k, err))
+		errors = append(errors, fmt.Errorf("%q is an empty string, which is not a valid JSON value", k))
+	} else if first := value[:1]; first != "{" {
+		switch value[:1] {
+		case " ", "\t", "\r", "\n":
+			errors = append(errors, fmt.Errorf("%q contains an invalid JSON policy: leading space characters are not allowed", k))
+		case `"`:
+			// There are some common mistakes that lead to strings appearing
+			// here instead of objects, so we'll try some heuristics to
+			// check for those so we might give more actionable feedback in
+			// these situations.
+			var hint string
+			var content string
+			var innerContent any
+			if err := json.Unmarshal([]byte(value), &content); err == nil {
+				if strings.HasSuffix(content, ".json") {
+					hint = " (have you passed a JSON-encoded filename instead of the content of that file?)"
+				} else if err := json.Unmarshal([]byte(content), &innerContent); err == nil {
+					hint = " (have you double-encoded your JSON data?)"
+				}
+			}
+			errors = append(errors, fmt.Errorf("%q contains an invalid JSON policy: contains a JSON-encoded string, not a JSON-encoded object%s", k, hint))
+		case `[`:
+			errors = append(errors, fmt.Errorf("%q contains an invalid JSON policy: contains a JSON array, not a JSON object", k))
+		default:
+			// Generic error for if we didn't find something more specific to say.
+			errors = append(errors, fmt.Errorf("%q contains an invalid JSON policy: not a JSON object", k))
+		}
+	} else if _, err := structure.NormalizeJsonString(v); err != nil {
+		errStr := err.Error()
+		if err, ok := err.(*json.SyntaxError); ok {
+			errStr = fmt.Sprintf("%s, at byte offset %d", errStr, err.Offset)
+		}
+		errors = append(errors, fmt.Errorf("%q contains an invalid JSON policy: %s", k, errStr))
 	}
+
 	return
 }
 

internal/verify/validate_test.go

@@ -380,60 +380,78 @@ func TestValidIAMPolicyJSONString(t *testing.T) {
 	t.Parallel()
 
 	type testCases struct {
-		Value    string
-		ErrCount int
+		Value     string
+		WantError string
 	}
-
-	invalidCases := []testCases{
+	tests := []testCases{
 		{
-			Value:    `{0:"1"}`,
-			ErrCount: 1,
+			Value: `{}`,
+			// Valid
 		},
 		{
-			Value:    `{'abc':1}`,
-			ErrCount: 1,
+			Value: `{"abc":["1","2"]}`,
+			// Valid
 		},
 		{
-			Value:    `{"def":}`,
-			ErrCount: 1,
+			Value:     `{0:"1"}`,
+			WantError: `"json" contains an invalid JSON policy: invalid character '0' looking for beginning of object key string, at byte offset 2`,
 		},
 		{
-			Value:    `{"xyz":[}}`,
-			ErrCount: 1,
+			Value:     `{'abc':1}`,
+			WantError: `"json" contains an invalid JSON policy: invalid character '\'' looking for beginning of object key string, at byte offset 2`,
 		},
 		{
-			Value:    ``,
-			ErrCount: 1,
+			Value:     `{"def":}`,
+			WantError: `"json" contains an invalid JSON policy: invalid character '}' looking for beginning of value, at byte offset 8`,
 		},
 		{
-			Value:    `    {"xyz": "foo"}`,
-			ErrCount: 1,
+			Value:     `{"xyz":[}}`,
+			WantError: `"json" contains an invalid JSON policy: invalid character '}' looking for beginning of value, at byte offset 9`,
 		},
-	}
-
-	for _, tc := range invalidCases {
-		_, errors := ValidIAMPolicyJSON(tc.Value, "json")
-		if len(errors) != tc.ErrCount {
-			t.Fatalf("Expected %q to trigger a validation error.", tc.Value)
-		}
-	}
-
-	validCases := []testCases{
 		{
-			Value:    `{}`,
-			ErrCount: 0,
+			Value:     ``,
+			WantError: `"json" is an empty string, which is not a valid JSON value`,
 		},
 		{
-			Value:    `{"abc":["1","2"]}`,
-			ErrCount: 0,
+			Value:     `    {"xyz": "foo"}`,
+			WantError: `"json" contains an invalid JSON policy: leading space characters are not allowed`,
 		},
-	}
+		{
+			Value:     `"blub"`,
+			WantError: `"json" contains an invalid JSON policy: contains a JSON-encoded string, not a JSON-encoded object`,
+		},
+		{
+			Value:     `"../some-filename.json"`,
+			WantError: `"json" contains an invalid JSON policy: contains a JSON-encoded string, not a JSON-encoded object (have you passed a JSON-encoded filename instead of the content of that file?)`,
+		},
+		{
+			Value:     `"{\"Version\":\"...\"}"`,
+			WantError: `"json" contains an invalid JSON policy: contains a JSON-encoded string, not a JSON-encoded object (have you double-encoded your JSON data?)`,
+		},
+		{
+			Value:     `[{}]`,
+			WantError: `"json" contains an invalid JSON policy: contains a JSON array, not a JSON object`,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.Value, func(t *testing.T) {
+			_, errs := ValidIAMPolicyJSON(test.Value, "json")
+
+			if test.WantError != "" {
+				if got, want := len(errs), 1; got != want {
+					t.Fatalf("wrong number of errors %d; want %d", got, want)
+				}
+				err := errs[0]
+				if got, want := err.Error(), test.WantError; got != want {
+					t.Fatalf("wrong error message\ngot:  %s\nwant: %s", got, want)
+				}
+				return
+			}
 
-	for _, tc := range validCases {
-		_, errors := ValidIAMPolicyJSON(tc.Value, "json")
-		if len(errors) != tc.ErrCount {
-			t.Fatalf("Expected %q not to trigger a validation error.", tc.Value)
-		}
+			for _, err := range errs {
+				t.Errorf("unexpected error: %s", err.Error())
+			}
+		})
 	}
 }