From f040ac406b5992a184494c3ca0c64a7e59eb5f65 Mon Sep 17 00:00:00 2001 From: Rahula Palu Caleffi Date: Thu, 14 May 2026 20:02:21 -0300 Subject: [PATCH 1/4] fix(authkit): decode HTML entities in password reset URL after coming back from WorkOS --- src/WorkOS/Auth/AuthKit/LoginTakeover.php | 27 ++++ tests/wpunit/AuthKitRendererTest.php | 144 ++++++++++++++++++++++ 2 files changed, 171 insertions(+) diff --git a/src/WorkOS/Auth/AuthKit/LoginTakeover.php b/src/WorkOS/Auth/AuthKit/LoginTakeover.php index fdecbb7..f6b8b62 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->normalize_query_string(); + if ( ! $this->should_takeover() ) { return; } @@ -196,6 +198,31 @@ private function resolve_profile(): Profile { return $this->router->resolve( $context ); } + /** + * Re-populate $_GET when the raw query string contains HTML-encoded + * ampersands (`&` or `&`). + * + * Email delivery services (e.g. Microsoft SafeLinks) sometimes preserve + * HTML entity encoding from the email template and forward the URL to the + * browser verbatim. PHP then parses `amp;token` instead of `token`, + * leaving $_GET['token'] empty and breaking the reset-confirm flow. + * + * @return void + */ + private function normalize_query_string(): void { + $raw = $_SERVER['QUERY_STRING'] ?? ''; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + if ( $raw === '' ) { + return; + } + if ( ! str_contains( $raw, '&' ) && ! str_contains( $raw, '&' ) ) { + return; + } + $decoded = html_entity_decode( $raw, ENT_QUOTES | ENT_HTML5, 'UTF-8' ); + parse_str( $decoded, $params ); + + $_GET = array_merge( $_GET, $params ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended + } + /** * Build the renderer context from current-request state. * diff --git a/tests/wpunit/AuthKitRendererTest.php b/tests/wpunit/AuthKitRendererTest.php index 46e73bc..3201b8b 100644 --- a/tests/wpunit/AuthKitRendererTest.php +++ b/tests/wpunit/AuthKitRendererTest.php @@ -676,4 +676,148 @@ 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 services (e.g. SafeLinks) that preserve HTML entity encoding + * from the email template — PHP mis-parses `amp;token` instead of + * `token`. normalize_query_string() must repair $_GET so that the + * reset-confirm step is reached. + */ + public function test_normalize_query_string_fixes_amp_entity_so_reset_confirm_is_reached(): void { + $this->bootstrap_workos_enabled(); + $this->repository->ensure_default(); + + $context = $this->run_takeover_capturing_context( + // 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->assertSame( + 'reset_confirm', + $context['initial_step'] ?? null, + 'normalize_query_string() must repair & so reset_confirm is reached.' + ); + $this->assertSame( 'tok_abc123', $context['reset_token'] ?? null ); + } + + /** + * The numeric entity `&` produced by WordPress esc_url() in display + * context must also be normalised. PHP splits on the `&` inside `&`, + * leaving `#038;token` as the key instead of `token`. + */ + public function test_normalize_query_string_fixes_numeric_entity_so_reset_confirm_is_reached(): void { + $this->bootstrap_workos_enabled(); + $this->repository->ensure_default(); + + $context = $this->run_takeover_capturing_context( + // 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->assertSame( + 'reset_confirm', + $context['initial_step'] ?? null, + 'normalize_query_string() must repair & so reset_confirm is reached.' + ); + $this->assertSame( 'tok_xyz789', $context['reset_token'] ?? null ); + } + + /** + * A correctly-encoded URL (literal & separators) must reach reset_confirm + * unchanged — the normaliser must not corrupt clean requests. + */ + public function test_normalize_query_string_is_noop_for_clean_query_string(): void { + $this->bootstrap_workos_enabled(); + $this->repository->ensure_default(); + + $context = $this->run_takeover_capturing_context( + [ + 'workos_action' => 'reset-password', + 'profile' => 'default', + 'token' => 'tok_clean', + ], + 'workos_action=reset-password&profile=default&token=tok_clean' + ); + + $this->assertSame( + 'reset_confirm', + $context['initial_step'] ?? null, + 'Clean URLs must still reach reset_confirm.' + ); + $this->assertSame( 'tok_clean', $context['reset_token'] ?? null ); + } + + /** + * workos_action=reset-password without a token must NOT set reset_confirm + * even after normalization — the existing gate in build_context() holds. + */ + public function test_normalize_query_string_without_token_does_not_start_reset_confirm(): void { + $this->bootstrap_workos_enabled(); + $this->repository->ensure_default(); + + $context = $this->run_takeover_capturing_context( + [ 'workos_action' => 'reset-password' ], + 'workos_action=reset-password' + ); + + $this->assertArrayNotHasKey( + 'initial_step', + $context ?? [], + 'Without a token, reset_confirm must not be set regardless of normalization.' + ); + } + + /** + * Invoke maybe_takeover() with synthesized $_GET and $_SERVER['QUERY_STRING'], + * capturing the context array passed to render_full_page(). Returns the + * context when the inline-render branch is reached, null if a redirect fired. + * + * @param array $get Synthesized $_GET (as PHP's parser produces it). + * @param string $query_string Raw QUERY_STRING (what the server receives). + * + * @return array|null + */ + private function run_takeover_capturing_context( 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 ); + + $listener = static function ( $location ): string { + throw new \RuntimeException( 'workos_test_redirect:' . $location ); + }; + add_filter( 'wp_redirect', $listener, 1, 1 ); + + $prev_get = $_GET; + $prev_qs = $_SERVER['QUERY_STRING'] ?? ''; + $_GET = $get; + $_SERVER['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; + remove_filter( 'wp_redirect', $listener, 1 ); + } + + return $fake_renderer->captured; + } } From 14177af7493c86ce68c61335b35110490e3108be Mon Sep 17 00:00:00 2001 From: Rahula Palu Caleffi Date: Thu, 14 May 2026 20:30:29 -0300 Subject: [PATCH 2/4] fix(authkit): decode HTML entities - use wp_unslash when capturing server query string --- src/WorkOS/Auth/AuthKit/LoginTakeover.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/WorkOS/Auth/AuthKit/LoginTakeover.php b/src/WorkOS/Auth/AuthKit/LoginTakeover.php index f6b8b62..e82a1dc 100644 --- a/src/WorkOS/Auth/AuthKit/LoginTakeover.php +++ b/src/WorkOS/Auth/AuthKit/LoginTakeover.php @@ -210,8 +210,8 @@ private function resolve_profile(): Profile { * @return void */ private function normalize_query_string(): void { - $raw = $_SERVER['QUERY_STRING'] ?? ''; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized - if ( $raw === '' ) { + $raw = \wp_unslash( $_SERVER['QUERY_STRING'] ?? '' ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + if ( '' === $raw ) { return; } if ( ! str_contains( $raw, '&' ) && ! str_contains( $raw, '&' ) ) { @@ -219,7 +219,6 @@ private function normalize_query_string(): void { } $decoded = html_entity_decode( $raw, ENT_QUOTES | ENT_HTML5, 'UTF-8' ); parse_str( $decoded, $params ); - $_GET = array_merge( $_GET, $params ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended } From 60f54b94bc853b6a2cc7999330f132b58b2fbb70 Mon Sep 17 00:00:00 2001 From: Rahula Palu Caleffi Date: Fri, 15 May 2026 12:23:09 -0300 Subject: [PATCH 3/4] fix(authkit): decode HTML entities - enhance security measures by allowing only pre-defined keys --- src/WorkOS/Auth/AuthKit/LoginTakeover.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/WorkOS/Auth/AuthKit/LoginTakeover.php b/src/WorkOS/Auth/AuthKit/LoginTakeover.php index e82a1dc..cdd00d8 100644 --- a/src/WorkOS/Auth/AuthKit/LoginTakeover.php +++ b/src/WorkOS/Auth/AuthKit/LoginTakeover.php @@ -210,7 +210,7 @@ private function resolve_profile(): Profile { * @return void */ private function normalize_query_string(): void { - $raw = \wp_unslash( $_SERVER['QUERY_STRING'] ?? '' ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + $raw = wp_unslash( $_SERVER['QUERY_STRING'] ?? '' ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized if ( '' === $raw ) { return; } @@ -219,7 +219,14 @@ private function normalize_query_string(): void { } $decoded = html_entity_decode( $raw, ENT_QUOTES | ENT_HTML5, 'UTF-8' ); parse_str( $decoded, $params ); - $_GET = array_merge( $_GET, $params ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended + + // Restore only the keys this class uses to avoid injecting arbitrary $_GET values. + $allowed = [ 'workos_action', 'workos_profile', 'token', 'invitation_token', 'redirect_to', 'fallback' ]; + foreach ( $allowed as $key ) { + if ( isset( $params[ $key ] ) && null === SuperGlobals::get_get_var( $key ) ) { + $_GET[ $key ] = $params[ $key ]; // phpcs:ignore WordPress.Security.NonceVerification.Recommended + } + } } /** From 8d9f58c9844634d8d9701c445ea524f1063f5042 Mon Sep 17 00:00:00 2001 From: Rahula Palu Caleffi Date: Fri, 15 May 2026 13:00:44 -0300 Subject: [PATCH 4/4] fix(authkit): redirect to canonical URL when query has entity-encoded separators --- src/WorkOS/Auth/AuthKit/LoginTakeover.php | 51 ++++++----- tests/wpunit/AuthKitRendererTest.php | 100 ++++++++++++---------- 2 files changed, 86 insertions(+), 65 deletions(-) diff --git a/src/WorkOS/Auth/AuthKit/LoginTakeover.php b/src/WorkOS/Auth/AuthKit/LoginTakeover.php index cdd00d8..6bd71b4 100644 --- a/src/WorkOS/Auth/AuthKit/LoginTakeover.php +++ b/src/WorkOS/Auth/AuthKit/LoginTakeover.php @@ -75,7 +75,7 @@ public function unregister(): void { * @return void */ public function maybe_takeover(): void { - $this->normalize_query_string(); + $this->maybe_canonicalize_query(); if ( ! $this->should_takeover() ) { return; @@ -199,34 +199,45 @@ private function resolve_profile(): Profile { } /** - * Re-populate $_GET when the raw query string contains HTML-encoded - * ampersands (`&` or `&`). + * Redirect to a canonical URL when the raw query string contains HTML + * entity-encoded separators (e.g. `&`, `&`, `&`, `&`). * - * Email delivery services (e.g. Microsoft SafeLinks) sometimes preserve - * HTML entity encoding from the email template and forward the URL to the - * browser verbatim. PHP then parses `amp;token` instead of `token`, - * leaving $_GET['token'] empty and breaking the reset-confirm flow. + * This fixes cases where encoded ampersands cause PHP to parse query keys + * incorrectly (breaking flows like password resets). * - * @return void + * @return void Exits via wp_safe_redirect on the canonical path; otherwise returns. */ - private function normalize_query_string(): void { - $raw = wp_unslash( $_SERVER['QUERY_STRING'] ?? '' ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized + 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, '&' ) ) { + if ( ! str_contains( $raw, '&' ) && ! str_contains( $raw, '&#' ) ) { return; } - $decoded = html_entity_decode( $raw, ENT_QUOTES | ENT_HTML5, 'UTF-8' ); - parse_str( $decoded, $params ); - - // Restore only the keys this class uses to avoid injecting arbitrary $_GET values. - $allowed = [ 'workos_action', 'workos_profile', 'token', 'invitation_token', 'redirect_to', 'fallback' ]; - foreach ( $allowed as $key ) { - if ( isset( $params[ $key ] ) && null === SuperGlobals::get_get_var( $key ) ) { - $_GET[ $key ] = $params[ $key ]; // phpcs:ignore WordPress.Security.NonceVerification.Recommended - } + + $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; } /** diff --git a/tests/wpunit/AuthKitRendererTest.php b/tests/wpunit/AuthKitRendererTest.php index 3201b8b..af13fea 100644 --- a/tests/wpunit/AuthKitRendererTest.php +++ b/tests/wpunit/AuthKitRendererTest.php @@ -679,16 +679,16 @@ public function test_takeover_does_not_forward_query_args_when_disabled(): void /** * When the raw query string contains `&` — as produced by email - * delivery services (e.g. SafeLinks) that preserve HTML entity encoding + * delivery chains (e.g. SafeLinks) that preserve HTML entity encoding * from the email template — PHP mis-parses `amp;token` instead of - * `token`. normalize_query_string() must repair $_GET so that the - * reset-confirm step is reached. + * `token`. maybe_canonicalize_query() must 302 to a canonical URL so + * the browser re-requests with clean separators. */ - public function test_normalize_query_string_fixes_amp_entity_so_reset_confirm_is_reached(): void { + public function test_canonicalize_query_redirects_when_amp_entity_present(): void { $this->bootstrap_workos_enabled(); $this->repository->ensure_default(); - $context = $this->run_takeover_capturing_context( + $result = $this->run_takeover( // PHP mis-parses & — profile and token land under wrong keys. [ 'workos_action' => 'reset-password', @@ -698,24 +698,22 @@ public function test_normalize_query_string_fixes_amp_entity_so_reset_confirm_is 'workos_action=reset-password&profile=default&token=tok_abc123' ); - $this->assertSame( - 'reset_confirm', - $context['initial_step'] ?? null, - 'normalize_query_string() must repair & so reset_confirm is reached.' - ); - $this->assertSame( 'tok_abc123', $context['reset_token'] ?? null ); + $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 normalised. PHP splits on the `&` inside `&`, - * leaving `#038;token` as the key instead of `token`. + * context must also be canonicalised — PHP splits on the `&` inside + * `&` and the redirect path must replace it with a literal `&`. */ - public function test_normalize_query_string_fixes_numeric_entity_so_reset_confirm_is_reached(): void { + public function test_canonicalize_query_redirects_when_numeric_entity_present(): void { $this->bootstrap_workos_enabled(); $this->repository->ensure_default(); - $context = $this->run_takeover_capturing_context( + $result = $this->run_takeover( // PHP splits & on the & — keys become #038;profile / #038;token. [ 'workos_action' => 'reset-password', @@ -725,23 +723,21 @@ public function test_normalize_query_string_fixes_numeric_entity_so_reset_confir 'workos_action=reset-password&profile=default&token=tok_xyz789' ); - $this->assertSame( - 'reset_confirm', - $context['initial_step'] ?? null, - 'normalize_query_string() must repair & so reset_confirm is reached.' - ); - $this->assertSame( 'tok_xyz789', $context['reset_token'] ?? null ); + $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 - * unchanged — the normaliser must not corrupt clean requests. + * without redirecting — the canonicaliser must not bounce clean requests. */ - public function test_normalize_query_string_is_noop_for_clean_query_string(): void { + public function test_canonicalize_query_is_noop_for_clean_url(): void { $this->bootstrap_workos_enabled(); $this->repository->ensure_default(); - $context = $this->run_takeover_capturing_context( + $result = $this->run_takeover( [ 'workos_action' => 'reset-password', 'profile' => 'default', @@ -750,45 +746,47 @@ public function test_normalize_query_string_is_noop_for_clean_query_string(): vo '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', - $context['initial_step'] ?? null, + $result['context']['initial_step'] ?? null, 'Clean URLs must still reach reset_confirm.' ); - $this->assertSame( 'tok_clean', $context['reset_token'] ?? null ); + $this->assertSame( 'tok_clean', $result['context']['reset_token'] ?? null ); } /** - * workos_action=reset-password without a token must NOT set reset_confirm - * even after normalization — the existing gate in build_context() holds. + * 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_normalize_query_string_without_token_does_not_start_reset_confirm(): void { + public function test_canonicalize_query_without_token_does_not_start_reset_confirm(): void { $this->bootstrap_workos_enabled(); $this->repository->ensure_default(); - $context = $this->run_takeover_capturing_context( + $result = $this->run_takeover( [ 'workos_action' => 'reset-password' ], 'workos_action=reset-password' ); + $this->assertNull( $result['redirect'] ); $this->assertArrayNotHasKey( 'initial_step', - $context ?? [], - 'Without a token, reset_confirm must not be set regardless of normalization.' + $result['context'] ?? [], + 'Without a token, reset_confirm must not be set.' ); } /** - * Invoke maybe_takeover() with synthesized $_GET and $_SERVER['QUERY_STRING'], - * capturing the context array passed to render_full_page(). Returns the - * context when the inline-render branch is reached, null if a redirect fired. + * 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|null + * @return array{context: ?array, redirect: ?string} */ - private function run_takeover_capturing_context( array $get, string $query_string ): ?array { + 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; @@ -798,26 +796,38 @@ public function render_full_page( Profile $profile, array $context = [] ): void }; $takeover = new LoginTakeover( $router, $fake_renderer ); - $listener = static function ( $location ): string { + $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'] ?? ''; - $_GET = $get; - $_SERVER['QUERY_STRING'] = $query_string; + $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; + $_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 $fake_renderer->captured; + return [ + 'context' => $fake_renderer->captured, + 'redirect' => $redirect_url, + ]; } }