fix: cookie-less cross-domain SSO login flow#1084
Conversation
When user is already logged in on main site and visits login page with SSO params, redirect them directly to the subsite with a verification token instead of showing 'already logged in' message. - Check for 'sso' param in addition to 'return_url' - Extract return_url from redirect_to query params if present - Handle WP_Error user object in handle_login_redirect
📝 WalkthroughWalkthroughThe SSO login redirect handler ChangesSSO Login Redirect Parameter Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
inc/sso/class-sso.php (1)
681-685:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
wu_sso_tokenhas no single-use enforcement – replay attack is possible
generate_sso_token()produces an HMAC-signed JWT-style token that is valid for 5 minutes with no consumption record. A token URL captured via browser history, proxy/CDN logs, orRefererheader can be replayed within that window to authenticate as the victim.Store a short-lived transient (keyed by the HMAC or a random nonce) when the token is issued, and delete it on first successful validation in
validate_sso_token_from_main_site():// On issue (generate_sso_token): $nonce = wp_generate_password(32, false); set_transient('wu_sso_token_' . $nonce, $user_id, 300); // Include $nonce in the token payload so the receiver can look it up and delete it. // On validation (validate_sso_token_from_main_site): if ( ! get_transient('wu_sso_token_' . $nonce) ) { return false; // already used or expired } delete_transient('wu_sso_token_' . $nonce);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/sso/class-sso.php` around lines 681 - 685, The wu_sso_token currently can be replayed because generate_sso_token() issues a short-lived token without a consumption record; modify generate_sso_token() to include a cryptographically-strong nonce (e.g., wp_generate_password) in the token payload and store a short-lived transient keyed by that nonce (e.g., set_transient('wu_sso_token_' . $nonce, $user_id, 300)) when issuing the token, and then update validate_sso_token_from_main_site() to extract the nonce from the token, verify the transient exists via get_transient('wu_sso_token_' . $nonce), reject if missing (used/expired), and delete the transient (delete_transient) on first successful validation to enforce single-use.
🧹 Nitpick comments (1)
inc/sso/class-sso.php (1)
653-664: ⚡ Quick winDuplicate
return_url-from-redirect_toextraction – extract a shared helperThis block is a near-exact duplicate of lines 569–577 inside
handle_login_redirect(). Extract to a private method (e.g.,extract_return_url_from_redirect($redirect_to)) and call it from both places.♻️ Proposed refactor
+ /** + * Extracts return_url from a redirect_to query string parameter. + * + * `@since` 2.0.11 + * + * `@param` string $redirect_to URL that may carry a return_url query param. + * `@return` string Extracted and sanitized return_url, or empty string. + */ + private function extract_return_url_from_redirect(string $redirect_to): string { + if ( empty($redirect_to) ) { + return ''; + } + $parsed = wp_parse_url($redirect_to, PHP_URL_QUERY); + if ( $parsed ) { + parse_str($parsed, $query_params); + if ( ! empty($query_params['return_url']) ) { + return esc_url_raw($query_params['return_url']); + } + } + return ''; + }Then in
handle_already_logged_in_on_login_page():- // Also extract return_url from redirect_to if present - if ( empty($return_url) ) { - $redirect_to = $this->input('redirect_to', ''); - if ( $redirect_to ) { - $parsed = wp_parse_url($redirect_to, PHP_URL_QUERY); - if ( $parsed ) { - parse_str($parsed, $query_params); - if ( ! empty($query_params['return_url']) ) { - $return_url = $query_params['return_url']; - } - } - } - } + // Also extract return_url from redirect_to if present + if ( empty($return_url) ) { + $return_url = $this->extract_return_url_from_redirect($this->input('redirect_to', '')); + }And in
handle_login_redirect()(lines 569-577):- if ( empty($return_url) && ! empty($redirect_to) ) { - $parsed = wp_parse_url($redirect_to, PHP_URL_QUERY); - if ( $parsed ) { - parse_str($parsed, $query_params); - if ( ! empty($query_params['return_url']) ) { - $return_url = $query_params['return_url']; - } - } - } + if ( empty($return_url) && ! empty($redirect_to) ) { + $return_url = $this->extract_return_url_from_redirect($redirect_to); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/sso/class-sso.php` around lines 653 - 664, The duplicate logic that pulls a return_url from a redirect_to query string should be extracted into a private helper (e.g., extract_return_url_from_redirect($redirect_to)) and used from both handle_already_logged_in_on_login_page() and handle_login_redirect(); create extract_return_url_from_redirect to accept the $redirect_to string, run wp_parse_url + parse_str and return the return_url if present (or null/empty), then replace the duplicated block in the code snippet (and the similar block at lines 569–577 inside handle_login_redirect()) with a call to this new method to centralize behavior and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/sso/class-sso.php`:
- Line 649: The code uses a hardcoded 'sso' literal when calling $this->input
which bypasses the wu_sso_get_url_path filter; change the call that sets
$sso_action to use $this->get_url_path() instead of the literal so the filtered
URL path is respected (i.e. replace $this->input('sso', '') with
$this->input($this->get_url_path(), '')); ensure this change is applied wherever
$sso_action is set so apply_filters('wu_sso_get_url_path', 'sso', ...) can take
effect.
- Line 660: The extracted $query_params['return_url'] is assigned to $return_url
without sanitization; update the code in the SSO handling (where parse_str
populates $query_params) to sanitize the value before assignment by running it
through a URL sanitizer (use esc_url_raw() per guidelines or wu_clean() as an
alternative) and ensure you check isset($query_params['return_url']) and provide
a safe fallback if missing/empty.
---
Outside diff comments:
In `@inc/sso/class-sso.php`:
- Around line 681-685: The wu_sso_token currently can be replayed because
generate_sso_token() issues a short-lived token without a consumption record;
modify generate_sso_token() to include a cryptographically-strong nonce (e.g.,
wp_generate_password) in the token payload and store a short-lived transient
keyed by that nonce (e.g., set_transient('wu_sso_token_' . $nonce, $user_id,
300)) when issuing the token, and then update
validate_sso_token_from_main_site() to extract the nonce from the token, verify
the transient exists via get_transient('wu_sso_token_' . $nonce), reject if
missing (used/expired), and delete the transient (delete_transient) on first
successful validation to enforce single-use.
---
Nitpick comments:
In `@inc/sso/class-sso.php`:
- Around line 653-664: The duplicate logic that pulls a return_url from a
redirect_to query string should be extracted into a private helper (e.g.,
extract_return_url_from_redirect($redirect_to)) and used from both
handle_already_logged_in_on_login_page() and handle_login_redirect(); create
extract_return_url_from_redirect to accept the $redirect_to string, run
wp_parse_url + parse_str and return the return_url if present (or null/empty),
then replace the duplicated block in the code snippet (and the similar block at
lines 569–577 inside handle_login_redirect()) with a call to this new method to
centralize behavior and avoid duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
|
||
| // Check if this is an SSO flow (return_url param present) | ||
| // Check if this is an SSO flow (sso param or return_url param present) | ||
| $sso_action = $this->input('sso', ''); |
There was a problem hiding this comment.
Hardcoded 'sso' parameter name bypasses the wu_sso_get_url_path filter
Every other call site in this file retrieves the SSO URL path via $this->get_url_path(), which wraps apply_filters('wu_sso_get_url_path', 'sso', ...). Using a raw 'sso' literal here means that if wu_sso_get_url_path is filtered to a different value, $sso_action will always be empty and this branch of the SSO detection silently degrades to relying solely on return_url.
🐛 Proposed fix
- $sso_action = $this->input('sso', '');
+ $sso_action = $this->input($this->get_url_path(), '');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $sso_action = $this->input('sso', ''); | |
| $sso_action = $this->input($this->get_url_path(), ''); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@inc/sso/class-sso.php` at line 649, The code uses a hardcoded 'sso' literal
when calling $this->input which bypasses the wu_sso_get_url_path filter; change
the call that sets $sso_action to use $this->get_url_path() instead of the
literal so the filtered URL path is respected (i.e. replace $this->input('sso',
'') with $this->input($this->get_url_path(), '')); ensure this change is applied
wherever $sso_action is set so apply_filters('wu_sso_get_url_path', 'sso', ...)
can take effect.
| if ( $parsed ) { | ||
| parse_str($parsed, $query_params); | ||
| if ( ! empty($query_params['return_url']) ) { | ||
| $return_url = $query_params['return_url']; |
There was a problem hiding this comment.
Missing URL sanitization on the extracted return_url
$query_params['return_url'] is derived from user-controlled redirect_to input via parse_str but is assigned without any sanitization. Per coding guidelines, wu_clean() or a WordPress sanitization function must be used. For a value treated as a URL, esc_url_raw() is the right choice.
🛡️ Proposed fix
- $return_url = $query_params['return_url'];
+ $return_url = esc_url_raw($query_params['return_url']);As per coding guidelines: "Use wu_clean() or WordPress sanitization functions for input sanitization".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $return_url = $query_params['return_url']; | |
| $return_url = esc_url_raw($query_params['return_url']); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@inc/sso/class-sso.php` at line 660, The extracted $query_params['return_url']
is assigned to $return_url without sanitization; update the code in the SSO
handling (where parse_str populates $query_params) to sanitize the value before
assignment by running it through a URL sanitizer (use esc_url_raw() per
guidelines or wu_clean() as an alternative) and ensure you check
isset($query_params['return_url']) and provide a safe fallback if missing/empty.
SummaryEnables SSO login from subsite to main site without requiring 3rd party cookies. ProblemWhen visiting a subsite (e.g., vidanuevanaz.org) wp-admin, users were redirected to the main site (mygratis.site) login page. After logging in, the redirect back to the subsite didn't work without 3rd party cookies enabled. Solution
Changes
Testing
aidevops.sh v3.14.51 plugin for OpenCode v1.14.33 with claude-sonnet-4-6 spent 15h 33m on this as a headless worker. Merged via PR #1084 to main. |
|
Performance Test Results Performance test results for 9490114 are in 🛎️! Note: the numbers in parentheses show the difference to the previous (baseline) test run. Differences below 2% or 0.5 in absolute values are not shown. URL:
|
1 participant
Summary
Enables SSO login from subsite to main site without requiring 3rd party cookies.
Problem
When visiting a subsite (e.g., vidanuevanaz.org) wp-admin, users were redirected to the main site (mygratis.site) login page. After logging in, the redirect back to the subsite didn't work without 3rd party cookies enabled.
Solution
Token-based authentication: After login on main site, generate a time-limited
wu_sso_tokenand add it to the return URL when redirecting to a different domain.Subsite token validation: The multi-tenancy plugin's
handle_sso_login()now accepts simpler token format from the main site's SSO handler.Already-logged-in handling: When user is already logged in on main site and visits login page with SSO params, redirect directly to subsite with token instead of showing "already logged in" message.
Changes
class-sso.php: Addgenerate_sso_token(),handle_login_redirect()extracts return_url from query params,handle_already_logged_in_on_login_page()handles logged-in users on login pageclass-remote-sso-handler.php(multi-tenancy): Addvalidate_sso_token_from_main_site()to accept simpler token formatTesting
vidanuevanaz.org/wp-admin/(unauthenticated)mygratis.site/login/?sso=login&return_url=https://vidanuevanaz.orgvidanuevanaz.org- logged inAlso test already-logged-in flow:
aidevops.sh v3.14.51 plugin for OpenCode v1.14.33 with claude-sonnet-4-6 spent 15h 33m on this as a headless worker.
Summary by CodeRabbit