feat: send a PKCE challenge, and stop holding providers between requests

[turegjorup]

Wires PKCE on itk-dev/openid-connect 5.1, and removes the one place the bundle carried request data between requests.

Changes

• PKCE (RFC 7636, S256), on by default. The login route generates a verifier, keeps it in the session under oauth2pkce_verifier, and sends only its challenge. The authenticator redeems the code with it.
• New per-provider pkce option, defaulting to true, and OpenIdConfigurationProviderManager::isPkceEnabled().
• The verifier joins the one-time session values consumed on every callback — success, failure and refusal alike. It is written on every login even when PKCE is off, so a verifier abandoned by an earlier login cannot be redeemed against this one's code.
• getProvider() returns a fresh provider each call; the Guzzle client is cached per provider instead.
• itk-dev/openid-connect bumped to ^5.1.
• README section, upgrade notes, changelog.

Why PKCE is on by default

RFC 6749 §3.1 requires an authorization server to ignore parameters it does not recognise, so a provider that has never heard of PKCE behaves exactly as before, and one that does gets the protection with no configuration. pkce: false is there for a provider that rejects unknown parameters rather than ignoring them.

Nonce validation already covered RFC 9700's code-injection countermeasure for a confidential client, so this is defence in depth rather than a fix — but it also protects the code in transit, and it is what every provider we target expects now.

Why providers are no longer held

AbstractProvider::getAuthorizationParameters() assigns $this->state on every getAuthorizationUrl() call (league/oauth2-client, AbstractProvider.php:432), not only when the caller omits state. So a memoized provider always ends a request holding that request's state.

Nothing reads it today. That stops being a safe thing to rely on the moment a process outlives a request, which is the point of worker mode — so the instance goes.

The HTTP client is what gets kept instead, which is the part actually worth keeping: it owns the connection pool, so a token exchange still does not renegotiate TLS on every login. It holds no request data and its options never change. Constructing the provider around it is cheap — discovery and JWKS are lazy and cached in the PSR-6 pool.

Building the client here means applying league's own filter, since league builds one per provider instance and we no longer keep those. AbstractProvider::getAllowedClientOptions() allows timeout and proxy always and verify only alongside a proxy — its rule that TLS verification may be turned off for a proxy and nowhere else. testVerifyIsNotForwardedWithoutAProxy pins that, and testProviderCredentialsNeverReachTheHttpClient pins that nothing else in a provider's configuration reaches Guzzle's request options.

The rest of the worker-mode audit

Every non-readonly property on every service in src/, and what it holds:

Service	State	Verdict
OpenIdConfigurationProviderManager::$providers	provider instances carrying league's $state	removed
OpenIdConfigurationProviderManager::$redirectUriPaths	strings derived from config + routing base URL	kept — a pure function of its key, and the key set is bounded by the base URLs an application answers on
OpenIdConfigurationProviderManager::$httpClients	Guzzle clients	added — configuration and a connection pool, no request data
OpenIdLoginAuthenticator::$logger	set once at container build	fine

Everything else in src/ is readonly or stateless. ClientSecretExpiry has public mutable properties but is a DTO, not a service.

One residue is left, and it is the library's to fix: OpenIdConfigurationProvider::generateState() still assigns $this->state. It is harmless now that we do not hold the instance, but a stateless generateState() — or one that only returns — would let a consumer hold a provider safely. Worth an issue on the library if worker mode is going ahead.

Tests

344 tests, 904 assertions.

• Controller: challenge sent and verifier kept; pkce: false sends neither; a stale verifier is overwritten when PKCE is off.
• Authenticator: the stored verifier reaches getIdToken(); a missing or non-string one sends no code_verifier; the verifier is consumed on every callback.
• Manager: fresh instance per call; no state survives onto the next provider; the client is reused; each provider gets its own; the two client-option filter tests above.
• Config: pkce defaults on, and can be turned off.

Check	Result
PHPUnit	344 tests, 904 assertions, OK
PHPStan max	No errors
php-cs-fixer, markdownlint, composer	Clean
Infection	444 killed, 1 escaped
Matrix, all 6 combinations	344 pass on each, all resolving 5.1.0

The escaped mutant is the pre-existing one in ItkDevOpenIdConnectExtension.php:114, untouched here. As before, task test:matrix cannot run on my machine — composer's parallel extraction races in the matrix containers — so I ran each combination with install and phpunit in one invocation. 8.4-lowest and 8.5-lowest report deprecations from the downgraded dev tooling, the artefact .github/workflows/php.yaml:47 already documents.

Note for the maintainer

CLAUDE.md still describes supports() as triggering on state + code, and now also predates PKCE and the provider-construction change. Flagging rather than editing, as last time.

Not done

No devops_itksites validation. PKCE against real Azure AD B2C is worth confirming before release — it is the one change here that alters what goes over the wire to the identity provider.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.74%. Comparing base (4f36faa) to head (9691244).

Files with missing lines	Patch %	Lines
...rc/Security/OpenIdConfigurationProviderManager.php	86.66%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop      #77      +/-   ##
=============================================
- Coverage     100.00%   99.74%   -0.26%     
- Complexity       195      201       +6     
=============================================
  Files             15       15              
  Lines            774      799      +25     
=============================================
+ Hits             774      797      +23     
- Misses             0        2       +2     

Flag	Coverage Δ
unittests	99.74% <93.93%> (-0.26%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.