Release 6.0.0 - #75
Merged
Merged
Conversation
This is the fix for the outage the whole line has been building towards. The 5.1 releases made the cause visible; this stops the loop. A failed callback threw AuthenticationException, which Symfony's security ExceptionListener catches and answers by calling the firewall's entry point — for this authenticator, another redirect to the identity provider. With a live SSO session upstream the IdP returned a fresh code immediately, which failed the same way. Nothing in the cycle degraded, so nothing stopped it: nine or more rounds in twenty-five seconds, no error page, nothing logged. onAuthenticationFailure() now throws AuthenticationFailedException, a \RuntimeException implementing the bundle marker per ADR 001. AuthenticatorManager::executeAuthenticator() catches only AuthenticationException, so this propagates to HttpKernel and the application renders its own error. The loop is prevented by the type of the exception rather than by a counter, which is why the regression guard is a single assertion: the thrown type must not be an AuthenticationException. That test catches the exception as Throwable on purpose — catching the expected type first would narrow it statically and make the assertion a tautology. Deviation from the plan worth noting: CliLoginTokenAuthenticator is left alone. It is not an entry point, so its failures fall through to the OIDC entry point and redirect to normal login, which cannot loop because supports() requires a loginToken and the redirect carries none. For a single-use token that has already been consumed, the login page beats a 500. The loop needs an authenticator whose own entry point re-triggers it, and only the OIDC one has that shape. Adds ADR 002 and UPGRADE-6.0.md. The upgrade guide covers only this change; making client_secret_expires_at required and removing the deprecated exception API follow separately, and the guide gains those sections then.
ADR 002 from 172 lines to 69, the upgrade guide from 78 to 46. The decisions, rejected alternatives and consequences are unchanged; what went was restatement — a Drivers section repeating the context, prose walking through the same mechanism twice, and migration advice padded around a one-line diff.
The security ExceptionListener walks the whole $previous chain, so chaining the AuthenticationException it hands us re-entered the entry point and the loop survived the type change. Chain the first cause below it instead. Reproduced through a booted firewall, which is what a unit test on the thrown type could not show.
- warn against rendering the exception message, which now reaches consumers - make the listener example paste-runnable - note in the changelog that 6.0 is incomplete until the expiry option is required - ADR 002 to Accepted - cover a library cause that hides a security exception beneath it - say why the functional test narrows where the unit test must not
FrameworkBundle autoconfigures a setLogger() call onto every LoggerAwareInterface service, so every OpenIdLoginAuthenticator subclass gets two: the bundle's, for the configured logger, and the application logger. The last call wins, and which one that is depends on the order the bundles were registered. The conventional order puts FrameworkBundle first and the configured logger wins; register this bundle first and logging_options.logger silently does nothing — including itkdev_openid_connect.null_logger, the documented way to switch logging off. Method calls carry no priority, so a compiler pass rewrites them after the instanceof conditionals are resolved. It recognises definitions by the call the bundle put there rather than by class, since checking classes would autoload every class in a consumer's container. Also key the test kernel's cache directory on the pid. Infection substitutes a mutant through an include interceptor rather than on disk, so nothing Symfony tracks as a resource changes and mutants were served a cached container: every mutation of compile-time code survived. Three did, and are now killed.
feat!: fail closed when an OpenID Connect callback cannot be validated
References in method calls are rewritten to concrete ids by ResolveReferencesToAliasesPass, which runs in the optimization phase ahead of this pass, so comparing them against a configured alias id never matched and the pass no-oped — leaving the ordering dependency it exists to remove, silently, for anyone who configured `logger` or another alias. Emitting the alias instead would have risked a dangling reference, since private aliases are removed afterwards. Also key the test kernel's cache on the pid only under Infection. Plain runs have nothing to isolate and were paying a container recompile per process, plus a directory each that nothing cleaned up.
fix: settle which logger an authenticator receives
A missing date now fails while the container compiles instead of emitting the 5.1 deprecation. The bundle cannot warn about an expiry it does not know about, which is how a routine credential rotation became an outage. Removes with it the deprecated ItkOpenIdConnectBundleException, the never-thrown UserDoesNotExistException, and symfony/deprecation-contracts, whose only caller was the deprecation.
A required node carrying a default is contradictory. Symfony 7.4 reports it by deprecating rather than refusing, raised through trigger_deprecation() and therefore suppressed, so PHPUnit's own failOnDeprecation never sees it and Symfony 6.2's config does not check at all — it surfaced on a consuming application's console instead. Guarded by a test that installs a handler while the tree is built and asserts no deprecation, and restore the exception handler a handled request leaves behind on 6.4, which PHPUnit was reporting as risky on the prefer-lowest leg.
YAML reads an unquoted 2027-01-31 as the integer 1801353600, and the parse check only inspected strings. Such a value passed configuration, was discarded as untyped by the extension, and reached the checker as null — which returns Unknown without logging. The most natural way to write the option therefore left the provider permanently unmonitored with nothing reported anywhere, which is the outcome the option exists to prevent. An explicit null had the same effect and is rejected too. Corrects the comment that claimed the checker reports this, the README's account of which parser runs where, and narrows the trait undoing 6.4's leaked exception handler so it pops only what a test added.
The tearDown() added in the previous commit landed between failingCallback() and its docblock. The pop loop also assumed a test only ever adds handlers: one that removed the handler it inherited would leave the current handler at null against a non-null baseline, and restore_exception_handler() on an empty stack is a no-op.
The docblock claimed Symfony will not accept an environment variable on a validated node, which this PR makes plainly false: the node now carries two closures and env vars compile fine. What Symfony refuses is a validated node that also disallows empty values, which is why cannotBeEmpty() is absent — and the reason runtime reporting is still needed is that the value is an unresolved placeholder at compile time. Also widen the rejection message beyond the quoting mistake, since null and true are not quoting errors and an %env(int:...)% cast fails here with a config file that does contain a string.
feat!: require client_secret_expires_at
supports() matched state and code on any path, so every URL behind the firewall was a potential callback. Since 6.0 fails closed, that meant an unauthenticated caller could raise a 500 on any URL by appending two query parameters. Requiring the provider's configured callback path leaves a forged callback to the firewall's ordinary handling, without weakening fail-closed behaviour for real ones. Paths are derived from configuration and memoized rather than read off a provider instance: building one pulls in discovery, an HTTP client and a cache pool, and this runs on every request. Nothing consults the session — that would start one for anonymous traffic, and a lost session would put the redirect loop back. callback_path covers proxies that rewrite the path, and each provider must now declare one of redirect_uri, redirect_route or callback_path, since without one it could never recognise a callback. getSupportedProviderKeys() defaults to every provider, so existing multi-authenticator firewalls are unaffected. Also adds createTargetPathRedirect(), in the same file, for returning the user to the page that sent them to log in.
Also corrects the ADR index, which still listed 002 as Draft, and a stale parent::__construct($providerManager, $requestStack) in the README example.
…ts limits The kernel test covered only the firewall's half — that the requested page is saved. This adds the whole trip: a link to a protected page is denied, the user comes back from the identity provider, and lands on that page rather than a default. The fixture authenticator now uses createTargetPathRedirect(), as the README tells consumers to. The deep link needed a route. Routing runs before security (RouterListener at priority 32 on kernel.request, the firewall at 8), so an unrouted path is a 404 before the firewall is reached: no entry point, nothing saved, and a test that measures 404 handling instead of the round trip. Documented, since it is also the answer to why a link to a non-existent page does not come back after login.
createTargetPathRedirect() could only return users to a page the firewall had denied. A login link on a public page — a header "Log in" button — denies nothing, so there was nothing saved and those users landed on the application's fallback. They can now name the destination: ?target_path=/admin/reports. Stored under a bundle-private session key, not TargetPathTrait's, which is keyed by firewall: LoginController has no firewall name, and writing there would put a value in the firewall's own record that the firewall never saved. A page the firewall denied still wins, since that is what the user was actually stopped from reaching, and both keys are cleared on use. The value reaches a Location header, so it is validated as a path within the application — one leading slash, no backslash, no scheme separator, no control characters — and dropped with a warning otherwise. Correcting one would be guessing at intent on a security boundary.
getPathInfo() has both a subdirectory deployment's base path and any trusted X-Forwarded-Prefix stripped out of it: Request::preparePathInfo() subtracts getBaseUrlReal(), while getBaseUrl() is the trusted prefix plus that. The configured paths contain them — a redirect_uri is the URL the identity provider was given, and UrlGenerator prepends the routing context's base URL, which RequestContext::fromRequest() takes from getBaseUrl(). So comparing path info alone rejected every callback both in a subdirectory deployment and behind a prefix-announcing proxy, and the README claimed the opposite of what happened. Derived paths are now memoized per routing-context base URL rather than once, so a service seeing both proxied and direct traffic is not frozen to whichever came first. Also: an empty callback_path no longer normalizes to "/" and shadows redirect_uri; a plain login link clears a target left by an abandoned one; the expiry docblock is back on its own method; and ADR 003 documents the comparison basis rather than trailing a second decision after References.
…lback-path feat!: accept a callback only on the provider's callback path
…nterface PHPStan reports the test kernel's inherited return type on Symfony 8.1, which deprecates HttpKernel\Bundle\BundleInterface in favour of the one in DependencyInjection\Kernel. Naming either would break the other end of the supported range, so the docblock names the three bundle classes the fixture actually returns: covariant with both, and more precise than either. CI does not see this today — the phpstan job runs in the PHP 8.3 container, and symfony/http-kernel 8.1 requires PHP >= 8.4.1, so it resolves 7.4 there. Verified on 8.4 (error before, clean after), on 8.3, and on prefer-lowest, where PHPStan's 30 pre-existing errors are unchanged by this.
PHPStan assumes the PHP version it runs on when phpVersion is unset, so the analysis
only ever spoke for one point in the range composer.json declares (php ^8.3).
Measured on PHP 8.5 with 8.4-only syntax (property hooks) in an analysed file:
phpVersion min 80300 / max 80500 -> "Property hooks are supported only on PHP 8.4
and later"
phpVersion unset -> No errors
Dormant on the current job, which runs in the PHP 8.3 container: there the runtime
assumption already equals the floor, and all three configurations behave identically.
It matters the moment analysis moves to a newer PHP — which is the only way to reach
Symfony 8.1 at all, since symfony/http-kernel 8.1 requires PHP >= 8.4.1 — and it is
insurance against the container image being bumped without anyone noticing that the
floor stopped being analysed.
It does not widen the dependency axis, which is where both of this bundle's real
static-analysis findings came from. That needs a second job, noted in the config.
composer.json advertises symfony/* ^8.0, but the phpstan job ran in the default phpfpm service — PHP 8.3 — and symfony/http-kernel 8.1 requires PHP >= 8.4.1. Composer could not install the version the bundle claims to support, so it was never analysed. That is how the BundleInterface deprecation stayed invisible. Only the phpstan job moves, to phpfpm85. Bumping the default image instead would have been wrong: the unit-test matrix names `service: phpfpm` as its PHP 8.3 leg, so the matrix would have quietly lost 8.3 coverage. The floor is protected by the phpVersion range in this branch rather than by the runtime version: verified that 8.4-only syntax is flagged when analysing on 8.5 with the range and passes silently without it. Verified as CI will run it — no lock in the repository, so composer resolves fresh: PHP 8.5 with Symfony 8.1.5 gives no PHPStan errors and 273 passing tests.
ci: analyse on PHP 8.5, pinned to the declared PHP range
… guide Two changes that cannot sensibly ship apart: the guide is reordered around what a consumer actually meets first, and the expiry date it used to lead with stops being required. Merging them separately would put a requirement into develop that the next commit removes, and a guide that documents it in between. Optional again, because requiring it could force a value but never a correct one. Symfony compiles a container per environment, so a required node has to appear in all of them: putting it under when@prod compiles in prod and fails dev with "must be configured". What that produces is a date in a committed .env default, reporting ok forever while monitoring nothing — worse than the unknown it replaced, because it is invisible rather than absent. It also asked for ceremony from applications that never read it: only ClientSecretExpiryChecker consumes the value and only LoginController injects it. Unset now means the provider is not monitored and reports unknown, which monitoring can alert on, while a value that is set is still judged — a non-string is rejected at compile time and an unparseable one is reported at error. The guide had accumulated across four PRs and opened with the fail-closed change, burying the compile-time blockers behind optional material. Reordered, and corrected against three consumers: devops_itksites (5.1.1), economics (4.2.0) and display-api-service (5.1.1). They exposed a composer step that only applies to the 4.x hop, an undefined environment variable that breaks the login route for applications using the bundle's LoginController and silently removes monitoring for those that do not, two compile errors quoted inexactly, and a README that linked neither upgrade guide.
feat: keep client_secret_expires_at optional, and reorder the upgrade guide
PHPStan only ever ran against current dependencies, so the floor this bundle declares was never analysed. It had a real defect: Symfony 6.4 declares Bundle::$extension untyped, where 7.0 typed it, so getContainerExtension() returned mixed against its own ?ExtensionInterface signature. Returning a local narrows it without an annotation or an ignore, and behaviour is unchanged — the property holds null, or false for a bundle with no extension, and this bundle always has one. The job lowers only the packages `require` names, derived from composer itself rather than duplicated in the workflow, so adding a requirement cannot leave the job behind. A plain --prefer-lowest also downgrades PHPUnit and phpstan-phpunit, which produced 29 artefacts for every 1 real finding — an analysis nobody would keep green. It runs on PHP 8.3, the declared floor, as the counterpart to the phpstan job's 8.5 ceiling. Verified both ends: Symfony 6.4.13 and 8.1.5 each report no errors with 274 tests passing.
…ncies ci: analyse the dependency floor, and fix what it found
Twelve references across six workflows. codecov/codecov-action is already on v7, the current major, and no other action is used — the jobs drive docker compose directly. v7.0.0's one behavioural change is that it refuses to check out a fork's PR head under pull_request_target and workflow_run. Neither trigger appears in this repository, which uses pull_request, push and tag push, so the change does not reach us. The only input passed anywhere is fetch-depth in the changelog workflow, unchanged in v7.
ci: update actions/checkout to v7
The first command could not do what the guide asked of it. Consumers pin a major — ^4.0, ^5.0 and ^5.1.1 across the three checked — and `composer update itk-dev/openid-connect-bundle` will not cross a major, so it reports nothing to do and leaves the reader on 5.x wondering why none of the guide applies. Nothing in the document mentioned ^6.0 at all. Found by walking the guide again after the restructure, this time following it literally rather than installing the bundle from a path repository, which had hidden the step. The 4.x hop gets the same treatment: `composer require` for both packages rather than `composer update`, which was where the partial-update refusal came from in the first place.
docs: open the upgrade guide with require, not update
Collapse the unreleased entries into a 6.0.0 section stating what changed, and add the compare link. Rationale, measurements and history stay in the commits and PRs where they belong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
============================================
Coverage 100.00% 100.00%
- Complexity 125 177 +52
============================================
Files 13 14 +1
Lines 590 732 +142
============================================
+ Hits 590 732 +142
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
202 lines for four breaking changes, most of it explaining why each decision was taken. Now 80: what changed, what to do about it, and the compile error to search for. The reasoning is in the commits, the PRs and ADR 002; the features are in the README.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release branch for 6.0.0, following the same shape as #61 (5.1.1) and #59 (5.1.0).
Only change on top of
develop: the changelog's unreleased entries collapsed into a## [6.0.0] - 2026-08-25section, plus the compare link. No version field to bump — the version comes from the tag.The section states what changed and nothing else. The arguments, measurements and background that had accumulated in those entries live in the commits and PR bodies; a reader who wants them can follow the links. Entry count is unchanged; the section went from 77 lines to 34.
What 6.0.0 is
Breaking — a failed callback throws
AuthenticationFailedExceptionand escapes the firewall instead of redirecting to the identity provider again;getPrevious()is the OpenID Connect cause; a callback is recognised only on the provider's callback path; every provider must declareredirect_uri,redirect_routeorcallback_path;ItkOpenIdConnectBundleException,UserDoesNotExistExceptionandsymfony/deprecation-contractsare gone.Added —
callback_path,getSupportedProviderKeys(),createTargetPathRedirect(),?target_path=,getRedirectUriPaths()/isCallbackPath().Not breaking —
client_secret_expires_atstays optional.Verified against consumers
Three applications walked through
UPGRADE-6.0.mdagainstdevelop, none needing a code change and none needing a configuration change:devops_itksiteseconomicsdisplay-api-serviceBundle itself: 274 tests, 100% lines, 100% covered MSI, PHPStan clean at both ends of the supported dependency range.
Not exercised: a real Azure login round trip, and the staging Playwright reproduction of the original outage. The callback path has been driven as far as a live token exchange that Azure rejected on a fabricated code.
After merge
Annotated tag on the merge commit, then back-merge to
develop. The GitHub Release is created by the Action on tag push — nothing to publish by hand.