Fix bug in validation of multiple audiences #441
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the audience validation logic where the order of evaluation in a for-range loop could yield inconsistent results when validating multiple audiences. Key changes include updates to the audience validation logic in validator.go and the addition of comprehensive tests in validator_test.go to cover various audience validation scenarios.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| validator_test.go | Added multiple test cases to verify audience validation behavior for different modes. |
| validator.go | Adjusted the loop logic in verifyAudience() to fix the order-dependent bug. |
There was a problem hiding this comment.
Thanks for spotting this. This indeed seemed to be a problem. It's really a piece of code that is not easily readable. I wonder if this could be approached from a different angle alltogether. I wonder whether there are any function available in the slices package that could help here.
|
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! |
sfinnman-cotn
force-pushed
the
multiple-aud-fix
branch
from
43e486a to
896291a
Compare
I am not really happy with this implementation, so if you have a better way that is easier to maintain, please go ahead! |
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.
sfinnman-cotn
force-pushed
the
multiple-aud-fix
branch
from
896291a to
02f9520
Compare
|
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 think it's much cleaner, thanks! @mfridman please have a look as well |
| } | ||
|
|
||
| stringClaims = stringClaims + a | ||
| return ErrTokenInvalidAudience |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Ah nope, disregard. We always require this to be true.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
@oxisto I'm okay we merge this, and we follow up with any improvements, if any. The only thing I can see is |
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. |
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:
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.