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 a7e8c3a..424182a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,55 @@ 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 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). 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 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`. +- `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 See [UPGRADE-6.0.md](UPGRADE-6.0.md). @@ -328,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/README.md b/README.md index 2b0930c..aea5e09 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,39 +32,60 @@ 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 > -> This means the following features of this bundle have no native Symfony -> equivalent: +> 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. +> +> ### 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). +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 @@ -136,6 +159,13 @@ 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: 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)%' @@ -649,8 +679,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); } } @@ -836,7 +867,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); } } @@ -860,6 +891,254 @@ 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. +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` +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. + +## 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. + +### How this is enforced + +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. + +## 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 @@ -948,6 +1227,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/UPGRADE-6.1.md b/UPGRADE-6.1.md new file mode 100644 index 0000000..33d0c5c --- /dev/null +++ b/UPGRADE-6.1.md @@ -0,0 +1,71 @@ +# Upgrading from 6.0 to 6.1 + +```sh +composer update itk-dev/openid-connect-bundle +``` + +Nothing is required. Check the points below that apply to you. + +## A refused login ends in an error page + +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. + +`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; + +if ($exception instanceof ProviderErrorException + && ProviderErrorException::ACCESS_DENIED === $exception->getError()) { + // "You cancelled the sign-in — try again" +} +``` + +If your application redirects 403 responses to a login page, exclude this exception +from that listener. + +## Chain the cause in your authenticator + +```php +} catch (OpenIdConnectExceptionInterface $exception) { + throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); +} +``` + +Without `previous` the status above is lost and a refusal arrives as a plain 500. + +## PKCE is on by default + +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. + +For a provider that rejects parameters it does not recognise: + +```yaml +openid_providers: + legacy: + options: + pkce: false +``` + +## Requires `itk-dev/openid-connect` ^5.1 + +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. + +## Smaller changes + +- 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`. diff --git a/composer.json b/composer.json index f925474..ef3d942 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", @@ -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/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/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": "" +} diff --git a/src/Controller/LoginController.php b/src/Controller/LoginController.php index 6cac73f..d7eec6f 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,27 @@ 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', - ]); + // Space-delimited, as RFC 6749 §3.3 defines the parameter. + 'scope' => implode(' ', $this->providerManager->getScopes($providerKey)), + ]; + + 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..f870627 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -158,10 +158,56 @@ 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 + // 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') 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/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 @@ + */ - 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,8 @@ class OpenIdConfigurationProviderManager * callback_path?: string, * leeway?: int, * cache_duration?: int, + * pkce?: bool, + * scopes?: string[], * allow_http?: bool, * http_client_options?: array{ * timeout?: float, @@ -55,6 +77,37 @@ public function getProviderKeys(): array return array_keys($this->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. + * + * 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 +219,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 +265,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 ee9cd81..67b65fe 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -6,14 +6,18 @@ use ItkDev\OpenIdConnect\Exception\ValidationException; 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; @@ -22,8 +26,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 @@ -45,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; @@ -58,6 +63,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; /** @@ -74,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. * @@ -83,6 +109,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 +128,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; } @@ -175,16 +212,75 @@ 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 + // provider's refusal, and a value left behind is one a later request can + // 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 — // 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 +294,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'); @@ -221,7 +301,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) { @@ -257,7 +341,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/Controller/LoginControllerTest.php b/tests/Controller/LoginControllerTest.php index 10c6641..ef58dd5 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,101 @@ 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 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'); @@ -256,7 +357,10 @@ public function testUnknownProviderIsRefusedBeforeTheExpiryCheck(): void $this->fail('Expected NotFoundHttpException'); } - private function createController(OpenIdConfigurationProvider $provider, ?ClientSecretExpiryChecker $expiryChecker = null): 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 @@ -264,6 +368,14 @@ private function createController(OpenIdConfigurationProvider $provider, ?Client ->method('getProvider') ->with('test') ->willReturn($provider); + $mockProviderManager + ->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 031d49b..ff3e0cd 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -526,4 +526,141 @@ 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']); + } + + 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 3e140fd..148a23b 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -8,6 +8,8 @@ use ItkDev\OpenIdConnectBundle\Exception\CacheException; 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; @@ -37,10 +39,36 @@ 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]; 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/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 20e05ba..2c3ffdd 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -10,6 +10,8 @@ use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; 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; @@ -17,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; @@ -150,6 +153,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 +165,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,19 +536,417 @@ private function assertValidationExceptionMessage(OpenIdLoginAuthenticator $auth $this->fail(sprintf('Expected ValidationException "%s"', $expectedMessage)); } - private function setSessionOnRequest(Request $request, ?string $nonce = 'test_nonce'): void + 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')); + $this->assertFalse($session->has('oauth2pkce_verifier'), 'A surviving verifier could be redeemed against a later code'); + } + + /** + * @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'); + } + + /** + * 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(); + $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); $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'); + $session->set('oauth2pkce_verifier', 'test_verifier'); + $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' + ); + } +}