jwt
jwt copied to clipboard
golang-jwt
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:
-
produce "token has invalid audience" if "example.org" was evaluated first
-
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.
There might be! I didn't want to change the implementation too much, since i assume there is a good reason for using a map key lookup as a string comparison function. I don't want to accidentally introduce new bugs with this fix. Ill add the comment as asked for!
There might be! I didn't want to change the implementation too much, since i assume there is a good reason for using a map key lookup as a string comparison function. I don't want to accidentally introduce new bugs with this fix. Ill add the comment as asked for!
I am not really happy with this implementation, so if you have a better way that is easier to maintain, please go ahead!
I pushed a rewrite of the method. This implementation is easier to understand, has worse asymptotic complexity, but is probably faster in most real world cases.
I'm a bit unsure what the expectation should be for the required flag, it could mean that there must exist a non-zero value in the aud claim, or just that the aud claim is present. It feels like a bit of a toss-up in terms of what a consumer of the api would expect in behaviour.
I pushed a rewrite of the method. This implementation is easier to understand, has worse asymptotic complexity, but is probably faster in most real world cases.
I think it's much cleaner, thanks!
@mfridman please have a look as well
@oxisto I'm okay we merge this, and we follow up with any improvements, if any. The only thing I can see is if !required and returning no error on line 252.
@oxisto I'm okay we merge this, and we follow up with any improvements, if any. The only thing I can see is
if !requiredand returning no error on line 252.
I am currently swamped at work and I need some time to think about this unhindered, I will have a look at this at the weekend, the rest looks good to merge.
