Support a caller-supplied redirect on /reset-remember-wayf

[kayjoosten]

What

Extends GET /reset-remember-wayf (introduced in #2085 / #2094) so it
accepts an optional redirect query parameter, and always appends a
wayfReset=removed|none query parameter to whichever redirect target is
used. This is a prerequisite for
OpenConext/OpenConext-profile#345,
which wants to link from Profile's "my personal data" page to this
endpoint and land the user back on that same page afterwards, knowing
whether a cookie was actually cleared.

Stacked on #2094

This branches from wayf-reset-endpoint (#2094, not yet merged), since it
directly extends ResetRememberedWayfController added there. The diff
shown here is the incremental change only; once #2094 merges to main,
this PR's base should be retargeted to main.

Details

• 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
  extracted into small private methods, so __invoke 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 Add endpoint to remove "remember my choice" cookie #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.

Testing

All run inside the Docker dev container (PHP 8.5):

• phpmd / phpcs / phpcs-legacy / docheader: clean.
• eb4: 244/244. unit: 1008/1008 (1003 baseline + 5 new). functional
  (APP_ENV=test): 123/123 (121 baseline + 2 new). integration:
  105/105.
• Behat default suite: 300/300 scenarios, 5557/5557 steps (unchanged —
  no new scenarios needed, this is pure PHPUnit-covered logic).
• Twig lint: 114/114.

Refs: OpenConext/OpenConext-profile#345

[johanib]

Looks good.

I think AI still found a valid looking case that seems to reproduce.
Seems good to close that hole and apply the basic sanitization.

[johanib]

Reject browser-normalized backslashes before allowlisting

parse_url() reports https://evil.example.org\@profile.example.org/ as having the allowed host profile.example.org, so this branch returns it. Browsers normalize the backslash to / and navigate to evil.example.org, allowing an attacker to bypass the host allowlist and turn the endpoint into an open redirect.

Relevant lines: /home/johan/project/surf/OpenConext-engineblock/.worktrees/pr-review-2097/src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php:74-88

Suggested approach: Use browser-compatible URL parsing and canonicalization, or reject backslashes and other ambiguous authority characters before comparing the host; only return the canonical URL after validation.

[kayjoosten]

Fixed in ff0404b. Values containing a backslash, whitespace or control characters are rejected before parse_url(), URLs with userinfo are rejected, and only https is allowed. Unit tests cover the backslash bypass from this comment, userinfo, tab/newline, a leading space, protocol-relative URLs and a lookalike host. Docs and changelog are updated.

[johanib]

The /reset-remember-wayf endpoint accepts a redirect.

Is it really interesting for the reader to know that it only accepts https? Or, should it instead say what it is used for?

Please rewrite so system operators know what to do and what its for.

[johanib]

Thanks, the feedback is addressed. https-only, the rejection of backslash/whitespace/control characters and userinfo, and the tests all look good. I also tried a few more bypasses (https://evil.com#@allowed/, ?@, [::1]@) and they're rejected correctly.

One non-blocking suggestion: since we require PHP 8.5, we could use the native WHATWG URL parser (Uri\WhatWg\Url) instead of a regex plus parse_url(). It follows the same spec browsers use, so the host we check is the host the browser navigates to. That means we no longer have to anticipate parser differences ourselves.

private function isAllowedRedirect(string $redirect): ?string
{
    $url = \Uri\WhatWg\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();
}

Quick check against the earlier cases:

• https://evil.example.org\@profile.example.org/ gives host evil.example.org, so it's rejected.
• https://profile.example.org\t.evil.com/ gives host profile.example.org.evil.com, so it's rejected.
• https://PROFILE.Example.ORG/ gives host profile.example.org, so it's allowed and canonicalised.

Returning toAsciiString() means we redirect to exactly the URL we validated. It differs from the current code in two ways: it accepts a few odd inputs the browser would also accept (such as a leading space or https:\\host), and it normalises them.

I didn't find a Symfony component for this. The Validator Url constraint has no host allowlist, and HtmlSanitizer's UrlSanitizer isn't installed and lets subdomains through.

Fine to merge as is. Your call whether to switch.

[johanib]

2 small issues, if fixed, good to merge! 👍