Fix issue with MapClaims VerifyAudience []string (golang-jwt#12)

* Fix issue with MapClaims VerifyAudience []string There was an issue in MapClaims's VerifyAudiance where a []string (which is valid in the spec) would return true (claim is found, or nil) when required was not set. It now checks interface types correctly and has tests written Signed-off-by: Alistair Hey <[email protected]> * Keep aud validation constant time compare Keep aud validation using constant time compare by not instantly returning on a true comparison, keep comparing all options and store result in a variable Signed-off-by: Alistair Hey <[email protected]> Co-authored-by: Banse, Christian <[email protected]>
hyeonjae · May 29, 2021 · 0f726ea · 0f726ea
1 parent 6a07921
commit 0f726ea
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 10 deletions.
diff --git a/.gitignore b/.gitignore
@@ -1,4 +1,4 @@
 .DS_Store
 bin
-
+.idea/
 
diff --git a/claims.go b/claims.go
@@ -61,7 +61,7 @@ func (c StandardClaims) Valid() error {
 // Compares the aud claim against cmp.
 // If required is false, this method will return true if the value matches or is unset
 func (c *StandardClaims) VerifyAudience(cmp string, req bool) bool {
-	return verifyAud(c.Audience, cmp, req)
+	return verifyAud([]string{c.Audience}, cmp, req)
 }
 
 // Compares the exp claim against cmp.
@@ -90,15 +90,27 @@ func (c *StandardClaims) VerifyNotBefore(cmp int64, req bool) bool {
 
 // ----- helpers
 
-func verifyAud(aud string, cmp string, required bool) bool {
-	if aud == "" {
+func verifyAud(aud []string, cmp string, required bool) bool {
+	if len(aud) == 0 {
 		return !required
 	}
-	if subtle.ConstantTimeCompare([]byte(aud), []byte(cmp)) != 0 {
-		return true
-	} else {
-		return false
+	// use a var here to keep constant time compare when looping over a number of claims
+	result := false
+
+	var stringClaims string
+	for _, a := range aud {
+		if subtle.ConstantTimeCompare([]byte(a), []byte(cmp)) != 0 {
+			result = true
+		}
+		stringClaims = stringClaims + a
 	}
+
+	// case where "" is sent in one or many aud claims
+	if len(stringClaims) == 0 {
+		return !required
+	}
+
+	return result
 }
 
 func verifyExp(exp int64, now int64, required bool) bool {

diff --git a/map_claims.go b/map_claims.go
@@ -10,10 +10,24 @@ import (
 // This is the default claims type if you don't supply one
 type MapClaims map[string]interface{}
 
-// Compares the aud claim against cmp.
+// VerifyAudience Compares the aud claim against cmp.
 // If required is false, this method will return true if the value matches or is unset
 func (m MapClaims) VerifyAudience(cmp string, req bool) bool {
-	aud, _ := m["aud"].(string)
+	var aud []string
+	switch v := m["aud"].(type) {
+	case string:
+		aud = append(aud, v)
+	case []string:
+		aud = v
+	case []interface{}:
+		for _, a := range v {
+			vs, ok := a.(string)
+			if !ok {
+				return false
+			}
+			aud = append(aud, vs)
+		}
+	}
 	return verifyAud(aud, cmp, req)
 }
 

diff --git a/map_claims_test.go b/map_claims_test.go
@@ -0,0 +1,68 @@
+package jwt
+
+import (
+	"testing"
+)
+
+func TestVerifyAud(t *testing.T) {
+	var nilInterface interface{}
+	var nilListInterface []interface{}
+	var intListInterface interface{} = []int{1,2,3}
+	type test struct{
+		Name string
+		MapClaims MapClaims
+		Expected bool
+		Comparison string
+		Required bool
+	}
+	tests := []test{
+		// Matching Claim in aud
+		// Required = true
+		{ Name: "String Aud matching required", MapClaims: MapClaims{"aud": "example.com"}, Expected: true , Required: true, Comparison: "example.com"},
+		{ Name: "[]String Aud with match required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: true, Comparison: "example.com"},
+
+		// Required = false
+		{ Name: "String Aud with match not required", MapClaims: MapClaims{"aud": "example.com"}, Expected: true , Required: false, Comparison: "example.com"},
+		{ Name: "Empty String Aud with match not required", MapClaims: MapClaims{}, Expected: true , Required: false, Comparison: "example.com"},
+		{ Name: "Empty String Aud with match not required", MapClaims: MapClaims{"aud": ""}, Expected: true , Required: false, Comparison: "example.com"},
+		{ Name: "Nil String Aud with match not required", MapClaims: MapClaims{"aud": nil}, Expected: true , Required: false, Comparison: "example.com"},
+
+		{ Name: "[]String Aud with match not required", MapClaims: MapClaims{"aud": []string{"example.com", "example.example.com"}}, Expected: true, Required: false, Comparison: "example.com"},
+		{ Name: "Empty []String Aud with match not required", MapClaims: MapClaims{"aud": []string{}}, Expected: true, Required: false, Comparison: "example.com"},
+
+		// Non-Matching Claim in aud
+		// Required = true
+		{ Name: "String Aud without match required", MapClaims: MapClaims{"aud": "not.example.com"}, Expected: false, Required: true, Comparison: "example.com"},
+		{ Name: "Empty String Aud without match required", MapClaims: MapClaims{"aud": ""}, Expected: false, Required: true, Comparison: "example.com"},
+		{ Name: "[]String Aud without match required", MapClaims: MapClaims{"aud": []string{"not.example.com", "example.example.com"}}, Expected: false, Required: true, Comparison: "example.com"},
+		{ Name: "Empty []String Aud without match required", MapClaims: MapClaims{"aud": []string{""}}, Expected: false, Required: true, Comparison: "example.com"},
+		{ Name: "String Aud without match not required", MapClaims: MapClaims{"aud": "not.example.com"}, Expected: false, Required: true, Comparison: "example.com"},
+		{ Name: "Empty String Aud without match not required", MapClaims: MapClaims{"aud": ""}, Expected: false, Required: true, Comparison: "example.com"},
+		{ Name: "[]String Aud without match not required", MapClaims: MapClaims{"aud": []string{"not.example.com", "example.example.com"}}, Expected: false, Required: true, Comparison: "example.com"},
+
+		// Required = false
+		{ Name: "Empty []String Aud without match required", MapClaims: MapClaims{"aud": []string{""}}, Expected: false, Required: true, Comparison: "example.com"},
+
+		// []interface{}
+		{ Name: "Empty []interface{} Aud without match required", MapClaims: MapClaims{"aud": nilListInterface}, Expected: true, Required: false, Comparison: "example.com"},
+		{ Name: "[]interface{} Aud wit match required", MapClaims: MapClaims{"aud": []interface{}{"a", "foo", "example.com"}}, Expected: true, Required: true, Comparison: "example.com"},
+		{ Name: "[]interface{} Aud wit match but invalid types", MapClaims: MapClaims{"aud": []interface{}{"a", 5, "example.com"}}, Expected: false, Required: true, Comparison: "example.com"},
+		{ Name: "[]interface{} Aud int wit match required", MapClaims: MapClaims{"aud": intListInterface}, Expected: false, Required: true, Comparison: "example.com"},
+
+
+		// interface{}
+		{ Name: "Empty interface{} Aud without match not required", MapClaims: MapClaims{"aud": nilInterface}, Expected: true, Required: false, Comparison: "example.com"},
+
+	}
+
+
+	for _, test := range tests {
+		t.Run(test.Name, func(t *testing.T) {
+			got := test.MapClaims.VerifyAudience(test.Comparison, test.Required)
+
+			if got != test.Expected {
+				t.Errorf("Expected %v, got %v", test.Expected, got)
+			}
+		})
+	}
+}