Conversation
WalkthroughThe changes extend domain redirect customization via a new WordPress filter, enhance login form redirect handling with query parameter sanitization and fallback logic, and introduce comprehensive end-to-end testing infrastructure for domain mapping roles including Cypress commands, a test suite, and documentation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
tests/e2e/cypress/integration/domain-mapping-roles.md (1)
59-63: Add a language hint to the fenced block.markdownlint (MD040) is flagging this block because the fence lacks a language specifier. Appending something like ```text keeps the docs clean and lint-happy. Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
inc/ui/class-domain-mapping-element.php(1 hunks)inc/ui/class-login-form-element.php(2 hunks)tests/e2e/cypress/integration/domain-mapping-roles.md(1 hunks)tests/e2e/cypress/integration/domain-mapping-roles.spec.js(1 hunks)tests/e2e/cypress/support/commands/domain-mapping.js(1 hunks)tests/e2e/cypress/support/commands/index.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
inc/ui/class-domain-mapping-element.php (1)
inc/functions/url.php (1)
wu_get_current_url(18-28)
🪛 markdownlint-cli2 (0.18.1)
tests/e2e/cypress/integration/domain-mapping-roles.md
59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cypress (8.2, chrome)
- GitHub Check: cypress (8.1, chrome)
🔇 Additional comments (5)
inc/ui/class-domain-mapping-element.php (1)
639-656: Filter contract looks solid.Nice touch exposing
wu_make_primary_domain_redirect_urlwith full context; this keeps the default behavior while opening the door for custom flows without hacks.inc/ui/class-login-form-element.php (2)
741-752: Redirect handling improvement looks good.Fetching
redirect_tofrom the query string and sanitizing it before use keeps behavior flexible while guarding against sketchy URLs. 👍
778-780: Comment tweak appreciated.The clarified wording better reflects the data structure we’re touching.
tests/e2e/cypress/support/commands/index.js (1)
1-4: Domain-mapping commands now wired up.Importing the new helpers keeps the command registry complete.
tests/e2e/cypress/support/commands/domain-mapping.js (1)
141-175: Verify that the Host header approach works in your test environment.The approach of setting a custom
Hostheader incy.visit()may not work reliably across all browsers and Cypress versions. The multiple commented alternatives (lines 160-172) suggest uncertainty about the correct approach.Additionally, using
failOnStatusCode: false(line 157) might hide legitimate errors when the domain mapping doesn't work correctly.Consider these actions:
- Test whether the Host header approach actually works with your Cypress configuration
- Add assertions after the visit to verify the correct site loaded
- Document which approach is recommended based on testing
If the Host header approach doesn't work, consider setting up actual DNS resolution in your test environment or using the query parameter approach mentioned in the comments.
| cy.log("Creating test site and user"); | ||
|
|
||
| // Create a new site | ||
| cy.createTestSite(testSite.path, testSite.title).then((siteId) => { | ||
| testSite.id = siteId; | ||
| cy.log(`Created site with ID: ${siteId}`); | ||
|
|
||
| // Create a test user on this site with editor role | ||
| cy.createTestUser( | ||
| testUser.username, | ||
| testUser.email, | ||
| testUser.password, | ||
| testUser.role, | ||
| siteId | ||
| ).then((userId) => { | ||
| testUser.id = userId; | ||
| cy.log(`Created user with ID: ${userId} and role: ${testUser.role}`); | ||
| }); | ||
| }); | ||
|
|
||
| // Login as the test user | ||
| cy.loginByApi(testUser.username, testUser.password); | ||
|
|
||
| // Switch to the test site | ||
| cy.switchToSite(testSite.id); | ||
|
|
||
| // Visit the site admin | ||
| cy.visit(`/wp-admin/`); | ||
|
|
||
| // Verify user is logged in and has access to admin | ||
| cy.get('#wpadminbar').should('be.visible'); | ||
|
|
||
| // Check that user has editor capabilities | ||
| // Editors should see Posts menu | ||
| cy.get('#menu-posts').should('be.visible'); | ||
|
|
||
| // Editors should NOT see Users menu (admin only) | ||
| cy.get('#menu-users').should('not.exist'); | ||
|
|
||
| // Verify user role via API/wp-admin | ||
| cy.wpCli(`user get ${testUser.id} --field=roles --format=json`, { | ||
| failOnNonZeroExit: false | ||
| }).then((result) => { | ||
| if (result.code === 0) { | ||
| const roles = JSON.parse(result.stdout); | ||
| expect(roles).to.include(testUser.role); | ||
| } | ||
| }); | ||
|
|
||
| cy.log("✓ Baseline test passed - roles work on original subdomain"); | ||
| }); | ||
|
|
||
| /** | ||
| * Test 2: Verify user roles are loaded on mapped domain |
There was a problem hiding this comment.
Fix command ordering so the user exists before login.
Right now cy.loginByApi(testUser.username, testUser.password) is queued immediately after cy.createTestSite(...), while cy.createTestUser(...) is scheduled later from inside the .then(...). Cypress will therefore attempt the login before the account is created, so the baseline test flakes/fails. We need to ensure user creation finishes before any login/switch/visit commands run.
One way to keep the order deterministic:
- // Create a new site
- cy.createTestSite(testSite.path, testSite.title).then((siteId) => {
- testSite.id = siteId;
- cy.log(`Created site with ID: ${siteId}`);
-
- // Create a test user on this site with editor role
- cy.createTestUser(
- testUser.username,
- testUser.email,
- testUser.password,
- testUser.role,
- siteId
- ).then((userId) => {
- testUser.id = userId;
- cy.log(`Created user with ID: ${userId} and role: ${testUser.role}`);
- });
- });
-
- // Login as the test user
- cy.loginByApi(testUser.username, testUser.password);
- // Switch to the test site
- cy.switchToSite(testSite.id);
+ cy.createTestSite(testSite.path, testSite.title)
+ .then((siteId) => {
+ testSite.id = siteId;
+ cy.log(`Created site with ID: ${siteId}`);
+
+ return cy
+ .createTestUser(
+ testUser.username,
+ testUser.email,
+ testUser.password,
+ testUser.role,
+ siteId
+ )
+ .then((userId) => {
+ testUser.id = userId;
+ cy.log(
+ `Created user with ID: ${userId} and role: ${testUser.role}`
+ );
+ });
+ })
+ .then(() => {
+ cy.loginByApi(testUser.username, testUser.password);
+ cy.switchToSite(testSite.id);
+ });This guarantees the user exists before the login happens and keeps the rest of the steps in sequence.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cy.log("Creating test site and user"); | |
| // Create a new site | |
| cy.createTestSite(testSite.path, testSite.title).then((siteId) => { | |
| testSite.id = siteId; | |
| cy.log(`Created site with ID: ${siteId}`); | |
| // Create a test user on this site with editor role | |
| cy.createTestUser( | |
| testUser.username, | |
| testUser.email, | |
| testUser.password, | |
| testUser.role, | |
| siteId | |
| ).then((userId) => { | |
| testUser.id = userId; | |
| cy.log(`Created user with ID: ${userId} and role: ${testUser.role}`); | |
| }); | |
| }); | |
| // Login as the test user | |
| cy.loginByApi(testUser.username, testUser.password); | |
| // Switch to the test site | |
| cy.switchToSite(testSite.id); | |
| // Visit the site admin | |
| cy.visit(`/wp-admin/`); | |
| // Verify user is logged in and has access to admin | |
| cy.get('#wpadminbar').should('be.visible'); | |
| // Check that user has editor capabilities | |
| // Editors should see Posts menu | |
| cy.get('#menu-posts').should('be.visible'); | |
| // Editors should NOT see Users menu (admin only) | |
| cy.get('#menu-users').should('not.exist'); | |
| // Verify user role via API/wp-admin | |
| cy.wpCli(`user get ${testUser.id} --field=roles --format=json`, { | |
| failOnNonZeroExit: false | |
| }).then((result) => { | |
| if (result.code === 0) { | |
| const roles = JSON.parse(result.stdout); | |
| expect(roles).to.include(testUser.role); | |
| } | |
| }); | |
| cy.log("✓ Baseline test passed - roles work on original subdomain"); | |
| }); | |
| /** | |
| * Test 2: Verify user roles are loaded on mapped domain | |
| cy.log("Creating test site and user"); | |
| cy.createTestSite(testSite.path, testSite.title) | |
| .then((siteId) => { | |
| testSite.id = siteId; | |
| cy.log(`Created site with ID: ${siteId}`); | |
| return cy | |
| .createTestUser( | |
| testUser.username, | |
| testUser.email, | |
| testUser.password, | |
| testUser.role, | |
| siteId | |
| ) | |
| .then((userId) => { | |
| testUser.id = userId; | |
| cy.log( | |
| `Created user with ID: ${userId} and role: ${testUser.role}` | |
| ); | |
| }); | |
| }) | |
| .then(() => { | |
| cy.loginByApi(testUser.username, testUser.password); | |
| cy.switchToSite(testSite.id); | |
| }); | |
| // Visit the site admin | |
| cy.visit(`/wp-admin/`); | |
| // Verify user is logged in and has access to admin | |
| cy.get('#wpadminbar').should('be.visible'); | |
| // Check that user has editor capabilities | |
| // Editors should see Posts menu | |
| cy.get('#menu-posts').should('be.visible'); | |
| // Editors should NOT see Users menu (admin only) | |
| cy.get('#menu-users').should('not.exist'); | |
| // Verify user role via API/wp-admin | |
| cy.wpCli(`user get ${testUser.id} --field=roles --format=json`, { | |
| failOnNonZeroExit: false | |
| }).then((result) => { | |
| if (result.code === 0) { | |
| const roles = JSON.parse(result.stdout); | |
| expect(roles).to.include(testUser.role); | |
| } | |
| }); | |
| cy.log("✓ Baseline test passed - roles work on original subdomain"); | |
| }); | |
| /** | |
| * Test 2: Verify user roles are loaded on mapped domain |
🤖 Prompt for AI Agents
In tests/e2e/cypress/integration/domain-mapping-roles.spec.js around lines 51 to
104, the test queues cy.loginByApi() immediately after initiating
cy.createTestSite(), but cy.createTestUser() runs inside the site-creation
.then(), so Cypress may attempt login before the user exists; move the login,
switchToSite, visit and subsequent assertions into the .then() callback that
runs after cy.createTestUser() completes (or explicitly chain/return the
user-creation promise) so the test user is created and testUser.id is set before
calling cy.loginByApi(), cy.switchToSite(), cy.visit() and the role checks.
| Cypress.Commands.add("createTestSite", (path, title = "Test Site") => { | ||
| return cy.wpCli( | ||
| `site create --slug="${path}" --title="${title}" --email="admin@example.com" --porcelain`, | ||
| { failOnNonZeroExit: true } | ||
| ).then((result) => { | ||
| const siteId = parseInt(result.stdout.trim(), 10); | ||
| cy.log(`Created site: ${path} (ID: ${siteId})`); | ||
| return cy.wrap(siteId); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: Command injection vulnerability in createTestSite.
The path and title parameters are directly interpolated into the WP-CLI command without sanitization. An attacker could inject additional command flags or shell metacharacters.
For example: createTestSite('test" --admin_user=attacker --role=administrator #', 'title') could inject malicious flags.
Sanitize or escape parameters before interpolation. Consider using WP-CLI's support for passing arguments separately if available, or at minimum escape double quotes and shell metacharacters:
Cypress.Commands.add("createTestSite", (path, title = "Test Site") => {
+ // Escape parameters to prevent command injection
+ const escapedPath = path.replace(/["\\$`]/g, '\\$&');
+ const escapedTitle = title.replace(/["\\$`]/g, '\\$&');
+
return cy.wpCli(
- `site create --slug="${path}" --title="${title}" --email="admin@example.com" --porcelain`,
+ `site create --slug="${escapedPath}" --title="${escapedTitle}" --email="admin@example.com" --porcelain`,
{ failOnNonZeroExit: true }
).then((result) => {🤖 Prompt for AI Agents
In tests/e2e/cypress/support/commands/domain-mapping.js around lines 15 to 24,
the createTestSite helper interpolates user-supplied path and title directly
into a shell command causing a command-injection vulnerability; fix by
validating and escaping inputs before building the WP-CLI invocation or,
preferably, switch to calling cy.wpCli with an argument array/API that passes
parameters separately (avoid string concatenation), e.g. restrict path/title to
an allowlist pattern (alphanumeric, hyphen, underscore), escape
quotes/backslashes/newlines if you must interpolate, or use a proper
shell-escaping utility so no untrusted characters can inject flags or commands.
| Cypress.Commands.add("createTestUser", (username, email, password, role = "subscriber", siteId = null) => { | ||
| let command = `user create "${username}" "${email}" --role="${role}" --user_pass="${password}" --porcelain`; | ||
|
|
||
| // If site ID is provided, create user on that site | ||
| if (siteId) { | ||
| command += ` --url=${siteId}`; | ||
| } | ||
|
|
||
| return cy.wpCli(command, { failOnNonZeroExit: true }).then((result) => { | ||
| const userId = parseInt(result.stdout.trim(), 10); | ||
| cy.log(`Created user: ${username} (ID: ${userId}, Role: ${role})`); | ||
| return cy.wrap(userId); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: Command injection vulnerability and incorrect flag usage.
Two issues here:
-
Command injection: All parameters (
username,email,password,role) are interpolated without sanitization, creating a critical security vulnerability. Thepasswordparameter is especially dangerous. -
Incorrect flag: Line 41 uses
--url=${siteId}butsiteIdis numeric. WP-CLI's--urlexpects a URL string. This should likely be--blog_id=${siteId}or omitted in favor of usingswitch_to_blog().
Apply this diff to address both issues:
Cypress.Commands.add("createTestUser", (username, email, password, role = "subscriber", siteId = null) => {
+ // Escape parameters to prevent command injection
+ const escapedUsername = username.replace(/["\\$`]/g, '\\$&');
+ const escapedEmail = email.replace(/["\\$`]/g, '\\$&');
+ const escapedPassword = password.replace(/["\\$`]/g, '\\$&');
+ const escapedRole = role.replace(/["\\$`]/g, '\\$&');
+
- let command = `user create "${username}" "${email}" --role="${role}" --user_pass="${password}" --porcelain`;
+ let command = `user create "${escapedUsername}" "${escapedEmail}" --role="${escapedRole}" --user_pass="${escapedPassword}" --porcelain`;
// If site ID is provided, create user on that site
if (siteId) {
- command += ` --url=${siteId}`;
+ command += ` --blog_id=${siteId}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cypress.Commands.add("createTestUser", (username, email, password, role = "subscriber", siteId = null) => { | |
| let command = `user create "${username}" "${email}" --role="${role}" --user_pass="${password}" --porcelain`; | |
| // If site ID is provided, create user on that site | |
| if (siteId) { | |
| command += ` --url=${siteId}`; | |
| } | |
| return cy.wpCli(command, { failOnNonZeroExit: true }).then((result) => { | |
| const userId = parseInt(result.stdout.trim(), 10); | |
| cy.log(`Created user: ${username} (ID: ${userId}, Role: ${role})`); | |
| return cy.wrap(userId); | |
| }); | |
| }); | |
| Cypress.Commands.add("createTestUser", (username, email, password, role = "subscriber", siteId = null) => { | |
| // Escape parameters to prevent command injection | |
| const escapedUsername = username.replace(/["\\$`]/g, '\\$&'); | |
| const escapedEmail = email.replace(/["\\$`]/g, '\\$&'); | |
| const escapedPassword = password.replace(/["\\$`]/g, '\\$&'); | |
| const escapedRole = role.replace(/["\\$`]/g, '\\$&'); | |
| let command = `user create "${escapedUsername}" "${escapedEmail}" --role="${escapedRole}" --user_pass="${escapedPassword}" --porcelain`; | |
| // If site ID is provided, create user on that site | |
| if (siteId) { | |
| command += ` --blog_id=${siteId}`; | |
| } | |
| return cy.wpCli(command, { failOnNonZeroExit: true }).then((result) => { | |
| const userId = parseInt(result.stdout.trim(), 10); | |
| cy.log(`Created user: ${username} (ID: ${userId}, Role: ${role})`); | |
| return cy.wrap(userId); | |
| }); | |
| }); |
| Cypress.Commands.add("switchToSite", (siteId) => { | ||
| cy.log(`Switching to site ID: ${siteId}`); | ||
|
|
||
| // Get site details to find the path | ||
| return cy.wpCli(`site list --field=url --format=csv`, { failOnNonZeroExit: false }) | ||
| .then((result) => { | ||
| if (result.code === 0) { | ||
| const urls = result.stdout.trim().split('\n'); | ||
| // The site URL will be in the list | ||
| cy.log(`Available sites: ${urls.join(', ')}`); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Misleading command name and incomplete implementation.
The switchToSite command doesn't actually switch to the specified site—it only logs available site URLs. This creates confusion and could lead to incorrect test logic where developers assume context has been switched when it hasn't.
Either implement actual site switching (e.g., by setting appropriate cookies/sessions or using WP-CLI's context switching), or rename this command to something like logAvailableSites to accurately reflect its behavior.
Consider implementing actual site switching or removing this command if it's not needed:
-Cypress.Commands.add("switchToSite", (siteId) => {
- cy.log(`Switching to site ID: ${siteId}`);
-
- // Get site details to find the path
- return cy.wpCli(`site list --field=url --format=csv`, { failOnNonZeroExit: false })
- .then((result) => {
- if (result.code === 0) {
- const urls = result.stdout.trim().split('\n');
- // The site URL will be in the list
- cy.log(`Available sites: ${urls.join(', ')}`);
- }
- });
-});
+// Removed: switchToSite command was not actually switching context.
+// Site context switching should be handled via WordPress authentication
+// and URL navigation in individual tests.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cypress.Commands.add("switchToSite", (siteId) => { | |
| cy.log(`Switching to site ID: ${siteId}`); | |
| // Get site details to find the path | |
| return cy.wpCli(`site list --field=url --format=csv`, { failOnNonZeroExit: false }) | |
| .then((result) => { | |
| if (result.code === 0) { | |
| const urls = result.stdout.trim().split('\n'); | |
| // The site URL will be in the list | |
| cy.log(`Available sites: ${urls.join(', ')}`); | |
| } | |
| }); | |
| }); | |
| // Removed: switchToSite command was not actually switching context. | |
| // Site context switching should be handled via WordPress authentication | |
| // and URL navigation in individual tests. |
🤖 Prompt for AI Agents
In tests/e2e/cypress/support/commands/domain-mapping.js around lines 56-68, the
Cypress command is misleadingly named `switchToSite` but only logs available
site URLs and does not change test context; either implement real switching or
rename the command to reflect its logging behavior. To implement switching:
accept the siteId, call WP-CLI to map siteId to its URL/path (parse `wp site
list --format=csv` or use `--url=<site>` to query one site), locate the matching
URL, then set the test context by visiting the site's admin or front-end (e.g.,
cy.visit(`${foundUrl}/wp-admin`) or set a cookie/session needed for subsequent
requests) and ensure future cy.wpCli calls include `--url=${foundUrl}`;
alternatively, if switching is not needed, rename the command to
`logAvailableSites` (and update all call sites) and keep only the logging
behavior.
| Cypress.Commands.add("addDomainMapping", (siteId, domain, active = true, primary = true) => { | ||
| cy.log(`Adding domain mapping: ${domain} → Site ${siteId}`); | ||
|
|
||
| // Use wp-cli to add domain mapping via eval-file or direct database insertion | ||
| const phpCode = ` | ||
| require_once 'wp-load.php'; | ||
|
|
||
| $domain_data = array( | ||
| 'blog_id' => ${siteId}, | ||
| 'domain' => '${domain}', | ||
| 'active' => ${active ? 1 : 0}, | ||
| 'primary_domain' => ${primary ? 1 : 0}, | ||
| 'secure' => 0, | ||
| 'stage' => 'live' | ||
| ); | ||
|
|
||
| // Try to use Ultimate Multisite's domain creation | ||
| if (function_exists('wu_create_domain')) { | ||
| $domain_obj = wu_create_domain($domain_data); | ||
| if (is_wp_error($domain_obj)) { | ||
| echo 'ERROR: ' . $domain_obj->get_error_message(); | ||
| exit(1); | ||
| } | ||
| echo 'Domain mapping created: ID ' . $domain_obj->get_id(); | ||
| } else { | ||
| // Fallback to direct database insertion | ||
| global $wpdb; | ||
| $table = $wpdb->base_prefix . 'wu_domains'; | ||
| $wpdb->insert($table, $domain_data); | ||
| echo 'Domain mapping created via database'; | ||
| } | ||
| `; | ||
|
|
||
| // Create a temporary PHP file | ||
| const tempFile = `/tmp/add-domain-${Date.now()}.php`; | ||
|
|
||
| return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false }) | ||
| .then(() => { | ||
| return cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }); | ||
| }) | ||
| .then((result) => { | ||
| // Clean up temp file | ||
| cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false }); | ||
|
|
||
| if (result.code === 0) { | ||
| cy.log(`✓ Domain mapping created: ${domain}`); | ||
| } else { | ||
| cy.log(`⚠ Domain mapping creation may have failed: ${result.stderr}`); | ||
| } | ||
|
|
||
| return cy.wrap(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: SQL injection and command injection vulnerabilities.
Multiple security issues in this command:
-
SQL injection (lines 86-87): The
domainandsiteIdparameters are directly interpolated into PHP code without escaping. A malicious domain likeexample.com', 1); DROP TABLE wp_users; --could execute arbitrary SQL. -
Command injection (line 114): The
echo '<?php ${phpCode}'command is vulnerable if any interpolated values contain shell metacharacters. -
Race condition (line 112): Using
Date.now()for temp file names can cause collisions in parallel test execution.
Refactor to use safer approaches:
Cypress.Commands.add("addDomainMapping", (siteId, domain, active = true, primary = true) => {
cy.log(`Adding domain mapping: ${domain} → Site ${siteId}`);
+ // Validate and sanitize inputs
+ const sanitizedDomain = domain.replace(/'/g, "\\'");
+ const sanitizedSiteId = parseInt(siteId, 10);
+ if (isNaN(sanitizedSiteId)) {
+ throw new Error('Invalid site ID');
+ }
+
const phpCode = `
require_once 'wp-load.php';
+
+ // Use prepared statement-like approach or proper escaping
+ global \\$wpdb;
$domain_data = array(
- 'blog_id' => ${siteId},
- 'domain' => '${domain}',
+ 'blog_id' => ${sanitizedSiteId},
+ 'domain' => '${sanitizedDomain}',
'active' => ${active ? 1 : 0},
'primary_domain' => ${primary ? 1 : 0},Also consider using a more robust temp file naming strategy:
- const tempFile = `/tmp/add-domain-${Date.now()}.php`;
+ const tempFile = `/tmp/add-domain-${Date.now()}-${Math.random().toString(36).substring(7)}.php`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cypress.Commands.add("addDomainMapping", (siteId, domain, active = true, primary = true) => { | |
| cy.log(`Adding domain mapping: ${domain} → Site ${siteId}`); | |
| // Use wp-cli to add domain mapping via eval-file or direct database insertion | |
| const phpCode = ` | |
| require_once 'wp-load.php'; | |
| $domain_data = array( | |
| 'blog_id' => ${siteId}, | |
| 'domain' => '${domain}', | |
| 'active' => ${active ? 1 : 0}, | |
| 'primary_domain' => ${primary ? 1 : 0}, | |
| 'secure' => 0, | |
| 'stage' => 'live' | |
| ); | |
| // Try to use Ultimate Multisite's domain creation | |
| if (function_exists('wu_create_domain')) { | |
| $domain_obj = wu_create_domain($domain_data); | |
| if (is_wp_error($domain_obj)) { | |
| echo 'ERROR: ' . $domain_obj->get_error_message(); | |
| exit(1); | |
| } | |
| echo 'Domain mapping created: ID ' . $domain_obj->get_id(); | |
| } else { | |
| // Fallback to direct database insertion | |
| global $wpdb; | |
| $table = $wpdb->base_prefix . 'wu_domains'; | |
| $wpdb->insert($table, $domain_data); | |
| echo 'Domain mapping created via database'; | |
| } | |
| `; | |
| // Create a temporary PHP file | |
| const tempFile = `/tmp/add-domain-${Date.now()}.php`; | |
| return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false }) | |
| .then(() => { | |
| return cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }); | |
| }) | |
| .then((result) => { | |
| // Clean up temp file | |
| cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false }); | |
| if (result.code === 0) { | |
| cy.log(`✓ Domain mapping created: ${domain}`); | |
| } else { | |
| cy.log(`⚠ Domain mapping creation may have failed: ${result.stderr}`); | |
| } | |
| return cy.wrap(true); | |
| }); | |
| }); | |
| Cypress.Commands.add("addDomainMapping", (siteId, domain, active = true, primary = true) => { | |
| cy.log(`Adding domain mapping: ${domain} → Site ${siteId}`); | |
| // Validate and sanitize inputs | |
| const sanitizedDomain = domain.replace(/'/g, "\\'"); | |
| const sanitizedSiteId = parseInt(siteId, 10); | |
| if (isNaN(sanitizedSiteId)) { | |
| throw new Error('Invalid site ID'); | |
| } | |
| // Use wp-cli to add domain mapping via eval-file or direct database insertion | |
| const phpCode = ` | |
| require_once 'wp-load.php'; | |
| // Use prepared statement-like approach or proper escaping | |
| global \$wpdb; | |
| $domain_data = array( | |
| 'blog_id' => ${sanitizedSiteId}, | |
| 'domain' => '${sanitizedDomain}', | |
| 'active' => ${active ? 1 : 0}, | |
| 'primary_domain' => ${primary ? 1 : 0}, | |
| 'secure' => 0, | |
| 'stage' => 'live' | |
| ); | |
| // Try to use Ultimate Multisite's domain creation | |
| if (function_exists('wu_create_domain')) { | |
| $domain_obj = wu_create_domain($domain_data); | |
| if (is_wp_error($domain_obj)) { | |
| echo 'ERROR: ' . $domain_obj->get_error_message(); | |
| exit(1); | |
| } | |
| echo 'Domain mapping created: ID ' . $domain_obj->get_id(); | |
| } else { | |
| // Fallback to direct database insertion | |
| global $wpdb; | |
| $table = $wpdb->base_prefix . 'wu_domains'; | |
| $wpdb->insert($table, $domain_data); | |
| echo 'Domain mapping created via database'; | |
| } | |
| `; | |
| // Create a temporary PHP file | |
| const tempFile = `/tmp/add-domain-${Date.now()}-${Math.random().toString(36).substring(7)}.php`; | |
| return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false }) | |
| .then(() => { | |
| return cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }); | |
| }) | |
| .then((result) => { | |
| // Clean up temp file | |
| cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false }); | |
| if (result.code === 0) { | |
| cy.log(`✓ Domain mapping created: ${domain}`); | |
| } else { | |
| cy.log(`⚠ Domain mapping creation may have failed: ${result.stderr}`); | |
| } | |
| return cy.wrap(true); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
tests/e2e/cypress/support/commands/domain-mapping.js lines 78-130: the command
builds a PHP script by interpolating unescaped siteId and domain into shell/SQL
which creates SQL and command injection risks and uses a non-unique temp
filename; fix by validating/casting siteId to an integer in JS,
sanitizing/escaping domain (e.g., allow only expected hostname characters or use
encodeURIComponent and validate pattern), change the PHP to use safe DB APIs
($wpdb->insert with prepared data or $wpdb->prepare) and avoid raw string
interpolation into SQL, stop using shell echo to write the file — create the
temp file via a Cypress task or Node fs.mkdtemp + fs.writeFile to avoid shell
metacharacters, generate a cryptographically unique temp filename (UUID or
fs.mkdtemp) to avoid races, and ensure all values are passed to PHP as sanitized
inputs rather than directly interpolated in a shell command or unescaped PHP
string.
| Cypress.Commands.add("deactivateDomainMapping", (siteId, domain) => { | ||
| cy.log(`Deactivating domain mapping: ${domain}`); | ||
|
|
||
| const phpCode = ` | ||
| require_once 'wp-load.php'; | ||
|
|
||
| if (function_exists('wu_get_domain_by_domain')) { | ||
| $domain_obj = wu_get_domain_by_domain('${domain}'); | ||
| if ($domain_obj) { | ||
| $domain_obj->set_active(false); | ||
| $domain_obj->save(); | ||
| echo 'Domain deactivated'; | ||
| } else { | ||
| echo 'Domain not found'; | ||
| } | ||
| } else { | ||
| global $wpdb; | ||
| $table = $wpdb->base_prefix . 'wu_domains'; | ||
| $wpdb->update($table, | ||
| array('active' => 0), | ||
| array('domain' => '${domain}', 'blog_id' => ${siteId}) | ||
| ); | ||
| echo 'Domain deactivated via database'; | ||
| } | ||
| `; | ||
|
|
||
| const tempFile = `/tmp/deactivate-domain-${Date.now()}.php`; | ||
|
|
||
| return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false }) | ||
| .then(() => { | ||
| return cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }); | ||
| }) | ||
| .then((result) => { | ||
| cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false }); | ||
| cy.log(`✓ Domain mapping deactivated: ${domain}`); | ||
| return cy.wrap(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: SQL injection vulnerability (same pattern as addDomainMapping).
This command has the same security vulnerabilities as addDomainMapping:
- SQL injection (lines 190, 203): The
domainparameter is directly interpolated into PHP code without escaping - Command injection (line 211): The
echocommand is vulnerable to shell injection - Race condition (line 209): Temp file naming collision risk in parallel execution
Apply similar sanitization as recommended for addDomainMapping:
Cypress.Commands.add("deactivateDomainMapping", (siteId, domain) => {
cy.log(`Deactivating domain mapping: ${domain}`);
+
+ // Sanitize inputs
+ const sanitizedDomain = domain.replace(/'/g, "\\'");
+ const sanitizedSiteId = parseInt(siteId, 10);
+ if (isNaN(sanitizedSiteId)) {
+ throw new Error('Invalid site ID');
+ }
const phpCode = `
require_once 'wp-load.php';
if (function_exists('wu_get_domain_by_domain')) {
- $domain_obj = wu_get_domain_by_domain('${domain}');
+ $domain_obj = wu_get_domain_by_domain('${sanitizedDomain}');Consider extracting the temp file execution pattern into a shared helper function to reduce code duplication and ensure consistent security handling.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/e2e/cypress/support/commands/domain-mapping.js around lines 183-220,
the deactivation helper directly interpolates siteId and domain into a
shell-echoed PHP file which creates SQL and command-injection and temp-file race
vulnerabilities; fix by writing the PHP file via cy.writeFile (not shell echo)
to a collision-resistant filename (include a UUID or timestamp+random), pass the
domain and siteId safely by embedding a JSON-encoded payload into the PHP and
decoding it (or by reading from STDIN) instead of raw string interpolation so
PHP uses the decoded values when updating the DB, avoid executing
shell-constructed commands (use cy.wpCli('eval-file', ...) without shell-echo)
and remove the temp file after use; consider extracting the temp-file + exec
pattern into a shared helper that performs these safe steps.
| Cypress.Commands.add("deleteDomainMapping", (siteId, domain) => { | ||
| cy.log(`Deleting domain mapping: ${domain}`); | ||
|
|
||
| const phpCode = ` | ||
| require_once 'wp-load.php'; | ||
|
|
||
| if (function_exists('wu_get_domain_by_domain')) { | ||
| $domain_obj = wu_get_domain_by_domain('${domain}'); | ||
| if ($domain_obj) { | ||
| $domain_obj->delete(); | ||
| echo 'Domain deleted'; | ||
| } else { | ||
| echo 'Domain not found'; | ||
| } | ||
| } else { | ||
| global $wpdb; | ||
| $table = $wpdb->base_prefix . 'wu_domains'; | ||
| $wpdb->delete($table, | ||
| array('domain' => '${domain}', 'blog_id' => ${siteId}) | ||
| ); | ||
| echo 'Domain deleted via database'; | ||
| } | ||
| `; | ||
|
|
||
| const tempFile = `/tmp/delete-domain-${Date.now()}.php`; | ||
|
|
||
| return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false }) | ||
| .then(() => { | ||
| return cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }); | ||
| }) | ||
| .then((result) => { | ||
| cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false }); | ||
| cy.log(`✓ Domain mapping deleted: ${domain}`); | ||
| return cy.wrap(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: SQL injection vulnerability and code duplication.
This command has identical security vulnerabilities as addDomainMapping and deactivateDomainMapping. Additionally, the pattern of creating temp PHP files, executing them, and cleaning up is duplicated across multiple commands.
- Apply the same input sanitization as recommended for previous commands
- Extract the temp file execution pattern into a shared helper function:
/**
* Execute PHP code via WP-CLI eval-file with proper cleanup
* @param {string} phpCode - PHP code to execute (without <?php tag)
* @param {string} commandName - Name for logging/temp file
* @returns Cypress chainable with execution result
*/
function executePhpCode(phpCode, commandName) {
const tempFile = `/tmp/${commandName}-${Date.now()}-${Math.random().toString(36).substring(7)}.php`;
return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false })
.then(() => cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }))
.then((result) => {
cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false });
return cy.wrap(result);
});
}Then use this helper in all commands that need PHP execution, reducing duplication and centralizing security fixes.
🤖 Prompt for AI Agents
tests/e2e/cypress/support/commands/domain-mapping.js lines 228-263: the
deleteDomainMapping command builds PHP with unsanitized interpolation and
duplicates the temp-file exec pattern; sanitize inputs (escape/safely quote
domain and siteId or validate types) before embedding into the PHP string to
prevent SQL injection, then refactor the temp-file creation/exec/cleanup into a
shared helper (executePhpCode) that accepts the PHP payload and a commandName,
uses a unique tempfile name, runs cy.exec to write the file, calls cy.wpCli
eval-file, removes the temp file in a finally-like chain, and returns the
wrapped result; update deleteDomainMapping to call that helper and remove
duplicated logic so all php-execution commands share the centralized, sanitized
flow.
| Cypress.Commands.add("getUserRoles", (userId, siteId = null) => { | ||
| let command = `user get ${userId} --field=roles --format=json`; | ||
|
|
||
| if (siteId) { | ||
| command += ` --url=${siteId}`; | ||
| } | ||
|
|
||
| return cy.wpCli(command, { failOnNonZeroExit: false }).then((result) => { | ||
| if (result.code === 0) { | ||
| const roles = JSON.parse(result.stdout); | ||
| cy.log(`User ${userId} roles: ${roles.join(', ')}`); | ||
| return cy.wrap(roles); | ||
| } else { | ||
| cy.log(`Could not get roles for user ${userId}`); | ||
| return cy.wrap([]); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: Command injection vulnerability and incorrect flag usage.
Two issues:
-
Command injection (line 273): The
userIdparameter is directly interpolated without validation. A malicious value like1 --role=administratorcould inject additional flags. -
Incorrect flag (line 276): Same issue as
createTestUser—the--urlflag expects a URL string, not a numeric site ID. Should be--blog_id=${siteId}.
Apply this diff:
Cypress.Commands.add("getUserRoles", (userId, siteId = null) => {
+ // Validate userId is numeric
+ const numericUserId = parseInt(userId, 10);
+ if (isNaN(numericUserId)) {
+ throw new Error('Invalid user ID');
+ }
+
- let command = `user get ${userId} --field=roles --format=json`;
+ let command = `user get ${numericUserId} --field=roles --format=json`;
if (siteId) {
- command += ` --url=${siteId}`;
+ command += ` --blog_id=${siteId}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cypress.Commands.add("getUserRoles", (userId, siteId = null) => { | |
| let command = `user get ${userId} --field=roles --format=json`; | |
| if (siteId) { | |
| command += ` --url=${siteId}`; | |
| } | |
| return cy.wpCli(command, { failOnNonZeroExit: false }).then((result) => { | |
| if (result.code === 0) { | |
| const roles = JSON.parse(result.stdout); | |
| cy.log(`User ${userId} roles: ${roles.join(', ')}`); | |
| return cy.wrap(roles); | |
| } else { | |
| cy.log(`Could not get roles for user ${userId}`); | |
| return cy.wrap([]); | |
| } | |
| }); | |
| }); | |
| Cypress.Commands.add("getUserRoles", (userId, siteId = null) => { | |
| // Validate userId is numeric | |
| const numericUserId = parseInt(userId, 10); | |
| if (isNaN(numericUserId)) { | |
| throw new Error('Invalid user ID'); | |
| } | |
| let command = `user get ${numericUserId} --field=roles --format=json`; | |
| if (siteId) { | |
| command += ` --blog_id=${siteId}`; | |
| } | |
| return cy.wpCli(command, { failOnNonZeroExit: false }).then((result) => { | |
| if (result.code === 0) { | |
| const roles = JSON.parse(result.stdout); | |
| cy.log(`User ${userId} roles: ${roles.join(', ')}`); | |
| return cy.wrap(roles); | |
| } else { | |
| cy.log(`Could not get roles for user ${userId}`); | |
| return cy.wrap([]); | |
| } | |
| }); | |
| }); |
| Cypress.Commands.add("userHasCapability", (userId, capability) => { | ||
| const phpCode = ` | ||
| require_once 'wp-load.php'; | ||
| $user = get_user_by('ID', ${userId}); | ||
| if ($user && $user->has_cap('${capability}')) { | ||
| echo 'true'; | ||
| } else { | ||
| echo 'false'; | ||
| } | ||
| `; | ||
|
|
||
| const tempFile = `/tmp/check-cap-${Date.now()}.php`; | ||
|
|
||
| return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false }) | ||
| .then(() => { | ||
| return cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }); | ||
| }) | ||
| .then((result) => { | ||
| cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false }); | ||
| const hasCap = result.stdout.trim() === 'true'; | ||
| cy.log(`User ${userId} has ${capability}: ${hasCap}`); | ||
| return cy.wrap(hasCap); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Critical: SQL injection vulnerability and code duplication.
Security issues:
-
PHP injection (lines 301-302): The
userIdandcapabilityparameters are directly interpolated into PHP code. A malicious capability likeread') || true; //could bypass the check. -
Type validation:
userIdshould be validated as numeric.
Additionally, this is another instance of the duplicated temp file execution pattern.
Apply input validation:
Cypress.Commands.add("userHasCapability", (userId, capability) => {
+ // Validate inputs
+ const numericUserId = parseInt(userId, 10);
+ if (isNaN(numericUserId)) {
+ throw new Error('Invalid user ID');
+ }
+ const sanitizedCapability = capability.replace(/'/g, "\\'");
+
const phpCode = `
require_once 'wp-load.php';
- $user = get_user_by('ID', ${userId});
- if ($user && $user->has_cap('${capability}')) {
+ $user = get_user_by('ID', ${numericUserId});
+ if ($user && $user->has_cap('${sanitizedCapability}')) {
echo 'true';And refactor to use the shared helper function recommended in the previous comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cypress.Commands.add("userHasCapability", (userId, capability) => { | |
| const phpCode = ` | |
| require_once 'wp-load.php'; | |
| $user = get_user_by('ID', ${userId}); | |
| if ($user && $user->has_cap('${capability}')) { | |
| echo 'true'; | |
| } else { | |
| echo 'false'; | |
| } | |
| `; | |
| const tempFile = `/tmp/check-cap-${Date.now()}.php`; | |
| return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false }) | |
| .then(() => { | |
| return cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }); | |
| }) | |
| .then((result) => { | |
| cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false }); | |
| const hasCap = result.stdout.trim() === 'true'; | |
| cy.log(`User ${userId} has ${capability}: ${hasCap}`); | |
| return cy.wrap(hasCap); | |
| }); | |
| }); | |
| Cypress.Commands.add("userHasCapability", (userId, capability) => { | |
| // Validate inputs | |
| const numericUserId = parseInt(userId, 10); | |
| if (isNaN(numericUserId)) { | |
| throw new Error('Invalid user ID'); | |
| } | |
| const sanitizedCapability = capability.replace(/'/g, "\\'"); | |
| const phpCode = ` | |
| require_once 'wp-load.php'; | |
| $user = get_user_by('ID', ${numericUserId}); | |
| if ($user && $user->has_cap('${sanitizedCapability}')) { | |
| echo 'true'; | |
| } else { | |
| echo 'false'; | |
| } | |
| `; | |
| const tempFile = `/tmp/check-cap-${Date.now()}.php`; | |
| return cy.exec(`echo '<?php ${phpCode}' > ${tempFile}`, { failOnNonZeroExit: false }) | |
| .then(() => { | |
| return cy.wpCli(`eval-file ${tempFile}`, { failOnNonZeroExit: false }); | |
| }) | |
| .then((result) => { | |
| cy.exec(`rm -f ${tempFile}`, { failOnNonZeroExit: false }); | |
| const hasCap = result.stdout.trim() === 'true'; | |
| cy.log(`User ${userId} has ${capability}: ${hasCap}`); | |
| return cy.wrap(hasCap); | |
| }); | |
| }); |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests