From 7d8b45a7aa55fce988430dce4ae14717c517fc1d Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Wed, 16 Sep 2026 14:45:40 +0200 Subject: [PATCH 1/6] 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 3e87245d6..27515549e 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -194,6 +194,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 8b4e3cffe..318878261 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php @@ -42,6 +42,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 @@ -75,7 +76,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()); } #[Test] @@ -96,7 +97,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()); } #[Test] @@ -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()); } #[Test] @@ -128,6 +129,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 = Mockery::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 = Mockery::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 = Mockery::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 = Mockery::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 = Mockery::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( @@ -141,9 +253,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 bf93512b053013d620d7746b5c441f3461258a28 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Tue, 22 Sep 2026 15:27:55 +0200 Subject: [PATCH 2/6] 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 318878261..98c526868 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php @@ -177,6 +177,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 { From d160d114e2061195e0dfe6b6e699d07cb7e0ed5e Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Thu, 8 Oct 2026 10:46:53 +0200 Subject: [PATCH 3/6] Document the redirect query parameter of the reset endpoint # Why is this change needed? Prior to this change, the redirect query parameter, the allowed redirect hosts and the wayfReset result parameter of /reset-remember-wayf were only described in parameters.yml.dist. Operators need to know about the open redirect protection and integrators about the result parameter. # How does it address the issue? This change extends the reset section in docs/wayf_remember_choice.md and adds an entry to the CHANGELOG. --- CHANGELOG.md | 4 ++++ docs/wayf_remember_choice.md | 14 ++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f277ef4c2..6a50909a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,10 @@ Features: * Added the endpoint `/reset-remember-wayf`, which removes the per-SP `rememberedidps` cookie and redirects to the URL configured in `wayf.reset_choice_per_idp_redirect`. This new parameter must not be empty. See `docs/wayf_remember_choice.md`. +* The `/reset-remember-wayf` endpoint accepts a `redirect` query parameter. Only hosts listed in the new parameter + `wayf.reset_choice_allowed_redirect_hosts` are accepted, other values fall back to + `wayf.reset_choice_per_idp_redirect`. The endpoint also appends `wayfReset=removed` or `wayfReset=none` to the + redirect URL. Updated: diff --git a/docs/wayf_remember_choice.md b/docs/wayf_remember_choice.md index 330f37851..7d6f9cf1f 100644 --- a/docs/wayf_remember_choice.md +++ b/docs/wayf_remember_choice.md @@ -105,6 +105,20 @@ The endpoint removes the `rememberedidps` cookie and redirects the user (HTTP 30 The endpoint only accepts `GET` requests and needs no authentication, because it only clears a cookie in the user's own browser. It does nothing when the cookie is not present. The `rememberchoice` cookie of the global mode is not touched. +The caller can choose where the user lands with the `redirect` query parameter, for example +`/reset-remember-wayf?redirect=https://profile.dev.openconext.local/my-choices`. To prevent an open redirect, the +`redirect` value is only used when it is an absolute `http` or `https` URL whose host is listed in: + + # Hosts that may be used in the redirect query parameter + wayf.reset_choice_allowed_redirect_hosts: + - profile.dev.openconext.local + +A missing, malformed or unlisted `redirect` falls back to `wayf.reset_choice_per_idp_redirect`. The value in `parameters.yml.dist` is the local +development host. Replace it with the hosts of your own deployment. + +The endpoint appends the query parameter `wayfReset` to the final URL, before any fragment. Its value is `removed` when +a cookie was removed and `none` when there was nothing to remove, so the target page can show a matching message. + ## Switching modes Cookies written in one mode are not read in the other. After switching, users have to make their choice once more. From ef5f51099a72de6ecb7d5c1c71739300264dd263 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Thu, 8 Oct 2026 14:46:40 +0200 Subject: [PATCH 4/6] Harden the redirect validation of the reset endpoint Only accept https redirects and reject backslashes, control characters, whitespace and userinfo in the redirect query parameter. Prior to this change, the redirect value was only checked with parse_url() against the host allowlist. Browsers normalise a value such as https://evil.example.org\@profile.example.org/ differently from parse_url(), so the host check could pass while the browser navigated to another host (open redirect). Plain http was also accepted without a need for it. This change rejects values containing a backslash, control character or whitespace, values with credentials, and any scheme other than https. Unit tests cover each of these cases and the docs and changelog describe the stricter rules. Review feedback on the redirect parameter pull request. --- CHANGELOG.md | 4 +- docs/wayf_remember_choice.md | 6 ++- .../ResetRememberedWayfController.php | 27 +++++++----- .../ResetRememberedWayfControllerTest.php | 41 ++++++++++++++++++- 4 files changed, 62 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a50909a6..b1ea0029b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,8 +26,8 @@ Features: * Added the endpoint `/reset-remember-wayf`, which removes the per-SP `rememberedidps` cookie and redirects to the URL configured in `wayf.reset_choice_per_idp_redirect`. This new parameter must not be empty. See `docs/wayf_remember_choice.md`. -* The `/reset-remember-wayf` endpoint accepts a `redirect` query parameter. Only hosts listed in the new parameter - `wayf.reset_choice_allowed_redirect_hosts` are accepted, other values fall back to +* The `/reset-remember-wayf` endpoint accepts a `redirect` query parameter. Only `https` URLs with a host listed in the + new parameter `wayf.reset_choice_allowed_redirect_hosts` are accepted, other values fall back to `wayf.reset_choice_per_idp_redirect`. The endpoint also appends `wayfReset=removed` or `wayfReset=none` to the redirect URL. diff --git a/docs/wayf_remember_choice.md b/docs/wayf_remember_choice.md index 7d6f9cf1f..86aaa3c42 100644 --- a/docs/wayf_remember_choice.md +++ b/docs/wayf_remember_choice.md @@ -107,13 +107,15 @@ browser. It does nothing when the cookie is not present. The `rememberchoice` co The caller can choose where the user lands with the `redirect` query parameter, for example `/reset-remember-wayf?redirect=https://profile.dev.openconext.local/my-choices`. To prevent an open redirect, the -`redirect` value is only used when it is an absolute `http` or `https` URL whose host is listed in: +`redirect` value is only used when it is an absolute `https` URL whose host is listed in: # Hosts that may be used in the redirect query parameter wayf.reset_choice_allowed_redirect_hosts: - profile.dev.openconext.local -A missing, malformed or unlisted `redirect` falls back to `wayf.reset_choice_per_idp_redirect`. The value in `parameters.yml.dist` is the local +A missing, malformed or unlisted `redirect` falls back to `wayf.reset_choice_per_idp_redirect`. Plain `http` URLs, URLs +with credentials (`user@host`), and URLs containing backslashes, whitespace or control characters are rejected as well, +because browsers interpret those differently from the host check. The value in `parameters.yml.dist` is the local development host. Replace it with the hosts of your own deployment. The endpoint appends the query parameter `wayfReset` to the final URL, before any fragment. Its value is `removed` when diff --git a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php index 9f8a716c4..313e9c0a8 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php @@ -30,8 +30,6 @@ final class ResetRememberedWayfController { - private const ALLOWED_REDIRECT_SCHEMES = ['http', 'https']; - public function __construct( private readonly RememberedIdpCookie $rememberedIdpCookie, private readonly LoggerInterface $logger, @@ -67,25 +65,32 @@ public function __invoke(Request $request): RedirectResponse private function resolveRedirectTarget(?string $redirect): string { - if ($redirect === null || $redirect === '') { + if ($redirect === null || $redirect === '' || !$this->isAllowedRedirect($redirect)) { return $this->redirectUrl; } - $parts = parse_url($redirect); + return $redirect; + } - if ($parts === false || !isset($parts['scheme'], $parts['host'])) { - return $this->redirectUrl; + private function isAllowedRedirect(string $redirect): bool + { + // Browsers normalise backslashes and drop control characters and whitespace, parse_url() does not. + // Such a value could pass the host check here and still send the browser to another host. + if (preg_match('/[\x00-\x20\x7f\\\\]/', $redirect) === 1) { + return false; } - if (!in_array($parts['scheme'], self::ALLOWED_REDIRECT_SCHEMES, true)) { - return $this->redirectUrl; + $parts = parse_url($redirect); + + if ($parts === false || ($parts['scheme'] ?? null) !== 'https' || !isset($parts['host'])) { + return false; } - if (!in_array($parts['host'], $this->allowedRedirectHosts, true)) { - return $this->redirectUrl; + if (isset($parts['user']) || isset($parts['pass'])) { + return false; } - return $redirect; + return in_array($parts['host'], $this->allowedRedirectHosts, true); } private function appendQueryParameter(string $url, string $key, string $value): string diff --git a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php index 98c526868..904ceb32d 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php @@ -27,6 +27,7 @@ use OpenConext\EngineBlock\Service\TimeProvider\TimeProvider; use OpenConext\EngineBlock\Service\Wayf\RememberedIdpCookie; use OpenConext\EngineBlockBundle\Controller\ResetRememberedWayfController; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -180,7 +181,7 @@ 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); + $cookieService = Mockery::mock(CookieService::class); $rememberedIdpCookie = $this->buildRememberedIdpCookie($cookieService); $logger = Mockery::mock(LoggerInterface::class); @@ -264,6 +265,44 @@ public function a_redirect_parameter_with_a_disallowed_scheme_falls_back_to_the_ $this->assertSame(self::REDIRECT_URL . '?wayfReset=none', $response->getTargetUrl()); } + #[Test] + #[DataProvider('unsafeRedirects')] + public function an_unsafe_redirect_parameter_falls_back_to_the_configured_redirect(string $redirect): void + { + $logger = Mockery::mock(LoggerInterface::class); + $logger->shouldNotReceive('info'); + + $controller = new ResetRememberedWayfController( + $this->buildRememberedIdpCookie(Mockery::mock(CookieService::class)), + $logger, + self::REDIRECT_URL, + self::ALLOWED_REDIRECT_HOSTS, + ); + + $response = $controller($this->buildRequest(null, $redirect)); + + $this->assertSame(self::REDIRECT_URL . '?wayfReset=none', $response->getTargetUrl()); + } + + /** + * @return array + */ + public static function unsafeRedirects(): array + { + return [ + 'backslash before the allowed host' => ['https://evil.example.org\\@profile.example.org/'], + 'backslash in the path' => ['https://profile.example.org\\evil.example.org/'], + 'plain http' => ['http://profile.example.org/my-profile'], + 'userinfo with the allowed host' => ['https://profile.example.org@evil.example.org/'], + 'userinfo before the allowed host' => ['https://evil.example.org@profile.example.org/'], + 'tab inside the scheme' => ["htt\tps://profile.example.org/"], + 'newline in the url' => ["https://profile.example.org/\nevil"], + 'leading space' => [' https://profile.example.org/'], + 'protocol relative url' => ['//profile.example.org/my-profile'], + 'allowed host as a subdomain of another host' => ['https://profile.example.org.evil.example.org/'], + ]; + } + private function buildRememberedIdpCookie(CookieService $cookieService): RememberedIdpCookie { return new RememberedIdpCookie( From c6e65c7efe465118ccb5c9af8d90b24c85b46595 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Mon, 12 Oct 2026 15:14:20 +0200 Subject: [PATCH 5/6] Explain what the redirect query parameter is for in the CHANGELOG # If applied, this commit will... Reword the CHANGELOG entry for the `redirect` query parameter so it explains the purpose of the parameter rather than leading with its https-only validation detail. # Why is this change needed? Prior to this change, the entry opened with the fact that only https URLs with an allowed host are accepted, without saying what the parameter is actually for. A system operator reading the CHANGELOG had no indication of why they might want to configure wayf.reset_choice_allowed_redirect_hosts. # How does it address the issue? This change leads with the purpose: callers such as Profile can send the user back to the page they came from after clearing their remembered per-SP WAYF choices, instead of always landing on the operator-configured wayf.reset_choice_per_idp_redirect. The validation rule and the wayfReset query parameter are kept as supporting detail. # Provide links to any relevant tickets, articles or other resources Addresses review comment on PR #2097. --- CHANGELOG.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1ea0029b..477bd463a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,10 +26,12 @@ Features: * Added the endpoint `/reset-remember-wayf`, which removes the per-SP `rememberedidps` cookie and redirects to the URL configured in `wayf.reset_choice_per_idp_redirect`. This new parameter must not be empty. See `docs/wayf_remember_choice.md`. -* The `/reset-remember-wayf` endpoint accepts a `redirect` query parameter. Only `https` URLs with a host listed in the - new parameter `wayf.reset_choice_allowed_redirect_hosts` are accepted, other values fall back to - `wayf.reset_choice_per_idp_redirect`. The endpoint also appends `wayfReset=removed` or `wayfReset=none` to the - redirect URL. +* The `/reset-remember-wayf` endpoint now accepts an optional `redirect` query parameter, so callers such as Profile + can send the user back to the page they came from after clearing their remembered per-SP WAYF choices, instead of + always sending them to the operator-configured `wayf.reset_choice_per_idp_redirect`. The target must be an `https` + URL whose host is listed in the new parameter `wayf.reset_choice_allowed_redirect_hosts`; any other value falls + back to `wayf.reset_choice_per_idp_redirect`. The endpoint also appends `wayfReset=removed` or `wayfReset=none` to + the redirect URL, so the caller can tell whether a cookie was actually cleared. Updated: From 0bc5296f80268965da7f3b77e4a3c44dc80ffaf1 Mon Sep 17 00:00:00 2001 From: Kay Joosten Date: Tue, 13 Oct 2026 13:38:20 +0200 Subject: [PATCH 6/6] Use the WHATWG URL parser to validate the reset redirect Prior to this change, the redirect query parameter of /reset-remember-wayf was validated with a regular expression and parse_url(). That meant anticipating every difference between parse_url() and the way browsers interpret a URL. This change parses the value with Uri\WhatWg\Url, the same specification browsers follow, so the host that is checked is the host the browser navigates to. The validated URL is returned in its normalised ASCII form and used as the redirect target. Inputs a browser would also accept, such as a leading space or a tab, are now accepted and normalised instead of rejected. --- docs/wayf_remember_choice.md | 7 ++-- .../ResetRememberedWayfController.php | 32 +++++++-------- .../ResetRememberedWayfControllerTest.php | 40 +++++++++++++++++-- 3 files changed, 54 insertions(+), 25 deletions(-) diff --git a/docs/wayf_remember_choice.md b/docs/wayf_remember_choice.md index 86aaa3c42..991a0a42a 100644 --- a/docs/wayf_remember_choice.md +++ b/docs/wayf_remember_choice.md @@ -113,9 +113,10 @@ The caller can choose where the user lands with the `redirect` query parameter, wayf.reset_choice_allowed_redirect_hosts: - profile.dev.openconext.local -A missing, malformed or unlisted `redirect` falls back to `wayf.reset_choice_per_idp_redirect`. Plain `http` URLs, URLs -with credentials (`user@host`), and URLs containing backslashes, whitespace or control characters are rejected as well, -because browsers interpret those differently from the host check. The value in `parameters.yml.dist` is the local +A missing, malformed or unlisted `redirect` falls back to `wayf.reset_choice_per_idp_redirect`. Plain `http` URLs and +URLs with credentials (`user@host`) are rejected as well. The value is parsed with the WHATWG URL parser, the same +specification browsers use, so the host that is checked is the host the browser navigates to. The endpoint redirects to +the normalised form of the URL (for example with a lowercase host). The value in `parameters.yml.dist` is the local development host. Replace it with the hosts of your own deployment. The endpoint appends the query parameter `wayfReset` to the final URL, before any fragment. Its value is `removed` when diff --git a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php index 313e9c0a8..d8634846e 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php @@ -27,6 +27,7 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\Routing\Attribute\Route; +use Uri\WhatWg\Url; final class ResetRememberedWayfController { @@ -65,32 +66,27 @@ public function __invoke(Request $request): RedirectResponse private function resolveRedirectTarget(?string $redirect): string { - if ($redirect === null || $redirect === '' || !$this->isAllowedRedirect($redirect)) { + if ($redirect === null || $redirect === '') { return $this->redirectUrl; } - return $redirect; + return $this->validatedRedirect($redirect) ?? $this->redirectUrl; } - private function isAllowedRedirect(string $redirect): bool + private function validatedRedirect(string $redirect): ?string { - // Browsers normalise backslashes and drop control characters and whitespace, parse_url() does not. - // Such a value could pass the host check here and still send the browser to another host. - if (preg_match('/[\x00-\x20\x7f\\\\]/', $redirect) === 1) { - return false; + $url = Url::parse($redirect); + + if ($url === null + || $url->getScheme() !== 'https' + || $url->getUsername() !== null + || $url->getPassword() !== null + || !in_array($url->getAsciiHost(), $this->allowedRedirectHosts, true) + ) { + return null; } - $parts = parse_url($redirect); - - if ($parts === false || ($parts['scheme'] ?? null) !== 'https' || !isset($parts['host'])) { - return false; - } - - if (isset($parts['user']) || isset($parts['pass'])) { - return false; - } - - return in_array($parts['host'], $this->allowedRedirectHosts, true); + return $url->toAsciiString(); } private function appendQueryParameter(string $url, string $key, string $value): string diff --git a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php index 904ceb32d..3a682ca73 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php @@ -291,15 +291,47 @@ public static function unsafeRedirects(): array { return [ 'backslash before the allowed host' => ['https://evil.example.org\\@profile.example.org/'], - 'backslash in the path' => ['https://profile.example.org\\evil.example.org/'], 'plain http' => ['http://profile.example.org/my-profile'], 'userinfo with the allowed host' => ['https://profile.example.org@evil.example.org/'], 'userinfo before the allowed host' => ['https://evil.example.org@profile.example.org/'], - 'tab inside the scheme' => ["htt\tps://profile.example.org/"], - 'newline in the url' => ["https://profile.example.org/\nevil"], - 'leading space' => [' https://profile.example.org/'], 'protocol relative url' => ['//profile.example.org/my-profile'], 'allowed host as a subdomain of another host' => ['https://profile.example.org.evil.example.org/'], + 'tab inside the host' => ["https://profile.example.org\t.evil.example.org/"], + 'fragment userinfo trick' => ['https://evil.example.org#@profile.example.org/'], + 'query userinfo trick' => ['https://evil.example.org?@profile.example.org/'], + 'ipv6 host with userinfo' => ['https://[::1]@profile.example.org/'], + ]; + } + + #[Test] + #[DataProvider('normalisedRedirects')] + public function a_redirect_parameter_the_browser_would_accept_is_redirected_to_in_its_normalised_form( + string $redirect, + string $expected, + ): void { + $controller = new ResetRememberedWayfController( + $this->buildRememberedIdpCookie(Mockery::mock(CookieService::class)), + Mockery::mock(LoggerInterface::class), + self::REDIRECT_URL, + self::ALLOWED_REDIRECT_HOSTS, + ); + + $response = $controller($this->buildRequest(null, $redirect)); + + $this->assertSame($expected . '?wayfReset=none', $response->getTargetUrl()); + } + + /** + * @return array + */ + public static function normalisedRedirects(): array + { + return [ + 'uppercase host' => ['https://PROFILE.Example.ORG/my-profile', 'https://profile.example.org/my-profile'], + 'leading space' => [' https://profile.example.org/', 'https://profile.example.org/'], + 'tab inside the scheme' => ["htt\tps://profile.example.org/", 'https://profile.example.org/'], + 'backslash in the path' => ['https://profile.example.org\\my-profile', 'https://profile.example.org/my-profile'], + 'newline in the path' => ["https://profile.example.org/\nevil", 'https://profile.example.org/evil'], ]; }