Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion inc/sso/class-sso.php
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ private function should_skip_redirect_due_to_loop(): bool {
$threshold = (int) apply_filters('wu_sso_redirect_loop_threshold', 3);
$window = (int) apply_filters('wu_sso_redirect_loop_window', 120);

if ($threshold < 1 || $window < 1) {
if ($threshold < 1 || $window < 1 || ! $this->is_sso_loop_request()) {
return false;
}

Expand Down Expand Up @@ -466,6 +466,34 @@ private function should_skip_redirect_due_to_loop(): bool {
return $count >= $threshold;
}

/**
* Check if the current request looks like an SSO loop hop.
*
* Plain logged-out visits to protected URLs can call auth_redirect()
* repeatedly without ever entering the SSO flow. Only requests carrying an
* SSO fingerprint should consume the redirect-loop budget.
*
* @since 2.0.11
* @return bool True when the request carries an SSO fingerprint.
*/
private function is_sso_loop_request(): bool {

if ($this->input('wu_sso_token') || 'login' === $this->input('sso') || $this->input('return_url') || $this->input('_jsonp')) {
return true;
}

$referer = wp_get_referer();

if ( ! $referer) {
return false;
}

$referer_path = (string) wp_parse_url($referer, PHP_URL_PATH);
$sso_path = '/' . ltrim($this->get_url_path(), '/');

return 0 === strpos($referer_path, $sso_path);
Comment on lines +491 to +494

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Tighten referer fingerprint to main-site /sso endpoint only.

Line 494 currently treats any referer path starting with /sso as an SSO hop, regardless of host and endpoint boundary. That can still consume wu_sso_redirect_attempts for non-SSO traffic.

Proposed fix
-		$referer_path = (string) wp_parse_url($referer, PHP_URL_PATH);
-		$sso_path     = '/' . ltrim($this->get_url_path(), '/');
-
-		return 0 === strpos($referer_path, $sso_path);
+		$referer_host = strtolower((string) wp_parse_url($referer, PHP_URL_HOST));
+		$referer_path = (string) wp_parse_url($referer, PHP_URL_PATH);
+		$main_host    = strtolower((string) wp_parse_url(get_home_url(wu_get_main_site_id()), PHP_URL_HOST));
+		$sso_path     = '/' . ltrim($this->get_url_path(), '/');
+
+		if (empty($referer_host) || $referer_host !== $main_host) {
+			return false;
+		}
+
+		return 1 === preg_match('#^' . preg_quote($sso_path, '#') . '(?:/|$)#', $referer_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` around lines 491 - 494, The referer check is too
permissive because it only compares $referer_path to $sso_path; update the logic
in class-sso.php so it validates both host and a strict endpoint boundary: parse
the referer host (e.g. $referer_host = (string) wp_parse_url($referer,
PHP_URL_HOST)) and compare it to the site/home host, then ensure the path
exactly matches $sso_path or starts with $sso_path followed by '/' (to avoid
matching paths like '/ssohijack'); use $this->get_url_path(), $referer_path and
wp_parse_url to implement these two checks and only return true when both host
and bounded-path conditions are met.

}

/**
* Listens for SSO requests and route them to the correct handler.
*
Expand Down
35 changes: 35 additions & 0 deletions tests/WP_Ultimo/SSO/SSO_Test.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,13 @@
unset($_REQUEST['broker']);
unset($_REQUEST['sso_verify']);
unset($_REQUEST['wu_sso_token']);
unset($_REQUEST['sso']);
unset($_REQUEST['return_url']);
unset($_REQUEST['redirect_to']);
unset($_REQUEST['_jsonp']);
unset($_COOKIE['wu_sso_denied']);
unset($_COOKIE['wu_sso_redirect_attempts']);
unset($_SERVER['HTTP_REFERER']);

remove_all_filters('wu_sso_redirect_loop_threshold');
remove_all_filters('wu_sso_redirect_loop_window');
Expand Down Expand Up @@ -253,6 +256,8 @@
public function test_sso_redirect_loop_counter_skips_at_threshold(): void {
$sso = SSO::get_instance();

$_REQUEST['sso'] = 'login';

add_filter(
'wu_sso_redirect_loop_threshold',
function () {
Expand All @@ -276,6 +281,8 @@
public function test_sso_redirect_loop_counter_resets_after_window(): void {
$sso = SSO::get_instance();

$_REQUEST['sso'] = 'login';

add_filter(
'wu_sso_redirect_loop_threshold',
function () {
Expand All @@ -299,6 +306,34 @@
$this->assertSame(1, $this->get_sso_redirect_attempt_count());
}

/**
* Test plain protected URL visits do not consume the SSO loop budget.
*/
public function test_sso_redirect_loop_counter_ignores_non_sso_requests(): void {
$sso = SSO::get_instance();

$method = new \ReflectionMethod($sso, 'should_skip_redirect_due_to_loop');
$method->setAccessible(true);

$this->assertFalse($method->invoke($sso));
$this->assertSame(0, $this->get_sso_redirect_attempt_count());
}

/**
* Test SSO referers consume the redirect-loop budget.
*/
public function test_sso_redirect_loop_counter_counts_sso_referer(): void {
$sso = SSO::get_instance();

$_SERVER['HTTP_REFERER'] = home_url('/sso');

$method = new \ReflectionMethod($sso, 'should_skip_redirect_due_to_loop');
$method->setAccessible(true);

$this->assertFalse($method->invoke($sso));
$this->assertSame(1, $this->get_sso_redirect_attempt_count());
}

/**
* Get the redirect-attempt count from the test cookie.
*
Expand Down Expand Up @@ -640,7 +675,7 @@
* when sso_verify is 'invalid', preventing redirect loops.
*/
public function test_handle_broker_source_sets_denied_cookie_on_invalid_verify(): void {
$source = file_get_contents(

Check warning on line 678 in tests/WP_Ultimo/SSO/SSO_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks

file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
dirname(__DIR__, 3) . '/inc/sso/class-sso.php'
);

Expand All @@ -658,7 +693,7 @@
* so that later code (handle_auth_redirect, enqueue_script) sees it immediately.
*/
public function test_handle_broker_source_sets_cookie_superglobal_on_invalid_verify(): void {
$source = file_get_contents(

Check warning on line 696 in tests/WP_Ultimo/SSO/SSO_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks

file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
dirname(__DIR__, 3) . '/inc/sso/class-sso.php'
);

Expand All @@ -675,7 +710,7 @@
* instead of leaving the user on the /sso 404 page.
*/
public function test_handle_broker_source_redirects_to_return_url_on_invalid_verify(): void {
$source = file_get_contents(

Check warning on line 713 in tests/WP_Ultimo/SSO/SSO_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks

file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
dirname(__DIR__, 3) . '/inc/sso/class-sso.php'
);

Expand All @@ -698,7 +733,7 @@
* causes an infinite redirect loop.
*/
public function test_handle_broker_source_returns_jsonp_error_for_unattached_broker(): void {
$source = file_get_contents(

Check warning on line 736 in tests/WP_Ultimo/SSO/SSO_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks

file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
dirname(__DIR__, 3) . '/inc/sso/class-sso.php'
);

Expand All @@ -717,7 +752,7 @@
* redirect -> sso_verify=invalid -> redirect -> repeat.
*/
public function test_sso_js_does_not_contain_incognito_redirect(): void {
$source = file_get_contents(

Check warning on line 755 in tests/WP_Ultimo/SSO/SSO_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks

file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
dirname(__DIR__, 3) . '/assets/js/sso.js'
);

Expand Down Expand Up @@ -749,7 +784,7 @@
* Test handle_requests normalizes sso-grant URLs before dispatching.
*/
public function test_handle_requests_source_normalizes_sso_grant_to_server_action(): void {
$source = file_get_contents(

Check warning on line 787 in tests/WP_Ultimo/SSO/SSO_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks

file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
dirname(__DIR__, 3) . '/inc/sso/class-sso.php'
);

Expand Down Expand Up @@ -903,7 +938,7 @@
* call before the printf to guard against regressions.
*/
public function test_handle_server_source_sets_javascript_content_type_for_jsonp(): void {
$source = file_get_contents(

Check warning on line 941 in tests/WP_Ultimo/SSO/SSO_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks

file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
dirname(__DIR__, 3) . '/inc/sso/class-sso.php'
);

Expand All @@ -927,7 +962,7 @@
* 4. handle_main_site_logged_in_user JSONP success response
*/
public function test_handle_broker_source_sets_javascript_content_type_for_jsonp(): void {
$source = file_get_contents(

Check warning on line 965 in tests/WP_Ultimo/SSO/SSO_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks

file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead.
dirname(__DIR__, 3) . '/inc/sso/class-sso.php'
);

Expand Down
Loading