internal/perms: Fix grant parser bugs (#3660)

* internal/perms: Fix grant parser bugs

This fixes two bugs in the grant parser:

1. The parser would accept "," as an output field when parsing from JSON.
  This would then make it into the canonical string,
  which would make it look like we had a trailing comma.
  Disallow the use of a comma as an output field.
2. The parser would accept empty IDs when parsing from JSON.
  Make this behavior consistent with the text behavior.

* internal/perms: also handle empty IDs in text form

* internal/perms: disallow any commas

Commas anywhere in ids or output fields corrupt
the canonical output string.

* internal/perms: disallow any reserved characters

id, ids and actions could all contain one of the
reserved characters ",", "=" or ";" when used with
JSON.

internal/perms/grants.go

@@ -219,8 +219,13 @@ func (g *Grant) unmarshalJSON(ctx context.Context, data []byte) error {
 	}
 	if rawId, ok := raw["id"]; ok {
 		id, ok := rawId.(string)
-		if !ok {
+		switch {
+		case !ok:
 			return errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("unable to interpret %q as string", "id"))
+		case id == "":
+			return errors.New(ctx, errors.InvalidParameter, op, "empty ID provided")
+		case strings.ContainsAny(id, ",;="):
+			return errors.New(ctx, errors.InvalidParameter, op, "ID cannot contain a comma, semicolon or equals sign")
 		}
 		g.id = id
 	}
@@ -232,8 +237,13 @@ func (g *Grant) unmarshalJSON(ctx context.Context, data []byte) error {
 		g.ids = make([]string, len(ids))
 		for i, id := range ids {
 			idStr, ok := id.(string)
-			if !ok {
+			switch {
+			case !ok:
 				return errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("unable to interpret %q element %q as string", "ids", id))
+			case idStr == "":
+				return errors.New(ctx, errors.InvalidParameter, op, "empty ID provided")
+			case strings.ContainsAny(idStr, ",;="):
+				return errors.New(ctx, errors.InvalidParameter, op, "ID cannot contain a comma, semicolon or equals sign")
 			}
 			g.ids[i] = idStr
 		}
@@ -262,6 +272,8 @@ func (g *Grant) unmarshalJSON(ctx context.Context, data []byte) error {
 					return errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("unable to interpret %v in actions array as string", v))
 				case actionStr == "":
 					return errors.New(ctx, errors.InvalidParameter, op, "empty action found")
+				case strings.ContainsAny(actionStr, ",;="):
+					return errors.New(ctx, errors.InvalidParameter, op, "action cannot contain a comma, semicolon or equals sign")
 				default:
 					g.actionsBeingParsed = append(g.actionsBeingParsed, strings.ToLower(actionStr))
 				}
@@ -286,6 +298,8 @@ func (g *Grant) unmarshalJSON(ctx context.Context, data []byte) error {
 				switch {
 				case !ok:
 					return errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("unable to interpret %v in output_fields array as string", v))
+				case strings.ContainsAny(field, ",;="):
+					return errors.New(ctx, errors.InvalidParameter, op, "output fields cannot contain a comma, semicolon or equals sign")
 				default:
 					fields = append(fields, field)
 				}
@@ -315,9 +329,17 @@ func (g *Grant) unmarshalText(ctx context.Context, grantString string) error {
 		switch kv[0] {
 		case "id":
 			g.id = kv[1]
+			if strings.Contains(g.id, ",") {
+				return errors.New(ctx, errors.InvalidParameter, op, "ID cannot contain a comma")
+			}
 
 		case "ids":
 			g.ids = strings.Split(kv[1], ",")
+			for _, id := range g.ids {
+				if id == "" {
+					return errors.New(ctx, errors.InvalidParameter, op, "empty ID provided")
+				}
+			}
 
 		case "type":
 			typeString := strings.ToLower(kv[1])

internal/perms/grants_test.go

@@ -321,6 +321,34 @@ func Test_Unmarshaling(t *testing.T) {
 			textInput: `id=`,
 			textErr:   `perms.(Grant).unmarshalText: segment "id=" not formatted correctly, missing value: parameter violation: error #100`,
 		},
+		{
+			name:      "bad segment id comma",
+			jsonInput: `{"id":","}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+			textInput: `id=,`,
+			textErr:   `perms.(Grant).unmarshalText: ID cannot contain a comma: parameter violation: error #100`,
+		},
+		{
+			name:      "bad segment id start with comma",
+			jsonInput: `{"id":",something"}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+			textInput: `id=,something`,
+			textErr:   `perms.(Grant).unmarshalText: ID cannot contain a comma: parameter violation: error #100`,
+		},
+		{
+			name:      "bad segment id with comma",
+			jsonInput: `{"id":"some,thing"}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+			textInput: `id=some,thing`,
+			textErr:   `perms.(Grant).unmarshalText: ID cannot contain a comma: parameter violation: error #100`,
+		},
+		{
+			name:      "bad segment id end with comma",
+			jsonInput: `{"id":"something,"}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+			textInput: `id=something,`,
+			textErr:   `perms.(Grant).unmarshalText: ID cannot contain a comma: parameter violation: error #100`,
+		},
 		{
 			name:      "bad segment ids",
 			jsonInput: `{"ids":true}`,
@@ -328,6 +356,73 @@ func Test_Unmarshaling(t *testing.T) {
 			textInput: `ids=`,
 			textErr:   `perms.(Grant).unmarshalText: segment "ids=" not formatted correctly, missing value: parameter violation: error #100`,
 		},
+		{
+			name:      "empty segment ids",
+			jsonInput: `{"ids":[""]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: empty ID provided: parameter violation: error #100`,
+			textInput: `ids=,`,
+			textErr:   `perms.(Grant).unmarshalText: empty ID provided: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids comma",
+			jsonInput: `{"ids":[","]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids starting with comma",
+			jsonInput: `{"ids":[",something"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids with comma",
+			jsonInput: `{"ids":["some,thing"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids ending with comma",
+			jsonInput: `{"ids":["something,"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids semicolon",
+			jsonInput: `{"ids":[","]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids starting with semicolon",
+			jsonInput: `{"ids":[";something"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids with semicolon",
+			jsonInput: `{"ids":["some;thing"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids ending with semicolon",
+			jsonInput: `{"ids":["something;"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids equals sign",
+			jsonInput: `{"ids":["="]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids starting with equals sign",
+			jsonInput: `{"ids":["=something"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids with equals sign",
+			jsonInput: `{"ids":["some=thing"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids ending with equals sign",
+			jsonInput: `{"ids":["something="]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
 		{
 			name: "good id",
 			expected: Grant{
@@ -415,6 +510,66 @@ func Test_Unmarshaling(t *testing.T) {
 			textInput: `output_fields=ids=version,name`,
 			textErr:   `perms.(Grant).unmarshalText: segment "output_fields=ids=version,name" not formatted correctly, wrong number of equal signs: parameter violation: error #100`,
 		},
+		{
+			name:      "bad output fields comma",
+			jsonInput: `{"output_fields":[","]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: output fields cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "output fields starting with comma",
+			jsonInput: `{"output_fields":[",something"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: output fields cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "output fields with comma",
+			jsonInput: `{"output_fields":["some,thing"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: output fields cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "output fields ending with comma",
+			jsonInput: `{"output_fields":["something,"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: output fields cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids semicolon",
+			jsonInput: `{"ids":[";"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids starting with semicolon",
+			jsonInput: `{"ids":[";something"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids with semicolon",
+			jsonInput: `{"ids":["some;thing"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids ending with semicolon",
+			jsonInput: `{"ids":["something;"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids equals sign",
+			jsonInput: `{"ids":["="]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids starting with equals sign",
+			jsonInput: `{"ids":["=something"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids with equals sign",
+			jsonInput: `{"ids":["some=thing"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment ids ending with equals sign",
+			jsonInput: `{"ids":["something="]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: ID cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
 		{
 			name: "good actions",
 			expected: Grant{
@@ -442,6 +597,26 @@ func Test_Unmarshaling(t *testing.T) {
 			jsonInput: `{"actions":[1, true]}`,
 			jsonErr:   `perms.(Grant).unmarshalJSON: unable to interpret 1 in actions array as string: parameter violation: error #100`,
 		},
+		{
+			name:      "segment actions comma",
+			jsonInput: `{"actions":[","]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: action cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment actions starting with comma",
+			jsonInput: `{"actions":[",something"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: action cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment actions with comma",
+			jsonInput: `{"actions":["some,thing"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: action cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
+		{
+			name:      "segment actions ending with comma",
+			jsonInput: `{"actions":["something,"]}`,
+			jsonErr:   `perms.(Grant).unmarshalJSON: action cannot contain a comma, semicolon or equals sign: parameter violation: error #100`,
+		},
 	}
 
 	for _, test := range tests {
@@ -1149,6 +1324,7 @@ func FuzzParse(f *testing.F) {
 		"ids={{account.id}},{{user.id}};actions=update,read",
 		`{"id":"foobar","type":"host-catalog","actions":["create"]}`,
 		`{"ids":["foobar"],"type":"host-catalog","actions":["create"]}`,
+		`{"ids":["\""]}`,
 	}
 	for _, tc := range tc {
 		f.Add(tc)

internal/perms/testdata/fuzz/FuzzParse/3187a710d9bc1612

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("ids=,;actions=reAd")

internal/perms/testdata/fuzz/FuzzParse/4572b81e0281070d

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("{\"output_fields\":[\",\"]}")

internal/perms/testdata/fuzz/FuzzParse/536fbfc4db40b83d

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("{\"id\":\"=\",\"actions\":[\"reAd\"]}")

internal/perms/testdata/fuzz/FuzzParse/5d676d09fce49675

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("{\"ids\":[\",00\"],\"actions\":[\"CreAte\"]}")

internal/perms/testdata/fuzz/FuzzParse/6bfe75c429073a6a

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("id=,;output_fields=0")

internal/perms/testdata/fuzz/FuzzParse/9b33c3b84320755d

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("id=,0;output_fields=0")

internal/perms/testdata/fuzz/FuzzParse/c9a7351b4994e353

@@ -0,0 +1,2 @@
+go test fuzz v1
+string("{\"ids\":[\"\"],\"actions\":[\"CreAte\"]}")