Skip to content

Fix bug in validation of multiple audiences #441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 16 additions & 30 deletions validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package jwt

import (
"fmt"
"slices"
"time"
)

Expand Down Expand Up @@ -235,46 +236,31 @@ func (v *Validator) verifyAudience(claims Claims, cmp []string, expectAllAud boo
return err
}

if len(aud) == 0 {
// Check that aud exists and is not empty.
if len(aud) == 0 || len(aud) == 1 && aud[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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be respecting required bool here? I.e., we best-effort (expectAllAud: false) looped through the audience(s) and didn't find a single match, but if required: false should we still be returning an error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@oxisto oxisto Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yes but we should still respect required within this function (even though it is always set to true from the outside).

It probably depends then whether we had an expected aud or not. If I did not have any expected ones and none match then this should not be an error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels odd to me that required would swallow validation errors. I do check that the aud claim is present if required is set to true. If required is false and the aud claim is either empty or missing, this validation short-circuits to a nil error.

What is the purpose, or intended behaviour of the required parameter?

}

// 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.
Expand Down
118 changes: 118 additions & 0 deletions validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,3 +261,121 @@ 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
required bool
}
tests := []struct {
name string
fields fields
args args
wantErr error
}{
{
name: "good without audience when expecting one aud match",
fields: fields{expectedAud: []string{"example.com"}},
args: args{
claims: MapClaims{},
cmp: []string{"example.com"},
expectAllAud: false,
required: false,
},
wantErr: nil,
},
{
name: "good without audience when expecting all aud matches",
fields: fields{expectedAud: []string{"example.com"}},
args: args{
claims: MapClaims{},
cmp: []string{"example.com"},
expectAllAud: true,
required: false,
},
wantErr: nil,
},
{
name: "audience matches",
fields: fields{expectedAud: []string{"example.com"}},
args: args{
claims: RegisteredClaims{Audience: ClaimStrings{"example.com"}},
cmp: []string{"example.com"},
expectAllAud: false,
required: true,
},
wantErr: nil,
},
{
name: "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,
required: true,
},
wantErr: nil,
},
{
name: "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,
required: true,
},
wantErr: nil,
},
{
name: "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,
required: true,
},
wantErr: ErrTokenInvalidAudience,
},
{
name: "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,
required: true,
},
wantErr: ErrTokenInvalidAudience,
},
{
name: "audience missing when required",
fields: fields{expectedAud: []string{"example.org", "example.com"}},
args: args{
claims: MapClaims{},
cmp: []string{"example.org", "example.com"},
expectAllAud: true,
required: 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,
}
if err := v.verifyAudience(tt.args.claims, tt.args.cmp, tt.args.expectAllAud, tt.args.required); (err != nil) && !errors.Is(err, tt.wantErr) {
t.Errorf("validator.verifyAudience() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}