Better storing of credentials for hosting providers #333
Conversation
📝 WalkthroughWalkthroughIntegration subsystem refactor: new Integration and Integration_Registry abstractions, capability modules for hosting features, credential encryption via Credential_Store, many legacy Base_Host_Provider implementations removed and replaced by provider Integration classes and capability modules, admin wizard and domain manager updated to use the registry and encrypted credentials. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminWizard as Admin Wizard
participant Registry as Integration Registry
participant Integration as Provider Integration
participant CapModule as Capability Module
participant Store as Credential Store
participant Options as Network Options
Note over AdminWizard,Registry: Admin opens integration wizard
AdminWizard->>Registry: resolve integration by id
Registry-->>Integration: returns Integration instance
AdminWizard->>Integration: get_fields()
Integration->>CapModule: aggregate capability fields (if any)
CapModule-->>Integration: capability fields list
Integration-->>AdminWizard: fields with current values (calls get_credential)
AdminWizard->>Integration: save_credentials(filtered_data)
Integration->>Store: encrypt(value)
Store->>Store: AES encrypt with IV
Store-->>Options: update_network_option(encrypted_value)
Options-->>Integration: ack
Integration-->>Registry: mark setup/enabled (if applicable)
Registry-->>AdminWizard: get_next_section_link()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
inc/integrations/host-providers/class-hestia-host-provider.php (1)
269-296: Add validation for API credentials before sending the request.Currently, an empty user or missing authentication (both hash and password empty) would send a malformed request to the API. Add an early validation check to fail fast with a clear error.
Suggested change
$api_user = $this->get_credential('WU_HESTIA_API_USER'); $api_hash = $this->get_credential('WU_HESTIA_API_HASH'); $api_pass = $this->get_credential('WU_HESTIA_API_PASSWORD'); + if (empty($api_user) || (empty($api_hash) && empty($api_pass))) { + return new \WP_Error('wu_hestia_no_auth', __('Missing Hestia API credentials', 'ultimate-multisite')); + } + $body['user'] = $api_user; if (! empty($api_hash)) { $body['hash'] = $api_hash; } else { $body['password'] = $api_pass;
🤖 Fix all issues with AI agents
In `@inc/helpers/class-credential-store.php`:
- Around line 50-52: The code currently falls back to base64-only storage
silently when OpenSSL is unavailable or encryption fails (see usage of
openssl_encrypt, openssl_get_cipher_methods, self::CIPHER_METHOD and
self::ENCRYPTED_PREFIX + base64_encode); update the encrypt/decrypt logic in the
credential store (the methods wrapping openssl_encrypt/openssl_decrypt) to
explicitly handle two cases: (1) if openssl functions or the cipher are missing,
emit a warning log (use the plugin logger or error_log/WP_Error) that includes
the exact reason and avoid silently persisting sensitive data without notice,
and (2) if openssl_encrypt fails at runtime, log the error with the failure
details and either throw an exception or return a failure result instead of
returning the base64-only value; apply the same explicit logging/exception
behavior for the symmetric decryption path (lines around the other fallback at
59-61) so both encryption and decryption failures are surfaced.
- Around line 99-108: The current decrypt logic uses explode('::', $decoded, 2)
which can break if the raw IV contains the '::' bytes; update both the encrypt
and decrypt flows to avoid a delimiter and instead use a fixed-length IV: in the
encrypt path (the method that calls openssl_random_pseudo_bytes()), concatenate
the raw IV directly before the ciphertext and base64_encode that result (no
separator), and in the decrypt path (the code using openssl_decrypt,
get_encryption_key(), self::CIPHER_METHOD) base64_decode the stored string, call
openssl_cipher_iv_length(self::CIPHER_METHOD) to determine the IV length, use
substr to slice the first N bytes as $iv and the remainder as $encrypted, then
call openssl_decrypt($encrypted, self::CIPHER_METHOD, $key, 0, $iv).
In `@inc/integrations/host-providers/class-closte-host-provider.php`:
- Around line 307-314: Update the messaging around the CLOSTE_CLIENT_API_KEY
check (the get_credential('CLOSTE_CLIENT_API_KEY') block) to use neutral
language about missing configuration rather than mentioning constants: change
the wu_log_add('integration-closte', ...) message and the returned 'error'
string so they say the Closte API key or API key is not configured / not found
(e.g., "Closte API key not configured" or "API key not configured") instead of
referencing constants; keep the same control flow and keys in the returned array
and do this inside the same function/method where get_credential is called.
In `@inc/integrations/host-providers/class-enhance-host-provider.php`:
- Around line 324-338: Log messages refer to "constants" but the values are
fetched via get_credential; update the wu_log_add calls in the validation around
$api_token and $api_base_url (inside the method using get_credential) to say the
credential/credential key is missing or empty rather than referencing constants;
keep the same context and include the credential key names (e.g.,
'WU_ENHANCE_API_TOKEN' and 'WU_ENHANCE_API_URL') in the messages so searches for
get_credential(...)/wu_log_add(...) still find them.
In `@inc/integrations/host-providers/class-hestia-host-provider.php`:
- Around line 210-215: The test_connection() method calls send_hestia_request()
with $account from get_credential() but does not guard for an empty string; add
the same explicit guard used in on_add_domain/on_remove_domain: after $account =
$this->get_credential('WU_HESTIA_ACCOUNT') check if $account is empty, handle it
by logging or returning an early failure (so you don’t call send_hestia_request
with an empty account), and keep the rest of test_connection() unchanged.
In `@inc/integrations/host-providers/class-wpmudev-host-provider.php`:
- Around line 131-133: The on_add_domain method is shadowing the incoming
$site_id parameter by assigning $site_id =
$this->get_credential('WPMUDEV_HOSTING_SITE_ID');; change the credential
assignment to a new, descriptive variable name (e.g. $hosting_site_id or
$wpmudev_site_id) and update any subsequent references in on_add_domain to use
that new variable so the original WordPress $site_id parameter remains intact
and unmodified.
| if ( ! function_exists('openssl_encrypt') || ! in_array(self::CIPHER_METHOD, openssl_get_cipher_methods(), true)) { | ||
| return self::ENCRYPTED_PREFIX . base64_encode($value); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode | ||
| } |
There was a problem hiding this comment.
Silent fallback to base64-only encoding degrades security.
When OpenSSL is unavailable or encryption fails, the code silently falls back to base64 encoding without actual encryption. This could leave credentials exposed if:
- The PHP installation lacks OpenSSL
- Encryption fails for any reason
Consider either:
- Logging a warning when falling back to non-encrypted storage
- Throwing an exception to prevent insecure storage
- At minimum, documenting this behavior for administrators
🛡️ Proposed fix to add logging for fallback behavior
if ( ! function_exists('openssl_encrypt') || ! in_array(self::CIPHER_METHOD, openssl_get_cipher_methods(), true)) {
+ wu_log_add('credential-store', __('OpenSSL not available - credentials stored with obfuscation only, not encryption.', 'ultimate-multisite'), \Psr\Log\LogLevel::WARNING);
return self::ENCRYPTED_PREFIX . base64_encode($value); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
}And for encryption failure:
if (false === $encrypted) {
+ wu_log_add('credential-store', __('Encryption failed - credentials stored with obfuscation only, not encryption.', 'ultimate-multisite'), \Psr\Log\LogLevel::WARNING);
return self::ENCRYPTED_PREFIX . base64_encode($value); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode
}Also applies to: 59-61
🤖 Prompt for AI Agents
In `@inc/helpers/class-credential-store.php` around lines 50 - 52, The code
currently falls back to base64-only storage silently when OpenSSL is unavailable
or encryption fails (see usage of openssl_encrypt, openssl_get_cipher_methods,
self::CIPHER_METHOD and self::ENCRYPTED_PREFIX + base64_encode); update the
encrypt/decrypt logic in the credential store (the methods wrapping
openssl_encrypt/openssl_decrypt) to explicitly handle two cases: (1) if openssl
functions or the cipher are missing, emit a warning log (use the plugin logger
or error_log/WP_Error) that includes the exact reason and avoid silently
persisting sensitive data without notice, and (2) if openssl_encrypt fails at
runtime, log the error with the failure details and either throw an exception or
return a failure result instead of returning the base64-only value; apply the
same explicit logging/exception behavior for the symmetric decryption path
(lines around the other fallback at 59-61) so both encryption and decryption
failures are surfaced.
| $parts = explode('::', $decoded, 2); | ||
|
|
||
| if (count($parts) !== 2) { | ||
| return ''; | ||
| } | ||
|
|
||
| [$iv, $encrypted] = $parts; | ||
|
|
||
| $key = self::get_encryption_key(); | ||
| $decrypted = openssl_decrypt($encrypted, self::CIPHER_METHOD, $key, 0, $iv); |
There was a problem hiding this comment.
IV may contain the '::' separator, causing decryption failures.
The IV is raw binary data from openssl_random_pseudo_bytes(). While rare, the IV could theoretically contain the byte sequence :: (0x3a3a), which would cause explode('::', $decoded, 2) to split at the wrong position, corrupting both the IV and ciphertext.
Consider using a fixed-length split based on the known IV length instead of a delimiter.
🔧 Proposed fix using fixed-length IV extraction
- $parts = explode('::', $decoded, 2);
-
- if (count($parts) !== 2) {
- return '';
- }
-
- [$iv, $encrypted] = $parts;
+ $iv_length = openssl_cipher_iv_length(self::CIPHER_METHOD);
+ if (strlen($decoded) < $iv_length + 2) { // +2 for '::'
+ return '';
+ }
+
+ $iv = substr($decoded, 0, $iv_length);
+ $separator = substr($decoded, $iv_length, 2);
+ if ($separator !== '::') {
+ return '';
+ }
+ $encrypted = substr($decoded, $iv_length + 2);Note: This requires also updating the encrypt() method to not use '::' as a separator, or to place the IV at a fixed position. A cleaner approach would be to simply concatenate IV and ciphertext without a separator:
- return self::ENCRYPTED_PREFIX . base64_encode($iv . '::' . $encrypted);
+ return self::ENCRYPTED_PREFIX . base64_encode($iv . $encrypted);And in decrypt:
+ $iv_length = openssl_cipher_iv_length(self::CIPHER_METHOD);
+ $iv = substr($decoded, 0, $iv_length);
+ $encrypted = substr($decoded, $iv_length);🤖 Prompt for AI Agents
In `@inc/helpers/class-credential-store.php` around lines 99 - 108, The current
decrypt logic uses explode('::', $decoded, 2) which can break if the raw IV
contains the '::' bytes; update both the encrypt and decrypt flows to avoid a
delimiter and instead use a fixed-length IV: in the encrypt path (the method
that calls openssl_random_pseudo_bytes()), concatenate the raw IV directly
before the ciphertext and base64_encode that result (no separator), and in the
decrypt path (the code using openssl_decrypt, get_encryption_key(),
self::CIPHER_METHOD) base64_decode the stored string, call
openssl_cipher_iv_length(self::CIPHER_METHOD) to determine the IV length, use
substr to slice the first N bytes as $iv and the remainder as $encrypted, then
call openssl_decrypt($encrypted, self::CIPHER_METHOD, $key, 0, $iv).
| $api_key = $this->get_credential('CLOSTE_CLIENT_API_KEY'); | ||
|
|
||
| if (empty(CLOSTE_CLIENT_API_KEY)) { | ||
| wu_log_add('integration-closte', 'CLOSTE_CLIENT_API_KEY is empty'); | ||
| if (empty($api_key)) { | ||
| wu_log_add('integration-closte', 'CLOSTE_CLIENT_API_KEY constant not defined or empty'); | ||
| return [ | ||
| 'success' => false, | ||
| 'error' => 'Closte API Key is empty.', | ||
| 'error' => 'Closte API Key not found.', | ||
| ]; |
There was a problem hiding this comment.
Update the log/error text to reflect credential storage, not constants.
The message now mentions constants, but credentials may come from the store. A neutral “API key not configured” wording will be clearer.
🔧 Suggested wording update
- if (empty($api_key)) {
- wu_log_add('integration-closte', 'CLOSTE_CLIENT_API_KEY constant not defined or empty');
+ if (empty($api_key)) {
+ wu_log_add('integration-closte', 'Closte API key not configured.');
return [
'success' => false,
- 'error' => 'Closte API Key not found.',
+ 'error' => 'Closte API key not configured.',
];
}📝 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.
| $api_key = $this->get_credential('CLOSTE_CLIENT_API_KEY'); | |
| if (empty(CLOSTE_CLIENT_API_KEY)) { | |
| wu_log_add('integration-closte', 'CLOSTE_CLIENT_API_KEY is empty'); | |
| if (empty($api_key)) { | |
| wu_log_add('integration-closte', 'CLOSTE_CLIENT_API_KEY constant not defined or empty'); | |
| return [ | |
| 'success' => false, | |
| 'error' => 'Closte API Key is empty.', | |
| 'error' => 'Closte API Key not found.', | |
| ]; | |
| $api_key = $this->get_credential('CLOSTE_CLIENT_API_KEY'); | |
| if (empty($api_key)) { | |
| wu_log_add('integration-closte', 'Closte API key not configured.'); | |
| return [ | |
| 'success' => false, | |
| 'error' => 'Closte API key not configured.', | |
| ]; | |
| } |
🤖 Prompt for AI Agents
In `@inc/integrations/host-providers/class-closte-host-provider.php` around lines
307 - 314, Update the messaging around the CLOSTE_CLIENT_API_KEY check (the
get_credential('CLOSTE_CLIENT_API_KEY') block) to use neutral language about
missing configuration rather than mentioning constants: change the
wu_log_add('integration-closte', ...) message and the returned 'error' string so
they say the Closte API key or API key is not configured / not found (e.g.,
"Closte API key not configured" or "API key not configured") instead of
referencing constants; keep the same control flow and keys in the returned array
and do this inside the same function/method where get_credential is called.
| $api_token = $this->get_credential('WU_ENHANCE_API_TOKEN'); | ||
|
|
||
| if (empty($api_token)) { | ||
| wu_log_add('integration-enhance', 'WU_ENHANCE_API_TOKEN constant not defined or empty'); | ||
| return [ | ||
| 'success' => false, | ||
| 'error' => 'Enhance API Token not found.', | ||
| ]; | ||
| } | ||
|
|
||
| if (defined('WU_ENHANCE_API_URL') === false || empty(WU_ENHANCE_API_URL)) { | ||
| $api_base_url = $this->get_credential('WU_ENHANCE_API_URL'); | ||
|
|
||
| if (empty($api_base_url)) { | ||
| wu_log_add('integration-enhance', 'WU_ENHANCE_API_URL constant not defined or empty'); | ||
| return [ |
There was a problem hiding this comment.
Update log text to reflect credential storage, not constants.
The current messages are misleading now that values come from the credential store.
✏️ Suggested change
- wu_log_add('integration-enhance', 'WU_ENHANCE_API_TOKEN constant not defined or empty');
+ wu_log_add('integration-enhance', 'Enhance API token not configured');
...
- wu_log_add('integration-enhance', 'WU_ENHANCE_API_URL constant not defined or empty');
+ wu_log_add('integration-enhance', 'Enhance API URL not configured');🤖 Prompt for AI Agents
In `@inc/integrations/host-providers/class-enhance-host-provider.php` around lines
324 - 338, Log messages refer to "constants" but the values are fetched via
get_credential; update the wu_log_add calls in the validation around $api_token
and $api_base_url (inside the method using get_credential) to say the
credential/credential key is missing or empty rather than referencing constants;
keep the same context and include the credential key names (e.g.,
'WU_ENHANCE_API_TOKEN' and 'WU_ENHANCE_API_URL') in the messages so searches for
get_credential(...)/wu_log_add(...) still find them.
| public function test_connection(): void { | ||
|
|
||
| $account = defined('WU_HESTIA_ACCOUNT') ? WU_HESTIA_ACCOUNT : ''; | ||
| $account = $this->get_credential('WU_HESTIA_ACCOUNT'); | ||
|
|
||
| $response = $this->send_hestia_request('v-list-web-domains', [$account, 'json']); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and get its content around the specified lines
cat -n inc/integrations/host-providers/class-hestia-host-provider.php | sed -n '200,230p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 1160
🏁 Script executed:
# Search for get_credential method definition in the file
rg -n "get_credential" inc/integrations/host-providers/class-hestia-host-provider.php -B 2 -A 5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2061
🏁 Script executed:
# Find the class definition and parent class
rg -n "class.*Hestia.*{" inc/integrations/host-providers/class-hestia-host-provider.php -A 3Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 176
🏁 Script executed:
# Search for class definition with simpler pattern
head -50 inc/integrations/host-providers/class-hestia-host-provider.php | grep -n "class\|extends"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 139
🏁 Script executed:
# Find the Base_Host_Provider class file
find . -name "*base*host*provider*" -o -name "*host*provider*base*" | head -20Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 143
🏁 Script executed:
# Search for get_credential method in the base class
rg -n "get_credential" inc/integrations/host-providers/class-base-host-provider.php -B 2 -A 10Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 1132
Add an explicit guard for missing account before calling the API.
The get_credential() method returns an empty string if the credential is not found. Other methods in this class (on_add_domain, on_remove_domain) guard against empty credentials before using them; test_connection() should follow the same pattern. Calling Hestia with an empty account yields a less clear error than failing fast here.
🛡️ Suggested change
public function test_connection(): void {
$account = $this->get_credential('WU_HESTIA_ACCOUNT');
+ if (empty($account)) {
+ wp_send_json_error(new \WP_Error('wu_hestia_no_account', __('Missing WU_HESTIA_ACCOUNT', 'ultimate-multisite')));
+ return;
+ }
$response = $this->send_hestia_request('v-list-web-domains', [$account, 'json']);🤖 Prompt for AI Agents
In `@inc/integrations/host-providers/class-hestia-host-provider.php` around lines
210 - 215, The test_connection() method calls send_hestia_request() with
$account from get_credential() but does not guard for an empty string; add the
same explicit guard used in on_add_domain/on_remove_domain: after $account =
$this->get_credential('WU_HESTIA_ACCOUNT') check if $account is empty, handle it
by logging or returning an early failure (so you don’t call send_hestia_request
with an empty account), and keep the rest of test_connection() unchanged.
| public function on_add_domain($domain, $site_id): void { | ||
|
|
||
| $site_id = WPMUDEV_HOSTING_SITE_ID; | ||
| $site_id = $this->get_credential('WPMUDEV_HOSTING_SITE_ID'); |
There was a problem hiding this comment.
Variable shadowing: $site_id parameter overwritten by credential.
The function parameter $site_id (WordPress site ID) is being overwritten with the WPMUDEV hosting site ID from credentials. This creates confusion between two different concepts and could cause bugs if the WordPress site ID is needed later in the method.
🔧 Proposed fix to rename the credential variable
public function on_add_domain($domain, $site_id): void {
- $site_id = $this->get_credential('WPMUDEV_HOSTING_SITE_ID');
+ $hosting_site_id = $this->get_credential('WPMUDEV_HOSTING_SITE_ID');
$api_key = get_site_option('wpmudev_apikey');
@@ -143,7 +143,7 @@ class WPMUDEV_Host_Provider extends Base_Host_Provider {
foreach ($domains as $_domain) {
$response = wp_remote_post(
- "https://premium.wpmudev.org/api/hosting/v1/$site_id/domains",
+ "https://premium.wpmudev.org/api/hosting/v1/$hosting_site_id/domains",
[
'timeout' => 50,
'body' => [
'domain' => $_domain,
- 'site_id' => $site_id,
+ 'site_id' => $hosting_site_id,
],🤖 Prompt for AI Agents
In `@inc/integrations/host-providers/class-wpmudev-host-provider.php` around lines
131 - 133, The on_add_domain method is shadowing the incoming $site_id parameter
by assigning $site_id = $this->get_credential('WPMUDEV_HOSTING_SITE_ID');;
change the credential assignment to a new, descriptive variable name (e.g.
$hosting_site_id or $wpmudev_site_id) and update any subsequent references in
on_add_domain to use that new variable so the original WordPress $site_id
parameter remains intact and unmodified.
Replace monolithic host provider classes with a modular integration system using capability interfaces (domain mapping, domain selling, email selling, multi-tenancy). Add DNS diagnostics to domain manager, MCP adapter updates, and comprehensive test coverage for the new provider architecture. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🔨 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: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
inc/checkout/class-cart.php (1)
1624-1671:⚠️ Potential issue | 🟠 MajorIndependent items can still seed cart duration and break later variation lookups.
When an independent-cycle product is added before the plan,$this->durationis set from that product, so the later plan add may try to resolve variations against the independent duration (see the variation lookup earlier in this method). This can yield wrong pricing or missing-variation errors depending on product order.Consider preventing independent items from setting the cart’s duration (or tracking the source and only using plan duration for plan variation lookup).
Possible fix
- if (empty($this->duration) || $product->is_recurring() === false) { + if ((empty($this->duration) || $product->is_recurring() === false) && ! wu_has_independent_billing_cycle($product->get_type())) { $this->duration = $product->get_duration(); $this->duration_unit = $product->get_duration_unit(); }
🤖 Fix all issues with AI agents
In `@inc/integrations/class-integration.php`:
- Around line 566-580: The get_constants_string method is vulnerable to PHP
injection because it directly interpolates unescaped $value into a define() via
sprintf; replace the unsafe defaults and escaping: use
shortcode_atts(array_fill_keys($this->get_all_constants(), ''),
$constant_values) instead of array_flip(...) to create proper string defaults,
and when building each define line in the foreach($constant_values as $constant
=> $value) use var_export($value, true) to produce a safely-escaped PHP literal
(e.g. "define( 'CONSTANT', " . var_export($value, true) . " );") so credential
values cannot break out of the single-quoted string.
- Around line 334-345: The save_credentials method currently builds defaults
with array_flip($this->get_all_constants()) and shortcode_atts, which yields
numeric defaults that are considered non-empty and get encrypted/stored; change
this to build an associative defaults array using
array_fill_keys($this->get_all_constants(), '') (or equivalent) so
shortcode_atts (or direct merging) yields empty-string defaults for missing
keys, then continue to call Credential_Store::encrypt() and
update_network_option(...) only for non-empty values and call
delete_network_option(...) for empty values to avoid corrupting stored
credentials.
In `@inc/integrations/providers/gridpane/class-gridpane-integration.php`:
- Around line 71-79: The test_connection method currently calls the destructive
'application/delete-domain' endpoint; change it to call a read-only GridPane
endpoint (for example 'application/get' or 'account/info') using the same
send_gridpane_api_request call so credentials are validated without side
effects. Update the payload keys (still using
$this->get_credential('WU_GRIDPANE_SERVER_ID') and
$this->get_credential('WU_GRIDPANE_APP_ID')) to match the chosen read-only
endpoint parameters, and keep the call inside test_connection and error handling
unchanged so only the endpoint string and any required non-destructive params
are replaced.
In `@inc/integrations/providers/hestia/class-hestia-integration.php`:
- Around line 189-198: The response-parsing code currently treats any numeric
return code as success; change the logic that examines $raw (the trimmed $trim
value) so that if returncode=yes and $trim is a numeric string not equal to '0'
it is treated as an error: create and return a WP_Error (or otherwise propagate
an error) containing the numeric code and a short message instead of returning
the raw value. Locate the response handling around the $raw/$trim variables
(used by test_connection()) and add a guard before the JSON decode that checks
is_numeric($trim) && $trim !== '0' and returns the error object so
test_connection() will fail for non-zero Hestia return codes.
In `@inc/integrations/providers/serverpilot/class-serverpilot-domain-mapping.php`:
- Around line 175-184: The on_remove_subdomain method is currently empty causing
ServerPilot to retain removed subdomains; implement it the same way
on_remove_domain does: use the Site ID to fetch the current domain list (as
on_remove_domain does), remove the matching $subdomain from that list, and call
the ServerPilot update API to save the filtered domains; ensure you reference
and reuse the same helpers/code paths used by on_remove_domain (e.g., the
domain-fetching logic and the API update call) and handle the case where the
resulting list is empty or unchanged and log errors similarly to
on_remove_domain.
In `@inc/integrations/providers/wpmudev/class-wpmudev-domain-mapping.php`:
- Around line 113-148: The loop handling wp_remote_post responses currently
treats any non-wp_error response as success; update the foreach ($domains as
$_domain) handling to validate HTTP status and JSON before logging success:
after wp_remote_post and is_wp_error checks, get the HTTP status with
wp_remote_retrieve_response_code($response) and if it is not a 2xx/status (e.g.,
200/201) call wu_log_add (using the same 'integration-wpmudev' tag and
LogLevel::ERROR) including the HTTP status and response body, then continue;
next, decode the body with json_decode and verify json_last_error() ===
JSON_ERROR_NONE (and that $body is an object/array) before using $body->message
— if JSON is invalid, log an error with the raw body and continue; only reach
the existing success path when status is 2xx and JSON is valid.
🟡 Minor comments (18)
inc/integrations/capabilities/interface-domain-selling-capability.php-120-128 (1)
120-128:⚠️ Potential issue | 🟡 MinorAdd PHPDoc documenting the expected return array structure.
The
get_dns_recordsmethod returns a genericarraywith no documentation of the expected structure. Document the array keys and their types in the PHPDoc to guide future implementations. Consider referencing the structure used inDomain_Manager(e.g.,entries,www_entries,network_ip,warnings) if that becomes the standard for implementations of this interface.inc/apis/trait-mcp-abilities.php-570-570 (1)
570-570:⚠️ Potential issue | 🟡 MinorUse
strpos()instead ofstr_contains()for broader compatibility.The plugin requires PHP 7.4.30, but
str_contains()was introduced in PHP 8.0. While the plugin includessymfony/polyfill-php80which provides a polyfill, usingstrpos($type, '|') !== falsewould be more robust and eliminate reliance on the polyfill.inc/functions/product.php-213-229 (1)
213-229:⚠️ Potential issue | 🟡 MinorDefensively cast filter output before
in_array.
If a third‑party filter returns a non‑array,in_array()will throw aTypeErrorin PHP 8+. A lightweight cast prevents fatals while keeping strict checks.Suggested fix
- $types = apply_filters('wu_independent_billing_cycle_product_types', []); + $types = (array) apply_filters('wu_independent_billing_cycle_product_types', []);readme.txt-152-154 (1)
152-154:⚠️ Potential issue | 🟡 MinorTighten grammar and capitalization in the hosting note.
“webserver / setup / wordpress” reads rough and should follow standard usage.✏️ Suggested edit
-For a private server with a dedicated IP, the webserver can be setup to serve the wordpress install by default and no extra steps are required for domain mapping. +For a private server with a dedicated IP, the web server can be set up to serve the WordPress install by default, and no extra steps are required for domain mapping.readme.txt-106-112 (1)
106-112:⚠️ Potential issue | 🟡 MinorFix duplicated “subdirectory” wording in the FAQ.
The current answers repeat “subdirectory installs,” which makes the subdomain question confusing.✏️ Suggested edit
-Yes. Ultimate Multisite supports both subdirectory installs and subdirectory installs. +Yes. Ultimate Multisite supports subdirectory installs. @@ -Yes. Ultimate Multisite supports both subdirectory installs and subdomain installs. +Yes. Ultimate Multisite supports subdomain installs.inc/integrations/providers/cpanel/class-cpanel-domain-mapping.php-238-238 (1)
238-238:⚠️ Potential issue | 🟡 MinorTypo: "ocurred" should be "occurred".
📝 Proposed fix
- wu_log_add('integration-cpanel', __('Unexpected error ocurred trying to sync domains with CPanel', 'ultimate-multisite'), LogLevel::ERROR); + wu_log_add('integration-cpanel', __('Unexpected error occurred trying to sync domains with CPanel', 'ultimate-multisite'), LogLevel::ERROR);inc/integrations/providers/cpanel/class-cpanel-domain-mapping.php-231-244 (1)
231-244:⚠️ Potential issue | 🟡 MinorAdd null checks before accessing nested properties in
log_calls.The method accesses
$results->cpanelresult->dataand$results->cpanelresult->data[0]without verifying these properties exist. If the cPanel API returns an unexpected response structure, this could cause fatal errors.🛡️ Proposed fix to add defensive checks
public function log_calls($results): void { + if (!isset($results->cpanelresult) || !isset($results->cpanelresult->data)) { + wu_log_add('integration-cpanel', __('Unexpected response structure from CPanel API', 'ultimate-multisite'), LogLevel::ERROR); + + return; + } + if (is_object($results->cpanelresult->data)) { wu_log_add('integration-cpanel', $results->cpanelresult->data->reason); return; } elseif ( ! isset($results->cpanelresult->data[0])) { wu_log_add('integration-cpanel', __('Unexpected error ocurred trying to sync domains with CPanel', 'ultimate-multisite'), LogLevel::ERROR); return; } wu_log_add('integration-cpanel', $results->cpanelresult->data[0]->reason); }tests/WP_Ultimo/Integrations/Integration_Test.php-138-141 (1)
138-141:⚠️ Potential issue | 🟡 MinorFix lint warning for array arrow spacing.
Static analysis flagged the extra spacing before
=>here.🧹 Proposed fix
- 'CONST_A' => 'value_a', - 'UNKNOWN' => 'should_be_ignored', + 'CONST_A' => 'value_a', + 'UNKNOWN' => 'should_be_ignored',tests/WP_Ultimo/Integrations/Integration_Test.php-175-185 (1)
175-185:⚠️ Potential issue | 🟡 MinorClean up credentials saved in this test to avoid cross-test leakage.
This test saves credentials on a new Integration instance, but only the main
$this->integrationis cleared intearDown().🧼 Proposed fix
$integration->save_credentials(['ALT_B' => 'val']); $this->assertTrue($integration->is_setup()); + + $integration->delete_credentials();inc/integrations/providers/wpmudev/class-wpmudev-integration.php-55-67 (1)
55-67:⚠️ Potential issue | 🟡 MinorAdd credential validation before making the API request.
Without checking if
$site_idor$api_keyare empty, the request fires with a malformed URL or missing authorization, resulting in a cryptic API error instead of a clear validation message.🛡️ Proposed fix
$site_id = $this->get_credential('WPMUDEV_HOSTING_SITE_ID'); $api_key = get_site_option('wpmudev_apikey'); + + if (empty($site_id) || empty($api_key)) { + return new \WP_Error( + 'wpmudev-missing-credentials', + __('WPMU DEV credentials are missing.', 'ultimate-multisite') + ); + }inc/integrations/providers/gridpane/class-gridpane-domain-mapping.php-118-128 (1)
118-128:⚠️ Potential issue | 🟡 MinorSame issue: missing error handling for domain removal.
For consistency with other provider implementations, consider adding logging for success/failure.
inc/integrations/providers/enhance/class-enhance-domain-mapping.php-190-196 (1)
190-196:⚠️ Potential issue | 🟡 MinorMissing response handling for DELETE request.
The
on_remove_domainmethod logs "removal request sent" but doesn't verify if the DELETE request succeeded. The add operation (lines 130-136) checks the response - consider applying the same pattern here.🛡️ Proposed fix to add response handling
- $this->get_enhance()->send_enhance_api_request( + $delete_response = $this->get_enhance()->send_enhance_api_request( '/orgs/' . $org_id . '/websites/' . $website_id . '/domains/' . $domain_id, 'DELETE' ); - wu_log_add('integration-enhance', sprintf('Domain %s removal request sent', $domain)); + if (isset($delete_response['error'])) { + wu_log_add('integration-enhance', sprintf('Failed to remove domain %s: %s', $domain, $delete_response['error'])); + } else { + wu_log_add('integration-enhance', sprintf('Domain %s removed successfully', $domain)); + } }inc/integrations/class-base-capability-module.php-69-72 (1)
69-72:⚠️ Potential issue | 🟡 MinorPotential error if
get_integration()called beforeset_integration().The
$integrationproperty is typed but not initialized. Callingget_integration()beforeset_integration()will throw aTypeError. Consider initializing with a nullable type or adding a guard.🛡️ Proposed fix to add defensive check
public function get_integration(): Integration { + if (! isset($this->integration)) { + throw new \LogicException('Integration not set. Call set_integration() first.'); + } + return $this->integration; }inc/integrations/providers/gridpane/class-gridpane-domain-mapping.php-97-107 (1)
97-107:⚠️ Potential issue | 🟡 MinorMissing error handling for API response.
The
on_add_domainmethod calls the GridPane API but doesn't check the response or log failures. Other providers (like Rocket and Enhance) include logging for success/failure states.🛡️ Proposed fix to add error handling
public function on_add_domain(string $domain, int $site_id): void { - $this->get_gridpane()->send_gridpane_api_request( + $response = $this->get_gridpane()->send_gridpane_api_request( 'application/add-domain', [ 'server_ip' => $this->get_gridpane()->get_credential('WU_GRIDPANE_SERVER_ID'), 'site_url' => $this->get_gridpane()->get_credential('WU_GRIDPANE_APP_ID'), 'domain_url' => $domain, ] ); + + if (is_wp_error($response) || (isset($response['success']) && ! $response['success'])) { + wu_log_add('integration-gridpane', sprintf('[Add Domain] %s: Failed', $domain)); + } }inc/integrations/providers/cloudflare/class-cloudflare-domain-mapping.php-236-261 (1)
236-261:⚠️ Potential issue | 🟡 MinorEarly return in removal loop prevents removing www variant.
Similar to
on_add_subdomain, if the first domain removal fails or has no DNS entries, the method returns early (lines 247-248, 257-258), preventing the www variant from being processed. This could leave orphaned DNS records.Proposed fix
foreach ($domains_to_remove as $original_subdomain) { $dns_entries = $this->get_cloudflare()->cloudflare_api_call( "client/v4/zones/$zone_id/dns_records/", 'GET', [ 'name' => $original_subdomain, 'type' => 'CNAME', ] ); if (is_wp_error($dns_entries) || ! $dns_entries->result) { - return; + continue; } $dns_entry_to_remove = $dns_entries->result[0]; $results = $this->get_cloudflare()->cloudflare_api_call("client/v4/zones/$zone_id/dns_records/$dns_entry_to_remove->id", 'DELETE'); if (is_wp_error($results)) { wu_log_add('integration-cloudflare', sprintf('Failed to remove subdomain "%s" from Cloudflare. Reason: %s', $subdomain, $results->get_error_message()), LogLevel::ERROR); - return; + continue; }inc/integrations/providers/cloudflare/class-cloudflare-domain-mapping.php-186-195 (1)
186-195:⚠️ Potential issue | 🟡 MinorEarly return in loop prevents adding www variant on failure.
If adding the first subdomain fails, the
returnstatement on line 191 exits the entire method, skipping the www variant. Consider usingcontinueinstead to attempt all domains, or handle partial failures differently.Proposed fix
if (is_wp_error($results)) { wu_log_add('integration-cloudflare', sprintf('Failed to add subdomain "%s" to Cloudflare. Reason: %s', $subdomain, $results->get_error_message()), LogLevel::ERROR); - return; + continue; }inc/integrations/providers/cloudways/class-cloudways-domain-mapping.php-251-277 (1)
251-277:⚠️ Potential issue | 🟡 MinorEncode domain names in DNS lookup requests.
Concatenating raw domain strings into a query URL can break lookups if the domain contains special characters. URL-encode the domain usingrawurlencode(). For international domain names (IDNs), consider converting to ASCII form first usingidn_to_ascii()to ensure proper Punycode representation before encoding.🔧 Safer DNS request
- $response = wp_remote_get('https://dns.google/resolve?name=' . $domain_name); + $response = wp_remote_get('https://dns.google/resolve?name=' . rawurlencode($domain_name));inc/integrations/class-integration-registry.php-338-372 (1)
338-372:⚠️ Potential issue | 🟡 MinorEscape dynamic values before rendering in settings HTML.
$url(line 347-352) and$integration->get_title()(line 357) are interpolated into HTML without proper escaping. The$urlshould be wrapped withesc_url()before output in the href attribute, andget_title()should be escaped withesc_html()in the sprintf. Whilewp_kses()in the field renderer allows<a>tags, it does not validate href attribute values.🛡️ Suggested hardening
- $html = $integration->is_enabled() - ? sprintf('<div class="wu-self-center wu-text-green-800 wu-mr-4"><span class="dashicons-wu-check"></span> %s</div>', __('Activated', 'ultimate-multisite')) - : ''; + $html = $integration->is_enabled() + ? sprintf('<div class="wu-self-center wu-text-green-800 wu-mr-4"><span class="dashicons-wu-check"></span> %s</div>', esc_html__('Activated', 'ultimate-multisite')) + : ''; - $url = wu_network_admin_url( - 'wp-ultimo-hosting-integration-wizard', - [ - 'integration' => $slug, - ] - ); + $url = esc_url( + wu_network_admin_url( + 'wp-ultimo-hosting-integration-wizard', + [ + 'integration' => $slug, + ] + ) + ); - $html .= sprintf('<a href="%s" class="button-primary">%s</a>', $url, __('Configuration', 'ultimate-multisite')); + $html .= sprintf('<a href="%s" class="button-primary">%s</a>', $url, esc_html__('Configuration', 'ultimate-multisite')); - $title = sprintf(__('%s Integration', 'ultimate-multisite'), $integration->get_title()); + $title = sprintf(esc_html__('%s Integration', 'ultimate-multisite'), esc_html($integration->get_title())); - __('Go to the setup wizard to setup this integration.', 'ultimate-multisite') + esc_html__('Go to the setup wizard to setup this integration.', 'ultimate-multisite')
🧹 Nitpick comments (20)
inc/integrations/capabilities/interface-domain-selling-capability.php (1)
130-149: Consider adding complementary methods for completeness.The interface defines
add_dns_recordbut lacksupdate_dns_recordanddelete_dns_recordmethods. Similarly,enable_whois_privacyexists without a correspondingdisable_whois_privacymethod.For a complete CRUD-style API surface and to match the pattern used with
enable_domain_lock/disable_domain_lock, consider adding:public function update_dns_record(string $domain_name, string $record_id, array $record_data): array; public function delete_dns_record(string $domain_name, string $record_id): array; public function disable_whois_privacy(string $domain_name): array;inc/apis/trait-mcp-abilities.php (1)
856-880: Consider whether unknown fields should error or warn.Returning an error for unknown fields provides good feedback for LLMs but may hurt forward compatibility. If an MCP client sends a newer field that the server doesn't recognize yet, the entire update fails rather than applying known fields.
An alternative approach: log/note unknown fields in the response but still apply valid changes. This maintains the "partial update" pattern common in REST APIs.
If the strict behavior is intentional for MCP tooling, this is fine as-is.
inc/integrations/providers/cpanel/class-cpanel-domain-mapping.php (1)
188-188: Emptyon_remove_subdomainmethod could benefit from a brief comment.While the empty implementation is consistent with other providers (GridPane, Closte), adding a brief comment explaining why it's a no-op would improve maintainability.
📝 Suggested improvement
-public function on_remove_subdomain(string $subdomain, int $site_id): void {} +public function on_remove_subdomain(string $subdomain, int $site_id): void { + // CPanel handles subdomain removal automatically. +}inc/integrations/providers/serverpilot/class-serverpilot-integration.php (2)
118-127: Inconsistent body encoding when$datais empty.When
$datais empty,bodyis set to an empty array[]whileContent-Typeisapplication/json. This inconsistency may cause issues with some API servers that expect either no body or an empty JSON object for requests with a JSON content type.♻️ Proposed fix
$post_fields = [ 'timeout' => 45, 'blocking' => true, 'method' => $method, - 'body' => $data ? wp_json_encode($data) : [], + 'body' => $data ? wp_json_encode($data) : '', 'headers' => [ 'Authorization' => 'Basic ' . base64_encode($this->get_credential('WU_SERVER_PILOT_CLIENT_ID') . ':' . $this->get_credential('WU_SERVER_PILOT_API_KEY')), // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode 'Content-Type' => 'application/json', ], ];
57-66:test_connectioncould provide more actionable error details.The method returns a generic error message when
wu_get_isset($response, 'error')is truthy, discarding any details from the API response. Consider including the actual error from the response for debugging.♻️ Suggested improvement
public function test_connection() { $response = $this->send_server_pilot_api_request('', [], 'GET'); - if (is_wp_error($response) || wu_get_isset($response, 'error')) { - return new \WP_Error('serverpilot-error', __('Could not connect to ServerPilot.', 'ultimate-multisite')); + if (is_wp_error($response)) { + return $response; + } + + $error_message = wu_get_isset($response, 'error'); + if ($error_message) { + return new \WP_Error('serverpilot-error', is_array($error_message) ? wp_json_encode($error_message) : $error_message); } return true; }inc/integrations/providers/hestia/class-hestia-domain-mapping.php (2)
172-183: Handle potentialwp_json_encodefailure.If
$resultis a resource or contains circular references,wp_json_encodecan returnfalse, which would log "false" instead of something meaningful.♻️ Suggested improvement
- wu_log_add('integration-hestia', sprintf('[%s] %s', $action_label, is_scalar($result) ? (string) $result : wp_json_encode($result))); + $log_result = is_scalar($result) ? (string) $result : wp_json_encode($result); + wu_log_add('integration-hestia', sprintf('[%s] %s', $action_label, $log_result ?: __('Unable to encode response', 'ultimate-multisite')));
149-149: Empty subdomain handlers could use explanatory comments.While the empty implementations are valid (Hestia may handle subdomains differently), brief comments would clarify this is intentional behavior.
📝 Suggested improvement
-public function on_add_subdomain(string $subdomain, int $site_id): void {} +public function on_add_subdomain(string $subdomain, int $site_id): void { + // Hestia does not require explicit subdomain registration. +}-public function on_remove_subdomain(string $subdomain, int $site_id): void {} +public function on_remove_subdomain(string $subdomain, int $site_id): void { + // Hestia does not require explicit subdomain removal. +}Also applies to: 160-160
tests/WP_Ultimo/Integrations/Providers/Hestia_Domain_Mapping_Test.php (3)
64-77: Test doesn't verify the actual domain being added.The test verifies the command name and credentials but doesn't assert that the domain
'example.com'is correctly passed to the API call (should be in$args[2]).💚 Proposed improvement
public function test_on_add_domain_calls_add_alias_command(): void { $this->integration->expects($this->atLeast(1)) ->method('send_hestia_request') ->willReturnCallback(function (string $cmd, array $args) { $this->assertSame('v-add-web-domain-alias', $cmd); $this->assertSame('admin', $args[0]); $this->assertSame('mysite.com', $args[1]); + $this->assertSame('example.com', $args[2]); return 0; }); $this->module->on_add_domain('example.com', 1); }
126-135: Test may not correctly verifytest_connectiondelegation.The test expects
send_hestia_requestto be called, butHestia_Domain_Mapping::test_connection()delegates to$this->get_hestia()->test_connection(). Sincetest_connectionis not in theonlyMethodslist (line 17), the realHestia_Integration::test_connection()method will be called, which in turn callssend_hestia_request. This works but could be made more explicit by either mockingtest_connectiondirectly or adding it toonlyMethods.
1-6: Consider adding file and class docblocks for consistency.While test files often omit docblocks, adding them would maintain consistency with the production code style used throughout this codebase.
inc/integrations/providers/enhance/class-enhance-integration.php (1)
180-184: Consider redacting sensitive data from logs.Request and response bodies are logged in full, which could expose API tokens or other sensitive information in log files. Consider redacting or limiting what's logged.
🔒 Proposed fix to redact sensitive data
- wu_log_add('integration-enhance', sprintf('Making %s request to: %s', $method, $api_url)); - - if (! empty($data)) { - wu_log_add('integration-enhance', sprintf('Request data: %s', wp_json_encode($data))); - } + wu_log_add('integration-enhance', sprintf('Making %s request to: %s', $method, $api_url)); + + if (! empty($data) && defined('WP_DEBUG') && WP_DEBUG) { + wu_log_add('integration-enhance', sprintf('Request data: %s', wp_json_encode($data))); + }inc/integrations/providers/cpanel/class-cpanel-integration.php (1)
67-76: Consider providing more specific error details.When the connection test fails, the error message is generic. Including the actual API error (if available) would help administrators troubleshoot.
💡 Proposed enhancement for better error reporting
public function test_connection() { $results = $this->load_api()->api2('Cron', 'fetchcron', []); if (isset($results->cpanelresult->data) && ! isset($results->cpanelresult->error)) { return true; } - return new \WP_Error('cpanel-error', __('Could not connect to cPanel.', 'ultimate-multisite')); + $error_message = isset($results->cpanelresult->error) + ? $results->cpanelresult->error + : __('Could not connect to cPanel.', 'ultimate-multisite'); + + return new \WP_Error('cpanel-error', $error_message); }inc/integrations/providers/wpengine/class-wpengine-domain-mapping.php (1)
87-102: Silent failure when WPE_API unavailable.When
WPE_APIclass doesn't exist, the method returns silently without logging. This makes debugging difficult when domain mapping fails on WP Engine environments.🔧 Proposed fix to add logging
public function on_add_domain(string $domain, int $site_id): void { $this->get_wpengine()->load_dependencies(); if (! class_exists('WPE_API')) { + wu_log_add('integration-wpengine', 'WPE_API class not available - domain mapping requires WP Engine environment'); + return; } $api = new \WPE_API(); $api->set_arg('method', 'domain'); $api->set_arg('domain', $domain); - $api->get(); + $result = $api->get(); + + wu_log_add('integration-wpengine', sprintf('[Add Domain] %s: Request sent', $domain)); }inc/integrations/providers/cloudflare/class-cloudflare-integration.php (1)
133-145: Missing JSON decode error handling and request timeout.The
json_decode()call on line 137 can returnnullif the response body is invalid JSON, but this isn't checked before returning. Additionally, consider adding atimeoutparameter towp_remote_requestfor predictable behavior.Proposed fix
$response = wp_remote_request( $endpoint_url, [ 'method' => $method, 'body' => 'GET' === $method ? $data : wp_json_encode($data), 'data_format' => 'body', + 'timeout' => 45, 'headers' => [ 'Authorization' => sprintf('Bearer %s', $this->get_credential('WU_CLOUDFLARE_API_KEY')), 'Content-Type' => 'application/json', ], ] ); if ( ! is_wp_error($response)) { $body = wp_remote_retrieve_body($response); if (wp_remote_retrieve_response_code($response) === 200) { - return json_decode($body); + $decoded = json_decode($body); + if (null === $decoded && json_last_error() !== JSON_ERROR_NONE) { + return new \WP_Error('cloudflare-json-error', 'Invalid JSON response from Cloudflare'); + } + return $decoded; } else {inc/integrations/providers/rocket/class-rocket-integration.php (1)
218-223: GET requests don't append query parameters.For GET requests, the
$dataparameter is ignored since the body is only set for POST/PUT/PATCH methods. If GET requests need query parameters, they won't be included. Currentlytest_connection()calls with empty data, so this works, but future GET calls with data would silently lose the parameters.Proposed fix
if (! empty($data) && in_array($method, ['POST', 'PUT', 'PATCH'], true)) { $args['body'] = wp_json_encode($data); + } elseif (! empty($data) && 'GET' === $method) { + $url = add_query_arg($data, $url); }inc/integrations/providers/cloudways/class-cloudways-integration.php (1)
172-216: API requests proceed with empty token if authentication fails.If
get_cloudways_access_token()returns an empty string (due to failed authentication),send_cloudways_request()will still proceed with the API call using an empty Bearer token. This results in a less descriptive error from Cloudways rather than an early failure with a clear authentication error message.Proposed fix
public function send_cloudways_request(string $endpoint, array $data = [], string $method = 'POST') { $token = $this->get_cloudways_access_token(); + if (empty($token)) { + return new \WP_Error('cloudways-auth-failed', __('Failed to authenticate with Cloudways API', 'ultimate-multisite')); + } + $endpoint = '/' . ltrim($endpoint, '/');inc/integrations/providers/runcloud/class-runcloud-integration.php (3)
48-51: Inconsistent string function usage.This file uses
strpos()for string detection while other integration files (Rocket, Cloudways) usestr_contains(). For consistency and readability, consider usingstr_contains().Proposed fix
public function detect(): bool { - return strpos(ABSPATH, 'runcloud') !== false; + return str_contains(ABSPATH, 'runcloud'); }
151-158: Logging full API responses may expose sensitive data.The log message includes the full response body, which could contain sensitive information (tokens, credentials, or user data). Consider logging only non-sensitive metadata or redacting sensitive fields.
Proposed fix
$log_message = sprintf( - "Request: %s %s\nStatus: %s\nResponse: %s", + "Request: %s %s\nStatus: %s", $method, $url, - wp_remote_retrieve_response_code($response), - wp_remote_retrieve_body($response) + wp_remote_retrieve_response_code($response) ); wu_log_add('integration-runcloud', $log_message);
170-183: Inconsistent return type inmaybe_return_runcloud_body().When the input is a
WP_Error, the method returns a string (the error message) instead of propagating theWP_Error. This makes error handling inconsistent for callers. The docblock indicates it should returnmixed|\WP_Error, but it returns a string for errors.Proposed fix
public function maybe_return_runcloud_body($response) { if (is_wp_error($response)) { - return $response->get_error_message(); + return $response; }tests/WP_Ultimo/Integrations/Providers/GridPane_Domain_Mapping_Test.php (1)
119-128: Test may not correctly verify delegation behavior.This test mocks
send_gridpane_api_requestand expects it to be called once, buttest_connection()inGridPane_Domain_Mappingdelegates to$this->get_gridpane()->test_connection(). IfGridPane_Integration::test_connection()is not mocked and it internally callssend_gridpane_api_request, this works. However, the test assertionassertNotNull($result)is weak—it doesn't verify the actual return value or that delegation occurred correctly.Consider mocking
test_connection()directly on the integration to verify proper delegation:Proposed fix
public function test_test_connection_delegates_to_integration(): void { - $this->integration->expects($this->once()) - ->method('send_gridpane_api_request') - ->willReturn(['success' => true]); + $this->integration = $this->getMockBuilder(GridPane_Integration::class) + ->onlyMethods(['send_gridpane_api_request', 'get_credential', 'test_connection']) + ->getMock(); + + $this->integration->method('test_connection') + ->willReturn(true); + + $this->module->set_integration($this->integration); $result = $this->module->test_connection(); - $this->assertNotNull($result); + $this->assertTrue($result); }
| public function save_credentials(array $values): void { | ||
|
|
||
| $allowed = array_flip($this->get_all_constants()); | ||
| $values = shortcode_atts($allowed, $values); | ||
|
|
||
| foreach ($values as $constant_name => $value) { | ||
| if ( ! empty($value)) { | ||
| update_network_option(null, 'wu_hosting_credential_' . $constant_name, Credential_Store::encrypt($value)); | ||
| } else { | ||
| delete_network_option(null, 'wu_hosting_credential_' . $constant_name); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the class-integration.php file and examine the get_all_constants method
find . -name "class-integration.php" -type fRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 121
🏁 Script executed:
# Search for the get_all_constants method definition
rg "get_all_constants" --type php -B 2 -A 5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 4010
🏁 Script executed:
# Look at the save_credentials method and surrounding context
grep -rn "save_credentials" --include="*.php" -B 2 -A 15Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 8979
🏁 Script executed:
# Check what shortcode_atts is - if it's WordPress or custom
rg "shortcode_atts" --type php -B 2 -A 2Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 4641
🏁 Script executed:
# Examine the get_all_constants method in detail
cat -n inc/integrations/class-integration.php | sed -n '320,330p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 313
🏁 Script executed:
# Look at the get_credential method to understand how values are retrieved
rg "get_credential" --type php -B 2 -A 8Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 49642
🏁 Script executed:
# Let's check the Credential_Store to understand encryption behavior
rg "class Credential_Store" --type php -B 2 -A 20Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 1317
🏁 Script executed:
# Examine how shortcode_atts behaves by checking WordPress documentation or usage patterns
# Let's see what the actual impact is by understanding the data flow
cat -n inc/integrations/class-integration.php | sed -n '300,380p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2658
🏁 Script executed:
python3 << 'EOF'
# Simulate what happens with array_flip on an indexed array
constants = ['CONST_A', 'CONST_B', 'CONST_C']
allowed = {k: i for i, k in enumerate(constants)} # simulates array_flip
print("After array_flip:")
print(allowed)
# shortcode_atts behavior: fills missing keys with values from $allowed
test_values = {'CONST_A': 'my_value'} # only one key provided
result = {**allowed, **test_values} # shortcode_atts merges with later values taking priority
print("\nAfter shortcode_atts with partial input:")
print(result)
# Now check which values would be encrypted
print("\nValues that would be encrypted (truthy/not empty):")
for key, value in result.items():
if value: # simulates ! empty($value)
print(f" {key} => {value}")
EOFRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 339
🏁 Script executed:
# Search for tests that might verify the corrupted credentials scenario
rg "save_credentials" tests/ -B 2 -A 10 | grep -E "missing|corrupt|default|partial"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 415
🏁 Script executed:
# Look at the actual test to see what values are being passed
cat -n tests/WP_Ultimo/Integrations/Integration_Test.php | sed -n '125,145p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 888
🏁 Script executed:
# Check if retrieving a numeric value encrypted would work
cat -n inc/helpers/class-credential-store.php | sed -n '1,80p'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2644
🏁 Script executed:
# Check what get_credential returns when decrypting a numeric value
rg "decrypt" inc/helpers/class-credential-store.php -B 2 -A 10Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 1530
Default handling in save_credentials corrupts missing credential values.
Using array_flip() + shortcode_atts() produces numeric defaults (0, 1, 2) for missing keys, which pass the !empty() check and get encrypted/stored as actual credential values. This corrupts the credential data. Use array_fill_keys() with empty-string defaults instead to ensure missing values are deleted, not stored.
🐛 Fix defaults to avoid corrupting stored credentials
- $allowed = array_flip($this->get_all_constants());
- $values = shortcode_atts($allowed, $values);
+ $allowed = array_fill_keys($this->get_all_constants(), '');
+ $values = shortcode_atts($allowed, $values);🤖 Prompt for AI Agents
In `@inc/integrations/class-integration.php` around lines 334 - 345, The
save_credentials method currently builds defaults with
array_flip($this->get_all_constants()) and shortcode_atts, which yields numeric
defaults that are considered non-empty and get encrypted/stored; change this to
build an associative defaults array using
array_fill_keys($this->get_all_constants(), '') (or equivalent) so
shortcode_atts (or direct merging) yields empty-string defaults for missing
keys, then continue to call Credential_Store::encrypt() and
update_network_option(...) only for non-empty values and call
delete_network_option(...) for empty values to avoid corrupting stored
credentials.
| public function get_constants_string(array $constant_values): string { | ||
|
|
||
| $content = [ | ||
| sprintf('// Ultimate Multisite - Domain Mapping - %s', $this->get_title()), | ||
| ]; | ||
|
|
||
| $constant_values = shortcode_atts(array_flip($this->get_all_constants()), $constant_values); | ||
|
|
||
| foreach ($constant_values as $constant => $value) { | ||
| $content[] = sprintf("define( '%s', '%s' );", $constant, $value); | ||
| } | ||
|
|
||
| $content[] = sprintf('// Ultimate Multisite - Domain Mapping - %s - End', $this->get_title()); | ||
|
|
||
| return implode(PHP_EOL, $content); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "class-integration.php" -path "*/inc/integrations/*" | head -5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 121
🏁 Script executed:
# Check the file size and read the relevant lines
if [ -f "inc/integrations/class-integration.php" ]; then
wc -l inc/integrations/class-integration.php
echo "---"
sed -n '560,590p' inc/integrations/class-integration.php
fiRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 788
🏁 Script executed:
# Look for the get_all_constants method to understand what it returns
rg "get_all_constants" --context 10Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 8232
🏁 Script executed:
# Let's also check how array_flip behaves with array_fill_keys in PHP
python3 << 'EOF'
# Simulate PHP array_flip behavior with missing values
constant_keys = ['CONSTANT_1', 'CONSTANT_2', 'CONSTANT_3']
# Method 1: array_flip (like in original code)
flipped = {v: k for k, v in enumerate(constant_keys)}
print("array_flip behavior:")
print(f"array_flip result: {flipped}")
# If input has missing keys:
provided_values = {'CONSTANT_1': 'value1'}
# array_flip will create numeric keys for missing values:
for key in constant_keys:
if key not in provided_values:
provided_values[key] = len([k for k in constant_keys if k not in provided_values])
# Method 2: array_fill_keys (proposed fix)
filled = {k: '' for k in constant_keys}
print("\narray_fill_keys behavior:")
print(f"array_fill_keys result: {filled}")
# Demonstrate the vulnerability with unescaped quotes
print("\n--- Vulnerability demonstration ---")
unsafe_value = "test'; define('MALICIOUS', '1'); echo '"
safe_define = f"define( 'TEST', '{unsafe_value}' );"
print(f"Unsafe (sprintf): {safe_define}")
print("This breaks PHP syntax!")
# Safe approach with var_export
import json
safe_define = f"define( 'TEST', {repr(unsafe_value)} );"
print(f"Safe (var_export equivalent): {safe_define}")
EOFRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 509
🏁 Script executed:
# Look for test cases that call get_constants_string
rg "get_constants_string" --context 5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 3103
🏁 Script executed:
# Check if there are any tests that show how values are passed
rg "constant_values" inc/integrations/ --context 3Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 1303
🏁 Script executed:
# Let's also verify the actual behavior: check what shortcode_atts does with array_flip defaults
python3 << 'EOF'
# Simulate WordPress shortcode_atts with array_flip defaults
def shortcode_atts_simulation_with_flip(pairs, atts):
"""Simulate WordPress shortcode_atts with array_flip defaults"""
# array_flip on ['CONST_A', 'CONST_B'] gives {0: 'CONST_A', 1: 'CONST_B'}
# So pairs would be {0: 'CONST_A', 1: 'CONST_B'} as keys
result = dict(pairs) # Start with defaults
for key in atts:
if key in result:
result[key] = atts[key]
return result
def shortcode_atts_simulation_with_fill_keys(pairs, atts):
"""Simulate with array_fill_keys"""
result = dict(pairs)
for key in atts:
if key in result:
result[key] = atts[key]
return result
# Test scenario
all_constants = ['CONST_A', 'CONST_B', 'CONST_C']
provided_values = {'CONST_A': 'value_a', 'CONST_B': 'value_b'}
# Current approach (array_flip)
flipped_dict = {c: i for i, c in enumerate(all_constants)}
print("array_flip approach:")
print(f"Defaults: {flipped_dict}")
result1 = shortcode_atts_simulation_with_flip(flipped_dict, provided_values)
print(f"Result: {result1}")
# Proposed approach (array_fill_keys)
filled_dict = {c: '' for c in all_constants}
print("\narray_fill_keys approach:")
print(f"Defaults: {filled_dict}")
result2 = shortcode_atts_simulation_with_fill_keys(filled_dict, provided_values)
print(f"Result: {result2}")
print("\nWith injection attempt:")
injection_value = {'CONST_A': "test'; define('EVIL', '1'); echo '"}
result_unsafe = shortcode_atts_simulation_with_flip(flipped_dict, injection_value)
print(f"With injection (array_flip): {result_unsafe}")
# This would generate: define( 'CONST_A', 'test'; define('EVIL', '1'); echo '' );
result_safe = shortcode_atts_simulation_with_fill_keys(filled_dict, injection_value)
print(f"With injection (array_fill_keys): {result_safe}")
# This still generates unsafely: define( 'CONST_A', 'test'; define('EVIL', '1'); echo '' );
# But var_export would generate: define( 'CONST_A', "test'; define('EVIL', '1'); echo '" );
EOFRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 618
🏁 Script executed:
# Check PHP documentation about var_export and other safe alternatives
python3 << 'EOF'
# Demonstrate the safest approach for PHP code generation
print("=== Safe PHP constant definition generation ===\n")
values = {
'SIMPLE': 'value',
'WITH_SINGLE_QUOTE': "it's a test",
'WITH_DOUBLE_QUOTE': 'say "hello"',
'WITH_BOTH': '''test'; define('EVIL', '1'); echo 'done''',
'WITH_NEWLINE': "line1\nline2",
}
print("Method 1: sprintf with single quotes (UNSAFE):")
for const, val in values.items():
result = f"define( '{const}', '{val}' );"
print(f" {result}")
print("\nMethod 2: sprintf with escaped quotes (BETTER but complicated):")
for const, val in values.items():
escaped = val.replace("\\", "\\\\").replace("'", "\\'")
result = f"define( '{const}', '{escaped}' );"
print(f" {result}")
print("\nMethod 3: var_export (BEST - PHP 4.2.0+):")
for const, val in values.items():
result = f"define( '{const}', {repr(val)} );"
print(f" {result}")
EOFRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 1003
🏁 Script executed:
# Check PHP version requirements
if [ -f "composer.json" ]; then
cat composer.json | grep -A 5 "php"
fi
if [ -f "README.md" ]; then
grep -i "php" README.md | head -10
fiRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 5375
🏁 Script executed:
# Check the wizard implementation to see if input is validated
fd "configuration-results\|host-integrations" --type f | grep -E "(\.php|\.js)" | head -10Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 63
🏁 Script executed:
# Look for the wizard form/POST handling
rg "host.integrations\|configuration" --type php -l | head -5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 63
🏁 Script executed:
# Find the wizard implementation and form handling
rg "configuration-results" --type php -B 5 -A 5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 63
🏁 Script executed:
# Look for where constant_values come from in the wizard
rg "get_constants_string" --type php -B 10 -A 2 | head -40Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 3498
🏁 Script executed:
# Check if there's any escaping or sanitization of the values before passing to get_constants_string
rg "constant_values\|CONST_A\|CONST_B" --type php -B 3 -A 3 | head -50Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 63
Unescaped credential values in generated wp-config snippet enable PHP code injection.
The sprintf with single quotes allows quote-based injection. A credential containing test'; define('EVIL', '1'); echo ' would break PHP syntax. Use var_export() for proper escaping, and array_fill_keys() to replace the odd numeric defaults from array_flip().
🛠️ Safer constant snippet generation
- $constant_values = shortcode_atts(array_flip($this->get_all_constants()), $constant_values);
+ $constant_values = shortcode_atts(array_fill_keys($this->get_all_constants(), ''), $constant_values);
- $content[] = sprintf("define( '%s', '%s' );", $constant, $value);
+ $content[] = sprintf("define( '%s', %s );", $constant, var_export($value, true));🤖 Prompt for AI Agents
In `@inc/integrations/class-integration.php` around lines 566 - 580, The
get_constants_string method is vulnerable to PHP injection because it directly
interpolates unescaped $value into a define() via sprintf; replace the unsafe
defaults and escaping: use
shortcode_atts(array_fill_keys($this->get_all_constants(), ''),
$constant_values) instead of array_flip(...) to create proper string defaults,
and when building each define line in the foreach($constant_values as $constant
=> $value) use var_export($value, true) to produce a safely-escaped PHP literal
(e.g. "define( 'CONSTANT', " . var_export($value, true) . " );") so credential
values cannot break out of the single-quoted string.
| public function test_connection() { | ||
|
|
||
| $results = $this->send_gridpane_api_request( | ||
| 'application/delete-domain', | ||
| [ | ||
| 'server_ip' => $this->get_credential('WU_GRIDPANE_SERVER_ID'), | ||
| 'site_url' => $this->get_credential('WU_GRIDPANE_APP_ID'), | ||
| 'domain_url' => 'test.com', | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GridPane API documentation endpoints connection test read-only
💡 Result:
For GridPane, the API docs are published as a Postman Documenter collection (this is where the endpoint list lives): [1], [2].
Because I can’t access the interactive Postman page content from here (it doesn’t render in this environment), I can’t reliably name the exact “test” endpoint path. The safest way to do a connection test that’s read-only is:
- Open the Postman docs collection and pick any endpoint that is clearly GET and described as returning account/user info (i.e., no “create/update/delete” wording). [1], [2]
- Call it with your GridPane API token (generated in the GridPane UI via User Settings → GridPane API → Create New Token). [2]
- Treat HTTP 200 + JSON response as “token/auth OK”; 401/403 as “auth/token issue”; 5xx as server-side trouble.
Sources: [1], [2]
🌐 Web query:
GridPane API GET endpoints list user info account details
💡 Result:
There are two different products that commonly get mixed up here:
1) GridPane (gridpane.com) – WordPress hosting platform
GridPane’s API endpoint list is published in their Postman documentation (they note it’s evolving and endpoints may change). The public KB article points you to the Postman docs, but the KB itself doesn’t enumerate the GET endpoints (including “user/account details”). [1]
2) GridPanel (gridpanel.net) – proxy/scraping service
GridPanel’s docs do explicitly list a user/account info GET endpoint:
- GET
/api/user– returns info about the user tied to the API key (example response includesfirst_name,last_name,email,gbp_balance_in_pence). [2]
If you confirm which one you mean (GridPane vs GridPanel), I can list the relevant “user/account” GET endpoints for that product (for GridPane, that means pulling them from the Postman collection).
Sources: [1] [2]
🏁 Script executed:
# Search for other GridPane API endpoint calls in the codebase
rg "send_gridpane_api_request|application/" --type php -A 3 -B 1 inc/integrations/providers/gridpane/Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2543
🏁 Script executed:
# Look at the full test_connection method
sed -n '71,85p' inc/integrations/providers/gridpane/class-gridpane-integration.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 543
Use a read-only endpoint for test_connection instead of application/delete-domain.
Connection testing should not execute destructive operations. Replace the delete-domain call with a read-only endpoint (e.g., retrieving account or application info from GridPane API) to validate credentials without side effects.
🤖 Prompt for AI Agents
In `@inc/integrations/providers/gridpane/class-gridpane-integration.php` around
lines 71 - 79, The test_connection method currently calls the destructive
'application/delete-domain' endpoint; change it to call a read-only GridPane
endpoint (for example 'application/get' or 'account/info') using the same
send_gridpane_api_request call so credentials are validated without side
effects. Update the payload keys (still using
$this->get_credential('WU_GRIDPANE_SERVER_ID') and
$this->get_credential('WU_GRIDPANE_APP_ID')) to match the chosen read-only
endpoint parameters, and keep the call inside test_connection and error handling
unchanged so only the endpoint string and any required non-destructive params
are replaced.
| $trim = trim((string) $raw); | ||
|
|
||
| if ('0' === $trim) { | ||
| return '0'; | ||
| } | ||
|
|
||
| // Try to decode JSON if present, otherwise return raw string | ||
| $json = json_decode($raw); | ||
|
|
||
| return null !== $json ? $json : $raw; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and the specific lines in question
find . -type f -name "class-hestia-integration.php" | head -5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 145
🏁 Script executed:
# Let's read the entire file to understand the context
wc -l inc/integrations/providers/hestia/class-hestia-integration.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 147
🏁 Script executed:
# Read the relevant section with more context
sed -n '170,210p' inc/integrations/providers/hestia/class-hestia-integration.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 766
🏁 Script executed:
# Look for test_connection and send_hestia_request methods
rg -n "test_connection|send_hestia_request" inc/integrations/providers/hestia/class-hestia-integration.php -A 10Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 777
🏁 Script executed:
# Search for where returncode=yes is used
rg -n "returncode" inc/integrations/providers/hestia/ -B 3 -A 3Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 683
🏁 Script executed:
# Look for any documentation or comments about Hestia API return codes
rg -n "returncode|return code|Hestia" inc/integrations/providers/hestia/ -i | head -20Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2937
🌐 Web query:
Hestia Control Panel API returncode parameter response format documentation
💡 Result:
In Hestia Control Panel’s API, the POST parameter returncode controls whether the HTTP response body contains command output or just the numeric exit code:
returncode=yes→ response body is a plain number (the CLI command’s exit code), e.g.0on success, or10for forbidden, etc. (dev.brepo.ru)returncode=no(default) → response body is the command output (often multi-line text; if the CLI command is called withjsonoutput it will be JSON), and if the command succeeds with no output it returns the literal stringOK. (dev.brepo.ru)
The numeric codes returned when returncode=yes are documented under “Return codes” (0=OK, 1=E_ARGS, …). (hestiacp.com)
Citations:
- 1: https://dev.brepo.ru/bayrepo/hestiacp/src/commit/95a0e8727164d67d18a875cdd5c8b3cac8da1cab/web/api/index.php?utm_source=openai
- 2: https://dev.brepo.ru/bayrepo/hestiacp/src/commit/95a0e8727164d67d18a875cdd5c8b3cac8da1cab/web/api/index.php?utm_source=openai
- 3: https://hestiacp.com/docs/server-administration/rest-api
🏁 Script executed:
# Search for all calls to send_hestia_request to understand the full impact
rg -n "send_hestia_request" inc/integrations/providers/hestia/ -B 2 -A 5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2509
🏁 Script executed:
# Also check class-hestia-domain-mapping.php since it uses get_hestia()->send_hestia_request
rg -n "->send_hestia_request\|api_perform" inc/integrations/providers/hestia/class-hestia-domain-mapping.php -B 3 -A 8Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 485
🏁 Script executed:
# Let's verify the complete context of test_connection and how it's used
sed -n '55,75p' inc/integrations/providers/hestia/class-hestia-integration.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 434
🏁 Script executed:
# Check if test_connection is called from elsewhere to understand impact
rg -n "test_connection" inc/integrations/providers/hestia/ -B 2 -A 2Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 1136
🏁 Script executed:
# Check the return type and documentation of send_hestia_request
sed -n '120,130p' inc/integrations/providers/hestia/class-hestia-integration.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 389
Return non-zero Hestia return codes as errors.
When returncode=yes, non-zero numeric codes in the response body indicate API errors (auth failure, forbidden, etc.), yet the current code treats them as success. This causes test_connection() to return true even when credentials are invalid, creating false-positive validations.
🔧 Suggested guard
$trim = trim((string) $raw);
if ('0' === $trim) {
return '0';
}
+
+if (is_numeric($trim) && '0' !== $trim) {
+ return new \WP_Error('wu_hestia_api_error', sprintf(__('Hestia API returned error code: %s', 'ultimate-multisite'), $trim));
+}
// Try to decode JSON if present, otherwise return raw string
$json = json_decode($raw);
return null !== $json ? $json : $raw;📝 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.
| $trim = trim((string) $raw); | |
| if ('0' === $trim) { | |
| return '0'; | |
| } | |
| // Try to decode JSON if present, otherwise return raw string | |
| $json = json_decode($raw); | |
| return null !== $json ? $json : $raw; | |
| $trim = trim((string) $raw); | |
| if ('0' === $trim) { | |
| return '0'; | |
| } | |
| if (is_numeric($trim) && '0' !== $trim) { | |
| return new \WP_Error('wu_hestia_api_error', sprintf(__('Hestia API returned error code: %s', 'ultimate-multisite'), $trim)); | |
| } | |
| // Try to decode JSON if present, otherwise return raw string | |
| $json = json_decode($raw); | |
| return null !== $json ? $json : $raw; |
🤖 Prompt for AI Agents
In `@inc/integrations/providers/hestia/class-hestia-integration.php` around lines
189 - 198, The response-parsing code currently treats any numeric return code as
success; change the logic that examines $raw (the trimmed $trim value) so that
if returncode=yes and $trim is a numeric string not equal to '0' it is treated
as an error: create and return a WP_Error (or otherwise propagate an error)
containing the numeric code and a short message instead of returning the raw
value. Locate the response handling around the $raw/$trim variables (used by
test_connection()) and add a guard before the JSON decode that checks
is_numeric($trim) && $trim !== '0' and returns the error object so
test_connection() will fail for non-zero Hestia return codes.
| /** | ||
| * Handles removing a subdomain from ServerPilot. | ||
| * | ||
| * @since 2.5.0 | ||
| * | ||
| * @param string $subdomain The subdomain. | ||
| * @param int $site_id The site ID. | ||
| * @return void | ||
| */ | ||
| public function on_remove_subdomain(string $subdomain, int $site_id): void {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n inc/integrations/providers/serverpilot/class-serverpilot-domain-mapping.php | head -250Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 7136
🏁 Script executed:
# Find similar methods in the class to understand patterns
rg "on_.*subdomain|on_.*domain" inc/integrations/providers/serverpilot/class-serverpilot-domain-mapping.php -A 15Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2485
🏁 Script executed:
# Check for serverpilot API request patterns
rg "send_server_pilot_api_request" inc/integrations/providers/serverpilot/ -B 5 -A 5Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 8076
🏁 Script executed:
# Search for any references to on_remove_subdomain
rg "on_remove_subdomain" . --type phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 5700
🏁 Script executed:
find . -name "*ServerPilot*Test.php" -o -name "*serverpilot*test.php"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 63
🏁 Script executed:
# Check if there's a test and what it expects
rg "ServerPilot_Domain_Mapping" . --type phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 407
Implement subdomain removal to keep ServerPilot in sync.
The on_remove_subdomain handler is empty despite being registered and called when subdomains are removed. This creates an asymmetric state: subdomains added via on_add_subdomain won't be removed from ServerPilot, causing domain mismatches and potential SSL/routing issues.
The pattern from on_remove_domain (lines 131–148) directly applies: fetch the current domain list, filter out the removed subdomain, and send the updated list via the API.
🔧 Suggested implementation
public function on_remove_subdomain(string $subdomain, int $site_id): void {}
+public function on_remove_subdomain(string $subdomain, int $site_id): void {
+
+ $current_domain_list = $this->get_server_pilot_domains();
+
+ if ($current_domain_list && is_array($current_domain_list)) {
+ $current_domain_list = array_filter(
+ $current_domain_list,
+ fn($remote_domain) => $remote_domain !== $subdomain
+ );
+
+ $this->get_serverpilot()->send_server_pilot_api_request(
+ '',
+ [
+ 'domains' => $current_domain_list,
+ ]
+ );
+ }
+}📝 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.
| /** | |
| * Handles removing a subdomain from ServerPilot. | |
| * | |
| * @since 2.5.0 | |
| * | |
| * @param string $subdomain The subdomain. | |
| * @param int $site_id The site ID. | |
| * @return void | |
| */ | |
| public function on_remove_subdomain(string $subdomain, int $site_id): void {} | |
| /** | |
| * Handles removing a subdomain from ServerPilot. | |
| * | |
| * `@since` 2.5.0 | |
| * | |
| * `@param` string $subdomain The subdomain. | |
| * `@param` int $site_id The site ID. | |
| * `@return` void | |
| */ | |
| public function on_remove_subdomain(string $subdomain, int $site_id): void { | |
| $current_domain_list = $this->get_server_pilot_domains(); | |
| if ($current_domain_list && is_array($current_domain_list)) { | |
| $current_domain_list = array_filter( | |
| $current_domain_list, | |
| fn($remote_domain) => $remote_domain !== $subdomain | |
| ); | |
| $this->get_serverpilot()->send_server_pilot_api_request( | |
| '', | |
| [ | |
| 'domains' => $current_domain_list, | |
| ] | |
| ); | |
| } | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 184-184: Avoid unused parameters such as '$subdomain'. (undefined)
(UnusedFormalParameter)
[warning] 184-184: Avoid unused parameters such as '$site_id'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In `@inc/integrations/providers/serverpilot/class-serverpilot-domain-mapping.php`
around lines 175 - 184, The on_remove_subdomain method is currently empty
causing ServerPilot to retain removed subdomains; implement it the same way
on_remove_domain does: use the Site ID to fetch the current domain list (as
on_remove_domain does), remove the matching $subdomain from that list, and call
the ServerPilot update API to save the filtered domains; ensure you reference
and reuse the same helpers/code paths used by on_remove_domain (e.g., the
domain-fetching logic and the API update call) and handle the case where the
resulting list is empty or unchanged and log errors similarly to
on_remove_domain.
| foreach ($domains as $_domain) { | ||
| $response = wp_remote_post( | ||
| "https://premium.wpmudev.org/api/hosting/v1/$hosting_site_id/domains", | ||
| [ | ||
| 'timeout' => 50, | ||
| 'body' => [ | ||
| 'domain' => $_domain, | ||
| 'site_id' => $hosting_site_id, | ||
| ], | ||
| 'headers' => [ | ||
| 'Authorization' => $api_key, | ||
| ], | ||
| ] | ||
| ); | ||
|
|
||
| if (is_wp_error($response)) { | ||
| wu_log_add( | ||
| 'integration-wpmudev', | ||
| /* translators: %s: domain name. */ | ||
| sprintf(__('An error occurred while trying to add the custom domain %s to WPMU Dev hosting.', 'ultimate-multisite'), $_domain), | ||
| LogLevel::ERROR | ||
| ); | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| $body = json_decode(wp_remote_retrieve_body($response)); | ||
|
|
||
| if (isset($body->message)) { | ||
| wu_log_add( | ||
| 'integration-wpmudev', | ||
| /* translators: %1$s: domain name, %2$s: error message. */ | ||
| sprintf(__('An error occurred while trying to add the custom domain %1$s to WPMU Dev hosting: %2$s', 'ultimate-multisite'), $_domain, $body->message->message ?? $body->message), | ||
| LogLevel::ERROR | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "inc/integrations/providers/wpmudev/class-wpmudev-domain-mapping.php" | head -200Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 6634
🏁 Script executed:
# Create a test to verify json_decode behavior with invalid JSON
python3 << 'EOF'
import json
# Test what json_decode-equivalent does with invalid JSON
test_cases = [
('{}', 'valid empty JSON'),
('{"message": "error"}', 'valid JSON with message'),
('invalid json', 'invalid JSON'),
('', 'empty string'),
(None, 'null input'),
]
for test_input, desc in test_cases:
if test_input is None:
print(f"{desc}: Cannot test None directly")
continue
try:
result = json.loads(test_input)
print(f"{desc}: Parses to {result}")
except json.JSONDecodeError as e:
print(f"{desc}: Raises JSONDecodeError - {e}")
EOFRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 367
🌐 Web query:
WPMU DEV hosting API v1 domains endpoint response codes success error format
💡 Result:
I couldn’t find any public documentation from WPMU DEV that specifies Hosting API v1 “domains” endpoint success/error response schemas or a catalog of response codes.
What is publicly documented/observable:
- WPMU DEV’s Terms state their API-based services are intended to be accessed only by WPMU DEV’s own plugins (not as a public API), which aligns with the absence of public “Hosting API v1 domains” response-code docs. [1]
- Their Dashboard/Hub connectivity troubleshooting references JSON API endpoints like
https://wpmudev.com/api/dashboard/v1/and.../v2/and shows typical HTTP behavior (e.g.,200 OKandapplication/json, and logged failures surfacing as HTTP500in some cases). [2][3] - Their “Domains” documentation is UI-focused (Hub Domains Manager) and does not describe a public domains API contract. [4]
Practical next steps (since codes/format aren’t published):
- Capture real responses by inspecting requests made by the Hub/Dashboard (browser devtools / server logs) for the domains action you care about.
- Or ask WPMU DEV support for the exact Hosting API v1 domains endpoint contract (status codes + success/error JSON schema).
Sources
[1] WPMU DEV Terms of Service – “API Services” section. https://wpmudev.com/terms-of-service/
[2] WPMU DEV forum thread showing /api/dashboard/v1/ returning JSON and 200 OK. https://wpmudev.com/forums/topic/cant-login-to-the-dashboard/
[3] WPMU DEV forum thread referencing /api/dashboard/v2/ POST connectivity. https://wpmudev.com/forums/topic/wpmudev-dashboard-cant-connect-to-the-hub/
[4] WPMU DEV Hub docs – “Managing Domains and DNS Records” (UI docs, not API contract). https://wpmudev.com/docs/hub-2-0/domains/
Add HTTP status and JSON validation checks before logging success.
Non‑200 responses and invalid JSON currently fall through to the success log path. This masks failed domain provisioning and makes debugging difficult.
The code should validate HTTP status codes and JSON validity before treating a response as successful, especially when handling external API integrations where network issues, rate limiting, or auth failures can occur.
🔧 Suggested hardening
- $body = json_decode(wp_remote_retrieve_body($response));
+ $code = wp_remote_retrieve_response_code($response);
+ $raw = wp_remote_retrieve_body($response);
+
+ if (200 !== $code) {
+ wu_log_add(
+ 'integration-wpmudev',
+ /* translators: %1$s: domain name, %2$d: HTTP status code */
+ sprintf(__('An error occurred while trying to add the custom domain %1$s to WPMU Dev hosting (HTTP %2$d).', 'ultimate-multisite'), $_domain, $code),
+ LogLevel::ERROR
+ );
+ continue;
+ }
+
+ $body = json_decode($raw);
+
+ if (JSON_ERROR_NONE !== json_last_error()) {
+ wu_log_add(
+ 'integration-wpmudev',
+ /* translators: %s: domain name. */
+ sprintf(__('An error occurred while trying to add the custom domain %s to WPMU Dev hosting (invalid JSON response).', 'ultimate-multisite'), $_domain),
+ LogLevel::ERROR
+ );
+ continue;
+ }🤖 Prompt for AI Agents
In `@inc/integrations/providers/wpmudev/class-wpmudev-domain-mapping.php` around
lines 113 - 148, The loop handling wp_remote_post responses currently treats any
non-wp_error response as success; update the foreach ($domains as $_domain)
handling to validate HTTP status and JSON before logging success: after
wp_remote_post and is_wp_error checks, get the HTTP status with
wp_remote_retrieve_response_code($response) and if it is not a 2xx/status (e.g.,
200/201) call wu_log_add (using the same 'integration-wpmudev' tag and
LogLevel::ERROR) including the HTTP status and response body, then continue;
next, decode the body with json_decode and verify json_last_error() ===
JSON_ERROR_NONE (and that $body is an object/array) before using $body->message
— if JSON is invalid, log an error with the raw body and continue; only reach
the existing success path when status is 2xx and JSON is valid.
Summary by CodeRabbit
Security
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.