From ec7b3b9d9cf1084baa74bd07eecd234707ab63dd Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Wed, 16 Sep 2026 14:45:40 +0200 Subject: [PATCH 1/2] Support a caller-supplied redirect on /reset-remember-wayf Let /reset-remember-wayf accept an optional `redirect` query parameter so callers such as Profile can send the user back to the page they came from once their remembered per-SP WAYF choices are cleared, instead of always sending everyone to the single operator-configured URL from #2085. Every redirect (whether the caller-supplied target or the configured fallback) now also carries a `wayfReset=removed|none` query parameter, so the caller can tell whether a cookie actually existed and was cleared. - New config wayf.reset_choice_allowed_redirect_hosts is a host allowlist (defaulting to profile.dev.openconext.local in dev/CI): the `redirect` parameter is only honoured when it parses to an http(s) URL whose host appears in this list, otherwise the endpoint silently falls back to the existing wayf.reset_choice_per_idp_redirect target. This mirrors the "parse and check an allowed value" shape of the existing AllowedSchemeValidator rather than introducing a new general-purpose URL validation abstraction for a single call site. - Resolving the redirect target and appending the wayfReset signal are both extracted into small private methods so the __invoke method reads as a straight sequence: clear/log the cookie if present, resolve where to send the user, tell them what happened. - The entry-count/log-message behaviour from #2085 is unchanged: a present-but-unparseable cookie is still treated the same as a valid one for logging and for the wayfReset signal (both are cleared and reported), since from the caller's perspective the cookie no longer exists either way. - Existing unit/functional tests were updated for the new `?wayfReset=` suffix on every redirect, and new cases cover an allowed redirect host, a redirect with its own query string, a disallowed host, a malformed URL, and a disallowed scheme. Needed by OpenConext/OpenConext-profile#345, which links back to Profile's "my personal data" page after resetting a user's remembered WAYF choices. --- config/packages/parameters.yml.dist | 7 + .../services/controllers/authentication.yml | 1 + .../ResetRememberedWayfController.php | 38 +++++- .../ResetRememberedWayfControllerTest.php | 42 +++++- .../ResetRememberedWayfControllerTest.php | 123 +++++++++++++++++- 5 files changed, 203 insertions(+), 8 deletions(-) diff --git a/config/packages/parameters.yml.dist b/config/packages/parameters.yml.dist index 4400de0ae..1d2f93274 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -193,6 +193,13 @@ parameters: ## real destination for their deployment; it must never be left empty, as the endpoint ## refuses to serve requests (failing fast at construction time) when it is blank. wayf.reset_choice_per_idp_redirect: 'https://engine.dev.openconext.local/' + ## Hostnames that the /reset-remember-wayf endpoint is allowed to redirect back to via + ## its `redirect` query parameter (e.g. so Profile can send the user back to the page + ## they came from). When the query parameter is missing, malformed, or points at a host + ## that is not in this list, the endpoint falls back to + ## `wayf.reset_choice_per_idp_redirect` instead. + wayf.reset_choice_allowed_redirect_hosts: + - profile.dev.openconext.local ## Toggle the default IdP quick link banner on the WAYF. wayf.display_default_idp_banner_on_wayf: true diff --git a/config/services/controllers/authentication.yml b/config/services/controllers/authentication.yml index 73ec6fe5e..2216eafe3 100644 --- a/config/services/controllers/authentication.yml +++ b/config/services/controllers/authentication.yml @@ -79,6 +79,7 @@ services: $rememberedIdpCookie: '@OpenConext\EngineBlock\Service\Wayf\RememberedIdpCookie' $logger: '@engineblock.compat.logger' $redirectUrl: '%wayf.reset_choice_per_idp_redirect%' + $allowedRedirectHosts: '%wayf.reset_choice_allowed_redirect_hosts%' OpenConext\EngineBlock\Service\RequestAccessMailer: arguments: diff --git a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php index 8dae9e830..9c6f4df07 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php @@ -30,10 +30,13 @@ final class ResetRememberedWayfController { + private const ALLOWED_REDIRECT_SCHEMES = ['http', 'https']; + public function __construct( private readonly RememberedIdpCookie $rememberedIdpCookie, private readonly LoggerInterface $logger, private readonly string $redirectUrl, + private readonly array $allowedRedirectHosts = [], ) { if ($this->redirectUrl === '') { throw new InvalidArgumentException( @@ -56,6 +59,39 @@ public function __invoke(Request $request): RedirectResponse )); } - return new RedirectResponse($this->redirectUrl, Response::HTTP_FOUND); + $target = $this->resolveRedirectTarget($request->query->get('redirect')); + $location = $this->appendQueryParameter($target, 'wayfReset', $raw !== null ? 'removed' : 'none'); + + return new RedirectResponse($location, Response::HTTP_FOUND); + } + + private function resolveRedirectTarget(?string $redirect): string + { + if ($redirect === null || $redirect === '') { + return $this->redirectUrl; + } + + $parts = parse_url($redirect); + + if ($parts === false || !isset($parts['scheme'], $parts['host'])) { + return $this->redirectUrl; + } + + if (!in_array($parts['scheme'], self::ALLOWED_REDIRECT_SCHEMES, true)) { + return $this->redirectUrl; + } + + if (!in_array($parts['host'], $this->allowedRedirectHosts, true)) { + return $this->redirectUrl; + } + + return $redirect; + } + + private function appendQueryParameter(string $url, string $key, string $value): string + { + $separator = str_contains($url, '?') ? '&' : '?'; + + return $url . $separator . $key . '=' . rawurlencode($value); } } diff --git a/tests/functional/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php b/tests/functional/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php index fd86703c6..d67197572 100644 --- a/tests/functional/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php +++ b/tests/functional/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php @@ -46,7 +46,7 @@ public function visiting_the_endpoint_with_a_remembered_idp_cookie_clears_it_and $response = $client->getResponse(); $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); $this->assertSame( - self::getContainer()->getParameter('wayf.reset_choice_per_idp_redirect'), + self::getContainer()->getParameter('wayf.reset_choice_per_idp_redirect') . '?wayfReset=removed', $response->headers->get('Location') ); } @@ -61,7 +61,45 @@ public function visiting_the_endpoint_without_a_remembered_idp_cookie_still_redi $response = $client->getResponse(); $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); $this->assertSame( - self::getContainer()->getParameter('wayf.reset_choice_per_idp_redirect'), + self::getContainer()->getParameter('wayf.reset_choice_per_idp_redirect') . '?wayfReset=none', + $response->headers->get('Location') + ); + } + + #[Test] + public function a_redirect_parameter_pointing_at_an_allowed_host_is_honoured(): void + { + $client = self::createClient(); + + $client->request( + 'GET', + 'https://engine.dev.openconext.local/reset-remember-wayf' + . '?redirect=' . urlencode('https://profile.dev.openconext.local/my-profile') + ); + + $response = $client->getResponse(); + $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); + $this->assertSame( + 'https://profile.dev.openconext.local/my-profile?wayfReset=none', + $response->headers->get('Location') + ); + } + + #[Test] + public function a_redirect_parameter_pointing_at_a_disallowed_host_falls_back_to_the_configured_redirect(): void + { + $client = self::createClient(); + + $client->request( + 'GET', + 'https://engine.dev.openconext.local/reset-remember-wayf' + . '?redirect=' . urlencode('https://evil.example.org/phishing') + ); + + $response = $client->getResponse(); + $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); + $this->assertSame( + self::getContainer()->getParameter('wayf.reset_choice_per_idp_redirect') . '?wayfReset=none', $response->headers->get('Location') ); } diff --git a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php index 99690f8f5..c19b2e59c 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php @@ -43,6 +43,7 @@ class ResetRememberedWayfControllerTest extends TestCase private const int MAX_ENTRIES = 16; private const string COOKIE_DOMAIN = 'engine.example.org'; private const string COOKIE_PATH = '/'; + private const array ALLOWED_REDIRECT_HOSTS = ['profile.example.org']; #[Test] public function cookie_with_valid_entries_is_cleared_and_logged_with_entry_count(): void @@ -66,7 +67,7 @@ public function cookie_with_valid_entries_is_cleared_and_logged_with_entry_count $response = $controller($this->buildRequest($raw)); $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); - $this->assertSame(self::REDIRECT_URL, $response->getTargetUrl()); + $this->assertSame(self::REDIRECT_URL . '?wayfReset=removed', $response->getTargetUrl()); Phake::verify($cookieService)->clearCookieWithSameSite( RememberedIdpCookie::NAME, self::COOKIE_PATH, @@ -95,7 +96,7 @@ public function invalid_cookie_is_still_cleared_and_logged_with_zero_entries(): $response = $controller($this->buildRequest('not valid base64 or deflated data!')); $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); - $this->assertSame(self::REDIRECT_URL, $response->getTargetUrl()); + $this->assertSame(self::REDIRECT_URL . '?wayfReset=removed', $response->getTargetUrl()); Phake::verify($cookieService)->clearCookieWithSameSite(Phake::anyParameters()); } @@ -113,7 +114,7 @@ public function missing_cookie_is_not_cleared_or_logged_but_still_redirects(): v $response = $controller($this->buildRequest(null)); $this->assertSame(Response::HTTP_FOUND, $response->getStatusCode()); - $this->assertSame(self::REDIRECT_URL, $response->getTargetUrl()); + $this->assertSame(self::REDIRECT_URL . '?wayfReset=none', $response->getTargetUrl()); Phake::verifyNoInteraction($cookieService); } @@ -129,6 +130,117 @@ public function constructor_rejects_an_empty_redirect_url(): void ); } + #[Test] + public function a_redirect_parameter_pointing_at_an_allowed_host_is_honoured(): void + { + $cookieService = Phake::mock(CookieService::class); + $rememberedIdpCookie = $this->buildRememberedIdpCookie($cookieService); + + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldNotReceive('info'); + + $controller = new ResetRememberedWayfController( + $rememberedIdpCookie, + $logger, + self::REDIRECT_URL, + self::ALLOWED_REDIRECT_HOSTS, + ); + + $response = $controller($this->buildRequest(null, 'https://profile.example.org/my-profile')); + + $this->assertSame( + 'https://profile.example.org/my-profile?wayfReset=none', + $response->getTargetUrl() + ); + } + + #[Test] + public function a_redirect_parameter_that_already_has_a_query_string_is_appended_to(): void + { + $cookieService = Phake::mock(CookieService::class); + $rememberedIdpCookie = $this->buildRememberedIdpCookie($cookieService); + + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldNotReceive('info'); + + $controller = new ResetRememberedWayfController( + $rememberedIdpCookie, + $logger, + self::REDIRECT_URL, + self::ALLOWED_REDIRECT_HOSTS, + ); + + $response = $controller($this->buildRequest(null, 'https://profile.example.org/my-profile?foo=bar')); + + $this->assertSame( + 'https://profile.example.org/my-profile?foo=bar&wayfReset=none', + $response->getTargetUrl() + ); + } + + #[Test] + public function a_redirect_parameter_pointing_at_a_host_that_is_not_allowed_falls_back_to_the_configured_redirect(): void + { + $cookieService = Phake::mock(CookieService::class); + $rememberedIdpCookie = $this->buildRememberedIdpCookie($cookieService); + + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldNotReceive('info'); + + $controller = new ResetRememberedWayfController( + $rememberedIdpCookie, + $logger, + self::REDIRECT_URL, + self::ALLOWED_REDIRECT_HOSTS, + ); + + $response = $controller($this->buildRequest(null, 'https://evil.example.org/phishing')); + + $this->assertSame(self::REDIRECT_URL . '?wayfReset=none', $response->getTargetUrl()); + } + + #[Test] + public function a_malformed_redirect_parameter_falls_back_to_the_configured_redirect(): void + { + $cookieService = Phake::mock(CookieService::class); + $rememberedIdpCookie = $this->buildRememberedIdpCookie($cookieService); + + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldNotReceive('info'); + + $controller = new ResetRememberedWayfController( + $rememberedIdpCookie, + $logger, + self::REDIRECT_URL, + self::ALLOWED_REDIRECT_HOSTS, + ); + + $response = $controller($this->buildRequest(null, 'not-a-url')); + + $this->assertSame(self::REDIRECT_URL . '?wayfReset=none', $response->getTargetUrl()); + } + + #[Test] + public function a_redirect_parameter_with_a_disallowed_scheme_falls_back_to_the_configured_redirect(): void + { + $cookieService = Phake::mock(CookieService::class); + $rememberedIdpCookie = $this->buildRememberedIdpCookie($cookieService); + + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldNotReceive('info'); + + $controller = new ResetRememberedWayfController( + $rememberedIdpCookie, + $logger, + self::REDIRECT_URL, + self::ALLOWED_REDIRECT_HOSTS, + ); + + $response = $controller($this->buildRequest(null, 'javascript:alert(1)//profile.example.org')); + + $this->assertSame(self::REDIRECT_URL . '?wayfReset=none', $response->getTargetUrl()); + } + private function buildRememberedIdpCookie(CookieService $cookieService): RememberedIdpCookie { return new RememberedIdpCookie( @@ -142,9 +254,10 @@ private function buildRememberedIdpCookie(CookieService $cookieService): Remembe ); } - private function buildRequest(?string $rememberedIdpsCookie): Request + private function buildRequest(?string $rememberedIdpsCookie, ?string $redirect = null): Request { - $request = Request::create('/reset-remember-wayf'); + $query = $redirect !== null ? ['redirect' => $redirect] : []; + $request = Request::create('/reset-remember-wayf', 'GET', $query); if ($rememberedIdpsCookie !== null) { $request->cookies->set(RememberedIdpCookie::NAME, $rememberedIdpsCookie); } From d64ba788aa77d0b9718010d6148bf0a1ce59f190 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Tue, 22 Sep 2026 15:27:55 +0200 Subject: [PATCH 2/2] Keep the URL fragment after the wayfReset query parameter # If applied, this commit will Fix redirects whose target URL contains a fragment so wayfReset actually reaches the destination app as a real query parameter. # Why is this change needed? Prior to this change, appendQueryParameter() always concatenated the wayfReset parameter onto the end of the URL string. For a target URL with a fragment, that put the query parameter after the '#', where it is treated as part of the fragment by browsers and never seen as a real query parameter. # How does it address the issue? This change splits off any fragment before appending the query parameter, then re-appends the fragment at the very end, producing the conventional ...?wayfReset=...#fragment shape. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2097. --- .../ResetRememberedWayfController.php | 10 +++++++- .../ResetRememberedWayfControllerTest.php | 24 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php index 9c6f4df07..9f8a716c4 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php @@ -90,8 +90,16 @@ private function resolveRedirectTarget(?string $redirect): string private function appendQueryParameter(string $url, string $key, string $value): string { + $fragment = ''; + $hashPosition = strpos($url, '#'); + + if ($hashPosition !== false) { + $fragment = substr($url, $hashPosition); + $url = substr($url, 0, $hashPosition); + } + $separator = str_contains($url, '?') ? '&' : '?'; - return $url . $separator . $key . '=' . rawurlencode($value); + return $url . $separator . $key . '=' . rawurlencode($value) . $fragment; } } diff --git a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php index c19b2e59c..9be881712 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php @@ -178,6 +178,30 @@ public function a_redirect_parameter_that_already_has_a_query_string_is_appended ); } + #[Test] + public function a_redirect_parameter_with_a_fragment_keeps_the_fragment_after_the_query_string(): void + { + $cookieService = Phake::mock(CookieService::class); + $rememberedIdpCookie = $this->buildRememberedIdpCookie($cookieService); + + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldNotReceive('info'); + + $controller = new ResetRememberedWayfController( + $rememberedIdpCookie, + $logger, + self::REDIRECT_URL, + self::ALLOWED_REDIRECT_HOSTS, + ); + + $response = $controller($this->buildRequest(null, 'https://profile.example.org/my-profile#login-methods')); + + $this->assertSame( + 'https://profile.example.org/my-profile?wayfReset=none#login-methods', + $response->getTargetUrl() + ); + } + #[Test] public function a_redirect_parameter_pointing_at_a_host_that_is_not_allowed_falls_back_to_the_configured_redirect(): void {