From e1403fd4709a57b5f80297b9176e367079843246 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 11:37:39 +0200 Subject: [PATCH] feat: handle provider error callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6749 §4.1.2.1: a provider that refuses redirects back with `error` and `state` and no `code`. supports() required a code, so that request was not a callback: the firewall answered it, the entry point asked again, and the provider refused again. Captured in production against Azure AD B2C as dozens of rounds, nothing logged, the one-time session values never spent. supports() now accepts `state` with either `code` or `error`, on the callback path only, so ADR 003 and issue #63 are untouched. validateClaims() consumes all three one-time values up front, checks state before reading anything else the URL carries, and throws ProviderErrorException with the provider's error code as an accessor. ProviderErrorException extends AuthenticationFailedException, so existing catches keep matching, and implements HttpExceptionInterface so a refusal is a 403 rather than a 500. onAuthenticationFailure() rethrows it unwrapped. State is now compared with hash_equals(), behind an explicit guard against an empty or missing stored value. See ADR 004. --- CHANGELOG.md | 31 ++ README.md | 81 ++++- UPGRADE-6.1.md | 59 ++++ .../004-handle-provider-error-callbacks.md | 145 ++++++++ docs/adr/README.md | 3 + src/Exception/ProviderErrorException.php | 94 +++++ src/Security/OpenIdLoginAuthenticator.php | 155 ++++++-- tests/Exception/ExceptionHierarchyTest.php | 24 ++ .../Exception/ProviderErrorExceptionTest.php | 87 +++++ .../Security/OpenIdLoginAuthenticatorTest.php | 332 ++++++++++++++++++ .../ProviderErrorCallbackDoesNotLoopTest.php | 213 +++++++++++ 11 files changed, 1199 insertions(+), 25 deletions(-) create mode 100644 UPGRADE-6.1.md create mode 100644 docs/adr/004-handle-provider-error-callbacks.md create mode 100644 src/Exception/ProviderErrorException.php create mode 100644 tests/Exception/ProviderErrorExceptionTest.php create mode 100644 tests/Security/ProviderErrorCallbackDoesNotLoopTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index a7e8c3a..7d58bad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,37 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `ProviderErrorException`, thrown when the identity provider refuses the authorization + request (RFC 6749 §4.1.2.1). It extends `AuthenticationFailedException`, so existing + `catch` blocks keep matching, and carries `getError()`, `getErrorDescription()` and + `getStatusCode()`. See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md). + +### Fixed + +- A provider error callback no longer loops between the application and the identity + provider. `supports()` accepts a callback carrying `state` and either `code` or + `error`, so a refusal — a cancelled consent screen, an expired provider session, a + tenant policy — ends in a page that says so instead of another authorization request. + Observed against Azure AD B2C. + +### Changed + +- A refused login is answered with the status that matches its cause: 403 where the + user or a policy declined, 503 where the provider reports its own trouble, 500 + otherwise. Other callback failures are unchanged and still surface as 500. +- `error` and `error_description` are sanitized before they are logged or held — + control characters collapsed, invalid UTF-8 dropped, capped at 200 characters — and + neither is read at all until the callback's state matches. +- `oauth2provider`, `oauth2state` and `oauth2nonce` are consumed on every callback, + including one carrying a provider error. +- The stored state is compared with `hash_equals()`, and an empty or missing stored + state is rejected explicitly rather than by comparison. +- A callback naming a provider that is not configured is now reported as an invalid + state at `warning` when its state does not match, rather than as an unconfigured + provider at `error`: the provider is built after the state check, not before it. + ## [6.0.0] - 2026-08-25 See [UPGRADE-6.0.md](UPGRADE-6.0.md). diff --git a/README.md b/README.md index 529149c..96afa7b 100644 --- a/README.md +++ b/README.md @@ -83,8 +83,9 @@ Symfony bundle for authorization via OpenID Connect. > deprecation will be announced here and in the [CHANGELOG](CHANGELOG.md) once a > migration path exists — realistically no earlier than 2028. -Upgrading from an earlier major? See [UPGRADE-6.0.md](UPGRADE-6.0.md) and -[UPGRADE-5.0.md](UPGRADE-5.0.md). +Upgrading? See [UPGRADE-6.1.md](UPGRADE-6.1.md), and +[UPGRADE-6.0.md](UPGRADE-6.0.md) / [UPGRADE-5.0.md](UPGRADE-5.0.md) if you are coming +from an earlier major. ## Installation @@ -671,8 +672,9 @@ class SomeAuthenticator extends OpenIdLoginAuthenticator // TODO: Implement authenticate() method. } catch (ItkOpenIdConnectException $exception) { - // Authentication failed - throw new CustomUserMessageAuthenticationException($exception->getMessage()); + // Authentication failed. Chain the cause: the bundle reads it back in + // onAuthenticationFailure() to decide what the user is shown. + throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); } } @@ -858,7 +860,7 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator return new SelfValidatingPassport(new UserBadge($user->getUserIdentifier())); } catch (ItkOpenIdConnectException|InvalidProviderException $exception) { - throw new CustomUserMessageAuthenticationException($exception->getMessage()); + throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); } } @@ -882,6 +884,75 @@ class AzureOIDCAuthenticator extends OpenIdLoginAuthenticator } ``` +### When the identity provider refuses + +A provider that will not issue a code redirects back to the callback with an `error` +and no `code` — the user closed the consent screen, their session at the provider had +expired, a tenant policy said no. The bundle recognises that callback, spends the +one-time session values like any other, and throws `ProviderErrorException`. + +It extends `AuthenticationFailedException`, so anything already catching the bundle's +login failure catches this too, and it implements Symfony's `HttpExceptionInterface`, +so the kernel answers a refusal with **403** rather than a 500 — 503 where the +provider reports its own trouble, 500 where the error says our request or +registration is wrong. Nothing is required of the application to get that. + +The error code is an accessor, not something to search the message for: + +```php +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; +use Symfony\Component\EventDispatcher\Attribute\AsEventListener; +use Symfony\Component\HttpKernel\Event\ExceptionEvent; +use Symfony\Component\HttpKernel\KernelEvents; + +#[AsEventListener(KernelEvents::EXCEPTION, priority: 1)] +public function onLoginRefused(ExceptionEvent $event): void +{ + $exception = $event->getThrowable(); + + if (!$exception instanceof ProviderErrorException) { + return; + } + + $template = ProviderErrorException::ACCESS_DENIED === $exception->getError() + ? 'security/login_cancelled.html.twig' + : 'security/login_failed.html.twig'; + + $event->setResponse(new Response( + $this->twig->render($template, ['error' => $exception->getError()]), + $exception->getStatusCode(), + )); +} +``` + +`error` and `error_description` reach you sanitized — control characters collapsed, +invalid UTF-8 dropped, capped at 200 characters — and neither is read at all until +the callback's state matches, so a forged callback cannot put text in your logs or on +your page. `getErrorDescription()` is whatever the provider sent, which may be +nothing; it is a diagnostic, not a message to show a user. + +You can also pin the status and log level without writing a listener: + +```yaml +framework: + exceptions: + ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException: + log_level: info + status_code: 403 +``` + +One thing is required of your authenticator: when `authenticate()` catches a bundle +exception and raises Symfony's, **chain the cause** — `previous: $exception`, as the +examples above do. The bundle reads it back to decide what the user is shown, and an +unchained failure arrives as a plain 500 with the reason only in the message. + +If your application has its own listener that redirects 403 responses to a login +page, exclude `ProviderErrorException` from it. Otherwise a refusal is sent straight +back to the provider that refused it, which is the loop this handling exists to +prevent. + +See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning. + ## Sign in from command line Rather than signing in via OpenId Connect, you can get a sign in url from the diff --git a/UPGRADE-6.1.md b/UPGRADE-6.1.md new file mode 100644 index 0000000..10d2e68 --- /dev/null +++ b/UPGRADE-6.1.md @@ -0,0 +1,59 @@ +# Upgrading from 6.0 to 6.1 + +```sh +composer update itk-dev/openid-connect-bundle +``` + +A minor: nothing is required of you. Two things are worth checking. + +## A refused login now ends in a page, not a loop + +When the identity provider refuses an authorization request — the user closed the +consent screen, their session at the provider had expired, a tenant policy said no — +it redirects back to your callback with an `error` and no `code`. + +Until 6.1 the bundle did not recognise that as a callback. The firewall answered it, +your entry point asked the provider again, the provider refused again, and the browser +never settled. Nothing was logged, because no failing callback existed to log. + +Now that callback is handled: the login ends, the reason is logged at `warning`, and +the user gets a **403** — 503 where the provider reports its own trouble, 500 where the +error says the request or the client registration is wrong. If you were filtering the +loop out of your monitoring, you can stop. + +Catch it if you want a friendlier page than your generic 403: + +```php +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; + +if ($exception instanceof ProviderErrorException + && ProviderErrorException::ACCESS_DENIED === $exception->getError()) { + // "You cancelled the sign-in — try again" +} +``` + +`ProviderErrorException` extends `AuthenticationFailedException`, so any `catch` you +already wrote for 6.0 keeps matching it. + +**If your application has a listener that redirects 403 responses to a login page, +exclude this exception from it** — otherwise a refusal is sent straight back to the +provider that refused it, rebuilding the loop from your own side. + +## Chain the cause in your authenticator + +When your `authenticate()` catches a bundle exception and raises Symfony's, pass the +original as `previous`: + +```php +} catch (OpenIdConnectExceptionInterface $exception) { + throw new CustomUserMessageAuthenticationException($exception->getMessage(), previous: $exception); +} +``` + +The bundle reads that cause back to decide what the user is shown. Without it the +refusal arrives as a plain 500 with the reason only in the message. This was always +the documented shape; 6.1 is the first release where dropping the cause costs you +something. + +See [ADR 004](docs/adr/004-handle-provider-error-callbacks.md) for the reasoning, and +[CHANGELOG.md](CHANGELOG.md) for the rest of the release. diff --git a/docs/adr/004-handle-provider-error-callbacks.md b/docs/adr/004-handle-provider-error-callbacks.md new file mode 100644 index 0000000..ca9c18b --- /dev/null +++ b/docs/adr/004-handle-provider-error-callbacks.md @@ -0,0 +1,145 @@ +# 004: Handle provider error callbacks + +- **Created By:** Ture Gjørup +- **Date:** 2026-08-26 +- **Decision Maker:** Ture Gjørup +- **Stakeholders:** Bundle consumers; operators of those applications; bundle + maintainers +- **Status:** Accepted + +## Context + +[RFC 6749 §4.1.2.1](https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1) +gives an authorization request two possible answers. One is the callback everyone +thinks about, carrying `code` and `state`. The other is a refusal, carrying `error` +and `state` and no `code` at all: the user declined consent, the session at the +provider had expired, the tenant policy said no. + +`OpenIdLoginAuthenticator::supports()` required both `state` and `code`, so a refusal +was not a callback. The firewall answered it as an ordinary unauthenticated request, +which means calling the entry point, which starts a fresh authorization request, +which the provider refuses again. Captured in production against Azure AD B2C: +dozens of rounds between the login route and the callback path, the browser never +settling. Nothing was logged, because no failing callback existed to log. The +one-time `oauth2state`, `oauth2nonce` and `oauth2provider` values were never +consumed, so each round replayed a session that was already half spent. + +This is the same shape as the outage behind +[ADR 002](002-fail-closed-on-authentication-failure.md), reached through the one door +that decision did not close. ADR 002 stopped a *failing* callback from being retried; +it did not help a request that was never recognised as a callback. + +## Options Considered + +1. **Recognise the error callback and end it (chosen).** The request the provider + actually sent gets an answer, the one-time values are consumed exactly as on any + other callback, and the reason reaches the log and the page. +2. **Leave `supports()` alone and count entry-point invocations in the session.** The + objections ADR 002 raised against a counter apply unchanged — it bounds the + symptom rather than removing the cause, and picks an arbitrary limit. It also + treats a refusal, which is a normal outcome, as an anomaly to be detected. +3. **Recognise it but reuse the state-mismatch failure.** Terminates the loop, but + throws away the only thing the provider told us. An expired consent grant and a + forged callback would be indistinguishable in the log and on the page, and the + status would be a 500 either way. + +## Decision + +Adopt option 1 in 6.1.0. `supports()` returns `true` on a configured callback path +when the query carries `state` and either `code` or `error`. + +- **State is checked before anything else the URL carries.** `error` and + `error_description` are chosen by whoever built the callback URL. Only a matching + state says the request belongs to a login this browser started, so nothing else in + the query is read, logged, stored or repeated back until it matches. +- **All three one-time values are consumed up front**, before any check can throw. A + callback is spent whether it succeeds, fails validation, or carries a refusal. A + value left behind is one a later request can replay. +- **An empty `error` still counts as a callback.** `supports()` tests for the + parameter's presence, not for a usable value. Anything else hands the request back + to the entry point, which mints a fresh state — and the next refusal then arrives + with a state that matches, making the loop indistinguishable from a first attempt. +- **The provider's text is sanitized before it is logged, held or shown.** Runs of + control characters collapse to a single space, input that is not valid UTF-8 is + dropped, and what remains is capped at 200 characters. +- **A distinct type, `ProviderErrorException`, carries it**, extending + `AuthenticationFailedException` so that every `catch` already written against the + bundle's login failure keeps matching. The error code is an accessor, + `getError()`, not message text a consumer would have to search for. +- **The type answers `getStatusCode()`**, mapping a refusal to 403, a provider outage + to 503, and everything else to 500. A user who clicked Cancel gets a page, not an + incident. +- **`onAuthenticationFailure()` rethrows it unwrapped.** It is already outside the + security hierarchy and carries nothing beneath it, which is what ADR 002 requires + of anything leaving there; wrapping it would only discard the status. +- **Nothing is constructed until the cheap checks pass.** Building a provider pulls + in discovery, an HTTP client and a cache pool, and a refusal has no use for any of + them, so `getProvider()` now runs after the state check rather than before it. + +## Consequences + +The loop cannot form on this shape either, and as with +[ADR 003](003-constrain-supports-to-callback-path.md) it is prevented by a type and a +path check rather than by a counter. Refusals become visible in the log at a level +that matches who is at fault: `warning` from the bundle, and a 4xx that Symfony's +`ErrorListener` records at `error` rather than `critical`. + +Accepted costs: + +- The bundle now states an HTTP status from a class under `Security/` rather than + `Controller/`. The status is metadata the kernel reads off the exception, not a + response the bundle renders, and the alternative is paging an operator every time a + user changes their mind. +- An application whose own listener redirects 403 responses to a login page can + rebuild a loop for itself. That listener is the application's, not the firewall's, + and `ProviderErrorException` is a distinct type precisely so it can be excluded. +- A callback carrying an `error` with nothing usable in it — empty, an array, or + nothing but control characters — is reported as a missing code. It still ends the + callback; it simply has no refusal to report. +- The reordering means a forged callback naming a provider that is no longer + configured is now reported as an invalid state at `warning`, where it used to be + reported as an unconfigured provider at `error`. +- The status reaches the application only if the consumer's `authenticate()` chains + the bundle exception into the `AuthenticationException` it raises, which is what + the documented subclass does. A consumer that drops the cause gets a 500 with the + reason in the message. + +## How much of what the provider says we repeat + +`error` and `error_description` arrive in a URL. On a good day the identity provider +put them there; on a bad one, anyone who can get a browser to load a link. They are +treated as input, not as a message. + +Control characters go first, because a newline in a log record forges a second +record, and an escape sequence is a command to whichever terminal someone reads the +log in. Input that is not valid UTF-8 is dropped, because the first JSON formatter to +meet it throws — replacing a legible failure with an illegible one, inside the code +that was handling a failure. What survives is capped, so no one can fill a log +pipeline with their own prose. The cap counts characters rather than bytes: cutting a +multi-byte character in half would produce exactly the invalid UTF-8 the step before +it just rejected. + +The same sanitized values, and only those, reach the exception, so nothing raw +crosses the bundle's public surface. And none of it is read at all until the state +matches. + +## References + +- [RFC 6749 §4.1.2.1](https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1) — + the error response, and [§10.12](https://datatracker.ietf.org/doc/html/rfc6749#section-10.12) + for what `state` is for +- [OpenID Connect Core 1.0 §3.1.2.6](https://openid.net/specs/openid-connect-core-1_0.html#AuthError) — + authentication error response +- [OAuth 2.0 Security Best Current Practice](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics) — + on what may be trusted in callback parameters +- [OWASP Logging Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html) — + log injection via untrusted text +- [Symfony: `HttpExceptionInterface` and error handling](https://symfony.com/doc/current/controller/error_pages.html), + and [`framework.exceptions`](https://symfony.com/doc/current/reference/configuration/framework.html#exceptions) + for pinning a status and log level per exception class +- [Microsoft Entra External ID error codes](https://learn.microsoft.com/en-us/azure/active-directory-b2c/error-codes) — + the vendor codes that make the `default` arm of the status mapping necessary +- [ADR 002](002-fail-closed-on-authentication-failure.md) — the fail-closed decision + this preserves +- [ADR 003](003-constrain-supports-to-callback-path.md) — the path constraint that + keeps the widened `supports()` from reopening issue #63 diff --git a/docs/adr/README.md b/docs/adr/README.md index 0b58831..d3d159c 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -18,3 +18,6 @@ context that drove them and the consequences we accept. See - **[003 — Treat only the configured callback path as a callback](003-constrain-supports-to-callback-path.md)** — Accepted — 2026-08-20 +- **[004 — Handle provider error + callbacks](004-handle-provider-error-callbacks.md)** — Accepted — + 2026-08-26 diff --git a/src/Exception/ProviderErrorException.php b/src/Exception/ProviderErrorException.php new file mode 100644 index 0000000..808073c --- /dev/null +++ b/src/Exception/ProviderErrorException.php @@ -0,0 +1,94 @@ +error; + } + + /** + * The provider's description of the error, sanitized, or null if it sent none usable. + */ + public function getErrorDescription(): ?string + { + return $this->errorDescription; + } + + public function getStatusCode(): int + { + return match ($this->error) { + // The user, or a policy standing in for them, said no. Nothing is + // broken and nobody needs paging. + self::ACCESS_DENIED, 'login_required', 'consent_required', + 'interaction_required', 'account_selection_required' => Response::HTTP_FORBIDDEN, + // The provider is having a bad day and says so — the same answer + // LoginController gives when it cannot reach one at all. + 'server_error', 'temporarily_unavailable' => Response::HTTP_SERVICE_UNAVAILABLE, + // Everything else says our request or our registration was wrong: + // invalid_request, unauthorized_client, invalid_scope, and whatever a + // given provider invents. That is an operator's problem, and 500 is how + // the application already reports one. + default => Response::HTTP_INTERNAL_SERVER_ERROR, + }; + } + + /** + * @return array + */ + public function getHeaders(): array + { + return []; + } +} diff --git a/src/Security/OpenIdLoginAuthenticator.php b/src/Security/OpenIdLoginAuthenticator.php index ee9cd81..0f22488 100644 --- a/src/Security/OpenIdLoginAuthenticator.php +++ b/src/Security/OpenIdLoginAuthenticator.php @@ -6,6 +6,7 @@ use ItkDev\OpenIdConnect\Exception\ValidationException; use ItkDev\OpenIdConnectBundle\EventSubscriber\AuthenticationAuditSubscriber; use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; @@ -22,8 +23,9 @@ * * A failed callback throws `AuthenticationFailedException`, which is not an * `AuthenticationException` and so is not turned back into another redirect to the - * identity provider. Consuming applications see a 500 and can render whatever they - * like from it; what they no longer see is an unbreakable redirect loop. + * identity provider. Consuming applications see an error they can render whatever + * they like from — a 403 where the provider refused, a 500 otherwise; what they no + * longer see is an unbreakable redirect loop. * * The logger is injected through `setLogger()` rather than the constructor on * purpose: this class is extended by consuming applications, whose subclasses @@ -58,6 +60,15 @@ abstract class OpenIdLoginAuthenticator extends AbstractAuthenticator implements */ public const string TARGET_PATH_SESSION_KEY = '_itkdev_oidc.target_path'; + /** + * The cap on provider-supplied text, in characters. + * + * Long enough for a real `error_description` — Azure AD B2C's run to a line or + * two — and short enough that a log pipeline cannot be filled with somebody + * else's prose. + */ + private const int PROVIDER_TEXT_MAX_LENGTH = 200; + private LoggerInterface $logger; /** @@ -83,6 +94,17 @@ public function setLogger(LoggerInterface $logger): void * Requiring the configured callback path as well leaves a forged callback to the * firewall's ordinary handling. * + * `error` is the other half of RFC 6749 §4.1.2.1: a provider that refuses + * redirects back with `error` and `state` and no `code` at all. Left + * unrecognised, that request falls to the firewall, the firewall calls this + * authenticator's entry point, the entry point asks the provider again, and the + * provider refuses again — a loop with no failing callback anywhere in it to log. + * + * `has()`, deliberately, rather than a test for a non-empty value: `?state=…&error=` + * has to be recognised too. If it is not, the entry point fires, mints a fresh + * state, and the next refusal arrives with a state that matches — a loop that + * cannot even be told apart from a first attempt. + * * Nothing here touches the session. This runs on every request through the * firewall, so starting a session for anonymous traffic would be a real cost, and * "is this a callback" must not depend on whether this browser began the login. @@ -91,7 +113,7 @@ public function setLogger(LoggerInterface $logger): void */ public function supports(Request $request): ?bool { - if (!$request->query->has('state') || !$request->query->has('code')) { + if (!$request->query->has('state') || (!$request->query->has('code') && !$request->query->has('error'))) { return false; } @@ -176,15 +198,68 @@ protected function getSupportedProviderKeys(): array protected function validateClaims(Request $request): array { $session = $request->getSession(); + + // Every one-time value is spent here, before anything below can throw. A + // callback is used up whether it succeeds, fails validation, or carries the + // provider's refusal, and a value left behind is one a later request can + // replay. $providerKey = $session->remove('oauth2provider'); $providerKey = is_string($providerKey) ? $providerKey : ''; + $oauth2state = $session->remove('oauth2state'); + $oauth2nonce = $session->remove('oauth2nonce'); // The session entry is removed above, so carry the provider key on the // request for anything downstream that needs to attribute this login — // the audit subscriber in particular, which sees only the security event. $request->attributes->set(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE, $providerKey); + // Read as an array rather than through InputBag::get(): `?state[]=x` makes + // that throw Symfony's BadRequestException, and a method whose whole job is + // to end a callback cleanly should not be the place a framework exception + // escapes from. A non-string simply fails the comparison below. + $query = $request->query->all(); + $state = $query['state'] ?? null; + + // Make sure state and oauth2state are the same. First, and before anything + // else in this URL is read: until the state matches, none of it is known to + // belong to a login this browser started, and the rest is whatever the + // sender chose to put there. + if (!is_string($oauth2state) || '' === $oauth2state || !is_string($state) || !hash_equals($oauth2state, $state)) { + $this->logger->warning('OIDC login failed: invalid state', ['provider' => $providerKey]); + + throw new ValidationException('Invalid state'); + } + + // RFC 6749 §4.1.2.1: a refusal comes back as `error` with no `code`. Handled + // here rather than left to the token exchange, which would report a missing + // code and drop the only thing that says why the login did not happen. + $error = self::sanitizeProviderText($query['error'] ?? null); + + if (null !== $error) { + $errorDescription = self::sanitizeProviderText($query['error_description'] ?? null); + + // Warning, not error: much the commonest cause is a user who decided + // not to log in, and there is nothing for an operator to fix. The + // status code on the exception carries the distinction that matters. + $this->logger->warning('OIDC login failed: the identity provider refused the request', [ + 'provider' => $providerKey, + 'error' => $error, + 'error_description' => $errorDescription, + ]); + + throw new ProviderErrorException($error, $errorDescription); + } + + if (!is_string($oauth2nonce) || '' === $oauth2nonce) { + $this->logger->warning('OIDC login failed: nonce empty or not found', ['provider' => $providerKey]); + + throw new ValidationException('Nonce empty or not found'); + } + try { + // Built last, and only for a callback that has passed every check that + // costs nothing: constructing a provider pulls in discovery, an HTTP + // client and a cache pool, none of which a refusal has any use for. $provider = $this->providerManager->getProvider($providerKey); } catch (OpenIdConnectExceptionInterface $exception) { // A callback whose session lost (or never had) the provider key @@ -198,22 +273,6 @@ protected function validateClaims(Request $request): array throw $exception; } - // Make sure state and oauth2state are the same - $oauth2state = $session->remove('oauth2state'); - - if ($request->query->get('state') !== $oauth2state) { - $this->logger->warning('OIDC login failed: invalid state', ['provider' => $providerKey]); - - throw new ValidationException('Invalid state'); - } - - $oauth2nonce = $session->remove('oauth2nonce'); - if (!is_string($oauth2nonce) || '' === $oauth2nonce) { - $this->logger->warning('OIDC login failed: nonce empty or not found', ['provider' => $providerKey]); - - throw new ValidationException('Nonce empty or not found'); - } - try { $code = $request->query->get('code'); @@ -257,7 +316,63 @@ public function onAuthenticationFailure(Request $request, AuthenticationExceptio // original exception, validateClaims() logged the specific reason, and the // application's error handling logs whatever escapes. A record here would be // the fourth for one failure. - throw new AuthenticationFailedException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), self::causeOutsideSecurity($exception)); + $cause = self::causeOutsideSecurity($exception); + + // A provider that refused already says why, and in what terms the + // application should answer. Wrapping it would replace a 403 the user + // caused by clicking Cancel with a 500 somebody gets paged for. It is + // already outside the security hierarchy and carries nothing beneath it, + // which is exactly what a type leaving here has to be. + if ($cause instanceof ProviderErrorException) { + throw $cause; + } + + throw new AuthenticationFailedException(sprintf('Error occurred validating openid login: %s', $exception->getMessage()), $exception->getCode(), $cause); + } + + /** + * Provider-supplied text, made fit to appear in a log record. + * + * `error` and `error_description` are chosen by whoever built the callback URL: + * the identity provider on a good day, anyone who can get a browser to load a + * URL on a bad one. So the text is treated as input rather than as a message. + * Control characters go, because a newline is a forged second log record and an + * escape sequence is a command to whichever terminal the log is read in; text + * that is not valid UTF-8 goes, because the first JSON formatter to meet it + * throws and replaces a legible failure with an illegible one; what is left is + * capped. + * + * Null for anything unusable — absent, empty, an array (`?error[]=x`), or + * nothing but control characters — so the caller can tell "no error" from "an + * error with nothing in it" in one check. + */ + private static function sanitizeProviderText(mixed $value): ?string + { + if (!is_string($value)) { + return null; + } + + // A run of control characters becomes one space rather than nothing, so a + // two-line description reads as two words and not as one. No /u here: this + // is a byte-level strip that has to work on input that is not valid UTF-8. + $text = trim(preg_replace('/[[:cntrl:]]+/', ' ', $value) ?? ''); + + // Capped in characters, not bytes: a byte cap can cut a multi-byte character + // in half, and half a character is exactly the invalid UTF-8 this is trying + // not to produce. `.` under /u is one code point, and the pattern is + // anchored and greedy, so this is the first N of them. + // + // The /u carries the UTF-8 validity check too: given input that is not valid + // UTF-8 the match fails outright and leaves $matches empty, which is the + // same "unusable" answer. That matters because the first JSON formatter to + // meet invalid UTF-8 throws, and replaces a legible failure with an + // illegible one. An empty $text matches empty and comes out null the same + // way, so there is nothing to check for separately. + $matches = []; + preg_match('/^.{0,'.self::PROVIDER_TEXT_MAX_LENGTH.'}/u', $text, $matches); + $capped = $matches[0] ?? ''; + + return '' === $capped ? null : $capped; } /** diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index 3e140fd..d5db498 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -8,6 +8,7 @@ use ItkDev\OpenIdConnectBundle\Exception\CacheException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; use ItkDev\OpenIdConnectBundle\Exception\TokenNotFoundException; use ItkDev\OpenIdConnectBundle\Exception\UsernameDoesNotExistException; use PHPUnit\Framework\Attributes\DataProvider; @@ -41,6 +42,29 @@ public static function concreteProvider(): iterable yield 'CacheException' => [CacheException::class, \RuntimeException::class]; yield 'TokenNotFoundException' => [TokenNotFoundException::class, \RuntimeException::class]; yield 'AuthenticationFailedException' => [AuthenticationFailedException::class, \RuntimeException::class]; + yield 'ProviderErrorException' => [ProviderErrorException::class, \RuntimeException::class]; + } + + /** + * A new concrete extends an existing one so that every `catch` already written + * against the older type keeps matching it — the SemVer commitment that lets a + * subtype ship in a minor. + * + * @return iterable, class-string<\Throwable>}> + */ + public static function subtypeProvider(): iterable + { + yield 'ProviderErrorException' => [ProviderErrorException::class, AuthenticationFailedException::class]; + } + + /** + * @param class-string<\Throwable> $concrete + * @param class-string<\Throwable> $existing + */ + #[DataProvider('subtypeProvider')] + public function testANewSubtypeExtendsAnExistingConcrete(string $concrete, string $existing): void + { + $this->assertInstanceOf($existing, new $concrete('test')); } /** diff --git a/tests/Exception/ProviderErrorExceptionTest.php b/tests/Exception/ProviderErrorExceptionTest.php new file mode 100644 index 0000000..5e33208 --- /dev/null +++ b/tests/Exception/ProviderErrorExceptionTest.php @@ -0,0 +1,87 @@ + + */ + public static function statusCodeProvider(): iterable + { + // The user, or a policy standing in for them, said no. + yield 'access_denied' => [ProviderErrorException::ACCESS_DENIED, Response::HTTP_FORBIDDEN]; + yield 'login_required' => ['login_required', Response::HTTP_FORBIDDEN]; + yield 'consent_required' => ['consent_required', Response::HTTP_FORBIDDEN]; + yield 'interaction_required' => ['interaction_required', Response::HTTP_FORBIDDEN]; + yield 'account_selection_required' => ['account_selection_required', Response::HTTP_FORBIDDEN]; + + // The provider is having a bad day and says so. + yield 'server_error' => ['server_error', Response::HTTP_SERVICE_UNAVAILABLE]; + yield 'temporarily_unavailable' => ['temporarily_unavailable', Response::HTTP_SERVICE_UNAVAILABLE]; + + // Our request or our registration is wrong. + yield 'invalid_request' => ['invalid_request', Response::HTTP_INTERNAL_SERVER_ERROR]; + yield 'unauthorized_client' => ['unauthorized_client', Response::HTTP_INTERNAL_SERVER_ERROR]; + yield 'invalid_scope' => ['invalid_scope', Response::HTTP_INTERNAL_SERVER_ERROR]; + // Providers invent their own; Azure AD B2C's policy errors look like this. + yield 'a vendor code nobody has a rule for' => ['AADB2C90118', Response::HTTP_INTERNAL_SERVER_ERROR]; + } + + #[DataProvider('statusCodeProvider')] + public function testTheStatusCodeSaysWhoIsAtFault(string $error, int $expected): void + { + $this->assertSame($expected, (new ProviderErrorException($error))->getStatusCode()); + } + + public function testTheMessageCarriesBothHalves(): void + { + $exception = new ProviderErrorException('access_denied', 'User cancelled'); + + $this->assertSame('The identity provider refused the request: access_denied (User cancelled)', $exception->getMessage()); + $this->assertSame('access_denied', $exception->getError()); + $this->assertSame('User cancelled', $exception->getErrorDescription()); + } + + public function testTheMessageWithoutADescription(): void + { + $exception = new ProviderErrorException('access_denied'); + + $this->assertSame('The identity provider refused the request: access_denied', $exception->getMessage()); + $this->assertNull($exception->getErrorDescription()); + } + + /** + * Nothing here has a meaningful numeric code, and a consumer switching on one + * would be reading a value the provider never sent. + */ + public function testTheCodeIsZero(): void + { + $this->assertSame(0, (new ProviderErrorException('access_denied'))->getCode()); + } + + public function testACauseIsKept(): void + { + $cause = new \RuntimeException('underneath'); + + $this->assertSame($cause, (new ProviderErrorException('access_denied', null, $cause))->getPrevious()); + } + + /** + * Nothing here warrants a `WWW-Authenticate` or a `Retry-After`: the browser is + * being shown a page, not asked to try again on its own. + */ + public function testNoHeadersAreImposed(): void + { + $this->assertSame([], (new ProviderErrorException('access_denied'))->getHeaders()); + } +} diff --git a/tests/Security/OpenIdLoginAuthenticatorTest.php b/tests/Security/OpenIdLoginAuthenticatorTest.php index 20e05ba..7eece0e 100644 --- a/tests/Security/OpenIdLoginAuthenticatorTest.php +++ b/tests/Security/OpenIdLoginAuthenticatorTest.php @@ -10,6 +10,7 @@ use ItkDev\OpenIdConnectBundle\Exception\AuthenticationFailedException; use ItkDev\OpenIdConnectBundle\Exception\InvalidProviderException; use ItkDev\OpenIdConnectBundle\Exception\OpenIdConnectBundleExceptionInterface; +use ItkDev\OpenIdConnectBundle\Exception\ProviderErrorException; use ItkDev\OpenIdConnectBundle\Security\OpenIdConfigurationProviderManager; use ItkDev\OpenIdConnectBundle\Security\OpenIdLoginAuthenticator; use ItkDev\OpenIdConnectBundle\Tests\TestLogger; @@ -150,6 +151,8 @@ public static function incompleteCallbackProvider(): iterable yield 'neither' => [[]]; yield 'state only' => [['state' => 'abcd']]; yield 'code only' => [['code' => 'xyz']]; + yield 'error only' => [['error' => 'access_denied']]; + yield 'error and code, no state' => [['error' => 'access_denied', 'code' => 'xyz']]; } #[DataProvider('incompleteCallbackProvider')] @@ -160,6 +163,34 @@ public function testTheRightPathAloneIsNotACallback(array $query): void $this->assertFalse($authenticator->supports(Request::create('/callback_uri?'.http_build_query($query)))); } + /** + * A refusal is a callback too, and the path is still what decides. RFC 6749 + * §4.1.2.1 sends `error` with `state` and no `code`. + */ + #[DataProvider('callbackPathProvider')] + public function testAnErrorCallbackIsAlsoACallback(string $path, bool $expected): void + { + $authenticator = $this->authenticatorWithPaths([ + 'test_provider_1' => '/callback_uri', + 'test_provider_2' => '/other_callback', + ]); + + $this->assertSame($expected, $authenticator->supports(Request::create($path.'?state=abcd&error=access_denied'))); + } + + /** + * The one that keeps the loop closed. A provider is entitled to send an empty + * `error`, and if that is not recognised as a callback the firewall answers it, + * the entry point mints a fresh state, and the next refusal comes back with a + * state that matches — a loop indistinguishable from a first attempt. + */ + public function testAnEmptyErrorIsStillACallback(): void + { + $authenticator = $this->authenticatorWithPaths(['test_provider_1' => '/callback_uri']); + + $this->assertTrue($authenticator->supports(Request::create('/callback_uri?state=abcd&error='))); + } + /** * A subclass bound to one provider does not answer another provider's callback, * which is what lets one authenticator per provider share a firewall. @@ -503,6 +534,292 @@ private function assertValidationExceptionMessage(OpenIdLoginAuthenticator $auth $this->fail(sprintf('Expected ValidationException "%s"', $expectedMessage)); } + public function testAProviderErrorIsReportedWithItsCode(): void + { + $request = new Request(query: [ + 'state' => 'test_state', + 'error' => 'access_denied', + 'error_description' => 'User cancelled', + ]); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (\Throwable $thrown) { + // ADR 002: nothing the security component will catch and answer with + // another trip to the identity provider. Asserted before the type is + // narrowed, or it holds statically and proves nothing. + $this->assertNotInstanceOf(AuthenticationException::class, $thrown); + $this->assertNull($thrown->getPrevious(), 'Nothing beneath it for the ExceptionListener to find'); + + $this->assertInstanceOf(ProviderErrorException::class, $thrown); + $this->assertSame('access_denied', $thrown->getError()); + $this->assertSame('User cancelled', $thrown->getErrorDescription()); + + $record = $this->logger->singleRecord(); + $this->assertSame(LogLevel::WARNING, $record['level'], 'A user who changed their mind is not an operator problem'); + $this->assertStringContainsString('refused the request', $record['message']); + $this->assertSame('test_provider_1', $record['context']['provider'] ?? null); + $this->assertSame('access_denied', $record['context']['error'] ?? null); + $this->assertSame('User cancelled', $record['context']['error_description'] ?? null); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + + public function testAProviderErrorWithNoDescriptionReportsNull(): void + { + $request = new Request(query: ['state' => 'test_state', 'error' => 'access_denied']); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (ProviderErrorException $thrown) { + $this->assertNull($thrown->getErrorDescription()); + $this->assertArrayHasKey('error_description', $this->logger->singleRecord()['context']); + $this->assertNull($this->logger->singleRecord()['context']['error_description']); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + + /** + * Constructing a provider pulls in discovery, an HTTP client and a cache pool. + * A refusal has no use for any of it. + */ + public function testAProviderErrorNeverBuildsAProvider(): void + { + $mockManager = $this->createMock(OpenIdConfigurationProviderManager::class); + $mockManager->expects($this->never())->method('getProvider'); + + $authenticator = new TestAuthenticator($mockManager); + $authenticator->setLogger($this->logger); + + $request = new Request(query: ['state' => 'test_state', 'error' => 'access_denied']); + $this->setSessionOnRequest($request); + + $this->expectException(ProviderErrorException::class); + $authenticator->authenticate($request); + } + + /** + * `error` and `error_description` are chosen by whoever built the callback URL. + * Until the state matches, nothing in it is known to belong to a login this + * browser started, so none of it is read, logged or repeated back. + */ + public function testAForgedStateHidesTheProvidersErrorText(): void + { + $request = new Request(query: [ + 'state' => 'wrong_test_state', + 'error' => 'access_denied', + 'error_description' => "attacker\ntext", + ]); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (\Throwable $thrown) { + $this->assertNotInstanceOf(ProviderErrorException::class, $thrown); + $this->assertInstanceOf(ValidationException::class, $thrown); + $this->assertSame('Invalid state', $thrown->getMessage()); + + $record = $this->logger->singleRecord(); + $this->assertSame(LogLevel::WARNING, $record['level']); + $this->assertStringContainsString('invalid state', $record['message']); + + $logged = json_encode($this->logger->records); + $this->assertIsString($logged); + $this->assertStringNotContainsString('access_denied', $logged, 'Nothing the sender wrote reaches the log'); + $this->assertStringNotContainsString('attacker', $logged); + + return; + } + $this->fail('Expected ValidationException'); + } + + /** + * @return iterable}> + */ + public static function oneTimeConsumptionProvider(): iterable + { + yield 'provider error' => ['test_state', ['error' => 'access_denied']]; + yield 'invalid state' => ['wrong_test_state', []]; + yield 'missing code' => ['test_state', []]; + } + + /** + * A value left in the session is one a later request can replay, so a callback + * is spent whatever becomes of it. + * + * @param array $extraQuery + */ + #[DataProvider('oneTimeConsumptionProvider')] + public function testEveryOneTimeSessionValueIsConsumed(string $state, array $extraQuery): void + { + $request = new Request(query: ['state' => $state] + $extraQuery); + $session = $this->realSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (\Throwable) { + // The failure itself is asserted elsewhere; what matters here is what + // the session no longer holds. + } + + $this->assertFalse($session->has('oauth2provider')); + $this->assertFalse($session->has('oauth2state')); + $this->assertFalse($session->has('oauth2nonce')); + } + + /** + * @return iterable + */ + public static function unusableProviderErrorProvider(): iterable + { + yield 'empty' => ['']; + yield 'an array' => [['access_denied']]; + yield 'nothing but control characters' => ["\n\t"]; + yield 'not valid UTF-8' => ["\xC3\x28"]; + } + + /** + * An `error` with nothing usable in it is not reported as a refusal — there + * would be nothing to report — but it still ends the callback rather than + * handing it back to the entry point. + */ + #[DataProvider('unusableProviderErrorProvider')] + public function testAnUnusableProviderErrorFallsThroughToTheMissingCodeFailure(mixed $error): void + { + $request = new Request(query: ['state' => 'test_state', 'error' => $error]); + $this->setSessionOnRequest($request); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Missing or invalid code'); + $this->authenticator->authenticate($request); + } + + public function testTheProvidersErrorTextIsCappedAndCleanedBeforeItIsLogged(): void + { + $request = new Request(query: [ + 'state' => 'test_state', + 'error' => 'access_denied', + 'error_description' => "line one\r\nline two\x1b[31m".str_repeat('a', 10000), + ]); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (ProviderErrorException $thrown) { + $logged = $this->logger->singleRecord()['context']['error_description'] ?? null; + $this->assertIsString($logged); + // Exact, not <=: an off-by-one in the cap has to fail here. + $this->assertSame(200, mb_strlen($logged)); + $this->assertSame(0, preg_match('/[[:cntrl:]]/', $logged), 'No forged log records, no terminal escapes'); + $this->assertStringContainsString('line one line two', $logged, 'A run of control characters reads as one space'); + $this->assertSame($logged, $thrown->getErrorDescription(), 'One sanitized value, two consumers'); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + + public function testCleanProviderTextIsPassedThroughUnchanged(): void + { + $request = new Request(query: [ + 'state' => 'test_state', + 'error' => 'access_denied', + 'error_description' => 'Consent was not granted', + ]); + $this->setSessionOnRequest($request); + + try { + $this->authenticator->authenticate($request); + } catch (ProviderErrorException $thrown) { + $this->assertSame('Consent was not granted', $thrown->getErrorDescription()); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + + /** + * `?state[]=x` makes `InputBag::get()` throw Symfony's `BadRequestException`. + * A method whose whole job is to end a callback cleanly is not where a + * framework exception should escape from. + */ + public function testAnArrayStateIsRejectedAsAnInvalidState(): void + { + $request = new Request(query: ['state' => ['test_state'], 'code' => 'test_code']); + $this->setSessionOnRequest($request); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Invalid state'); + $this->authenticator->authenticate($request); + } + + /** + * @return iterable + */ + public static function unusableStoredStateProvider(): iterable + { + yield 'never stored' => [null]; + yield 'stored empty' => ['']; + yield 'not a string' => [['test_state']]; + } + + /** + * Without the guard, an empty stored state and an empty query state compare + * equal and the callback passes. The safety is local here, not emergent from + * whatever the controller happened to write. + */ + #[DataProvider('unusableStoredStateProvider')] + public function testAnUnusableStoredStateIsAnInvalidState(mixed $stored): void + { + $request = new Request(query: ['state' => '', 'code' => 'test_code']); + $stubSession = $this->createStub(SessionInterface::class); + $stubSession->method('remove')->willReturnMap([ + ['oauth2provider', 'test_provider_1'], + ['oauth2state', $stored], + ['oauth2nonce', 'test_nonce'], + ]); + $request->setSession($stubSession); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Invalid state'); + $this->authenticator->authenticate($request); + } + + /** + * A refusal already says why, and in what terms the application should answer. + * Wrapping it would replace a 403 the user caused by clicking Cancel with a 500 + * somebody gets paged for. + */ + public function testTheProviderErrorLeavesOnAuthenticationFailureUnwrapped(): void + { + $providerError = new ProviderErrorException('access_denied'); + + try { + $this->authenticator->onAuthenticationFailure( + new Request(), + new AuthenticationException('sanitised', 0, $providerError), + ); + } catch (\Throwable $thrown) { + $this->assertNotInstanceOf(AuthenticationException::class, $thrown); + // The SemVer promise: everything already catching the bundle's login + // failure keeps catching this one. Both asserted before assertSame() + // below narrows the type and makes them hold statically. + $this->assertInstanceOf(AuthenticationFailedException::class, $thrown); + $this->assertSame($providerError, $thrown, 'Rethrown as it stands, not rebuilt'); + $this->assertSame(403, $thrown->getStatusCode()); + $this->assertSame([], $this->logger->records, 'Already logged in validateClaims()'); + + return; + } + $this->fail('Expected ProviderErrorException'); + } + private function setSessionOnRequest(Request $request, ?string $nonce = 'test_nonce'): void { $stubSession = $this->createStub(SessionInterface::class); @@ -516,6 +833,21 @@ private function setSessionOnRequest(Request $request, ?string $nonce = 'test_no $request->setSession($stubSession); } + /** + * A real session where the stub above will not do: asserting that a one-time + * value is gone needs a `has()` that answers truthfully. + */ + private function realSessionOnRequest(Request $request): Session + { + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2provider', 'test_provider_1'); + $session->set('oauth2state', 'test_state'); + $session->set('oauth2nonce', 'test_nonce'); + $request->setSession($session); + + return $session; + } + /** * The property above is deliberately typed as the abstract class, so the fixture * method exposing the protected helper needs a concrete local. diff --git a/tests/Security/ProviderErrorCallbackDoesNotLoopTest.php b/tests/Security/ProviderErrorCallbackDoesNotLoopTest.php new file mode 100644 index 0000000..589646d --- /dev/null +++ b/tests/Security/ProviderErrorCallbackDoesNotLoopTest.php @@ -0,0 +1,213 @@ +captureExceptionHandler(); + $this->kernel = new ItkDevOpenIdConnectBundleTestingKernel([ + __DIR__.'/../config/framework.yml', + __DIR__.'/../config/framework_routing.yml', + __DIR__.'/../config/security_consumer.yml', + __DIR__.'/../config/itkdev_openid_connect.yml', + ]); + $this->kernel->boot(); + } + + protected function tearDown(): void + { + $this->restoreExceptionHandlers(); + } + + /** + * A refusal that belongs to the login this browser started: the state matches. + */ + private function refusedCallback(string $query = 'state=the-real-state&error=access_denied&error_description=User+cancelled'): Request + { + $request = Request::create('/callback_uri?'.$query); + $request->setSession($this->startedLogin()); + + return $request; + } + + private function startedLogin(): Session + { + $session = new Session(new MockArraySessionStorage()); + $session->set('oauth2provider', 'test_provider_1'); + $session->set('oauth2state', 'the-real-state'); + $session->set('oauth2nonce', 'the-real-nonce'); + + return $session; + } + + /** + * Guards against the whole test passing vacuously, exactly as the sibling does: + * an unroutable path or a firewall that does not match gives a non-redirect too. + */ + private function assertTheAuthenticatorHandledTheCallback(Request $request): void + { + $this->assertSame( + 'test_provider_1', + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'The request never reached validateClaims(), so this test proves nothing.' + ); + } + + /** + * The assertion the production capture is about. A redirect here is the loop. + */ + public function testAProviderErrorCallbackIsNotAnsweredWithARedirect(): void + { + $request = $this->refusedCallback(); + $response = $this->kernel->handle($request, catch: true); + + $this->assertTheAuthenticatorHandledTheCallback($request); + $this->assertNull( + $response->headers->get('Location'), + 'A refused login was answered with a redirect: the firewall re-entered its entry point and the loop is back.' + ); + $this->assertSame( + Response::HTTP_FORBIDDEN, + $response->getStatusCode(), + 'A user who declined to log in has not caused a server error.' + ); + } + + public function testAProviderErrorConsumesTheOneTimeSessionValues(): void + { + $request = $this->refusedCallback(); + $session = $request->getSession(); + + $this->kernel->handle($request, catch: true); + + $this->assertTheAuthenticatorHandledTheCallback($request); + $this->assertFalse($session->has('oauth2provider')); + $this->assertFalse($session->has('oauth2state')); + $this->assertFalse($session->has('oauth2nonce'), 'The nonce is unread on this path but still spent'); + } + + /** + * The type has to survive the trip through the firewall intact, or the status + * and the error code never reach the application that renders the page. + */ + public function testTheProviderErrorReachesTheApplicationIntact(): void + { + $request = $this->refusedCallback(); + + try { + $this->kernel->handle($request, catch: false); + $this->fail('A refused login should not be handled silently.'); + } catch (ProviderErrorException $exception) { + $this->assertTheAuthenticatorHandledTheCallback($request); + $this->assertSame('access_denied', $exception->getError()); + $this->assertSame('User cancelled', $exception->getErrorDescription()); + $this->assertSame(Response::HTTP_FORBIDDEN, $exception->getStatusCode()); + + // The security ExceptionListener walks the whole chain, so a single + // AuthenticationException anywhere beneath this would rebuild the loop. + for ($cause = $exception; null !== $cause; $cause = $cause->getPrevious()) { + $this->assertNotInstanceOf(AuthenticationException::class, $cause); + } + } + } + + /** + * A forged callback carries whatever text its sender chose. State is checked + * before any of it is read, so none of it reaches the log, the exception or the + * page — and the refusal is not reported as one. + */ + public function testAForgedStateOnAnErrorCallbackTellsTheUserNothingTheAttackerWrote(): void + { + $request = $this->refusedCallback('state=does-not-match&error=access_denied&error_description=Call+0800+SCAM'); + + try { + $this->kernel->handle($request, catch: false); + $this->fail('A forged callback should not be handled silently.'); + } catch (AuthenticationFailedException $exception) { + $this->assertTheAuthenticatorHandledTheCallback($request); + $this->assertNotInstanceOf(ProviderErrorException::class, $exception); + $this->assertStringContainsString('Invalid state', $exception->getMessage()); + $this->assertStringNotContainsString('access_denied', $exception->getMessage()); + $this->assertStringNotContainsString('SCAM', $exception->getMessage()); + } + + // Still terminal, and still spent — on a fresh request, since the one above + // consumed its session. + $forged = $this->refusedCallback('state=does-not-match&error=access_denied&error_description=Call+0800+SCAM'); + $session = $forged->getSession(); + $response = $this->kernel->handle($forged, catch: true); + + $this->assertNull($response->headers->get('Location')); + $this->assertSame(Response::HTTP_INTERNAL_SERVER_ERROR, $response->getStatusCode()); + $this->assertFalse($session->has('oauth2state')); + } + + /** + * Issue #63 restated in the new query shape: widening `supports()` must not put + * back the ability to turn any page under the firewall into a failed login. + */ + public function testAnErrorCallbackOnAStrayPathIsLeftToTheFirewall(): void + { + $request = Request::create('/protected?state=forged&error=access_denied'); + $request->setSession(new Session(new MockArraySessionStorage())); + + $response = $this->kernel->handle($request, catch: true); + + $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); + $this->assertSame(ConsumerAuthenticator::LOGIN_PATH, $response->headers->get('Location')); + $this->assertNull( + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'validateClaims() ran, so the authenticator accepted a callback on a path that is not one' + ); + } + + /** + * `state` on its own is still not a callback. The callback path is outside + * `access_control`, so the route answers it and the authenticator stays out. + */ + public function testACallbackWithNeitherCodeNorErrorIsNotACallback(): void + { + $request = Request::create('/callback_uri?state=the-real-state'); + $request->setSession($this->startedLogin()); + + $response = $this->kernel->handle($request, catch: true); + + $this->assertSame(Response::HTTP_OK, $response->getStatusCode()); + $this->assertNull( + $request->attributes->get(AuthenticationAuditSubscriber::PROVIDER_ATTRIBUTE), + 'validateClaims() ran for a request carrying neither a code nor an error' + ); + } +}