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. |
oxisto
left a comment
oxisto
left a comment
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 |
|
@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. |
|
LGTM, thanks for the updates @oxisto |
|
Can we please have this released? |
The minimum requirement that we have was silently bumped to 1.21 in #441 because of the `slices` package. It seems that we did not update the `go.mod` when we updated our CI range, because CI did not fail because it was not testing older versions. We probably should just update the `go.mod` when we update the CI target in the future? Although we could theoretically stay at 1.21 in terms of the code base.
* Bump Go version to indicate correct minimum requirement The minimum requirement that we have was silently bumped to 1.21 in #441 because of the `slices` package. It seems that we did not update the `go.mod` when we updated our CI range, because CI did not fail because it was not testing older versions. We probably should just update the `go.mod` when we update the CI target in the future? Although we could theoretically stay at 1.21 in terms of the code base. * Removed outdated build tags * Remove more build tags * Removed code for Go < 1.20
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.