fix(sso): return verify: must-redirect from unattached JSONP probe#1309
Merged
Conversation
The unattached-broker JSONP branch in handle_broker() returned
'{code: 0, message: "Broker not attached"}', which sso.js treats
as a denial (assets/js/sso.js lines 101-117). That set the
wu_sso_denied cookie for 5 minutes on the very first subsite
front-end page-load, silently disabling auto-SSO before it could
complete a single round-trip — even when the user was in fact
logged in on the main site.
The denial payload is the wrong signal here: the broker not being
attached on the JSONP probe is the expected state on the first
hit, not a failure. Return 'verify: must-redirect' instead so the
already-existing sso.js branch (assets/js/sso.js lines 93-95)
performs a top-level navigation to '<broker>/sso?return_url=...'.
That request re-enters handle_broker() as a regular page load and
falls through to the non-JSONP redirect, which after #1301 reaches
the cookie-less server handler via getAttachUrl() and completes the
SSO round-trip.
This is the minimal subset of #1297 that the post-#1301/#1302
codebase still needs. The /sso-grant routing fix (#1301) and the
scoped redirect-loop counter (#1302) already shipped, so the rest
of #1297's PHP changes are no longer required.
Regression tests in tests/WP_Ultimo/SSO/SSO_Test.php:
- test_handle_broker_source_jsonp_unattached_returns_must_redirect:
locks in the new payload and forbids the legacy 'Broker not
attached' denial.
- test_sso_js_handles_must_redirect_verify: confirms sso.js still
recognises the must-redirect verify value.
Contributor
📝 WalkthroughWalkthroughThe PR modifies the SSO broker attachment flow: when a broker is not attached, the ChangesUnattached Broker Must-Redirect JSONP Flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/WP_Ultimo/SSO/SSO_Test.php (2)
725-747: ⚡ Quick winReconcile the now-stale companion test. This PR changes the unattached-broker JSONP payload from an error into a
must-redirectsuccess, but the section header (Lines 725-727) andtest_handle_broker_source_returns_jsonp_error_for_unattached_brokerstill describe a "JSONP error (not redirect)". The regex only asserts thatwu.sso(is present, so it keeps passing while documenting the wrong contract. Recommend updating/renaming it or removing it in favor of the new..._returns_must_redirecttest to avoid a misleading test.🤖 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 `@tests/WP_Ultimo/SSO/SSO_Test.php` around lines 725 - 747, Update the stale test and its surrounding docblock to match the new contract: rename test_handle_broker_source_returns_jsonp_error_for_unattached_broker to something like test_handle_broker_source_returns_must_redirect_for_unattached_broker (or delete the test if duplicate with the new must-redirect test), update the docblock header/comments (the three-line section comment and the method phpdoc) to state "must-redirect" success for JSONP when broker is unattached, and adjust the assertion or regex if necessary so it verifies the new behavior (e.g., assert the presence of the "must-redirect" payload or the specific code path that emits wu.sso(... 'must-redirect' ...) instead of the old error expectation).
768-768: 💤 Low valueUse single quotes for this non-interpolated regex. Flagged by static analysis since there's no variable interpolation.
♻️ Proposed change
- if (preg_match("/isAttached\(\).*?wp_safe_redirect/s", $source, $matches)) { + if (preg_match('/isAttached\(\).*?wp_safe_redirect/s', $source, $matches)) {As per coding guidelines: "Prefer single quotes for strings; use double quotes only when interpolating variables".
🤖 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 `@tests/WP_Ultimo/SSO/SSO_Test.php` at line 768, The regex string passed to preg_match is using double quotes unnecessarily; change the argument "isAttached\(\).*?wp_safe_redirect/s" to use single quotes instead (i.e., update the preg_match call in SSO_Test.php that contains preg_match("/isAttached\(\).*?wp_safe_redirect/s", $source, $matches)) so the pattern is single-quoted and all escapes and modifiers remain unchanged.
🤖 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.
Nitpick comments:
In `@tests/WP_Ultimo/SSO/SSO_Test.php`:
- Around line 725-747: Update the stale test and its surrounding docblock to
match the new contract: rename
test_handle_broker_source_returns_jsonp_error_for_unattached_broker to something
like test_handle_broker_source_returns_must_redirect_for_unattached_broker (or
delete the test if duplicate with the new must-redirect test), update the
docblock header/comments (the three-line section comment and the method phpdoc)
to state "must-redirect" success for JSONP when broker is unattached, and adjust
the assertion or regex if necessary so it verifies the new behavior (e.g.,
assert the presence of the "must-redirect" payload or the specific code path
that emits wu.sso(... 'must-redirect' ...) instead of the old error
expectation).
- Line 768: The regex string passed to preg_match is using double quotes
unnecessarily; change the argument "isAttached\(\).*?wp_safe_redirect/s" to use
single quotes instead (i.e., update the preg_match call in SSO_Test.php that
contains preg_match("/isAttached\(\).*?wp_safe_redirect/s", $source, $matches))
so the pattern is single-quoted and all escapes and modifiers remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ca275ac-8e73-4ed9-a260-5d753210fcfd
📒 Files selected for processing (2)
inc/sso/class-sso.phptests/WP_Ultimo/SSO/SSO_Test.php
🔨 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: |
superdav42
added
the
review-feedback-scanned
superdav42
added a commit
that referenced
this pull request
May 29, 2026
… on anonymous /sso-grant Anonymous visitors hitting /sso-grant via the must-redirect flow from PR #1309 were getting a WordPress 404, then a forced wp-login.php redirect in the first version of this fix. Both are wrong for intentionally-anonymous broker pages (free-checkout, landing pages, marketing pages). This change re-uses the existing denial-signalling pattern already implemented in handle_broker (the 'invalid' === $verify_code branch): when handle_server runs on /sso-grant for a not-logged-in user, redirect back to the broker's /sso URL with sso_verify=invalid appended. handle_broker on the broker domain recognises the signal, sets wu_sso_denied=1 on the broker domain for 5 minutes, and redirects to the original return_url. sso.js (assets/js/sso.js:22) then sees the cookie and skips the JSONP probe so the user browses anonymously without further SSO disruption. Regression test updates accordingly (renamed test_handle_server_source_redirects_to_login_when_not_logged_in_non_jsonp to test_handle_server_source_signals_denial_when_not_logged_in_non_jsonp) and adds an explicit assertDoesNotMatchRegularExpression guard that catches any future attempt to call wp_safe_redirect(wp_login_url(...)) in that branch. Refs #1309
superdav42
added a commit
that referenced
this pull request
May 29, 2026
… on anonymous /sso-grant (#1321) Anonymous visitors hitting /sso-grant via the must-redirect flow from PR #1309 were getting a WordPress 404, then a forced wp-login.php redirect in the first version of this fix. Both are wrong for intentionally-anonymous broker pages (free-checkout, landing pages, marketing pages). This change re-uses the existing denial-signalling pattern already implemented in handle_broker (the 'invalid' === $verify_code branch): when handle_server runs on /sso-grant for a not-logged-in user, redirect back to the broker's /sso URL with sso_verify=invalid appended. handle_broker on the broker domain recognises the signal, sets wu_sso_denied=1 on the broker domain for 5 minutes, and redirects to the original return_url. sso.js (assets/js/sso.js:22) then sees the cookie and skips the JSONP probe so the user browses anonymously without further SSO disruption. Regression test updates accordingly (renamed test_handle_server_source_redirects_to_login_when_not_logged_in_non_jsonp to test_handle_server_source_signals_denial_when_not_logged_in_non_jsonp) and adds an explicit assertDoesNotMatchRegularExpression guard that catches any future attempt to call wp_safe_redirect(wp_login_url(...)) in that branch. Refs #1309
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Minimal replacement for #1297. Changes a single response payload in
handle_broker()so the JSONP unattached probe no longer tripssso.jsinto thewu_sso_denieddenial path on the very first subsite front-end page-load.Bug
When a logged-in main-site user visits a subsite front-end page,
sso.jsruns the JSONP<script src="<broker>/sso?_jsonp=1">attach probe. On the first hit the broker is unattached (that's the point of the probe), sohandle_broker()enters the! $broker->isAttached()JSONP branch and returns:{ "code": 0, "message": "Broker not attached" }sso.jsseescode !== 200and falls into the denial path (window.wu.sso_denied()), which setswu_sso_denied=1for 5 minutes. After that there is no path that ever surfaces the main-site session to the broker — auto-SSO silently dies before completing a single round-trip, even though the user is in fact logged in on the main site.Fix
Return
verify: 'must-redirect'instead:sso.jsalready has the matching branch (assets/js/sso.js:93-95, present onmaintoday):That top-level navigation re-enters
handle_broker()as a regular page load (not JSONP) and falls through to the non-JSONP redirect via$broker->getAttachUrl(). After #1301 that URL (/sso-grant) routes correctly towu_sso_handle_sso_grantand completes the cookie-less SSO round-trip.Why this is the whole fix now (vs #1297)
PR #1297 was opened against a codebase where two regressions overlapped: the JSONP
code: 0denial and the broken/sso-grantrouting. Two follow-up PRs have since merged tomain:fix: route sso grant attach urls(normalises/sso-grantsogetAttachUrl()works again)fix(sso): scope redirect loop counter to SSO hopsWith those landed, the only remaining defect from #1297's premise is the JSONP denial payload above. The broader rewrite #1297 proposed (redirecting through
/ssoinstead of/sso-grant, plus bundled wp-env Cypress fixture changes, sunrise.php glob fallback, andsso.jscheckout-form skip) is no longer needed for this bug and is intentionally not included here. Closing #1297 in favour of this minimal change.Tests
Two source-code regression tests added to
tests/WP_Ultimo/SSO/SSO_Test.php, matching the style of the existingtest_handle_broker_source_*assertions:test_handle_broker_source_jsonp_unattached_returns_must_redirect— asserts the unattached-broker JSONP response now contains'verify' => 'must-redirect'and no longer contains'message' => 'Broker not attached'.test_sso_js_handles_must_redirect_verify— assertssso.jsstill recognises themust-redirectverify value.Verification
The one failing test (
test_sso_redirect_loop_counter_ignores_non_sso_requests) fails identically on unmodifiedmain(verified viagit stash+ run), so it is a pre-existing issue unrelated to this PR.Out of scope (and explicitly not included)
sso.jscheckout_form//register/skip from fix(sso): restore front-end auto-SSO via main-site magic-link redirect #1297 — separate concern, should be its own PR with its own reproduction.setup-sso-test.php,setup-checkout-form.php,060-sso-cross-domain.spec.js) — those are CI environment workarounds, not part of this bug.sunrise.phpdynamic plugin-dirglob()fallback — also CI environment scoped.Resolves the actual auto-SSO regression #1297 was trying to address.
Summary by CodeRabbit
Bug Fixes
Tests