feat: handle provider error callbacks

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).

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

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.

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

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