From 482c4a782bebdb6cd43866e46a713f5f20576ff3 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 25 Aug 2026 21:23:18 +0200 Subject: [PATCH 1/8] docs: update Symfony native OIDC note for upstream oidc_login --- README.md | 54 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 2b0930c..529149c 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,8 @@ Symfony bundle for authorization via OpenID Connect. > > ## Symfony Native OIDC Support > +> Status as of August 2026. +> > Since this bundle was created Symfony has added [support for OpenID Connect](https://symfony.com/blog/new-in-symfony-6-3-openid-connect-token-handler) > as documented in ["Using OpenID Connect (OIDC)"](https://symfony.com/doc/current/security/access_token.html#using-openid-connect-oidc). > @@ -30,36 +32,56 @@ Symfony bundle for authorization via OpenID Connect. > * [JWE (encrypted token) support](https://github.com/symfony/symfony/pull/57721) > was added in Symfony 7.3 for OIDC token handlers. > -> However, Symfony's native OIDC support is designed for **stateless bearer -> token validation** (the `access_token` authenticator) only. It validates tokens -> that are already present on the request (e.g. in an `Authorization: Bearer` -> header). -> -> It does **not** implement the **authorization code flow** — the browser-based +> Everything released so far is designed for **stateless bearer token +> validation** (the `access_token` authenticator) only. It validates tokens that +> are already present on the request (e.g. in an `Authorization: Bearer` header), +> and does not implement the **authorization code flow** — the browser-based > login where the application redirects to the IdP, handles the callback with an -> authorization code, exchanges it for tokens, and establishes a session. This -> is tracked upstream in [symfony/symfony#50896](https://github.com/symfony/symfony/issues/50896). +> authorization code, exchanges it for tokens, and establishes a session. That +> gap is tracked in [symfony/symfony#50896](https://github.com/symfony/symfony/issues/50896). +> +> ### The authorization code flow is coming upstream +> +> A native `oidc_login` authenticator is being added in +> [symfony/symfony#64954](https://github.com/symfony/symfony/pull/64954), +> targeted at **Symfony 8.2 (November 2026)**. The pull request is in active +> review and is being reworked into a feature-complete implementation covering +> discovery, PKCE, configurable scopes and claims mapping, token-endpoint client +> authentication, and RP-initiated logout. One review question is still open — +> ID token signature verification, which this bundle's underlying library +> already does. > -> This means the following features of this bundle have no native Symfony -> equivalent: +> ### What this bundle still provides > > | Feature | This bundle | Symfony native | > |--------------------------------|:-----------:|:--------------:| -> | Authorization code flow | ✅ | ❌ | -> | Session-based browser login | ✅ | ❌ | -> | Multiple named OIDC providers | ✅ | ❌ ¹ | +> | Authorization code flow | ✅ | ⏳ ¹ | +> | Session-based browser login | ✅ | ⏳ ¹ | +> | Multiple named OIDC providers | ✅ | ❌ ² | > | CLI login tokens | ✅ | ❌ | +> | Client secret expiry checks | ✅ | ❌ | > | OIDC discovery | ✅ | ✅ | > | Bearer token validation (API) | ❌ | ✅ | > | OAuth2 token introspection | ❌ | ✅ | > -> ¹ Symfony's `access_token` handler accepts multiple `issuers` for token +> ¹ In review for Symfony 8.2, see above. +> +> ² Symfony's `access_token` handler accepts multiple `issuers` for token > validation, but this is not the same as this bundle's named provider model > with distinct client credentials, redirect URIs, and selectable login routes > per provider. > -> If your application needs browser-based OIDC login, this bundle is still -> required. +> ### What this means for the bundle +> +> Long term we expect Symfony core to replace most of this bundle. It is not +> there yet: multiple providers per firewall, CLI login and the client secret +> expiry checks have no upstream equivalent, and our applications track Symfony +> LTS releases. +> +> Until those gaps close the bundle remains fully supported. New features that +> upstream will provide are frozen; security and compatibility fixes continue. A +> deprecation will be announced here and in the [CHANGELOG](CHANGELOG.md) once a +> migration path exists — realistically no earlier than 2028. Upgrading from an earlier major? See [UPGRADE-6.0.md](UPGRADE-6.0.md) and [UPGRADE-5.0.md](UPGRADE-5.0.md). From e1403fd4709a57b5f80297b9176e367079843246 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 11:37:39 +0200 Subject: [PATCH 2/8] feat: handle provider error callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6749 §4.1.2.1: a provider that refuses redirects back with `error` and `state` and no `code`. supports() required a code, so that request was not a callback: the firewall answered it, the entry point asked again, and the provider refused again. Captured in production against Azure AD B2C as dozens of rounds, nothing logged, the one-time session values never spent. supports() now accepts `state` with either `code` or `error`, on the callback path only, so ADR 003 and issue #63 are untouched. validateClaims() consumes all three one-time values up front, checks state before reading anything else the URL carries, and throws ProviderErrorException with the provider's error code as an accessor. ProviderErrorException extends AuthenticationFailedException, so existing catches keep matching, and implements HttpExceptionInterface so a refusal is a 403 rather than a 500. onAuthenticationFailure() rethrows it unwrapped. State is now compared with hash_equals(), behind an explicit guard against an empty or missing stored value. See ADR 004. --- CHANGELOG.md | 31 ++ README.md | 81 ++++- UPGRADE-6.1.md | 59 ++++ .../004-handle-provider-error-callbacks.md | 145 ++++++++ docs/adr/README.md | 3 + src/Exception/ProviderErrorException.php | 94 +++++ src/Security/OpenIdLoginAuthenticator.php | 155 ++++++-- tests/Exception/ExceptionHierarchyTest.php | 24 ++ .../Exception/ProviderErrorExceptionTest.php | 87 +++++ .../Security/OpenIdLoginAuthenticatorTest.php | 332 ++++++++++++++++++ .../ProviderErrorCallbackDoesNotLoopTest.php | 213 +++++++++++ 11 files changed, 1199 insertions(+), 25 deletions(-) create mode 100644 UPGRADE-6.1.md create mode 100644 docs/adr/004-handle-provider-error-callbacks.md create mode 100644 src/Exception/ProviderErrorException.php create mode 100644 tests/Exception/ProviderErrorExceptionTest.php create mode 100644 tests/Security/ProviderErrorCallbackDoesNotLoopTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index a7e8c3a..7d58bad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,37 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `ProviderErrorException`, thrown when the identity provider refuses the authorization + request (RFC 6749 §4.1.2.1). It extends `AuthenticationFailedException`, so existing + `catch` blocks keep matching, and carries `getError()`, `getErrorDescription()` and + `getStatusCode()`. See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md). + +### Fixed + +- A provider error callback no longer loops between the application and the identity + provider. `supports()` accepts a callback carrying `state` and either `code` or + `error`, so a refusal — a cancelled consent screen, an expired provider session, a + tenant policy — ends in a page that says so instead of another authorization request. + Observed against Azure AD B2C. + +### Changed + +- A refused login is answered with the status that matches its cause: 403 where the + user or a policy declined, 503 where the provider reports its own trouble, 500 + otherwise. Other callback failures are unchanged and still surface as 500. +- `error` and `error_description` are sanitized before they are logged or held — + control characters collapsed, invalid UTF-8 dropped, capped at 200 characters — and + neither is read at all until the callback's state matches. +- `oauth2provider`, `oauth2state` and `oauth2nonce` are consumed on every callback, + including one carrying a provider error. +- The stored state is compared with `hash_equals()`, and an empty or missing stored + state is rejected explicitly rather than by comparison. +- A callback naming a provider that is not configured is now reported as an invalid + state at `warning` when its state does not match, rather than as an unconfigured + provider at `error`: the provider is built after the state check, not before it. + ## [6.0.0] - 2026-08-25 See [UPGRADE-6.0.md](UPGRADE-6.0.md). diff --git a/README.md b/README.md index 529149c..96afa7b 100644 --- a/README.md +++ b/README.md @@ -83,8 +83,9 @@ Symfony bundle for authorization via OpenID Connect. > deprecation will be announced here and in the [CHANGELOG](CHANGELOG.md) once a > migration path exists — realistically no earlier than 2028. -Upgrading from an earlier major? See [UPGRADE-6.0.md](UPGRADE-6.0.md) and -[UPGRADE-5.0.md](UPGRADE-5.0.md). +Upgrading? See [UPGRADE-6.1.md](UPGRADE-6.1.md), and +[UPGRADE-6.0.md](UPGRADE-6.0.md) / [UPGRADE-5.0.md](UPGRADE-5.0.md) if you are coming +from an earlier major. ## Installation @@ -671,8 +672,9 @@ class SomeAuthenticator extends OpenIdLoginAuthenticator // TODO: Implement authenticate() method. } catch (ItkOpenIdConnectException $exception) { - // Authentication failed - throw new CustomUserMessageAuthenticationException($exception->getMessage()); + // Authentication failed. Chain the cause: the bundle reads it back in + // onAuthenticationFailure() to decide what the user is shown. + throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); } } @@ -858,7 +860,7 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator return new SelfValidatingPassport(new UserBadge($user->getUserIdentifier())); } catch (ItkOpenIdConnectException|InvalidProviderException $exception) { - throw new CustomUserMessageAuthenticationException($exception->getMessage()); + throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); } } @@ -882,6 +884,75 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator } ``` +### When the identity provider refuses + +A provider that will not issue a code redirects back to the callback with an `error` +and no `code` — the user closed the consent screen, their session at the provider had +expired, a tenant policy said no. The bundle recognises that callback, spends the +one-time session values like any other, and throws `ProviderErrorException`. + +It extends `AuthenticationFailedException`, so anything already catching the bundle's +login failure catches this too, and it implements Symfony's `HttpExceptionInterface`, +so the kernel answers a refusal with **403** rather than a 500 — 503 where the +provider reports its own trouble, 500 where the error says our request or +registration is wrong. Nothing is required of the application to get that. + +The error code is an accessor, not something to search the message for: + +```php +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; +use Symfony\Component\EventDispatcher\Attribute\AsEventListener; +use Symfony\Component\HttpKernel\Event\ExceptionEvent; +use Symfony\Component\HttpKernel\KernelEvents; + +#[AsEventListener(KernelEvents::EXCEPTION, priority: 1)] +public function onLoginRefused(ExceptionEvent $event): void +{ + $exception = $event->getThrowable(); + + if (!$exception instanceof ProviderErrorException) { + return; + } + + $template = ProviderErrorException::ACCESS_DENIED === $exception->getError() + ? 'security/login_cancelled.html.twig' + : 'security/login_failed.html.twig'; + + $event->setResponse(new Response( + $this->twig->render($template, ['error' => $exception->getError()]), + $exception->getStatusCode(), + )); +} +``` + +`error` and `error_description` reach you sanitized — control characters collapsed, +invalid UTF-8 dropped, capped at 200 characters — and neither is read at all until +the callback's state matches, so a forged callback cannot put text in your logs or on +your page. `getErrorDescription()` is whatever the provider sent, which may be +nothing; it is a diagnostic, not a message to show a user. + +You can also pin the status and log level without writing a listener: + +```yaml +framework: + exceptions: + ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException: + log_level: info + status_code: 403 +``` + +One thing is required of your authenticator: when `authenticate()` catches a bundle +exception and raises Symfony's, **chain the cause** — `previous: $exception`, as the +examples above do. The bundle reads it back to decide what the user is shown, and an +unchained failure arrives as a plain 500 with the reason only in the message. + +If your application has its own listener that redirects 403 responses to a login +page, exclude `ProviderErrorException` from it. Otherwise a refusal is sent straight +back to the provider that refused it, which is the loop this handling exists to +prevent. + +See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning. + ## Sign in from command line Rather than signing in via OpenId Connect, you can get a sign in url from the diff --git a/UPGRADE-6.1.md b/UPGRADE-6.1.md new file mode 100644 index 0000000..10d2e68 --- /dev/null +++ b/UPGRADE-6.1.md @@ -0,0 +1,59 @@ +# Upgrading from 6.0 to 6.1 + +```sh +composer update itk-dev/openid-connect-bundle +``` + +A minor: nothing is required of you. Two things are worth checking. + +## A refused login now ends in a page, not a loop + +When the identity provider refuses an authorization request — the user closed the +consent screen, their session at the provider had expired, a tenant policy said no — +it redirects back to your callback with an `error` and no `code`. + +Until 6.1 the bundle did not recognise that as a callback. The firewall answered it, +your entry point asked the provider again, the provider refused again, and the browser +never settled. Nothing was logged, because no failing callback existed to log. + +Now that callback is handled: the login ends, the reason is logged at `warning`, and +the user gets a **403** — 503 where the provider reports its own trouble, 500 where the +error says the request or the client registration is wrong. If you were filtering the +loop out of your monitoring, you can stop. + +Catch it if you want a friendlier page than your generic 403: + +```php +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; + +if ($exception instanceof ProviderErrorException + && ProviderErrorException::ACCESS_DENIED === $exception->getError()) { + // "You cancelled the sign-in — try again" +} +``` + +`ProviderErrorException` extends `AuthenticationFailedException`, so any `catch` you +already wrote for 6.0 keeps matching it. + +**If your application has a listener that redirects 403 responses to a login page, +exclude this exception from it** — otherwise a refusal is sent straight back to the +provider that refused it, rebuilding the loop from your own side. + +## Chain the cause in your authenticator + +When your `authenticate()` catches a bundle exception and raises Symfony's, pass the +original as `previous`: + +```php +} catch (OpenIdConnectExceptionInterface $exception) { + throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); +} +``` + +The bundle reads that cause back to decide what the user is shown. Without it the +refusal arrives as a plain 500 with the reason only in the message. This was always +the documented shape; 6.1 is the first release where dropping the cause costs you +something. + +See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning, and +[CHANGELOG.md](CHANGELOG.md) for the rest of the release. diff --git a/docs/adr/004-handle-provider-error-callbacks.md b/docs/adr/004-handle-provider-error-callbacks.md new file mode 100644 index 0000000..ca9c18b --- /dev/null +++ b/docs/adr/004-handle-provider-error-callbacks.md @@ -0,0 +1,145 @@ +# 004: Handle provider error callbacks + +- **Created By:** Ture Gjørup +- **Date:** 2026-08-26 +- **Decision Maker:** Ture Gjørup +- **Stakeholders:** Bundle consumers; operators of those applications; bundle + maintainers +- **Status:** Accepted + +## Context + +[RFC 6749 §4.1.2.1](https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1) +gives an authorization request two possible answers. One is the callback everyone +thinks about, carrying `code` and `state`. The other is a refusal, carrying `error` +and `state` and no `code` at all: the user declined consent, the session at the +provider had expired, the tenant policy said no. + +`OpenIdLoginAuthenticator::supports()` required both `state` and `code`, so a refusal +was not a callback. The firewall answered it as an ordinary unauthenticated request, +which means calling the entry point, which starts a fresh authorization request, +which the provider refuses again. Captured in production against Azure AD B2C: +dozens of rounds between the login route and the callback path, the browser never +settling. Nothing was logged, because no failing callback existed to log. The +one-time `oauth2state`, `oauth2nonce` and `oauth2provider` values were never +consumed, so each round replayed a session that was already half spent. + +This is the same shape as the outage behind +[ADR 002](002-fail-closed-on-authentication-failure.md), reached through the one door +that decision did not close. ADR 002 stopped a *failing* callback from being retried; +it did not help a request that was never recognised as a callback. + +## Options Considered + +1. **Recognise the error callback and end it (chosen).** The request the provider + actually sent gets an answer, the one-time values are consumed exactly as on any + other callback, and the reason reaches the log and the page. +2. **Leave `supports()` alone and count entry-point invocations in the session.** The + objections ADR 002 raised against a counter apply unchanged — it bounds the + symptom rather than removing the cause, and picks an arbitrary limit. It also + treats a refusal, which is a normal outcome, as an anomaly to be detected. +3. **Recognise it but reuse the state-mismatch failure.** Terminates the loop, but + throws away the only thing the provider told us. An expired consent grant and a + forged callback would be indistinguishable in the log and on the page, and the + status would be a 500 either way. + +## Decision + +Adopt option 1 in 6.1.0. `supports()` returns `true` on a configured callback path +when the query carries `state` and either `code` or `error`. + +- **State is checked before anything else the URL carries.** `error` and + `error_description` are chosen by whoever built the callback URL. Only a matching + state says the request belongs to a login this browser started, so nothing else in + the query is read, logged, stored or repeated back until it matches. +- **All three one-time values are consumed up front**, before any check can throw. A + callback is spent whether it succeeds, fails validation, or carries a refusal. A + value left behind is one a later request can replay. +- **An empty `error` still counts as a callback.** `supports()` tests for the + parameter's presence, not for a usable value. Anything else hands the request back + to the entry point, which mints a fresh state — and the next refusal then arrives + with a state that matches, making the loop indistinguishable from a first attempt. +- **The provider's text is sanitized before it is logged, held or shown.** Runs of + control characters collapse to a single space, input that is not valid UTF-8 is + dropped, and what remains is capped at 200 characters. +- **A distinct type, `ProviderErrorException`, carries it**, extending + `AuthenticationFailedException` so that every `catch` already written against the + bundle's login failure keeps matching. The error code is an accessor, + `getError()`, not message text a consumer would have to search for. +- **The type answers `getStatusCode()`**, mapping a refusal to 403, a provider outage + to 503, and everything else to 500. A user who clicked Cancel gets a page, not an + incident. +- **`onAuthenticationFailure()` rethrows it unwrapped.** It is already outside the + security hierarchy and carries nothing beneath it, which is what ADR 002 requires + of anything leaving there; wrapping it would only discard the status. +- **Nothing is constructed until the cheap checks pass.** Building a provider pulls + in discovery, an HTTP client and a cache pool, and a refusal has no use for any of + them, so `getProvider()` now runs after the state check rather than before it. + +## Consequences + +The loop cannot form on this shape either, and as with +[ADR 003](003-constrain-supports-to-callback-path.md) it is prevented by a type and a +path check rather than by a counter. Refusals become visible in the log at a level +that matches who is at fault: `warning` from the bundle, and a 4xx that Symfony's +`ErrorListener` records at `error` rather than `critical`. + +Accepted costs: + +- The bundle now states an HTTP status from a class under `Security/` rather than + `Controller/`. The status is metadata the kernel reads off the exception, not a + response the bundle renders, and the alternative is paging an operator every time a + user changes their mind. +- An application whose own listener redirects 403 responses to a login page can + rebuild a loop for itself. That listener is the application's, not the firewall's, + and `ProviderErrorException` is a distinct type precisely so it can be excluded. +- A callback carrying an `error` with nothing usable in it — empty, an array, or + nothing but control characters — is reported as a missing code. It still ends the + callback; it simply has no refusal to report. +- The reordering means a forged callback naming a provider that is no longer + configured is now reported as an invalid state at `warning`, where it used to be + reported as an unconfigured provider at `error`. +- The status reaches the application only if the consumer's `authenticate()` chains + the bundle exception into the `AuthenticationException` it raises, which is what + the documented subclass does. A consumer that drops the cause gets a 500 with the + reason in the message. + +## How much of what the provider says we repeat + +`error` and `error_description` arrive in a URL. On a good day the identity provider +put them there; on a bad one, anyone who can get a browser to load a link. They are +treated as input, not as a message. + +Control characters go first, because a newline in a log record forges a second +record, and an escape sequence is a command to whichever terminal someone reads the +log in. Input that is not valid UTF-8 is dropped, because the first JSON formatter to +meet it throws — replacing a legible failure with an illegible one, inside the code +that was handling a failure. What survives is capped, so no one can fill a log +pipeline with their own prose. The cap counts characters rather than bytes: cutting a +multi-byte character in half would produce exactly the invalid UTF-8 the step before +it just rejected. + +The same sanitized values, and only those, reach the exception, so nothing raw +crosses the bundle's public surface. And none of it is read at all until the state +matches. + +## References + +- [RFC 6749 §4.1.2.1](https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1) — + the error response, and [§10.12](https://datatracker.ietf.org/doc/html/rfc6749#section-10.12) + for what `state` is for +- [OpenID Connect Core 1.0 §3.1.2.6](https://openid.net/specs/openid-connect-core-1_0.html#AuthError) — + authentication error response +- [OAuth 2.0 Security Best Current Practice](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics) — + on what may be trusted in callback parameters +- [OWASP Logging Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html) — + log injection via untrusted text +- [Symfony: `HttpExceptionInterface` and error handling](https://symfony.com/doc/current/controller/error_pages.html), + and [`framework.exceptions`](https://symfony.com/doc/current/reference/configuration/framework.html#exceptions) + for pinning a status and log level per exception class +- [Microsoft Entra External ID error codes](https://learn.microsoft.com/en-us/azure/active-directory-b2c/error-codes) — + the vendor codes that make the `default` arm of the status mapping necessary +- [ADR 002](002-fail-closed-on-authentication-failure.md) — the fail-closed decision + this preserves +- [ADR 003](003-constrain-supports-to-callback-path.md) — the path constraint that + keeps the widened `supports()` from reopening issue #63 diff --git a/docs/adr/README.md b/docs/adr/README.md index 0b58831..d3d159c 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -18,3 +18,6 @@ context that drove them and the consequences we accept. See - **[003 — Treat only the configured callback path as a callback](003-constrain-supports-to-callback-path.md)** — Accepted — 2026-08-20 +- **[004 — Handle provider error + callbacks](004-handle-provider-error-callbacks.md)** — Accepted — + 2026-08-26 diff --git a/src/Exception/ProviderErrorException.php b/src/Exception/ProviderErrorException.php new file mode 100644 index 0000000..808073c --- /dev/null +++ b/src/Exception/ProviderErrorException.php @@ -0,0 +1,94 @@ +error; + } + + /** + * The provider's description of the error, sanitized, or null if it sent none usable. + */ + public function getErrorDescription(): ?string + { + return $this->errorDescription; + } + + public function getStatusCode(): int + { + return match ($this->error) { + // The user, or a policy standing in for them, said no. Nothing is + // broken and nobody needs paging. + self::ACCESS_DENIED, 'login_required', 'consent_required', + 'interaction_required', 'account_selection_required' => Response::HTTP_FORBIDDEN, + // The provider is having a bad day and says so — the same answer + // LoginController gives when it cannot reach one at all. + 'server_error', 'temporarily_unavailable' => Response::HTTP_SERVICE_UNAVAILABLE, + // Everything else says our request or our registration was wrong: + // invalid_request, unauthorized_client, invalid_scope, and whatever a + // given provider invents. That is an operator's problem, and 500 is how + // the application already reports one. + default => Response::HTTP_INTERNAL_SERVER_ERROR, + }; + } + + /** + * @return array + */ + public function getHeaders(): array + { + return []; + } +} diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index ee9cd81..0f22488 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -6,6 +6,7 @@ use ItkDev\OpenIdConnect\Exception\ValidationException; use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; @@ -22,8 +23,9 @@ * * A failed callback throws `AuthenticationFailedException`, which is not an * `AuthenticationException` and so is not turned back into another redirect to the - * identity provider. Consuming applications see a 500 and can render whatever they - * like from it; what they no longer see is an unbreakable redirect loop. + * identity provider. Consuming applications see an error they can render whatever + * they like from — a 403 where the provider refused, a 500 otherwise; what they no + * longer see is an unbreakable redirect loop. * * The logger is injected through `setLogger()` rather than the constructor on * purpose: this class is extended by consuming applications, whose subclasses @@ -58,6 +60,15 @@ abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements */ public const string TARGET_PATH_SESSION_KEY = '_itkdev_oidc.target_path'; + /** + * The cap on provider-supplied text, in characters. + * + * Long enough for a real `error_description` — Azure AD B2C's run to a line or + * two — and short enough that a log pipeline cannot be filled with somebody + * else's prose. + */ + private const int PROVIDER_TEXT_MAX_LENGTH = 200; + private LoggerInterface $logger; /** @@ -83,6 +94,17 @@ public function setLogger(LoggerInterface $logger): void * Requiring the configured callback path as well leaves a forged callback to the * firewall's ordinary handling. * + * `error` is the other half of RFC 6749 §4.1.2.1: a provider that refuses + * redirects back with `error` and `state` and no `code` at all. Left + * unrecognised, that request falls to the firewall, the firewall calls this + * authenticator's entry point, the entry point asks the provider again, and the + * provider refuses again — a loop with no failing callback anywhere in it to log. + * + * `has()`, deliberately, rather than a test for a non-empty value: `?state=…&error=` + * has to be recognised too. If it is not, the entry point fires, mints a fresh + * state, and the next refusal arrives with a state that matches — a loop that + * cannot even be told apart from a first attempt. + * * 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. @@ -91,7 +113,7 @@ public function setLogger(LoggerInterface $logger): void */ public function supports(Request $request): ?bool { - if (!$request->query->has('state') || !$request->query->has('code')) { + if (!$request->query->has('state') || (!$request->query->has('code') && !$request->query->has('error'))) { return false; } @@ -176,15 +198,68 @@ protected function getSupportedProviderKeys(): array protected function validateClaims(Request $request): array { $session = $request->getSession(); + + // Every one-time value is spent here, before anything below can throw. A + // callback is used up whether it succeeds, fails validation, or carries the + // provider's refusal, and a value left behind is one a later request can + // replay. $providerKey = $session->remove('oauth2provider'); $providerKey = is_string($providerKey) ? $providerKey : ''; + $oauth2state = $session->remove('oauth2state'); + $oauth2nonce = $session->remove('oauth2nonce'); // The session entry is removed above, so carry the provider key on the // request for anything downstream that needs to attribute this login — // the audit subscriber in particular, which sees only the security event. $request->attributes->set(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE, $providerKey); + // Read as an array rather than through InputBag::get(): `?state[]=x` makes + // that throw Symfony's BadRequestException, and a method whose whole job is + // to end a callback cleanly should not be the place a framework exception + // escapes from. A non-string simply fails the comparison below. + $query = $request->query->all(); + $state = $query['state'] ?? null; + + // Make sure state and oauth2state are the same. First, and before anything + // else in this URL is read: until the state matches, none of it is known to + // belong to a login this browser started, and the rest is whatever the + // sender chose to put there. + if (!is_string($oauth2state) || '' === $oauth2state || !is_string($state) || !hash_equals($oauth2state, $state)) { + $this->logger->warning('OIDC login failed: invalid state', ['provider' => $providerKey]); + + throw new ValidationException('Invalid state'); + } + + // RFC 6749 §4.1.2.1: a refusal comes back as `error` with no `code`. Handled + // here rather than left to the token exchange, which would report a missing + // code and drop the only thing that says why the login did not happen. + $error = self::sanitizeProviderText($query['error'] ?? null); + + if (null !== $error) { + $errorDescription = self::sanitizeProviderText($query['error_description'] ?? null); + + // Warning, not error: much the commonest cause is a user who decided + // not to log in, and there is nothing for an operator to fix. The + // status code on the exception carries the distinction that matters. + $this->logger->warning('OIDC login failed: the identity provider refused the request', [ + 'provider' => $providerKey, + 'error' => $error, + 'error_description' => $errorDescription, + ]); + + throw new ProviderErrorException($error, $errorDescription); + } + + if (!is_string($oauth2nonce) || '' === $oauth2nonce) { + $this->logger->warning('OIDC login failed: nonce empty or not found', ['provider' => $providerKey]); + + throw new ValidationException('Nonce empty or not found'); + } + try { + // Built last, and only for a callback that has passed every check that + // costs nothing: constructing a provider pulls in discovery, an HTTP + // client and a cache pool, none of which a refusal has any use for. $provider = $this->providerManager->getProvider($providerKey); } catch (OpenIdConnectExceptionInterface $exception) { // A callback whose session lost (or never had) the provider key @@ -198,22 +273,6 @@ protected function validateClaims(Request $request): array throw $exception; } - // Make sure state and oauth2state are the same - $oauth2state = $session->remove('oauth2state'); - - if ($request->query->get('state') !== $oauth2state) { - $this->logger->warning('OIDC login failed: invalid state', ['provider' => $providerKey]); - - throw new ValidationException('Invalid state'); - } - - $oauth2nonce = $session->remove('oauth2nonce'); - if (!is_string($oauth2nonce) || '' === $oauth2nonce) { - $this->logger->warning('OIDC login failed: nonce empty or not found', ['provider' => $providerKey]); - - throw new ValidationException('Nonce empty or not found'); - } - try { $code = $request->query->get('code'); @@ -257,7 +316,63 @@ 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(), self::causeOutsideSecurity($exception)); + $cause = self::causeOutsideSecurity($exception); + + // A provider that refused already says why, and in what terms the + // application should answer. Wrapping it would replace a 403 the user + // caused by clicking Cancel with a 500 somebody gets paged for. It is + // already outside the security hierarchy and carries nothing beneath it, + // which is exactly what a type leaving here has to be. + if ($cause instanceof ProviderErrorException) { + throw $cause; + } + + throw new AuthenticationFailedException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), $cause); + } + + /** + * Provider-supplied text, made fit to appear in a log record. + * + * `error` and `error_description` are chosen by whoever built the callback URL: + * the identity provider on a good day, anyone who can get a browser to load a + * URL on a bad one. So the text is treated as input rather than as a message. + * Control characters go, because a newline is a forged second log record and an + * escape sequence is a command to whichever terminal the log is read in; text + * that is not valid UTF-8 goes, because the first JSON formatter to meet it + * throws and replaces a legible failure with an illegible one; what is left is + * capped. + * + * Null for anything unusable — absent, empty, an array (`?error[]=x`), or + * nothing but control characters — so the caller can tell "no error" from "an + * error with nothing in it" in one check. + */ + private static function sanitizeProviderText(mixed $value): ?string + { + if (!is_string($value)) { + return null; + } + + // A run of control characters becomes one space rather than nothing, so a + // two-line description reads as two words and not as one. No /u here: this + // is a byte-level strip that has to work on input that is not valid UTF-8. + $text = trim(preg_replace('/[[:cntrl:]]+/', ' ', $value) ?? ''); + + // Capped in characters, not bytes: a byte cap can cut a multi-byte character + // in half, and half a character is exactly the invalid UTF-8 this is trying + // not to produce. `.` under /u is one code point, and the pattern is + // anchored and greedy, so this is the first N of them. + // + // The /u carries the UTF-8 validity check too: given input that is not valid + // UTF-8 the match fails outright and leaves $matches empty, which is the + // same "unusable" answer. That matters because the first JSON formatter to + // meet invalid UTF-8 throws, and replaces a legible failure with an + // illegible one. An empty $text matches empty and comes out null the same + // way, so there is nothing to check for separately. + $matches = []; + preg_match('/^.{0,'.self::PROVIDER_TEXT_MAX_LENGTH.'}/u', $text, $matches); + $capped = $matches[0] ?? ''; + + return '' === $capped ? null : $capped; } /** diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index 3e140fd..d5db498 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -8,6 +8,7 @@ use ItkDev\OpenIdConnectBundle\Exception\CacheException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; use ItkDev\OpenIdConnectBundle\Exception\TokenNotFoundException; use ItkDev\OpenIdConnectBundle\Exception\UsernameDoesNotExistException; use PHPUnit\Framework\Attributes\DataProvider; @@ -41,6 +42,29 @@ public static function concreteProvider(): iterable yield 'CacheException' => [CacheException::class, \RuntimeException::class]; yield 'TokenNotFoundException' => [TokenNotFoundException::class, \RuntimeException::class]; yield 'AuthenticationFailedException' => [AuthenticationFailedException::class, \RuntimeException::class]; + yield 'ProviderErrorException' => [ProviderErrorException::class, \RuntimeException::class]; + } + + /** + * A new concrete extends an existing one so that every `catch` already written + * against the older type keeps matching it — the SemVer commitment that lets a + * subtype ship in a minor. + * + * @return iterable, class-string<\Throwable>}> + */ + public static function subtypeProvider(): iterable + { + yield 'ProviderErrorException' => [ProviderErrorException::class, AuthenticationFailedException::class]; + } + + /** + * @param class-string<\Throwable> $concrete + * @param class-string<\Throwable> $existing + */ + #[DataProvider('subtypeProvider')] + public function testANewSubtypeExtendsAnExistingConcrete(string $concrete, string $existing): void + { + $this->assertInstanceOf($existing, new $concrete('test')); } /** diff --git a/tests/Exception/ProviderErrorExceptionTest.php b/tests/Exception/ProviderErrorExceptionTest.php new file mode 100644 index 0000000..5e33208 --- /dev/null +++ b/tests/Exception/ProviderErrorExceptionTest.php @@ -0,0 +1,87 @@ + + */ + public static function statusCodeProvider(): iterable + { + // The user, or a policy standing in for them, said no. + yield 'access_denied' => [ProviderErrorException::ACCESS_DENIED, Response::HTTP_FORBIDDEN]; + yield 'login_required' => ['login_required', Response::HTTP_FORBIDDEN]; + yield 'consent_required' => ['consent_required', Response::HTTP_FORBIDDEN]; + yield 'interaction_required' => ['interaction_required', Response::HTTP_FORBIDDEN]; + yield 'account_selection_required' => ['account_selection_required', Response::HTTP_FORBIDDEN]; + + // The provider is having a bad day and says so. + yield 'server_error' => ['server_error', Response::HTTP_SERVICE_UNAVAILABLE]; + yield 'temporarily_unavailable' => ['temporarily_unavailable', Response::HTTP_SERVICE_UNAVAILABLE]; + + // Our request or our registration is wrong. + yield 'invalid_request' => ['invalid_request', Response::HTTP_INTERNAL_SERVER_ERROR]; + yield 'unauthorized_client' => ['unauthorized_client', Response::HTTP_INTERNAL_SERVER_ERROR]; + yield 'invalid_scope' => ['invalid_scope', Response::HTTP_INTERNAL_SERVER_ERROR]; + // Providers invent their own; Azure AD B2C's policy errors look like this. + yield 'a vendor code nobody has a rule for' => ['AADB2C90118', Response::HTTP_INTERNAL_SERVER_ERROR]; + } + + #[DataProvider('statusCodeProvider')] + public function testTheStatusCodeSaysWhoIsAtFault(string $error, int $expected): void + { + $this->assertSame($expected, (new ProviderErrorException($error))->getStatusCode()); + } + + public function testTheMessageCarriesBothHalves(): void + { + $exception = new ProviderErrorException('access_denied', 'User cancelled'); + + $this->assertSame('The identity provider refused the request: access_denied (User cancelled)', $exception->getMessage()); + $this->assertSame('access_denied', $exception->getError()); + $this->assertSame('User cancelled', $exception->getErrorDescription()); + } + + public function testTheMessageWithoutADescription(): void + { + $exception = new ProviderErrorException('access_denied'); + + $this->assertSame('The identity provider refused the request: access_denied', $exception->getMessage()); + $this->assertNull($exception->getErrorDescription()); + } + + /** + * Nothing here has a meaningful numeric code, and a consumer switching on one + * would be reading a value the provider never sent. + */ + public function testTheCodeIsZero(): void + { + $this->assertSame(0, (new ProviderErrorException('access_denied'))->getCode()); + } + + public function testACauseIsKept(): void + { + $cause = new \RuntimeException('underneath'); + + $this->assertSame($cause, (new ProviderErrorException('access_denied', null, $cause))->getPrevious()); + } + + /** + * Nothing here warrants a `WWW-Authenticate` or a `Retry-After`: the browser is + * being shown a page, not asked to try again on its own. + */ + public function testNoHeadersAreImposed(): void + { + $this->assertSame([], (new ProviderErrorException('access_denied'))->getHeaders()); + } +} diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 20e05ba..7eece0e 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -10,6 +10,7 @@ use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; @@ -150,6 +151,8 @@ public static function incompleteCallbackProvider(): iterable yield 'neither' => [[]]; yield 'state only' => [['state' => 'abcd']]; yield 'code only' => [['code' => 'xyz']]; + yield 'error only' => [['error' => 'access_denied']]; + yield 'error and code, no state' => [['error' => 'access_denied', 'code' => 'xyz']]; } #[DataProvider('incompleteCallbackProvider')] @@ -160,6 +163,34 @@ public function testTheRightPathAloneIsNotACallback(array $query): void $this->assertFalse($authenticator->supports(Request::create('/callback_uri?'.http_build_query($query)))); } + /** + * A refusal is a callback too, and the path is still what decides. RFC 6749 + * §4.1.2.1 sends `error` with `state` and no `code`. + */ + #[DataProvider('callbackPathProvider')] + public function testAnErrorCallbackIsAlsoACallback(string $path, bool $expected): void + { + $authenticator = $this->authenticatorWithPaths([ + 'test_provider_1' => '/callback_uri', + 'test_provider_2' => '/other_callback', + ]); + + $this->assertSame($expected, $authenticator->supports(Request::create($path.'?state=abcd&error=access_denied'))); + } + + /** + * The one that keeps the loop closed. A provider is entitled to send an empty + * `error`, and if that is not recognised as a callback the firewall answers it, + * the entry point mints a fresh state, and the next refusal comes back with a + * state that matches — a loop indistinguishable from a first attempt. + */ + public function testAnEmptyErrorIsStillACallback(): void + { + $authenticator = $this->authenticatorWithPaths(['test_provider_1' => '/callback_uri']); + + $this->assertTrue($authenticator->supports(Request::create('/callback_uri?state=abcd&error='))); + } + /** * A subclass bound to one provider does not answer another provider's callback, * which is what lets one authenticator per provider share a firewall. @@ -503,6 +534,292 @@ private function assertValidationExceptionMessage(OpenIdLoginAuthenticator $auth $this->fail(sprintf('Expected ValidationException "%s"', $expectedMessage)); } + public function testAProviderErrorIsReportedWithItsCode(): void + { + $request = new Request(query: [ + 'state' => 'test_state', + 'error' => 'access_denied', + 'error_description' => 'User cancelled', + ]); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (\Throwable $thrown) { + // ADR 002: nothing the security component will catch and answer with + // another trip to the identity provider. Asserted before the type is + // narrowed, or it holds statically and proves nothing. + $this->assertNotInstanceOf(AuthenticationException::class, $thrown); + $this->assertNull($thrown->getPrevious(), 'Nothing beneath it for the ExceptionListener to find'); + + $this->assertInstanceOf(ProviderErrorException::class, $thrown); + $this->assertSame('access_denied', $thrown->getError()); + $this->assertSame('User cancelled', $thrown->getErrorDescription()); + + $record = $this->logger->singleRecord(); + $this->assertSame(LogLevel::WARNING, $record['level'], 'A user who changed their mind is not an operator problem'); + $this->assertStringContainsString('refused the request', $record['message']); + $this->assertSame('test_provider_1', $record['context']['provider'] ?? null); + $this->assertSame('access_denied', $record['context']['error'] ?? null); + $this->assertSame('User cancelled', $record['context']['error_description'] ?? null); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + + public function testAProviderErrorWithNoDescriptionReportsNull(): void + { + $request = new Request(query: ['state' => 'test_state', 'error' => 'access_denied']); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (ProviderErrorException $thrown) { + $this->assertNull($thrown->getErrorDescription()); + $this->assertArrayHasKey('error_description', $this->logger->singleRecord()['context']); + $this->assertNull($this->logger->singleRecord()['context']['error_description']); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + + /** + * Constructing a provider pulls in discovery, an HTTP client and a cache pool. + * A refusal has no use for any of it. + */ + public function testAProviderErrorNeverBuildsAProvider(): void + { + $mockManager = $this->createMock(OpenIdConfigurationProviderManager::class); + $mockManager->expects($this->never())->method('getProvider'); + + $authenticator = new TestAuthenticator($mockManager); + $authenticator->setLogger($this->logger); + + $request = new Request(query: ['state' => 'test_state', 'error' => 'access_denied']); + $this->setSessionOnRequest($request); + + $this->expectException(ProviderErrorException::class); + $authenticator->authenticate($request); + } + + /** + * `error` and `error_description` are chosen by whoever built the callback URL. + * Until the state matches, nothing in it is known to belong to a login this + * browser started, so none of it is read, logged or repeated back. + */ + public function testAForgedStateHidesTheProvidersErrorText(): void + { + $request = new Request(query: [ + 'state' => 'wrong_test_state', + 'error' => 'access_denied', + 'error_description' => "attacker\ntext", + ]); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (\Throwable $thrown) { + $this->assertNotInstanceOf(ProviderErrorException::class, $thrown); + $this->assertInstanceOf(ValidationException::class, $thrown); + $this->assertSame('Invalid state', $thrown->getMessage()); + + $record = $this->logger->singleRecord(); + $this->assertSame(LogLevel::WARNING, $record['level']); + $this->assertStringContainsString('invalid state', $record['message']); + + $logged = json_encode($this->logger->records); + $this->assertIsString($logged); + $this->assertStringNotContainsString('access_denied', $logged, 'Nothing the sender wrote reaches the log'); + $this->assertStringNotContainsString('attacker', $logged); + + return; + } + $this->fail('Expected ValidationException'); + } + + /** + * @return iterable}> + */ + public static function oneTimeConsumptionProvider(): iterable + { + yield 'provider error' => ['test_state', ['error' => 'access_denied']]; + yield 'invalid state' => ['wrong_test_state', []]; + yield 'missing code' => ['test_state', []]; + } + + /** + * A value left in the session is one a later request can replay, so a callback + * is spent whatever becomes of it. + * + * @param array $extraQuery + */ + #[DataProvider('oneTimeConsumptionProvider')] + public function testEveryOneTimeSessionValueIsConsumed(string $state, array $extraQuery): void + { + $request = new Request(query: ['state' => $state] + $extraQuery); + $session = $this->realSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (\Throwable) { + // The failure itself is asserted elsewhere; what matters here is what + // the session no longer holds. + } + + $this->assertFalse($session->has('oauth2provider')); + $this->assertFalse($session->has('oauth2state')); + $this->assertFalse($session->has('oauth2nonce')); + } + + /** + * @return iterable + */ + public static function unusableProviderErrorProvider(): iterable + { + yield 'empty' => ['']; + yield 'an array' => [['access_denied']]; + yield 'nothing but control characters' => ["\n\t"]; + yield 'not valid UTF-8' => ["\xC3\x28"]; + } + + /** + * An `error` with nothing usable in it is not reported as a refusal — there + * would be nothing to report — but it still ends the callback rather than + * handing it back to the entry point. + */ + #[DataProvider('unusableProviderErrorProvider')] + public function testAnUnusableProviderErrorFallsThroughToTheMissingCodeFailure(mixed $error): void + { + $request = new Request(query: ['state' => 'test_state', 'error' => $error]); + $this->setSessionOnRequest($request); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Missing or invalid code'); + $this->authenticator->authenticate($request); + } + + public function testTheProvidersErrorTextIsCappedAndCleanedBeforeItIsLogged(): void + { + $request = new Request(query: [ + 'state' => 'test_state', + 'error' => 'access_denied', + 'error_description' => "line one\r\nline two\x1b[31m".str_repeat('a', 10000), + ]); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (ProviderErrorException $thrown) { + $logged = $this->logger->singleRecord()['context']['error_description'] ?? null; + $this->assertIsString($logged); + // Exact, not <=: an off-by-one in the cap has to fail here. + $this->assertSame(200, mb_strlen($logged)); + $this->assertSame(0, preg_match('/[[:cntrl:]]/', $logged), 'No forged log records, no terminal escapes'); + $this->assertStringContainsString('line one line two', $logged, 'A run of control characters reads as one space'); + $this->assertSame($logged, $thrown->getErrorDescription(), 'One sanitized value, two consumers'); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + + public function testCleanProviderTextIsPassedThroughUnchanged(): void + { + $request = new Request(query: [ + 'state' => 'test_state', + 'error' => 'access_denied', + 'error_description' => 'Consent was not granted', + ]); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (ProviderErrorException $thrown) { + $this->assertSame('Consent was not granted', $thrown->getErrorDescription()); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + + /** + * `?state[]=x` makes `InputBag::get()` throw Symfony's `BadRequestException`. + * A method whose whole job is to end a callback cleanly is not where a + * framework exception should escape from. + */ + public function testAnArrayStateIsRejectedAsAnInvalidState(): void + { + $request = new Request(query: ['state' => ['test_state'], 'code' => 'test_code']); + $this->setSessionOnRequest($request); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Invalid state'); + $this->authenticator->authenticate($request); + } + + /** + * @return iterable + */ + public static function unusableStoredStateProvider(): iterable + { + yield 'never stored' => [null]; + yield 'stored empty' => ['']; + yield 'not a string' => [['test_state']]; + } + + /** + * Without the guard, an empty stored state and an empty query state compare + * equal and the callback passes. The safety is local here, not emergent from + * whatever the controller happened to write. + */ + #[DataProvider('unusableStoredStateProvider')] + public function testAnUnusableStoredStateIsAnInvalidState(mixed $stored): void + { + $request = new Request(query: ['state' => '', 'code' => 'test_code']); + $stubSession = $this->createStub(SessionInterface::class); + $stubSession->method('remove')->willReturnMap([ + ['oauth2provider', 'test_provider_1'], + ['oauth2state', $stored], + ['oauth2nonce', 'test_nonce'], + ]); + $request->setSession($stubSession); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Invalid state'); + $this->authenticator->authenticate($request); + } + + /** + * A refusal already says why, and in what terms the application should answer. + * Wrapping it would replace a 403 the user caused by clicking Cancel with a 500 + * somebody gets paged for. + */ + public function testTheProviderErrorLeavesOnAuthenticationFailureUnwrapped(): void + { + $providerError = new ProviderErrorException('access_denied'); + + try { + $this->authenticator->onAuthenticationFailure( + new Request(), + new AuthenticationException('sanitised', 0, $providerError), + ); + } catch (\Throwable $thrown) { + $this->assertNotInstanceOf(AuthenticationException::class, $thrown); + // The SemVer promise: everything already catching the bundle's login + // failure keeps catching this one. Both asserted before assertSame() + // below narrows the type and makes them hold statically. + $this->assertInstanceOf(AuthenticationFailedException::class, $thrown); + $this->assertSame($providerError, $thrown, 'Rethrown as it stands, not rebuilt'); + $this->assertSame(403, $thrown->getStatusCode()); + $this->assertSame([], $this->logger->records, 'Already logged in validateClaims()'); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + private function setSessionOnRequest(Request $request, ?string $nonce = 'test_nonce'): void { $stubSession = $this->createStub(SessionInterface::class); @@ -516,6 +833,21 @@ private function setSessionOnRequest(Request $request, ?string $nonce = 'test_no $request->setSession($stubSession); } + /** + * A real session where the stub above will not do: asserting that a one-time + * value is gone needs a `has()` that answers truthfully. + */ + private function realSessionOnRequest(Request $request): Session + { + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2provider', 'test_provider_1'); + $session->set('oauth2state', 'test_state'); + $session->set('oauth2nonce', 'test_nonce'); + $request->setSession($session); + + return $session; + } + /** * The property above is deliberately typed as the abstract class, so the fixture * method exposing the protected helper needs a concrete local. diff --git a/tests/Security/ProviderErrorCallbackDoesNotLoopTest.php b/tests/Security/ProviderErrorCallbackDoesNotLoopTest.php new file mode 100644 index 0000000..589646d --- /dev/null +++ b/tests/Security/ProviderErrorCallbackDoesNotLoopTest.php @@ -0,0 +1,213 @@ +captureExceptionHandler(); + $this->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(); + } + + protected function tearDown(): void + { + $this->restoreExceptionHandlers(); + } + + /** + * A refusal that belongs to the login this browser started: the state matches. + */ + private function refusedCallback(string $query = 'state=the-real-state&error=access_denied&error_description=User+cancelled'): Request + { + $request = Request::create('/callback_uri?'.$query); + $request->setSession($this->startedLogin()); + + return $request; + } + + private function startedLogin(): Session + { + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2provider', 'test_provider_1'); + $session->set('oauth2state', 'the-real-state'); + $session->set('oauth2nonce', 'the-real-nonce'); + + return $session; + } + + /** + * Guards against the whole test passing vacuously, exactly as the sibling does: + * an unroutable path or a firewall that does not match gives a non-redirect too. + */ + private function assertTheAuthenticatorHandledTheCallback(Request $request): void + { + $this->assertSame( + 'test_provider_1', + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'The request never reached validateClaims(), so this test proves nothing.' + ); + } + + /** + * The assertion the production capture is about. A redirect here is the loop. + */ + public function testAProviderErrorCallbackIsNotAnsweredWithARedirect(): void + { + $request = $this->refusedCallback(); + $response = $this->kernel->handle($request, catch: true); + + $this->assertTheAuthenticatorHandledTheCallback($request); + $this->assertNull( + $response->headers->get('Location'), + 'A refused login was answered with a redirect: the firewall re-entered its entry point and the loop is back.' + ); + $this->assertSame( + Response::HTTP_FORBIDDEN, + $response->getStatusCode(), + 'A user who declined to log in has not caused a server error.' + ); + } + + public function testAProviderErrorConsumesTheOneTimeSessionValues(): void + { + $request = $this->refusedCallback(); + $session = $request->getSession(); + + $this->kernel->handle($request, catch: true); + + $this->assertTheAuthenticatorHandledTheCallback($request); + $this->assertFalse($session->has('oauth2provider')); + $this->assertFalse($session->has('oauth2state')); + $this->assertFalse($session->has('oauth2nonce'), 'The nonce is unread on this path but still spent'); + } + + /** + * The type has to survive the trip through the firewall intact, or the status + * and the error code never reach the application that renders the page. + */ + public function testTheProviderErrorReachesTheApplicationIntact(): void + { + $request = $this->refusedCallback(); + + try { + $this->kernel->handle($request, catch: false); + $this->fail('A refused login should not be handled silently.'); + } catch (ProviderErrorException $exception) { + $this->assertTheAuthenticatorHandledTheCallback($request); + $this->assertSame('access_denied', $exception->getError()); + $this->assertSame('User cancelled', $exception->getErrorDescription()); + $this->assertSame(Response::HTTP_FORBIDDEN, $exception->getStatusCode()); + + // The security ExceptionListener walks the whole chain, so a single + // AuthenticationException anywhere beneath this would rebuild the loop. + for ($cause = $exception; null !== $cause; $cause = $cause->getPrevious()) { + $this->assertNotInstanceOf(AuthenticationException::class, $cause); + } + } + } + + /** + * A forged callback carries whatever text its sender chose. State is checked + * before any of it is read, so none of it reaches the log, the exception or the + * page — and the refusal is not reported as one. + */ + public function testAForgedStateOnAnErrorCallbackTellsTheUserNothingTheAttackerWrote(): void + { + $request = $this->refusedCallback('state=does-not-match&error=access_denied&error_description=Call+0800+SCAM'); + + try { + $this->kernel->handle($request, catch: false); + $this->fail('A forged callback should not be handled silently.'); + } catch (AuthenticationFailedException $exception) { + $this->assertTheAuthenticatorHandledTheCallback($request); + $this->assertNotInstanceOf(ProviderErrorException::class, $exception); + $this->assertStringContainsString('Invalid state', $exception->getMessage()); + $this->assertStringNotContainsString('access_denied', $exception->getMessage()); + $this->assertStringNotContainsString('SCAM', $exception->getMessage()); + } + + // Still terminal, and still spent — on a fresh request, since the one above + // consumed its session. + $forged = $this->refusedCallback('state=does-not-match&error=access_denied&error_description=Call+0800+SCAM'); + $session = $forged->getSession(); + $response = $this->kernel->handle($forged, catch: true); + + $this->assertNull($response->headers->get('Location')); + $this->assertSame(Response::HTTP_INTERNAL_SERVER_ERROR, $response->getStatusCode()); + $this->assertFalse($session->has('oauth2state')); + } + + /** + * Issue #63 restated in the new query shape: widening `supports()` must not put + * back the ability to turn any page under the firewall into a failed login. + */ + public function testAnErrorCallbackOnAStrayPathIsLeftToTheFirewall(): void + { + $request = Request::create('/protected?state=forged&error=access_denied'); + $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' + ); + } + + /** + * `state` on its own is still not a callback. The callback path is outside + * `access_control`, so the route answers it and the authenticator stays out. + */ + public function testACallbackWithNeitherCodeNorErrorIsNotACallback(): void + { + $request = Request::create('/callback_uri?state=the-real-state'); + $request->setSession($this->startedLogin()); + + $response = $this->kernel->handle($request, catch: true); + + $this->assertSame(Response::HTTP_OK, $response->getStatusCode()); + $this->assertNull( + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'validateClaims() ran for a request carrying neither a code nor an error' + ); + } +} From 9691244902f3920d900f17b339451aa0ec79d8e1 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 14:28:42 +0200 Subject: [PATCH 3/8] feat: send a PKCE challenge, and stop holding providers between requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires RFC 7636 S256 PKCE through the login flow on itk-dev/openid-connect 5.1. The login route generates a verifier, keeps it in the session, and sends only its challenge; the authenticator redeems the code with it. On by default — RFC 6749 §3.1 makes an authorization server ignore parameters it does not know — with `pkce: false` per provider for one that rejects them instead. The verifier joins the one-time session values consumed on every callback, so it cannot be redeemed against a code it does not belong to. getProvider() now builds a fresh provider each call. AbstractProvider assigns $this->state on every getAuthorizationUrl(), so a memoized instance carried one request's state into the next — nothing reads it today, but that stops being true the moment a process outlives a request. The HTTP client is cached per provider instead, so the connection pool still survives; it is built with league's own option filter, which keeps `verify` reachable only alongside a proxy. --- CHANGELOG.md | 18 ++- README.md | 27 +++++ UPGRADE-6.1.md | 45 +++++++- composer.json | 2 +- src/Controller/LoginController.php | 22 +++- src/DependencyInjection/Configuration.php | 9 ++ .../OpenIdConfigurationProviderManager.php | 93 +++++++++++++-- src/Security/OpenIdLoginAuthenticator.php | 10 +- tests/Controller/LoginControllerTest.php | 91 ++++++++++++++- .../DependencyInjection/ConfigurationTest.php | 17 +++ ...OpenIdConfigurationProviderManagerTest.php | 109 +++++++++++++++++- .../Security/OpenIdLoginAuthenticatorTest.php | 69 ++++++++++- 12 files changed, 488 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d58bad..cccd3f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- PKCE (RFC 7636, S256), on by default. The login route generates a verifier, keeps it + in the session under `oauth2pkce_verifier`, and sends the challenge; the + authenticator redeems the code with it. Turn it off per provider with `pkce: false` + for an identity provider that rejects the parameters rather than ignoring them. +- `OpenIdConfigurationProviderManager::isPkceEnabled()`. - `ProviderErrorException`, thrown when the identity provider refuses the authorization request (RFC 6749 §4.1.2.1). It extends `AuthenticationFailedException`, so existing `catch` blocks keep matching, and carries `getError()`, `getErrorDescription()` and @@ -24,14 +29,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Requires `itk-dev/openid-connect` `^5.1`, for its PKCE support. That release also + enforces `allowHttp` on every discovered endpoint, requires `exp` and `iat` on the + ID token, and changes the JWKS cache key — see its changelog before upgrading. +- `getProvider()` returns a fresh provider on every call instead of a memoized one. + `league/oauth2-client` writes the authorization request's `state` onto the provider, + so a held instance carried one request's state into the next — harmless today, but + not under a worker runtime where the process outlives the request. The HTTP client + is now what is kept per provider, so the connection pool still survives. - A refused login is answered with the status that matches its cause: 403 where the user or a policy declined, 503 where the provider reports its own trouble, 500 otherwise. Other callback failures are unchanged and still surface as 500. - `error` and `error_description` are sanitized before they are logged or held — control characters collapsed, invalid UTF-8 dropped, capped at 200 characters — and neither is read at all until the callback's state matches. -- `oauth2provider`, `oauth2state` and `oauth2nonce` are consumed on every callback, - including one carrying a provider error. +- Every one-time session value is consumed on every callback, including one carrying a + provider error: `oauth2provider`, `oauth2state`, `oauth2nonce` and + `oauth2pkce_verifier`. - The stored state is compared with `hash_equals()`, and an empty or missing stored state is rejected explicitly rather than by comparison. - A callback naming a provider that is not configured is now reported as an invalid diff --git a/README.md b/README.md index 96afa7b..76e0b13 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,9 @@ itkdev_openid_connect: # Optional: Cache duration (seconds) for the OIDC discovery document and JWKS # Defaults to 86400 (24 hours) cache_duration: '%env(int:ADMIN_OIDC_CACHE_DURATION)%' + # Optional: Send a PKCE challenge (RFC 7636, S256) with the authorization + # request. Defaults to true. See "PKCE" below. + pkce: true # Optional: Allow (non-secure) http requests (used for mocking a IdP). NOT RECOMMENDED FOR PRODUCTION. # Defaults to false allow_http: '%env(bool:ADMIN_OIDC_ALLOW_HTTP)%' @@ -884,6 +887,30 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator } ``` +### PKCE + +The bundle sends a PKCE challenge (RFC 7636, S256) with every authorization request. +The login route generates a verifier, keeps it in the session, and sends only its +SHA-256 challenge; the authenticator redeems the authorization code with the verifier. +An intercepted code is then useless to whoever intercepted it, because they do not +have the verifier. + +It is on by default and needs no configuration. RFC 6749 §3.1 requires an +authorization server to ignore parameters it does not recognise, so an identity +provider that has never heard of PKCE behaves exactly as it did before. Turn it off +only for one that rejects the parameters outright: + +```yaml +openid_providers: + legacy: + options: + pkce: false +``` + +The verifier lives in the session alongside the state and the nonce, and is consumed +on every callback — success, failure or refusal — so it can never be redeemed against +a code it does not belong to. + ### When the identity provider refuses A provider that will not issue a code redirects back to the callback with an `error` diff --git a/UPGRADE-6.1.md b/UPGRADE-6.1.md index 10d2e68..cf46e95 100644 --- a/UPGRADE-6.1.md +++ b/UPGRADE-6.1.md @@ -55,5 +55,46 @@ refusal arrives as a plain 500 with the reason only in the message. This was alw the documented shape; 6.1 is the first release where dropping the cause costs you something. -See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning, and -[CHANGELOG.md](CHANGELOG.md) for the rest of the release. +## PKCE is on by default + +Every authorization request now carries a PKCE challenge (RFC 7636, S256). RFC 6749 +§3.1 requires an authorization server to ignore parameters it does not recognise, so a +provider that does not support PKCE behaves as it did before, and one that does gets +the extra protection with no configuration from you. + +If you have an identity provider that rejects unknown parameters rather than ignoring +them, turn it off for that provider: + +```yaml +openid_providers: + legacy: + options: + pkce: false +``` + +The verifier is kept in the session under `oauth2pkce_verifier`. If your application +clears or rewrites the session between the login redirect and the callback, it must +preserve that key alongside `oauth2state` and `oauth2nonce`. + +## The library requires 5.1 + +`itk-dev/openid-connect` `^5.1` comes with this release. Three of its changes affect +running deployments: an identity provider announcing plain-http endpoints now needs +`allow_http`, an ID token without `exp` or `iat` is rejected, and the JWKS cache key +changed so 5.0's entries are not reused. Read its changelog before deploying. + +## `getProvider()` no longer returns the same instance + +`OpenIdConfigurationProviderManager::getProvider()` builds a fresh provider on every +call. `league/oauth2-client` records the authorization request's `state` on the +provider, so a memoized instance carried one request's state into the next — which +matters once a process outlives a request, as under a FrankenPHP worker. The HTTP +client is kept per provider instead, so connections to the identity provider are still +reused. + +Nothing to do unless you held the returned provider and relied on getting the same +object back. + +See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning behind +the error-callback handling, and [CHANGELOG.md](CHANGELOG.md) for the rest of the +release. diff --git a/composer.json b/composer.json index f925474..621365b 100644 --- a/composer.json +++ b/composer.json @@ -18,7 +18,7 @@ "ext-json": "*", "ext-openssl": "*", "doctrine/orm": "^2.8 || ^3.0", - "itk-dev/openid-connect": "^5.0", + "itk-dev/openid-connect": "^5.1", "psr/log": "^3.0", "symfony/cache": "^6.4 || ^7.0 || ^8.0", "symfony/clock": "^6.4 || ^7.0 || ^8.0", diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 6cac73f..ecd7a05 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -58,6 +58,11 @@ public function login(Request $request, SessionInterface $session, string $provi $nonce = $provider->generateNonce(); $state = $provider->generateState(); + // The verifier is kept, the challenge is sent. Only the holder of the + // verifier can redeem the code, which is what makes a code intercepted in + // transit useless to whoever intercepted it (RFC 7636). + $pkceVerifier = $this->providerManager->isPkceEnabled($providerKey) ? $provider->generatePkceVerifier() : null; + $this->rememberNamedTargetPath($request, $session); // Save to session @@ -65,13 +70,26 @@ public function login(Request $request, SessionInterface $session, string $provi $session->set('oauth2state', $state); $session->set('oauth2nonce', $nonce); + // Written on every login, null included. The key names the verifier for this + // login and nothing else: leaving an abandoned login's verifier in place + // would offer it up against a code it does not belong to. + $session->set('oauth2pkce_verifier', $pkceVerifier); + try { - $authUrl = $provider->getAuthorizationUrl([ + $options = [ 'state' => $state, 'nonce' => $nonce, 'response_type' => 'code', 'scope' => 'openid email profile', - ]); + ]; + + if (null !== $pkceVerifier) { + // Passing a challenge is what turns PKCE on in the library; it adds + // code_challenge_method=S256 alongside. + $options['code_challenge'] = $provider->getPkceChallenge($pkceVerifier); + } + + $authUrl = $provider->getAuthorizationUrl($options); } catch (OpenIdConnectExceptionInterface $e) { // Building the authorization URL fetches the IdP's discovery // document. Surface upstream/transport/cache failures as 503 with diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index bd01dc1..d408470 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -163,6 +163,15 @@ public function getConfigTreeBuilder(): TreeBuilder ->info('Cache duration in seconds for the OIDC discovery document and JWKS (default: 86400 — 24 hours)') ->defaultValue(86400) ->end() + ->booleanNode('pkce') + // On by default: RFC 6749 §3.1 requires an authorization + // server to ignore parameters it does not recognise, so a + // challenge costs nothing at an identity provider that does + // not support PKCE. Turn it off for one that rejects + // unknown parameters outright. + ->info('Send a PKCE challenge (RFC 7636, S256) with the authorization request') + ->defaultTrue() + ->end() ->scalarNode('redirect_uri') ->info('Redirect URI registered at identity provider') ->cannotBeEmpty() diff --git a/src/Security/OpenIdConfigurationProviderManager.php b/src/Security/OpenIdConfigurationProviderManager.php index d3941ba..bb4b233 100644 --- a/src/Security/OpenIdConfigurationProviderManager.php +++ b/src/Security/OpenIdConfigurationProviderManager.php @@ -2,6 +2,7 @@ namespace ItkDev\OpenIdConnectBundle\Security; +use GuzzleHttp\Client as GuzzleClient; use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; use ItkDev\OpenIdConnect\Security\OpenIdConfigurationProvider; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; @@ -11,10 +12,29 @@ class OpenIdConfigurationProviderManager { - /** @var array */ - private array $providers = []; + /** + * One HTTP client per provider, for the life of the process. + * + * The client owns the connection pool, so sharing it across logins means a token + * exchange reuses an open connection to the identity provider rather than + * renegotiating TLS. Its options come from configuration and never change, and it + * holds nothing belonging to a request, so it is safe to share however long the + * process lives. + * + * @var array + */ + private array $httpClients = []; - /** @var array> */ + /** + * Callback paths, keyed by the routing context's base URL. + * + * Safe to hold across requests: the values are a pure function of configuration + * and that base URL, so two requests sharing a key derive identical paths. The + * key set is bounded by the number of base URLs an application answers on, which + * is one in every deployment shape the bundle documents. + * + * @var array> + */ private array $redirectUriPaths = []; /** @@ -30,6 +50,7 @@ class OpenIdConfigurationProviderManager * callback_path?: string, * leeway?: int, * cache_duration?: int, + * pkce?: bool, * allow_http?: bool, * http_client_options?: array{ * timeout?: float, @@ -55,6 +76,21 @@ public function getProviderKeys(): array return array_keys($this->config['providers']); } + /** + * Whether this provider's authorization request carries a PKCE challenge. + * + * Read from configuration, like the callback paths: the caller needs the answer + * before it has any use for a provider. + * + * An unconfigured key answers true. The config tree makes that unreachable, since + * `pkce` defaults to true there, and matching it here keeps one rule in two places + * from drifting apart. + */ + public function isPkceEnabled(string $providerKey): bool + { + return $this->config['providers'][$providerKey]['pkce'] ?? true; + } + /** * The request path each provider's callback arrives on, keyed by provider. * @@ -166,11 +202,19 @@ private function normalizePath(string $path): string /** * Get a provider by key. * + * A fresh instance every call. A provider belongs to one request: + * `league/oauth2-client` assigns `$this->state` on every `getAuthorizationUrl()` + * call, so an instance shared between requests holds the previous one's state. + * + * Constructing one is cheap. Discovery and JWKS are lazy and cached in the PSR-6 + * pool, and the costly collaborator — the HTTP client, with its connection pool — + * comes from `$httpClients`. + * * @throws OpenIdConnectExceptionInterface */ public function getProvider(string $key): OpenIdConfigurationProvider { - if (!isset($this->providers[$key]) && isset($this->config['providers'][$key])) { + if (isset($this->config['providers'][$key])) { $options = $this->config['providers'][$key]; $providerOptions = [ 'openIDConnectMetadataUrl' => $options['metadata_url'], @@ -204,13 +248,46 @@ public function getProvider(string $key): OpenIdConfigurationProvider $providerOptions += $options['http_client_options']; } - $this->providers[$key] = new OpenIdConfigurationProvider($providerOptions); + return new OpenIdConfigurationProvider( + $providerOptions, + ['httpClient' => $this->httpClient($key, $providerOptions)], + ); } - if (isset($this->providers[$key])) { - return $this->providers[$key]; + throw new InvalidProviderException(sprintf('Invalid provider: %s', $key)); + } + + /** + * The HTTP client for a provider, built once and shared by its providers. + * + * `league/oauth2-client` builds a client per provider instance, which would mean + * a new connection pool per request; building it here gives every provider for a + * key the same one. + * + * The option filter mirrors `AbstractProvider::getAllowedClientOptions()`, so the + * client is configured exactly as league configures its own: `timeout` and + * `proxy` always, `verify` only alongside a proxy — league's rule that TLS + * verification may be relaxed for a proxy and nowhere else. + * + * @param array $providerOptions + */ + private function httpClient(string $key, array $providerOptions): GuzzleClient + { + if (isset($this->httpClients[$key])) { + return $this->httpClients[$key]; } - throw new InvalidProviderException(sprintf('Invalid provider: %s', $key)); + $allowed = ['timeout', 'proxy']; + + // `proxy` is a scalar node, so it arrives as a string or not at all; an empty + // one names no proxy. This matches league's own `empty()` test for every + // value the config tree can produce. + if (isset($providerOptions['proxy']) && '' !== $providerOptions['proxy']) { + $allowed[] = 'verify'; + } + + return $this->httpClients[$key] = new GuzzleClient( + array_intersect_key($providerOptions, array_flip($allowed)) + ); } } diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index 0f22488..d3411d6 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -202,11 +202,13 @@ protected function validateClaims(Request $request): array // Every one-time value is spent here, before anything below can throw. A // callback is used up whether it succeeds, fails validation, or carries the // provider's refusal, and a value left behind is one a later request can - // replay. + // replay. The PKCE verifier belongs to that set: a verifier surviving a + // failed callback could be redeemed against a later code. $providerKey = $session->remove('oauth2provider'); $providerKey = is_string($providerKey) ? $providerKey : ''; $oauth2state = $session->remove('oauth2state'); $oauth2nonce = $session->remove('oauth2nonce'); + $pkceVerifier = $session->remove('oauth2pkce_verifier'); // The session entry is removed above, so carry the provider key on the // request for anything downstream that needs to attribute this login — @@ -280,7 +282,11 @@ protected function validateClaims(Request $request): array throw new ValidationException('Missing or invalid code'); } - $idToken = $provider->getIdToken($code); + // Null where no challenge was sent: the provider has PKCE turned off, or + // the login began under a session that never stored a verifier. The token + // request then carries no code_verifier, which is what an identity + // provider that received no challenge expects. + $idToken = $provider->getIdToken($code, is_string($pkceVerifier) ? $pkceVerifier : null); $claims = $provider->validateIdToken($idToken, $oauth2nonce); // Authentication successful } catch (OpenIdConnectExceptionInterface $exception) { diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index 10c6641..2f475d4 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -68,7 +68,7 @@ public function testLogin(): void $request = new Request(query: ['provider' => 'test']); $mockSession = $this->createMock(SessionInterface::class); - $matcher = $this->exactly(3); + $matcher = $this->exactly(4); $mockSession ->expects($matcher) ->method('set')->willReturnCallback(function (...$parameters) use ($matcher) { @@ -84,6 +84,12 @@ public function testLogin(): void $this->assertEquals('oauth2nonce', $parameters[0]); $this->assertEquals('1234', $parameters[1]); } + if (4 === $matcher->numberOfInvocations()) { + // Written even with PKCE off, so a verifier left by an earlier + // login cannot be redeemed against this one's code. + $this->assertEquals('oauth2pkce_verifier', $parameters[0]); + $this->assertNull($parameters[1]); + } }); $response = $controller->login($request, $mockSession, 'test'); @@ -91,6 +97,83 @@ public function testLogin(): void $this->assertSame([], $this->logger->records, 'A successful login must not log a failure.'); } + public function testPkceSendsAChallengeAndKeepsTheVerifier(): void + { + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider->method('generateNonce')->willReturn('1234'); + $mockProvider->method('generateState')->willReturn('abcd'); + $mockProvider + ->expects($this->once()) + ->method('generatePkceVerifier') + ->willReturn('the-verifier'); + $mockProvider + ->expects($this->once()) + ->method('getPkceChallenge') + ->with('the-verifier') + ->willReturn('the-challenge'); + $mockProvider + ->expects($this->once()) + ->method('getAuthorizationUrl') + ->with([ + 'state' => 'abcd', + 'nonce' => '1234', + 'response_type' => 'code', + 'scope' => 'openid email profile', + // The library adds code_challenge_method=S256 alongside this. + 'code_challenge' => 'the-challenge', + ]) + ->willReturn('https://provider.example.org/authorize'); + + $controller = $this->createController($mockProvider, pkce: true); + $session = new Session(new MockArraySessionStorage()); + + $controller->login(new Request(), $session, 'test'); + + // The verifier is kept, never sent. Only the challenge goes over the wire. + $this->assertSame('the-verifier', $session->get('oauth2pkce_verifier')); + } + + public function testPkceCanBeTurnedOffForAProviderThatRejectsIt(): void + { + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider->method('generateNonce')->willReturn('1234'); + $mockProvider->method('generateState')->willReturn('abcd'); + $mockProvider->expects($this->never())->method('generatePkceVerifier'); + $mockProvider->expects($this->never())->method('getPkceChallenge'); + $mockProvider + ->expects($this->once()) + ->method('getAuthorizationUrl') + ->with($this->logicalNot($this->arrayHasKey('code_challenge'))) + ->willReturn('https://provider.example.org/authorize'); + + $controller = $this->createController($mockProvider, pkce: false); + $session = new Session(new MockArraySessionStorage()); + + $controller->login(new Request(), $session, 'test'); + + $this->assertNull($session->get('oauth2pkce_verifier')); + } + + /** + * A verifier from an abandoned login must not be redeemable against the code this + * one is about to receive, so the key is written on every login rather than only + * when PKCE is on. + */ + public function testAStaleVerifierIsOverwrittenWhenPkceIsOff(): void + { + $stubProvider = $this->createStub(OpenIdConfigurationProvider::class); + $stubProvider->method('generateNonce')->willReturn('1234'); + $stubProvider->method('generateState')->willReturn('abcd'); + $stubProvider->method('getAuthorizationUrl')->willReturn('https://provider.example.org/authorize'); + + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2pkce_verifier', 'left-over-from-an-earlier-login'); + + $this->createController($stubProvider, pkce: false)->login(new Request(), $session, 'test'); + + $this->assertNull($session->get('oauth2pkce_verifier')); + } + public function testUnknownProviderKeyMapsTo404(): void { $cause = new InvalidProviderException('Invalid provider: bogus'); @@ -256,7 +339,7 @@ public function testUnknownProviderIsRefusedBeforeTheExpiryCheck(): void $this->fail('Expected NotFoundHttpException'); } - private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null): LoginController + private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null, bool $pkce = false): LoginController { $mockProviderManager = $this->createMock(OpenIdConfigurationProviderManager::class); $mockProviderManager @@ -264,6 +347,10 @@ private function createController(OpenIdConfigurationProvider $provider, ?Client ->method('getProvider') ->with('test') ->willReturn($provider); + $mockProviderManager + ->method('isPkceEnabled') + ->with('test') + ->willReturn($pkce); return new LoginController($mockProviderManager, $this->logger, $expiryChecker ?? $this->createExpiryChecker()); } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 031d49b..0694d96 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -526,4 +526,21 @@ public function testCallbackPathAloneSatisfiesTheRequirement(): void $this->assertSame('/auth/callback', $config['openid_providers']['provider1']['options']['callback_path']); } + + public function testPkceDefaultsToOn(): void + { + $config = $this->processor->processConfiguration($this->configuration, [$this->getMinimalConfig()]); + + $this->assertTrue($config['openid_providers']['provider1']['options']['pkce']); + } + + public function testPkceCanBeTurnedOff(): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['pkce'] = false; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertFalse($config['openid_providers']['provider1']['options']['pkce']); + } } diff --git a/tests/Security/OpenIdConfigurationProviderManagerTest.php b/tests/Security/OpenIdConfigurationProviderManagerTest.php index 7660c83..09ab224 100644 --- a/tests/Security/OpenIdConfigurationProviderManagerTest.php +++ b/tests/Security/OpenIdConfigurationProviderManagerTest.php @@ -221,7 +221,11 @@ public function testGetProviderWithoutHttpClientOptionsLeavesGuzzleDefaults(): v $this->assertNull($this->getGuzzleConfig($httpClient, 'timeout')); } - public function testGetProviderCachesInstance(): void + /** + * A provider belongs to one request: `league/oauth2-client` records the + * authorization request's `state` on it, so sharing an instance shares that state. + */ + public function testGetProviderReturnsAFreshInstance(): void { $manager = $this->createManager([ 'test' => $this->getBaseProviderConfig() + [ @@ -229,10 +233,107 @@ public function testGetProviderCachesInstance(): void ], ]); - $provider1 = $manager->getProvider('test'); - $provider2 = $manager->getProvider('test'); + $this->assertNotSame($manager->getProvider('test'), $manager->getProvider('test')); + } - $this->assertSame($provider1, $provider2); + /** + * Nothing one request puts on a provider reaches the next. Asserted through the + * state league records, so the test tracks the library's observable behaviour and + * not the shape of its internals. + */ + public function testNoRequestStateSurvivesOnTheNextProvider(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback', + ], + ]); + + $first = $manager->getProvider('test'); + $firstState = $first->generateState(); + + $this->assertSame($firstState, $first->getState(), 'The library records state on the provider; if it stops, this test needs rewriting'); + $this->assertNotSame($firstState, $manager->getProvider('test')->getState()); + } + + /** + * The client owns the connection pool: sharing it is what lets a token exchange + * reuse an open connection to the identity provider. + */ + public function testTheHttpClientIsReusedAcrossProviders(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback', + ], + ]); + + $this->assertSame( + $manager->getProvider('test')->getHttpClient(), + $manager->getProvider('test')->getHttpClient() + ); + } + + /** + * league's rule, kept: TLS verification may be turned off for a proxy and + * nowhere else. Without a proxy, `verify` is not forwarded at all, so Guzzle's + * default — verify — stands. + */ + public function testVerifyIsNotForwardedWithoutAProxy(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback', + 'http_client_options' => [ + 'timeout' => 1.5, + 'verify' => false, + ], + ], + ]); + + $httpClient = $manager->getProvider('test')->getHttpClient(); + $this->assertInstanceOf(GuzzleClient::class, $httpClient); + + $this->assertSame(1.5, $this->getGuzzleConfig($httpClient, 'timeout')); + $this->assertNotFalse($this->getGuzzleConfig($httpClient, 'verify'), 'Verification must not be disabled without a proxy'); + } + + /** + * Only the three options league forwards reach the client. The rest of a + * provider's configuration — its client secret above all — has no business in + * Guzzle's request options. + */ + public function testProviderCredentialsNeverReachTheHttpClient(): void + { + $manager = $this->createManager([ + 'test' => $this->getBaseProviderConfig() + [ + 'redirect_uri' => 'https://app.example.org/callback', + 'http_client_options' => ['timeout' => 1.5], + ], + ]); + + $httpClient = $manager->getProvider('test')->getHttpClient(); + $this->assertInstanceOf(GuzzleClient::class, $httpClient); + + $this->assertNull($this->getGuzzleConfig($httpClient, 'clientSecret')); + $this->assertNull($this->getGuzzleConfig($httpClient, 'clientId')); + $this->assertNull($this->getGuzzleConfig($httpClient, 'cacheItemPool')); + } + + /** + * Each provider keeps its own, since `http_client_options` is per provider. + */ + public function testEachProviderGetsItsOwnHttpClient(): void + { + $manager = $this->createManager([ + 'one' => $this->getBaseProviderConfig() + ['redirect_uri' => 'https://app.example.org/one'], + 'two' => $this->getBaseProviderConfig() + ['redirect_uri' => 'https://app.example.org/two'], + ]); + + $this->assertNotSame( + $manager->getProvider('one')->getHttpClient(), + $manager->getProvider('two')->getHttpClient() + ); } /** diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 7eece0e..5fe8982 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -671,6 +671,7 @@ public function testEveryOneTimeSessionValueIsConsumed(string $state, array $ext $this->assertFalse($session->has('oauth2provider')); $this->assertFalse($session->has('oauth2state')); $this->assertFalse($session->has('oauth2nonce')); + $this->assertFalse($session->has('oauth2pkce_verifier'), 'A surviving verifier could be redeemed against a later code'); } /** @@ -820,13 +821,78 @@ public function testTheProviderErrorLeavesOnAuthenticationFailureUnwrapped(): vo $this->fail('Expected ProviderErrorException'); } - private function setSessionOnRequest(Request $request, ?string $nonce = 'test_nonce'): void + public function testTheStoredVerifierIsSentWithTheTokenRequest(): void + { + $claims = new \stdClass(); + $claims->email = 'test@example.org'; + + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider + ->expects($this->once()) + ->method('getIdToken') + ->with('test_code', 'test_verifier') + ->willReturn('an.id.token'); + $mockProvider->method('validateIdToken')->willReturn($claims); + $this->stubProviderManager->method('getProvider')->willReturn($mockProvider); + + $request = new Request(query: ['state' => 'test_state', 'code' => 'test_code']); + $this->setSessionOnRequest($request, pkceVerifier: 'test_verifier'); + + $this->authenticator->authenticate($request); + } + + /** + * @return iterable + */ + public static function absentVerifierProvider(): iterable + { + // PKCE off for this provider, or a callback from a login that began before + // the verifier was ever stored. + yield 'never stored' => [null]; + // Nothing writes a non-string, but the session is shared with the application. + yield 'not a string' => [['test_verifier']]; + } + + /** + * Without a verifier the token request goes out without a `code_verifier`, which + * is what the identity provider expects when it was sent no challenge. + */ + #[DataProvider('absentVerifierProvider')] + public function testNoVerifierMeansNoCodeVerifier(mixed $stored): void + { + $claims = new \stdClass(); + $claims->email = 'test@example.org'; + + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider + ->expects($this->once()) + ->method('getIdToken') + ->with('test_code', null) + ->willReturn('an.id.token'); + $mockProvider->method('validateIdToken')->willReturn($claims); + $this->stubProviderManager->method('getProvider')->willReturn($mockProvider); + + $request = new Request(query: ['state' => 'test_state', 'code' => 'test_code']); + $stubSession = $this->createStub(SessionInterface::class); + $stubSession->method('remove')->willReturnMap([ + ['oauth2provider', 'test_provider_1'], + ['oauth2state', 'test_state'], + ['oauth2nonce', 'test_nonce'], + ['oauth2pkce_verifier', $stored], + ]); + $request->setSession($stubSession); + + $this->authenticator->authenticate($request); + } + + private function setSessionOnRequest(Request $request, ?string $nonce = 'test_nonce', ?string $pkceVerifier = null): void { $stubSession = $this->createStub(SessionInterface::class); $map = [ ['oauth2provider', 'test_provider_1'], ['oauth2state', 'test_state'], ['oauth2nonce', $nonce], + ['oauth2pkce_verifier', $pkceVerifier], ]; $stubSession->method('remove')->willReturnMap($map); @@ -843,6 +909,7 @@ private function realSessionOnRequest(Request $request): Session $session->set('oauth2provider', 'test_provider_1'); $session->set('oauth2state', 'test_state'); $session->set('oauth2nonce', 'test_nonce'); + $session->set('oauth2pkce_verifier', 'test_verifier'); $request->setSession($session); return $session; From f461b761f5211200a7cadc05ac2737db766ef3e2 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 14:43:13 +0200 Subject: [PATCH 4/8] feat: configurable scopes, config bounds, and two clearer failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scopes are a per-provider option, defaulting to the openid/email/profile the bundle has always asked for. A list or a space-separated string, so the value can come from an environment variable. `openid` is required: OIDC Core §3.1.2.1 defines an authentication request as one that asks for it, and without it the provider returns no ID token. `leeway` and `cache_duration` reject a negative value at compile time. A negative leeway used to fail at the first login that needed it; a negative cache duration passed through unnoticed. The authenticator implements InteractiveAuthenticatorInterface, so a completed login dispatches security.interactive_login and remember-me treats the token as one a user asked for. A firewall declared `stateless: true` now throws StatelessFirewallException naming the setting, rather than surfacing Symfony's SessionNotFoundException as an unexplained 500. --- CHANGELOG.md | 15 ++- README.md | 28 ++++ UPGRADE-6.1.md | 15 +++ src/Controller/LoginController.php | 3 +- src/DependencyInjection/Configuration.php | 37 ++++++ src/Exception/StatelessFirewallException.php | 19 +++ .../OpenIdConfigurationProviderManager.php | 17 +++ src/Security/OpenIdLoginAuthenticator.php | 23 +++- tests/Controller/LoginControllerTest.php | 27 +++- .../DependencyInjection/ConfigurationTest.php | 120 ++++++++++++++++++ tests/Exception/ExceptionHierarchyTest.php | 4 + .../Security/OpenIdLoginAuthenticatorTest.php | 32 +++++ 12 files changed, 335 insertions(+), 5 deletions(-) create mode 100644 src/Exception/StatelessFirewallException.php diff --git a/CHANGELOG.md b/CHANGELOG.md index cccd3f0..32515ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 in the session under `oauth2pkce_verifier`, and sends the challenge; the authenticator redeems the code with it. Turn it off per provider with `pkce: false` for an identity provider that rejects the parameters rather than ignoring them. -- `OpenIdConfigurationProviderManager::isPkceEnabled()`. +- `OpenIdConfigurationProviderManager::isPkceEnabled()` and `getScopes()`. +- Per-provider `scopes`, defaulting to `openid`, `email` and `profile` — the scopes the + bundle has always requested. Accepts a list or a space-separated string, so the value + can come from an environment variable. A list without `openid` is rejected at compile + time. +- `StatelessFirewallException`, naming the misconfiguration when the authenticator is + put on a firewall declared `stateless: true`. Previously Symfony's + `SessionNotFoundException` surfaced as an unexplained 500. - `ProviderErrorException`, thrown when the identity provider refuses the authorization request (RFC 6749 §4.1.2.1). It extends `AuthenticationFailedException`, so existing `catch` blocks keep matching, and carries `getError()`, `getErrorDescription()` and @@ -29,6 +36,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `OpenIdLoginAuthenticator` implements `InteractiveAuthenticatorInterface`, so a + completed login dispatches `security.interactive_login` and remember-me treats the + token as one a user asked for. +- `leeway` and `cache_duration` reject a negative value while the container compiles. + A negative leeway used to fail at the first login that needed it, and a negative + cache duration passed through to the cache unnoticed. - Requires `itk-dev/openid-connect` `^5.1`, for its PKCE support. That release also enforces `allowHttp` on every discovered endpoint, requires `exp` and `iat` on the ID token, and changes the JWKS cache key — see its changelog before upgrading. diff --git a/README.md b/README.md index 76e0b13..12927ff 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,10 @@ itkdev_openid_connect: # Optional: Send a PKCE challenge (RFC 7636, S256) with the authorization # request. Defaults to true. See "PKCE" below. pkce: true + # Optional: Scopes to request. Defaults to openid, email, profile. + # Must include openid. A space-separated string is accepted too, + # so the value can come from an environment variable. + scopes: ['openid', 'email', 'profile'] # Optional: Allow (non-secure) http requests (used for mocking a IdP). NOT RECOMMENDED FOR PRODUCTION. # Defaults to false allow_http: '%env(bool:ADMIN_OIDC_ALLOW_HTTP)%' @@ -887,6 +891,30 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator } ``` +### Scopes + +The authorization request asks for `openid`, `email` and `profile`. Set `scopes` per +provider to ask for something else: + +```yaml +openid_providers: + admin: + options: + scopes: ['openid', 'profile', 'groups'] +``` + +`openid` must be among them — OpenID Connect Core 1.0 §3.1.2.1 defines an +authentication request as one that asks for it, and without it the provider returns an +OAuth2 grant with no ID token, which is the only thing this bundle can validate. A list +missing it fails at compile time. + +A space-separated string is accepted and split, since an environment variable can only +carry a scalar: + +```yaml + scopes: '%env(ADMIN_OIDC_SCOPES)%' # ADMIN_OIDC_SCOPES=openid profile groups +``` + ### PKCE The bundle sends a PKCE challenge (RFC 7636, S256) with every authorization request. diff --git a/UPGRADE-6.1.md b/UPGRADE-6.1.md index cf46e95..5168b1c 100644 --- a/UPGRADE-6.1.md +++ b/UPGRADE-6.1.md @@ -76,6 +76,16 @@ The verifier is kept in the session under `oauth2pkce_verifier`. If your applica clears or rewrites the session between the login redirect and the callback, it must preserve that key alongside `oauth2state` and `oauth2nonce`. +## A stateless firewall is now named as such + +Putting the OpenID Connect authenticator on a firewall declared `stateless: true` +throws `StatelessFirewallException`, naming the setting to remove. It used to surface +as Symfony's `SessionNotFoundException` and an unexplained 500. + +The flow spans two requests and the session is where the state, nonce and PKCE +verifier wait, so such a firewall could never complete a login. Nothing that worked +before stops working. + ## The library requires 5.1 `itk-dev/openid-connect` `^5.1` comes with this release. Three of its changes affect @@ -95,6 +105,11 @@ reused. Nothing to do unless you held the returned provider and relied on getting the same object back. +## Optional: scopes are configurable + +The authorization request still asks for `openid`, `email` and `profile`. Set `scopes` +per provider to change that; `openid` must remain among them. + See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning behind the error-callback handling, and [CHANGELOG.md](CHANGELOG.md) for the rest of the release. diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index ecd7a05..d7eec6f 100644 --- a/src/Controller/LoginController.php +++ b/src/Controller/LoginController.php @@ -80,7 +80,8 @@ public function login(Request $request, SessionInterface $session, string $provi 'state' => $state, 'nonce' => $nonce, 'response_type' => 'code', - 'scope' => 'openid email profile', + // Space-delimited, as RFC 6749 §3.3 defines the parameter. + 'scope' => implode(' ', $this->providerManager->getScopes($providerKey)), ]; if (null !== $pkceVerifier) { diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index d408470..f870627 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -158,10 +158,47 @@ public function getConfigTreeBuilder(): TreeBuilder ->integerNode('leeway') ->info('Leeway in seconds to account for clock skew between server and provider') ->defaultValue(10) + // A window, so zero means no window. A negative one is + // rejected here rather than at the first login it breaks. + ->min(0) ->end() ->integerNode('cache_duration') ->info('Cache duration in seconds for the OIDC discovery document and JWKS (default: 86400 — 24 hours)') ->defaultValue(86400) + // Zero fetches the discovery document every time, which is + // wasteful but coherent. A negative lifetime is not. + ->min(0) + ->end() + ->arrayNode('scopes') + ->info('Scopes requested from the identity provider (default: openid, email, profile)') + ->scalarPrototype()->end() + ->defaultValue(['openid', 'email', 'profile']) + ->requiresAtLeastOneElement() + // Accept a space-separated string so the list can come from + // an environment variable, which can only carry a scalar. + ->beforeNormalization() + ->ifString() + ->then(static function (string $scopes): array { + // NO_EMPTY drops the empty strings a + // leading or trailing space produces, + // so surrounding whitespace needs no + // separate trim. + $split = preg_split('/\s+/', $scopes, -1, PREG_SPLIT_NO_EMPTY); + + // preg_split only fails on a malformed + // pattern; this one is a literal. An + // empty list is caught below. + return false === $split ? [] : $split; + }) + ->end() + ->validate() + // OpenID Connect Core 1.0 §3.1.2.1: an authentication + // request is one that asks for `openid`. Without it the + // provider answers with a plain OAuth2 grant and no ID + // token, and every check this bundle makes needs one. + ->ifTrue(static fn (array $scopes): bool => !in_array('openid', $scopes, true)) + ->thenInvalid('scopes must include openid: without it the provider returns no ID token.') + ->end() ->end() ->booleanNode('pkce') // On by default: RFC 6749 §3.1 requires an authorization diff --git a/src/Exception/StatelessFirewallException.php b/src/Exception/StatelessFirewallException.php new file mode 100644 index 0000000..b21b420 --- /dev/null +++ b/src/Exception/StatelessFirewallException.php @@ -0,0 +1,19 @@ +config['providers']); } + /** + * The scopes this provider's authorization request asks for. + * + * Read from configuration, like the callback paths: the caller needs the answer + * before it has any use for a provider. + * + * An unconfigured key answers the same default the config tree applies, keeping + * one rule in two places from drifting apart. + * + * @return string[] + */ + public function getScopes(string $providerKey): array + { + return $this->config['providers'][$providerKey]['scopes'] ?? ['openid', 'email', 'profile']; + } + /** * Whether this provider's authorization request carries a PKCE challenge. * diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index d3411d6..67b65fe 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -7,14 +7,17 @@ use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; +use ItkDev\OpenIdConnectBundle\Exception\StatelessFirewallException; use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException; 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\Authenticator\InteractiveAuthenticatorInterface; use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface; use Symfony\Component\Security\Http\Util\TargetPathTrait; @@ -47,7 +50,7 @@ * class with two states where only one is meaningful. Defaulting to a * `NullLogger` keeps a single code path for consumers who never get a logger. */ -abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements AuthenticationEntryPointInterface, LoggerAwareInterface +abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements AuthenticationEntryPointInterface, InteractiveAuthenticatorInterface, LoggerAwareInterface { use TargetPathTrait; @@ -85,6 +88,18 @@ public function setLogger(LoggerInterface $logger): void $this->logger = $logger; } + /** + * A person logged in here, at an identity provider, just now. + * + * Symfony dispatches `security.interactive_login` for an authenticator that says + * so, and remember-me treats the resulting token as one a user actually asked + * for. Both are true of every login this authenticator completes. + */ + public function isInteractive(): bool + { + return true; + } + /** * Whether this request is a callback for one of this authenticator's providers. * @@ -197,7 +212,11 @@ protected function getSupportedProviderKeys(): array */ protected function validateClaims(Request $request): array { - $session = $request->getSession(); + try { + $session = $request->getSession(); + } catch (SessionNotFoundException $exception) { + throw new StatelessFirewallException('The OpenID Connect authenticator needs a session to hold the state, nonce and PKCE verifier between the authorization request and the callback. Remove `stateless: true` from this firewall.', previous: $exception); + } // Every one-time value is spent here, before anything below can throw. A // callback is used up whether it succeeds, fails validation, or carries the diff --git a/tests/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index 2f475d4..ef58dd5 100644 --- a/tests/Controller/LoginControllerTest.php +++ b/tests/Controller/LoginControllerTest.php @@ -174,6 +174,24 @@ public function testAStaleVerifierIsOverwrittenWhenPkceIsOff(): void $this->assertNull($session->get('oauth2pkce_verifier')); } + public function testConfiguredScopesReachTheAuthorizationRequest(): void + { + $mockProvider = $this->createMock(OpenIdConfigurationProvider::class); + $mockProvider->method('generateNonce')->willReturn('1234'); + $mockProvider->method('generateState')->willReturn('abcd'); + $mockProvider + ->expects($this->once()) + ->method('getAuthorizationUrl') + ->with($this->callback( + static fn (array $options): bool => 'openid profile groups' === ($options['scope'] ?? null) + )) + ->willReturn('https://provider.example.org/authorize'); + + $controller = $this->createController($mockProvider, scopes: ['openid', 'profile', 'groups']); + + $controller->login(new Request(), new Session(new MockArraySessionStorage()), 'test'); + } + public function testUnknownProviderKeyMapsTo404(): void { $cause = new InvalidProviderException('Invalid provider: bogus'); @@ -339,7 +357,10 @@ public function testUnknownProviderIsRefusedBeforeTheExpiryCheck(): void $this->fail('Expected NotFoundHttpException'); } - private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null, bool $pkce = false): LoginController + /** + * @param string[] $scopes + */ + private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null, bool $pkce = false, array $scopes = ['openid', 'email', 'profile']): LoginController { $mockProviderManager = $this->createMock(OpenIdConfigurationProviderManager::class); $mockProviderManager @@ -351,6 +372,10 @@ private function createController(OpenIdConfigurationProvider $provider, ?Client ->method('isPkceEnabled') ->with('test') ->willReturn($pkce); + $mockProviderManager + ->method('getScopes') + ->with('test') + ->willReturn($scopes); return new LoginController($mockProviderManager, $this->logger, $expiryChecker ?? $this->createExpiryChecker()); } diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index 0694d96..ff3e0cd 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -543,4 +543,124 @@ public function testPkceCanBeTurnedOff(): void $this->assertFalse($config['openid_providers']['provider1']['options']['pkce']); } + + public function testScopesDefaultToTheOpenIdConnectBasics(): void + { + $config = $this->processor->processConfiguration($this->configuration, [$this->getMinimalConfig()]); + + $this->assertSame(['openid', 'email', 'profile'], $config['openid_providers']['provider1']['options']['scopes']); + } + + public function testScopesCanBeConfigured(): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['scopes'] = ['openid', 'profile', 'groups']; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame(['openid', 'profile', 'groups'], $config['openid_providers']['provider1']['options']['scopes']); + } + + /** + * @return iterable + */ + public static function scopeStringProvider(): iterable + { + yield 'single space' => ['openid profile groups', ['openid', 'profile', 'groups']]; + yield 'surrounding whitespace' => [' openid profile ', ['openid', 'profile']]; + yield 'runs of whitespace' => ["openid\t\tprofile\ngroups", ['openid', 'profile', 'groups']]; + yield 'one scope' => ['openid', ['openid']]; + } + + /** + * An environment variable can only carry a scalar, so the space-delimited form + * RFC 6749 §3.3 already uses on the wire is accepted here too. + * + * @param string[] $expected + */ + #[DataProvider('scopeStringProvider')] + public function testScopesAcceptASpaceSeparatedString(string $configured, array $expected): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['scopes'] = $configured; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame($expected, $config['openid_providers']['provider1']['options']['scopes']); + } + + /** + * @return iterable + */ + public static function scopesWithoutOpenIdProvider(): iterable + { + yield 'a list' => [['email', 'profile']]; + yield 'a string' => ['email profile']; + } + + #[DataProvider('scopesWithoutOpenIdProvider')] + public function testScopesMustIncludeOpenId(mixed $configured): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['scopes'] = $configured; + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('scopes must include openid: without it the provider returns no ID token.'); + $this->processor->processConfiguration($this->configuration, [$input]); + } + + public function testScopesCannotBeEmpty(): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options']['scopes'] = []; + + $this->expectException(InvalidConfigurationException::class); + $this->processor->processConfiguration($this->configuration, [$input]); + } + + /** + * @return iterable + */ + public static function negativeDurationProvider(): iterable + { + yield 'leeway' => ['leeway', -1]; + yield 'cache_duration' => ['cache_duration', -1]; + } + + /** + * Rejected while the container compiles rather than at the first login that + * needs the value. + */ + #[DataProvider('negativeDurationProvider')] + public function testDurationsCannotBeNegative(string $option, int $value): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options'][$option] = $value; + + $this->expectException(InvalidConfigurationException::class); + $this->processor->processConfiguration($this->configuration, [$input]); + } + + /** + * @return iterable + */ + public static function zeroableDurationProvider(): iterable + { + yield 'leeway' => ['leeway']; + yield 'cache_duration' => ['cache_duration']; + } + + /** + * Zero is a coherent setting for both: no clock-skew window, and no caching. + */ + #[DataProvider('zeroableDurationProvider')] + public function testDurationsMayBeZero(string $option): void + { + $input = $this->getMinimalConfig(); + $input['openid_providers']['provider1']['options'][$option] = 0; + + $config = $this->processor->processConfiguration($this->configuration, [$input]); + + $this->assertSame(0, $config['openid_providers']['provider1']['options'][$option]); + } } diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index d5db498..148a23b 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -9,6 +9,7 @@ use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; +use ItkDev\OpenIdConnectBundle\Exception\StatelessFirewallException; use ItkDev\OpenIdConnectBundle\Exception\TokenNotFoundException; use ItkDev\OpenIdConnectBundle\Exception\UsernameDoesNotExistException; use PHPUnit\Framework\Attributes\DataProvider; @@ -38,6 +39,9 @@ public static function concreteProvider(): iterable yield 'InvalidProviderException' => [InvalidProviderException::class, \InvalidArgumentException::class]; yield 'UsernameDoesNotExistException' => [UsernameDoesNotExistException::class, \InvalidArgumentException::class]; + // Programmer error that should be fixed in code → \LogicException + yield 'StatelessFirewallException' => [StatelessFirewallException::class, \LogicException::class]; + // Runtime conditions → \RuntimeException yield 'CacheException' => [CacheException::class, \RuntimeException::class]; yield 'TokenNotFoundException' => [TokenNotFoundException::class, \RuntimeException::class]; diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 5fe8982..2c3ffdd 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -11,6 +11,7 @@ use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; +use ItkDev\OpenIdConnectBundle\Exception\StatelessFirewallException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; @@ -18,6 +19,7 @@ use PHPUnit\Framework\MockObject\Stub; use PHPUnit\Framework\TestCase; use Psr\Log\LogLevel; +use Symfony\Component\HttpFoundation\Exception\SessionNotFoundException; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Session\Session; use Symfony\Component\HttpFoundation\Session\SessionInterface; @@ -821,6 +823,36 @@ public function testTheProviderErrorLeavesOnAuthenticationFailureUnwrapped(): vo $this->fail('Expected ProviderErrorException'); } + /** + * Symfony dispatches `security.interactive_login` for an authenticator that says + * so, and keys remember-me off it. + */ + public function testTheAuthenticatorIsInteractive(): void + { + $this->assertTrue($this->authenticator->isInteractive()); + } + + /** + * The flow spans two requests and the session is what ties them together, so a + * firewall with none can never complete a login. Named as the misconfiguration it + * is rather than surfacing Symfony's SessionNotFoundException as a 500. + */ + public function testAStatelessFirewallIsNamedAsTheProblem(): void + { + // A Request with no session set is what a stateless firewall hands over. + $request = new Request(query: ['state' => 'test_state', 'code' => 'test_code']); + + try { + $this->authenticator->authenticate($request); + } catch (StatelessFirewallException $thrown) { + $this->assertStringContainsString('stateless: true', $thrown->getMessage()); + $this->assertInstanceOf(SessionNotFoundException::class, $thrown->getPrevious()); + + return; + } + $this->fail('Expected StatelessFirewallException'); + } + public function testTheStoredVerifierIsSentWithTheTokenRequest(): void { $claims = new \stdClass(); From 88dda852b4dc194b1f0cc22990381036c5fecfa8 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 14:56:09 +0200 Subject: [PATCH 5/8] docs: state the bundle's worker-mode contract --- CHANGELOG.md | 3 +++ README.md | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32515ee..5ac1eae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 authenticator redeems the code with it. Turn it off per provider with `pkce: false` for an identity provider that rejects the parameters rather than ignoring them. - `OpenIdConfigurationProviderManager::isPkceEnabled()` and `getScopes()`. +- A README section on running under a worker runtime: what the bundle shares between + requests and why, what a consumer's authenticator must not hold, and the + stateful-firewall requirement. - Per-provider `scopes`, defaulting to `openid`, `email` and `profile` — the scopes the bundle has always requested. Accepts a list or a space-separated string, so the value can come from an environment variable. A list without `openid` is rejected at compile diff --git a/README.md b/README.md index 12927ff..34ab264 100644 --- a/README.md +++ b/README.md @@ -1008,6 +1008,58 @@ prevent. See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning. +## Worker mode (FrankenPHP, Roadrunner) + +The bundle is safe to run under a worker runtime, where one process serves many +requests and every service outlives the request that created it. + +No service in the bundle retains request data. State, nonce, PKCE verifier and claims +live on the request or in the session, never on a collaborator, and every one-time +session value is spent on the callback that uses it. `getProvider()` returns a fresh +provider each call, because `league/oauth2-client` records the authorization request's +`state` on the provider it builds. + +Three things are shared across requests on purpose: + +| What | Why it is safe | +| --- | --- | +| The Guzzle client, one per provider | Fixed options and a connection pool. Sharing it is the point: a token exchange reuses an open connection instead of renegotiating TLS. | +| The derived callback paths | Computed from your configuration and the routing base URL, and cached under that base URL. Two requests sharing a key derive identical values. | +| The authenticator's logger | Injected once by the container, never per request. | + +### What your authenticator must not do + +Your `OpenIdLoginAuthenticator` subclass is a shared service too. Its `authenticate()` +and `onAuthenticationSuccess()` run once per request on the same object, so nothing +belonging to a request may be assigned to a property: + +```php +// Wrong: the next request through this process sees the previous user's claims. +private array $claims; + +public function authenticate(Request $request): Passport +{ + $this->claims = $this->validateClaims($request); + // ... +} +``` + +Pass the values down instead, or put them on the request's attributes. The same applies +to a user provider, a claims mapper, or anything else you inject into the flow. + +### The firewall must be stateful + +The authorization code flow spans two requests, and the session is what ties them +together. A firewall declared `stateless: true` throws `StatelessFirewallException`, +naming the setting to remove. + +### If you audit this yourself + +[`igor-php`](https://github.com/igor-php/igor-php) is a static analyser for worker-mode +state leaks. Pointed at this bundle's `src/` it reports the three shared values in the +table above and nothing else — they are shared deliberately, so record them in an +`igor-baseline.json` with a reason rather than refactoring them away. + ## Sign in from command line Rather than signing in via OpenId Connect, you can get a sign in url from the From 0cf6a0f633741658be099a7e3492f66ae4b8489a Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 15:08:06 +0200 Subject: [PATCH 6/8] ci: gate worker-mode state leaks with Igor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Igor reports shared mutable state, which is not the same thing as a leak. The four values it finds here are shared on purpose — Guzzle clients and their connection pools, the derived callback paths, the authenticator's logger, and Symfony's own lazy init of the bundle extension — and igor-baseline.json records each with a written reason. What the gate protects is the difference: state that appears without one. That is the case worth catching. The provider memoization removed in the PKCE work was exactly this shape, and nothing but review would have stopped it coming back. IGOR_VERSION pins the binary the composer bootstrapper fetches, in the Taskfile and the workflow. Igor is pre-1.0 and its rules move between releases, which would move the baseline underneath us. --- .github/workflows/php.yaml | 22 ++++++++++++++++++++++ CHANGELOG.md | 4 ++++ README.md | 32 +++++++++++++++++++++++++++----- Taskfile.yml | 20 ++++++++++++++++++++ composer.json | 1 + igor-baseline.json | 26 ++++++++++++++++++++++++++ igor.json | 26 ++++++++++++++++++++++++++ 7 files changed, 126 insertions(+), 5 deletions(-) create mode 100644 igor-baseline.json create mode 100644 igor.json diff --git a/.github/workflows/php.yaml b/.github/workflows/php.yaml index 8807a29..2748def 100644 --- a/.github/workflows/php.yaml +++ b/.github/workflows/php.yaml @@ -25,6 +25,28 @@ jobs: docker compose run --rm phpfpm composer install docker compose run --rm phpfpm vendor/bin/php-cs-fixer fix --dry-run --diff + worker-mode: + # Igor reports shared mutable state, which is not the same thing as a leak: the + # values in igor-baseline.json are shared on purpose, each with a written reason. + # What this job protects is the difference — state that appears without one. + # + # IGOR_VERSION pins the binary the composer bootstrapper fetches. Igor is pre-1.0 + # and its rules change between releases, which would move the baseline underneath + # us. Bump it deliberately, and regenerate the baseline when you do. + name: Worker mode (Igor) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - name: Create docker network + run: | + docker network create frontend + + - run: | + docker compose run --rm phpfpm composer install + docker compose run --rm -e IGOR_VERSION=0.9.5 phpfpm vendor/bin/igor-php --no-agent . + docker compose run --rm -e IGOR_VERSION=0.9.5 phpfpm vendor/bin/igor-php --no-agent --check-baseline . + 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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ac1eae..913ff0f 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 authenticator redeems the code with it. Turn it off per provider with `pkce: false` for an identity provider that rejects the parameters rather than ignoring them. - `OpenIdConfigurationProviderManager::isPkceEnabled()` and `getScopes()`. +- CI gates worker-mode compatibility with [Igor](https://github.com/igor-php/igor-php). + `igor-baseline.json` records the state the bundle shares on purpose, one written + reason per entry; anything else fails the build. `task analyze:worker` runs it + locally. - A README section on running under a worker runtime: what the bundle shares between requests and why, what a consumer's authenticator must not hold, and the stateful-firewall requirement. diff --git a/README.md b/README.md index 34ab264..ebea185 100644 --- a/README.md +++ b/README.md @@ -1053,12 +1053,16 @@ The authorization code flow spans two requests, and the session is what ties the together. A firewall declared `stateless: true` throws `StatelessFirewallException`, naming the setting to remove. -### If you audit this yourself +### How this is enforced -[`igor-php`](https://github.com/igor-php/igor-php) is a static analyser for worker-mode -state leaks. Pointed at this bundle's `src/` it reports the three shared values in the -table above and nothing else — they are shared deliberately, so record them in an -`igor-baseline.json` with a reason rather than refactoring them away. +CI runs [`igor-php`](https://github.com/igor-php/igor-php), a static analyser for +worker-mode state leaks, on every pull request. The three values above are recorded in +`igor-baseline.json`, each with a written reason for why sharing it is safe. Anything +else that appears fails the build. + +If you audit your own application with it, expect the same shape of result: the tool +reports shared mutable state, which is not the same thing as a leak. Judge each finding +and record the safe ones with a reason rather than refactoring them away. ## Sign in from command line @@ -1148,6 +1152,24 @@ to `infection.log` and `infection.html` on each run. task analyze ``` +### Worker Mode Analysis + +```shell +task analyze:worker # audit for state that leaks between requests +task analyze:worker:check # fail if the baseline lists findings that no longer occur +task analyze:worker:baseline # regenerate the baseline after judging new findings +``` + +`igor-baseline.json` records the state this bundle shares on purpose, one written +reason per entry. A new finding fails `task analyze:worker`: either make the code +stateless, or add it to the baseline with a reason that says why sharing it is safe. +Never add an entry without one. + +The analyser is a Go binary that the composer package downloads on first run. `task` +pins the version, since Igor is pre-1.0 and its rules change between releases; bump +`IGOR_VERSION` in `Taskfile.yml` and `.github/workflows/php.yaml` together, and +regenerate the baseline when you do. + ### Coding Standards Check all coding standards: diff --git a/Taskfile.yml b/Taskfile.yml index 806f150..e04df9e 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -6,6 +6,10 @@ vars: DOCKER_COMPOSE: "docker compose" PHP: "{{.DOCKER_COMPOSE}} exec phpfpm" COMPOSER: "{{.PHP}} composer" + # Pinned: Igor is pre-1.0 and its rules change between releases, which would move + # the baseline underneath us. The composer package is only a bootstrapper; this is + # the version of the binary it fetches. + IGOR_VERSION: "0.9.5" tasks: default: @@ -132,6 +136,21 @@ tasks: cmds: - "{{.PHP}} vendor/bin/phpstan" + analyze:worker: + desc: Audit worker-mode (FrankenPHP) state leaks with Igor + cmds: + - "{{.DOCKER_COMPOSE}} exec -e IGOR_VERSION={{.IGOR_VERSION}} phpfpm vendor/bin/igor-php --no-agent ." + + analyze:worker:baseline: + desc: Regenerate the Igor baseline (each entry then needs a reason) + cmds: + - "{{.DOCKER_COMPOSE}} exec -e IGOR_VERSION={{.IGOR_VERSION}} phpfpm vendor/bin/igor-php --no-agent --generate-baseline ." + + analyze:worker:check: + desc: Fail if the Igor baseline lists findings that no longer occur + cmds: + - "{{.DOCKER_COMPOSE}} exec -e IGOR_VERSION={{.IGOR_VERSION}} phpfpm vendor/bin/igor-php --no-agent --check-baseline ." + # Testing test: @@ -254,5 +273,6 @@ tasks: - task: composer:check - task: lint - task: analyze:php + - task: analyze:worker - task: test:matrix - task: test:mutation diff --git a/composer.json b/composer.json index 621365b..ef3d942 100644 --- a/composer.json +++ b/composer.json @@ -31,6 +31,7 @@ "require-dev": { "ergebnis/composer-normalize": "^2.28", "friendsofphp/php-cs-fixer": "^3.11", + "igor-php/igor-php": "^0.9", "infection/infection": "*", "phpstan/phpstan": "^2.1.41", "phpstan/phpstan-deprecation-rules": "^2.0", diff --git a/igor-baseline.json b/igor-baseline.json new file mode 100644 index 0000000..bc90791 --- /dev/null +++ b/igor-baseline.json @@ -0,0 +1,26 @@ +{ + "files": { + "src/ItkDevOpenIdConnectBundle.php": [ + { + "message": "Mutation of state 'extension' in ItkDevOpenIdConnectBundle::getContainerExtension()", + "reason": "Symfony's own lazy init for a bundle's extension, inherited from Bundle. It runs while the container is compiled, not while a request is served, and the extension it stores is immutable." + } + ], + "src/Security/OpenIdConfigurationProviderManager.php": [ + { + "message": "Mutation of state 'redirectUriPaths' in OpenIdConfigurationProviderManager::getRedirectUriPaths()", + "reason": "Paths derived from configuration and the routing context's base URL, cached under that base URL. Two requests sharing a key derive identical values, so nothing one request sees came from another, and the key set is bounded by the base URLs the application answers on. supports() consults this on every request through the firewall, which is why it is cached." + }, + { + "message": "Mutation of state 'httpClients' in OpenIdConfigurationProviderManager::httpClient()", + "reason": "Guzzle clients, one per provider: fixed options from configuration plus a connection pool. Sharing them is the point, so that a token exchange reuses an open connection to the identity provider instead of renegotiating TLS. The provider built around the client is not shared; getProvider() returns a fresh one, pinned by testNoRequestStateSurvivesOnTheNextProvider()." + } + ], + "src/Security/OpenIdLoginAuthenticator.php": [ + { + "message": "Mutation of state 'logger' in OpenIdLoginAuthenticator::setLogger()", + "reason": "Written once by the container at build time, through LoggerAwareInterface autoconfiguration. Never written while a request is served." + } + ] + } +} diff --git a/igor.json b/igor.json new file mode 100644 index 0000000..a3f6dc4 --- /dev/null +++ b/igor.json @@ -0,0 +1,26 @@ +{ + "exclude": [ + "tests", + "vendor", + "var", + "node_modules", + "build", + "coverage", + "phpstan" + ], + "safe_namespaces": [ + "Symfony\\", + "Doctrine\\", + "Psr\\", + "IgorPhp\\IgorBundle\\" + ], + "scan_vendors": null, + "ignore_vendors": true, + "ignore_external_baseline": false, + "console_path": "", + "env": "prod", + "verbose": false, + "baseline": "igor-baseline.json", + "output": "", + "container_dump": "" +} From 3eb11ff5a554c18578d21c4e11b594cbc1dc0475 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 15:58:45 +0200 Subject: [PATCH 7/8] docs: developing against a mock identity provider --- CHANGELOG.md | 2 ++ README.md | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 913ff0f..accfba2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `igor-baseline.json` records the state the bundle shares on purpose, one written reason per entry; anything else fails the build. `task analyze:worker` runs it locally. +- A README section on developing against a mock identity provider, and why that beats + turning the firewall off in `dev`. - A README section on running under a worker runtime: what the bundle shares between requests and why, what a consumer's authenticator must not hold, and the stateful-firewall requirement. diff --git a/README.md b/README.md index ebea185..aea5e09 100644 --- a/README.md +++ b/README.md @@ -1064,6 +1064,81 @@ If you audit your own application with it, expect the same shape of result: the reports shared mutable state, which is not the same thing as a leak. Judge each finding and record the safe ones with a reason rather than refactoring them away. +## Local development against a mock identity provider + +Pointing a development environment at the real identity provider is usually +impractical: it will not have your local hostname among its registered redirect URIs, +and you may not want real accounts logging into a laptop. A mock provider gives you +the whole authorization code flow locally, so the callback path, the claims mapping +and the failure paths are exercised the way they will be in production. + +[`oidc-provider-mock`](https://github.com/geigerzaehler/oidc-provider-mock) needs no +configuration file — users are given as repeated `--user-claims` flags, and it accepts +any client id and secret. A service in an override file, so it never starts on a +server: + +```yaml +services: + idp: + image: ghcr.io/geigerzaehler/oidc-provider-mock:latest + networks: [app] + expose: + - "80" + command: + - "--port" + - "80" + - "--user-claims" + - '{"sub": "admin", "email": "admin@example.org", "name": "Admin Jensen", "groups": ["administrator"]}' + - "--user-claims" + - '{"sub": "editor", "email": "editor@example.org", "name": "Ed Editor", "groups": ["editor"]}' +``` + +At the login screen you pick which of those identities to be, which makes testing a +role or a claim a matter of choosing a different subject. + +Point a provider at it in development-only configuration: + +```yaml +# config/packages/dev/itkdev_openid_connect.yaml +itkdev_openid_connect: + openid_providers: + admin: + options: + metadata_url: 'http://idp/.well-known/openid-configuration' + # Any values will do; the mock accepts whatever it is given. + client_id: 'client-id' + client_secret: 'client-secret' + redirect_uri: 'http://localhost:8080/openid-connect/callback' + # Required: the mock is reached over http between containers, and the + # bundle refuses plain http otherwise. Never set this in production. + allow_http: true +``` + +Two things to know: + +* **`allow_http: true` is mandatory here.** Traffic between containers is http, and + since `itk-dev/openid-connect` 5.1 the scheme check covers every endpoint the + discovery document announces, not only `metadata_url`. Keep it in development-only + configuration rather than driving it from an environment variable that could be set + wrong somewhere else. +* **PKCE needs no special handling.** The mock accepts the challenge and the verifier, + so a login completes with the bundle's default. It does not advertise + `code_challenge_methods_supported`, so it is not checking the challenge — the round + trip is exercised, the protection is not. Set `pkce: false` only for a provider that + rejects the parameters outright rather than ignoring them. + +**Prefer a mock over turning security off in `dev`.** Disabling the firewall for the +development environment is the tempting shortcut, and it means no OpenID Connect code +path is exercised until it reaches a server: a broken callback path, a renamed claim +or a login loop all stay invisible locally. It is also easy to forget, so the next +person to debug an authentication problem loses an afternoon to a firewall that was +never running. + +[deltag.aarhus.dk](https://github.com/itk-dev/deltag.aarhus.dk/blob/develop/docker-compose.oidc.yml) +has a worked example, including two providers side by side. + +ITK Dev developers: the internal ITK Dev documentation covers the fuller setup. + ## Sign in from command line Rather than signing in via OpenId Connect, you can get a sign in url from the From 1577a2ffef97169d196b233f09ae0f30f184f21f Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 16:15:16 +0200 Subject: [PATCH 8/8] docs: prepare 6.1.0 Collapse the unreleased entries into a 6.1.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 | 95 +++++++++++++++++++--------------------------- UPGRADE-6.1.md | 100 ++++++++++++++----------------------------------- 2 files changed, 67 insertions(+), 128 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index accfba2..424182a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,72 +7,54 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [6.1.0] - 2026-08-26 + +See [UPGRADE-6.1.md](UPGRADE-6.1.md). Nothing is required of a consumer. + ### Added -- PKCE (RFC 7636, S256), on by default. The login route generates a verifier, keeps it - in the session under `oauth2pkce_verifier`, and sends the challenge; the - authenticator redeems the code with it. Turn it off per provider with `pkce: false` - for an identity provider that rejects the parameters rather than ignoring them. -- `OpenIdConfigurationProviderManager::isPkceEnabled()` and `getScopes()`. -- CI gates worker-mode compatibility with [Igor](https://github.com/igor-php/igor-php). - `igor-baseline.json` records the state the bundle shares on purpose, one written - reason per entry; anything else fails the build. `task analyze:worker` runs it - locally. -- A README section on developing against a mock identity provider, and why that beats - turning the firewall off in `dev`. -- A README section on running under a worker runtime: what the bundle shares between - requests and why, what a consumer's authenticator must not hold, and the - stateful-firewall requirement. -- Per-provider `scopes`, defaulting to `openid`, `email` and `profile` — the scopes the - bundle has always requested. Accepts a list or a space-separated string, so the value - can come from an environment variable. A list without `openid` is rejected at compile - time. -- `StatelessFirewallException`, naming the misconfiguration when the authenticator is - put on a firewall declared `stateless: true`. Previously Symfony's - `SessionNotFoundException` surfaced as an unexplained 500. +- PKCE (RFC 7636, S256) on every authorization request. The verifier is kept in the + session under `oauth2pkce_verifier`. `pkce: false` per provider turns it off. +- `scopes` per provider, defaulting to `openid`, `email` and `profile`. Accepts a list + or a space-separated string, and must include `openid`. - `ProviderErrorException`, thrown when the identity provider refuses the authorization - request (RFC 6749 §4.1.2.1). It extends `AuthenticationFailedException`, so existing - `catch` blocks keep matching, and carries `getError()`, `getErrorDescription()` and - `getStatusCode()`. See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md). + request (RFC 6749 §4.1.2.1). Extends `AuthenticationFailedException` and carries + `getError()`, `getErrorDescription()` and `getStatusCode()`. See + [ADR 004](docs/adr/004-handle-provider-error-callbacks.md). +- `StatelessFirewallException`, thrown when the authenticator is used on a firewall + declared `stateless: true`. +- `OpenIdConfigurationProviderManager::isPkceEnabled()` and `getScopes()`. +- README sections on developing against a mock identity provider and on running under + a worker runtime. +- A worker-mode CI gate ([Igor](https://github.com/igor-php/igor-php)) with + `igor-baseline.json`. `task analyze:worker` runs it locally. ### Fixed -- A provider error callback no longer loops between the application and the identity - provider. `supports()` accepts a callback carrying `state` and either `code` or - `error`, so a refusal — a cancelled consent screen, an expired provider session, a - tenant policy — ends in a page that says so instead of another authorization request. - Observed against Azure AD B2C. +- A callback carrying `error` and no `code` is recognised, so a refused login ends in + an error page instead of another authorization request (#63 shape, seen against + Azure AD B2C). ### Changed +- Requires `itk-dev/openid-connect` `^5.1`, which enforces `allowHttp` on every + discovered endpoint, requires `exp` and `iat` on the ID token, and changes the JWKS + cache key. +- A refused login answers 403, or 503 where the provider reports its own trouble and + 500 otherwise. Other callback failures still answer 500. +- `error` and `error_description` are sanitized before they are logged or held, and are + not read until the callback's state matches. +- `oauth2provider`, `oauth2state`, `oauth2nonce` and `oauth2pkce_verifier` are consumed + on every callback. +- The stored state is compared with `hash_equals()`; an empty or missing one is + rejected explicitly. +- `getProvider()` returns a fresh provider on every call. The HTTP client is cached per + provider instead, so connections are still reused. - `OpenIdLoginAuthenticator` implements `InteractiveAuthenticatorInterface`, so a - completed login dispatches `security.interactive_login` and remember-me treats the - token as one a user asked for. -- `leeway` and `cache_duration` reject a negative value while the container compiles. - A negative leeway used to fail at the first login that needed it, and a negative - cache duration passed through to the cache unnoticed. -- Requires `itk-dev/openid-connect` `^5.1`, for its PKCE support. That release also - enforces `allowHttp` on every discovered endpoint, requires `exp` and `iat` on the - ID token, and changes the JWKS cache key — see its changelog before upgrading. -- `getProvider()` returns a fresh provider on every call instead of a memoized one. - `league/oauth2-client` writes the authorization request's `state` onto the provider, - so a held instance carried one request's state into the next — harmless today, but - not under a worker runtime where the process outlives the request. The HTTP client - is now what is kept per provider, so the connection pool still survives. -- A refused login is answered with the status that matches its cause: 403 where the - user or a policy declined, 503 where the provider reports its own trouble, 500 - otherwise. Other callback failures are unchanged and still surface as 500. -- `error` and `error_description` are sanitized before they are logged or held — - control characters collapsed, invalid UTF-8 dropped, capped at 200 characters — and - neither is read at all until the callback's state matches. -- Every one-time session value is consumed on every callback, including one carrying a - provider error: `oauth2provider`, `oauth2state`, `oauth2nonce` and - `oauth2pkce_verifier`. -- The stored state is compared with `hash_equals()`, and an empty or missing stored - state is rejected explicitly rather than by comparison. -- A callback naming a provider that is not configured is now reported as an invalid - state at `warning` when its state does not match, rather than as an unconfigured - provider at `error`: the provider is built after the state check, not before it. + completed login dispatches `security.interactive_login`. +- `leeway` and `cache_duration` reject negative values while the container compiles. +- A callback whose state does not match is reported as an invalid state even when its + provider key is no longer configured. ## [6.0.0] - 2026-08-25 @@ -395,6 +377,7 @@ See [UPGRADE-6.0.md](UPGRADE-6.0.md). - OpenId Connect Bundle: Added CLI login feature. [unreleased]: https://github.com/itk-dev/openid-connect-bundle/compare/6.0.0...HEAD +[6.1.0]: https://github.com/itk-dev/openid-connect-bundle/compare/6.0.0...6.1.0 [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 diff --git a/UPGRADE-6.1.md b/UPGRADE-6.1.md index 5168b1c..33d0c5c 100644 --- a/UPGRADE-6.1.md +++ b/UPGRADE-6.1.md @@ -4,24 +4,17 @@ composer update itk-dev/openid-connect-bundle ``` -A minor: nothing is required of you. Two things are worth checking. +Nothing is required. Check the points below that apply to you. -## A refused login now ends in a page, not a loop +## A refused login ends in an error page -When the identity provider refuses an authorization request — the user closed the -consent screen, their session at the provider had expired, a tenant policy said no — -it redirects back to your callback with an `error` and no `code`. +A provider that refuses the authorization request — cancelled consent screen, expired +provider session, tenant policy — now ends the login instead of starting another one. +The bundle throws `ProviderErrorException` and the kernel answers 403, or 503 where the +provider reports its own trouble, 500 otherwise. -Until 6.1 the bundle did not recognise that as a callback. The firewall answered it, -your entry point asked the provider again, the provider refused again, and the browser -never settled. Nothing was logged, because no failing callback existed to log. - -Now that callback is handled: the login ends, the reason is logged at `warning`, and -the user gets a **403** — 503 where the provider reports its own trouble, 500 where the -error says the request or the client registration is wrong. If you were filtering the -loop out of your monitoring, you can stop. - -Catch it if you want a friendlier page than your generic 403: +`ProviderErrorException` extends `AuthenticationFailedException`, so a `catch` written +for 6.0 still matches it. For a friendlier page than your generic 403: ```php use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; @@ -32,38 +25,26 @@ if ($exception instanceof ProviderErrorException } ``` -`ProviderErrorException` extends `AuthenticationFailedException`, so any `catch` you -already wrote for 6.0 keeps matching it. - -**If your application has a listener that redirects 403 responses to a login page, -exclude this exception from it** — otherwise a refusal is sent straight back to the -provider that refused it, rebuilding the loop from your own side. +If your application redirects 403 responses to a login page, exclude this exception +from that listener. ## Chain the cause in your authenticator -When your `authenticate()` catches a bundle exception and raises Symfony's, pass the -original as `previous`: - ```php } catch (OpenIdConnectExceptionInterface $exception) { throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); } ``` -The bundle reads that cause back to decide what the user is shown. Without it the -refusal arrives as a plain 500 with the reason only in the message. This was always -the documented shape; 6.1 is the first release where dropping the cause costs you -something. +Without `previous` the status above is lost and a refusal arrives as a plain 500. ## PKCE is on by default -Every authorization request now carries a PKCE challenge (RFC 7636, S256). RFC 6749 -§3.1 requires an authorization server to ignore parameters it does not recognise, so a -provider that does not support PKCE behaves as it did before, and one that does gets -the extra protection with no configuration from you. +Every authorization request carries an S256 challenge. The verifier is kept in the +session under `oauth2pkce_verifier`; preserve that key if you rewrite the session +between the login redirect and the callback. -If you have an identity provider that rejects unknown parameters rather than ignoring -them, turn it off for that provider: +For a provider that rejects parameters it does not recognise: ```yaml openid_providers: @@ -72,44 +53,19 @@ openid_providers: pkce: false ``` -The verifier is kept in the session under `oauth2pkce_verifier`. If your application -clears or rewrites the session between the login redirect and the callback, it must -preserve that key alongside `oauth2state` and `oauth2nonce`. - -## A stateless firewall is now named as such - -Putting the OpenID Connect authenticator on a firewall declared `stateless: true` -throws `StatelessFirewallException`, naming the setting to remove. It used to surface -as Symfony's `SessionNotFoundException` and an unexplained 500. - -The flow spans two requests and the session is where the state, nonce and PKCE -verifier wait, so such a firewall could never complete a login. Nothing that worked -before stops working. - -## The library requires 5.1 - -`itk-dev/openid-connect` `^5.1` comes with this release. Three of its changes affect -running deployments: an identity provider announcing plain-http endpoints now needs -`allow_http`, an ID token without `exp` or `iat` is rejected, and the JWKS cache key -changed so 5.0's entries are not reused. Read its changelog before deploying. - -## `getProvider()` no longer returns the same instance - -`OpenIdConfigurationProviderManager::getProvider()` builds a fresh provider on every -call. `league/oauth2-client` records the authorization request's `state` on the -provider, so a memoized instance carried one request's state into the next — which -matters once a process outlives a request, as under a FrankenPHP worker. The HTTP -client is kept per provider instead, so connections to the identity provider are still -reused. - -Nothing to do unless you held the returned provider and relied on getting the same -object back. +## Requires `itk-dev/openid-connect` ^5.1 -## Optional: scopes are configurable +Three of its changes affect running deployments: a provider announcing plain-http +endpoints now needs `allow_http`, an ID token without `exp` or `iat` is rejected, and +the JWKS cache key changed. See its changelog. -The authorization request still asks for `openid`, `email` and `profile`. Set `scopes` -per provider to change that; `openid` must remain among them. +## Smaller changes -See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning behind -the error-callback handling, and [CHANGELOG.md](CHANGELOG.md) for the rest of the -release. +- A firewall declared `stateless: true` throws `StatelessFirewallException` naming the + setting, where Symfony's `SessionNotFoundException` used to surface as a 500. +- `getProvider()` returns a fresh provider on every call. Only matters if you held the + returned object and relied on getting the same one back. +- `scopes` is configurable per provider and must include `openid`. +- `leeway` and `cache_duration` reject negative values while the container compiles. +- The authenticator implements `InteractiveAuthenticatorInterface`, so a completed + login dispatches `security.interactive_login`.