Support a caller-supplied redirect on /reset-remember-wayf (#2097)

* 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.

* 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.

* 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.

* 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.

* 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.

* 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.

Author: kayjoosten

CHANGELOG.md

@@ -26,6 +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 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:
 

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

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:

docs/wayf_remember_choice.md

@@ -105,6 +105,23 @@ 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 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
+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.

src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php

@@ -27,13 +27,15 @@
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Routing\Attribute\Route;
+use Uri\WhatWg\Url;
 
 final class ResetRememberedWayfController
 {
     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 +58,49 @@ 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;
+        }
+
+        return $this->validatedRedirect($redirect) ?? $this->redirectUrl;
+    }
+
+    private function validatedRedirect(string $redirect): ?string
+    {
+        $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;
+        }
+
+        return $url->toAsciiString();
+    }
+
+    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;
     }
 }

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')
         );
     }