Skip to content

Commit 51cb8a0

Browse files
committed
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.
1 parent 048e2bd commit 51cb8a0

2 files changed

Lines changed: 33 additions & 1 deletion

File tree

‎src/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfController.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,16 @@ private function resolveRedirectTarget(?string $redirect): string
9090

9191
private function appendQueryParameter(string $url, string $key, string $value): string
9292
{
93+
$fragment = '';
94+
$hashPosition = strpos($url, '#');
95+
96+
if ($hashPosition !== false) {
97+
$fragment = substr($url, $hashPosition);
98+
$url = substr($url, 0, $hashPosition);
99+
}
100+
93101
$separator = str_contains($url, '?') ? '&' : '?';
94102

95-
return $url . $separator . $key . '=' . rawurlencode($value);
103+
return $url . $separator . $key . '=' . rawurlencode($value) . $fragment;
96104
}
97105
}

‎tests/unit/OpenConext/EngineBlockBundle/Controller/ResetRememberedWayfControllerTest.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,30 @@ public function a_redirect_parameter_that_already_has_a_query_string_is_appended
178178
);
179179
}
180180

181+
#[Test]
182+
public function a_redirect_parameter_with_a_fragment_keeps_the_fragment_after_the_query_string(): void
183+
{
184+
$cookieService = Phake::mock(CookieService::class);
185+
$rememberedIdpCookie = $this->buildRememberedIdpCookie($cookieService);
186+
187+
$logger = Mockery::mock(LoggerInterface::class);
188+
$logger->shouldNotReceive('info');
189+
190+
$controller = new ResetRememberedWayfController(
191+
$rememberedIdpCookie,
192+
$logger,
193+
self::REDIRECT_URL,
194+
self::ALLOWED_REDIRECT_HOSTS,
195+
);
196+
197+
$response = $controller($this->buildRequest(null, 'https://profile.example.org/my-profile#login-methods'));
198+
199+
$this->assertSame(
200+
'https://profile.example.org/my-profile?wayfReset=none#login-methods',
201+
$response->getTargetUrl()
202+
);
203+
}
204+
181205
#[Test]
182206
public function a_redirect_parameter_pointing_at_a_host_that_is_not_allowed_falls_back_to_the_configured_redirect(): void
183207
{

0 commit comments

Comments
 (0)