feat!: accept a callback only on the provider's callback path - #66
feat!: accept a callback only on the provider's callback path#66turegjorup wants to merge 6 commits into
Conversation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #66 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 143 178 +35
===========================================
Files 14 14
Lines 637 730 +93
===========================================
+ Hits 637 730 +93
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:
|
…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.
|
Findings
The documented claim "a proxy sending X-Forwarded-Prefix with trusted proxies configured needs no callback_path — the path then already matches" is exactly backwards. With the header trusted, the redirect_uri-derived path is /prefix/auth/callback while getPathInfo() stays /auth/callback — still a mismatch, and now the redirect_route derivation also mismatches because generation picks up the prefix. The plan made this same claim (A2), so the bug was inherited, not introduced. Suggested fix: compare derived paths against $request->getBaseUrl().$request->getPathInfo() (normalized) instead of pathInfo alone. That makes every case line up: root deployments unchanged, subdirectory deployments work, the X-Forwarded-Prefix claim becomes true, and the no-header rewriting proxy still needs callback_path as designed. One residual to note in the ADR: redirect_route derivation is memoized with the first request's RequestContext, so mixed proxied/direct traffic with differing base URLs can't be satisfied by one frozen value — a limitation worth a sentence.
|
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.
|
All five fixed. Finding 1 was right in every particular, and I verified each mechanic in the installed source before changing anything:
So both derivations carried the prefix while the comparison basis did not, and the README claimed the reverse of what happened. Now comparing
Covered by On the memoization limitation you flagged: rather than document it, The other four:
Separately, on 273 tests, 100% lines, 100% covered MSI (409 killed, none escaped), PHPStan clean, green on framework-bundle 8.x and 6.4.13. |
2 participants
Implements the plan for issue #63. Stays in the unreleased 6.0, which is being held.
Changes
supports()requiresstate,codeand a path matching one of this authenticator's providers.OpenIdConfigurationProviderManager::getRedirectUriPaths()derives the path per provider fromcallback_path, the generatedredirect_route(asABSOLUTE_PATH), or the path ofredirect_uri— normalised, and memoized.callback_path, for proxies that rewrite the path.redirect_uri,redirect_routeorcallback_path(compile-time).getSupportedProviderKeys()to narrow an authenticator to its own providers; defaults to all, so existing multi-authenticator firewalls are untouched.createTargetPathRedirect()for returning the user to the page that sent them to log in (Part B).?target_path=on the login route, so a login link on a public page can name where to return to.Why
stateandcodealone made every URL behind the firewall a callback. Combined with #62's fail-closed behaviour, an unauthenticated caller could raise a 500 on any URL by appending two query parameters. Now a forged callback is the firewall's business again — an entry-point redirect — while a real callback that fails still fails closed.Decisions the plan left open
Hard compile-time requirement for a callback target per provider, per the plan's recommendation. A provider without one can never recognise a callback, and "matches every path" is the defect being removed.
redirect_routegenerated asABSOLUTE_PATH, so a route's host or scheme requirements do not enter the comparison. Noted in the ADR as unsupported.No logging from
supports(): it runs pre-authentication on every request, so a log call there amplifies anyone sending traffic.?target_path=on the login controller is included after all. The plan deferred it, but neither stated obstacle was one: the firewall-name problem disappears if the value lives under a bundle-private session key thatcreateTargetPathRedirect()already reads, and the open-redirect risk is validation, which is the work rather than a reason to postpone. A page the firewall denied takes precedence, since that is what the user was actually stopped from reaching, and a value that is not a path within the application is dropped with awarning— one leading/, no backslash, no://, no control characters.Deep links to protected pages never needed it: the firewall saves the requested page when the entry point fires, and
testADeepLinkSurvivesTheLoginRoundTrippins the whole trip.?target_path=covers only the case where nothing was denied, so nothing was saved.Validated on
devops_itksitesInstalled from disk, artifact verified in
vendor/first:redirect_uri{"azure_az":"/openid-connect/generic"}500— reachesvalidateClaims(), still fails closed/admin302 -> /openidconnect/login/azure_az— the #63 fixcache:cleardev + prod?target_path=/admin/server?target_path=https://evil.example.org/phish?target_path=//evil.example.org?target_path=/admin\evilOn the risk of a silent break, since this is the failure mode I flagged when scoping #63: a
redirect_urithat is not a real URL derives a path nothing matches, so callbacks would stop being recognised. Checked what that actually does — with the placeholder value, the authorize request sendsredirect_uri=AZURE_AZ_APP_REDIRECT_URIto the identity provider, which rejects it, so login was already broken in that deployment before this change. The genuine new exposure is a rewriting proxy, whereredirect_uriis valid and registered but the internal path differs;callback_pathis for exactly that and the upgrade note says so.Tests
241 tests, 100% lines, 100% covered MSI (367 killed, none escaped), PHPStan clean, green on both ends of the supported range (framework-bundle 8.x and 6.4.13).
Worth calling out two fixture changes:
FailedCallbackDoesNotLoopTestpointed at/protected?state=…&code=…, which is no longer a callback, so the loop assertions would have passed vacuously. Its own guard caught that — it is retargeted at/callback_uri, which needed a route so the request reaches the firewall.security_consumer.ymlgainedaccess_control, without which nothing demanded authentication, no entry point fired, and the two new tests asserted against a 200.