[Security] Improve error handling in OIDC access token handlers #50695
Conversation
|
|
||
| // Verify the signature | ||
| if (!$jwsVerifier->verifyWithKey($jws, $this->jwk, 0)) { | ||
| throw new InvalidSignatureException(); |
There was a problem hiding this comment.
throwing specific exceptions to catch them immediately is pointless, the BadCredentialsException's message (not obfuscated messageKey) is enough. If we want to provide extension points to allow e.g. fixing the token on the fly, we can do it by dispatching specific events.
chalasr
force-pushed
the
oidc-error-handling
branch
from
7fb11f5 to
2c4df90
Compare
| // UserLoader argument can be overridden by a UserProvider on AccessTokenAuthenticator::authenticate | ||
| return new UserBadge($claims[$this->claim], fn () => $this->createUser($claims), $claims); | ||
| } catch (\Exception $e) { | ||
| $this->logger?->error('An error while decoding and validating the token.', [ |
chalasr
force-pushed
the
oidc-error-handling
branch
from
2c4df90 to
7f0f235
Compare
|
@chalasr did you see the test failure? might be related to the change:
|
chalasr
force-pushed
the
oidc-error-handling
branch
from
7f0f235 to
18498d7
Compare
src/Symfony/Component/Security/Http/AccessToken/Oidc/OidcTokenHandler.php
Outdated
Show resolved
Hide resolved
chalasr
force-pushed
the
oidc-error-handling
branch
from
18498d7 to
8adf786
Compare
|
Comments addressed. The failing test should turn back to green once merged up to 7.0. |
|
Can you please rebase? Can you also pleas update the description of the PR so that we can get what this is about? :) |
chalasr
force-pushed
the
oidc-error-handling
branch
from
8adf786 to
b4e1042
Compare
|
@nicolas-grekas done |
| @@ -6,6 +6,8 @@ CHANGELOG | |||
|
|
|||
| * `UserValueResolver` no longer implements `ArgumentValueResolverInterface` | |||
| * Deprecate calling the constructor of `DefaultLoginRateLimiter` with an empty secret | |||
| * [BC BREAK] Remove experimental `InvalidSignatureException` and `MissingClaimException` classes from the `AccessToken\Oidc\Exception` namespace | |||
There was a problem hiding this comment.
This BC break is not worth it IMO. you could deprecate this 2 exceptions instead and remove them in 7.0
| ) { | ||
| if (!$clock || $clock instanceof LoggerInterface) { | ||
| $this->clock = new Clock(); |
There was a problem hiding this comment.
You also need to account for the case where the clock is pass explicitly in the old position instead of ignoring it and assuming that the default clock is used.
|
That logic is getting updated in some more recent PRs. I may resume it later if needed. |
This PR fine-tunes the way errors are handled in OIDC token handlers by catching more specific exceptions from the underlying JWT library and moving error-free assignments out of try/catch blocks, which makes 2 OIDC-specific exception classes (experimental) useless.