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' + ); + } +}