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..9f8a716c4 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,47 @@ 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 + { + $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..9be881712 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,141 @@ 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()); + } + private function buildRememberedIdpCookie(CookieService $cookieService): RememberedIdpCookie { return new RememberedIdpCookie( @@ -142,9 +278,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); }