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
38 changes: 35 additions & 3 deletions inc/sso/class-sso.php
Original file line number Diff line number Diff line change
Expand Up @@ -578,9 +578,41 @@ public function handle_server($response_type = 'redirect'): void {
exit;
}

// Get the return URL (subsite) to redirect to after login.
// Just let WordPress show the login page - don't exit.
// The login form will have redirect_to parameter.
/*
* Not logged in on the main site. Anonymous visitors must not
* be forced through wp-login.php — the broker page they came
* from may be intentionally anonymous (free-checkout, landing
* pages, public marketing pages, etc.). The earlier "Just let
* WordPress show the login page - don't exit" comment that used
* to live here predated the must-redirect flow #1309 enabled
* and was incorrect: /sso-grant is not wp-login.php, so falling
* through has WordPress render its 404 template instead.
*
* Re-use the existing denial-signalling pattern already
* implemented in handle_broker() (see the `'invalid' ===
* $verify_code` branch a few hundred lines below): redirect
* back to the broker's /sso URL with sso_verify=invalid
* appended. handle_broker() recognises the signal, sets
* wu_sso_denied=1 on the broker domain for 5 minutes, and
* redirects the user to their original return_url. sso.js
* then sees the cookie (assets/js/sso.js:22) and skips the
* JSONP probe entirely, so the user browses anonymously
* without further SSO disruption.
*
* If the broker URL is missing (malformed grant request) we
* just exit; the request was malformed so an empty response
* is preferable to either a 404 or a forced login.
*/
$broker_url = $this->input('return_url', '');

if (empty($broker_url)) {
exit;
}

$denial_url = add_query_arg('sso_verify', 'invalid', $broker_url);

wp_safe_redirect($denial_url, 302, 'WP-Ultimo-SSO');
exit;
}

/**
Expand Down
79 changes: 79 additions & 0 deletions tests/WP_Ultimo/SSO/SSO_Test.php
Original file line number Diff line number Diff line change
Expand Up @@ -675,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 @@ -693,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 @@ -710,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 @@ -733,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 @@ -759,13 +759,13 @@
* as a non-JSONP request and falls through to the redirect branch.
*/
public function test_handle_broker_source_jsonp_unattached_returns_must_redirect(): void {
$source = file_get_contents(

Check warning on line 762 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'
);

$unattached_section = '';

if (preg_match("/isAttached\(\).*?wp_safe_redirect/s", $source, $matches)) {

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

View workflow job for this annotation

GitHub Actions / Code Quality Checks

String "/isAttached\(\).*?wp_safe_redirect/s" does not require double quotes; use single quotes instead
$unattached_section = $matches[0];
}

Expand Down Expand Up @@ -793,7 +793,7 @@
* fix above would have no effect on the browser side.
*/
public function test_sso_js_handles_must_redirect_verify(): void {
$source = file_get_contents(

Check warning on line 796 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 All @@ -810,7 +810,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 813 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 @@ -842,7 +842,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 845 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 @@ -996,7 +996,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 999 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 @@ -1009,6 +1009,85 @@
);
}

/**
* Regression guard for the /sso-grant 404 introduced when #1309
* enabled the `verify: must-redirect` top-level navigation from
* the unattached JSONP probe.
*
* Flow that exposed the bug: anonymous user on a broker subsite ->
* sso.js JSONP probe -> handle_broker returns must-redirect ->
* browser top-navigates to <broker>/sso?return_url=... ->
* handle_broker non-JSONP redirect via getAttachUrl() ->
* <main>/sso-grant -> handle_server() with not-logged-in user and
* response_type=redirect. Before this guard, that branch fell
* through with the comment "Just let WordPress show the login page
* - don't exit", but /sso-grant is not wp-login.php, so WordPress
* rendered its 404 template and Sidekick subsequently cached it,
* leaving the cross-domain SSO entry permanently broken for new
* anonymous visitors.
*
* The correct behaviour for anonymous visitors is: do not force a
* redirect to wp-login.php (the broker page may be intentionally
* anonymous — free-checkout, landing, marketing). Instead, re-use
* the existing denial-signalling pattern in handle_broker() (the
* `'invalid' === $verify_code` branch): redirect back to the
* broker's /sso URL with sso_verify=invalid appended. handle_broker
* recognises the signal, sets wu_sso_denied=1 on the broker domain
* for 5 minutes, and redirects to the original return_url. sso.js
* (line 22) then sees the cookie and skips the JSONP probe so the
* user browses anonymously without further SSO disruption.
*
* Since handle_server() calls exit, we lock the behaviour in via a
* source-pattern match (matching the project convention used by
* test_handle_server_source_sets_javascript_content_type_for_jsonp
* and its peers in this file).
*/
public function test_handle_server_source_signals_denial_when_not_logged_in_non_jsonp(): void {
$source = file_get_contents(
dirname(__DIR__, 3) . '/inc/sso/class-sso.php'
);

$start = strpos( $source, 'public function handle_server(' );
$end = strpos( $source, 'private function handle_main_site_logged_in_user' );

$this->assertNotFalse( $start, 'handle_server() must be present for inspection' );
$this->assertNotFalse( $end, 'handle_main_site_logged_in_user() must be present for inspection' );

$body = substr( $source, $start, $end - $start );

// The non-JSONP not-logged-in branch must redirect back to the
// broker's /sso URL with sso_verify=invalid appended, so
// handle_broker can set wu_sso_denied on the broker domain
// and bounce the user back to their original page.
$this->assertMatchesRegularExpression(
'/add_query_arg\s*\(\s*[\'"]sso_verify[\'"]\s*,\s*[\'"]invalid[\'"]/s',
$body,
'handle_server() not-logged-in non-JSONP branch must append sso_verify=invalid to the broker URL so handle_broker can set wu_sso_denied and bounce the user back to the original page anonymously (instead of forcing wp-login.php).'
);

$this->assertMatchesRegularExpression(
'/wp_safe_redirect\s*\(\s*\$denial_url\s*,\s*302\s*,\s*[\'"]WP-Ultimo-SSO[\'"]\s*\)\s*;\s*exit\s*;/s',
$body,
'handle_server() not-logged-in non-JSONP branch must call wp_safe_redirect( $denial_url, 302, \'WP-Ultimo-SSO\' ); exit; after building the denial URL.'
);

// Must NOT force wp-login.php on anonymous visitors — broker
// pages may legitimately be anonymous (free-checkout, landing).
$this->assertDoesNotMatchRegularExpression(
'/wp_safe_redirect\s*\(\s*wp_login_url\s*\(/s',
$body,
'handle_server() must NOT redirect anonymous visitors to wp_login_url() — that would break free-checkout and other intentionally-anonymous broker pages. Use the sso_verify=invalid denial signal instead.'
);

// The stale pre-#1309 comment that misled callers into thinking
// WP would render wp-login.php must be gone.
$this->assertStringNotContainsString(
"Just let WordPress show the login page - don't exit",
$body,
'Stale comment must be removed: /sso-grant is not wp-login.php, so falling through has WP render its 404 template (pre-#1309 misconception).'
);
}

/**
* Verify that all JSONP branches set the
* Content-Type: application/javascript header.
Expand Down
Loading