test(e2e): restore cross-domain SSO spec#1356
Conversation
🔨 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: |
📝 WalkthroughWalkthroughAdds a CI hosts entry for ChangesCross-domain SSO hostname mapping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/e2e/cypress/fixtures/setup-sso-test.php`:
- Around line 54-57: This file is missing the required ABSPATH guard; add the
standard PHP entry guard at the top of the file by inserting defined('ABSPATH')
|| exit; as the very first executable line to prevent direct web access. Locate
the fixture file (setup-sso-test.php) and ensure the guard appears before any DB
mutation or references (e.g., before usage of $wpdb and the $table assignment
for 'wu_domain_mappings') so the script exits immediately if ABSPATH is not
defined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da60512b-bd95-4791-b8bc-642304b3339a
📒 Files selected for processing (4)
.github/workflows/e2e.ymltests/e2e/cypress/fixtures/setup-sso-test.phptests/e2e/cypress/integration/060-sso-cross-domain.spec.jstests/e2e/cypress/integration/065-sso-redirect-loop.spec.js
| // 2. Insert domain mapping for sso-test.ultimate-multisite.test:PORT directly | ||
| // into the DB. The Domain model's validation rejects domains with ports, so | ||
| // we bypass it. | ||
| $table = $wpdb->base_prefix . 'wu_domain_mappings'; |
There was a problem hiding this comment.
Add an ABSPATH entry guard before this mutation fixture executes.
Line 54 onward mutates DB/settings, but this file is missing the required defined('ABSPATH') || exit; gate at file start. That leaves direct execution risk in environments where test files are web-reachable.
🔧 Proposed fix
<?php
+defined('ABSPATH') || exit;
/**
* Set up the SSO e2e test environment.As per coding guidelines, “Every PHP file must start with defined('ABSPATH') || exit;”.
🧰 Tools
🪛 GitHub Check: Code Quality Checks
[warning] 56-56:
Expected 1 space before comment text but found 4; use block comment if you need indentation
[warning] 55-55:
Expected 1 space before comment text but found 4; use block comment if you need indentation
🤖 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/e2e/cypress/fixtures/setup-sso-test.php` around lines 54 - 57, This
file is missing the required ABSPATH guard; add the standard PHP entry guard at
the top of the file by inserting defined('ABSPATH') || exit; as the very first
executable line to prevent direct web access. Locate the fixture file
(setup-sso-test.php) and ensure the guard appears before any DB mutation or
references (e.g., before usage of $wpdb and the $table assignment for
'wu_domain_mappings') so the script exits immediately if ABSPATH is not defined.
Source: Coding guidelines
superdav42
added
the
ci-feedback-routed
🔨 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: |
🔨 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: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/cypress/integration/060-sso-cross-domain.spec.js (1)
14-122:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix indentation: use tabs instead of spaces.
The file uses spaces for indentation throughout, but the coding guidelines require tabs for all
.jsfiles. This violates the WordPress ESLint configuration standard for this project.As per coding guidelines: "indentation must be tabs" for
**/*.jsfiles.🔧 Suggested approach
Run your editor's "Convert Spaces to Tabs" command, or use a script to convert all leading spaces to tabs. Most editors have a built-in command for this (e.g., VS Code: "Convert Indentation to Tabs").
For example, you can use the following command to convert spaces to tabs:
# This is just an example command - run it locally, not in the sandbox sed -i 's/^ /\t/g; s/^\t /\t\t/g; s/^\t\t /\t\t\t/g' tests/e2e/cypress/integration/060-sso-cross-domain.spec.jsOr configure your editor to use tabs for
.jsfiles in this project.🤖 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/e2e/cypress/integration/060-sso-cross-domain.spec.js` around lines 14 - 122, The file uses spaces for indentation but project style requires tabs; update leading indentation throughout this test file (e.g., the top-level const declarations, the before() hook, and each it() block including nested callbacks like the cy.request/.then and cy.visit sections) to use tabs instead of spaces so it conforms to the WordPress ESLint rules for .js files; run your editor or a conversion tool to replace leading space runs with equivalent tabs and re-check the file (060-sso-cross-domain.spec.js) so functions/hooks such as before, it, cy.request, and cy.visit are indented with tabs.Source: Coding guidelines
🤖 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 `@tests/e2e/cypress/integration/060-sso-cross-domain.spec.js`:
- Around line 89-99: The test name/comments in 060-sso-cross-domain.spec.js
should be clarified to state it verifies the wp-env main-origin handoff
behavior, and you should add a post-SSO verification that the mapped domain
remains authenticated: after the existing assertions that the browser landed on
mainSiteUrl with /wp-admin/ and `#wpadminbar`, navigate to
`${mappedDomainUrl}/wp-admin/` and assert you land on the admin UI (URL includes
`/wp-admin/`) and `#wpadminbar` exists without being redirected back into the
SSO flow; update the spec title/comment to mention "wp-env main-origin handoff"
or add the mapped-domain check to cover cross-domain authentication.
---
Outside diff comments:
In `@tests/e2e/cypress/integration/060-sso-cross-domain.spec.js`:
- Around line 14-122: The file uses spaces for indentation but project style
requires tabs; update leading indentation throughout this test file (e.g., the
top-level const declarations, the before() hook, and each it() block including
nested callbacks like the cy.request/.then and cy.visit sections) to use tabs
instead of spaces so it conforms to the WordPress ESLint rules for .js files;
run your editor or a conversion tool to replace leading space runs with
equivalent tabs and re-check the file (060-sso-cross-domain.spec.js) so
functions/hooks such as before, it, cy.request, and cy.visit are indented with
tabs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb7342bb-bd31-4c18-8d49-00722b8b8b26
📒 Files selected for processing (2)
tests/e2e/cypress/integration/060-sso-cross-domain.spec.jstests/e2e/cypress/integration/065-sso-redirect-loop.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/cypress/integration/065-sso-redirect-loop.spec.js
| // 3. After SSO redirect chain completes, the user should land on an | ||
| // authenticated wp-admin page. In wp-env the auth handoff returns | ||
| // through the main localhost origin where the login cookie exists. | ||
| cy.url({ timeout: 60000 }).should("include", "/wp-admin/"); | ||
| cy.get("body", { timeout: 30000 }).should("have.class", "wp-admin"); | ||
|
|
||
| // Confirm we are logged in: admin bar should be present. | ||
| cy.get("#wpadminbar").should("exist"); | ||
|
|
||
| // Confirm we are on the SSO test subsite (not the main site). | ||
| cy.url().should("include", "/sso-test-site/"); | ||
| // Confirm the SSO flow authenticated the browser session. | ||
| cy.url().should("include", mainSiteUrl); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the SSO redirect and return URL logic to understand expected final destination
# 1. Find get_sso_redirect_to implementation (referenced in context snippet 4)
echo "=== Checking get_sso_redirect_to implementation ==="
rg -n -A 20 'function get_sso_redirect_to' --type=php
# 2. Find get_sso_return_url implementation (referenced in context snippet 4)
echo -e "\n=== Checking get_sso_return_url implementation ==="
rg -n -A 20 'function get_sso_return_url' --type=php
# 3. Check if there's any SSO flow documentation or comments about expected behavior
echo -e "\n=== Checking for SSO flow documentation ==="
rg -n -C 3 'SSO.*flow|redirect.*chain|return.*main.*site' --type=php inc/sso/
# 4. Look for any existing tests or specs that verify SSO landing page
echo -e "\n=== Checking other SSO-related test expectations ==="
rg -n 'mappedDomain|mainSite.*url.*should' tests/e2e/cypress/integration/ --type=jsRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 8240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show 060 spec around line 60-130 ==="
nl -ba tests/e2e/cypress/integration/060-sso-cross-domain.spec.js | sed -n '60,140p'
echo -e "\n=== Inspect SSO main-site-logged-in handling ==="
rg -n "handle_main_site_logged_in_user|unwrap_sso_handoff_url|is_cross_domain_url|get_sso_redirect_to|get_sso_return_url" inc/sso/class-sso.php
echo -e "\n=== Show surrounding code for get_sso_return_url + get_sso_redirect_to ==="
nl -ba inc/sso/class-sso.php | sed -n '1040,1145p'
echo -e "\n=== Show surrounding code for handle_main_site_logged_in_user ==="
rg -n "function handle_main_site_logged_in_user" -n inc/sso/class-sso.php
line=$(rg -n "function handle_main_site_logged_in_user" inc/sso/class-sso.php | head -n1 | cut -d: -f1)
start=$((line-40)); end=$((line+120))
nl -ba inc/sso/class-sso.php | sed -n "${start},${end}p"
echo -e "\n=== Show unwrap_sso_handoff_url implementation ==="
rg -n "function unwrap_sso_handoff_url" inc/sso/class-sso.php
line=$(rg -n "function unwrap_sso_handoff_url" inc/sso/class-sso.php | head -n1 | cut -d: -f1)
start=$((line-40)); end=$((line+120))
nl -ba inc/sso/class-sso.php | sed -n "${start},${end}p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show 060 spec around line 60-140 (with line numbers) ==="
awk '{printf "%6d\t%s\n", NR, $0}' tests/e2e/cypress/integration/060-sso-cross-domain.spec.js | sed -n '60,140p'
echo -e "\n=== Inspect SSO main-site-logged-in handling (function names) ==="
rg -n "handle_main_site_logged_in_user|unwrap_sso_handoff_url|is_cross_domain_url|get_sso_redirect_to|get_sso_return_url" inc/sso/class-sso.php
echo -e "\n=== Show surrounding code for get_sso_return_url + get_sso_redirect_to ==="
awk '{printf "%6d\t%s\n", NR, $0}' inc/sso/class-sso.php | sed -n '1040,1145p'
echo -e "\n=== Show surrounding code for handle_main_site_logged_in_user ==="
line=$(rg -n "function handle_main_site_logged_in_user" inc/sso/class-sso.php | head -n1 | cut -d: -f1)
start=$((line-40)); end=$((line+140))
awk '{printf "%6d\t%s\n", NR, $0}' inc/sso/class-sso.php | sed -n "${start},${end}p"
echo -e "\n=== Show surrounding code for unwrap_sso_handoff_url ==="
line=$(rg -n "function unwrap_sso_handoff_url" inc/sso/class-sso.php | head -n1 | cut -d: -f1)
start=$((line-40)); end=$((line+120))
awk '{printf "%6d\t%s\n", NR, $0}' inc/sso/class-sso.php | sed -n "${start},${end}p"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 20729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect SSO redirect/attach flow entrypoints ==="
rg -n "handle_auth_redirect|auth_redirect|sso=login|sso_verify|redirect_to|return_url" inc/sso/class-sso.php
echo -e "\n=== Show handle_auth_redirect (or nearest match) ==="
pos=$(rg -n "function handle_auth_redirect|function handle_.*auth_redirect|auth_redirect\(" inc/sso/class-sso.php | head -n1 | cut -d: -f1)
if [ -n "${pos:-}" ]; then
start=$((pos-60)); end=$((pos+220))
awk '{printf "%6d\t%s\n", NR, $0}' inc/sso/class-sso.php | sed -n "${start},${end}p"
fi
echo -e "\n=== Inspect assets/js/sso.js for where it navigates/redirects after verification ==="
rg -n "return_url|redirect_to|window\.location|location\.href|wp-admin|sso_verify" assets/js/sso.js inc/sso -S || true
ls -la assets/js || true
if [ -f assets/js/sso.js ]; then
awk '{printf "%6d\t%s\n", NR, $0}' assets/js/sso.js | sed -n '1,220p'
fi
echo -e "\n=== Check cookie-less token consumption endpoint(s) ==="
rg -n "handle_cookie_less_sso_token|wu_sso_token" inc/sso/class-sso.php inc/sso -SRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 40810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show full handle_cookie_less_sso_token implementation (inc/sso/class-sso.php 1128+) ==="
awk '{printf "%6d\t%s\n", NR, $0}' inc/sso/class-sso.php | sed -n '1100,1185p'
echo -e "\n=== Show is_sso_token / redirect helpers around 1140-1170 if present ==="
rg -n "handle_cookie_less_sso_token|get_current_url|redirect_to|wu_sso_token|wp_safe_redirect" inc/sso/class-sso.php | head -n 50Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 6631
Clarify what the SSO test validates (wp-env main-origin handoff) and strengthen mapped-domain assertions.
The SSO implementation redirects based on return_url/redirect_to: when the user is already logged in on the main site, handle_main_site_logged_in_user() redirects with a cookie-less token and a redirect_to param, and handle_cookie_less_sso_token() consumes wu_sso_token and then wp_safe_redirect()s to redirect_to (defaulting to admin_url). In wp-env, that can legitimately result in the final landing URL being the main site's origin (as the spec comment already describes), so the end-state assertions on /wp-admin/ + #wpadminbar are consistent with “SSO-authenticated session exists” even if the browser ends up back on mainSiteUrl.
What’s missing for “cross-domain authentication” coverage is a direct post-SSO check that the mapped domain stays authenticated (e.g., after the final step, revisit ${mappedDomainUrl}/wp-admin/ and assert you land on the admin UI without being re-triggered into login/SSO). Update the test name/comments to reflect the wp-env handoff behavior, or add the mapped-domain verification as above.
🤖 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/e2e/cypress/integration/060-sso-cross-domain.spec.js` around lines 89 -
99, The test name/comments in 060-sso-cross-domain.spec.js should be clarified
to state it verifies the wp-env main-origin handoff behavior, and you should add
a post-SSO verification that the mapped domain remains authenticated: after the
existing assertions that the browser landed on mainSiteUrl with /wp-admin/ and
`#wpadminbar`, navigate to `${mappedDomainUrl}/wp-admin/` and assert you land on
the admin UI (URL includes `/wp-admin/`) and `#wpadminbar` exists without being
redirected back into the SSO flow; update the spec title/comment to mention
"wp-env main-origin handoff" or add the mapped-domain check to cover
cross-domain authentication.
|
Permission check failed for this PR (HTTP unknown from collaborator permission API). Unable to determine if @superdav42 is a maintainer or external contributor. A maintainer must review and merge this PR manually. This is a fail-closed safety measure — the pulse will not auto-merge until the permission API succeeds. aidevops.sh v3.20.40 plugin for OpenCode v1.16.2 with gpt-5.5 spent 1m and 32,332 tokens on this as a headless worker. |
|
Correction: the previous “Permission check failed” notice was posted by a supervisor gate invocation error, not by an actual collaborator-permission API failure.
aidevops.sh v3.20.40 plugin for OpenCode v1.16.2 with gpt-5.5 spent 4m and 74,570 tokens on this as a headless worker. Overall, 4h 45m since this issue was created. |
superdav42
added
the
review-feedback-scanned
1 participant
Summary
sso-test.ultimate-multisite.testso Cypress can reach wp-env on the existing port while preserving a mapped-domain Host headersso-test.ultimate-multisite.test:PORTdirectly in the domain mapping table060-sso-cross-domain.spec.jsand align the redirect-loop SSO spec with the same mapped hostnameResolves #1322
Verification
php -l tests/e2e/cypress/fixtures/setup-sso-test.phpnode --check tests/e2e/cypress/integration/060-sso-cross-domain.spec.jsnode --check tests/e2e/cypress/integration/065-sso-redirect-loop.spec.jsgit diff --check origin/main...HEADNot run locally: full Cypress/wp-env spec, because this environment is missing installed npm dependencies and requires Docker/wp-env.
aidevops.sh v3.20.36 plugin for OpenCode v1.16.2 with gpt-5.5
Summary by CodeRabbit