From 90aa5c31021ff47f8beae98d32c8f3228e1997f3 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 19 Aug 2026 15:51:54 +0200 Subject: [PATCH 01/28] feat!: fail closed when an OpenID Connect callback cannot be validated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the fix for the outage the whole line has been building towards. The 5.1 releases made the cause visible; this stops the loop. A failed callback threw AuthenticationException, which Symfony's security ExceptionListener catches and answers by calling the firewall's entry point — for this authenticator, another redirect to the identity provider. With a live SSO session upstream the IdP returned a fresh code immediately, which failed the same way. Nothing in the cycle degraded, so nothing stopped it: nine or more rounds in twenty-five seconds, no error page, nothing logged. onAuthenticationFailure() now throws AuthenticationFailedException, a \RuntimeException implementing the bundle marker per ADR 001. AuthenticatorManager::executeAuthenticator() catches only AuthenticationException, so this propagates to HttpKernel and the application renders its own error. The loop is prevented by the type of the exception rather than by a counter, which is why the regression guard is a single assertion: the thrown type must not be an AuthenticationException. That test catches the exception as Throwable on purpose — catching the expected type first would narrow it statically and make the assertion a tautology. Deviation from the plan worth noting: CliLoginTokenAuthenticator is left alone. It is not an entry point, so its failures fall through to the OIDC entry point and redirect to normal login, which cannot loop because supports() requires a loginToken and the redirect carries none. For a single-use token that has already been consumed, the login page beats a 500. The loop needs an authenticator whose own entry point re-triggers it, and only the OIDC one has that shape. Adds ADR 002 and UPGRADE-6.0.md. The upgrade guide covers only this change; making client_secret_expires_at required and removing the deprecated exception API follow separately, and the guide gains those sections then. --- CHANGELOG.md | 13 ++ UPGRADE-6.0.md | 78 ++++++++ ...2-fail-closed-on-authentication-failure.md | 172 ++++++++++++++++++ docs/adr/README.md | 3 + .../AuthenticationFailedException.php | 17 ++ src/Security/OpenIdLoginAuthenticator.php | 27 ++- tests/Exception/ExceptionHierarchyTest.php | 2 + .../Security/OpenIdLoginAuthenticatorTest.php | 39 +++- 8 files changed, 337 insertions(+), 14 deletions(-) create mode 100644 UPGRADE-6.0.md create mode 100644 docs/adr/002-fail-closed-on-authentication-failure.md create mode 100644 src/Exception/AuthenticationFailedException.php diff --git a/CHANGELOG.md b/CHANGELOG.md index ab2818b..5a05c18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed (BREAKING) + +- A failed OpenID Connect callback now throws + `AuthenticationFailedException` instead of Symfony's + `AuthenticationException`. The security component caught the latter and + answered by redirecting to the identity provider again, so a permanent + failure such as an expired client secret produced an unbreakable redirect + loop. The exception now escapes the firewall and the application renders its + own error. See `UPGRADE-6.0.md` and + `docs/adr/002-fail-closed-on-authentication-failure.md`. + `CliLoginTokenAuthenticator` is unchanged: it has no entry point of its own, + so it cannot loop. + ## [5.1.1] - 2026-08-19 ### Fixed diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md new file mode 100644 index 0000000..f54e891 --- /dev/null +++ b/UPGRADE-6.0.md @@ -0,0 +1,78 @@ +# Upgrading from 5.x to 6.0 + +6.0 changes what happens when an OpenID Connect callback cannot be validated: the +bundle now fails closed instead of sending the user back to the identity provider. + +Before 6.0 a failed callback threw an `AuthenticationException`, which Symfony's +security component catches and answers by calling the firewall's entry point — +another redirect to the identity provider. When the failure was permanent, such as +an expired client secret, that produced an unbreakable redirect loop with no error +page and nothing in the logs. This is what took `sites.itkdev.dk` down on +2026-08-12. + +See the architecture decision in +[docs/adr/002-fail-closed-on-authentication-failure.md](docs/adr/002-fail-closed-on-authentication-failure.md). + +## A failed login is now an error, not a redirect + +`OpenIdLoginAuthenticator::onAuthenticationFailure()` throws +`\ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException`, which is +**not** a `Symfony\Component\Security\Core\Exception\AuthenticationException`. The +firewall therefore does not catch it: it reaches `HttpKernel`, and your application +renders it like any other unhandled exception — a 500 by default. + +**What you need to do depends on what you have today.** + +If you catch `AuthenticationException` anywhere around the login callback, that +catch no longer matches: + +```diff +- } catch (\Symfony\Component\Security\Core\Exception\AuthenticationException $e) { ++ } catch (\ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface $e) { +``` + +Catching the bundle marker is the recommended form — per +[ADR 001](docs/adr/001-marker-interface-exception-hierarchy.md) it covers every +failure this bundle and the upstream library raise. `AuthenticationFailedException` +extends `\RuntimeException`, so a `catch (\RuntimeException $e)` also matches. + +If you catch nothing, you need no code change. Verify that a failed login renders +an acceptable error page and that your error reporting picks it up. + +## Rendering something friendlier than a 500 + +The bundle ships no templates and takes no view on your layout, so it does not +render an error page for you. To show one, listen for the exception: + +```php +use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; +use Symfony\Component\EventDispatcher\Attribute\AsEventListener; +use Symfony\Component\HttpKernel\Event\ExceptionEvent; + +#[AsEventListener] +final class LoginFailureListener +{ + public function __invoke(ExceptionEvent $event): void + { + if (!$event->getThrowable() instanceof AuthenticationFailedException) { + return; + } + + // The cause is chained as $previous, and has already been logged by the + // bundle on the `openid_connect` channel. + $event->setResponse(new Response($this->twig->render('login_failed.html.twig'), 503)); + } +} +``` + +Do not answer such a listener with a redirect back to the login route. That +reintroduces the loop this release exists to remove. + +## The CLI login authenticator is unchanged + +`CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a +consumed or invalid login token still sends the user to your normal login page. +That path cannot loop — it has no entry point of its own, and the redirect carries +no `loginToken` — and for a single-use token that has already been used, arriving +at the login page is friendlier than an error. If you have a catch around the CLI +login flow specifically, it keeps working. diff --git a/docs/adr/002-fail-closed-on-authentication-failure.md b/docs/adr/002-fail-closed-on-authentication-failure.md new file mode 100644 index 0000000..eaad5a8 --- /dev/null +++ b/docs/adr/002-fail-closed-on-authentication-failure.md @@ -0,0 +1,172 @@ +# 002: Fail closed when an OpenID Connect callback cannot be validated + +- **Created By:** Ture Gjørup +- **Date:** 2026-08-19 +- **Decision Maker:** Ture Gjørup (draft — awaits team review) +- **Stakeholders:** Bundle consumers (applications whose users log in via this + bundle); operators of those applications; + `itk-dev/openid-connect-bundle` maintainers +- **Status:** Draft + +## Context + +On 2026-08-12 `sites.itkdev.dk` became unreachable. The Azure client secret had +expired, so the token exchange returned `invalid_client`. What users saw was not +an error but an endless redirect: + +```text +GET /admin → 302 /openidconnect/login/azure_az + → B2C authorize → Azure AD (silent SSO SUCCEEDS) → valid ?code= +GET /openid-connect/generic?state=…&code=… → 302 /openidconnect/login/azure_az +``` + +Nine or more cycles in twenty-five seconds, no error page, nothing in the logs. +The IdP authenticated the user correctly every round; the bundle rejected its own +callback and started again. + +The mechanism is the interaction between two pieces of Symfony: + +1. `OpenIdLoginAuthenticator::onAuthenticationFailure()` throws an + `AuthenticationException`. +2. Symfony's security `ExceptionListener` catches any `AuthenticationException` + raised during a request and calls the firewall's *entry point* to start + authentication. For this firewall the entry point is this same authenticator, + whose `start()` redirects to the identity provider. + +So every failure re-enters the flow that just failed. Because the IdP has a live +SSO session it answers immediately with a fresh `code`, which fails the same way. +Nothing in the cycle degrades, so nothing stops it: no backoff, no counter, no +error surface. The only signal available to an operator was that the site was +unusable. + +`AuthenticatorManager::executeAuthenticator()` catches **only** +`AuthenticationException` (verified in `symfony/security-http` 6.4 and 8.1). An +exception outside that hierarchy propagates to `HttpKernel` and is rendered by the +application's own error handling. + +### Drivers + +- **Functional:** + - A failure that cannot be resolved by retrying must not be retried. Nothing + about a rejected callback improves by asking the identity provider again. + - The failure must reach a human. A 500 with the cause in the log is + actionable; a redirect loop is not. + - Consumers must be able to catch it. Per + [ADR 001](001-marker-interface-exception-hierarchy.md), that means + implementing `OpenIdConnectBundleExceptionInterface`. +- **Non-functional:** + - This changes an exception type thrown from a public method, so it is a + consumer-visible break and belongs in a MAJOR release. + - The 5.1 line already made the *cause* visible through logging. This is about + the *behaviour*, and the two were deliberately separated so the diagnostics + could ship without waiting for a major. + +## Options Considered + +1. **Throw outside the security hierarchy (proposed).** Introduce + `AuthenticationFailedException extends \RuntimeException implements + OpenIdConnectBundleExceptionInterface` and throw it from + `onAuthenticationFailure()`. The firewall does not catch it, so it reaches + `HttpKernel` and the application renders its own error page. + - **Pros:** Removes the loop by construction rather than by counting attempts. + One line of behaviour, no new state. Satisfies the ADR 001 marker contract, + so a consumer can still catch every OIDC failure with one `catch`. The + invariant is expressible as a single test assertion — the thrown type is not + an `AuthenticationException` — which is what stops the loop from returning. + - **Cons:** Consumers see a 500 where they previously saw a redirect. Anyone + who caught `AuthenticationException` around this path must migrate. Requires + a MAJOR release. + +2. **Loop protection: count attempts in the session.** Keep throwing + `AuthenticationException`, but track failures per session and stop redirecting + after N. + - **Pros:** No exception-type change, so no BC break. + - **Cons:** Adds mutable session state to a failure path, and the failure modes + that matter can lose the session (the loop began with a state-validation + failure, and a lost session is one cause of that — the counter would be lost + with it). Picking N is arbitrary; the user still suffers N pointless round + trips through the identity provider first. It treats a symptom. + +3. **Render an error page from the bundle.** Return a `Response` from + `onAuthenticationFailure()` instead of throwing. + - **Pros:** No exception type change; the user sees something intelligible. + - **Cons:** Puts presentation in a bundle that has no templates and no opinion + about the application's layout, and it swallows the failure — the + application's error handling and error reporters never see it. A 500 that the + application already knows how to render and report is better. + +## Decision + +**Adopt Option 1**, in **6.0.0**. + +`OpenIdLoginAuthenticator::onAuthenticationFailure()` throws +`AuthenticationFailedException`, chaining the original exception as `$previous` +and preserving its message and code. + +### Scope: the OIDC authenticator only + +`CliLoginTokenAuthenticator` keeps throwing `AuthenticationException`, and that is +deliberate rather than an oversight. It is not an +`AuthenticationEntryPointInterface`, so its failures fall through to the +firewall's entry point — the OIDC authenticator — and redirect to normal login. +That cannot loop: `supports()` requires a `loginToken` query parameter, and the +redirect does not carry one. For a single-use CLI token that has already been +consumed, arriving at the normal login page is a better outcome than a 500. + +The distinction to keep in mind is that the loop needs an authenticator whose own +entry point re-triggers it. Only the OIDC authenticator has that shape. + +### Not logged at the failure handler + +`AuthenticatorManager` already logs the original exception before substituting a +sanitised one, `validateClaims()` logs the specific reason on the +`openid_connect` channel, and the application's error handling logs whatever +escapes. A record here would be the fourth for one failure. + +## Consequences + +### Positive + +- The loop cannot recur. It is prevented by the type of the exception, not by + configuration, a counter, or an operator remembering something. +- A failing callback surfaces as an error the application already knows how to + render, report and alert on. +- Paired with the 5.1 logging, an expired client secret now produces: a + `warning`/`error` naming the cause at the callback, an application-level error + for the escaping exception, and — where `client_secret_expires_at` is + configured — a `critical` before it expires at all. + +### Negative / Trade-offs + +- **Consumer-visible break.** A consuming application that catches + `AuthenticationException` around the callback no longer catches this. Migration + is mechanical: catch `OpenIdConnectBundleExceptionInterface`, or let it become a + 500. Covered in `UPGRADE-6.0.md`. +- Users see the application's error page rather than being bounced back to login. + For a genuinely broken configuration that is the point; for a transient IdP + failure it is less forgiving than a retry. Judged the right trade, because the + bundle cannot tell the two apart and the retry path is what produced the outage. +- Applications wanting something friendlier than a 500 must add an exception + listener. Documented, but it is work pushed onto consumers. + +### Follow-up Actions + +- [ ] `UPGRADE-6.0.md` — consumer migration notes for the exception type +- [ ] Consider whether `CliLoginTokenAuthenticator` should gain its own entry + point, which would make its failure handling a separate decision rather + than an inherited one +- [ ] Revisit whether the bundle should offer an opt-in error-page listener, if + several consumers end up writing the same one + +## References + +- Symfony `AuthenticatorManager::executeAuthenticator()` — catches only + `AuthenticationException`: + +- Symfony security `ExceptionListener` — converts an `AuthenticationException` + into a call to the firewall entry point: + +- [ADR 001](001-marker-interface-exception-hierarchy.md) — the marker-interface + exception contract this new concrete follows +- Keep a Changelog: +- Semantic Versioning 2.0.0: diff --git a/docs/adr/README.md b/docs/adr/README.md index d766d1a..a032376 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -12,3 +12,6 @@ context that drove them and the consequences we accept. See - **[001 — Adopt marker-interface exception hierarchy across library and bundle](001-marker-interface-exception-hierarchy.md)** — Draft — 2026-05-11 +- **[002 — Fail closed when an OpenID Connect callback cannot be + validated](002-fail-closed-on-authentication-failure.md)** — Draft — + 2026-08-19 diff --git a/src/Exception/AuthenticationFailedException.php b/src/Exception/AuthenticationFailedException.php new file mode 100644 index 0000000..85f495f --- /dev/null +++ b/src/Exception/AuthenticationFailedException.php @@ -0,0 +1,17 @@ +getMessage()), $exception->getCode(), $exception); + // Still not logged here: AuthenticatorManager has already logged the + // original exception, validateClaims() logged the specific reason, and the + // application's error handling logs whatever escapes. A record here would be + // the fourth for one failure. + throw new AuthenticationFailedException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), $exception); } } diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index 9ee9a93..06f4bf1 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -4,6 +4,7 @@ use ItkDev\OpenIdConnect\Exception\HttpException as LibraryHttpException; use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; +use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\CacheException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\ItkOpenIdConnectBundleException; @@ -42,6 +43,7 @@ public static function concreteProvider(): iterable yield 'CacheException' => [CacheException::class, \RuntimeException::class]; yield 'TokenNotFoundException' => [TokenNotFoundException::class, \RuntimeException::class]; yield 'UserDoesNotExistException' => [UserDoesNotExistException::class, \RuntimeException::class]; + yield 'AuthenticationFailedException' => [AuthenticationFailedException::class, \RuntimeException::class]; } /** diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 799ca1c..c77c309 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -7,7 +7,9 @@ use ItkDev\OpenIdConnect\Exception\ValidationException; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; +use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; +use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; @@ -48,19 +50,44 @@ public function testSupports(): void $this->assertTrue($this->authenticator->supports($request)); } - public function testOnAuthenticationFailurePreservesCause(): void + /** + * The assertion that encodes "the loop cannot come back". + * + * Everything else here is detail; what matters is the type. Symfony's security + * ExceptionListener catches `AuthenticationException` and re-enters the entry + * point, which for this authenticator is another redirect to the identity + * provider. Throwing something outside that hierarchy is what stops a failing + * callback from being retried forever. + */ + public function testOnAuthenticationFailureThrowsOutsideTheSecurityHierarchy(): void { $cause = new AuthenticationException('Original cause message'); + // Caught as Throwable on purpose: catching the expected type first would + // narrow it statically and make the assertions below tautologies, which is + // precisely the mistake that would let the type quietly regress. try { $this->authenticator->onAuthenticationFailure(new Request(), $cause); - $this->fail('Expected AuthenticationException'); - } catch (AuthenticationException $thrown) { + $this->fail('Expected AuthenticationFailedException'); + } catch (\Throwable $thrown) { + $this->assertNotInstanceOf( + AuthenticationException::class, + $thrown, + 'An AuthenticationException would be caught by the firewall and turned back into a redirect to the identity provider', + ); + $this->assertInstanceOf( + OpenIdConnectBundleExceptionInterface::class, + $thrown, + 'Consumers catch the bundle marker, per ADR 001', + ); + $this->assertInstanceOf(AuthenticationFailedException::class, $thrown); + $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained as previous'); $this->assertStringContainsString('Original cause message', $thrown->getMessage(), 'Cause message must be preserved for logs'); // Deliberately no record: the framework already logs the original - // exception, and validateClaims() logged the specific reason. + // exception, validateClaims() logged the specific reason, and the + // application logs whatever escapes. $this->assertSame([], $this->logger->records); } } @@ -271,8 +298,8 @@ public function testEveryFailurePathWorksWithoutALogger(): void // onAuthenticationFailure(). try { $authenticator->onAuthenticationFailure(new Request(), new AuthenticationException('boom')); - $this->fail('Expected AuthenticationException'); - } catch (AuthenticationException $thrown) { + $this->fail('Expected AuthenticationFailedException'); + } catch (AuthenticationFailedException $thrown) { $this->assertStringContainsString('boom', $thrown->getMessage()); } } From 3e6266b6404b83025b919ffd872353fe1392e1a0 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 19 Aug 2026 15:59:28 +0200 Subject: [PATCH 02/28] docs: cut ADR 002 and UPGRADE-6.0 down to the point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 002 from 172 lines to 69, the upgrade guide from 78 to 46. The decisions, rejected alternatives and consequences are unchanged; what went was restatement — a Drivers section repeating the context, prose walking through the same mechanism twice, and migration advice padded around a one-line diff. --- UPGRADE-6.0.md | 70 ++----- ...2-fail-closed-on-authentication-failure.md | 191 ++++-------------- 2 files changed, 63 insertions(+), 198 deletions(-) diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index f54e891..2163166 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -1,78 +1,46 @@ # Upgrading from 5.x to 6.0 -6.0 changes what happens when an OpenID Connect callback cannot be validated: the -bundle now fails closed instead of sending the user back to the identity provider. +A failed OpenID Connect callback now throws +`\ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException` instead of +Symfony's `AuthenticationException`. -Before 6.0 a failed callback threw an `AuthenticationException`, which Symfony's -security component catches and answers by calling the firewall's entry point — -another redirect to the identity provider. When the failure was permanent, such as -an expired client secret, that produced an unbreakable redirect loop with no error -page and nothing in the logs. This is what took `sites.itkdev.dk` down on -2026-08-12. +Before 6.0 the security component caught that exception and redirected to the +identity provider again, so a permanent failure such as an expired client secret +looped forever with no error page. The new exception escapes the firewall, so your +application renders it — a 500 by default. -See the architecture decision in -[docs/adr/002-fail-closed-on-authentication-failure.md](docs/adr/002-fail-closed-on-authentication-failure.md). +See [ADR 002](docs/adr/002-fail-closed-on-authentication-failure.md). -## A failed login is now an error, not a redirect - -`OpenIdLoginAuthenticator::onAuthenticationFailure()` throws -`\ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException`, which is -**not** a `Symfony\Component\Security\Core\Exception\AuthenticationException`. The -firewall therefore does not catch it: it reaches `HttpKernel`, and your application -renders it like any other unhandled exception — a 500 by default. - -**What you need to do depends on what you have today.** - -If you catch `AuthenticationException` anywhere around the login callback, that -catch no longer matches: +## Migrate catch blocks ```diff - } catch (\Symfony\Component\Security\Core\Exception\AuthenticationException $e) { + } catch (\ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface $e) { ``` -Catching the bundle marker is the recommended form — per -[ADR 001](docs/adr/001-marker-interface-exception-hierarchy.md) it covers every -failure this bundle and the upstream library raise. `AuthenticationFailedException` -extends `\RuntimeException`, so a `catch (\RuntimeException $e)` also matches. - -If you catch nothing, you need no code change. Verify that a failed login renders -an acceptable error page and that your error reporting picks it up. +If you catch nothing today, no code change is needed. Check that a failed login +renders an acceptable error and that your error reporting picks it up. ## Rendering something friendlier than a 500 -The bundle ships no templates and takes no view on your layout, so it does not -render an error page for you. To show one, listen for the exception: +Listen for the exception. Do not answer with a redirect to the login route — that +reintroduces the loop. ```php -use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; -use Symfony\Component\EventDispatcher\Attribute\AsEventListener; -use Symfony\Component\HttpKernel\Event\ExceptionEvent; - #[AsEventListener] final class LoginFailureListener { public function __invoke(ExceptionEvent $event): void { - if (!$event->getThrowable() instanceof AuthenticationFailedException) { - return; + if ($event->getThrowable() instanceof AuthenticationFailedException) { + $event->setResponse(new Response($this->twig->render('login_failed.html.twig'), 503)); } - - // The cause is chained as $previous, and has already been logged by the - // bundle on the `openid_connect` channel. - $event->setResponse(new Response($this->twig->render('login_failed.html.twig'), 503)); } } ``` -Do not answer such a listener with a redirect back to the login route. That -reintroduces the loop this release exists to remove. - -## The CLI login authenticator is unchanged +## CLI login is unchanged -`CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a -consumed or invalid login token still sends the user to your normal login page. -That path cannot loop — it has no entry point of its own, and the redirect carries -no `loginToken` — and for a single-use token that has already been used, arriving -at the login page is friendlier than an error. If you have a catch around the CLI -login flow specifically, it keeps working. +`CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed +or invalid login token still sends the user to your login page. That path has no +entry point of its own and cannot loop. diff --git a/docs/adr/002-fail-closed-on-authentication-failure.md b/docs/adr/002-fail-closed-on-authentication-failure.md index eaad5a8..bfa4306 100644 --- a/docs/adr/002-fail-closed-on-authentication-failure.md +++ b/docs/adr/002-fail-closed-on-authentication-failure.md @@ -3,170 +3,67 @@ - **Created By:** Ture Gjørup - **Date:** 2026-08-19 - **Decision Maker:** Ture Gjørup (draft — awaits team review) -- **Stakeholders:** Bundle consumers (applications whose users log in via this - bundle); operators of those applications; - `itk-dev/openid-connect-bundle` maintainers +- **Stakeholders:** Bundle consumers; operators of those applications; bundle + maintainers - **Status:** Draft ## Context -On 2026-08-12 `sites.itkdev.dk` became unreachable. The Azure client secret had -expired, so the token exchange returned `invalid_client`. What users saw was not -an error but an endless redirect: +On 2026-08-12 an expired Azure client secret took `sites.itkdev.dk` down as an +endless redirect rather than an error: nine rounds in twenty-five seconds, no error +page, nothing logged. -```text -GET /admin → 302 /openidconnect/login/azure_az - → B2C authorize → Azure AD (silent SSO SUCCEEDS) → valid ?code= -GET /openid-connect/generic?state=…&code=… → 302 /openidconnect/login/azure_az -``` +`onAuthenticationFailure()` threw `AuthenticationException`. Symfony's security +`ExceptionListener` catches those and calls the firewall's entry point, which for +this authenticator redirects to the identity provider — so every failure re-entered +the flow that had just failed. A live SSO session upstream returned a fresh `code` +immediately, so nothing degraded and nothing stopped it. -Nine or more cycles in twenty-five seconds, no error page, nothing in the logs. -The IdP authenticated the user correctly every round; the bundle rejected its own -callback and started again. - -The mechanism is the interaction between two pieces of Symfony: - -1. `OpenIdLoginAuthenticator::onAuthenticationFailure()` throws an - `AuthenticationException`. -2. Symfony's security `ExceptionListener` catches any `AuthenticationException` - raised during a request and calls the firewall's *entry point* to start - authentication. For this firewall the entry point is this same authenticator, - whose `start()` redirects to the identity provider. - -So every failure re-enters the flow that just failed. Because the IdP has a live -SSO session it answers immediately with a fresh `code`, which fails the same way. -Nothing in the cycle degrades, so nothing stops it: no backoff, no counter, no -error surface. The only signal available to an operator was that the site was -unusable. - -`AuthenticatorManager::executeAuthenticator()` catches **only** -`AuthenticationException` (verified in `symfony/security-http` 6.4 and 8.1). An -exception outside that hierarchy propagates to `HttpKernel` and is rendered by the -application's own error handling. - -### Drivers - -- **Functional:** - - A failure that cannot be resolved by retrying must not be retried. Nothing - about a rejected callback improves by asking the identity provider again. - - The failure must reach a human. A 500 with the cause in the log is - actionable; a redirect loop is not. - - Consumers must be able to catch it. Per - [ADR 001](001-marker-interface-exception-hierarchy.md), that means - implementing `OpenIdConnectBundleExceptionInterface`. -- **Non-functional:** - - This changes an exception type thrown from a public method, so it is a - consumer-visible break and belongs in a MAJOR release. - - The 5.1 line already made the *cause* visible through logging. This is about - the *behaviour*, and the two were deliberately separated so the diagnostics - could ship without waiting for a major. +`AuthenticatorManager::executeAuthenticator()` catches only +`AuthenticationException` (verified in `symfony/security-http` 6.4 and 8.1). +Anything else propagates to `HttpKernel`. ## Options Considered -1. **Throw outside the security hierarchy (proposed).** Introduce - `AuthenticationFailedException extends \RuntimeException implements - OpenIdConnectBundleExceptionInterface` and throw it from - `onAuthenticationFailure()`. The firewall does not catch it, so it reaches - `HttpKernel` and the application renders its own error page. - - **Pros:** Removes the loop by construction rather than by counting attempts. - One line of behaviour, no new state. Satisfies the ADR 001 marker contract, - so a consumer can still catch every OIDC failure with one `catch`. The - invariant is expressible as a single test assertion — the thrown type is not - an `AuthenticationException` — which is what stops the loop from returning. - - **Cons:** Consumers see a 500 where they previously saw a redirect. Anyone - who caught `AuthenticationException` around this path must migrate. Requires - a MAJOR release. - -2. **Loop protection: count attempts in the session.** Keep throwing - `AuthenticationException`, but track failures per session and stop redirecting - after N. - - **Pros:** No exception-type change, so no BC break. - - **Cons:** Adds mutable session state to a failure path, and the failure modes - that matter can lose the session (the loop began with a state-validation - failure, and a lost session is one cause of that — the counter would be lost - with it). Picking N is arbitrary; the user still suffers N pointless round - trips through the identity provider first. It treats a symptom. - -3. **Render an error page from the bundle.** Return a `Response` from - `onAuthenticationFailure()` instead of throwing. - - **Pros:** No exception type change; the user sees something intelligible. - - **Cons:** Puts presentation in a bundle that has no templates and no opinion - about the application's layout, and it swallows the failure — the - application's error handling and error reporters never see it. A 500 that the - application already knows how to render and report is better. +1. **Throw outside the security hierarchy (chosen).** The firewall does not catch + it, so the loop is impossible by construction and the invariant is one test + assertion. Costs a MAJOR: consumers catching `AuthenticationException` must + migrate, and users see a 500 instead of a redirect. +2. **Count attempts in the session.** No BC break, but it adds mutable state to a + failure path that can itself lose the session, N is arbitrary, and the user + still makes N pointless round trips first. Treats the symptom. +3. **Return an error `Response` from the handler.** Puts presentation in a bundle + with no templates, and swallows the failure so error reporters never see it. ## Decision -**Adopt Option 1**, in **6.0.0**. - -`OpenIdLoginAuthenticator::onAuthenticationFailure()` throws -`AuthenticationFailedException`, chaining the original exception as `$previous` -and preserving its message and code. +Adopt option 1 in 6.0.0. `OpenIdLoginAuthenticator::onAuthenticationFailure()` +throws `AuthenticationFailedException` — a `\RuntimeException` implementing the +ADR 001 marker — chaining the cause. -### Scope: the OIDC authenticator only +**Scope: the OIDC authenticator only.** `CliLoginTokenAuthenticator` is not an +entry point, so its failures redirect to normal login and cannot loop: `supports()` +requires a `loginToken` the redirect does not carry. For a consumed single-use +token that is friendlier than a 500. The loop needs an authenticator whose own +entry point re-triggers it. -`CliLoginTokenAuthenticator` keeps throwing `AuthenticationException`, and that is -deliberate rather than an oversight. It is not an -`AuthenticationEntryPointInterface`, so its failures fall through to the -firewall's entry point — the OIDC authenticator — and redirect to normal login. -That cannot loop: `supports()` requires a `loginToken` query parameter, and the -redirect does not carry one. For a single-use CLI token that has already been -consumed, arriving at the normal login page is a better outcome than a 500. - -The distinction to keep in mind is that the loop needs an authenticator whose own -entry point re-triggers it. Only the OIDC authenticator has that shape. - -### Not logged at the failure handler - -`AuthenticatorManager` already logs the original exception before substituting a -sanitised one, `validateClaims()` logs the specific reason on the -`openid_connect` channel, and the application's error handling logs whatever -escapes. A record here would be the fourth for one failure. +**Not logged here.** `AuthenticatorManager` logs the original exception, +`validateClaims()` logs the specific reason, and the application logs what escapes. ## Consequences -### Positive - -- The loop cannot recur. It is prevented by the type of the exception, not by - configuration, a counter, or an operator remembering something. -- A failing callback surfaces as an error the application already knows how to - render, report and alert on. -- Paired with the 5.1 logging, an expired client secret now produces: a - `warning`/`error` naming the cause at the callback, an application-level error - for the escaping exception, and — where `client_secret_expires_at` is - configured — a `critical` before it expires at all. - -### Negative / Trade-offs - -- **Consumer-visible break.** A consuming application that catches - `AuthenticationException` around the callback no longer catches this. Migration - is mechanical: catch `OpenIdConnectBundleExceptionInterface`, or let it become a - 500. Covered in `UPGRADE-6.0.md`. -- Users see the application's error page rather than being bounced back to login. - For a genuinely broken configuration that is the point; for a transient IdP - failure it is less forgiving than a retry. Judged the right trade, because the - bundle cannot tell the two apart and the retry path is what produced the outage. -- Applications wanting something friendlier than a 500 must add an exception - listener. Documented, but it is work pushed onto consumers. - -### Follow-up Actions - -- [ ] `UPGRADE-6.0.md` — consumer migration notes for the exception type -- [ ] Consider whether `CliLoginTokenAuthenticator` should gain its own entry - point, which would make its failure handling a separate decision rather - than an inherited one -- [ ] Revisit whether the bundle should offer an opt-in error-page listener, if - several consumers end up writing the same one +- The loop cannot recur. It is prevented by a type, not by configuration or a + counter. +- Consumers catching `AuthenticationException` around the callback must switch to + `OpenIdConnectBundleExceptionInterface`. See `UPGRADE-6.0.md`. +- A transient identity-provider failure is now an error rather than a silent retry. + Accepted: the bundle cannot tell transient from permanent, and retrying is what + caused the outage. +- Applications wanting better than a 500 add an exception listener. ## References -- Symfony `AuthenticatorManager::executeAuthenticator()` — catches only - `AuthenticationException`: - -- Symfony security `ExceptionListener` — converts an `AuthenticationException` - into a call to the firewall entry point: - -- [ADR 001](001-marker-interface-exception-hierarchy.md) — the marker-interface - exception contract this new concrete follows -- Keep a Changelog: -- Semantic Versioning 2.0.0: +- [ADR 001](001-marker-interface-exception-hierarchy.md) — the marker contract this + concrete follows +- `AuthenticatorManager` and the security `ExceptionListener` in + `symfony/security-http` From 53b30b226b1df1422d5954929b09f2e8f6afbc8b Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 19 Aug 2026 16:17:20 +0200 Subject: [PATCH 03/28] fix: keep the failure cause outside the security hierarchy The security ExceptionListener walks the whole $previous chain, so chaining the AuthenticationException it hands us re-entered the entry point and the loop survived the type change. Chain the first cause below it instead. Reproduced through a booted firewall, which is what a unit test on the thrown type could not show. --- CHANGELOG.md | 3 + UPGRADE-6.0.md | 4 + ...2-fail-closed-on-authentication-failure.md | 13 ++- src/Security/OpenIdLoginAuthenticator.php | 35 +++++- ...ItkDevOpenIdConnectBundleTestingKernel.php | 9 ++ tests/Security/ConsumerAuthenticator.php | 56 +++++++++ .../FailedCallbackDoesNotLoopTest.php | 108 ++++++++++++++++++ .../Security/OpenIdLoginAuthenticatorTest.php | 37 +++++- tests/Security/ProtectedController.php | 17 +++ tests/config/framework_routing.yml | 6 + tests/config/routes.yml | 3 + tests/config/security_consumer.yml | 21 ++++ 12 files changed, 309 insertions(+), 3 deletions(-) create mode 100644 tests/Security/ConsumerAuthenticator.php create mode 100644 tests/Security/FailedCallbackDoesNotLoopTest.php create mode 100644 tests/Security/ProtectedController.php create mode 100644 tests/config/framework_routing.yml create mode 100644 tests/config/routes.yml create mode 100644 tests/config/security_consumer.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a05c18..453c7b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `docs/adr/002-fail-closed-on-authentication-failure.md`. `CliLoginTokenAuthenticator` is unchanged: it has no entry point of its own, so it cannot loop. +- `getPrevious()` on that exception is the underlying OpenID Connect exception + rather than the `AuthenticationException`, which the security component would + have followed straight back into the loop. ## [5.1.1] - 2026-08-19 diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index 2163166..724c6b1 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -18,6 +18,10 @@ See [ADR 002](docs/adr/002-fail-closed-on-authentication-failure.md). + } catch (\ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface $e) { ``` +`getPrevious()` on the new exception is the underlying OpenID Connect exception, +not Symfony's `AuthenticationException`: the security listener follows the chain, +so one left there would loop again. + If you catch nothing today, no code change is needed. Check that a failed login renders an acceptable error and that your error reporting picks it up. diff --git a/docs/adr/002-fail-closed-on-authentication-failure.md b/docs/adr/002-fail-closed-on-authentication-failure.md index bfa4306..05a58f0 100644 --- a/docs/adr/002-fail-closed-on-authentication-failure.md +++ b/docs/adr/002-fail-closed-on-authentication-failure.md @@ -39,7 +39,16 @@ Anything else propagates to `HttpKernel`. Adopt option 1 in 6.0.0. `OpenIdLoginAuthenticator::onAuthenticationFailure()` throws `AuthenticationFailedException` — a `\RuntimeException` implementing the -ADR 001 marker — chaining the cause. +ADR 001 marker. + +**The cause has to stay outside the hierarchy as well.** `ExceptionListener` walks +the whole `$previous` chain, so chaining the `AuthenticationException` it handed us +re-enters the entry point exactly as throwing one would. The bundle chains the +first cause below it instead — the library exception that says why the callback +failed — and nothing at all when the chain holds only security exceptions. A +knowing departure from ADR 001's "always pass `$previous`": the chain is kept as +far as it can be without restoring the loop, and the message carries the original +text either way. **Scope: the OIDC authenticator only.** `CliLoginTokenAuthenticator` is not an entry point, so its failures redirect to normal login and cannot loop: `supports()` @@ -56,6 +65,8 @@ entry point re-triggers it. counter. - Consumers catching `AuthenticationException` around the callback must switch to `OpenIdConnectBundleExceptionInterface`. See `UPGRADE-6.0.md`. +- `getPrevious()` is the underlying OpenID Connect exception, not the + `AuthenticationException` the firewall raised. - A transient identity-provider failure is now an error rather than a silent retry. Accepted: the bundle cannot tell transient from permanent, and retrying is what caused the outage. diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index a213735..c198086 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -158,6 +158,39 @@ public function onAuthenticationFailure(Request $request, AuthenticationExceptio // original exception, validateClaims() logged the specific reason, and the // application's error handling logs whatever escapes. A record here would be // the fourth for one failure. - throw new AuthenticationFailedException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), $exception); + throw new AuthenticationFailedException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), self::causeOutsideSecurity($exception)); + } + + /** + * The first cause carrying no `AuthenticationException` anywhere beneath it. + * + * Changing the thrown type is not enough on its own: the security + * `ExceptionListener` walks the whole `$previous` chain, so chaining the + * `AuthenticationException` it handed us would put one back within reach and it + * would redirect to the entry point regardless — the loop restored by the cause + * instead of by the type. The library exception underneath carries the reason + * worth keeping, and `validateClaims()` has already logged it with the full + * chain attached. + */ + private static function causeOutsideSecurity(\Throwable $exception): ?\Throwable + { + for ($cause = $exception->getPrevious(); null !== $cause; $cause = $cause->getPrevious()) { + if (!self::containsSecurityException($cause)) { + return $cause; + } + } + + return null; + } + + private static function containsSecurityException(\Throwable $exception): bool + { + for ($current = $exception; null !== $current; $current = $current->getPrevious()) { + if ($current instanceof AuthenticationException) { + return true; + } + } + + return false; } } diff --git a/tests/ItkDevOpenIdConnectBundleTestingKernel.php b/tests/ItkDevOpenIdConnectBundleTestingKernel.php index ff0acb0..6d61abe 100644 --- a/tests/ItkDevOpenIdConnectBundleTestingKernel.php +++ b/tests/ItkDevOpenIdConnectBundleTestingKernel.php @@ -8,6 +8,8 @@ namespace ItkDev\OpenIdConnectBundle\Tests; use ItkDev\OpenIdConnectBundle\ItkDevOpenIdConnectBundle; +use ItkDev\OpenIdConnectBundle\Tests\Security\ConsumerAuthenticator; +use ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController; use ItkDev\OpenIdConnectBundle\Tests\Security\TestAuthenticator; use Symfony\Bundle\FrameworkBundle\FrameworkBundle; use Symfony\Bundle\SecurityBundle\SecurityBundle; @@ -59,6 +61,13 @@ public function registerContainerConfiguration(LoaderInterface $loader): void { $loader->load(function (ContainerBuilder $builder) { $builder->register(TestAuthenticator::class, TestAuthenticator::class); + // Autowired and autoconfigured, the way a consumer registers its own + // authenticator: autoconfiguration is what delivers the configured logger + // to `setLogger()`, and without it this fixture gets a NullLogger. + $builder->register(ConsumerAuthenticator::class, ConsumerAuthenticator::class) + ->setAutowired(true) + ->setAutoconfigured(true); + $builder->register(ProtectedController::class, ProtectedController::class)->setPublic(true); // Available as a logger a config fixture can point at, so a test can // read what the bundle actually wrote through the container. $builder->register(TestLogger::class, TestLogger::class)->setPublic(true); diff --git a/tests/Security/ConsumerAuthenticator.php b/tests/Security/ConsumerAuthenticator.php new file mode 100644 index 0000000..373bf34 --- /dev/null +++ b/tests/Security/ConsumerAuthenticator.php @@ -0,0 +1,56 @@ +validateClaims($request); + } catch (OpenIdConnectExceptionInterface $exception) { + throw new CustomUserMessageAuthenticationException($exception->getMessage(), [], 0, $exception); + } + + return new SelfValidatingPassport( + new UserBadge( + $claims['email'], + fn (string $email) => new TestUser($email) + ) + ); + } + + public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response + { + return null; + } + + public function start(Request $request, ?AuthenticationException $authException = null): Response + { + return new RedirectResponse(self::LOGIN_PATH); + } +} diff --git a/tests/Security/FailedCallbackDoesNotLoopTest.php b/tests/Security/FailedCallbackDoesNotLoopTest.php new file mode 100644 index 0000000..c927dfd --- /dev/null +++ b/tests/Security/FailedCallbackDoesNotLoopTest.php @@ -0,0 +1,108 @@ +kernel = new ItkDevOpenIdConnectBundleTestingKernel([ + __DIR__.'/../config/framework.yml', + __DIR__.'/../config/framework_routing.yml', + __DIR__.'/../config/security_consumer.yml', + __DIR__.'/../config/itkdev_openid_connect.yml', + ]); + $this->kernel->boot(); + } + + /** + * A callback whose state does not match the session: the shape of every + * failure the outage produced, an expired client secret included. + */ + private function failingCallback(): Request + { + $request = Request::create('/protected?state=does-not-match&code=some-code'); + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2provider', 'test_provider_1'); + $session->set('oauth2state', 'the-real-state'); + $session->set('oauth2nonce', 'the-real-nonce'); + $request->setSession($session); + + return $request; + } + + /** + * Guards against the whole test passing vacuously. An unroutable path or a + * firewall that does not match gives a 500 with no redirect too, and every + * assertion below would then hold for the wrong reason. `validateClaims()` + * publishes the provider it resolved, so the attribute is proof it ran. + */ + private function assertTheAuthenticatorRejectedTheCallback(Request $request): void + { + $this->assertSame( + 'test_provider_1', + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'The request never reached validateClaims(), so this test proves nothing.' + ); + } + + public function testAFailedCallbackIsNotAnsweredWithARedirect(): void + { + $request = $this->failingCallback(); + $response = $this->kernel->handle($request, catch: true); + + $this->assertTheAuthenticatorRejectedTheCallback($request); + $this->assertNull( + $response->headers->get('Location'), + 'A failed callback was answered with a redirect: the firewall re-entered its entry point and the loop is back.' + ); + $this->assertSame( + Response::HTTP_INTERNAL_SERVER_ERROR, + $response->getStatusCode(), + 'The failure should surface as an error the application renders.' + ); + } + + public function testTheExceptionAndItsWholeCauseChainStayOutsideTheSecurityHierarchy(): void + { + $request = $this->failingCallback(); + + try { + $this->kernel->handle($request, catch: false); + $this->fail('A failed callback should not be handled silently.'); + } catch (AuthenticationFailedException $exception) { + $this->assertTheAuthenticatorRejectedTheCallback($request); + + for ($cause = $exception; null !== $cause; $cause = $cause->getPrevious()) { + $this->assertNotInstanceOf( + AuthenticationException::class, + $cause, + 'An AuthenticationException in the chain is enough for the ExceptionListener to redirect: it walks $previous.' + ); + } + + $this->assertStringContainsString('Invalid state', $exception->getMessage(), 'The cause is still reported'); + } + } +} diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index c77c309..53f6c16 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -13,6 +13,7 @@ use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; use Psr\Log\LogLevel; @@ -82,7 +83,12 @@ public function testOnAuthenticationFailureThrowsOutsideTheSecurityHierarchy(): ); $this->assertInstanceOf(AuthenticationFailedException::class, $thrown); - $this->assertSame($cause, $thrown->getPrevious(), 'Original exception must be chained as previous'); + // Not chained, even though ADR 001 asks for a cause: the security + // ExceptionListener walks the whole $previous chain, so an + // AuthenticationException reachable through it is caught and turned back + // into a redirect exactly as if it had been thrown directly. The message + // carries the reason instead. + $this->assertNull($thrown->getPrevious(), 'An AuthenticationException must not be reachable through the chain'); $this->assertStringContainsString('Original cause message', $thrown->getMessage(), 'Cause message must be preserved for logs'); // Deliberately no record: the framework already logs the original @@ -92,6 +98,35 @@ public function testOnAuthenticationFailureThrowsOutsideTheSecurityHierarchy(): } } + /** + * The chain is dropped only as far as it has to be. A library exception below + * the AuthenticationException is what says *why* the callback failed, and it + * is safe to keep because the listener does not act on it. + */ + #[DataProvider('causeChainProvider')] + public function testACauseOutsideTheSecurityHierarchyIsKept(\Throwable $cause, ?\Throwable $expected): void + { + try { + $this->authenticator->onAuthenticationFailure(new Request(), new AuthenticationException('Sanitised by the firewall', 0, $cause)); + $this->fail('Expected AuthenticationFailedException'); + } catch (\Throwable $thrown) { + $this->assertSame($expected, $thrown->getPrevious()); + } + } + + /** + * @return iterable + */ + public static function causeChainProvider(): iterable + { + $root = new ValidationException('Invalid state'); + + yield 'library cause is kept' => [$root, $root]; + // The firewall wraps more than once in places, so one skip is not enough. + yield 'reached past nested security exceptions' => [new AuthenticationException('inner', 0, $root), $root]; + yield 'nothing left to keep' => [new AuthenticationException('inner'), null]; + } + public function testUnknownProviderIsLoggedAndRethrown(): void { $cause = new InvalidProviderException('Invalid provider: test_provider_1'); diff --git a/tests/Security/ProtectedController.php b/tests/Security/ProtectedController.php new file mode 100644 index 0000000..3dd7c30 --- /dev/null +++ b/tests/Security/ProtectedController.php @@ -0,0 +1,17 @@ + Date: Wed, 19 Aug 2026 16:32:53 +0200 Subject: [PATCH 04/28] docs: address review on fail-closed - warn against rendering the exception message, which now reaches consumers - make the listener example paste-runnable - note in the changelog that 6.0 is incomplete until the expiry option is required - ADR 002 to Accepted - cover a library cause that hides a security exception beneath it - say why the functional test narrows where the unit test must not --- CHANGELOG.md | 4 ++++ UPGRADE-6.0.md | 8 ++++++++ docs/adr/002-fail-closed-on-authentication-failure.md | 4 ++-- tests/Security/FailedCallbackDoesNotLoopTest.php | 5 +++++ tests/Security/OpenIdLoginAuthenticatorTest.php | 7 +++++++ 5 files changed, 26 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 453c7b7..f3c58ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +> **Note** +> Do not tag from here until `client_secret_expires_at` is required. The breaking +> changes below are 6.0.0 and incomplete on their own. + ### Changed (BREAKING) - A failed OpenID Connect callback now throws diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index 724c6b1..aabb66b 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -34,6 +34,10 @@ reintroduces the loop. #[AsEventListener] final class LoginFailureListener { + public function __construct(private Environment $twig) + { + } + public function __invoke(ExceptionEvent $event): void { if ($event->getThrowable() instanceof AuthenticationFailedException) { @@ -43,6 +47,10 @@ final class LoginFailureListener } ``` +Render your own template, as above, rather than `getMessage()`. The message carries +the identity provider's error text, which the security component used to reduce to +a safe message key before anything could display it. + ## CLI login is unchanged `CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed diff --git a/docs/adr/002-fail-closed-on-authentication-failure.md b/docs/adr/002-fail-closed-on-authentication-failure.md index 05a58f0..240c56e 100644 --- a/docs/adr/002-fail-closed-on-authentication-failure.md +++ b/docs/adr/002-fail-closed-on-authentication-failure.md @@ -2,10 +2,10 @@ - **Created By:** Ture Gjørup - **Date:** 2026-08-19 -- **Decision Maker:** Ture Gjørup (draft — awaits team review) +- **Decision Maker:** Ture Gjørup - **Stakeholders:** Bundle consumers; operators of those applications; bundle maintainers -- **Status:** Draft +- **Status:** Accepted ## Context diff --git a/tests/Security/FailedCallbackDoesNotLoopTest.php b/tests/Security/FailedCallbackDoesNotLoopTest.php index c927dfd..d25a390 100644 --- a/tests/Security/FailedCallbackDoesNotLoopTest.php +++ b/tests/Security/FailedCallbackDoesNotLoopTest.php @@ -92,6 +92,11 @@ public function testTheExceptionAndItsWholeCauseChainStayOutsideTheSecurityHiera $this->kernel->handle($request, catch: false); $this->fail('A failed callback should not be handled silently.'); } catch (AuthenticationFailedException $exception) { + // Catching the concrete type narrows it statically, which the unit test + // in OpenIdLoginAuthenticatorTest deliberately avoids. That guard belongs + // there and this test does not repeat it: what is under test here is the + // chain, and catching the type is how we get hold of it. Do not "align" + // the two tests by moving the narrowing into that one. $this->assertTheAuthenticatorRejectedTheCallback($request); for ($cause = $exception; null !== $cause; $cause = $cause->getPrevious()) { diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 53f6c16..939623a 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -124,6 +124,13 @@ public static function causeChainProvider(): iterable yield 'library cause is kept' => [$root, $root]; // The firewall wraps more than once in places, so one skip is not enough. yield 'reached past nested security exceptions' => [new AuthenticationException('inner', 0, $root), $root]; + // A library exception is not safe merely by being one: skipping only the + // leading security exceptions would keep this outer cause and leave an + // AuthenticationException reachable one level further down. + yield 'library cause hiding a security exception is skipped too' => [ + new ValidationException('outer', 0, new AuthenticationException('inner', 0, $root)), + $root, + ]; yield 'nothing left to keep' => [new AuthenticationException('inner'), null]; } From d5be021a564ed787b65bb97e8a0f80061918e75b Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 19 Aug 2026 16:44:33 +0200 Subject: [PATCH 05/28] fix: settle which logger an authenticator receives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FrameworkBundle autoconfigures a setLogger() call onto every LoggerAwareInterface service, so every OpenIdLoginAuthenticator subclass gets two: the bundle's, for the configured logger, and the application logger. The last call wins, and which one that is depends on the order the bundles were registered. The conventional order puts FrameworkBundle first and the configured logger wins; register this bundle first and logging_options.logger silently does nothing — including itkdev_openid_connect.null_logger, the documented way to switch logging off. Method calls carry no priority, so a compiler pass rewrites them after the instanceof conditionals are resolved. It recognises definitions by the call the bundle put there rather than by class, since checking classes would autoload every class in a consumer's container. Also key the test kernel's cache directory on the pid. Infection substitutes a mutant through an include interceptor rather than on disk, so nothing Symfony tracks as a resource changes and mutants were served a cached container: every mutation of compile-time code survived. Three did, and are now killed. --- CHANGELOG.md | 9 + infection.json5 | 9 + .../Compiler/ConfiguredLoggerPass.php | 106 +++++++++++ .../ItkDevOpenIdConnectExtension.php | 9 +- src/ItkDevOpenIdConnectBundle.php | 13 ++ .../ConfiguredLoggerPassTest.php | 166 ++++++++++++++++++ ...ItkDevOpenIdConnectBundleTestingKernel.php | 32 +++- ...tkdev_openid_connect_configured_logger.yml | 22 +++ 8 files changed, 357 insertions(+), 9 deletions(-) create mode 100644 src/DependencyInjection/Compiler/ConfiguredLoggerPass.php create mode 100644 tests/DependencyInjection/ConfiguredLoggerPassTest.php create mode 100644 tests/config/itkdev_openid_connect_configured_logger.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index f3c58ec..ee51fdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 > Do not tag from here until `client_secret_expires_at` is required. The breaking > changes below are 6.0.0 and incomplete on their own. +### Fixed + +- `logging_options.logger` no longer depends on bundle registration order. + FrameworkBundle autoconfigures a `setLogger()` call onto every + `LoggerAwareInterface` service and the last call wins, so an application + registering this bundle before FrameworkBundle received the application logger + instead of the configured one — `itkdev_openid_connect.null_logger` included. + The conventional order, FrameworkBundle first, was unaffected. + ### Changed (BREAKING) - A failed OpenID Connect callback now throws diff --git a/infection.json5 b/infection.json5 index 6686ea2..b3b81d2 100644 --- a/infection.json5 +++ b/infection.json5 @@ -24,6 +24,15 @@ // typed int property then coerces, so the mutant is behaviourally // identical and cannot be killed by a test. The cast stays because // PHPStan at max level requires it. + // `Bundle::build()` is empty in Symfony, so dropping the parent call changes + // nothing observable. It stays because a future Symfony may put something + // there. Scoped to the call itself: the `addCompilerPass()` below it is a + // real mutant and is killed by a test. + "MethodCallRemoval": { + "ignoreSourceCodeByRegex": [ + "parent::build\\(\\$container\\);" + ] + }, "CastInt": { "ignore": [ "ItkDev\\OpenIdConnectBundle\\Util\\ClientSecretExpiryChecker::getStatus" diff --git a/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php b/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php new file mode 100644 index 0000000..e247c03 --- /dev/null +++ b/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php @@ -0,0 +1,106 @@ +hasParameter(self::LOGGER_PARAMETER)) { + return; + } + + $loggerId = $container->getParameter(self::LOGGER_PARAMETER); + $container->getParameterBag()->remove(self::LOGGER_PARAMETER); + + if (!is_string($loggerId)) { + return; + } + + foreach ($container->getDefinitions() as $definition) { + $this->giveTheConfiguredLoggerTheLastWord($definition, $loggerId); + } + } + + /** + * Definitions are recognised by the call the bundle put there, not by their + * class. Checking classes would mean autoloading every class in the container, + * which is fatal for a consumer whose container names a class from a package it + * has not installed. + */ + private function giveTheConfiguredLoggerTheLastWord(Definition $definition, string $loggerId): void + { + /** @var list $calls */ + $calls = $definition->getMethodCalls(); + + $kept = []; + $wanted = false; + + foreach ($calls as $call) { + if (self::METHOD === $call[0]) { + // Every existing call goes, the configured one being appended below. + // Keeping FrameworkBundle's would put it after ours again. + $wanted = $wanted || $this->references($call[1], $loggerId); + + continue; + } + + $kept[] = $call; + } + + // Without the bundle's own call this is some other LoggerAware service, or an + // authenticator whose consumer turned autoconfiguration off — which the README + // documents as opting out of the bundle's logging. Neither is ours to change, + // and returning here leaves the definition exactly as it was. + if (!$wanted) { + return; + } + + $kept[] = [self::METHOD, [new Reference($loggerId)]]; + + $definition->setMethodCalls($kept); + } + + /** + * @param mixed[] $arguments + */ + private function references(array $arguments, string $loggerId): bool + { + return ($arguments[0] ?? null) instanceof Reference && $loggerId === (string) $arguments[0]; + } +} diff --git a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php index e906f08..d59b068 100644 --- a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php +++ b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php @@ -4,6 +4,7 @@ use ItkDev\OpenIdConnectBundle\Command\UserLoginCommand; use ItkDev\OpenIdConnectBundle\Controller\LoginController; +use ItkDev\OpenIdConnectBundle\DependencyInjection\Compiler\ConfiguredLoggerPass; use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; use ItkDev\OpenIdConnectBundle\Log\AuthenticationAuditLogger; use ItkDev\OpenIdConnectBundle\Security\CliLoginTokenAuthenticator; @@ -103,10 +104,14 @@ private function configureLogging(ContainerBuilder $container, array $options): // `OpenIdLoginAuthenticator` is abstract and subclassed by the consuming // application, so its subclasses are services this extension cannot name. - // Autoconfiguration reaches them, and runs after FrameworkBundle's own - // LoggerAwareInterface pass, so the configured logger wins. + // Autoconfiguration reaches them. $container->registerForAutoconfiguration(OpenIdLoginAuthenticator::class) ->addMethodCall('setLogger', [$logger]); + + // Whether this call or FrameworkBundle's is the one that takes effect depends + // on bundle registration order: see ConfiguredLoggerPass, which reads this + // parameter and settles it. + $container->setParameter(ConfiguredLoggerPass::LOGGER_PARAMETER, $options['logger']); } /** diff --git a/src/ItkDevOpenIdConnectBundle.php b/src/ItkDevOpenIdConnectBundle.php index 7090f97..206861f 100644 --- a/src/ItkDevOpenIdConnectBundle.php +++ b/src/ItkDevOpenIdConnectBundle.php @@ -2,7 +2,10 @@ namespace ItkDev\OpenIdConnectBundle; +use ItkDev\OpenIdConnectBundle\DependencyInjection\Compiler\ConfiguredLoggerPass; use ItkDev\OpenIdConnectBundle\DependencyInjection\ItkDevOpenIdConnectExtension; +use Symfony\Component\DependencyInjection\Compiler\PassConfig; +use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Extension\ExtensionInterface; use Symfony\Component\HttpKernel\Bundle\Bundle; @@ -26,6 +29,16 @@ public function getContainerExtension(): ?ExtensionInterface return $this->extension; } + #[\Override] + public function build(ContainerBuilder $container): void + { + parent::build($container); + + // Runs after the instanceof conditionals it has to correct, and does nothing + // unless the extension recorded a configured logger. + $container->addCompilerPass(new ConfiguredLoggerPass(), PassConfig::TYPE_BEFORE_REMOVING); + } + #[\Override] public function getPath(): string { diff --git a/tests/DependencyInjection/ConfiguredLoggerPassTest.php b/tests/DependencyInjection/ConfiguredLoggerPassTest.php new file mode 100644 index 0000000..1b7bba9 --- /dev/null +++ b/tests/DependencyInjection/ConfiguredLoggerPassTest.php @@ -0,0 +1,166 @@ +kernel = new ItkDevOpenIdConnectBundleTestingKernel([ + __DIR__.'/../config/framework.yml', + __DIR__.'/../config/framework_routing.yml', + __DIR__.'/../config/security_consumer.yml', + __DIR__.'/../config/itkdev_openid_connect_configured_logger.yml', + ]); + $this->kernel->boot(); + } + + private function configuredLogger(): TestLogger + { + $logger = $this->kernel->getContainer()->get(TestLogger::class); + $this->assertInstanceOf(TestLogger::class, $logger); + + return $logger; + } + + public function testTheBuiltAuthenticatorHoldsTheConfiguredLogger(): void + { + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $logger = (new \ReflectionProperty(OpenIdLoginAuthenticator::class, 'logger'))->getValue($authenticator); + + $this->assertSame( + $this->configuredLogger(), + $logger, + 'The application logger overwrote the configured one, so logging_options.logger turns on bundle registration order.' + ); + } + + /** + * The same thing again through behaviour, because holding the right object and + * writing to it are not quite the same claim. + */ + public function testAFailedLoginIsWrittenToTheConfiguredLogger(): void + { + $request = Request::create('/protected?state=does-not-match&code=some-code'); + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2provider', 'test_provider_1'); + $session->set('oauth2state', 'the-real-state'); + $session->set('oauth2nonce', 'the-real-nonce'); + $request->setSession($session); + + $this->kernel->handle($request, catch: true); + + $this->assertContains( + 'OIDC login failed: invalid state', + array_column($this->configuredLogger()->records, 'message'), + ); + } + + /** + * A service that opts out of autoconfiguration keeps its NullLogger: the pass + * must not decide to start logging on a consumer's behalf. + */ + public function testAServiceWithoutAutoconfigurationIsLeftAlone(): void + { + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class.'.not_autoconfigured'); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $logger = (new \ReflectionProperty(OpenIdLoginAuthenticator::class, 'logger'))->getValue($authenticator); + + $this->assertInstanceOf(LoggerInterface::class, $logger); + $this->assertNotSame($this->configuredLogger(), $logger); + } + + public function testTheParameterDoesNotLingerInTheContainer(): void + { + $this->assertFalse( + $this->kernel->getContainer()->hasParameter(ConfiguredLoggerPass::LOGGER_PARAMETER), + 'The pass consumes its parameter rather than leaving it in a consumer container' + ); + } + + /** + * @param array}> $calls + */ + private function process(array $calls, string|array|null $parameter = 'configured.logger'): Definition + { + $container = new ContainerBuilder(); + if (null !== $parameter) { + $container->setParameter(ConfiguredLoggerPass::LOGGER_PARAMETER, $parameter); + } + $definition = $container->register('some.service', \stdClass::class); + $definition->setMethodCalls($calls); + + (new ConfiguredLoggerPass())->process($container); + + return $definition; + } + + public function testAParameterThatIsNotAServiceIdIsIgnored(): void + { + $calls = [['setLogger', [new Reference('configured.logger')]]]; + + $this->assertEquals($calls, $this->process($calls, parameter: ['not', 'a', 'service id'])->getMethodCalls()); + } + + public function testNoParameterMeansNoConfiguredLogger(): void + { + $calls = [['setLogger', [new Reference('logger')]]]; + + $this->assertEquals($calls, $this->process($calls, parameter: null)->getMethodCalls()); + } + + public function testAnUnrelatedLoggerAwareServiceKeepsItsCall(): void + { + // Every LoggerAware service in the application carries this call. Only the + // ones the bundle also wrote to are ours to rewrite. + $calls = [['setLogger', [new Reference('logger')]]]; + + $this->assertEquals($calls, $this->process($calls)->getMethodCalls()); + } + + public function testTheConfiguredCallIsMovedLastAndTheOtherOneDropped(): void + { + $definition = $this->process([ + ['setLogger', [new Reference('configured.logger')]], + ['setDependency', []], + ['setLogger', [new Reference('logger')]], + ]); + + $this->assertEquals([ + ['setDependency', []], + ['setLogger', [new Reference('configured.logger')]], + ], $definition->getMethodCalls()); + } +} diff --git a/tests/ItkDevOpenIdConnectBundleTestingKernel.php b/tests/ItkDevOpenIdConnectBundleTestingKernel.php index 6d61abe..1687fb3 100644 --- a/tests/ItkDevOpenIdConnectBundleTestingKernel.php +++ b/tests/ItkDevOpenIdConnectBundleTestingKernel.php @@ -32,19 +32,31 @@ public function __construct( } /** - * A cache directory per config set. + * A cache directory per config set, and per process. * - * Without this every kernel in the suite shares `var/cache/test`, so the first - * container compiled is the one every later test gets — silently, and with - * whatever configuration that first test happened to use. Any test that boots a - * different configuration is then asserting against the wrong container. + * Per config set, because otherwise every kernel in the suite shares + * `var/cache/test`: the first container compiled is the one every later test + * gets, silently, with whatever configuration that first test happened to use. + * + * Per process, because Infection substitutes a mutated file through an include + * interceptor rather than by writing to disk, so nothing Symfony tracks as a + * resource changes and a cached container is served to the mutant unchanged. + * Every mutation of compile-time code then survives by default. Each mutant runs + * in its own process, so the pid is what distinguishes them. */ #[\Override] public function getCacheDir(): string { - return parent::getCacheDir().'/'.substr(hash('xxh128', implode('|', $this->pathToConfigs)), 0, 12); + $key = hash('xxh128', implode('|', $this->pathToConfigs)); + + return parent::getCacheDir().'/'.substr($key, 0, 12).'-'.getmypid(); } + /** + * This bundle is registered before FrameworkBundle deliberately. It is the + * unconventional order, and the one where autoconfigured method calls land in the + * losing order — so it is the order that holds ConfiguredLoggerPass to its job. + */ public function registerBundles(): iterable { return [ @@ -66,7 +78,13 @@ public function registerContainerConfiguration(LoaderInterface $loader): void // to `setLogger()`, and without it this fixture gets a NullLogger. $builder->register(ConsumerAuthenticator::class, ConsumerAuthenticator::class) ->setAutowired(true) - ->setAutoconfigured(true); + ->setAutoconfigured(true) + ->setPublic(true); + // A consumer who turned autoconfiguration off. Nothing calls setLogger on + // this one, and nothing should start. + $builder->register(ConsumerAuthenticator::class.'.not_autoconfigured', ConsumerAuthenticator::class) + ->setAutowired(true) + ->setPublic(true); $builder->register(ProtectedController::class, ProtectedController::class)->setPublic(true); // Available as a logger a config fixture can point at, so a test can // read what the bundle actually wrote through the container. diff --git a/tests/config/itkdev_openid_connect_configured_logger.yml b/tests/config/itkdev_openid_connect_configured_logger.yml new file mode 100644 index 0000000..d751e39 --- /dev/null +++ b/tests/config/itkdev_openid_connect_configured_logger.yml @@ -0,0 +1,22 @@ +itkdev_openid_connect: + logging_options: + logger: ItkDev\OpenIdConnectBundle\Tests\TestLogger + cache_options: + cache_pool: "cache.array" + cli_login_options: + route: "route_test" + user_provider: "security.user.provider.concrete.test_users_1" + openid_providers: + test_provider_1: + options: + metadata_url: "https://provider.example.org/openid-configuration" + client_id: "test_id" + client_secret: "test_secret" + redirect_uri: "https://app.example.org/callback_uri" + test_provider_2: + options: + metadata_url: "https://provider.example.org/openid-configuration" + client_id: "test_id" + leeway: 5 + client_secret: "test_secret" + redirect_uri: "https://app.example.org/callback_uri" From 05c9c09fe8afd3543d19e746b76f515346ef2eed Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 19 Aug 2026 17:02:20 +0200 Subject: [PATCH 06/28] fix: resolve an alias configured as logging_options.logger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit References in method calls are rewritten to concrete ids by ResolveReferencesToAliasesPass, which runs in the optimization phase ahead of this pass, so comparing them against a configured alias id never matched and the pass no-oped — leaving the ordering dependency it exists to remove, silently, for anyone who configured `logger` or another alias. Emitting the alias instead would have risked a dangling reference, since private aliases are removed afterwards. Also key the test kernel's cache on the pid only under Infection. Plain runs have nothing to isolate and were paying a container recompile per process, plus a directory each that nothing cleaned up. --- README.md | 4 +++ infection.json5 | 8 +++++ .../Compiler/ConfiguredLoggerPass.php | 17 ++++++++++ .../ConfiguredLoggerPassTest.php | 31 +++++++++++++++++-- ...ItkDevOpenIdConnectBundleTestingKernel.php | 24 +++++++++----- .../itkdev_openid_connect_alias_logger.yml | 22 +++++++++++++ 6 files changed, 95 insertions(+), 11 deletions(-) create mode 100644 tests/config/itkdev_openid_connect_alias_logger.yml diff --git a/README.md b/README.md index db3301e..71f5c83 100644 --- a/README.md +++ b/README.md @@ -406,6 +406,10 @@ default for services in `config/services.yaml`. With autoconfiguration disabled the authenticator falls back to a `NullLogger` and logs nothing, while the rest of the bundle keeps logging. +A configured logger also takes precedence over a `setLogger()` call on the +authenticator's own service definition. Disabling autoconfiguration is the way to +wire a logger yourself. + #### Audit logging Separately from the failure logging above, the bundle can write an diff --git a/infection.json5 b/infection.json5 index b3b81d2..0b231c3 100644 --- a/infection.json5 +++ b/infection.json5 @@ -24,6 +24,14 @@ // typed int property then coerces, so the mutant is behaviourally // identical and cannot be killed by a test. The cast stays because // PHPStan at max level requires it. + // Alias::__toString() returns the target id, so dropping the cast leaves an + // Alias object that every consumer of $loggerId coerces to the same string. + // The cast stays because PHPStan at max level requires it. + "CastString": { + "ignoreSourceCodeByRegex": [ + "\\$loggerId = \\(string\\) \\$container->getAlias\\(\\$loggerId\\);" + ] + }, // `Bundle::build()` is empty in Symfony, so dropping the parent call changes // nothing observable. It stays because a future Symfony may put something // there. Scoped to the call itself: the `addCompilerPass()` below it is a diff --git a/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php b/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php index e247c03..d223fa7 100644 --- a/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php +++ b/src/DependencyInjection/Compiler/ConfiguredLoggerPass.php @@ -24,6 +24,12 @@ * * Method calls carry no priority, so making that deterministic means rewriting them * once the instanceof conditionals that produced them have been resolved. + * + * Two consequences of rewriting rather than reordering: the configured logger also + * overrides an explicit `setLogger()` call on the definition, since nothing here can + * tell a consumer's deliberate call from FrameworkBundle's — disable + * autoconfiguration for the service to opt out of all of this — and the call now runs + * last among the setters rather than first. */ final class ConfiguredLoggerPass implements CompilerPassInterface { @@ -52,6 +58,17 @@ public function process(ContainerBuilder $container): void return; } + // References in existing method calls have already been rewritten to concrete + // ids by ResolveReferencesToAliasesPass, which runs in the optimization phase + // ahead of this one. Comparing an alias against them would never match, so a + // logger configured by alias id — `logger` among them — would silently leave + // the ordering unsettled. Emitting the alias would be worse still: private + // aliases are removed after this pass, leaving a dangling reference. + // That pass has already rejected circular aliases, so this terminates. + while ($container->hasAlias($loggerId)) { + $loggerId = (string) $container->getAlias($loggerId); + } + foreach ($container->getDefinitions() as $definition) { $this->giveTheConfiguredLoggerTheLastWord($definition, $loggerId); } diff --git a/tests/DependencyInjection/ConfiguredLoggerPassTest.php b/tests/DependencyInjection/ConfiguredLoggerPassTest.php index 1b7bba9..4dfd3e0 100644 --- a/tests/DependencyInjection/ConfiguredLoggerPassTest.php +++ b/tests/DependencyInjection/ConfiguredLoggerPassTest.php @@ -35,13 +35,20 @@ class ConfiguredLoggerPassTest extends TestCase protected function setUp(): void { - $this->kernel = new ItkDevOpenIdConnectBundleTestingKernel([ + $this->kernel = $this->boot('itkdev_openid_connect_configured_logger.yml'); + } + + private function boot(string $bundleConfig): ItkDevOpenIdConnectBundleTestingKernel + { + $kernel = new ItkDevOpenIdConnectBundleTestingKernel([ __DIR__.'/../config/framework.yml', __DIR__.'/../config/framework_routing.yml', __DIR__.'/../config/security_consumer.yml', - __DIR__.'/../config/itkdev_openid_connect_configured_logger.yml', + __DIR__.'/../config/'.$bundleConfig, ]); - $this->kernel->boot(); + $kernel->boot(); + + return $kernel; } private function configuredLogger(): TestLogger @@ -87,6 +94,24 @@ public function testAFailedLoginIsWrittenToTheConfiguredLogger(): void ); } + /** + * An alias is a service id like any other as far as configuration goes, but by + * the time this pass runs the references it is compared against have already been + * rewritten to the concrete id — so matching on the alias would quietly fail and + * leave exactly the ordering dependency this pass exists to remove. + */ + public function testALoggerConfiguredByAliasIsResolved(): void + { + $this->kernel = $this->boot('itkdev_openid_connect_alias_logger.yml'); + + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $logger = (new \ReflectionProperty(OpenIdLoginAuthenticator::class, 'logger'))->getValue($authenticator); + + $this->assertSame($this->configuredLogger(), $logger); + } + /** * A service that opts out of autoconfiguration keeps its NullLogger: the pass * must not decide to start logging on a consumer's behalf. diff --git a/tests/ItkDevOpenIdConnectBundleTestingKernel.php b/tests/ItkDevOpenIdConnectBundleTestingKernel.php index 1687fb3..589b52f 100644 --- a/tests/ItkDevOpenIdConnectBundleTestingKernel.php +++ b/tests/ItkDevOpenIdConnectBundleTestingKernel.php @@ -32,24 +32,29 @@ public function __construct( } /** - * A cache directory per config set, and per process. + * A cache directory per config set, and under Infection per process. * * Per config set, because otherwise every kernel in the suite shares * `var/cache/test`: the first container compiled is the one every later test * gets, silently, with whatever configuration that first test happened to use. * - * Per process, because Infection substitutes a mutated file through an include - * interceptor rather than by writing to disk, so nothing Symfony tracks as a - * resource changes and a cached container is served to the mutant unchanged. - * Every mutation of compile-time code then survives by default. Each mutant runs - * in its own process, so the pid is what distinguishes them. + * Per process under Infection, because it substitutes a mutated file through an + * include interceptor rather than by writing to disk. Nothing Symfony tracks as a + * resource changes, so a mutant is served the cached container and every mutation + * of compile-time code survives by default. Each mutant runs in its own process, + * so the pid separates them. Plain runs stay on the shared directory: they have + * nothing to isolate, and a recompile per process is a cost with no return. */ #[\Override] public function getCacheDir(): string { - $key = hash('xxh128', implode('|', $this->pathToConfigs)); + $key = substr(hash('xxh128', implode('|', $this->pathToConfigs)), 0, 12); - return parent::getCacheDir().'/'.substr($key, 0, 12).'-'.getmypid(); + if (false !== getenv('INFECTION')) { + $key .= '-'.getmypid(); + } + + return parent::getCacheDir().'/'.$key; } /** @@ -89,6 +94,9 @@ public function registerContainerConfiguration(LoaderInterface $loader): void // Available as a logger a config fixture can point at, so a test can // read what the bundle actually wrote through the container. $builder->register(TestLogger::class, TestLogger::class)->setPublic(true); + // Aliases are resolved out of method calls before this bundle's compiler + // pass runs, so a logger configured by alias needs its own coverage. + $builder->setAlias('test.logger_alias', TestLogger::class); }); foreach ($this->pathToConfigs as $path) { diff --git a/tests/config/itkdev_openid_connect_alias_logger.yml b/tests/config/itkdev_openid_connect_alias_logger.yml new file mode 100644 index 0000000..0a59e9a --- /dev/null +++ b/tests/config/itkdev_openid_connect_alias_logger.yml @@ -0,0 +1,22 @@ +itkdev_openid_connect: + logging_options: + logger: "test.logger_alias" + cache_options: + cache_pool: "cache.array" + cli_login_options: + route: "route_test" + user_provider: "security.user.provider.concrete.test_users_1" + openid_providers: + test_provider_1: + options: + metadata_url: "https://provider.example.org/openid-configuration" + client_id: "test_id" + client_secret: "test_secret" + redirect_uri: "https://app.example.org/callback_uri" + test_provider_2: + options: + metadata_url: "https://provider.example.org/openid-configuration" + client_id: "test_id" + leeway: 5 + client_secret: "test_secret" + redirect_uri: "https://app.example.org/callback_uri" From 73796a64e2243c162e1b9bf2381834bf1d4af0f8 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 19 Aug 2026 17:14:41 +0200 Subject: [PATCH 07/28] feat!: require client_secret_expires_at A missing date now fails while the container compiles instead of emitting the 5.1 deprecation. The bundle cannot warn about an expiry it does not know about, which is how a routine credential rotation became an outage. Removes with it the deprecated ItkOpenIdConnectBundleException, the never-thrown UserDoesNotExistException, and symfony/deprecation-contracts, whose only caller was the deprecation. --- CHANGELOG.md | 18 ++++-- README.md | 17 +++--- UPGRADE-6.0.md | 37 +++++++++++++ composer.json | 1 - src/DependencyInjection/Configuration.php | 3 +- .../ItkDevOpenIdConnectExtension.php | 17 ++---- .../ItkOpenIdConnectBundleException.php | 13 ----- src/Exception/UserDoesNotExistException.php | 7 --- .../DependencyInjection/ConfigurationTest.php | 16 +++++- .../ItkDevOpenIdConnectExtensionTest.php | 55 ------------------- tests/Exception/ExceptionHierarchyTest.php | 19 ------- tests/config/itkdev_openid_connect.yml | 2 + .../itkdev_openid_connect_alias_logger.yml | 2 + ...tkdev_openid_connect_configured_logger.yml | 2 + 14 files changed, 84 insertions(+), 125 deletions(-) delete mode 100644 src/Exception/ItkOpenIdConnectBundleException.php delete mode 100644 src/Exception/UserDoesNotExistException.php diff --git a/CHANGELOG.md b/CHANGELOG.md index ee51fdd..51a6219 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -> **Note** -> Do not tag from here until `client_secret_expires_at` is required. The breaking -> changes below are 6.0.0 and incomplete on their own. - ### Fixed - `logging_options.logger` no longer depends on bundle registration order. @@ -35,6 +31,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `getPrevious()` on that exception is the underlying OpenID Connect exception rather than the `AuthenticationException`, which the security component would have followed straight back into the loop. +- `client_secret_expires_at` is now required for every provider. A missing date + fails while the container compiles instead of emitting the 5.1 deprecation, + since the bundle cannot warn about an expiry it does not know about. See + `UPGRADE-6.0.md`. + +### Removed (BREAKING) + +- `ItkOpenIdConnectBundleException`, `@deprecated` since 5.0. Catch + `OpenIdConnectBundleExceptionInterface` instead. +- `UserDoesNotExistException`, which was thrown nowhere. Symfony's + `UserNotFoundException` covers the case and `UserLoginCommand` already handles + it. `UsernameDoesNotExistException` is unaffected. +- `symfony/deprecation-contracts` from `require`, the last + `trigger_deprecation()` call having gone with the option becoming required. ## [5.1.1] - 2026-08-19 diff --git a/README.md b/README.md index 71f5c83..ecd5a11 100644 --- a/README.md +++ b/README.md @@ -115,9 +115,9 @@ itkdev_openid_connect: metadata_url: '%env(string:ADMIN_OIDC_METADATA_URL)%' client_id: '%env(string:ADMIN_OIDC_CLIENT_ID)%' client_secret: '%env(string:ADMIN_OIDC_CLIENT_SECRET)%' - # Date the client secret expires. An expired secret breaks every login, - # so setting this lets the bundle warn while there is still time to - # rotate. Will be REQUIRED in 6.0. See "Client secret expiry" below. + # Required. Date the client secret expires. An expired secret breaks + # every login, so the bundle warns while there is still time to rotate. + # See "Client secret expiry" below. client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' # Specify redirect URI redirect_uri: '%env(string:ADMIN_OIDC_REDIRECT_URI)%' @@ -226,13 +226,10 @@ For a genuinely expired secret that means the login still fails, at the callback with `invalid_client` — but the `critical` record here and the failure record from the callback together name the cause without anyone having to reproduce it. -Until the date is configured a provider sits in `unknown`, where none of the above -applies and nothing is reported. - -> [!NOTE] -> `client_secret_expires_at` is optional in 5.x and **will be required in 6.0**. -> Providers without it emit a deprecation warning, because the bundle cannot warn -> about an expiry it does not know about. +`client_secret_expires_at` is required, because the bundle cannot warn about an +expiry it does not know about. A provider still reaches `unknown` when the value is +set to something unusable — an environment variable that resolved to nothing, or a +date `strtotime()` cannot read — and that is reported at `error`. ##### Monitoring expiry diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index aabb66b..d25b2f8 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -51,6 +51,43 @@ Render your own template, as above, rather than `getMessage()`. The message carr the identity provider's error text, which the security component used to reduce to a safe message key before anything could display it. +## `client_secret_expires_at` is now required + +Every provider must declare when its client secret expires. Without it the bundle +cannot warn before an expiry takes every login down, which is what happened. + +```yaml +itkdev_openid_connect: + openid_providers: + admin: + options: + client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' +``` + +Anything `strtotime()` understands. A missing key now fails at compile time: + +```text +The child config "client_secret_expires_at" under +"itkdev_openid_connect.openid_providers.admin.options" must be configured +``` + +The value is not trusted as fact — nothing here blocks a login, and a value that +cannot be parsed is reported at `error` and treated as `unknown`. Keep it beside the +secret itself, so rotating one prompts updating the other. The 5.1 deprecation +warning for a missing date is gone with it. + +## Removed exceptions + +| Removed | Use instead | +| --- | --- | +| `ItkOpenIdConnectBundleException` (abstract, `@deprecated` since 5.0) | `OpenIdConnectBundleExceptionInterface` | +| `UserDoesNotExistException` | Symfony's `UserNotFoundException`, which the bundle already handles | + +`UserDoesNotExistException` was thrown nowhere. If your user provider throws it, +switch to `UserNotFoundException`: `UserLoginCommand` catches that and reports the +username as unknown. Not to be confused with `UsernameDoesNotExistException`, which +stays — the CLI authenticator throws it when a token resolves to no username. + ## CLI login is unchanged `CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed diff --git a/composer.json b/composer.json index bb4385b..f925474 100644 --- a/composer.json +++ b/composer.json @@ -22,7 +22,6 @@ "psr/log": "^3.0", "symfony/cache": "^6.4 || ^7.0 || ^8.0", "symfony/clock": "^6.4 || ^7.0 || ^8.0", - "symfony/deprecation-contracts": "^2.5 || ^3.0", "symfony/event-dispatcher": "^6.4 || ^7.0 || ^8.0", "symfony/framework-bundle": "^6.4.13 || ^7.0 || ^8.0", "symfony/security-bundle": "^6.4.13 || ^7.0 || ^8.0", diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index d5372a2..755166b 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -111,6 +111,7 @@ public function getConfigTreeBuilder(): TreeBuilder ->isRequired()->cannotBeEmpty() ->end() ->scalarNode('client_secret_expires_at') + ->isRequired() // No cannotBeEmpty() here, and it cannot come back: // VariableNode::finalizeValue() refuses an environment variable // whenever empty values are disallowed and the node has any @@ -122,7 +123,7 @@ public function getConfigTreeBuilder(): TreeBuilder // ClientSecretExpiryChecker. It never caught whitespace-only // values regardless: ScalarNode::isValueEmpty() is // `null === $value || '' === $value`. - ->info('Date the client secret expires, e.g. "2027-01-31". Anything strtotime() understands, and usually an environment variable. An expired secret breaks every login, so configuring this lets the bundle warn while there is still time to rotate. Will be required in 6.0.') + ->info('Required. Date the client secret expires, e.g. "2027-01-31". Anything strtotime() understands, and usually an environment variable. An expired secret breaks every login, so the bundle warns while there is still time to rotate.') ->defaultNull() ->validate() // '' is exempt because it is the dummy fixture Symfony diff --git a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php index d59b068..bffd6d1 100644 --- a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php +++ b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php @@ -145,7 +145,7 @@ private function configureAuditLogging(ContainerBuilder $container, array $optio } /** - * Wire the expiry checker, and nudge installations that have not set a date. + * Wire the expiry checker. * * @param array}> $providers * @param array{warning_days: int} $options @@ -156,19 +156,10 @@ private function configureSecretExpiry(ContainerBuilder $container, array $provi foreach ($providers as $providerKey => $provider) { $expiresAt = $provider['options']['client_secret_expires_at'] ?? null; + // Required by the configuration, so a null here means an environment + // variable that resolved to something that is not a string. + // ClientSecretExpiryChecker reports that at runtime. $expiryDates[$providerKey] = is_string($expiresAt) ? $expiresAt : null; - - if (null === $expiryDates[$providerKey]) { - // Symfony's setDeprecated() fires when a node *is* used, which is - // the inverse of what is needed: the point is to nudge the - // installations that have not set a date yet. - trigger_deprecation( - 'itk-dev/openid-connect-bundle', - '5.1', - 'Not configuring "client_secret_expires_at" for OIDC provider "%s" is deprecated. Without it the bundle cannot warn before the secret expires, and an expired secret breaks every login. It will be required in 6.0.', - $providerKey, - ); - } } $definition = $container->getDefinition(ClientSecretExpiryChecker::class); diff --git a/src/Exception/ItkOpenIdConnectBundleException.php b/src/Exception/ItkOpenIdConnectBundleException.php deleted file mode 100644 index ce0d69f..0000000 --- a/src/Exception/ItkOpenIdConnectBundleException.php +++ /dev/null @@ -1,13 +0,0 @@ - 'https://example.com/.well-known/openid-configuration', 'client_id' => 'my_id', 'client_secret' => 'my_secret', + 'client_secret_expires_at' => '2027-01-31', ], ], ], @@ -70,8 +71,7 @@ public function testMinimalConfig(): void $this->assertNull($config['audit_options']['logger']); $this->assertSame(AuthenticationAuditLogger::IDENTIFIER_RAW, $config['audit_options']['identifier']); - // No expiry date yet, and a 30-day default warning window. - $this->assertNull($provider['client_secret_expires_at']); + $this->assertSame('2027-01-31', $provider['client_secret_expires_at']); $this->assertSame(30, $config['secret_expiry_options']['warning_days']); } @@ -356,6 +356,17 @@ public function testProviderKeysAreNotNormalized(): void $this->assertArrayNotHasKey('my_provider', $config['openid_providers']); } + public function testTheExpiryDateIsRequired(): void + { + $input = $this->getMinimalConfig(); + unset($input['openid_providers']['provider1']['options']['client_secret_expires_at']); + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('The child config "client_secret_expires_at" under "itkdev_openid_connect.openid_providers.provider1.options" must be configured'); + + $this->processor->processConfiguration($this->configuration, [$input]); + } + public function testMultipleProviders(): void { $input = $this->getMinimalConfig(); @@ -364,6 +375,7 @@ public function testMultipleProviders(): void 'metadata_url' => 'https://other-provider.example.org/.well-known/openid-configuration', 'client_id' => 'other_id', 'client_secret' => 'other_secret', + 'client_secret_expires_at' => '2028-06-30', ], ]; diff --git a/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php b/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php index a0db5a0..0583691 100644 --- a/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php +++ b/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php @@ -218,61 +218,6 @@ public function testExpiryDateIsStrippedBeforeReachingTheProviderManager(): void $this->assertSame('test_secret', $provider['client_secret']); } - public function testMissingExpiryDateTriggersADeprecation(): void - { - $extension = new ItkDevOpenIdConnectExtension(); - $container = new ContainerBuilder(); - - $config = $this->getBaseConfig(); - // Built without the date rather than unset from the base config, which is - // untyped and would need narrowing for no gain. - $config['openid_providers'] = [ - 'test_provider' => [ - 'options' => [ - 'metadata_url' => 'https://example.com/.well-known/openid-configuration', - 'client_id' => 'test_id', - 'client_secret' => 'test_secret', - ], - ], - ]; - - $this->expectUserDeprecationMessage('Since itk-dev/openid-connect-bundle 5.1: Not configuring "client_secret_expires_at" for OIDC provider "test_provider" is deprecated. Without it the bundle cannot warn before the secret expires, and an expired secret breaks every login. It will be required in 6.0.'); - - $extension->load([$config], $container); - - $expiryDates = $container->getDefinition(ClientSecretExpiryChecker::class)->getArgument('$expiryDates'); - $this->assertIsArray($expiryDates); - $this->assertNull($expiryDates['test_provider'], 'An unset date is recorded as unknown, not guessed at'); - } - - public function testConfiguredExpiryDateTriggersNoDeprecation(): void - { - $extension = new ItkDevOpenIdConnectExtension(); - $container = new ContainerBuilder(); - - $deprecations = []; - // All four arguments must be forwarded: the handler being wrapped is - // PHPUnit's, whose __invoke() requires file and line. Passing two worked - // only while nothing else raised an error during the call. - $previous = set_error_handler(static function (int $level, string $message, string $file = '', int $line = 0) use (&$deprecations, &$previous): bool { - if (\E_USER_DEPRECATED === $level) { - $deprecations[] = $message; - - return true; - } - - return null !== $previous && false !== ($previous)($level, $message, $file, $line); - }); - - try { - $extension->load([$this->getBaseConfig()], $container); - } finally { - restore_error_handler(); - } - - $this->assertSame([], $deprecations, 'An installation that has set the date must not be nagged'); - } - public function testLoadWiresProviderManagerConfig(): void { $extension = new ItkDevOpenIdConnectExtension(); diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index 06f4bf1..3e140fd 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -7,10 +7,8 @@ use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\CacheException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; -use ItkDev\OpenIdConnectBundle\Exception\ItkOpenIdConnectBundleException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\TokenNotFoundException; -use ItkDev\OpenIdConnectBundle\Exception\UserDoesNotExistException; use ItkDev\OpenIdConnectBundle\Exception\UsernameDoesNotExistException; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -42,7 +40,6 @@ public static function concreteProvider(): iterable // Runtime conditions → \RuntimeException yield 'CacheException' => [CacheException::class, \RuntimeException::class]; yield 'TokenNotFoundException' => [TokenNotFoundException::class, \RuntimeException::class]; - yield 'UserDoesNotExistException' => [UserDoesNotExistException::class, \RuntimeException::class]; yield 'AuthenticationFailedException' => [AuthenticationFailedException::class, \RuntimeException::class]; } @@ -116,20 +113,4 @@ public function testLibraryMarkerCatchesBothPackages(): void $this->assertSame([LibraryHttpException::class, CacheException::class], $caught); } - - public function testDeprecatedAbstractBaseImplementsBundleMarker(): void - { - // `ItkOpenIdConnectBundleException` is kept as a deprecated alias through 5.x. - // Concrete bundle exceptions no longer extend it, but it still implements the - // marker so any consumer-defined subclass remains catchable via the marker. - // PHPStan can statically prove the assertion holds today; the test exists so - // the day a refactor removes the implements, the failure is loud. - $deprecated = ItkOpenIdConnectBundleException::class; // @phpstan-ignore classConstant.deprecatedClass (the test asserts a property of this deprecated class on purpose) - // @phpstan-ignore method.alreadyNarrowedType (the assertion is the guard — PHPStan proves it today; the test fails the day the guard stops holding) - $this->assertTrue( - // @phpstan-ignore function.alreadyNarrowedType (same as above — the static proof IS the contract being asserted) - is_subclass_of($deprecated, OpenIdConnectBundleExceptionInterface::class), - 'Deprecated abstract base must continue to implement the bundle marker through 5.x.', - ); - } } diff --git a/tests/config/itkdev_openid_connect.yml b/tests/config/itkdev_openid_connect.yml index 59d5054..91f18e8 100644 --- a/tests/config/itkdev_openid_connect.yml +++ b/tests/config/itkdev_openid_connect.yml @@ -10,6 +10,7 @@ itkdev_openid_connect: metadata_url: "https://provider.example.org/openid-configuration" client_id: "test_id" client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" redirect_uri: "https://app.example.org/callback_uri" test_provider_2: options: @@ -17,4 +18,5 @@ itkdev_openid_connect: client_id: "test_id" leeway: 5 client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" redirect_uri: "https://app.example.org/callback_uri" diff --git a/tests/config/itkdev_openid_connect_alias_logger.yml b/tests/config/itkdev_openid_connect_alias_logger.yml index 0a59e9a..73769d2 100644 --- a/tests/config/itkdev_openid_connect_alias_logger.yml +++ b/tests/config/itkdev_openid_connect_alias_logger.yml @@ -12,6 +12,7 @@ itkdev_openid_connect: metadata_url: "https://provider.example.org/openid-configuration" client_id: "test_id" client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" redirect_uri: "https://app.example.org/callback_uri" test_provider_2: options: @@ -19,4 +20,5 @@ itkdev_openid_connect: client_id: "test_id" leeway: 5 client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" redirect_uri: "https://app.example.org/callback_uri" diff --git a/tests/config/itkdev_openid_connect_configured_logger.yml b/tests/config/itkdev_openid_connect_configured_logger.yml index d751e39..5eb1710 100644 --- a/tests/config/itkdev_openid_connect_configured_logger.yml +++ b/tests/config/itkdev_openid_connect_configured_logger.yml @@ -12,6 +12,7 @@ itkdev_openid_connect: metadata_url: "https://provider.example.org/openid-configuration" client_id: "test_id" client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" redirect_uri: "https://app.example.org/callback_uri" test_provider_2: options: @@ -19,4 +20,5 @@ itkdev_openid_connect: client_id: "test_id" leeway: 5 client_secret: "test_secret" + client_secret_expires_at: "2027-01-31" redirect_uri: "https://app.example.org/callback_uri" From 5c23695fdf5d529001d22718eff6bf2c44332c6b Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 19 Aug 2026 17:22:43 +0200 Subject: [PATCH 08/28] fix: drop the default from the now-required expiry date MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A required node carrying a default is contradictory. Symfony 7.4 reports it by deprecating rather than refusing, raised through trigger_deprecation() and therefore suppressed, so PHPUnit's own failOnDeprecation never sees it and Symfony 6.2's config does not check at all — it surfaced on a consuming application's console instead. Guarded by a test that installs a handler while the tree is built and asserts no deprecation, and restore the exception handler a handled request leaves behind on 6.4, which PHPUnit was reporting as risky on the prefer-lowest leg. --- src/DependencyInjection/Configuration.php | 1 - .../DependencyInjection/ConfigurationTest.php | 34 +++++++++++++++++++ .../ConfiguredLoggerPassTest.php | 3 ++ tests/RestoresExceptionHandlers.php | 27 +++++++++++++++ .../FailedCallbackDoesNotLoopTest.php | 3 ++ 5 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 tests/RestoresExceptionHandlers.php diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 755166b..d901af8 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -124,7 +124,6 @@ public function getConfigTreeBuilder(): TreeBuilder // values regardless: ScalarNode::isValueEmpty() is // `null === $value || '' === $value`. ->info('Required. Date the client secret expires, e.g. "2027-01-31". Anything strtotime() understands, and usually an environment variable. An expired secret breaks every login, so the bundle warns while there is still time to rotate.') - ->defaultNull() ->validate() // '' is exempt because it is the dummy fixture Symfony // substitutes for %env(string:...)% while compiling diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index ea6cbc8..f117ff3 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -356,6 +356,40 @@ public function testProviderKeysAreNotNormalized(): void $this->assertArrayNotHasKey('my_provider', $config['openid_providers']); } + /** + * The definition itself must be free of deprecations. + * + * Symfony reports a contradictory definition — a required node that also carries + * a default, say — by deprecating it rather than refusing it, and + * `trigger_deprecation()` raises that with `@`, which PHPUnit's own + * `failOnDeprecation` respects and therefore never sees. A handler installed here + * does see it. Otherwise the first report comes from a consuming application's + * console, which is where this one was found. + */ + public function testTheDefinitionEmitsNoDeprecations(): void + { + $deprecations = []; + // All four arguments are forwarded: the handler being wrapped is PHPUnit's, + // whose __invoke() requires file and line. + $previous = set_error_handler(static function (int $level, string $message, string $file = '', int $line = 0) use (&$deprecations, &$previous): bool { + if (\E_USER_DEPRECATED === $level) { + $deprecations[] = $message; + + return true; + } + + return null !== $previous && false !== ($previous)($level, $message, $file, $line); + }); + + try { + $this->processor->processConfiguration($this->configuration, [$this->getMinimalConfig()]); + } finally { + restore_error_handler(); + } + + $this->assertSame([], $deprecations); + } + public function testTheExpiryDateIsRequired(): void { $input = $this->getMinimalConfig(); diff --git a/tests/DependencyInjection/ConfiguredLoggerPassTest.php b/tests/DependencyInjection/ConfiguredLoggerPassTest.php index 4dfd3e0..4ceb8ba 100644 --- a/tests/DependencyInjection/ConfiguredLoggerPassTest.php +++ b/tests/DependencyInjection/ConfiguredLoggerPassTest.php @@ -5,6 +5,7 @@ use ItkDev\OpenIdConnectBundle\DependencyInjection\Compiler\ConfiguredLoggerPass; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\ItkDevOpenIdConnectBundleTestingKernel; +use ItkDev\OpenIdConnectBundle\Tests\RestoresExceptionHandlers; use ItkDev\OpenIdConnectBundle\Tests\Security\ConsumerAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; use PHPUnit\Framework\TestCase; @@ -31,6 +32,8 @@ */ class ConfiguredLoggerPassTest extends TestCase { + use RestoresExceptionHandlers; + private ItkDevOpenIdConnectBundleTestingKernel $kernel; protected function setUp(): void diff --git a/tests/RestoresExceptionHandlers.php b/tests/RestoresExceptionHandlers.php new file mode 100644 index 0000000..2cf7b53 --- /dev/null +++ b/tests/RestoresExceptionHandlers.php @@ -0,0 +1,27 @@ + Date: Wed, 19 Aug 2026 19:54:56 +0200 Subject: [PATCH 09/28] fix: reject a non-string expiry date instead of discarding it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit YAML reads an unquoted 2027-01-31 as the integer 1801353600, and the parse check only inspected strings. Such a value passed configuration, was discarded as untyped by the extension, and reached the checker as null — which returns Unknown without logging. The most natural way to write the option therefore left the provider permanently unmonitored with nothing reported anywhere, which is the outcome the option exists to prevent. An explicit null had the same effect and is rejected too. Corrects the comment that claimed the checker reports this, the README's account of which parser runs where, and narrows the trait undoing 6.4's leaked exception handler so it pops only what a test added. --- CHANGELOG.md | 7 +-- README.md | 10 ++-- UPGRADE-6.0.md | 4 +- src/DependencyInjection/Configuration.php | 9 ++++ .../ItkDevOpenIdConnectExtension.php | 7 +-- .../DependencyInjection/ConfigurationTest.php | 53 +++++++++++++------ .../ConfiguredLoggerPassTest.php | 6 +++ tests/RestoresExceptionHandlers.php | 35 +++++++++--- .../FailedCallbackDoesNotLoopTest.php | 6 +++ 9 files changed, 104 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51a6219..36b9a15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,9 +31,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `getPrevious()` on that exception is the underlying OpenID Connect exception rather than the `AuthenticationException`, which the security component would have followed straight back into the loop. -- `client_secret_expires_at` is now required for every provider. A missing date - fails while the container compiles instead of emitting the 5.1 deprecation, - since the bundle cannot warn about an expiry it does not know about. See +- `client_secret_expires_at` is now required for every provider, and must be a + string. A missing date fails while the container compiles instead of emitting the + 5.1 deprecation, and an unquoted `2027-01-31` — which YAML reads as a number — is + rejected rather than silently leaving the provider unmonitored. See `UPGRADE-6.0.md`. ### Removed (BREAKING) diff --git a/README.md b/README.md index ecd5a11..fc7f06c 100644 --- a/README.md +++ b/README.md @@ -227,9 +227,13 @@ with `invalid_client` — but the `critical` record here and the failure record the callback together name the cause without anyone having to reproduce it. `client_secret_expires_at` is required, because the bundle cannot warn about an -expiry it does not know about. A provider still reaches `unknown` when the value is -set to something unusable — an environment variable that resolved to nothing, or a -date `strtotime()` cannot read — and that is reported at `error`. +expiry it does not know about. Quote it: YAML reads an unquoted `2027-01-31` as a +number, and a value that is not a string is rejected while the container compiles. + +A provider still reaches `unknown` at runtime when the value resolves to something +unusable — an environment variable that is set but blank, or a date +`DateTimeImmutable` cannot parse — and that is reported at `error`, because an +unmonitored secret is no better than not having this feature. ##### Monitoring expiry diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index d25b2f8..9b4c74f 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -64,7 +64,9 @@ itkdev_openid_connect: client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' ``` -Anything `strtotime()` understands. A missing key now fails at compile time: +Anything `strtotime()` understands, but **quote it** — YAML reads an unquoted +`2027-01-31` as the number `1801353600`, and a non-string is rejected. A missing key +fails at compile time too: ```text The child config "client_secret_expires_at" under diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index d901af8..ecd5f88 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -124,6 +124,15 @@ public function getConfigTreeBuilder(): TreeBuilder // values regardless: ScalarNode::isValueEmpty() is // `null === $value || '' === $value`. ->info('Required. Date the client secret expires, e.g. "2027-01-31". Anything strtotime() understands, and usually an environment variable. An expired secret breaks every login, so the bundle warns while there is still time to rotate.') + ->validate() + // YAML reads an unquoted 2027-01-31 as the integer 1801353600, and the + // closure below only inspects strings, so without this the most natural + // way to write the value would pass, be discarded as untyped, and leave + // the provider unmonitored with nothing logged. Also catches an explicit + // null, which isRequired() accepts because the key is present. + ->ifTrue(static fn (mixed $v): bool => !is_string($v)) + ->thenInvalid('client_secret_expires_at must be a quoted string: YAML reads an unquoted date as a number. Use client_secret_expires_at: "2027-01-31". Got %s.') + ->end() ->validate() // '' is exempt because it is the dummy fixture Symfony // substitutes for %env(string:...)% while compiling diff --git a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php index bffd6d1..5140f1f 100644 --- a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php +++ b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php @@ -156,9 +156,10 @@ private function configureSecretExpiry(ContainerBuilder $container, array $provi foreach ($providers as $providerKey => $provider) { $expiresAt = $provider['options']['client_secret_expires_at'] ?? null; - // Required by the configuration, so a null here means an environment - // variable that resolved to something that is not a string. - // ClientSecretExpiryChecker reports that at runtime. + // Configuration requires the key and rejects a non-string, so the + // fallback is unreachable and kept only because the shape here is mixed. + // It must not become the quiet path it used to be: a null reaches + // ClientSecretExpiryChecker as Unknown with nothing logged. $expiryDates[$providerKey] = is_string($expiresAt) ? $expiresAt : null; } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index f117ff3..ea960ee 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -105,27 +105,18 @@ public function testClientSecretExpiresAtRejectsUnparseableLiterals(string $date $this->processor->processConfiguration($this->configuration, [$input]); } - /** - * @return iterable - */ - public static function toleratedEmptyDateProvider(): iterable - { - // '' is the fixture Symfony substitutes for a string env var while - // compiling, so it has to pass here; the checker reports it at runtime. - yield 'empty string' => ['']; - // An explicit null is a deliberate "not configured", not a typo. - yield 'explicit null' => [null]; - } - - #[DataProvider('toleratedEmptyDateProvider')] - public function testClientSecretExpiresAtToleratesEmptyValues(?string $date): void + public function testClientSecretExpiresAtToleratesAnEmptyString(): void { + // '' is the fixture Symfony substitutes for a string env var while compiling, + // so it has to pass here; the checker reports it at runtime. An explicit null + // is no longer tolerated — see testANonStringExpiryDateIsRejected. It used to + // mean "not configured", which is not a thing a required option has. $input = $this->getMinimalConfig(); - $input['openid_providers']['provider1']['options']['client_secret_expires_at'] = $date; + $input['openid_providers']['provider1']['options']['client_secret_expires_at'] = ''; $config = $this->processor->processConfiguration($this->configuration, [$input]); - $this->assertSame($date, $config['openid_providers']['provider1']['options']['client_secret_expires_at']); + $this->assertSame('', $config['openid_providers']['provider1']['options']['client_secret_expires_at']); } public function testClientSecretExpiresAtAccepted(): void @@ -390,6 +381,36 @@ public function testTheDefinitionEmitsNoDeprecations(): void $this->assertSame([], $deprecations); } + /** + * The value a reader would most likely write. + * + * `client_secret_expires_at: 2027-01-31` without quotes is the integer + * 1801353600 by the time configuration sees it. Accepting it would discard the + * date and leave the provider unmonitored with nothing logged anywhere, which is + * the exact outcome this option exists to prevent. + * + * @return iterable + */ + public static function nonStringDateProvider(): iterable + { + yield 'unquoted date, read as a timestamp' => [1801353600]; + yield 'digits' => [20270131]; + yield 'boolean' => [true]; + yield 'explicit null, which isRequired() accepts' => [null]; + } + + #[DataProvider('nonStringDateProvider')] + public function testANonStringExpiryDateIsRejected(mixed $configured): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['client_secret_expires_at'] = $configured; + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('client_secret_expires_at must be a quoted string'); + + $this->processor->processConfiguration($this->configuration, [$input]); + } + public function testTheExpiryDateIsRequired(): void { $input = $this->getMinimalConfig(); diff --git a/tests/DependencyInjection/ConfiguredLoggerPassTest.php b/tests/DependencyInjection/ConfiguredLoggerPassTest.php index 4ceb8ba..1a74454 100644 --- a/tests/DependencyInjection/ConfiguredLoggerPassTest.php +++ b/tests/DependencyInjection/ConfiguredLoggerPassTest.php @@ -38,9 +38,15 @@ class ConfiguredLoggerPassTest extends TestCase protected function setUp(): void { + $this->captureExceptionHandler(); $this->kernel = $this->boot('itkdev_openid_connect_configured_logger.yml'); } + protected function tearDown(): void + { + $this->restoreExceptionHandlers(); + } + private function boot(string $bundleConfig): ItkDevOpenIdConnectBundleTestingKernel { $kernel = new ItkDevOpenIdConnectBundleTestingKernel([ diff --git a/tests/RestoresExceptionHandlers.php b/tests/RestoresExceptionHandlers.php index 2cf7b53..d5e3766 100644 --- a/tests/RestoresExceptionHandlers.php +++ b/tests/RestoresExceptionHandlers.php @@ -6,22 +6,43 @@ * Undo the exception handler a handled request leaves behind. * * On Symfony 6.4 — the floor this bundle supports — handling a request in debug mode - * installs an exception handler that is never removed. PHPUnit reports the test as + * installs an exception handler and never removes it. PHPUnit reports the test as * risky, and rightly: left in place it would also handle exceptions raised by later * tests in the same process. + * + * Both methods are called explicitly rather than hooked into `setUp()`/`tearDown()` + * here, so that a class adding either of its own cannot silently switch this off. */ trait RestoresExceptionHandlers { - protected function tearDown(): void + private mixed $handlerBeforeTest = null; + + /** + * Note what was installed before the test, so only what the test added is undone. + */ + protected function captureExceptionHandler(): void { - // set_exception_handler(null) pushes a null handler and returns the one that - // was current, so each iteration pops both it and the handler it found. - while (null !== set_exception_handler(null)) { - restore_exception_handler(); + $this->handlerBeforeTest = $this->currentExceptionHandler(); + } + + protected function restoreExceptionHandlers(): void + { + // Pops one handler at a time rather than draining the stack, which would + // discard a global handler registered before the suite ran. + while ($this->currentExceptionHandler() !== $this->handlerBeforeTest) { restore_exception_handler(); } + } - // The last iteration pushed a null handler of its own. + /** + * Read the current handler without changing the stack: the push is undone + * immediately, and `set_exception_handler()` returns what it displaced. + */ + private function currentExceptionHandler(): mixed + { + $handler = set_exception_handler(null); restore_exception_handler(); + + return $handler; } } diff --git a/tests/Security/FailedCallbackDoesNotLoopTest.php b/tests/Security/FailedCallbackDoesNotLoopTest.php index 5e1fa72..478cdf2 100644 --- a/tests/Security/FailedCallbackDoesNotLoopTest.php +++ b/tests/Security/FailedCallbackDoesNotLoopTest.php @@ -30,6 +30,7 @@ class FailedCallbackDoesNotLoopTest extends TestCase protected function setUp(): void { + $this->captureExceptionHandler(); $this->kernel = new ItkDevOpenIdConnectBundleTestingKernel([ __DIR__.'/../config/framework.yml', __DIR__.'/../config/framework_routing.yml', @@ -43,6 +44,11 @@ protected function setUp(): void * A callback whose state does not match the session: the shape of every * failure the outage produced, an expired client secret included. */ + protected function tearDown(): void + { + $this->restoreExceptionHandlers(); + } + private function failingCallback(): Request { $request = Request::create('/protected?state=does-not-match&code=some-code'); From bf87a0bca38ad214ceb563b83ddcef38b0fad1ab Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 19 Aug 2026 20:03:05 +0200 Subject: [PATCH 10/28] test: reunite a docblock with its method and guard the handler loop The tearDown() added in the previous commit landed between failingCallback() and its docblock. The pop loop also assumed a test only ever adds handlers: one that removed the handler it inherited would leave the current handler at null against a non-null baseline, and restore_exception_handler() on an empty stack is a no-op. --- tests/RestoresExceptionHandlers.php | 7 +++++-- tests/Security/FailedCallbackDoesNotLoopTest.php | 8 ++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/RestoresExceptionHandlers.php b/tests/RestoresExceptionHandlers.php index d5e3766..dec4b30 100644 --- a/tests/RestoresExceptionHandlers.php +++ b/tests/RestoresExceptionHandlers.php @@ -28,8 +28,11 @@ protected function captureExceptionHandler(): void protected function restoreExceptionHandlers(): void { // Pops one handler at a time rather than draining the stack, which would - // discard a global handler registered before the suite ran. - while ($this->currentExceptionHandler() !== $this->handlerBeforeTest) { + // discard a global handler registered before the suite ran. The null check is + // the terminating condition for a test that removed the handler it inherited + // instead of adding on top of it: restore_exception_handler() on an empty + // stack is a no-op, so without it the loop would never finish. + while (null !== ($current = $this->currentExceptionHandler()) && $current !== $this->handlerBeforeTest) { restore_exception_handler(); } } diff --git a/tests/Security/FailedCallbackDoesNotLoopTest.php b/tests/Security/FailedCallbackDoesNotLoopTest.php index 478cdf2..ae6f21d 100644 --- a/tests/Security/FailedCallbackDoesNotLoopTest.php +++ b/tests/Security/FailedCallbackDoesNotLoopTest.php @@ -40,15 +40,15 @@ protected function setUp(): void $this->kernel->boot(); } - /** - * A callback whose state does not match the session: the shape of every - * failure the outage produced, an expired client secret included. - */ protected function tearDown(): void { $this->restoreExceptionHandlers(); } + /** + * A callback whose state does not match the session: the shape of every + * failure the outage produced, an expired client secret included. + */ private function failingCallback(): Request { $request = Request::create('/protected?state=does-not-match&code=some-code'); From 5a1aac4a48d1d3b832e602dcfc6c389a9d6bb885 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 20 Aug 2026 10:51:42 +0200 Subject: [PATCH 11/28] docs: correct the expiry checker's account of env vars and validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docblock claimed Symfony will not accept an environment variable on a validated node, which this PR makes plainly false: the node now carries two closures and env vars compile fine. What Symfony refuses is a validated node that also disallows empty values, which is why cannotBeEmpty() is absent — and the reason runtime reporting is still needed is that the value is an unresolved placeholder at compile time. Also widen the rejection message beyond the quoting mistake, since null and true are not quoting errors and an %env(int:...)% cast fails here with a config file that does contain a string. --- src/DependencyInjection/Configuration.php | 6 +++++- src/Util/ClientSecretExpiryChecker.php | 9 ++++++--- tests/DependencyInjection/ConfigurationTest.php | 2 +- tests/RestoresExceptionHandlers.php | 4 ++++ 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index ecd5f88..9db9c2f 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -131,7 +131,7 @@ public function getConfigTreeBuilder(): TreeBuilder // the provider unmonitored with nothing logged. Also catches an explicit // null, which isRequired() accepts because the key is present. ->ifTrue(static fn (mixed $v): bool => !is_string($v)) - ->thenInvalid('client_secret_expires_at must be a quoted string: YAML reads an unquoted date as a number. Use client_secret_expires_at: "2027-01-31". Got %s.') + ->thenInvalid('client_secret_expires_at must be a string. YAML reads an unquoted date as a number, so quote it: "2027-01-31". From an environment variable, cast it as %%env(string:NAME)%%. Got %s.') ->end() ->validate() // '' is exempt because it is the dummy fixture Symfony @@ -147,6 +147,10 @@ public function getConfigTreeBuilder(): TreeBuilder // a timestamp rather than false, the same "blank means now" // quirk DateTimeImmutable has, so whitespace would otherwise // sail through as a valid date. + // is_string() is unreachable-looking now that the closure above + // rejects non-strings, but it is what lets trim() and strtotime() + // take a mixed value under PHPStan, and it keeps this closure + // correct on its own terms rather than by ordering. ->ifTrue(static fn (mixed $v): bool => is_string($v) && '' !== $v && ('' === trim($v) || false === strtotime($v))) ->thenInvalid('client_secret_expires_at must be a date parseable by strtotime(), e.g. "2027-01-31". Got %s.') ->end() diff --git a/src/Util/ClientSecretExpiryChecker.php b/src/Util/ClientSecretExpiryChecker.php index 95db216..0e565a3 100644 --- a/src/Util/ClientSecretExpiryChecker.php +++ b/src/Util/ClientSecretExpiryChecker.php @@ -85,9 +85,12 @@ public function getStatus(string $providerKey): ClientSecretExpiry /** * Report a date that cannot be used. * - * Neither an empty nor a malformed value can be rejected when the container - * compiles: this comes from the environment in every real deployment, and - * Symfony will not accept an environment variable on a node that is validated. + * A literal is rejected when the container compiles, but the value comes from + * the environment in every real deployment and is still an unresolved + * placeholder then, so what it resolves to can only be judged here. Validation + * closures and environment variables do coexist on that node; what Symfony + * refuses is a validated node that also disallows empty values, which is why + * `cannotBeEmpty()` is absent from it. * So it is reported rather than thrown — a mistyped date must not take an * application down — but reported loudly, because the effect is that nothing is * monitoring this secret, and silence would equal not having the feature. diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index ea960ee..fe80a63 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -406,7 +406,7 @@ public function testANonStringExpiryDateIsRejected(mixed $configured): void $input['openid_providers']['provider1']['options']['client_secret_expires_at'] = $configured; $this->expectException(InvalidConfigurationException::class); - $this->expectExceptionMessage('client_secret_expires_at must be a quoted string'); + $this->expectExceptionMessage('client_secret_expires_at must be a string'); $this->processor->processConfiguration($this->configuration, [$input]); } diff --git a/tests/RestoresExceptionHandlers.php b/tests/RestoresExceptionHandlers.php index dec4b30..69300bb 100644 --- a/tests/RestoresExceptionHandlers.php +++ b/tests/RestoresExceptionHandlers.php @@ -27,6 +27,10 @@ protected function captureExceptionHandler(): void protected function restoreExceptionHandlers(): void { + // Compares identity: a test that replaced the baseline handler with an equal + // but distinct instance would drain past it. Nothing does, and nothing in + // PHPUnit's lifecycle can. + // // Pops one handler at a time rather than draining the stack, which would // discard a global handler registered before the suite ran. The null check is // the terminating condition for a test that removed the handler it inherited From 9f9b8d2f4dfa17e81d04593f29e8a0e385b170fd Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 20 Aug 2026 11:23:15 +0200 Subject: [PATCH 12/28] feat!: accept a callback only on the provider's callback path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit supports() matched state and code on any path, so every URL behind the firewall was a potential callback. Since 6.0 fails closed, that meant an unauthenticated caller could raise a 500 on any URL by appending two query parameters. Requiring the provider's configured callback path leaves a forged callback to the firewall's ordinary handling, without weakening fail-closed behaviour for real ones. Paths are derived from configuration and memoized rather than read off a provider instance: building one pulls in discovery, an HTTP client and a cache pool, and this runs on every request. Nothing consults the session — that would start one for anonymous traffic, and a lost session would put the redirect loop back. callback_path covers proxies that rewrite the path, and each provider must now declare one of redirect_uri, redirect_route or callback_path, since without one it could never recognise a callback. getSupportedProviderKeys() defaults to every provider, so existing multi-authenticator firewalls are unaffected. Also adds createTargetPathRedirect(), in the same file, for returning the user to the page that sent them to log in. --- src/DependencyInjection/Configuration.php | 24 +++ .../OpenIdConfigurationProviderManager.php | 78 ++++++++ src/Security/OpenIdLoginAuthenticator.php | 83 ++++++++- .../DependencyInjection/ConfigurationTest.php | 76 ++++++++ .../ConfiguredLoggerPassTest.php | 2 +- .../ItkDevOpenIdConnectExtensionTest.php | 1 + .../FailedCallbackDoesNotLoopTest.php | 43 ++++- ...OpenIdConfigurationProviderManagerTest.php | 84 +++++++++ .../Security/OpenIdLoginAuthenticatorTest.php | 170 +++++++++++++++++- .../Security/SingleProviderAuthenticator.php | 16 ++ tests/Security/TestAuthenticator.php | 10 ++ tests/config/routes.yml | 6 + tests/config/security_consumer.yml | 4 + 13 files changed, 586 insertions(+), 11 deletions(-) create mode 100644 tests/Security/SingleProviderAuthenticator.php diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 9db9c2f..2eb3979 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -174,6 +174,22 @@ public function getConfigTreeBuilder(): TreeBuilder ->arrayNode('redirect_route_parameters') ->info('Redirect route parameters') ->end() + ->scalarNode('callback_path') + ->info('Optional. The request path the callback arrives on, when a reverse proxy rewrites it so that the path of redirect_uri is not the path this application sees. Defaults to the path of redirect_uri, or of the generated redirect_route.') + // As on client_secret_expires_at: a validated node that also + // disallows empty values refuses environment variables, and the + // closure is the half worth keeping. + ->validate() + ->ifTrue(static fn (mixed $v): bool => !is_string($v)) + ->thenInvalid('callback_path must be a string, e.g. "/auth/callback". Got %s.') + ->end() + ->validate() + // '' is the fixture Symfony substitutes for a string + // environment variable while compiling, so it has to pass. + ->ifTrue(static fn (mixed $v): bool => is_string($v) && '' !== $v && !str_starts_with($v, '/')) + ->thenInvalid('callback_path must start with "/", e.g. "/auth/callback". Got %s.') + ->end() + ->end() ->booleanNode('allow_http') ->info('Whether to allow http or not (default: false)') ->defaultValue(false) @@ -202,6 +218,14 @@ public function getConfigTreeBuilder(): TreeBuilder ->validate() ->ifTrue(static fn (array $v) => isset($v['redirect_uri'], $v['redirect_route'])) ->thenInvalid('Only one of redirect_uri or redirect_route must be set.') + ->end() + ->validate() + // Without one of these there is no path to recognise a callback on, + // and since 6.0 that means the provider can never complete a login: + // supports() matches the configured callback path, not any path + // carrying state and code. + ->ifTrue(static fn (array $v) => !isset($v['redirect_uri']) && !isset($v['redirect_route']) && !isset($v['callback_path'])) + ->thenInvalid('One of redirect_uri, redirect_route or callback_path must be set: it is how a callback is recognised.') ->end() ->end() ->end() diff --git a/src/Security/OpenIdConfigurationProviderManager.php b/src/Security/OpenIdConfigurationProviderManager.php index dabf67b..7332908 100644 --- a/src/Security/OpenIdConfigurationProviderManager.php +++ b/src/Security/OpenIdConfigurationProviderManager.php @@ -13,6 +13,9 @@ class OpenIdConfigurationProviderManager /** @var array */ private array $providers = []; + /** @var array|null */ + private ?array $redirectUriPaths = null; + /** * @param array{ * default_providers_options: array, @@ -23,6 +26,7 @@ class OpenIdConfigurationProviderManager * redirect_uri?: string, * redirect_route?: string, * redirect_route_parameters?: array, + * callback_path?: string, * leeway?: int, * cache_duration?: int, * allow_http?: bool, @@ -50,6 +54,80 @@ public function getProviderKeys(): array return array_keys($this->config['providers']); } + /** + * The request path each provider's callback arrives on, keyed by provider. + * + * Derived from configuration rather than from a provider instance: + * `supports()` consults this on every request through the firewall, and + * building a provider pulls in discovery, HTTP client and cache configuration + * for no reason. Memoized for the same reason. + * + * @return array + */ + public function getRedirectUriPaths(): array + { + if (null !== $this->redirectUriPaths) { + return $this->redirectUriPaths; + } + + $paths = []; + + foreach ($this->config['providers'] as $key => $options) { + $path = $this->derivePath($options); + + if (null !== $path) { + $paths[$key] = $path; + } + } + + return $this->redirectUriPaths = $paths; + } + + /** + * @param array{redirect_uri?: string, redirect_route?: string, redirect_route_parameters?: array, callback_path?: string} $options + */ + private function derivePath(array $options): ?string + { + // callback_path first: it exists precisely for deployments where the + // external redirect_uri path is not the path this application receives. + if (isset($options['callback_path'])) { + return $this->normalizePath($options['callback_path']); + } + + // Generated as a path, not a URL, so a reverse proxy's prefix handling is + // already accounted for by the router. + if (isset($options['redirect_route'])) { + return $this->normalizePath($this->router->generate( + $options['redirect_route'], + $options['redirect_route_parameters'] ?? [], + UrlGeneratorInterface::ABSOLUTE_PATH + )); + } + + if (isset($options['redirect_uri'])) { + // An external URL: its path is what the identity provider sends the + // browser to, which is the internal path only when nothing rewrites it. + $path = parse_url($options['redirect_uri'], \PHP_URL_PATH); + + // A redirect_uri with no path at all, or one that could not be parsed: + // the provider then answers at the application root. + return $this->normalizePath(is_string($path) ? $path : '/'); + } + + return null; + } + + /** + * Leading slash, no trailing slash, so that the comparison in `supports()` + * does not turn on how the value was written. + */ + private function normalizePath(string $path): string + { + $trimmed = rtrim('/'.ltrim($path, '/'), '/'); + + return '' === $trimmed ? '/' : $trimmed; + } + /** * Get a provider by key. * diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index c198086..a6b0df2 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -9,11 +9,13 @@ use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Http\Authenticator\AbstractAuthenticator; use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface; +use Symfony\Component\Security\Http\Util\TargetPathTrait; /** * Authenticator for OpenId Connect login. @@ -45,6 +47,8 @@ */ abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements AuthenticationEntryPointInterface, LoggerAwareInterface { + use TargetPathTrait; + private LoggerInterface $logger; /** @@ -61,10 +65,85 @@ public function setLogger(LoggerInterface $logger): void $this->logger = $logger; } + /** + * Whether this request is a callback for one of this authenticator's providers. + * + * `state` and `code` alone used to be enough, which made every URL under the + * firewall a callback: anyone could turn any page into a failed login, and since + * the bundle fails closed that means a 500 raised by an unauthenticated caller. + * Requiring the configured callback path as well leaves a forged callback to the + * firewall's ordinary handling. + * + * Nothing here touches the session. This runs on every request through the + * firewall, so starting a session for anonymous traffic would be a real cost, and + * "is this a callback" must not depend on whether this browser began the login. + * The session's provider key is still what decides which provider validates it, + * in `validateClaims()`. + */ public function supports(Request $request): ?bool { - // Check if request has state and code - return $request->query->has('state') && $request->query->has('code'); + if (!$request->query->has('state') || !$request->query->has('code')) { + return false; + } + + $path = rtrim($request->getPathInfo(), '/'); + $path = '' === $path ? '/' : $path; + + $paths = $this->providerManager->getRedirectUriPaths(); + + foreach ($this->getSupportedProviderKeys() as $providerKey) { + // Case-sensitive: paths are, and an identity provider sends the browser + // to the redirect URI exactly as it was registered. + if (($paths[$providerKey] ?? null) === $path) { + return true; + } + } + + return false; + } + + /** + * Redirect to the page the user originally asked for. + * + * Symfony saves that page when the entry point fires, which covers both shapes of + * consumer: one that redirects straight to the identity provider, and one that + * shows a login screen carrying a link to it. `$fallbackUrl` is for a user who + * reached the login link without being sent there — nothing was saved then. + * + * The saved path is cleared on use, so a later visit to the login link does not + * replay a stale target. + */ + protected function createTargetPathRedirect(Request $request, string $firewallName, string $fallbackUrl): RedirectResponse + { + $targetPath = $this->getTargetPath($request->getSession(), $firewallName); + + if (null === $targetPath || '' === $targetPath) { + return new RedirectResponse($fallbackUrl); + } + + $this->removeTargetPath($request->getSession(), $firewallName); + + return new RedirectResponse($targetPath); + } + + /** + * Provider keys whose callbacks this authenticator answers. + * + * Every configured provider by default, which is what keeps several + * `OpenIdLoginAuthenticator` subclasses on one firewall working as they do + * today: each supports every callback path, Symfony asks them in the order + * `security.yaml` lists them, and the session's provider key decides which + * provider validates the callback. + * + * Override in a subclass bound to particular providers so that, with a distinct + * callback path per provider, each callback is answered by the authenticator that + * owns it. + * + * @return string[] + */ + protected function getSupportedProviderKeys(): array + { + return $this->providerManager->getProviderKeys(); } /** diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index fe80a63..ae482eb 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -36,6 +36,7 @@ private function getMinimalConfig(): array 'client_id' => 'my_id', 'client_secret' => 'my_secret', 'client_secret_expires_at' => '2027-01-31', + 'redirect_uri' => 'https://app.example.org/callback_uri', ], ], ], @@ -252,6 +253,8 @@ public function testFullConfig(): void public function testRedirectRouteConfig(): void { $input = $this->getMinimalConfig(); + // Mutually exclusive with redirect_uri, which the minimal config sets. + unset($input['openid_providers']['provider1']['options']['redirect_uri']); $input['openid_providers']['provider1']['options']['redirect_route'] = 'my_redirect_route'; $config = $this->processor->processConfiguration( @@ -431,6 +434,7 @@ public function testMultipleProviders(): void 'client_id' => 'other_id', 'client_secret' => 'other_secret', 'client_secret_expires_at' => '2028-06-30', + 'redirect_uri' => 'https://app.example.org/other_callback', ], ]; @@ -443,4 +447,76 @@ public function testMultipleProviders(): void $this->assertArrayHasKey('provider1', $config['openid_providers']); $this->assertArrayHasKey('provider2', $config['openid_providers']); } + + /** + * @return iterable + */ + public static function invalidCallbackPathProvider(): iterable + { + yield 'not a string' => [42, 'callback_path must be a string']; + yield 'null' => [null, 'callback_path must be a string']; + yield 'no leading slash' => ['auth/callback', 'callback_path must start with "/"']; + yield 'a full url' => ['https://app.example.org/auth/callback', 'callback_path must start with "/"']; + } + + #[DataProvider('invalidCallbackPathProvider')] + public function testAnInvalidCallbackPathIsRejected(mixed $configured, string $expectedMessage): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['callback_path'] = $configured; + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage($expectedMessage); + + $this->processor->processConfiguration($this->configuration, [$input]); + } + + /** + * @return iterable + */ + public static function validCallbackPathProvider(): iterable + { + yield 'a path' => ['/auth/callback']; + yield 'the root' => ['/']; + // As on client_secret_expires_at: '' is the fixture Symfony substitutes for a + // string environment variable while compiling, so it must pass here. + yield 'the environment variable fixture' => ['']; + } + + #[DataProvider('validCallbackPathProvider')] + public function testAValidCallbackPathIsAccepted(string $configured): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['callback_path'] = $configured; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame($configured, $config['openid_providers']['provider1']['options']['callback_path']); + } + + /** + * A provider that declares no callback target cannot recognise a callback, so it + * could never complete a login. + */ + public function testAProviderMustDeclareACallbackTarget(): void + { + $input = $this->getMinimalConfig(); + unset($input['openid_providers']['provider1']['options']['redirect_uri']); + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('One of redirect_uri, redirect_route or callback_path must be set'); + + $this->processor->processConfiguration($this->configuration, [$input]); + } + + public function testCallbackPathAloneSatisfiesTheRequirement(): void + { + $input = $this->getMinimalConfig(); + unset($input['openid_providers']['provider1']['options']['redirect_uri']); + $input['openid_providers']['provider1']['options']['callback_path'] = '/auth/callback'; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame('/auth/callback', $config['openid_providers']['provider1']['options']['callback_path']); + } } diff --git a/tests/DependencyInjection/ConfiguredLoggerPassTest.php b/tests/DependencyInjection/ConfiguredLoggerPassTest.php index 1a74454..3e4db16 100644 --- a/tests/DependencyInjection/ConfiguredLoggerPassTest.php +++ b/tests/DependencyInjection/ConfiguredLoggerPassTest.php @@ -88,7 +88,7 @@ public function testTheBuiltAuthenticatorHoldsTheConfiguredLogger(): void */ public function testAFailedLoginIsWrittenToTheConfiguredLogger(): void { - $request = Request::create('/protected?state=does-not-match&code=some-code'); + $request = Request::create('/callback_uri?state=does-not-match&code=some-code'); $session = new Session(new MockArraySessionStorage()); $session->set('oauth2provider', 'test_provider_1'); $session->set('oauth2state', 'the-real-state'); diff --git a/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php b/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php index 0583691..34f1e35 100644 --- a/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php +++ b/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php @@ -36,6 +36,7 @@ private function getBaseConfig(?string $userProvider = null): array 'client_id' => 'test_id', 'client_secret' => 'test_secret', 'client_secret_expires_at' => '2027-01-31', + 'redirect_uri' => 'https://app.example.org/callback_uri', ], ], ], diff --git a/tests/Security/FailedCallbackDoesNotLoopTest.php b/tests/Security/FailedCallbackDoesNotLoopTest.php index ae6f21d..1927348 100644 --- a/tests/Security/FailedCallbackDoesNotLoopTest.php +++ b/tests/Security/FailedCallbackDoesNotLoopTest.php @@ -51,7 +51,7 @@ protected function tearDown(): void */ private function failingCallback(): Request { - $request = Request::create('/protected?state=does-not-match&code=some-code'); + $request = Request::create('/callback_uri?state=does-not-match&code=some-code'); $session = new Session(new MockArraySessionStorage()); $session->set('oauth2provider', 'test_provider_1'); $session->set('oauth2state', 'the-real-state'); @@ -119,4 +119,45 @@ public function testTheExceptionAndItsWholeCauseChainStayOutsideTheSecurityHiera $this->assertStringContainsString('Invalid state', $exception->getMessage(), 'The cause is still reported'); } } + + /** + * The observable fix for issue #63. + * + * `state` and `code` on a path that is not a callback used to enter the flow and, + * since the bundle fails closed, surface as a 500 that any unauthenticated caller + * could raise on any URL. It is the firewall's business again: an anonymous + * request is sent to the entry point, exactly as it would be without the query + * string. + */ + public function testAStrayCallbackIsLeftToTheFirewall(): void + { + $request = Request::create('/protected?state=forged&code=forged'); + $request->setSession(new Session(new MockArraySessionStorage())); + + $response = $this->kernel->handle($request, catch: true); + + $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); + $this->assertSame(ConsumerAuthenticator::LOGIN_PATH, $response->headers->get('Location')); + $this->assertNull( + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'validateClaims() ran, so the authenticator accepted a callback on a path that is not one' + ); + } + + /** + * Symfony's half of "return to the page you asked for": the entry point fires and + * the target path is saved. Pinned here so a framework upgrade cannot quietly + * drop it and leave createTargetPathRedirect() with nothing to read. + */ + public function testTheEntryPointSavesTheRequestedPage(): void + { + $request = Request::create('/protected'); + $session = new Session(new MockArraySessionStorage()); + $request->setSession($session); + + $response = $this->kernel->handle($request, catch: true); + + $this->assertSame(ConsumerAuthenticator::LOGIN_PATH, $response->headers->get('Location')); + $this->assertSame('http://localhost/protected', $session->get('_security.main.target_path')); + } } diff --git a/tests/Security/OpenIdConfigurationProviderManagerTest.php b/tests/Security/OpenIdConfigurationProviderManagerTest.php index 1da56bb..426f577 100644 --- a/tests/Security/OpenIdConfigurationProviderManagerTest.php +++ b/tests/Security/OpenIdConfigurationProviderManagerTest.php @@ -6,6 +6,7 @@ use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; use Symfony\Component\Cache\Adapter\ArrayAdapter; @@ -220,4 +221,87 @@ public function testGetProviderCachesInstance(): void $this->assertSame($provider1, $provider2); } + + /** + * @return iterable, string}> + */ + public static function pathDerivationProvider(): iterable + { + yield 'path of an absolute redirect_uri' => [['redirect_uri' => 'https://app.example.org/callback_uri'], '/callback_uri']; + yield 'trailing slash removed' => [['redirect_uri' => 'https://app.example.org/callback_uri/'], '/callback_uri']; + yield 'nested path' => [['redirect_uri' => 'https://app.example.org/auth/oidc/callback'], '/auth/oidc/callback']; + yield 'query and fragment ignored' => [['redirect_uri' => 'https://app.example.org/callback_uri?x=1#f'], '/callback_uri']; + // A redirect_uri naming only a host answers at the root. + yield 'no path at all' => [['redirect_uri' => 'https://app.example.org'], '/']; + yield 'bare root' => [['redirect_uri' => 'https://app.example.org/'], '/']; + // callback_path exists for proxies that rewrite the external path, so it has + // to win over the redirect_uri it contradicts. + yield 'callback_path overrides redirect_uri' => [ + ['redirect_uri' => 'https://app.example.org/prefix/auth/callback', 'callback_path' => '/auth/callback'], + '/auth/callback', + ]; + yield 'callback_path is normalized too' => [['callback_path' => '/auth/callback/'], '/auth/callback']; + } + + /** + * @param array $options + */ + #[DataProvider('pathDerivationProvider')] + public function testRedirectUriPathsAreDerivedAndNormalized(array $options, string $expected): void + { + $manager = $this->createManager(['provider1' => $this->getBaseProviderConfig() + $options]); + + $this->assertSame(['provider1' => $expected], $manager->getRedirectUriPaths()); + } + + public function testARouteIsGeneratedAsAPathNotAUrl(): void + { + // ABSOLUTE_PATH, so that whatever a reverse proxy does to the host or scheme + // cannot affect the comparison, and the router's base path is included. + $router = $this->createMock(RouterInterface::class); + $router->expects($this->once()) + ->method('generate') + ->with('my_route', ['id' => '7'], UrlGeneratorInterface::ABSOLUTE_PATH) + ->willReturn('/generated/callback'); + + $config = [ + 'default_providers_options' => [], + 'providers' => ['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_route' => 'my_route', + 'redirect_route_parameters' => ['id' => '7'], + ]], + ]; + + $manager = new OpenIdConfigurationProviderManager($router, $config); + + $this->assertSame(['provider1' => '/generated/callback'], $manager->getRedirectUriPaths()); + // Memoized: supports() asks on every request through the firewall, and the + // once() above is what holds that. + $manager->getRedirectUriPaths(); + } + + public function testAProviderWithNoRedirectTargetIsAbsentRatherThanMatchingEverything(): void + { + $manager = $this->createManager([ + 'with_path' => $this->getBaseProviderConfig() + ['redirect_uri' => 'https://app.example.org/callback_uri'], + 'without_path' => $this->getBaseProviderConfig(), + ]); + + $this->assertSame(['with_path' => '/callback_uri'], $manager->getRedirectUriPaths()); + } + + public function testDerivingPathsDoesNotBuildProviders(): void + { + // Building a provider pulls in discovery, an HTTP client and a cache pool. + // Nothing in this config could support that, so a successful call proves + // supports() is not paying for it on every request. + $manager = $this->createManager(['provider1' => [ + 'metadata_url' => 'https://unreachable.invalid/.well-known/openid-configuration', + 'client_id' => 'id', + 'client_secret' => 'secret', + 'redirect_uri' => 'https://app.example.org/callback_uri', + ]]); + + $this->assertSame(['provider1' => '/callback_uri'], $manager->getRedirectUriPaths()); + } } diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 939623a..ac9bfdc 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -18,7 +18,9 @@ use PHPUnit\Framework\TestCase; use Psr\Log\LogLevel; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; use Symfony\Component\Security\Core\Exception\AuthenticationException; class OpenIdLoginAuthenticatorTest extends TestCase @@ -38,17 +40,104 @@ protected function setUp(): void $this->authenticator->setLogger($this->logger); } - public function testSupports(): void + /** + * @param array $paths + */ + private function authenticatorWithPaths(array $paths): TestAuthenticator { - $request = new Request(); + $manager = $this->createStub(OpenIdConfigurationProviderManager::class); + $manager->method('getRedirectUriPaths')->willReturn($paths); + $manager->method('getProviderKeys')->willReturn(array_keys($paths)); + + $authenticator = new TestAuthenticator($manager); + $authenticator->setLogger($this->logger); - $this->assertFalse($this->authenticator->supports($request)); + return $authenticator; + } + + /** + * `state` and `code` are necessary but no longer sufficient: without the path + * check any URL under the firewall is a callback, so an unauthenticated caller + * can turn any page into a failed login — a 500, since the bundle fails closed. + * + * @return iterable + */ + public static function callbackPathProvider(): iterable + { + yield 'the configured path' => ['/callback_uri', true]; + yield 'trailing slash is the same path' => ['/callback_uri/', true]; + yield 'another provider on this authenticator' => ['/other_callback', true]; + yield 'a protected page' => ['/protected', false]; + yield 'the root' => ['/', false]; + yield 'below the callback path' => ['/callback_uri/extra', false]; + yield 'above the callback path' => ['/callback', false]; + yield 'differing in case' => ['/Callback_Uri', false]; + yield 'the path as a query parameter' => ['/protected/callback_uri', false]; + } + + #[DataProvider('callbackPathProvider')] + public function testSupportsOnlyTheConfiguredCallbackPaths(string $path, bool $expected): void + { + $authenticator = $this->authenticatorWithPaths([ + 'test_provider_1' => '/callback_uri', + 'test_provider_2' => '/other_callback', + ]); + + $request = Request::create($path.'?state=abcd&code=xyz'); + + $this->assertSame($expected, $authenticator->supports($request)); + } + + /** + * @return iterable}> + */ + public static function incompleteCallbackProvider(): iterable + { + yield 'neither' => [[]]; + yield 'state only' => [['state' => 'abcd']]; + yield 'code only' => [['code' => 'xyz']]; + } + + #[DataProvider('incompleteCallbackProvider')] + public function testTheRightPathAloneIsNotACallback(array $query): void + { + $authenticator = $this->authenticatorWithPaths(['test_provider_1' => '/callback_uri']); + + $this->assertFalse($authenticator->supports(Request::create('/callback_uri?'.http_build_query($query)))); + } + + /** + * A subclass bound to one provider does not answer another provider's callback, + * which is what lets one authenticator per provider share a firewall. + */ + public function testASubclassCanNarrowTheProvidersItAnswersFor(): void + { + $manager = $this->createStub(OpenIdConfigurationProviderManager::class); + $manager->method('getRedirectUriPaths')->willReturn([ + 'test_provider_1' => '/callback_uri', + 'test_provider_2' => '/other_callback', + ]); + $manager->method('getProviderKeys')->willReturn(['test_provider_1', 'test_provider_2']); + + $authenticator = new SingleProviderAuthenticator($manager); + + $this->assertTrue($authenticator->supports(Request::create('/callback_uri?state=a&code=b'))); + $this->assertFalse($authenticator->supports(Request::create('/other_callback?state=a&code=b'))); + } + + /** + * A provider with no derivable path contributes no match rather than matching + * everything, which would be the bug this constraint removes. + */ + public function testAProviderWithoutAPathMatchesNothing(): void + { + $manager = $this->createStub(OpenIdConfigurationProviderManager::class); + $manager->method('getRedirectUriPaths')->willReturn([]); + $manager->method('getProviderKeys')->willReturn(['test_provider_1']); - $request->query->set('state', 'abcd'); - $this->assertFalse($this->authenticator->supports($request)); + $authenticator = new TestAuthenticator($manager); - $request->query->set('code', 'xyz'); - $this->assertTrue($this->authenticator->supports($request)); + $this->assertFalse($authenticator->supports(Request::create('/callback_uri?state=a&code=b'))); } /** @@ -370,4 +459,71 @@ private function setSessionOnRequest(Request $request, ?string $nonce = 'test_no $request->setSession($stubSession); } + + /** + * The property above is deliberately typed as the abstract class, so the fixture + * method exposing the protected helper needs a concrete local. + */ + private function fixtureAuthenticator(): TestAuthenticator + { + $authenticator = new TestAuthenticator($this->stubProviderManager); + $authenticator->setLogger($this->logger); + + return $authenticator; + } + + private function requestWithSession(?string $targetPath): Request + { + $request = new Request(); + $session = new Session(new MockArraySessionStorage()); + + if (null !== $targetPath) { + $session->set('_security.main.target_path', $targetPath); + } + + $request->setSession($session); + + return $request; + } + + public function testTheRequestedPageIsReturnedToAndThenForgotten(): void + { + $request = $this->requestWithSession('/admin/reports'); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/admin/reports', $response->getTargetUrl()); + // Cleared, so a later visit to the login link does not replay it. + $this->assertFalse($request->getSession()->has('_security.main.target_path')); + } + + /** + * @return iterable + */ + public static function noTargetPathProvider(): iterable + { + yield 'nothing saved' => [null]; + yield 'saved but empty' => ['']; + } + + #[DataProvider('noTargetPathProvider')] + public function testTheFallbackIsUsedWhenNoPageWasRequested(?string $targetPath): void + { + // A user who went to the login link directly, rather than being sent there. + $request = $this->requestWithSession($targetPath); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/dashboard', $response->getTargetUrl()); + } + + public function testTheTargetPathIsReadForTheRightFirewall(): void + { + $request = $this->requestWithSession('/admin/reports'); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'other_firewall', '/dashboard'); + + $this->assertSame('/dashboard', $response->getTargetUrl()); + $this->assertTrue($request->getSession()->has('_security.main.target_path'), 'Another firewall\'s target path is left alone'); + } } diff --git a/tests/Security/SingleProviderAuthenticator.php b/tests/Security/SingleProviderAuthenticator.php new file mode 100644 index 0000000..4bdc7da --- /dev/null +++ b/tests/Security/SingleProviderAuthenticator.php @@ -0,0 +1,16 @@ +createTargetPathRedirect($request, $firewallName, $fallbackUrl); + } } diff --git a/tests/config/routes.yml b/tests/config/routes.yml index 8940aa2..e5b2e76 100644 --- a/tests/config/routes.yml +++ b/tests/config/routes.yml @@ -1,3 +1,9 @@ protected: path: /protected controller: ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController + +# The path of the test providers' redirect_uri. supports() only recognises a +# callback here, so a request has to be able to reach it. +callback: + path: /callback_uri + controller: ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController diff --git a/tests/config/security_consumer.yml b/tests/config/security_consumer.yml index 588dabd..a04d795 100644 --- a/tests/config/security_consumer.yml +++ b/tests/config/security_consumer.yml @@ -19,3 +19,7 @@ security: - ItkDev\OpenIdConnectBundle\Security\CliLoginTokenAuthenticator entry_point: ItkDev\OpenIdConnectBundle\Tests\Security\ConsumerAuthenticator provider: test_users_1 + # Without this nothing demands authentication, so the entry point never fires and + # there is no target path to save — the behaviour these tests are about. + access_control: + - { path: "^/protected", roles: "ROLE_ADMIN" } From fed54eaa27e844a910c65cc0bb6c207222b7d179 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 20 Aug 2026 11:23:15 +0200 Subject: [PATCH 13/28] docs: ADR 003, upgrade notes and README for the callback path constraint Also corrects the ADR index, which still listed 002 as Draft, and a stale parent::__construct($providerManager, $requestStack) in the README example. --- CHANGELOG.md | 18 +++++ README.md | 64 +++++++++++++++- UPGRADE-6.0.md | 44 +++++++++++ ...003-constrain-supports-to-callback-path.md | 73 +++++++++++++++++++ docs/adr/README.md | 5 +- 5 files changed, 200 insertions(+), 4 deletions(-) create mode 100644 docs/adr/003-constrain-supports-to-callback-path.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 36b9a15..bd0ca87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `supports()` no longer treats `?state=…&code=…` on an arbitrary path as a + callback (#63). A forged callback is handled by the firewall — an entry point + redirect for anonymous visitors — instead of surfacing as a 500 that any + unauthenticated caller could raise on any URL. - `logging_options.logger` no longer depends on bundle registration order. FrameworkBundle autoconfigures a `setLogger()` call onto every `LoggerAwareInterface` service and the last call wins, so an application @@ -31,12 +35,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `getPrevious()` on that exception is the underlying OpenID Connect exception rather than the `AuthenticationException`, which the security component would have followed straight back into the loop. +- Each provider must declare `redirect_uri`, `redirect_route` or `callback_path`. + It is how a callback is recognised, so a provider without one could never + complete a login. Enforced while the container compiles. - `client_secret_expires_at` is now required for every provider, and must be a string. A missing date fails while the container compiles instead of emitting the 5.1 deprecation, and an unquoted `2027-01-31` — which YAML reads as a number — is rejected rather than silently leaving the provider unmonitored. See `UPGRADE-6.0.md`. +### Added + +- `callback_path` per provider, for deployments where a reverse proxy rewrites the + path so that the path of `redirect_uri` is not the one the application receives. +- `OpenIdLoginAuthenticator::getSupportedProviderKeys()`, to narrow an + authenticator to the providers whose callbacks it answers. Defaults to all of + them, so existing multi-authenticator firewalls are unaffected. +- `OpenIdLoginAuthenticator::createTargetPathRedirect()`, for returning the user to + the page that sent them to log in. +- `OpenIdConfigurationProviderManager::getRedirectUriPaths()`. + ### Removed (BREAKING) - `ItkOpenIdConnectBundleException`, `@deprecated` since 5.0. Catch diff --git a/README.md b/README.md index fc7f06c..0778ed8 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,11 @@ itkdev_openid_connect: client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' # Specify redirect URI redirect_uri: '%env(string:ADMIN_OIDC_REDIRECT_URI)%' + # Optional: the path the callback arrives on, when a reverse proxy rewrites + # it so the path of redirect_uri is not the one this application + # sees. Defaults to the path of redirect_uri, or of the generated + # redirect_route. See "Which requests count as a callback" below. + callback_path: '/auth/callback' # Optional: Specify leeway (seconds) to account for clock skew between provider and hosting # Defaults to 10 leeway: '%env(int:ADMIN_OIDC_LEEWAY)%' @@ -643,7 +648,8 @@ class SomeAuthenticator extends OpenIdLoginAuthenticator public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response { - // TODO: Implement onAuthenticationSuccess() method. + // Back to whatever the user was trying to reach, or your default. + return $this->createTargetPathRedirect($request, $firewallName, '/'); } public function start(Request $request, AuthenticationException $authException = null) @@ -668,6 +674,54 @@ security: entry_point: App\Security\ExampleAuthenticator ``` +With one authenticator per provider, override `getSupportedProviderKeys()` in each so +it only answers its own provider's callback: + +```php +protected function getSupportedProviderKeys(): array +{ + return ['admin']; +} +``` + +Without the override every authenticator supports every callback path, Symfony asks +them in the order above, and the session's provider key decides which provider +validates the callback — which is how existing setups already work. + +#### Which requests count as a callback + +A request is treated as an OpenID Connect callback when it carries both `state` and +`code` **and** arrives on a provider's configured callback path — the path of +`redirect_uri`, of the generated `redirect_route`, or `callback_path` when set. Every +provider must declare one of the three. + +`?state=…&code=…` on any other URL is ignored by the authenticator, and the firewall +handles the request as it would without them: an anonymous visitor is sent to your +entry point, a logged-in one gets the page. + +Set `callback_path` when a reverse proxy rewrites the path, so that an external +`https://app.example.org/prefix/auth/callback` reaches this application as +`/auth/callback`. A proxy that sends `X-Forwarded-Prefix`, with Symfony's trusted +proxies configured, needs no `callback_path` — the path then already matches. + +#### Returning to the originally requested page + +`createTargetPathRedirect()` sends the user back to the page that triggered the login, +falling back to a URL of your choosing when there is nothing to go back to: + +```php +public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response +{ + return $this->createTargetPathRedirect($request, $firewallName, $this->router->generate('dashboard')); +} +``` + +Symfony saves the requested page when your entry point fires, so this works both for +applications that redirect straight to the identity provider and for those that show a +login screen with a provider link on it. The fallback covers a user who went to the +login link directly. The saved page is cleared on use, so a later visit to that link +does not replay it. + #### Example authenticator functions Here is an example using a `User` with a name and email property. First we @@ -713,7 +767,7 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator private readonly UrlGeneratorInterface $router, private readonly OpenIdConfigurationProviderManager $providerManager ) { - parent::__construct($providerManager, $requestStack); + parent::__construct($providerManager); } /** @inheritDoc */ @@ -750,7 +804,11 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator /** @inheritDoc */ public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response { - return new RedirectResponse($this->router->generate('homepage_authenticated')); + return $this->createTargetPathRedirect( + $request, + $firewallName, + $this->router->generate('homepage_authenticated') + ); } /** @inheritDoc */ diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index 9b4c74f..6aecad2 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -90,6 +90,50 @@ switch to `UserNotFoundException`: `UserLoginCommand` catches that and reports t username as unknown. Not to be confused with `UsernameDoesNotExistException`, which stays — the CLI authenticator throws it when a token resolves to no username. +## Callbacks are only accepted on the configured callback path + +A request counts as a callback when it carries `state` and `code` **and** arrives on a +provider's callback path. `?state=…&code=…` on any other URL is left to the firewall, +as it was before 6.0 — which is the point: since a failed callback now escapes as an +exception, any URL was otherwise a 500 an anonymous caller could trigger. + +Every provider must declare one of `redirect_uri`, `redirect_route` or the new +`callback_path`, or the container will not compile: + +```text +One of redirect_uri, redirect_route or callback_path must be set: it is how a +callback is recognised. +``` + +Set `callback_path` if a reverse proxy rewrites the path, so an external +`https://app.example.org/prefix/auth/callback` arrives here as `/auth/callback`: + +```yaml +openid_providers: + admin: + options: + redirect_uri: 'https://app.example.org/prefix/auth/callback' + callback_path: '/auth/callback' +``` + +A proxy sending `X-Forwarded-Prefix` with trusted proxies configured needs none: the +path already matches. If you run one authenticator per provider, override +`getSupportedProviderKeys()` so each answers only its own callback; without it they +behave exactly as in 5.x. + +## Redirecting back to the originally requested page + +`createTargetPathRedirect()` returns the user to the page that sent them to log in: + +```php +public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response +{ + return $this->createTargetPathRedirect($request, $firewallName, $this->router->generate('dashboard')); +} +``` + +Optional — existing `onAuthenticationSuccess()` implementations keep working. + ## CLI login is unchanged `CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed diff --git a/docs/adr/003-constrain-supports-to-callback-path.md b/docs/adr/003-constrain-supports-to-callback-path.md new file mode 100644 index 0000000..a2dc152 --- /dev/null +++ b/docs/adr/003-constrain-supports-to-callback-path.md @@ -0,0 +1,73 @@ +# 003: Treat only the configured callback path as a callback + +- **Created By:** Ture Gjørup +- **Date:** 2026-08-20 +- **Decision Maker:** Ture Gjørup +- **Stakeholders:** Bundle consumers; operators of those applications; bundle + maintainers +- **Status:** Accepted + +## Context + +`OpenIdLoginAuthenticator::supports()` matched on `state` and `code` alone, so every +URL behind the firewall was a potential callback. Before 6.0 a stray or forged +callback degraded quietly: the failure was an `AuthenticationException`, so the +firewall answered with a redirect to the entry point. [ADR 002](002-fail-closed-on-authentication-failure.md) +made that failure escape as `AuthenticationFailedException`, which turned the same +request into a 500 — raisable on any URL by an unauthenticated caller, and noise for +error reporting. Issue #63. + +## Options Considered + +1. **Match the provider's configured callback path (chosen).** The path comes from + configuration the consumer already writes, and a request that is not a callback is + left to the firewall. +2. **Require the session to hold `oauth2provider`.** No new configuration, but it + reinstates the outage: a lost session would make the request merely + unauthenticated, so the firewall calls the entry point, the provider returns a + fresh `code`, and it arrives back with the session still broken — the loop of + 2026-08-12. It also conflates "is this a callback" with "did this browser start a + login", and touching the session in `supports()` starts one for anonymous traffic. +3. **Leave it and filter in error reporting.** Moves a bundle defect into every + consumer's monitoring configuration. + +## Decision + +Adopt option 1 in 6.0.0. `supports()` requires `state`, `code`, and a path matching +one of this authenticator's providers. + +- **Paths come from configuration, not from a provider instance.** Building a provider + pulls in discovery, an HTTP client and a cache pool; `supports()` runs on every + request. `OpenIdConfigurationProviderManager::getRedirectUriPaths()` derives and + memoizes them. +- **`callback_path` is the escape hatch** for a proxy that rewrites the path, where the + external `redirect_uri` path is not the one the application sees. +- **`redirect_route` is generated as `ABSOLUTE_PATH`**, so host and scheme + requirements on the route do not enter the comparison; a route whose path varies by + host is not supported. +- **A provider must declare `redirect_uri`, `redirect_route` or `callback_path`.** + Enforced when the container compiles. A provider with none has no callback path, and + "matches every path" is the defect being removed. +- **`getSupportedProviderKeys()`** defaults to every provider, so existing + multi-authenticator firewalls behave as before, and can be overridden by an + authenticator bound to one provider. +- **Nothing is logged from `supports()`.** It runs pre-authentication on every + request; a log call there is an amplifier for anyone sending traffic. The firewall's + own handling is the record. + +## Consequences + +- A forged callback is handled by the firewall again, as it was before 6.0, without + giving up fail-closed behaviour for real callbacks. +- Consumers behind a rewriting proxy must set `callback_path`, and every provider must + declare a callback target. See `UPGRADE-6.0.md`. +- The callback path is now part of the bundle's contract with the identity provider: + changing `redirect_uri` without changing the registration at the provider fails in + the same way it always did, but changing it *only* at the provider now also stops + callbacks being recognised. + +## References + +- [ADR 002](002-fail-closed-on-authentication-failure.md) — the fail-closed decision + that made this worth fixing now +- Issue #63; the deferred `?target_path=` follow-up is filed separately diff --git a/docs/adr/README.md b/docs/adr/README.md index a032376..0b58831 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -13,5 +13,8 @@ context that drove them and the consequences we accept. See bundle](001-marker-interface-exception-hierarchy.md)** — Draft — 2026-05-11 - **[002 — Fail closed when an OpenID Connect callback cannot be - validated](002-fail-closed-on-authentication-failure.md)** — Draft — + validated](002-fail-closed-on-authentication-failure.md)** — Accepted — 2026-08-19 +- **[003 — Treat only the configured callback path as a + callback](003-constrain-supports-to-callback-path.md)** — Accepted — + 2026-08-20 From 33afa710305b27a46ac766368a859b2b26dbf587 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 20 Aug 2026 11:34:49 +0200 Subject: [PATCH 14/28] test: prove a deep link survives the login round trip, and document its limits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kernel test covered only the firewall's half — that the requested page is saved. This adds the whole trip: a link to a protected page is denied, the user comes back from the identity provider, and lands on that page rather than a default. The fixture authenticator now uses createTargetPathRedirect(), as the README tells consumers to. The deep link needed a route. Routing runs before security (RouterListener at priority 32 on kernel.request, the firewall at 8), so an unrouted path is a 404 before the firewall is reached: no entry point, nothing saved, and a test that measures 404 handling instead of the round trip. Documented, since it is also the answer to why a link to a non-existent page does not come back after login. --- README.md | 7 +++ UPGRADE-6.0.md | 4 ++ tests/Security/ConsumerAuthenticator.php | 4 +- .../FailedCallbackDoesNotLoopTest.php | 59 +++++++++++++++++++ tests/config/routes.yml | 6 ++ 5 files changed, 79 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 0778ed8..bb25b60 100644 --- a/README.md +++ b/README.md @@ -722,6 +722,13 @@ login screen with a provider link on it. The fallback covers a user who went to login link directly. The saved page is cleared on use, so a later visit to that link does not replay it. +Only pages that exist and are access-controlled return this way, and that is by +design. Routing runs before security — `RouterListener` on `kernel.request` at +priority 32, the firewall at 8 — so a link to a URL with no route is a 404 before the +firewall is reached: no entry point fires, nothing is saved, and there is nothing to +come back to. A link to a page that exists but is public simply loads. Neither is +affected by the login flow. + #### Example authenticator functions Here is an example using a `User` with a name and email property. First we diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index 6aecad2..0d13273 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -134,6 +134,10 @@ public function onAuthenticationSuccess(Request $request, TokenInterface $token, Optional — existing `onAuthenticationSuccess()` implementations keep working. +Only pages that exist and are access-controlled come back this way: routing runs +before security, so a link to a URL with no route is a 404 before the firewall sees +it and there is nothing to return to. + ## CLI login is unchanged `CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed diff --git a/tests/Security/ConsumerAuthenticator.php b/tests/Security/ConsumerAuthenticator.php index 373bf34..1477114 100644 --- a/tests/Security/ConsumerAuthenticator.php +++ b/tests/Security/ConsumerAuthenticator.php @@ -27,6 +27,7 @@ class ConsumerAuthenticator extends OpenIdLoginAuthenticator { public const string LOGIN_PATH = '/openidconnect/login/test_provider_1'; + public const string FALLBACK_PATH = '/dashboard'; public function authenticate(Request $request): Passport { @@ -46,7 +47,8 @@ public function authenticate(Request $request): Passport public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response { - return null; + // As the README tells consumers to write it. + return $this->createTargetPathRedirect($request, $firewallName, self::FALLBACK_PATH); } public function start(Request $request, ?AuthenticationException $authException = null): Response diff --git a/tests/Security/FailedCallbackDoesNotLoopTest.php b/tests/Security/FailedCallbackDoesNotLoopTest.php index 1927348..5a83edb 100644 --- a/tests/Security/FailedCallbackDoesNotLoopTest.php +++ b/tests/Security/FailedCallbackDoesNotLoopTest.php @@ -7,10 +7,12 @@ use ItkDev\OpenIdConnectBundle\Tests\ItkDevOpenIdConnectBundleTestingKernel; use ItkDev\OpenIdConnectBundle\Tests\RestoresExceptionHandlers; use PHPUnit\Framework\TestCase; +use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; +use Symfony\Component\Security\Core\Authentication\Token\PreAuthenticatedToken; use Symfony\Component\Security\Core\Exception\AuthenticationException; /** @@ -160,4 +162,61 @@ public function testTheEntryPointSavesTheRequestedPage(): void $this->assertSame(ConsumerAuthenticator::LOGIN_PATH, $response->headers->get('Location')); $this->assertSame('http://localhost/protected', $session->get('_security.main.target_path')); } + + /** + * The deep link, end to end. + * + * A user follows a link to a page they cannot see yet, logs in through the + * identity provider, and lands on the page they asked for — not on a default. + * Both halves are needed and neither is enough: the firewall saves the target + * when the entry point fires, and the authenticator reads it back on success. + */ + public function testADeepLinkSurvivesTheLoginRoundTrip(): void + { + $deepLink = Request::create('/protected/report/7'); + $session = new Session(new MockArraySessionStorage()); + $deepLink->setSession($session); + + // Leg one: denied, sent to the login flow, target remembered. + $response = $this->kernel->handle($deepLink, catch: true); + $this->assertSame(ConsumerAuthenticator::LOGIN_PATH, $response->headers->get('Location')); + + // Leg two: the same session, now arriving back from the identity provider. + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $callback = Request::create('/callback_uri?state=s&code=c'); + $callback->setSession($session); + + $success = $authenticator->onAuthenticationSuccess( + $callback, + new PreAuthenticatedToken(new TestUser('someone@example.com'), 'main'), + 'main' + ); + + $this->assertInstanceOf(RedirectResponse::class, $success); + $this->assertSame('http://localhost/protected/report/7', $success->getTargetUrl()); + } + + /** + * Nothing was requested, so there is nothing to return to: a user who went + * straight to the login link gets the application's default. + */ + public function testWithoutARequestedPageTheFallbackIsUsed(): void + { + $authenticator = $this->kernel->getContainer()->get(ConsumerAuthenticator::class); + $this->assertInstanceOf(ConsumerAuthenticator::class, $authenticator); + + $callback = Request::create('/callback_uri?state=s&code=c'); + $callback->setSession(new Session(new MockArraySessionStorage())); + + $success = $authenticator->onAuthenticationSuccess( + $callback, + new PreAuthenticatedToken(new TestUser('someone@example.com'), 'main'), + 'main' + ); + + $this->assertInstanceOf(RedirectResponse::class, $success); + $this->assertSame(ConsumerAuthenticator::FALLBACK_PATH, $success->getTargetUrl()); + } } diff --git a/tests/config/routes.yml b/tests/config/routes.yml index e5b2e76..17a49f6 100644 --- a/tests/config/routes.yml +++ b/tests/config/routes.yml @@ -7,3 +7,9 @@ protected: callback: path: /callback_uri controller: ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController + +# A deeper protected page, to check a link into the application survives the login +# round trip rather than collapsing to a default. +protected_report: + path: /protected/report/{id} + controller: ItkDev\OpenIdConnectBundle\Tests\Security\ProtectedController From 5b08e49b10cb3cdf528fd93e760d4526ec889b61 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 20 Aug 2026 11:36:19 +0200 Subject: [PATCH 15/28] docs: drop ADR 003's claim of a filed follow-up issue --- docs/adr/003-constrain-supports-to-callback-path.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/adr/003-constrain-supports-to-callback-path.md b/docs/adr/003-constrain-supports-to-callback-path.md index a2dc152..e7a66db 100644 --- a/docs/adr/003-constrain-supports-to-callback-path.md +++ b/docs/adr/003-constrain-supports-to-callback-path.md @@ -70,4 +70,11 @@ one of this authenticator's providers. - [ADR 002](002-fail-closed-on-authentication-failure.md) — the fail-closed decision that made this worth fixing now -- Issue #63; the deferred `?target_path=` follow-up is filed separately +- Issue #63 + +## Not decided here + +A login link followed from a public page has no requested page to return to, so it +lands on the application's fallback. Letting the link name its own destination +(`?target_path=`) would need the firewall name, which `LoginController` does not have, +and hard validation against open redirects. Left alone until a consumer needs it. From 26e467ea3ab55ffa5217f80ab3ccf1c127c326b7 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 20 Aug 2026 12:50:35 +0200 Subject: [PATCH 16/28] feat: let a login link name where to return to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createTargetPathRedirect() could only return users to a page the firewall had denied. A login link on a public page — a header "Log in" button — denies nothing, so there was nothing saved and those users landed on the application's fallback. They can now name the destination: ?target_path=/admin/reports. Stored under a bundle-private session key, not TargetPathTrait's, which is keyed by firewall: LoginController has no firewall name, and writing there would put a value in the firewall's own record that the firewall never saved. A page the firewall denied still wins, since that is what the user was actually stopped from reaching, and both keys are cleared on use. The value reaches a Location header, so it is validated as a path within the application — one leading slash, no backslash, no scheme separator, no control characters — and dropped with a warning otherwise. Correcting one would be guessing at intent on a security boundary. --- CHANGELOG.md | 4 + README.md | 12 +++ UPGRADE-6.0.md | 4 + ...003-constrain-supports-to-callback-path.md | 20 +++-- src/Controller/LoginController.php | 60 +++++++++++++++ src/Security/OpenIdLoginAuthenticator.php | 32 ++++++-- tests/Controller/LoginControllerTest.php | 77 +++++++++++++++++++ .../Security/OpenIdLoginAuthenticatorTest.php | 50 ++++++++++++ 8 files changed, 249 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd0ca87..a9009f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 them, so existing multi-authenticator firewalls are unaffected. - `OpenIdLoginAuthenticator::createTargetPathRedirect()`, for returning the user to the page that sent them to log in. +- `?target_path=` on the login route, for a login link on a public page where the + firewall saved no requested page. Validated as a path within the application and + otherwise dropped and logged, since the value reaches a `Location` header. A page + the firewall denied takes precedence over it. - `OpenIdConfigurationProviderManager::getRedirectUriPaths()`. ### Removed (BREAKING) diff --git a/README.md b/README.md index bb25b60..040bc35 100644 --- a/README.md +++ b/README.md @@ -722,6 +722,18 @@ login screen with a provider link on it. The fallback covers a user who went to login link directly. The saved page is cleared on use, so a later visit to that link does not replay it. +For a login link on a public page, where nothing was denied and so nothing was saved, +name the destination on the link itself: + +```twig +Log in +``` + +The value must be a path within the application: a single leading `/`, no backslash, +no `://`, no control characters. Anything else is dropped and logged at `warning`, +because it would otherwise turn the login route into an open redirect. When a page was +also denied, that page wins — it is what the user was actually stopped from reaching. + Only pages that exist and are access-controlled return this way, and that is by design. Routing runs before security — `RouterListener` on `kernel.request` at priority 32, the firewall at 8 — so a link to a URL with no route is a 404 before the diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index 0d13273..7676ca8 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -138,6 +138,10 @@ Only pages that exist and are access-controlled come back this way: routing runs before security, so a link to a URL with no route is a 404 before the firewall sees it and there is nothing to return to. +A login link on a public page can name its own destination with +`?target_path=/admin/reports`. The value must be a path within the application, or it +is dropped and logged. + ## CLI login is unchanged `CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed diff --git a/docs/adr/003-constrain-supports-to-callback-path.md b/docs/adr/003-constrain-supports-to-callback-path.md index e7a66db..bc65a72 100644 --- a/docs/adr/003-constrain-supports-to-callback-path.md +++ b/docs/adr/003-constrain-supports-to-callback-path.md @@ -72,9 +72,19 @@ one of this authenticator's providers. that made this worth fixing now - Issue #63 -## Not decided here +## Returning to a page the firewall never saw -A login link followed from a public page has no requested page to return to, so it -lands on the application's fallback. Letting the link name its own destination -(`?target_path=`) would need the firewall name, which `LoginController` does not have, -and hard validation against open redirects. Left alone until a consumer needs it. +Symfony saves the requested page when the entry point fires, which covers a user who +was denied something. A login link followed from a public page has no such record, so +`?target_path=` on the login route lets the link name where to go. + +It is stored under a bundle-private session key rather than in `TargetPathTrait`'s +slot: that slot is keyed by firewall, `LoginController` has no firewall name, and +writing there would put a value in the firewall's own record that the firewall never +saved. `createTargetPathRedirect()` prefers the firewall's record when both exist — +that is the page the user was actually stopped from reaching — and clears both. + +The parameter ends up in a `Location` header, so it is validated as a path within the +application and dropped otherwise: a single leading `/`, no backslashes, no scheme +separator, no control characters. A rejected value is logged at `warning`; correcting +one would be guessing at intent on a security boundary. diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index b40a76f..38e6704 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -5,6 +5,7 @@ use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; +use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Util\ClientSecretExpiryChecker; use Psr\Log\LoggerInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; @@ -19,6 +20,11 @@ */ class LoginController extends AbstractController { + /** + * Query parameter naming where to go after a successful login. + */ + public const string TARGET_PATH_PARAMETER = 'target_path'; + public function __construct( private readonly OpenIdConfigurationProviderManager $providerManager, private readonly LoggerInterface $logger, @@ -52,6 +58,8 @@ public function login(Request $request, SessionInterface $session, string $provi $nonce = $provider->generateNonce(); $state = $provider->generateState(); + $this->rememberNamedTargetPath($request, $session); + // Save to session $session->set('oauth2provider', $providerKey); $session->set('oauth2state', $state); @@ -91,6 +99,58 @@ public function login(Request $request, SessionInterface $session, string $provi * when it does stop working, the reason is already in the log rather than * something to be worked out afterwards. */ + /** + * Remember a return target named on the login link. + * + * For a login link followed from a public page: the firewall saves nothing, + * because nothing was denied, so there is no requested page for + * `createTargetPathRedirect()` to return to. A link may name one instead. + * + * Anything not plainly a path within this application is dropped rather than + * corrected. This value ends up in a `Location` header after a successful login, + * so a permissive reading turns the login route into an open redirect for anyone + * who can get a user to follow a link. + */ + private function rememberNamedTargetPath(Request $request, SessionInterface $session): void + { + $target = $request->query->get(self::TARGET_PATH_PARAMETER); + + if (null === $target) { + return; + } + + if (!self::isLocalPath($target)) { + $this->logger->warning('OIDC login: ignoring an unusable target_path', [ + 'target_path' => $target, + ]); + + return; + } + + $session->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, $target); + } + + /** + * Whether a value is a path into this application and nothing else. + * + * Rejected, in order: anything not starting with a single `/` (absolute URLs, + * scheme-relative `//host`, bare words); a backslash anywhere, since browsers + * have historically read `/\host` as scheme-relative; a scheme separator + * anywhere; and control characters, which belong to header-splitting attempts. + */ + private static function isLocalPath(string $target): bool + { + if (!str_starts_with($target, '/') || str_starts_with($target, '//')) { + return false; + } + + if (str_contains($target, '\\') || str_contains($target, '://')) { + return false; + } + + return 1 !== preg_match('/[\x00-\x1F\x7F]/', $target); + } + private function checkClientSecretExpiry(string $providerKey): void { $expiry = $this->expiryChecker->getStatus($providerKey); diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index a6b0df2..87b2da8 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -49,6 +49,15 @@ abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements { use TargetPathTrait; + /** + * Where `LoginController` puts a target named on the login link itself. + * + * Not `TargetPathTrait`'s key, which is per firewall: the controller has no + * firewall name, and inventing one to write into Symfony's slot would put a value + * there that the firewall never saved. + */ + public const string TARGET_PATH_SESSION_KEY = '_itkdev_oidc.target_path'; + private LoggerInterface $logger; /** @@ -115,15 +124,28 @@ public function supports(Request $request): ?bool */ protected function createTargetPathRedirect(Request $request, string $firewallName, string $fallbackUrl): RedirectResponse { - $targetPath = $this->getTargetPath($request->getSession(), $firewallName); + $session = $request->getSession(); - if (null === $targetPath || '' === $targetPath) { - return new RedirectResponse($fallbackUrl); + // The firewall's record first: it is the page the user was actually denied. + $targetPath = $this->getTargetPath($session, $firewallName); + + if (null !== $targetPath && '' !== $targetPath) { + $this->removeTargetPath($session, $firewallName); + $session->remove(self::TARGET_PATH_SESSION_KEY); + + return new RedirectResponse($targetPath); } - $this->removeTargetPath($request->getSession(), $firewallName); + // Then a target named on the login link, for a user who was never denied + // anything — they followed a login link from a public page. + $named = $session->get(self::TARGET_PATH_SESSION_KEY); + $session->remove(self::TARGET_PATH_SESSION_KEY); + + if (is_string($named) && '' !== $named) { + return new RedirectResponse($named); + } - return new RedirectResponse($targetPath); + return new RedirectResponse($fallbackUrl); } /** diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index fa1ed9b..ca8288d 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -9,6 +9,7 @@ use ItkDev\OpenIdConnectBundle\Controller\LoginController; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; +use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; use ItkDev\OpenIdConnectBundle\Util\ClientSecretExpiryChecker; use PHPUnit\Framework\Attributes\DataProvider; @@ -16,7 +17,9 @@ use Psr\Log\LogLevel; use Symfony\Component\Clock\MockClock; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\HttpKernel\Exception\ServiceUnavailableHttpException; @@ -264,4 +267,78 @@ private function createController(OpenIdConfigurationProvider $provider, ?Client return new LoginController($mockProviderManager, $this->logger, $expiryChecker ?? $this->createExpiryChecker()); } + + private function loginWith(?string $target): Session + { + // A stub, not a mock: nothing here asserts on the provider itself. + $provider = $this->createStub(OpenIdConfigurationProvider::class); + $provider->method('generateNonce')->willReturn('1234'); + $provider->method('generateState')->willReturn('abcd'); + $provider->method('getAuthorizationUrl')->willReturn('https://provider.example.org/authorize'); + + $query = ['provider' => 'test']; + + if (null !== $target) { + $query[LoginController::TARGET_PATH_PARAMETER] = $target; + } + + $session = new Session(new MockArraySessionStorage()); + $this->createController($provider)->login(new Request(query: $query), $session, 'test'); + + return $session; + } + + public function testATargetPathOnTheLinkIsRemembered(): void + { + $session = $this->loginWith('/admin/reports?page=2'); + + $this->assertSame('/admin/reports?page=2', $session->get(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + $this->assertSame([], $this->logger->records); + } + + public function testWithoutATargetPathNothingIsRememberedOrLogged(): void + { + $session = $this->loginWith(null); + + $this->assertFalse($session->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + $this->assertSame([], $this->logger->records); + } + + /** + * This value reaches a `Location` header after a successful login, so anything + * that is not plainly a path inside this application would make the login route + * an open redirect for anyone who can get a user to follow a link. + * + * @return iterable + */ + public static function unusableTargetPathProvider(): iterable + { + yield 'absolute url' => ['https://evil.example.org/phish']; + yield 'scheme relative' => ['//evil.example.org/phish']; + yield 'backslash scheme relative' => ['/\evil.example.org/phish']; + yield 'backslash anywhere' => ['/admin\reports']; + yield 'a scheme further in' => ['/redirect?to=https://evil.example.org']; + yield 'no leading slash' => ['admin/reports']; + yield 'empty' => ['']; + yield 'a bare word' => ['dashboard']; + yield 'header split attempt' => ["/admin\r\nSet-Cookie: session=stolen"]; + yield 'null byte' => ["/admin\0/reports"]; + yield 'javascript' => ['javascript:alert(1)']; + } + + #[DataProvider('unusableTargetPathProvider')] + public function testAnUnusableTargetPathIsDroppedAndReported(string $target): void + { + $session = $this->loginWith($target); + + $this->assertFalse( + $session->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY), + 'A value that is not a local path must never reach a Location header' + ); + + $record = $this->logger->singleRecord(); + $this->assertSame(LogLevel::WARNING, $record['level']); + $this->assertStringContainsString('ignoring an unusable target_path', $record['message']); + $this->assertSame($target, $record['context']['target_path']); + } } diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index ac9bfdc..876a34d 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -526,4 +526,54 @@ public function testTheTargetPathIsReadForTheRightFirewall(): void $this->assertSame('/dashboard', $response->getTargetUrl()); $this->assertTrue($request->getSession()->has('_security.main.target_path'), 'Another firewall\'s target path is left alone'); } + + public function testATargetNamedOnTheLoginLinkIsUsedWhenNothingWasDenied(): void + { + // The case the firewall cannot cover: the user was never refused anything, so + // Symfony saved nothing. They followed a login link that named where to go. + $request = $this->requestWithSession(null); + $request->getSession()->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, '/admin/reports'); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/admin/reports', $response->getTargetUrl()); + $this->assertFalse($request->getSession()->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY), 'Consumed, so it cannot replay'); + } + + public function testTheDeniedPageWinsOverATargetNamedOnTheLink(): void + { + // Both present: the firewall's record is what the user was actually stopped + // from reaching, so it is the more faithful answer. + $request = $this->requestWithSession('/admin/denied-page'); + $request->getSession()->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, '/admin/reports'); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/admin/denied-page', $response->getTargetUrl()); + // Both cleared, or the unused one would resurface on a later login. + $this->assertFalse($request->getSession()->has('_security.main.target_path')); + $this->assertFalse($request->getSession()->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + } + + /** + * @return iterable + */ + public static function unusableNamedTargetProvider(): iterable + { + yield 'empty' => ['']; + // Nothing writes a non-string, but the session is shared with the application. + yield 'not a string' => [['/admin/reports']]; + } + + #[DataProvider('unusableNamedTargetProvider')] + public function testAnUnusableNamedTargetFallsBack(mixed $stored): void + { + $request = $this->requestWithSession(null); + $request->getSession()->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, $stored); + + $response = $this->fixtureAuthenticator()->callCreateTargetPathRedirect($request, 'main', '/dashboard'); + + $this->assertSame('/dashboard', $response->getTargetUrl()); + $this->assertFalse($request->getSession()->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + } } From f4d76a060266d9d4c30a2cc008a5a49a3215f881 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Thu, 20 Aug 2026 14:25:01 +0200 Subject: [PATCH 17/28] fix: compare the callback path against the base URL as well MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getPathInfo() has both a subdirectory deployment's base path and any trusted X-Forwarded-Prefix stripped out of it: Request::preparePathInfo() subtracts getBaseUrlReal(), while getBaseUrl() is the trusted prefix plus that. The configured paths contain them — a redirect_uri is the URL the identity provider was given, and UrlGenerator prepends the routing context's base URL, which RequestContext::fromRequest() takes from getBaseUrl(). So comparing path info alone rejected every callback both in a subdirectory deployment and behind a prefix-announcing proxy, and the README claimed the opposite of what happened. Derived paths are now memoized per routing-context base URL rather than once, so a service seeing both proxied and direct traffic is not frozen to whichever came first. Also: an empty callback_path no longer normalizes to "/" and shadows redirect_uri; a plain login link clears a target left by an abandoned one; the expiry docblock is back on its own method; and ADR 003 documents the comparison basis rather than trailing a second decision after References. --- README.md | 28 +++-- UPGRADE-6.0.md | 7 +- ...003-constrain-supports-to-callback-path.md | 20 +++- src/Controller/LoginController.php | 28 +++-- src/DependencyInjection/Configuration.php | 2 +- .../OpenIdConfigurationProviderManager.php | 50 ++++++-- src/Security/OpenIdLoginAuthenticator.php | 12 +- tests/Controller/LoginControllerTest.php | 20 ++++ ...OpenIdConfigurationProviderManagerTest.php | 108 ++++++++++++++++-- .../Security/OpenIdLoginAuthenticatorTest.php | 84 +++++++++++--- tests/Security/RequestWithBaseUrl.php | 37 ++++++ 11 files changed, 329 insertions(+), 67 deletions(-) create mode 100644 tests/Security/RequestWithBaseUrl.php diff --git a/README.md b/README.md index 040bc35..fc6dc37 100644 --- a/README.md +++ b/README.md @@ -121,10 +121,10 @@ itkdev_openid_connect: client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' # Specify redirect URI redirect_uri: '%env(string:ADMIN_OIDC_REDIRECT_URI)%' - # Optional: the path the callback arrives on, when a reverse proxy rewrites - # it so the path of redirect_uri is not the one this application - # sees. Defaults to the path of redirect_uri, or of the generated - # redirect_route. See "Which requests count as a callback" below. + # Optional: the path the callback arrives on, for a proxy that rewrites it + # without sending X-Forwarded-Prefix. Defaults to the path of + # redirect_uri, or of the generated redirect_route. See "Which + # requests count as a callback" below. callback_path: '/auth/callback' # Optional: Specify leeway (seconds) to account for clock skew between provider and hosting # Defaults to 10 @@ -699,10 +699,22 @@ provider must declare one of the three. handles the request as it would without them: an anonymous visitor is sent to your entry point, a logged-in one gets the page. -Set `callback_path` when a reverse proxy rewrites the path, so that an external -`https://app.example.org/prefix/auth/callback` reaches this application as -`/auth/callback`. A proxy that sends `X-Forwarded-Prefix`, with Symfony's trusted -proxies configured, needs no `callback_path` — the path then already matches. +The path is matched against `getBaseUrl()` plus `getPathInfo()`, so an application +deployed in a subdirectory, or behind a proxy that sends `X-Forwarded-Prefix` with +Symfony's [trusted proxies](https://symfony.com/doc/current/deployment/proxies.html) +configured, matches without further configuration: the prefix is part of the base URL +on the way in and part of `redirect_uri` on the way out. + +Set `callback_path` when a proxy rewrites the path **without** announcing it — an +external `https://app.example.org/prefix/auth/callback` that arrives here as +`/auth/callback`. Nothing in the request says where the prefix went, so the path has to +be declared: + +```yaml +callback_path: '/auth/callback' +``` + +Give it the path as this application receives it, including any base path of its own. #### Returning to the originally requested page diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index 7676ca8..cb8a074 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -116,8 +116,11 @@ openid_providers: callback_path: '/auth/callback' ``` -A proxy sending `X-Forwarded-Prefix` with trusted proxies configured needs none: the -path already matches. If you run one authenticator per provider, override +A subdirectory deployment, or a proxy sending `X-Forwarded-Prefix` with trusted +proxies configured, needs no `callback_path`: the path is matched against +`getBaseUrl()` plus `getPathInfo()`, so the prefix is accounted for on both sides. +`callback_path` is for a proxy that rewrites the path without announcing it, where +nothing in the request says so. If you run one authenticator per provider, override `getSupportedProviderKeys()` so each answers only its own callback; without it they behave exactly as in 5.x. diff --git a/docs/adr/003-constrain-supports-to-callback-path.md b/docs/adr/003-constrain-supports-to-callback-path.md index bc65a72..465313a 100644 --- a/docs/adr/003-constrain-supports-to-callback-path.md +++ b/docs/adr/003-constrain-supports-to-callback-path.md @@ -45,6 +45,14 @@ one of this authenticator's providers. - **`redirect_route` is generated as `ABSOLUTE_PATH`**, so host and scheme requirements on the route do not enter the comparison; a route whose path varies by host is not supported. +- **The request path is `getBaseUrl().getPathInfo()`, not path info alone.** + `Request::preparePathInfo()` strips `getBaseUrlReal()`, so path info excludes a + subdirectory deployment's base path and any trusted `X-Forwarded-Prefix` — while a + `redirect_uri` contains them, being the URL the identity provider was given, and + `UrlGenerator` prepends the routing context's base URL, which includes the trusted + prefix. Comparing path info alone rejected every callback in either deployment. + Derived paths are therefore memoized per routing-context base URL, so a service that + sees both proxied and direct traffic is not frozen to whichever arrived first. - **A provider must declare `redirect_uri`, `redirect_route` or `callback_path`.** Enforced when the container compiles. A provider with none has no callback path, and "matches every path" is the defect being removed. @@ -66,12 +74,6 @@ one of this authenticator's providers. the same way it always did, but changing it *only* at the provider now also stops callbacks being recognised. -## References - -- [ADR 002](002-fail-closed-on-authentication-failure.md) — the fail-closed decision - that made this worth fixing now -- Issue #63 - ## Returning to a page the firewall never saw Symfony saves the requested page when the entry point fires, which covers a user who @@ -88,3 +90,9 @@ The parameter ends up in a `Location` header, so it is validated as a path withi application and dropped otherwise: a single leading `/`, no backslashes, no scheme separator, no control characters. A rejected value is logged at `warning`; correcting one would be guessing at intent on a security boundary. + +## References + +- [ADR 002](002-fail-closed-on-authentication-failure.md) — the fail-closed decision + that made this worth fixing now +- Issue #63 diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 38e6704..6cac73f 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -87,18 +87,6 @@ public function login(Request $request, SessionInterface $session, string $provi return new RedirectResponse($authUrl); } - /** - * Report on the client secret's expiry without standing in the way. - * - * Deliberately non-fatal, even once expired. The status depends on a manually - * maintained date, which can fall out of step with the secret it describes: - * rotate a secret without updating `client_secret_expires_at` and the date - * reads "expired" while the secret works perfectly. So the date is treated as - * an indicator rather than as authority — the identity provider is what - * actually decides whether a secret still works. These records exist so that - * when it does stop working, the reason is already in the log rather than - * something to be worked out afterwards. - */ /** * Remember a return target named on the login link. * @@ -116,6 +104,10 @@ private function rememberNamedTargetPath(Request $request, SessionInterface $ses $target = $request->query->get(self::TARGET_PATH_PARAMETER); if (null === $target) { + // A target from an abandoned login link would otherwise sit in the session + // and be spent by whatever login came next. + $session->remove(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY); + return; } @@ -151,6 +143,18 @@ private static function isLocalPath(string $target): bool return 1 !== preg_match('/[\x00-\x1F\x7F]/', $target); } + /** + * Report on the client secret's expiry without standing in the way. + * + * Deliberately non-fatal, even once expired. The status depends on a manually + * maintained date, which can fall out of step with the secret it describes: + * rotate a secret without updating `client_secret_expires_at` and the date + * reads "expired" while the secret works perfectly. So the date is treated as + * an indicator rather than as authority — the identity provider is what + * actually decides whether a secret still works. These records exist so that + * when it does stop working, the reason is already in the log rather than + * something to be worked out afterwards. + */ private function checkClientSecretExpiry(string $providerKey): void { $expiry = $this->expiryChecker->getStatus($providerKey); diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 2eb3979..92f99d0 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -175,7 +175,7 @@ public function getConfigTreeBuilder(): TreeBuilder ->info('Redirect route parameters') ->end() ->scalarNode('callback_path') - ->info('Optional. The request path the callback arrives on, when a reverse proxy rewrites it so that the path of redirect_uri is not the path this application sees. Defaults to the path of redirect_uri, or of the generated redirect_route.') + ->info('Optional. The request path the callback arrives on, for a proxy that rewrites it without sending X-Forwarded-Prefix. Include any base path. Defaults to the path of redirect_uri, or of the generated redirect_route; a trusted X-Forwarded-Prefix or a subdirectory deployment is already accounted for without this.') // As on client_secret_expires_at: a validated node that also // disallows empty values refuses environment variables, and the // closure is the half worth keeping. diff --git a/src/Security/OpenIdConfigurationProviderManager.php b/src/Security/OpenIdConfigurationProviderManager.php index 7332908..d3941ba 100644 --- a/src/Security/OpenIdConfigurationProviderManager.php +++ b/src/Security/OpenIdConfigurationProviderManager.php @@ -5,6 +5,7 @@ use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; +use Psr\Cache\CacheItemPoolInterface; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; use Symfony\Component\Routing\RouterInterface; @@ -13,12 +14,12 @@ class OpenIdConfigurationProviderManager /** @var array */ private array $providers = []; - /** @var array|null */ - private ?array $redirectUriPaths = null; + /** @var array> */ + private array $redirectUriPaths = []; /** * @param array{ - * default_providers_options: array, + * default_providers_options: array{cacheItemPool?: CacheItemPoolInterface}, * providers: arrayredirectUriPaths) { - return $this->redirectUriPaths; + // Keyed by the routing context's base URL, not memoized flat: a generated + // route includes that base URL, and it differs between a proxied request + // carrying X-Forwarded-Prefix and a direct one. One frozen map could only + // ever match one of them. + $memoKey = $this->router->getContext()->getBaseUrl(); + + if (isset($this->redirectUriPaths[$memoKey])) { + return $this->redirectUriPaths[$memoKey]; } $paths = []; @@ -80,7 +87,31 @@ public function getRedirectUriPaths(): array } } - return $this->redirectUriPaths = $paths; + return $this->redirectUriPaths[$memoKey] = $paths; + } + + /** + * Whether a request path is the callback path of a given provider. + * + * Takes the path as `$request->getBaseUrl().$request->getPathInfo()`, which is + * what lines up with every derivation. `getPathInfo()` alone does not: + * `Request::preparePathInfo()` strips `getBaseUrlReal()`, so it excludes both a + * subdirectory deployment's base path and any trusted `X-Forwarded-Prefix` — + * while a `redirect_uri`'s path contains the prefix as the identity provider sees + * it, and `UrlGenerator` prepends the routing context's base URL, which includes + * the trusted prefix. + */ + public function isCallbackPath(string $requestPath, string $providerKey): bool + { + $paths = $this->getRedirectUriPaths(); + + if (!isset($paths[$providerKey])) { + return false; + } + + // Case-sensitive: paths are, and an identity provider sends the browser to + // the redirect URI exactly as it was registered. + return $paths[$providerKey] === $this->normalizePath($requestPath); } /** @@ -90,7 +121,11 @@ private function derivePath(array $options): ?string { // callback_path first: it exists precisely for deployments where the // external redirect_uri path is not the path this application receives. - if (isset($options['callback_path'])) { + // '' passes configuration on purpose — it is the fixture Symfony substitutes + // for a string environment variable while compiling — so an environment + // variable that resolves to nothing arrives here. Normalizing it would make + // the site root the callback path and shadow redirect_uri. + if (isset($options['callback_path']) && '' !== $options['callback_path']) { return $this->normalizePath($options['callback_path']); } @@ -169,7 +204,6 @@ public function getProvider(string $key): OpenIdConfigurationProvider $providerOptions += $options['http_client_options']; } - // @phpstan-ignore argument.type (library 5.0 narrowed $options to a strict array shape; the incremental build above is verified by the manager's tests but PHPStan can't track its evolution to the final shape) $this->providers[$key] = new OpenIdConfigurationProvider($providerOptions); } diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index 87b2da8..ee9cd81 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -95,15 +95,13 @@ public function supports(Request $request): ?bool return false; } - $path = rtrim($request->getPathInfo(), '/'); - $path = '' === $path ? '/' : $path; - - $paths = $this->providerManager->getRedirectUriPaths(); + // Base URL included: getPathInfo() has any subdirectory base path and trusted + // proxy prefix stripped out, while the configured paths contain them. See + // OpenIdConfigurationProviderManager::isCallbackPath(). + $path = $request->getBaseUrl().$request->getPathInfo(); foreach ($this->getSupportedProviderKeys() as $providerKey) { - // Case-sensitive: paths are, and an identity provider sends the browser - // to the redirect URI exactly as it was registered. - if (($paths[$providerKey] ?? null) === $path) { + if ($this->providerManager->isCallbackPath($path, $providerKey)) { return true; } } diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index ca8288d..10c6641 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -304,6 +304,26 @@ public function testWithoutATargetPathNothingIsRememberedOrLogged(): void $this->assertSame([], $this->logger->records); } + /** + * The last login link wins. A target left behind by an abandoned link would + * otherwise be spent by whatever login came next, sending the user somewhere they + * did not ask for this time. + */ + public function testAPlainLoginLinkForgetsAnEarlierTarget(): void + { + $provider = $this->createStub(OpenIdConfigurationProvider::class); + $provider->method('generateNonce')->willReturn('1234'); + $provider->method('generateState')->willReturn('abcd'); + $provider->method('getAuthorizationUrl')->willReturn('https://provider.example.org/authorize'); + + $session = new Session(new MockArraySessionStorage()); + $session->set(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY, '/admin/abandoned'); + + $this->createController($provider)->login(new Request(query: ['provider' => 'test']), $session, 'test'); + + $this->assertFalse($session->has(OpenIdLoginAuthenticator::TARGET_PATH_SESSION_KEY)); + } + /** * This value reaches a `Location` header after a successful login, so anything * that is not plainly a path inside this application would make the login route diff --git a/tests/Security/OpenIdConfigurationProviderManagerTest.php b/tests/Security/OpenIdConfigurationProviderManagerTest.php index 426f577..7660c83 100644 --- a/tests/Security/OpenIdConfigurationProviderManagerTest.php +++ b/tests/Security/OpenIdConfigurationProviderManagerTest.php @@ -9,8 +9,10 @@ use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; +use Psr\Cache\CacheItemPoolInterface; use Symfony\Component\Cache\Adapter\ArrayAdapter; use Symfony\Component\Routing\Generator\UrlGeneratorInterface; +use Symfony\Component\Routing\RequestContext; use Symfony\Component\Routing\RouterInterface; class OpenIdConfigurationProviderManagerTest extends TestCase @@ -36,12 +38,24 @@ private function getBaseProviderConfig(): array } /** - * Test helper: callers build provider arrays from {@see getBaseProviderConfig()} - * plus optional fields, so the parameter is intentionally typed loosely. The - * production manager constructor has the precise array shape. - * - * @param array> $providers - * @param array $defaultOptions + * @param array, + * callback_path?: string, + * leeway?: int, + * cache_duration?: int, + * allow_http?: bool, + * http_client_options?: array{ + * timeout?: float, + * proxy?: string, + * verify?: bool, + * }, + * }> $providers + * @param array{cacheItemPool?: CacheItemPoolInterface} $defaultOptions */ private function createManager(array $providers, array $defaultOptions = []): OpenIdConfigurationProviderManager { @@ -53,7 +67,6 @@ private function createManager(array $providers, array $defaultOptions = []): Op 'providers' => $providers, ]; - // @phpstan-ignore argument.type (test helper relaxes the strict provider shape declared by the production constructor — callers build configs ad-hoc from getBaseProviderConfig() plus optional fields) return new OpenIdConfigurationProviderManager($this->stubRouter, $config); } @@ -223,7 +236,7 @@ public function testGetProviderCachesInstance(): void } /** - * @return iterable, string}> + * @return iterable */ public static function pathDerivationProvider(): iterable { @@ -244,7 +257,7 @@ public static function pathDerivationProvider(): iterable } /** - * @param array $options + * @param array{redirect_uri?: string, callback_path?: string} $options */ #[DataProvider('pathDerivationProvider')] public function testRedirectUriPathsAreDerivedAndNormalized(array $options, string $expected): void @@ -304,4 +317,81 @@ public function testDerivingPathsDoesNotBuildProviders(): void $this->assertSame(['provider1' => '/callback_uri'], $manager->getRedirectUriPaths()); } + + /** + * @return iterable + */ + public static function requestPathProvider(): iterable + { + yield 'exactly' => ['/callback_uri', true]; + yield 'trailing slash' => ['/callback_uri/', true]; + yield 'another path' => ['/protected', false]; + yield 'below it' => ['/callback_uri/extra', false]; + yield 'differing in case' => ['/Callback_Uri', false]; + } + + #[DataProvider('requestPathProvider')] + public function testIsCallbackPathNormalizesWhatItIsGiven(string $requestPath, bool $expected): void + { + $manager = $this->createManager(['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback_uri', + ]]); + + $this->assertSame($expected, $manager->isCallbackPath($requestPath, 'provider1')); + } + + public function testAnUnknownProviderIsNotACallbackPath(): void + { + $manager = $this->createManager(['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback_uri', + ]]); + + $this->assertFalse($manager->isCallbackPath('/callback_uri', 'never-heard-of-it')); + } + + /** + * An environment variable that resolves to nothing must not turn the site root + * into the callback path: configuration lets '' through deliberately, because it + * is the fixture Symfony substitutes while compiling. + */ + public function testAnEmptyCallbackPathFallsThroughToRedirectUri(): void + { + $manager = $this->createManager(['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback_uri', + 'callback_path' => '', + ]]); + + $this->assertSame(['provider1' => '/callback_uri'], $manager->getRedirectUriPaths()); + } + + /** + * Generated routes carry the routing context's base URL, which differs between a + * request arriving through a proxy that sends X-Forwarded-Prefix and a direct one. + * Memoizing one map for both would leave one of them unable to match. + */ + public function testRoutePathsAreMemoizedPerBaseUrl(): void + { + $context = new RequestContext(); + // A stub: this asserts on the paths, not on how the router was called. + $router = $this->createStub(RouterInterface::class); + $router->method('getContext')->willReturn($context); + $router->method('generate')->willReturnCallback( + static fn (string $name, array $parameters, int $type): string => $context->getBaseUrl().'/generated/callback' + ); + + $config = ['default_providers_options' => [], 'providers' => ['provider1' => $this->getBaseProviderConfig() + [ + 'redirect_route' => 'my_route', + ]]]; + + $manager = new OpenIdConfigurationProviderManager($router, $config); + + $this->assertSame(['provider1' => '/generated/callback'], $manager->getRedirectUriPaths()); + + $context->setBaseUrl('/prefix'); + $this->assertSame(['provider1' => '/prefix/generated/callback'], $manager->getRedirectUriPaths()); + + // And back: still memoized per base URL rather than recomputed blindly. + $context->setBaseUrl(''); + $this->assertSame(['provider1' => '/generated/callback'], $manager->getRedirectUriPaths()); + } } diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 876a34d..20e05ba 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -21,6 +21,7 @@ use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\SessionInterface; use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; +use Symfony\Component\Routing\RouterInterface; use Symfony\Component\Security\Core\Exception\AuthenticationException; class OpenIdLoginAuthenticatorTest extends TestCase @@ -40,16 +41,37 @@ protected function setUp(): void $this->authenticator->setLogger($this->logger); } + /** + * A real manager, not a stub: the path comparison lives there, and stubbing it + * would mean reimplementing normalization in the test — where a bug in the real + * one could not be seen. + * + * @param array $paths callback_path per provider + */ + private function managerWithPaths(array $paths): OpenIdConfigurationProviderManager + { + $providers = []; + + foreach ($paths as $key => $path) { + $providers[$key] = [ + 'metadata_url' => 'https://provider.example.org/.well-known/openid-configuration', + 'client_id' => 'id', + 'client_secret' => 'secret', + 'callback_path' => $path, + ]; + } + + $config = ['default_providers_options' => [], 'providers' => $providers]; + + return new OpenIdConfigurationProviderManager($this->createStub(RouterInterface::class), $config); + } + /** * @param array $paths */ private function authenticatorWithPaths(array $paths): TestAuthenticator { - $manager = $this->createStub(OpenIdConfigurationProviderManager::class); - $manager->method('getRedirectUriPaths')->willReturn($paths); - $manager->method('getProviderKeys')->willReturn(array_keys($paths)); - - $authenticator = new TestAuthenticator($manager); + $authenticator = new TestAuthenticator($this->managerWithPaths($paths)); $authenticator->setLogger($this->logger); return $authenticator; @@ -88,6 +110,38 @@ public function testSupportsOnlyTheConfiguredCallbackPaths(string $path, bool $e $this->assertSame($expected, $authenticator->supports($request)); } + /** + * Deployments where the request path is not the whole story. + * + * `getPathInfo()` has both a subdirectory's base path and a trusted + * `X-Forwarded-Prefix` stripped out of it, while a configured `redirect_uri` + * contains them — it is the URL the identity provider was given. Comparing path + * info alone would reject every callback in either deployment. + * + * @return iterable + */ + public static function baseUrlProvider(): iterable + { + // configured path, base url, request path info, expected + yield 'subdirectory deployment' => ['/app/callback_uri', '/app', true]; + yield 'trusted proxy prefix' => ['/prefix/callback_uri', '/prefix', true]; + yield 'root deployment' => ['/callback_uri', '', true]; + // A proxy that rewrites without a prefix header: the internal path really is + // different, which is what callback_path exists to declare. + yield 'rewriting proxy, no header' => ['/prefix/callback_uri', '', false]; + } + + #[DataProvider('baseUrlProvider')] + public function testTheCallbackPathIncludesTheBaseUrl(string $configured, string $baseUrl, bool $expected): void + { + $authenticator = $this->authenticatorWithPaths(['test_provider_1' => $configured]); + + $request = new RequestWithBaseUrl($baseUrl, ['state' => 'abcd', 'code' => 'xyz']); + $request->server->set('REQUEST_URI', $baseUrl.'/callback_uri?state=abcd&code=xyz'); + + $this->assertSame($expected, $authenticator->supports($request)); + } + /** * @return iterable}> */ @@ -112,14 +166,10 @@ public function testTheRightPathAloneIsNotACallback(array $query): void */ public function testASubclassCanNarrowTheProvidersItAnswersFor(): void { - $manager = $this->createStub(OpenIdConfigurationProviderManager::class); - $manager->method('getRedirectUriPaths')->willReturn([ + $authenticator = new SingleProviderAuthenticator($this->managerWithPaths([ 'test_provider_1' => '/callback_uri', 'test_provider_2' => '/other_callback', - ]); - $manager->method('getProviderKeys')->willReturn(['test_provider_1', 'test_provider_2']); - - $authenticator = new SingleProviderAuthenticator($manager); + ])); $this->assertTrue($authenticator->supports(Request::create('/callback_uri?state=a&code=b'))); $this->assertFalse($authenticator->supports(Request::create('/other_callback?state=a&code=b'))); @@ -131,9 +181,15 @@ public function testASubclassCanNarrowTheProvidersItAnswersFor(): void */ public function testAProviderWithoutAPathMatchesNothing(): void { - $manager = $this->createStub(OpenIdConfigurationProviderManager::class); - $manager->method('getRedirectUriPaths')->willReturn([]); - $manager->method('getProviderKeys')->willReturn(['test_provider_1']); + // No redirect_uri, redirect_route or callback_path: nothing to match on, and + // matching everything is the defect this constraint removes. + $config = ['default_providers_options' => [], 'providers' => ['test_provider_1' => [ + 'metadata_url' => 'https://provider.example.org/.well-known/openid-configuration', + 'client_id' => 'id', + 'client_secret' => 'secret', + ]]]; + + $manager = new OpenIdConfigurationProviderManager($this->createStub(RouterInterface::class), $config); $authenticator = new TestAuthenticator($manager); diff --git a/tests/Security/RequestWithBaseUrl.php b/tests/Security/RequestWithBaseUrl.php new file mode 100644 index 0000000..e3ea19e --- /dev/null +++ b/tests/Security/RequestWithBaseUrl.php @@ -0,0 +1,37 @@ + $query + */ + public function __construct(private readonly string $overriddenBaseUrl, array $query = []) + { + parent::__construct($query); + } + + #[\Override] + public function getBaseUrl(): string + { + return $this->overriddenBaseUrl; + } + + #[\Override] + public function getPathInfo(): string + { + return '/callback_uri'; + } +} From 45c9f8a0d4b538f63a8a9567e10dd8ec6f473214 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 11:11:26 +0200 Subject: [PATCH 18/28] docs: list isCallbackPath and correct the callback_path changelog entry --- CHANGELOG.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9009f2..9db8eb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,8 +46,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- `callback_path` per provider, for deployments where a reverse proxy rewrites the - path so that the path of `redirect_uri` is not the one the application receives. +- `callback_path` per provider, for a reverse proxy that rewrites the path without + announcing it, so the path of `redirect_uri` is not the one the application + receives. A subdirectory deployment or a trusted `X-Forwarded-Prefix` needs none: + the request path is matched as `getBaseUrl()` plus `getPathInfo()`. - `OpenIdLoginAuthenticator::getSupportedProviderKeys()`, to narrow an authenticator to the providers whose callbacks it answers. Defaults to all of them, so existing multi-authenticator firewalls are unaffected. @@ -57,7 +59,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 firewall saved no requested page. Validated as a path within the application and otherwise dropped and logged, since the value reaches a `Location` header. A page the firewall denied takes precedence over it. -- `OpenIdConfigurationProviderManager::getRedirectUriPaths()`. +- `OpenIdConfigurationProviderManager::getRedirectUriPaths()` and + `isCallbackPath()`. ### Removed (BREAKING) From b8e4ee8fa67cfb6bcdd3887f310fc58bd86ca257 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 12:54:27 +0200 Subject: [PATCH 19/28] fix: annotate registerBundles() past Symfony 8.1's deprecated BundleInterface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PHPStan reports the test kernel's inherited return type on Symfony 8.1, which deprecates HttpKernel\Bundle\BundleInterface in favour of the one in DependencyInjection\Kernel. Naming either would break the other end of the supported range, so the docblock names the three bundle classes the fixture actually returns: covariant with both, and more precise than either. CI does not see this today — the phpstan job runs in the PHP 8.3 container, and symfony/http-kernel 8.1 requires PHP >= 8.4.1, so it resolves 7.4 there. Verified on 8.4 (error before, clean after), on 8.3, and on prefer-lowest, where PHPStan's 30 pre-existing errors are unchanged by this. --- tests/ItkDevOpenIdConnectBundleTestingKernel.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/ItkDevOpenIdConnectBundleTestingKernel.php b/tests/ItkDevOpenIdConnectBundleTestingKernel.php index 589b52f..38bb378 100644 --- a/tests/ItkDevOpenIdConnectBundleTestingKernel.php +++ b/tests/ItkDevOpenIdConnectBundleTestingKernel.php @@ -61,6 +61,15 @@ public function getCacheDir(): string * This bundle is registered before FrameworkBundle deliberately. It is the * unconventional order, and the one where autoconfigured method calls land in the * losing order — so it is the order that holds ConfiguredLoggerPass to its job. + * + * The return is annotated with the three bundle classes rather than inherited as + * `iterable`: Symfony 8.1 deprecates + * `HttpKernel\Bundle\BundleInterface` in favour of + * `DependencyInjection\Kernel\BundleInterface`, and naming either one would break + * on the other end of the supported range. The concrete classes are covariant with + * both, and more precise than either. + * + * @return list */ public function registerBundles(): iterable { From f10b33527dedee6e7e7cb6a8abe2364bd13484b2 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 14:06:05 +0200 Subject: [PATCH 20/28] fix: analyse against the declared PHP range, not the runtime version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PHPStan assumes the PHP version it runs on when phpVersion is unset, so the analysis only ever spoke for one point in the range composer.json declares (php ^8.3). Measured on PHP 8.5 with 8.4-only syntax (property hooks) in an analysed file: phpVersion min 80300 / max 80500 -> "Property hooks are supported only on PHP 8.4 and later" phpVersion unset -> No errors Dormant on the current job, which runs in the PHP 8.3 container: there the runtime assumption already equals the floor, and all three configurations behave identically. It matters the moment analysis moves to a newer PHP — which is the only way to reach Symfony 8.1 at all, since symfony/http-kernel 8.1 requires PHP >= 8.4.1 — and it is insurance against the container image being bumped without anyone noticing that the floor stopped being analysed. It does not widen the dependency axis, which is where both of this bundle's real static-analysis findings came from. That needs a second job, noted in the config. --- phpstan.neon | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/phpstan.neon b/phpstan.neon index dfebdc7..17a6df5 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -15,6 +15,19 @@ parameters: - phpstan/Rule reportIgnoresWithoutComments: true + # Analyse against the whole range composer.json declares (php ^8.3), not just the + # version this happens to run on. Without it PHPStan assumes the runtime PHP, so + # analysing on 8.3 says nothing about 8.5 and analysing on 8.5 would accept syntax + # that breaks 8.3 consumers. + # + # It does not cover the *dependency* axis, which is where this bundle's real + # findings have come from: Symfony 8.1 deprecations are invisible on PHP 8.3 + # because symfony/http-kernel 8.1 requires PHP >= 8.4.1, so composer cannot install + # it here. Widening that needs a second job, not a setting. + phpVersion: + min: 80300 + max: 80500 + ignoreErrors: # PHPUnit declares assertions as static methods on `Assert`, but the # pervasive idiom is `$this->assertX()`. phpstan-phpunit handles type From 8b418a0d0b232276574f71c7e015c9e17a200d5d Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 14:08:05 +0200 Subject: [PATCH 21/28] docs: changelog entry for the PHP range pin --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9db8eb2..2f7480b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Static analysis is pinned to the PHP range `composer.json` declares rather than + whichever version it happens to run on, so analysing on a newer PHP cannot silently + stop protecting the `^8.3` floor. - `supports()` no longer treats `?state=…&code=…` on an arbitrary path as a callback (#63). A forged callback is handled by the firewall — an entry point redirect for anonymous visitors — instead of surfacing as a 500 that any From 7a7a2682b0644f345083e98ede90d51fb467c915 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 14:15:11 +0200 Subject: [PATCH 22/28] ci: analyse on PHP 8.5 so Symfony 8 is analysed at all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit composer.json advertises symfony/* ^8.0, but the phpstan job ran in the default phpfpm service — PHP 8.3 — and symfony/http-kernel 8.1 requires PHP >= 8.4.1. Composer could not install the version the bundle claims to support, so it was never analysed. That is how the BundleInterface deprecation stayed invisible. Only the phpstan job moves, to phpfpm85. Bumping the default image instead would have been wrong: the unit-test matrix names `service: phpfpm` as its PHP 8.3 leg, so the matrix would have quietly lost 8.3 coverage. The floor is protected by the phpVersion range in this branch rather than by the runtime version: verified that 8.4-only syntax is flagged when analysing on 8.5 with the range and passes silently without it. Verified as CI will run it — no lock in the repository, so composer resolves fresh: PHP 8.5 with Symfony 8.1.5 gives no PHPStan errors and 273 passing tests. --- .github/workflows/php.yaml | 8 ++++++-- CHANGELOG.md | 8 +++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/.github/workflows/php.yaml b/.github/workflows/php.yaml index f1d2939..f576a07 100644 --- a/.github/workflows/php.yaml +++ b/.github/workflows/php.yaml @@ -26,6 +26,10 @@ jobs: docker compose run --rm phpfpm vendor/bin/php-cs-fixer fix --dry-run --diff phpstan: + # Analysed on the highest supported PHP, not the default 8.3 service: Symfony 8.1 + # requires PHP >= 8.4.1, so on 8.3 composer cannot install the Symfony 8 that + # composer.json advertises and the analysis never sees it. phpstan.neon pins the + # analysis to the declared php ^8.3 range, so the floor stays protected from here. name: PHPStan runs-on: ubuntu-latest steps: @@ -36,8 +40,8 @@ jobs: docker network create frontend - run: | - docker compose run --rm phpfpm composer install - docker compose run --rm phpfpm vendor/bin/phpstan + docker compose run --rm phpfpm85 composer install + docker compose run --rm phpfpm85 vendor/bin/phpstan unit-tests: name: Unit tests (${{ matrix.php }}, ${{ matrix.prefer }}) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f7480b..d42235b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,9 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Static analysis is pinned to the PHP range `composer.json` declares rather than - whichever version it happens to run on, so analysing on a newer PHP cannot silently - stop protecting the `^8.3` floor. +- Static analysis now covers the Symfony versions this bundle claims to support. It + runs on the highest supported PHP, since Symfony 8.1 requires PHP >= 8.4.1 and was + therefore uninstallable — and so unanalysed — in the PHP 8.3 container it ran in. + Analysis is pinned to the PHP range `composer.json` declares, so moving up cannot + silently stop protecting the `^8.3` floor. - `supports()` no longer treats `?state=…&code=…` on an arbitrary path as a callback (#63). A forged callback is handled by the firewall — an entry point redirect for anonymous visitors — instead of surfacing as a 500 that any From 252e5048b7da6da10654e35010f3331489343061 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 14:30:20 +0200 Subject: [PATCH 23/28] feat: keep client_secret_expires_at optional, and reorder the upgrade guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes that cannot sensibly ship apart: the guide is reordered around what a consumer actually meets first, and the expiry date it used to lead with stops being required. Merging them separately would put a requirement into develop that the next commit removes, and a guide that documents it in between. Optional again, because requiring it could force a value but never a correct one. Symfony compiles a container per environment, so a required node has to appear in all of them: putting it under when@prod compiles in prod and fails dev with "must be configured". What that produces is a date in a committed .env default, reporting ok forever while monitoring nothing — worse than the unknown it replaced, because it is invisible rather than absent. It also asked for ceremony from applications that never read it: only ClientSecretExpiryChecker consumes the value and only LoginController injects it. Unset now means the provider is not monitored and reports unknown, which monitoring can alert on, while a value that is set is still judged — a non-string is rejected at compile time and an unparseable one is reported at error. The guide had accumulated across four PRs and opened with the fail-closed change, burying the compile-time blockers behind optional material. Reordered, and corrected against three consumers: devops_itksites (5.1.1), economics (4.2.0) and display-api-service (5.1.1). They exposed a composer step that only applies to the 4.x hop, an undefined environment variable that breaks the login route for applications using the bundle's LoginController and silently removes monitoring for those that do not, two compile errors quoted inexactly, and a README that linked neither upgrade guide. --- CHANGELOG.md | 20 +- README.md | 23 +- UPGRADE-6.0.md | 207 +++++++++++------- src/DependencyInjection/Configuration.php | 8 +- .../ItkDevOpenIdConnectExtension.php | 8 +- .../DependencyInjection/ConfigurationTest.php | 15 +- .../ItkDevOpenIdConnectExtensionTest.php | 29 +++ tests/Util/ClientSecretExpiryCheckerTest.php | 4 + 8 files changed, 208 insertions(+), 106 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d42235b..fd1a1ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 therefore uninstallable — and so unanalysed — in the PHP 8.3 container it ran in. Analysis is pinned to the PHP range `composer.json` declares, so moving up cannot silently stop protecting the `^8.3` floor. +- `UPGRADE-6.0.md` is ordered by what a consumer hits first, leads with the one key + that is required, quotes the compile errors as they actually read, and says where to + put `client_secret_expires_at` and why a committed default is worse than leaving it + out. `README.md` links the upgrade guides, which nothing did. - `supports()` no longer treats `?state=…&code=…` on an arbitrary path as a callback (#63). A forged callback is handled by the firewall — an entry point redirect for anonymous visitors — instead of surfacing as a 500 that any @@ -43,11 +47,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Each provider must declare `redirect_uri`, `redirect_route` or `callback_path`. It is how a callback is recognised, so a provider without one could never complete a login. Enforced while the container compiles. -- `client_secret_expires_at` is now required for every provider, and must be a - string. A missing date fails while the container compiles instead of emitting the - 5.1 deprecation, and an unquoted `2027-01-31` — which YAML reads as a number — is - rejected rather than silently leaving the provider unmonitored. See - `UPGRADE-6.0.md`. + +### Changed + +- `client_secret_expires_at` stays optional, and the 5.1 deprecation for leaving it + unset is gone. A required key can force a value but never a correct one, and Symfony + compiles a container per environment, so requiring it meant a date in a committed + default — reporting `ok` while measuring nothing. Unset now means the provider is not + monitored and reports `unknown`, which monitoring can alert on. A value that *is* set + must be a string and parseable: an unquoted `2027-01-31`, which YAML reads as a + number, is rejected while the container compiles, and an unparseable one is reported + at `error`. ### Added diff --git a/README.md b/README.md index fc6dc37..2b0930c 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,9 @@ Symfony bundle for authorization via OpenID Connect. > If your application needs browser-based OIDC login, this bundle is still > required. +Upgrading from an earlier major? See [UPGRADE-6.0.md](UPGRADE-6.0.md) and +[UPGRADE-5.0.md](UPGRADE-5.0.md). + ## Installation To install run @@ -115,9 +118,10 @@ itkdev_openid_connect: metadata_url: '%env(string:ADMIN_OIDC_METADATA_URL)%' client_id: '%env(string:ADMIN_OIDC_CLIENT_ID)%' client_secret: '%env(string:ADMIN_OIDC_CLIENT_SECRET)%' - # Required. Date the client secret expires. An expired secret breaks - # every login, so the bundle warns while there is still time to rotate. - # See "Client secret expiry" below. + # Optional: date the client secret expires. Set it and the bundle warns + # before the secret expires; unset means the provider is not + # monitored and reports "unknown". Set it where the real secret + # lives. See "Client secret expiry" below. client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' # Specify redirect URI redirect_uri: '%env(string:ADMIN_OIDC_REDIRECT_URI)%' @@ -231,9 +235,13 @@ For a genuinely expired secret that means the login still fails, at the callback with `invalid_client` — but the `critical` record here and the failure record from the callback together name the cause without anyone having to reproduce it. -`client_secret_expires_at` is required, because the bundle cannot warn about an -expiry it does not know about. Quote it: YAML reads an unquoted `2027-01-31` as a -number, and a value that is not a string is rejected while the container compiles. +`client_secret_expires_at` is optional, and where you set it matters more than that +you set it. Put it with the real secret — the production secret store, or a `when@prod` +block. A date in a committed `.env` default is a date nobody maintains: it reports `ok` +while measuring nothing, which is worse than the `unknown` you get by leaving it out. + +Quote it: YAML reads an unquoted `2027-01-31` as a number, and a value that is not a +string is rejected while the container compiles. A provider still reaches `unknown` at runtime when the value resolves to something unusable — an environment variable that is set but blank, or a date @@ -981,6 +989,9 @@ We use [SemVer](http://semver.org/) for versioning. For the versions available, see the [tags on this repository](https://github.com/itk-dev/openid-connect/tags). +Upgrading across a major: [UPGRADE-6.0.md](UPGRADE-6.0.md), +[UPGRADE-5.0.md](UPGRADE-5.0.md). [CHANGELOG.md](CHANGELOG.md) has the rest. + ## License This project is licensed under the MIT License - see the diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index cb8a074..ff57b1d 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -1,34 +1,103 @@ # Upgrading from 5.x to 6.0 -A failed OpenID Connect callback now throws -`\ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException` instead of -Symfony's `AuthenticationException`. +Every provider must declare where its callback arrives, a failed callback becomes an +error instead of a redirect, and a callback is only recognised on that path. Most +applications already satisfy the first, in which case there is nothing to change unless +you catch `AuthenticationException` around the callback. + +```sh +composer update itk-dev/openid-connect-bundle +``` + +Coming from 4.x? Do [UPGRADE-5.0.md](UPGRADE-5.0.md) first — this guide assumes 5.x. On +that hop the library moves too and a partial update refuses (`the package is fixed to +4.1.2 (lock file version) by a partial update`), so name both packages. From 5.x the +bundle alone is enough: the only requirement that changes is `symfony/deprecation-contracts`, +which is dropped. + +## 1. Declare where each callback arrives + +Every provider must set one of `redirect_uri`, `redirect_route` or the new +`callback_path`, or the container will not compile: + +```text +Invalid configuration for path "itkdev_openid_connect.openid_providers.admin.options": +One of redirect_uri, redirect_route or callback_path must be set: it is how a callback +is recognised. +``` + +Most applications already have `redirect_uri` and need no change. + +A request is treated as a callback when it carries `state` and `code` **and** arrives +on that path. `?state=…&code=…` on any other URL is left to the firewall, exactly as +before 6.0 — an anonymous visitor goes to your entry point, a logged-in one gets the +page. Without the path check, and with failures now raising an exception (step 2), any URL +in the application was a 500 an anonymous caller could trigger. + +### When you need `callback_path` + +Only for a reverse proxy that rewrites the path **without announcing it** — an +external `https://app.example.org/prefix/auth/callback` that arrives here as +`/auth/callback`: + +```yaml +openid_providers: + admin: + options: + redirect_uri: 'https://app.example.org/prefix/auth/callback' + callback_path: '/auth/callback' +``` + +A subdirectory deployment, or a proxy sending `X-Forwarded-Prefix` with Symfony's +trusted proxies configured, needs none: the request path is matched as `getBaseUrl()` +plus `getPathInfo()`, so the prefix is accounted for on both sides. + +### One authenticator per provider + +Override `getSupportedProviderKeys()` so each answers only its own provider's +callback: + +```php +protected function getSupportedProviderKeys(): array +{ + return ['admin']; +} +``` + +Without the override every authenticator supports every callback path and the +session's provider key decides which provider validates it — exactly as in 5.x. -Before 6.0 the security component caught that exception and redirected to the -identity provider again, so a permanent failure such as an expired client secret -looped forever with no error page. The new exception escapes the firewall, so your -application renders it — a 500 by default. +## 2. A failed callback is now an error, not another redirect -See [ADR 002](docs/adr/002-fail-closed-on-authentication-failure.md). +`OpenIdLoginAuthenticator::onAuthenticationFailure()` throws +`\ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException` instead of +Symfony's `AuthenticationException`. The security component used to catch the latter +and call your entry point again, so a permanent failure such as an expired client +secret looped forever with no error page. The new exception escapes the firewall and +your application renders it — a 500 by default. See +[ADR 002](docs/adr/002-fail-closed-on-authentication-failure.md). -## Migrate catch blocks +**Most applications need no code change.** Catching library exceptions *inside* +`authenticate()` — the pattern in this README — is unaffected. What needs attention is +a `catch` of Symfony's `AuthenticationException` around the callback, or an overridden +`onAuthenticationFailure()`: ```diff - } catch (\Symfony\Component\Security\Core\Exception\AuthenticationException $e) { + } catch (\ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface $e) { ``` -`getPrevious()` on the new exception is the underlying OpenID Connect exception, -not Symfony's `AuthenticationException`: the security listener follows the chain, -so one left there would loop again. +`getPrevious()` is the underlying OpenID Connect exception rather than the +`AuthenticationException`, because the security listener walks the chain and one left +there would loop again. -If you catch nothing today, no code change is needed. Check that a failed login -renders an acceptable error and that your error reporting picks it up. +Then check that a failed login renders something acceptable and that your error +reporting picks it up. -## Rendering something friendlier than a 500 +### Rendering something friendlier than a 500 -Listen for the exception. Do not answer with a redirect to the login route — that -reintroduces the loop. +Listen for the exception, and do not answer with a redirect to the login route — that +reintroduces the loop: ```php #[AsEventListener] @@ -47,38 +116,11 @@ final class LoginFailureListener } ``` -Render your own template, as above, rather than `getMessage()`. The message carries -the identity provider's error text, which the security component used to reduce to -a safe message key before anything could display it. - -## `client_secret_expires_at` is now required - -Every provider must declare when its client secret expires. Without it the bundle -cannot warn before an expiry takes every login down, which is what happened. - -```yaml -itkdev_openid_connect: - openid_providers: - admin: - options: - client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' -``` - -Anything `strtotime()` understands, but **quote it** — YAML reads an unquoted -`2027-01-31` as the number `1801353600`, and a non-string is rejected. A missing key -fails at compile time too: - -```text -The child config "client_secret_expires_at" under -"itkdev_openid_connect.openid_providers.admin.options" must be configured -``` - -The value is not trusted as fact — nothing here blocks a login, and a value that -cannot be parsed is reported at `error` and treated as `unknown`. Keep it beside the -secret itself, so rotating one prompts updating the other. The 5.1 deprecation -warning for a missing date is gone with it. +Render your own template rather than `getMessage()`: the message carries the identity +provider's error text, which the security component used to reduce to a safe message +key before anything could display it. -## Removed exceptions +## 3. Removed exceptions | Removed | Use instead | | --- | --- | @@ -90,43 +132,42 @@ switch to `UserNotFoundException`: `UserLoginCommand` catches that and reports t username as unknown. Not to be confused with `UsernameDoesNotExistException`, which stays — the CLI authenticator throws it when a token resolves to no username. -## Callbacks are only accepted on the configured callback path +`symfony/deprecation-contracts` is no longer required by the bundle. Require it +yourself if your own code calls `trigger_deprecation()`. -A request counts as a callback when it carries `state` and `code` **and** arrives on a -provider's callback path. `?state=…&code=…` on any other URL is left to the firewall, -as it was before 6.0 — which is the point: since a failed callback now escapes as an -exception, any URL was otherwise a 500 an anonymous caller could trigger. +## Recommended: monitor client secret expiry -Every provider must declare one of `redirect_uri`, `redirect_route` or the new -`callback_path`, or the container will not compile: +`client_secret_expires_at` stays optional. Set it and the bundle warns before a secret +expires; leave it unset and the provider reports `unknown` and is not monitored. -```text -One of redirect_uri, redirect_route or callback_path must be set: it is how a -callback is recognised. +```yaml +itkdev_openid_connect: + openid_providers: + admin: + options: + client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' ``` -Set `callback_path` if a reverse proxy rewrites the path, so an external -`https://app.example.org/prefix/auth/callback` arrives here as `/auth/callback`: +**Set it where the real secret lives** — the production secret store, or a `when@prod` +block. A date carried in a committed `.env` default is a date nobody maintains: it +reports `ok` while measuring nothing, which is worse than reporting `unknown`. That is +also why the key is not required: a required key can force a value, never a correct +one, and Symfony compiles a container per environment, so it would have to be set in +all of them. -```yaml -openid_providers: - admin: - options: - redirect_uri: 'https://app.example.org/prefix/auth/callback' - callback_path: '/auth/callback' -``` +Anything `strtotime()` understands, and **quote it**: YAML reads an unquoted +`2027-01-31` as the number `1801353600`, and a non-string is rejected while the +container compiles. A value that is set but cannot be parsed is reported at `error` and +treated as `unknown` — set-but-broken is a mistake, unset is a decision. -A subdirectory deployment, or a proxy sending `X-Forwarded-Prefix` with trusted -proxies configured, needs no `callback_path`: the path is matched against -`getBaseUrl()` plus `getPathInfo()`, so the prefix is accounted for on both sides. -`callback_path` is for a proxy that rewrites the path without announcing it, where -nothing in the request says so. If you run one authenticator per provider, override -`getSupportedProviderKeys()` so each answers only its own callback; without it they -behave exactly as in 5.x. +Nothing here blocks a login. An unmonitored provider shows up as `unknown` in +`ClientSecretExpiryChecker::getAllStatuses()`, which is where monitoring should alert +on it. The 5.1 deprecation for leaving the date unset is gone. -## Redirecting back to the originally requested page +## Optional: return users to the page they asked for -`createTargetPathRedirect()` returns the user to the page that sent them to log in: +`createTargetPathRedirect()` sends the user back to whatever sent them to log in, +falling back to a URL of your choosing: ```php public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response @@ -135,18 +176,18 @@ public function onAuthenticationSuccess(Request $request, TokenInterface $token, } ``` -Optional — existing `onAuthenticationSuccess()` implementations keep working. +Existing `onAuthenticationSuccess()` implementations keep working unchanged. Only pages that exist and are access-controlled come back this way: routing runs -before security, so a link to a URL with no route is a 404 before the firewall sees -it and there is nothing to return to. +before security, so a link to a URL with no route is a 404 before the firewall sees it +and there is nothing to return to. A login link on a public page can name its own destination with `?target_path=/admin/reports`. The value must be a path within the application, or it is dropped and logged. -## CLI login is unchanged +## Unchanged: CLI login -`CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed -or invalid login token still sends the user to your login page. That path has no -entry point of its own and cannot loop. +`CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed or +invalid login token still sends the user to your login page. That path has no entry +point of its own and cannot loop. diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 92f99d0..bd01dc1 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -111,7 +111,6 @@ public function getConfigTreeBuilder(): TreeBuilder ->isRequired()->cannotBeEmpty() ->end() ->scalarNode('client_secret_expires_at') - ->isRequired() // No cannotBeEmpty() here, and it cannot come back: // VariableNode::finalizeValue() refuses an environment variable // whenever empty values are disallowed and the node has any @@ -123,13 +122,14 @@ public function getConfigTreeBuilder(): TreeBuilder // ClientSecretExpiryChecker. It never caught whitespace-only // values regardless: ScalarNode::isValueEmpty() is // `null === $value || '' === $value`. - ->info('Required. Date the client secret expires, e.g. "2027-01-31". Anything strtotime() understands, and usually an environment variable. An expired secret breaks every login, so the bundle warns while there is still time to rotate.') + ->info('Optional. Date the client secret expires, e.g. "2027-01-31". Anything strtotime() understands, and usually an environment variable. Set it and the bundle warns before the secret expires; leave it unset and the provider reports "unknown" and is not monitored. Set it where the real secret lives — a date carried in a committed default is a date nobody maintains.') ->validate() // YAML reads an unquoted 2027-01-31 as the integer 1801353600, and the // closure below only inspects strings, so without this the most natural // way to write the value would pass, be discarded as untyped, and leave - // the provider unmonitored with nothing logged. Also catches an explicit - // null, which isRequired() accepts because the key is present. + // the provider unmonitored with nothing logged. Leaving the key out + // entirely is a decision and reports "unknown"; writing a value that + // cannot be one is a mistake, including an explicit null. ->ifTrue(static fn (mixed $v): bool => !is_string($v)) ->thenInvalid('client_secret_expires_at must be a string. YAML reads an unquoted date as a number, so quote it: "2027-01-31". From an environment variable, cast it as %%env(string:NAME)%%. Got %s.') ->end() diff --git a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php index 5140f1f..1450545 100644 --- a/src/DependencyInjection/ItkDevOpenIdConnectExtension.php +++ b/src/DependencyInjection/ItkDevOpenIdConnectExtension.php @@ -156,10 +156,10 @@ private function configureSecretExpiry(ContainerBuilder $container, array $provi foreach ($providers as $providerKey => $provider) { $expiresAt = $provider['options']['client_secret_expires_at'] ?? null; - // Configuration requires the key and rejects a non-string, so the - // fallback is unreachable and kept only because the shape here is mixed. - // It must not become the quiet path it used to be: a null reaches - // ClientSecretExpiryChecker as Unknown with nothing logged. + // The key is optional, so null is the ordinary "not monitored" case: + // ClientSecretExpiryChecker reports it as Unknown without logging, since + // an unset date is a choice rather than a fault. A value that is set but + // unusable is a different matter, and is reported at error. $expiryDates[$providerKey] = is_string($expiresAt) ? $expiresAt : null; } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index ae482eb..031d49b 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -414,15 +414,22 @@ public function testANonStringExpiryDateIsRejected(mixed $configured): void $this->processor->processConfiguration($this->configuration, [$input]); } - public function testTheExpiryDateIsRequired(): void + /** + * Optional on purpose. A required key can force a value, never a correct one, and + * it cannot be scoped to the environment where the real secret lives — Symfony + * compiles a container per environment, so a required node has to appear in all of + * them. What that produces is a date in a committed default, which reports `ok` + * forever while monitoring nothing. Unset is the honest state, and it is visible: + * the provider reports `unknown`. + */ + public function testTheExpiryDateIsOptional(): void { $input = $this->getMinimalConfig(); unset($input['openid_providers']['provider1']['options']['client_secret_expires_at']); - $this->expectException(InvalidConfigurationException::class); - $this->expectExceptionMessage('The child config "client_secret_expires_at" under "itkdev_openid_connect.openid_providers.provider1.options" must be configured'); + $config = $this->processor->processConfiguration($this->configuration, [$input]); - $this->processor->processConfiguration($this->configuration, [$input]); + $this->assertArrayNotHasKey('client_secret_expires_at', $config['openid_providers']['provider1']['options']); } public function testMultipleProviders(): void diff --git a/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php b/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php index 34f1e35..30b40c3 100644 --- a/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php +++ b/tests/DependencyInjection/ItkDevOpenIdConnectExtensionTest.php @@ -185,6 +185,35 @@ public function testAuditOptionsAreWired(): void $this->assertSame('%kernel.secret%', $arguments['$identifierSecret']); } + public function testAProviderWithoutAnExpiryDateIsWiredAsUnmonitored(): void + { + $extension = new ItkDevOpenIdConnectExtension(); + $container = new ContainerBuilder(); + + $config = $this->getBaseConfig(); + // Built without the date rather than unset from the base config, which is + // untyped and would need narrowing for no gain. + $config['openid_providers'] = [ + 'test_provider' => [ + 'options' => [ + 'metadata_url' => 'https://example.com/.well-known/openid-configuration', + 'client_id' => 'test_id', + 'client_secret' => 'test_secret', + 'redirect_uri' => 'https://app.example.org/callback_uri', + ], + ], + ]; + + $extension->load([$config], $container); + + // null, not absent: the checker reports every configured provider, and one + // without a date has to be reportable as `unknown` rather than missing. + $this->assertSame( + ['test_provider' => null], + $container->getDefinition(ClientSecretExpiryChecker::class)->getArgument('$expiryDates') + ); + } + public function testSecretExpiryIsWired(): void { $extension = new ItkDevOpenIdConnectExtension(); diff --git a/tests/Util/ClientSecretExpiryCheckerTest.php b/tests/Util/ClientSecretExpiryCheckerTest.php index 3b07576..15a5a2f 100644 --- a/tests/Util/ClientSecretExpiryCheckerTest.php +++ b/tests/Util/ClientSecretExpiryCheckerTest.php @@ -45,6 +45,10 @@ public function testUnknownWhenNoDateConfigured(): void $this->assertNull($status->daysRemaining); $this->assertFalse($status->isExpired()); $this->assertFalse($status->isExpiringSoon()); + // And nothing logged: the option is optional, so an unset date is a decision + // rather than a fault. A value that is set but unusable is the opposite, and + // is reported at error. The gap is visible as this status, not as a record. + $this->assertSame([], $this->logger->records); } public function testUnknownForAProviderWithNoEntryAtAll(): void From b532bfa666c15338c009b1abac61f3390c39f56a Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 14:40:17 +0200 Subject: [PATCH 24/28] ci: analyse the dependency floor, and fix what it found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PHPStan only ever ran against current dependencies, so the floor this bundle declares was never analysed. It had a real defect: Symfony 6.4 declares Bundle::$extension untyped, where 7.0 typed it, so getContainerExtension() returned mixed against its own ?ExtensionInterface signature. Returning a local narrows it without an annotation or an ignore, and behaviour is unchanged — the property holds null, or false for a bundle with no extension, and this bundle always has one. The job lowers only the packages `require` names, derived from composer itself rather than duplicated in the workflow, so adding a requirement cannot leave the job behind. A plain --prefer-lowest also downgrades PHPUnit and phpstan-phpunit, which produced 29 artefacts for every 1 real finding — an analysis nobody would keep green. It runs on PHP 8.3, the declared floor, as the counterpart to the phpstan job's 8.5 ceiling. Verified both ends: Symfony 6.4.13 and 8.1.5 each report no errors with 274 tests passing. --- .github/workflows/php.yaml | 29 +++++++++++++++++++++++++++++ CHANGELOG.md | 7 ++++++- src/ItkDevOpenIdConnectBundle.php | 16 +++++++++++++--- 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/.github/workflows/php.yaml b/.github/workflows/php.yaml index f576a07..d04c712 100644 --- a/.github/workflows/php.yaml +++ b/.github/workflows/php.yaml @@ -43,6 +43,35 @@ jobs: docker compose run --rm phpfpm85 composer install docker compose run --rm phpfpm85 vendor/bin/phpstan + phpstan-lowest: + # The declared dependency floor, analysed with current dev tooling. A plain + # --prefer-lowest also downgrades PHPUnit and phpstan-phpunit, and then nearly + # every error reported is an artefact of that rather than a statement about + # Symfony 6.4 — 29 of 30, when this was measured. Lowering only what `require` + # names keeps the analysis about the runtime the bundle claims to support. + # + # Runs on the PHP floor too, in the default 8.3 service, as the counterpart to the + # phpstan job's ceiling on 8.5. + # + # Analysis only: lowering the runtime packages drags shared Symfony components + # (finder, console) down with them, and php-cs-fixer built on those emits nonsense + # such as `previous : $e`. Never add a formatter to this job, and do not reuse the + # resulting vendor/ for anything but PHPStan. + name: PHPStan (lowest runtime dependencies) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + + - name: Create docker network + run: | + docker network create frontend + + - run: | + docker compose run --rm phpfpm composer install + docker compose run --rm phpfpm sh -c \ + 'composer update --prefer-lowest --with-all-dependencies $(composer show --direct --no-dev --name-only)' + docker compose run --rm phpfpm vendor/bin/phpstan + unit-tests: name: Unit tests (${{ matrix.php }}, ${{ matrix.prefer }}) runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index fd1a1ba..3f40eed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,11 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `getContainerExtension()` no longer returns `mixed` on Symfony 6.4, which declares + `Bundle::$extension` untyped where 7.0 typed it. Found by analysing the dependency + floor, which nothing did before. - Static analysis now covers the Symfony versions this bundle claims to support. It runs on the highest supported PHP, since Symfony 8.1 requires PHP >= 8.4.1 and was therefore uninstallable — and so unanalysed — in the PHP 8.3 container it ran in. Analysis is pinned to the PHP range `composer.json` declares, so moving up cannot - silently stop protecting the `^8.3` floor. + silently stop protecting the `^8.3` floor. A second job analyses the dependency + floor, lowering only what `require` names so the result is about Symfony 6.4 rather + than about a downgraded PHPUnit. - `UPGRADE-6.0.md` is ordered by what a consumer hits first, leads with the one key that is required, quotes the compile errors as they actually read, and says where to put `client_secret_expires_at` and why a committed default is worse than leaving it diff --git a/src/ItkDevOpenIdConnectBundle.php b/src/ItkDevOpenIdConnectBundle.php index 206861f..4f12346 100644 --- a/src/ItkDevOpenIdConnectBundle.php +++ b/src/ItkDevOpenIdConnectBundle.php @@ -18,15 +18,25 @@ class ItkDevOpenIdConnectBundle extends Bundle * {@inheritdoc} * * Overridden to allow for the custom extension alias. + * + * Returns a local rather than the property: Symfony 6.4 declares + * `Bundle::$extension` untyped, so on the supported floor its value is `mixed` and + * returning it directly does not satisfy this signature. 7.0 typed the property, + * which is why analysing only against current dependencies never saw it. */ #[\Override] public function getContainerExtension(): ?ExtensionInterface { - if (null === $this->extension || false === $this->extension) { - $this->extension = new ItkDevOpenIdConnectExtension(); + if ($this->extension instanceof ExtensionInterface) { + return $this->extension; } - return $this->extension; + // Reached when the property is null, or false as Symfony sets it for a bundle + // with no extension — this bundle always has one. + $extension = new ItkDevOpenIdConnectExtension(); + $this->extension = $extension; + + return $extension; } #[\Override] From 543a43e4a0799c428b753adf1928ca94c6b43b41 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 14:49:54 +0200 Subject: [PATCH 25/28] ci: update actions/checkout to v7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Twelve references across six workflows. codecov/codecov-action is already on v7, the current major, and no other action is used — the jobs drive docker compose directly. v7.0.0's one behavioural change is that it refuses to check out a fork's PR head under pull_request_target and workflow_run. Neither trigger appears in this repository, which uses pull_request, push and tag push, so the change does not reach us. The only input passed anywhere is fetch-depth in the changelog workflow, unchanged in v7. --- .github/workflows/changelog.yaml | 2 +- .github/workflows/composer.yaml | 6 +++--- .github/workflows/github_build_release.yml | 2 +- .github/workflows/markdown.yaml | 2 +- .github/workflows/php.yaml | 10 +++++----- .github/workflows/yaml.yaml | 2 +- CHANGELOG.md | 2 ++ 7 files changed, 14 insertions(+), 12 deletions(-) diff --git a/.github/workflows/changelog.yaml b/.github/workflows/changelog.yaml index 63638c2..71da908 100644 --- a/.github/workflows/changelog.yaml +++ b/.github/workflows/changelog.yaml @@ -18,7 +18,7 @@ jobs: fail-fast: false steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@v7 with: fetch-depth: 2 diff --git a/.github/workflows/composer.yaml b/.github/workflows/composer.yaml index 9752a66..8c27ac6 100644 --- a/.github/workflows/composer.yaml +++ b/.github/workflows/composer.yaml @@ -44,7 +44,7 @@ jobs: matrix: prefer: [prefer-lowest, prefer-stable] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -58,7 +58,7 @@ jobs: strategy: fail-fast: false steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -73,7 +73,7 @@ jobs: strategy: fail-fast: false steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | diff --git a/.github/workflows/github_build_release.yml b/.github/workflows/github_build_release.yml index b2c083f..abb8cd8 100644 --- a/.github/workflows/github_build_release.yml +++ b/.github/workflows/github_build_release.yml @@ -16,7 +16,7 @@ jobs: APP_ENV: prod steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Create a release in GitHub run: gh release create ${{ github.ref_name }} --verify-tag --generate-notes diff --git a/.github/workflows/markdown.yaml b/.github/workflows/markdown.yaml index 8f0fc25..b0f74c5 100644 --- a/.github/workflows/markdown.yaml +++ b/.github/workflows/markdown.yaml @@ -34,7 +34,7 @@ jobs: fail-fast: false steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@v7 - name: Create docker network run: | diff --git a/.github/workflows/php.yaml b/.github/workflows/php.yaml index d04c712..8807a29 100644 --- a/.github/workflows/php.yaml +++ b/.github/workflows/php.yaml @@ -15,7 +15,7 @@ jobs: name: PHP - Check Coding Standards runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -33,7 +33,7 @@ jobs: name: PHPStan runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -60,7 +60,7 @@ jobs: name: PHPStan (lowest runtime dependencies) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -98,7 +98,7 @@ jobs: php: "8.5" prefer: prefer-stable steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | @@ -129,7 +129,7 @@ jobs: php: "8.3" prefer: prefer-stable steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | diff --git a/.github/workflows/yaml.yaml b/.github/workflows/yaml.yaml index 299d4e1..80d0a23 100644 --- a/.github/workflows/yaml.yaml +++ b/.github/workflows/yaml.yaml @@ -31,7 +31,7 @@ jobs: yaml-lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - name: Create docker network run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f40eed..e59069f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `actions/checkout` updated from v6 to v7 across every workflow. + `codecov/codecov-action` was already current. - `getContainerExtension()` no longer returns `mixed` on Symfony 6.4, which declares `Bundle::$extension` untyped where 7.0 typed it. Found by analysing the dependency floor, which nothing did before. From 1f4a6ddc6e3b1db131ec682016544db74bc227a0 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 14:57:33 +0200 Subject: [PATCH 26/28] docs: open the upgrade guide with require, not update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first command could not do what the guide asked of it. Consumers pin a major — ^4.0, ^5.0 and ^5.1.1 across the three checked — and `composer update itk-dev/openid-connect-bundle` will not cross a major, so it reports nothing to do and leaves the reader on 5.x wondering why none of the guide applies. Nothing in the document mentioned ^6.0 at all. Found by walking the guide again after the restructure, this time following it literally rather than installing the bundle from a path repository, which had hidden the step. The 4.x hop gets the same treatment: `composer require` for both packages rather than `composer update`, which was where the partial-update refusal came from in the first place. --- CHANGELOG.md | 3 +++ UPGRADE-6.0.md | 19 ++++++++++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e59069f..2e5e110 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `UPGRADE-6.0.md` opens with `composer require …:^6.0` rather than + `composer update`, which cannot cross the major consumers pin and so reported + nothing to do. - `actions/checkout` updated from v6 to v7 across every workflow. `codecov/codecov-action` was already current. - `getContainerExtension()` no longer returns `mixed` on Symfony 6.4, which declares diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index ff57b1d..0fa75e1 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -6,14 +6,23 @@ applications already satisfy the first, in which case there is nothing to change you catch `AuthenticationException` around the callback. ```sh -composer update itk-dev/openid-connect-bundle +composer require itk-dev/openid-connect-bundle:^6.0 ``` +`require`, not `update`: your `composer.json` pins a major — `^5.0` or similar — and +`composer update` will not cross it, so it would report nothing to do and leave you on +5.x wondering why none of this applies. + Coming from 4.x? Do [UPGRADE-5.0.md](UPGRADE-5.0.md) first — this guide assumes 5.x. On -that hop the library moves too and a partial update refuses (`the package is fixed to -4.1.2 (lock file version) by a partial update`), so name both packages. From 5.x the -bundle alone is enough: the only requirement that changes is `symfony/deprecation-contracts`, -which is dropped. +that hop the library moves with the bundle, and naming the bundle alone refuses with +`the package is fixed to 4.1.2 (lock file version) by a partial update`, so name both: + +```sh +composer require itk-dev/openid-connect-bundle:^5.0 itk-dev/openid-connect:^5.0 +``` + +From 5.x to 6.0 the bundle alone is enough: the only requirement that changes is +`symfony/deprecation-contracts`, which is dropped. ## 1. Declare where each callback arrives From 3060ce06e78743a29319c8bea863bf16bd512896 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 19:56:17 +0200 Subject: [PATCH 27/28] docs: prepare 6.0.0 Collapse the unreleased entries into a 6.0.0 section stating what changed, and add the compare link. Rationale, measurements and history stay in the commits and PRs where they belong. --- CHANGELOG.md | 111 ++++++++++++++++----------------------------------- 1 file changed, 34 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e5e110..a7e8c3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,95 +7,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Fixed +## [6.0.0] - 2026-08-25 -- `UPGRADE-6.0.md` opens with `composer require …:^6.0` rather than - `composer update`, which cannot cross the major consumers pin and so reported - nothing to do. -- `actions/checkout` updated from v6 to v7 across every workflow. - `codecov/codecov-action` was already current. -- `getContainerExtension()` no longer returns `mixed` on Symfony 6.4, which declares - `Bundle::$extension` untyped where 7.0 typed it. Found by analysing the dependency - floor, which nothing did before. -- Static analysis now covers the Symfony versions this bundle claims to support. It - runs on the highest supported PHP, since Symfony 8.1 requires PHP >= 8.4.1 and was - therefore uninstallable — and so unanalysed — in the PHP 8.3 container it ran in. - Analysis is pinned to the PHP range `composer.json` declares, so moving up cannot - silently stop protecting the `^8.3` floor. A second job analyses the dependency - floor, lowering only what `require` names so the result is about Symfony 6.4 rather - than about a downgraded PHPUnit. -- `UPGRADE-6.0.md` is ordered by what a consumer hits first, leads with the one key - that is required, quotes the compile errors as they actually read, and says where to - put `client_secret_expires_at` and why a committed default is worse than leaving it - out. `README.md` links the upgrade guides, which nothing did. -- `supports()` no longer treats `?state=…&code=…` on an arbitrary path as a - callback (#63). A forged callback is handled by the firewall — an entry point - redirect for anonymous visitors — instead of surfacing as a 500 that any - unauthenticated caller could raise on any URL. -- `logging_options.logger` no longer depends on bundle registration order. - FrameworkBundle autoconfigures a `setLogger()` call onto every - `LoggerAwareInterface` service and the last call wins, so an application - registering this bundle before FrameworkBundle received the application logger - instead of the configured one — `itkdev_openid_connect.null_logger` included. - The conventional order, FrameworkBundle first, was unaffected. +See [UPGRADE-6.0.md](UPGRADE-6.0.md). ### Changed (BREAKING) -- A failed OpenID Connect callback now throws - `AuthenticationFailedException` instead of Symfony's - `AuthenticationException`. The security component caught the latter and - answered by redirecting to the identity provider again, so a permanent - failure such as an expired client secret produced an unbreakable redirect - loop. The exception now escapes the firewall and the application renders its - own error. See `UPGRADE-6.0.md` and - `docs/adr/002-fail-closed-on-authentication-failure.md`. - `CliLoginTokenAuthenticator` is unchanged: it has no entry point of its own, - so it cannot loop. -- `getPrevious()` on that exception is the underlying OpenID Connect exception - rather than the `AuthenticationException`, which the security component would - have followed straight back into the loop. +- A failed OpenID Connect callback throws `AuthenticationFailedException` instead of + Symfony's `AuthenticationException`, so it escapes the firewall rather than + redirecting to the identity provider again. `CliLoginTokenAuthenticator` is + unchanged. +- `getPrevious()` on that exception is the underlying OpenID Connect exception. +- A callback is recognised only on the provider's callback path, not on any URL + carrying `state` and `code` (#63). - Each provider must declare `redirect_uri`, `redirect_route` or `callback_path`. - It is how a callback is recognised, so a provider without one could never - complete a login. Enforced while the container compiles. + +### Added + +- `callback_path` per provider, for a proxy that rewrites the path without announcing + it. +- `OpenIdLoginAuthenticator::getSupportedProviderKeys()`, to narrow an authenticator to + named providers. Defaults to all of them. +- `OpenIdLoginAuthenticator::createTargetPathRedirect()`, returning the user to the + page that sent them to log in. +- `?target_path=` on the login route, validated as a path within the application. +- `OpenIdConfigurationProviderManager::getRedirectUriPaths()` and `isCallbackPath()`. ### Changed -- `client_secret_expires_at` stays optional, and the 5.1 deprecation for leaving it - unset is gone. A required key can force a value but never a correct one, and Symfony - compiles a container per environment, so requiring it meant a date in a committed - default — reporting `ok` while measuring nothing. Unset now means the provider is not - monitored and reports `unknown`, which monitoring can alert on. A value that *is* set - must be a string and parseable: an unquoted `2027-01-31`, which YAML reads as a - number, is rejected while the container compiles, and an unparseable one is reported - at `error`. +- `client_secret_expires_at` remains optional, and the 5.1 deprecation for leaving it + unset is gone. Unset reports `unknown`; a value that is set must be a string and + parseable. +- `UPGRADE-6.0.md` rewritten and linked from `README.md`. +- Static analysis runs against both ends of the supported dependency range. +- `actions/checkout` updated to v7. -### Added +### Fixed -- `callback_path` per provider, for a reverse proxy that rewrites the path without - announcing it, so the path of `redirect_uri` is not the one the application - receives. A subdirectory deployment or a trusted `X-Forwarded-Prefix` needs none: - the request path is matched as `getBaseUrl()` plus `getPathInfo()`. -- `OpenIdLoginAuthenticator::getSupportedProviderKeys()`, to narrow an - authenticator to the providers whose callbacks it answers. Defaults to all of - them, so existing multi-authenticator firewalls are unaffected. -- `OpenIdLoginAuthenticator::createTargetPathRedirect()`, for returning the user to - the page that sent them to log in. -- `?target_path=` on the login route, for a login link on a public page where the - firewall saved no requested page. Validated as a path within the application and - otherwise dropped and logged, since the value reaches a `Location` header. A page - the firewall denied takes precedence over it. -- `OpenIdConfigurationProviderManager::getRedirectUriPaths()` and - `isCallbackPath()`. +- `logging_options.logger` no longer depends on bundle registration order. +- `getContainerExtension()` no longer returns `mixed` on Symfony 6.4. ### Removed (BREAKING) -- `ItkOpenIdConnectBundleException`, `@deprecated` since 5.0. Catch - `OpenIdConnectBundleExceptionInterface` instead. -- `UserDoesNotExistException`, which was thrown nowhere. Symfony's - `UserNotFoundException` covers the case and `UserLoginCommand` already handles - it. `UsernameDoesNotExistException` is unaffected. -- `symfony/deprecation-contracts` from `require`, the last - `trigger_deprecation()` call having gone with the option becoming required. +- `ItkOpenIdConnectBundleException`. Catch `OpenIdConnectBundleExceptionInterface`. +- `UserDoesNotExistException`. Use Symfony's `UserNotFoundException`. +- `symfony/deprecation-contracts` from `require`. ## [5.1.1] - 2026-08-19 @@ -371,7 +327,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `itk-dev/openid-connect` 1.0.0 to 2.1.0 - OpenId Connect Bundle: Added CLI login feature. -[unreleased]: https://github.com/itk-dev/openid-connect-bundle/compare/5.1.1...HEAD +[unreleased]: https://github.com/itk-dev/openid-connect-bundle/compare/6.0.0...HEAD +[6.0.0]: https://github.com/itk-dev/openid-connect-bundle/compare/5.1.1...6.0.0 [5.1.1]: https://github.com/itk-dev/openid-connect-bundle/compare/5.1.0...5.1.1 [5.1.0]: https://github.com/itk-dev/openid-connect-bundle/compare/5.0.0...5.1.0 [5.0.0]: https://github.com/itk-dev/openid-connect-bundle/compare/4.2.0...5.0.0 From 9c61921c073dc67665150f75326a0cb35d9416a1 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 20:09:42 +0200 Subject: [PATCH 28/28] docs: cut UPGRADE-6.0 to the breaking changes and how to adapt 202 lines for four breaking changes, most of it explaining why each decision was taken. Now 80: what changed, what to do about it, and the compile error to search for. The reasoning is in the commits, the PRs and ADR 002; the features are in the README. --- UPGRADE-6.0.md | 196 ++++++++++--------------------------------------- 1 file changed, 37 insertions(+), 159 deletions(-) diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index 0fa75e1..82b6d71 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -1,33 +1,16 @@ # Upgrading from 5.x to 6.0 -Every provider must declare where its callback arrives, a failed callback becomes an -error instead of a redirect, and a callback is only recognised on that path. Most -applications already satisfy the first, in which case there is nothing to change unless -you catch `AuthenticationException` around the callback. - ```sh composer require itk-dev/openid-connect-bundle:^6.0 ``` -`require`, not `update`: your `composer.json` pins a major — `^5.0` or similar — and -`composer update` will not cross it, so it would report nothing to do and leave you on -5.x wondering why none of this applies. - -Coming from 4.x? Do [UPGRADE-5.0.md](UPGRADE-5.0.md) first — this guide assumes 5.x. On -that hop the library moves with the bundle, and naming the bundle alone refuses with -`the package is fixed to 4.1.2 (lock file version) by a partial update`, so name both: - -```sh -composer require itk-dev/openid-connect-bundle:^5.0 itk-dev/openid-connect:^5.0 -``` - -From 5.x to 6.0 the bundle alone is enough: the only requirement that changes is -`symfony/deprecation-contracts`, which is dropped. +`require`, not `update`: `composer update` will not cross the major your +`composer.json` pins. Coming from 4.x, do [UPGRADE-5.0.md](UPGRADE-5.0.md) first. -## 1. Declare where each callback arrives +## Every provider must declare where its callback arrives -Every provider must set one of `redirect_uri`, `redirect_route` or the new -`callback_path`, or the container will not compile: +Set one of `redirect_uri`, `redirect_route` or `callback_path` per provider, or the +container will not compile: ```text Invalid configuration for path "itkdev_openid_connect.openid_providers.admin.options": @@ -35,19 +18,13 @@ One of redirect_uri, redirect_route or callback_path must be set: it is how a ca is recognised. ``` -Most applications already have `redirect_uri` and need no change. +Most applications already set `redirect_uri` and need no change. -A request is treated as a callback when it carries `state` and `code` **and** arrives -on that path. `?state=…&code=…` on any other URL is left to the firewall, exactly as -before 6.0 — an anonymous visitor goes to your entry point, a logged-in one gets the -page. Without the path check, and with failures now raising an exception (step 2), any URL -in the application was a 500 an anonymous caller could trigger. +## A callback is only recognised on that path -### When you need `callback_path` - -Only for a reverse proxy that rewrites the path **without announcing it** — an -external `https://app.example.org/prefix/auth/callback` that arrives here as -`/auth/callback`: +`?state=…&code=…` on any other URL is left to the firewall. If a reverse proxy rewrites +the path without sending `X-Forwarded-Prefix`, declare the path the application +receives: ```yaml openid_providers: @@ -57,146 +34,47 @@ openid_providers: callback_path: '/auth/callback' ``` -A subdirectory deployment, or a proxy sending `X-Forwarded-Prefix` with Symfony's -trusted proxies configured, needs none: the request path is matched as `getBaseUrl()` -plus `getPathInfo()`, so the prefix is accounted for on both sides. +A subdirectory deployment, or a proxy sending `X-Forwarded-Prefix` with trusted proxies +configured, needs no `callback_path`. -### One authenticator per provider +## A failed callback throws `AuthenticationFailedException` -Override `getSupportedProviderKeys()` so each answers only its own provider's -callback: +`OpenIdLoginAuthenticator::onAuthenticationFailure()` now throws +`\ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException`, which is not a +Symfony `AuthenticationException` and so escapes the firewall. Your application renders +it — a 500 by default. -```php -protected function getSupportedProviderKeys(): array -{ - return ['admin']; -} -``` - -Without the override every authenticator supports every callback path and the -session's provider key decides which provider validates it — exactly as in 5.x. - -## 2. A failed callback is now an error, not another redirect - -`OpenIdLoginAuthenticator::onAuthenticationFailure()` throws -`\ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException` instead of -Symfony's `AuthenticationException`. The security component used to catch the latter -and call your entry point again, so a permanent failure such as an expired client -secret looped forever with no error page. The new exception escapes the firewall and -your application renders it — a 500 by default. See -[ADR 002](docs/adr/002-fail-closed-on-authentication-failure.md). - -**Most applications need no code change.** Catching library exceptions *inside* -`authenticate()` — the pattern in this README — is unaffected. What needs attention is -a `catch` of Symfony's `AuthenticationException` around the callback, or an overridden -`onAuthenticationFailure()`: +Nothing to do unless you catch `AuthenticationException` around the callback, or +override `onAuthenticationFailure()`: ```diff - } catch (\Symfony\Component\Security\Core\Exception\AuthenticationException $e) { + } catch (\ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface $e) { ``` -`getPrevious()` is the underlying OpenID Connect exception rather than the -`AuthenticationException`, because the security listener walks the chain and one left -there would loop again. - -Then check that a failed login renders something acceptable and that your error -reporting picks it up. - -### Rendering something friendlier than a 500 - -Listen for the exception, and do not answer with a redirect to the login route — that -reintroduces the loop: - -```php -#[AsEventListener] -final class LoginFailureListener -{ - public function __construct(private Environment $twig) - { - } - - public function __invoke(ExceptionEvent $event): void - { - if ($event->getThrowable() instanceof AuthenticationFailedException) { - $event->setResponse(new Response($this->twig->render('login_failed.html.twig'), 503)); - } - } -} -``` +`getPrevious()` is the underlying OpenID Connect exception, not the +`AuthenticationException`. + +To render something friendlier than a 500, listen for the exception and set a response. +Do not redirect to the login route — that reintroduces the loop this replaced. Render a +template rather than `getMessage()`, which carries the identity provider's error text. -Render your own template rather than `getMessage()`: the message carries the identity -provider's error text, which the security component used to reduce to a safe message -key before anything could display it. +`CliLoginTokenAuthenticator` is unchanged. -## 3. Removed exceptions +## Removed | Removed | Use instead | | --- | --- | -| `ItkOpenIdConnectBundleException` (abstract, `@deprecated` since 5.0) | `OpenIdConnectBundleExceptionInterface` | -| `UserDoesNotExistException` | Symfony's `UserNotFoundException`, which the bundle already handles | - -`UserDoesNotExistException` was thrown nowhere. If your user provider throws it, -switch to `UserNotFoundException`: `UserLoginCommand` catches that and reports the -username as unknown. Not to be confused with `UsernameDoesNotExistException`, which -stays — the CLI authenticator throws it when a token resolves to no username. - -`symfony/deprecation-contracts` is no longer required by the bundle. Require it -yourself if your own code calls `trigger_deprecation()`. - -## Recommended: monitor client secret expiry - -`client_secret_expires_at` stays optional. Set it and the bundle warns before a secret -expires; leave it unset and the provider reports `unknown` and is not monitored. - -```yaml -itkdev_openid_connect: - openid_providers: - admin: - options: - client_secret_expires_at: '%env(string:ADMIN_OIDC_CLIENT_SECRET_EXPIRES_AT)%' -``` - -**Set it where the real secret lives** — the production secret store, or a `when@prod` -block. A date carried in a committed `.env` default is a date nobody maintains: it -reports `ok` while measuring nothing, which is worse than reporting `unknown`. That is -also why the key is not required: a required key can force a value, never a correct -one, and Symfony compiles a container per environment, so it would have to be set in -all of them. - -Anything `strtotime()` understands, and **quote it**: YAML reads an unquoted -`2027-01-31` as the number `1801353600`, and a non-string is rejected while the -container compiles. A value that is set but cannot be parsed is reported at `error` and -treated as `unknown` — set-but-broken is a mistake, unset is a decision. - -Nothing here blocks a login. An unmonitored provider shows up as `unknown` in -`ClientSecretExpiryChecker::getAllStatuses()`, which is where monitoring should alert -on it. The 5.1 deprecation for leaving the date unset is gone. - -## Optional: return users to the page they asked for - -`createTargetPathRedirect()` sends the user back to whatever sent them to log in, -falling back to a URL of your choosing: - -```php -public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): ?Response -{ - return $this->createTargetPathRedirect($request, $firewallName, $this->router->generate('dashboard')); -} -``` - -Existing `onAuthenticationSuccess()` implementations keep working unchanged. - -Only pages that exist and are access-controlled come back this way: routing runs -before security, so a link to a URL with no route is a 404 before the firewall sees it -and there is nothing to return to. +| `ItkOpenIdConnectBundleException` | `OpenIdConnectBundleExceptionInterface` | +| `UserDoesNotExistException` | Symfony's `UserNotFoundException` | -A login link on a public page can name its own destination with -`?target_path=/admin/reports`. The value must be a path within the application, or it -is dropped and logged. +`UsernameDoesNotExistException` stays. `symfony/deprecation-contracts` is no longer +required by the bundle; require it yourself if your own code calls +`trigger_deprecation()`. -## Unchanged: CLI login +## Also worth knowing -`CliLoginTokenAuthenticator` still throws `AuthenticationException`, so a consumed or -invalid login token still sends the user to your login page. That path has no entry -point of its own and cannot loop. +`client_secret_expires_at` is still optional, and the 5.1 deprecation for leaving it +unset is gone. New in 6.0: `callback_path`, `getSupportedProviderKeys()`, +`createTargetPathRedirect()` and `?target_path=` — see the +[README](README.md) and [CHANGELOG](CHANGELOG.md).