fix(sso): signal SSO denial to broker instead of forcing wp-login.php on anonymous /sso-grant#1321
Conversation
|
Warning Review limit reached
More reviews will be available in 16 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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: |
… 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
cdedc40 to
bed116a
Compare
🔨 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: |
Summary
Fixes the
/sso-grant404 introduced by PR #1309 for anonymous visitors,without forcing them through
wp-login.php.PR #1309 changed
handle_broker()'s JSONP probe response from{code: 0, message: 'Broker not attached'}(which triggeredwu.sso_denied()on the client and set the
wu_sso_deniedcookie) to{verify: 'must-redirect', server: <main_url>}(which top-level navigatesto
<main>/sso-grant?broker=...&token=...&checksum=...&return_url=<broker>).That fix resolved a real bug — logged-in main-site users were getting
falsely denied on first hit — but it introduced a regression for
anonymous visitors on broker subsites:
page, a landing page, a marketing page).
sso.jsruns the JSONP probe → main site respondsmust-redirect.<main>/sso-grant?....handle_server()runs on the main site with no logged-in user andresponse_type = redirect— the not-logged-in non-JSONP branch.// Just let WordPress show the login page - don't exit, but/sso-grantis notwp-login.php, so WordPress rendered its404 template and Sidekick/page caches subsequently cached it,
leaving the cross-domain SSO entry permanently broken for new
anonymous visitors. Observed in production on the
sdaiagent.comfree-checkout flow today.What this PR does
When
handle_server()runs on/sso-grantfor a not-logged-in user,redirect back to the broker's
/ssoURL withsso_verify=invalidappended.
This re-uses the existing denial-signalling pattern already implemented
in
handle_broker()(the'invalid' === $verify_codebranch a fewhundred lines below): on receiving the signal,
handle_broker()setswu_sso_denied=1on the broker domain for 5 minutes and redirects tothe original
return_url.sso.js(assets/js/sso.js:22) then seesthe cookie and skips the JSONP probe so the user browses anonymously
without further SSO disruption.
If the broker URL is missing (malformed grant request) we exit silently
rather than 404 or force login.
Why not redirect to
wp-login.php?An earlier revision of this PR redirected anonymous visitors to
wp_login_url($return_url). That was wrong for the same reasonfalling through to the 404 template was wrong: the broker page the
visitor came from may be intentionally anonymous — free-checkout,
landing pages, public marketing pages. Forcing those visitors through
the WordPress login screen breaks the conversion flow they were on.
The denial-signal mechanism mirrors the explicit "anonymous, do
nothing" contract that existed before PR #1309 and re-uses the
infrastructure (
wu_sso_deniedcookie, sso.js skip path) that'salready in place. No new client-side code, no new cookie, no new
contract.
Regression test
tests/WP_Ultimo/SSO/SSO_Test.php—test_handle_server_source_signals_denial_when_not_logged_in_non_jsonp()Source-pattern test (matching the convention already used in this
file because
handle_server()callsexit) that asserts:sso_verify=invalidtothe broker URL via
add_query_arg().wp_safe_redirect($denial_url, 302, 'WP-Ultimo-SSO'); exit;.wp_safe_redirect(wp_login_url(...))(explicit guard catching the rejected earlier approach).
Verification
Local dev (
mygratis.siteon the dev FrankenPHP instance, with thisbranch checked out):
Pre-fix this returned HTTP 200 with the WordPress 404 template
(later cached as
x-sidekick-cache: HITon subsequent requests).Test suite
Notes for reviewers
wp_safe_redirect()from the main site to a brokerdomain is permitted via the
allow_network_redirect_hostsfilterregistered in
inc/class-domain-mapping.php:61. Same mechanismhandle_broker()already relies on for the existingsso_verify=invalidbranch.
wu_request()runs values throughsanitize_text_field(), which strips%XXsequences. A nestedreturn_urlinside the broker URL will therefore lose its percentencoding. In practice the broker host stays intact (which is what
matters for the denial cookie), and
wp_safe_redirect()inhandle_broker()falls back tohome_url()for malformedreturn_urlvalues. Fixing the broader URL-sanitisation behaviouris out of scope for this regression fix.
Refs #1309
aidevops.sh v3.20.5 plugin for OpenCode v1.15.12 with claude-opus-4-7 spent 1d 14h and 767,081 tokens on this with the user in an interactive session.