diff --git a/CHANGELOG.md b/CHANGELOG.md index f7f76d49c..3e0ec8283 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 `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. ## 7.2.1 diff --git a/config/packages/parameters.yml.dist b/config/packages/parameters.yml.dist index 29a845f09..980551dbc 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -200,6 +200,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/docs/wayf_remember_choice.md b/docs/wayf_remember_choice.md index 330f37851..86aaa3c42 100644 --- a/docs/wayf_remember_choice.md +++ b/docs/wayf_remember_choice.md @@ -105,6 +105,22 @@ 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 `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`. 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 +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. diff --git a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php index 8dae9e830..313e9c0a8 100644 --- a/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php +++ b/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php @@ -34,6 +34,7 @@ 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 +57,54 @@ 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 === '' || !$this->isAllowedRedirect($redirect)) { + return $this->redirectUrl; + } + + return $redirect; + } + + 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; + } + + $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); + } + + 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) . $fragment; } } 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..b53d77ed3 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php @@ -28,6 +28,7 @@ use OpenConext\EngineBlock\Service\Wayf\RememberedIdpCookie; use OpenConext\EngineBlockBundle\Controller\ResetRememberedWayfController; use Phake; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -43,6 +44,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 +68,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 +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()); Phake::verify($cookieService)->clearCookieWithSameSite(Phake::anyParameters()); } @@ -113,7 +115,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 +131,179 @@ 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_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 + { + $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()); + } + + #[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(Phake::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( @@ -142,9 +317,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); }