add admin notice when invalid COOKIE_DOMAIN is set#261
Conversation
WalkthroughAdds a new admin Configuration_Checker class that validates Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant WP_Ultimo as WP_Ultimo Loader
participant Config as Configuration_Checker
participant WPAdmin as WP admin init
participant Notices as WP_Ultimo()->notices
Browser->>WP_Ultimo: Request admin page
WP_Ultimo->>WP_Ultimo: load_public_apis() (is_admin block)
WP_Ultimo->>WPAdmin: require admin.php
WP_Ultimo->>Config: Configuration_Checker::get_instance()
WPAdmin->>Config: network_admin_init (hook runs)
Config->>Config: check_cookie_domain_configuration()
alt COOKIE_DOMAIN defined and === false on subdomain install
Config->>Notices: push network-admin warning (cookie_domain_false_warning)
Notices->>Browser: warning shown in network-admin
else Otherwise
Config->>Notices: no action
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
🧹 Nitpick comments (2)
inc/admin/class-configuration-checker.php (2)
44-68: Consider performance optimization and broader validation.The check runs on every
network_admin_initrequest. Consider these improvements:
Deduplication: Verify that the notices system (with identifier
'cookie_domain_false_warning') prevents duplicate notices across page loads to avoid performance overhead.Broader validation: The check only catches
COOKIE_DOMAIN === false. Consider also validating:
- Empty strings:
COOKIE_DOMAIN === ''- Invalid domain formats
- Domains that don't match the network's domain structure
Check frequency: If the notice is persistent, consider running this check less frequently (e.g., only on specific admin pages or with transient caching).
For the broader validation, you could extend the check:
// Check if COOKIE_DOMAIN is defined and set to false - if (defined('COOKIE_DOMAIN') && COOKIE_DOMAIN === false) { + if (defined('COOKIE_DOMAIN') && (COOKIE_DOMAIN === false || COOKIE_DOMAIN === '')) { $message = sprintf( // translators: %1$s is the opening code tag, %2$s is the closing code tag, %3$s is a link to WordPress documentation - __('Your <strong>wp-config.php</strong> has %1$sCOOKIE_DOMAIN%2$s set to %1$sfalse%2$s, which can cause authentication and session issues on subdomain multisite installations. Please remove this line from your wp-config.php file or set it to an appropriate value. %3$s', 'ultimate-multisite'), + __('Your <strong>wp-config.php</strong> has %1$sCOOKIE_DOMAIN%2$s set to an invalid value, which can cause authentication and session issues on subdomain multisite installations. Please remove this line from your wp-config.php file or set it to an appropriate value. %3$s', 'ultimate-multisite'),
53-59: HTML in translatable strings is acceptable but be aware of XSS risks.The message includes HTML tags (
<code>,<a>,<strong>) within the translatable string. While this is a common WordPress pattern and generally acceptable, be aware that if translators have malicious intent, they could inject harmful HTML.This is a minor concern given that translation files are typically controlled by the plugin maintainer, but it's worth noting for security awareness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
inc/admin/class-configuration-checker.php(1 hunks)inc/class-wp-ultimo.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
inc/admin/class-configuration-checker.php (1)
inc/class-wp-ultimo.php (2)
WP_Ultimo(23-948)init(115-213)
inc/class-wp-ultimo.php (1)
inc/admin/class-configuration-checker.php (1)
Configuration_Checker(20-69)
🔇 Additional comments (2)
inc/class-wp-ultimo.php (1)
581-584: LGTM! Consistent integration pattern.The Configuration_Checker is initialized following the same pattern as other components in
load_extra_components(). The placement afterNetwork_Usage_Columnsis appropriate for admin-related functionality.However, this depends on the Singleton trait properly initializing the instance (see concerns raised in the Configuration_Checker file review).
inc/admin/class-configuration-checker.php (1)
20-33: The original review comment is incorrect—the Singleton trait does automatically callinit().The
\WP_Ultimo\Traits\Singletontrait has a private constructor and aget_instance()method that explicitly callsstatic::$instance->init()after instantiation (line 36 of the trait). This pattern guarantees that whenConfiguration_Checker::get_instance()is invoked, theinit()method will execute and register the hook on line 32. The code is correct as written.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
inc/admin/class-configuration-checker.php (1)
30-33: Usenetwork_admin_inithook for better performance.The method registers on
admin_initbutcheck_cookie_domain_configuration()immediately exits if not in network admin (Line 45). This causes the method to run unnecessarily on every sub-site admin page load.Apply this diff:
public function init(): void { - add_action('admin_init', [$this, 'check_cookie_domain_configuration']); + add_action('network_admin_init', [$this, 'check_cookie_domain_configuration']); }With this change, the guard clause at lines 45-47 becomes redundant (though harmless as defensive coding).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
inc/admin/class-configuration-checker.php(1 hunks)inc/class-wp-ultimo.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
inc/class-wp-ultimo.php (1)
inc/admin/class-configuration-checker.php (1)
Configuration_Checker(20-72)
inc/admin/class-configuration-checker.php (1)
inc/class-wp-ultimo.php (1)
WP_Ultimo(23-948)
⏰ 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.1, chrome)
- GitHub Check: cypress (8.2, chrome)
🔇 Additional comments (2)
inc/admin/class-configuration-checker.php (1)
44-71: LGTM! Clear detection logic and actionable warning message.The method correctly identifies the problematic
COOKIE_DOMAINconfiguration with appropriate guards for subdomain multisite installs. The warning message is clear, provides context, and includes a documentation link with proper security attributes.inc/class-wp-ultimo.php (1)
427-430: LGTM! Clean integration of the Configuration_Checker.The initialization is appropriately placed in the admin section of
load_public_apis(), follows the singleton pattern consistent with other components in this file, and includes a clear descriptive comment.
there is no 'network_admin_init' hook. Unless you can show me the documentation |
1 participant
Summary by CodeRabbit