Support a caller-supplied redirect on /reset-remember-wayf by kayjoosten · Pull Request #2097 · OpenConext/OpenConext-engineblock

kayjoosten · 2026-09-16T12:46:16Z

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

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

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 · 2026-09-28T08:47:34Z


 final class ResetRememberedWayfController
 {
+    private const ALLOWED_REDIRECT_SCHEMES = ['http', 'https'];


Is http needed? devconf runs on https. So just for github pipeline, or?

Not needed, you are right. Only https is accepted now (ff0404b), the unit test covers a plain http URL being rejected.

johanib · 2026-09-28T09:00:52Z

+            return $this->redirectUrl;
+        }
+
+        $parts = parse_url($redirect);


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.

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.

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

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.

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

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

# If applied, this commit will... Only accept https redirects and reject backslashes, control characters, whitespace and userinfo in the redirect query parameter. # Why is this change needed? 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. # How does it address the issue? 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. # Provide links to any relevant tickets, articles or other resources Review feedback on the redirect parameter pull request.

kayjoosten mentioned this pull request Sep 16, 2026

Add link to reset remembered WAYF login-method choices OpenConext/OpenConext-profile#353

Open

kayjoosten force-pushed the wayf-reset-endpoint branch from 3081177 to a362211 Compare September 24, 2026 07:19

kayjoosten force-pushed the wayf-reset-redirect-param branch from 924a15c to d64ba78 Compare September 24, 2026 07:21

OpenConext deleted a comment from ontmoetmid Sep 28, 2026

johanib requested changes Sep 28, 2026

View reviewed changes

kayjoosten force-pushed the wayf-reset-endpoint branch from a362211 to 0f9a300 Compare October 8, 2026 08:27

kayjoosten force-pushed the wayf-reset-redirect-param branch from d64ba78 to 51cb8a0 Compare October 8, 2026 08:27

kayjoosten added 3 commits October 8, 2026 10:46

kayjoosten force-pushed the wayf-reset-endpoint branch from 0f9a300 to a7aacbc Compare October 8, 2026 08:49

kayjoosten force-pushed the wayf-reset-redirect-param branch from 51cb8a0 to 357be6c Compare October 8, 2026 08:49

Navigation Menu

Repository navigation

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

Support a caller-supplied redirect on /reset-remember-wayf#2097
kayjoosten wants to merge 4 commits into
wayf-reset-endpointfrom
wayf-reset-redirect-param

kayjoosten commented Sep 16, 2026

Uh oh!

johanib left a comment

Uh oh!

johanib Sep 28, 2026

Uh oh!

kayjoosten Oct 8, 2026

Uh oh!

johanib Sep 28, 2026

Uh oh!

kayjoosten Oct 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kayjoosten commented Sep 16, 2026

What

Stacked on #2094

Details

Testing

Uh oh!

johanib left a comment

Choose a reason for hiding this comment

Uh oh!

johanib Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

kayjoosten Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

johanib Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

kayjoosten Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants