diff --git a/src/WorkOS/Auth/AuthKit/LoginTakeover.php b/src/WorkOS/Auth/AuthKit/LoginTakeover.php index fdecbb7..6bd71b4 100644 --- a/src/WorkOS/Auth/AuthKit/LoginTakeover.php +++ b/src/WorkOS/Auth/AuthKit/LoginTakeover.php @@ -75,6 +75,8 @@ public function unregister(): void { * @return void */ public function maybe_takeover(): void { + $this->maybe_canonicalize_query(); + if ( ! $this->should_takeover() ) { return; } @@ -196,6 +198,48 @@ private function resolve_profile(): Profile { return $this->router->resolve( $context ); } + /** + * Redirect to a canonical URL when the raw query string contains HTML + * entity-encoded separators (e.g. `&`, `&`, `&`, `&`). + * + * This fixes cases where encoded ampersands cause PHP to parse query keys + * incorrectly (breaking flows like password resets). + * + * @return void Exits via wp_safe_redirect on the canonical path; otherwise returns. + */ + private function maybe_canonicalize_query(): void { + if ( 'GET' !== SuperGlobals::get_server_var( 'REQUEST_METHOD' ) ) { + return; + } + + $raw = (string) wp_unslash( $_SERVER['QUERY_STRING'] ?? '' ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + if ( '' === $raw ) { + return; + } + if ( ! str_contains( $raw, '&' ) && ! str_contains( $raw, '&#' ) ) { + return; + } + + $canonical = strtr( + $raw, + [ + '&' => '&', + '&' => '&', + '&' => '&', + '&' => '&', + ] + ); + if ( $canonical === $raw ) { + return; + } + + $path = (string) strtok( (string) SuperGlobals::get_server_var( 'REQUEST_URI' ), '?' ); + + nocache_headers(); + wp_safe_redirect( $path . '?' . $canonical, 302 ); + exit; + } + /** * Build the renderer context from current-request state. * diff --git a/tests/wpunit/AuthKitRendererTest.php b/tests/wpunit/AuthKitRendererTest.php index 46e73bc..af13fea 100644 --- a/tests/wpunit/AuthKitRendererTest.php +++ b/tests/wpunit/AuthKitRendererTest.php @@ -676,4 +676,158 @@ public function test_takeover_does_not_forward_query_args_when_disabled(): void $this->assertSame( '/dashboard', $captured ); } + + /** + * When the raw query string contains `&` — as produced by email + * delivery chains (e.g. SafeLinks) that preserve HTML entity encoding + * from the email template — PHP mis-parses `amp;token` instead of + * `token`. maybe_canonicalize_query() must 302 to a canonical URL so + * the browser re-requests with clean separators. + */ + public function test_canonicalize_query_redirects_when_amp_entity_present(): void { + $this->bootstrap_workos_enabled(); + $this->repository->ensure_default(); + + $result = $this->run_takeover( + // PHP mis-parses & — profile and token land under wrong keys. + [ + 'workos_action' => 'reset-password', + 'amp;profile' => 'default', + 'amp;token' => 'tok_abc123', + ], + 'workos_action=reset-password&profile=default&token=tok_abc123' + ); + + $this->assertNotNull( $result['redirect'], 'A 302 to the canonical URL must be issued.' ); + $this->assertStringContainsString( '?workos_action=reset-password&profile=default&token=tok_abc123', (string) $result['redirect'] ); + $this->assertStringNotContainsString( 'amp;', (string) $result['redirect'] ); + $this->assertNull( $result['context'], 'Renderer must not run on the redirect path.' ); + } + + /** + * The numeric entity `&` produced by WordPress esc_url() in display + * context must also be canonicalised — PHP splits on the `&` inside + * `&` and the redirect path must replace it with a literal `&`. + */ + public function test_canonicalize_query_redirects_when_numeric_entity_present(): void { + $this->bootstrap_workos_enabled(); + $this->repository->ensure_default(); + + $result = $this->run_takeover( + // PHP splits & on the & — keys become #038;profile / #038;token. + [ + 'workos_action' => 'reset-password', + '#038;profile' => 'default', + '#038;token' => 'tok_xyz789', + ], + 'workos_action=reset-password&profile=default&token=tok_xyz789' + ); + + $this->assertNotNull( $result['redirect'], 'A 302 to the canonical URL must be issued.' ); + $this->assertStringContainsString( '?workos_action=reset-password&profile=default&token=tok_xyz789', (string) $result['redirect'] ); + $this->assertStringNotContainsString( '#038;', (string) $result['redirect'] ); + $this->assertNull( $result['context'], 'Renderer must not run on the redirect path.' ); + } + + /** + * A correctly-encoded URL (literal & separators) must reach reset_confirm + * without redirecting — the canonicaliser must not bounce clean requests. + */ + public function test_canonicalize_query_is_noop_for_clean_url(): void { + $this->bootstrap_workos_enabled(); + $this->repository->ensure_default(); + + $result = $this->run_takeover( + [ + 'workos_action' => 'reset-password', + 'profile' => 'default', + 'token' => 'tok_clean', + ], + 'workos_action=reset-password&profile=default&token=tok_clean' + ); + + $this->assertNull( $result['redirect'], 'No redirect must fire on a clean URL.' ); + $this->assertSame( + 'reset_confirm', + $result['context']['initial_step'] ?? null, + 'Clean URLs must still reach reset_confirm.' + ); + $this->assertSame( 'tok_clean', $result['context']['reset_token'] ?? null ); + } + + /** + * workos_action=reset-password without a token must NOT set reset_confirm — + * the gate in build_context() still applies on the canonical request. + */ + public function test_canonicalize_query_without_token_does_not_start_reset_confirm(): void { + $this->bootstrap_workos_enabled(); + $this->repository->ensure_default(); + + $result = $this->run_takeover( + [ 'workos_action' => 'reset-password' ], + 'workos_action=reset-password' + ); + + $this->assertNull( $result['redirect'] ); + $this->assertArrayNotHasKey( + 'initial_step', + $result['context'] ?? [], + 'Without a token, reset_confirm must not be set.' + ); + } + + /** + * Invoke maybe_takeover() with synthesized $_GET and $_SERVER state. Returns + * both the captured render context (when the inline-render branch is reached) + * and the redirect URL (when wp_safe_redirect is called). + * + * @param array $get Synthesized $_GET (as PHP's parser produces it). + * @param string $query_string Raw QUERY_STRING (what the server receives). + * + * @return array{context: ?array, redirect: ?string} + */ + private function run_takeover( array $get, string $query_string ): array { + $router = new ProfileRouter( $this->repository ); + $fake_renderer = new class() extends Renderer { + public ?array $captured = null; + public function render_full_page( Profile $profile, array $context = [] ): void { + $this->captured = $context; + } + }; + $takeover = new LoginTakeover( $router, $fake_renderer ); + + $redirect_url = null; + $listener = static function ( $location ) use ( &$redirect_url ): string { + $redirect_url = (string) $location; + throw new \RuntimeException( 'workos_test_redirect:' . $location ); + }; + add_filter( 'wp_redirect', $listener, 1, 1 ); + + $prev_get = $_GET; + $prev_qs = $_SERVER['QUERY_STRING'] ?? ''; + $prev_method = $_SERVER['REQUEST_METHOD'] ?? ''; + $prev_uri = $_SERVER['REQUEST_URI'] ?? ''; + + $_GET = $get; + $_SERVER['QUERY_STRING'] = $query_string; + $_SERVER['REQUEST_METHOD'] = 'GET'; + $_SERVER['REQUEST_URI'] = '/wp-login.php' . ( '' !== $query_string ? '?' . $query_string : '' ); + + try { + $takeover->maybe_takeover(); + } catch ( \RuntimeException $e ) { + $this->assertStringStartsWith( 'workos_test_redirect:', $e->getMessage() ); + } finally { + $_GET = $prev_get; + $_SERVER['QUERY_STRING'] = $prev_qs; + $_SERVER['REQUEST_METHOD'] = $prev_method; + $_SERVER['REQUEST_URI'] = $prev_uri; + remove_filter( 'wp_redirect', $listener, 1 ); + } + + return [ + 'context' => $fake_renderer->captured, + 'redirect' => $redirect_url, + ]; + } }