Fix bug in validation of multiple audiences (#441)

* Fix bug in validation of multiple audiences

In a situation where multiple audiences are validated by the
validator, the order of evaluation of the for-range loop
affects the result.

If we produce matches such as:

```
{
  "example.org": true,
  "example.com": false,
}
```

and we configured the validator to expect a single match on
audience, the code would either:

1. produce "token has invalid audience" if "example.org" was
evaluated first

2. produce a passing result if "example.com" was evaluated first

This commit fixes this bug, and adds a suite of tests as well
as regression tests to prevent this issue in future.

* Adding three more test cases to be sure

* Removing required alltogether form verifyAudience

* Removing required

---------

Co-authored-by: Christian Banse <[email protected]>

Author: sfinnman-cotn

validator.go

@@ -2,6 +2,7 @@ package jwt
 
 import (
 	"fmt"
+	"slices"
 	"time"
 )
 
@@ -124,7 +125,7 @@ func (v *Validator) Validate(claims Claims) error {
 
 	// If we have an expected audience, we also require the audience claim
 	if len(v.expectedAud) > 0 {
-		if err = v.verifyAudience(claims, v.expectedAud, v.expectAllAud, true); err != nil {
+		if err = v.verifyAudience(claims, v.expectedAud, v.expectAllAud); err != nil {
 			errs = append(errs, err)
 		}
 	}
@@ -229,52 +230,39 @@ func (v *Validator) verifyNotBefore(claims Claims, cmp time.Time, required bool)
 //
 // Additionally, if any error occurs while retrieving the claim, e.g., when its
 // the wrong type, an ErrTokenUnverifiable error will be returned.
-func (v *Validator) verifyAudience(claims Claims, cmp []string, expectAllAud bool, required bool) error {
+func (v *Validator) verifyAudience(claims Claims, cmp []string, expectAllAud bool) error {
 	aud, err := claims.GetAudience()
 	if err != nil {
 		return err
 	}
 
-	if len(aud) == 0 {
+	// Check that aud exists and is not empty. We only require the aud claim
+	// if we expect at least one audience to be present.
+	if len(aud) == 0 || len(aud) == 1 && aud[0] == "" {
+		required := len(v.expectedAud) > 0
 		return errorIfRequired(required, "aud")
 	}
 
-	// use a var here to keep constant time compare when looping over a number of claims
-	matching := make(map[string]bool, 0)
-
-	// build a matching hashmap out of the expected aud
-	for _, expected := range cmp {
-		matching[expected] = false
-	}
-
-	// compare the expected aud with the actual aud in a constant time manner by looping over all actual values
-	var stringClaims string
-	for _, a := range aud {
-		a := a
-		_, ok := matching[a]
-		if ok {
-			matching[a] = true
+	if !expectAllAud {
+		for _, a := range aud {
+			// If we only expect one match, we can stop early if we find a match
+			if slices.Contains(cmp, a) {
+				return nil
+			}
 		}
 
-		stringClaims = stringClaims + a
+		return ErrTokenInvalidAudience
 	}
 
-	// check if all expected auds are present
-	result := true
-	for _, match := range matching {
-		if !expectAllAud && match {
-			break
-		} else if !match {
-			result = false
+	// Note that we are looping cmp here to ensure that all expected audiences
+	// are present in the aud claim.
+	for _, a := range cmp {
+		if !slices.Contains(aud, a) {
+			return ErrTokenInvalidAudience
 		}
 	}
 
-	// case where "" is sent in one or many aud claims
-	if stringClaims == "" {
-		return errorIfRequired(required, "aud")
-	}
-
-	return errorIfFalse(result, ErrTokenInvalidAudience)
+	return nil
 }
 
 // verifyIssuer compares the iss claim in claims against cmp.

validator_test.go

@@ -261,3 +261,116 @@ func Test_Validator_verifyIssuedAt(t *testing.T) {
 		})
 	}
 }
+
+func Test_Validator_verifyAudience(t *testing.T) {
+	type fields struct {
+		expectedAud []string
+	}
+	type args struct {
+		claims       Claims
+		cmp          []string
+		expectAllAud bool
+	}
+	tests := []struct {
+		name    string
+		fields  fields
+		args    args
+		wantErr error
+	}{
+		{
+			name:   "fail without audience when expecting one aud match",
+			fields: fields{expectedAud: []string{"example.com"}},
+			args: args{
+				claims:       MapClaims{},
+				cmp:          []string{"example.com"},
+				expectAllAud: false,
+			},
+			wantErr: ErrTokenRequiredClaimMissing,
+		},
+		{
+			name:   "fail without audience when expecting all aud matches",
+			fields: fields{expectedAud: []string{"example.com"}},
+			args: args{
+				claims:       MapClaims{},
+				cmp:          []string{"example.com"},
+				expectAllAud: true,
+			},
+			wantErr: ErrTokenRequiredClaimMissing,
+		},
+		{
+			name:   "good when audience matches",
+			fields: fields{expectedAud: []string{"example.com"}},
+			args: args{
+				claims:       RegisteredClaims{Audience: ClaimStrings{"example.com"}},
+				cmp:          []string{"example.com"},
+				expectAllAud: false,
+			},
+			wantErr: nil,
+		},
+		{
+			name:   "fail when audience matches with one value",
+			fields: fields{expectedAud: []string{"example.org", "example.com"}},
+			args: args{
+				claims:       RegisteredClaims{Audience: ClaimStrings{"example.com"}},
+				cmp:          []string{"example.org", "example.com"},
+				expectAllAud: false,
+			},
+			wantErr: nil,
+		},
+		{
+			name:   "fail when audience matches with all values",
+			fields: fields{expectedAud: []string{"example.org", "example.com"}},
+			args: args{
+				claims:       RegisteredClaims{Audience: ClaimStrings{"example.org", "example.com"}},
+				cmp:          []string{"example.org", "example.com"},
+				expectAllAud: true,
+			},
+			wantErr: nil,
+		},
+		{
+			name:   "fail when audience not matching",
+			fields: fields{expectedAud: []string{"example.org", "example.com"}},
+			args: args{
+				claims:       RegisteredClaims{Audience: ClaimStrings{"example.net"}},
+				cmp:          []string{"example.org", "example.com"},
+				expectAllAud: false,
+			},
+			wantErr: ErrTokenInvalidAudience,
+		},
+		{
+			name:   "fail when audience not matching all values",
+			fields: fields{expectedAud: []string{"example.org", "example.com"}},
+			args: args{
+				claims:       RegisteredClaims{Audience: ClaimStrings{"example.org", "example.net"}},
+				cmp:          []string{"example.org", "example.com"},
+				expectAllAud: true,
+			},
+			wantErr: ErrTokenInvalidAudience,
+		},
+		{
+			name:   "fail when audience missing",
+			fields: fields{expectedAud: []string{"example.org", "example.com"}},
+			args: args{
+				claims:       MapClaims{},
+				cmp:          []string{"example.org", "example.com"},
+				expectAllAud: true,
+			},
+			wantErr: ErrTokenRequiredClaimMissing,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			v := &Validator{
+				expectedAud:  tt.fields.expectedAud,
+				expectAllAud: tt.args.expectAllAud,
+			}
+
+			err := v.verifyAudience(tt.args.claims, tt.args.cmp, tt.args.expectAllAud)
+			if tt.wantErr == nil && err != nil {
+				t.Errorf("validator.verifyAudience() error = %v, wantErr %v", err, tt.wantErr)
+			} else if tt.wantErr != nil && !errors.Is(err, tt.wantErr) {
+				t.Errorf("validator.verifyAudience() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}