-
-
Notifications
You must be signed in to change notification settings - Fork 75
Reverse proxy fix #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reverse proxy fix #191
Changes from 4 commits
79743da
416dc27
8abce99
1624b2e
0ece38c
287f620
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -904,4 +904,154 @@ public function load_integrations(): void { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| do_action('wp_ultimo_host_providers_load'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Register the domain verification endpoint. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @since 2.0.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @return void | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public function register_domain_verification_endpoint(): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| register_rest_route( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'wu/v1', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '/domain-verification/(?P<domain>[a-zA-Z0-9.-]+)', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| array( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'methods' => \WP_REST_Server::READABLE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'callback' => array($this, 'domain_verification_endpoint'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'permission_callback' => '__return_true', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'args' => array( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'domain' => array( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'required' => true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'sanitize_callback' => 'sanitize_text_field', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | |
| * Register the domain verification endpoint. | |
| * | |
| * @since 2.0.0 | |
| * @return void | |
| */ | |
| public function register_domain_verification_endpoint(): void { | |
| register_rest_route( | |
| 'wu/v1', | |
| '/domain-verification/(?P<domain>[a-zA-Z0-9.-]+)', | |
| array( | |
| 'methods' => \WP_REST_Server::READABLE, | |
| 'callback' => array($this, 'domain_verification_endpoint'), | |
| 'permission_callback' => '__return_true', | |
| 'args' => array( | |
| 'domain' => array( | |
| 'required' => true, | |
| 'sanitize_callback' => 'sanitize_text_field', | |
| ), | |
| ), | |
| ) | |
| ); | |
| } | |
| public function init(): void { | |
| $this->enable_rest_api(); | |
| add_action( 'rest_api_init', [ $this, 'register_domain_verification_endpoint' ] ); | |
| $this->enable_wp_cli(); | |
| } |
🤖 Prompt for AI Agents
In inc/managers/class-domain-manager.php around lines 907 to 931, the
register_domain_verification_endpoint() method is defined but never hooked into
WordPress so the route is never registered; wire this method into the REST
bootstrap by hooking it to rest_api_init (or by calling it from the class init()
or the Rest_Api trait's route registration flow) so the endpoint is registered
at runtime—add the appropriate add_action('rest_api_init', ...) registration or
invoke the method from the existing route registration entry point to ensure the
route is registered.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid regenerating the secret on each verification attempt
Regenerating can cause race conditions; fetch the current secret instead.
Apply:
- $expected_secret = $this->generate_domain_verification_secret($domain);
+ $expected_secret = $this->get_domain_verification_secret($domain);📝 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.
| public function verify_domain_with_secret($domain) { | |
| $expected_secret = $this->generate_domain_verification_secret($domain); | |
| $verification_url = "https://{$domain}/wp-json/wu/v1/domain-verification/{$domain}"; | |
| public function verify_domain_with_secret($domain) { | |
| $expected_secret = $this->get_domain_verification_secret($domain); | |
| $verification_url = "https://{$domain}/wp-json/wu/v1/domain-verification/{$domain}"; |
🤖 Prompt for AI Agents
In inc/managers/class-domain-manager.php around lines 1003 to 1007, do not call
generate_domain_verification_secret($domain) during verification because it
re-creates the secret and can cause race conditions; instead retrieve the
already-stored/current secret (e.g. from the option/metadata or a dedicated
getter like get_stored_domain_verification_secret($domain)) and compare that
stored value to the secret presented during verification, falling back to a safe
behaviour if no stored secret exists; update the verification logic to use the
fetched secret and remove the regeneration call.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not disable TLS verification on remote requests
Disabling sslverify invites MITM and weakens transport security.
Apply:
- 'timeout' => 10,
- 'sslverify' => false,
+ 'timeout' => 10,
+ 'sslverify' => apply_filters('wu_verify_domain_secret_sslverify', true),
+ 'redirection' => 3,
+ 'headers' => array('Accept' => 'application/json'),📝 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.
| $verification_url, | |
| array( | |
| 'timeout' => 10, | |
| 'sslverify' => false, | |
| ) | |
| ); | |
| $verification_url, | |
| array( | |
| - 'timeout' => 10, | |
| 'timeout' => 10, | |
| 'sslverify' => apply_filters( 'wu_verify_domain_secret_sslverify', true ), | |
| 'redirection' => 3, | |
| 'headers' => array( | |
| 'Accept' => 'application/json', | |
| ), | |
| ) | |
| ); |
🤖 Prompt for AI Agents
In inc/managers/class-domain-manager.php around lines 1010 to 1015, the call to
wp_remote_get currently disables TLS verification by setting 'sslverify' =>
false; remove that option or set 'sslverify' => true so remote requests use
proper certificate validation, and handle valid certificate errors explicitly
(e.g. catch wp_remote_get errors and log/return a clear error if verification
fails, or allow specifying a CA bundle if you must trust a custom CA). Ensure
you do not ship code that disables TLS verification and add a comment noting why
sslverify must remain true.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -397,6 +397,18 @@ public function has_correct_dns() { | |
|
|
||
| $domain_url = $this->get_domain(); | ||
|
|
||
| $domain_manager = \WP_Ultimo\Managers\Domain_Manager::get_instance(); | ||
|
|
||
| $secret_verification_enabled = apply_filters('wu_enable_secret_domain_verification', true, $this); | ||
|
|
||
| if ( $secret_verification_enabled ) { | ||
| $secret_result = $domain_manager->verify_domain_with_secret($domain_url); | ||
|
|
||
| if ( $secret_result ) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
|
Comment on lines
+400
to
+405
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Loopback short-circuit is fine; ensure TLS verification is actually enforced This path depends on Domain_Manager::verify_domain_with_loopback_request(). In that method, the “HTTPS with SSL verification” variant currently uses sslverify tied to https_local_ssl_verify with a default false. That disables certificate verification by default and invites MITM. Proposed fix in inc/managers/class-domain-manager.php: - 'sslverify' => apply_filters('https_local_ssl_verify', false),
+ 'sslverify' => apply_filters('https_local_ssl_verify', true),Also ensure the log label matches behavior (i.e., only claim “with SSL verification” when sslverify is true). |
||
| $network_ip_address = Helper::get_network_public_ip(); | ||
|
|
||
| $results = \WP_Ultimo\Managers\Domain_Manager::dns_get_record($domain_url); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1679,7 +1679,11 @@ public function save() { | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| $user_id = $customer->get_user_id(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| add_user_to_blog($this->get_id(), $user_id, $role); | ||||||||||||||||||||||
| // only add user to blog if they are not already a member, or we are downgrading their role. | ||||||||||||||||||||||
| // Without this check the user could lose additional roles added manually or with hooks. | ||||||||||||||||||||||
| if ('administrator' !== $role || ! is_user_member_of_blog($user_id, $this->get_id())) { | ||||||||||||||||||||||
| add_user_to_blog($this->get_id(), $user_id, $role); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+1682
to
+1686
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Guard add_user_to_blog when user_id is empty. If get_user_id() returns 0/false, add_user_to_blog may misbehave. Add a truthy check. Apply this diff: - if ('administrator' !== $role || ! is_user_member_of_blog($user_id, $this->get_id())) {
- add_user_to_blog($this->get_id(), $user_id, $role);
- }
+ if ($user_id && ('administrator' !== $role || ! is_user_member_of_blog($user_id, $this->get_id()))) {
+ add_user_to_blog($this->get_id(), $user_id, $role);
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| } elseif ($this->get_type() !== Site_Type::CUSTOMER_OWNED && $original_customer_id) { | ||||||||||||||||||||||
| $user_id = wu_get_customer($original_customer_id)->get_user_id(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid role update when customer has no WP user_id.
Prevent accidental calls with a falsy user_id; minor perf win by not repeating get_user_id().
Apply this diff:
📝 Committable suggestion
🤖 Prompt for AI Agents