diff --git a/cleantalk.php b/cleantalk.php index 5d8a47fab..e3a7c07e8 100644 --- a/cleantalk.php +++ b/cleantalk.php @@ -2754,6 +2754,43 @@ function apbct_cookies_test() return null; } +/** + * Whether an API result is a transport/connection failure (not an invalid Access key). + * + * @param mixed $result API result array, error string, or errors['account_check'] bucket + * @return bool + */ +function apbct__is_connection_error_result($result) +{ + if ( is_array($result) ) { + if ( isset($result['error']) ) { + $result = $result['error']; + } else { + // State::errorAdd stores a list of error entries under the type key + $last = end($result); + if ( is_array($last) && isset($last['error']) ) { + $result = $last['error']; + } elseif ( is_string($last) ) { + $result = $last; + } else { + return false; + } + } + } + + if ( ! is_string($result) || $result === '' ) { + return false; + } + + $error = strtolower($result); + + return strpos($error, 'connection_error') !== false + || strpos($error, 'json_decode_error') !== false + || strpos($error, 'curl error') !== false + || strpos($error, 'failed to connect') !== false + || strpos($error, 'operation timed out') !== false; +} + /** * Inner function - Account status check. Scheduled in 1800 seconds for default! * @param $api_key @@ -2764,13 +2801,16 @@ function ct_account_status_check($api_key = null, $process_errors = true) { global $apbct; - $api_key = $api_key ?: $apbct->api_key; - $result = API::methodNoticePaidTill( + $previous_key_is_ok = ! empty($apbct->data['key_is_ok']); + $api_key = $api_key ?: $apbct->api_key; + $result = API::methodNoticePaidTill( $api_key, preg_replace('/http[s]?:\/\//', '', get_option('home'), 1), ! is_main_site() && $apbct->white_label ? 'anti-spam-hosting' : 'antispam' ); + $is_connection_error = ! empty($result['error']) && apbct__is_connection_error_result($result); + if ( empty($result['error']) || ! empty($result['valid']) ) { // Notices $apbct->data['notice_show'] = TT::getArrayValueAsInt($result, 'show_notice', 0); @@ -2865,6 +2905,11 @@ function ct_account_status_check($api_key = null, $process_errors = true) if ( ! empty($result['valid']) ) { $apbct->data['key_is_ok'] = true; $result = true; + } elseif ( $is_connection_error ) { + // Transport/SSL/timeout failures must not mark the Access key as invalid. + // @see https://app.doboard.com/1/task/54504 + $apbct->data['key_is_ok'] = $previous_key_is_ok; + $result = false; } else { $apbct->data['key_is_ok'] = false; $result = false; diff --git a/inc/apbct-sync-react.php b/inc/apbct-sync-react.php index e93aa5e8a..a9f78ae0f 100644 --- a/inc/apbct-sync-react.php +++ b/inc/apbct-sync-react.php @@ -45,27 +45,33 @@ function apbct_react_access_key_check() $apbct->errorDeleteAll(true); $account_is_ok = (bool) ct_account_status_check($apbct->settings['apikey']); + $connection_error = ! empty($apbct->errors['account_check']) + && apbct__is_connection_error_result($apbct->errors['account_check']); if ( $account_is_ok ) { $apbct->data['key_is_ok'] = true; $apbct->errorDelete('key_invalid key_get', 'save'); + $message = ''; + } elseif ( $connection_error ) { + // Keep key status; surface the real transport error instead of "key invalid" + $last_error = end($apbct->errors['account_check']); + $message = __('Error occurred while checking account status.', 'cleantalk-spam-protect'); + if ( is_array($last_error) && isset($last_error['error']) ) { + $message = (string) $last_error['error']; + } } else { $apbct->data['key_is_ok'] = false; $apbct->errorAdd( 'key_invalid', __('Testing failed. Please check the Access key.', 'cleantalk-spam-protect') ); + $message = __('Testing failed. Please check the Access key.', 'cleantalk-spam-protect'); } $apbct->data['key_changed'] = false; $apbct->saveData(); - apbct_react_sync_json_response( - $account_is_ok, - $account_is_ok - ? '' - : __('Testing failed. Please check the Access key.', 'cleantalk-spam-protect') - ); + apbct_react_sync_json_response($account_is_ok, $message); } function apbct_react_sfw_update() diff --git a/inc/cleantalk-settings.php b/inc/cleantalk-settings.php index 590ed3de9..89efaefeb 100644 --- a/inc/cleantalk-settings.php +++ b/inc/cleantalk-settings.php @@ -2714,9 +2714,6 @@ function apbct_settings__sync($direct_call = false) // Feedback with app_agent ct_send_feedback('0:' . APBCT_AGENT); // 0 - request_id, agent version. - // Key is good by default - $apbct->data['key_is_ok'] = true; - // Checking account status $result = ct_account_status_check($apbct->settings['apikey']); @@ -2740,8 +2737,11 @@ function apbct_settings__sync($direct_call = false) // Updating brief data for dashboard widget cleantalk_get_brief_data($apbct->settings['apikey']); - // Key is not valid - } else { + // Key is not valid — but only when the cloud explicitly rejected it (not on connection errors) + } elseif ( + empty($apbct->errors['account_check']) || + ! apbct__is_connection_error_result($apbct->errors['account_check']) + ) { $apbct->data['key_is_ok'] = false; $apbct->errorAdd( 'key_invalid', diff --git a/lib/Cleantalk/Antispam/Integrations/CleantalkExternalForms.php b/lib/Cleantalk/Antispam/Integrations/CleantalkExternalForms.php index 6ef38e73f..279fb960b 100644 --- a/lib/Cleantalk/Antispam/Integrations/CleantalkExternalForms.php +++ b/lib/Cleantalk/Antispam/Integrations/CleantalkExternalForms.php @@ -4,6 +4,7 @@ use Cleantalk\ApbctWP\Variables\Post; use Cleantalk\ApbctWP\Escape; +use Cleantalk\ApbctWP\Validate; class CleantalkExternalForms extends IntegrationBase { @@ -14,8 +15,15 @@ public function doPrepareActions($argument) { if ( empty($_POST) || !apbct_is_post() - || Post::get('cleantalk_hidden_method') === '' - || Post::get('cleantalk_hidden_action') === '' + || Post::getString('cleantalk_hidden_method') === '' + || Post::getString('cleantalk_hidden_action') === '' + ) { + return false; + } + + // Reject non-http(s) actions early (e.g. javascript:/data:) before any output. + if ( ! $this->isAllowedExternalFormAction(Post::getString('cleantalk_hidden_action')) + || ! $this->isAllowedExternalFormMethod(Post::getString('cleantalk_hidden_method')) ) { return false; } @@ -27,11 +35,27 @@ public function getDataForChecking($argument) { if ( ! empty($_POST) && apbct_is_post() - && Post::get('cleantalk_hidden_method') !== '' - && Post::get('cleantalk_hidden_action') !== '' + && Post::getString('cleantalk_hidden_method') !== '' + && Post::getString('cleantalk_hidden_action') !== '' ) { - $this->action = Escape::escHtml(Post::get('cleantalk_hidden_action')); - $this->method = Escape::escHtml(Post::get('cleantalk_hidden_method')); + $action = Post::getString('cleantalk_hidden_action'); + $method = Post::getString('cleantalk_hidden_method'); + + if ( ! $this->isAllowedExternalFormAction($action) + || ! $this->isAllowedExternalFormMethod($method) + ) { + return null; + } + + // Keep a sanitized raw URL; escape for HTML only when rendering the form. + // HTML escaping alone does not block javascript: URLs in action attributes. + $this->action = Escape::escUrlRaw($action); + $this->method = strtoupper($method); + + if ( empty($this->action) ) { + return null; + } + unset($_POST['cleantalk_hidden_action'], $_POST['cleantalk_hidden_method']); /** @@ -111,6 +135,22 @@ private function constructFormInnerElements($arr, $recursive_key) private function constructOriginExternalForm($action, $method) { + if ( empty($action) + || empty($method) + || ! $this->isAllowedExternalFormAction($action) + || ! $this->isAllowedExternalFormMethod($method) + ) { + return ''; + } + + // Restrict protocols at output: only http/https are allowed in form action. + $action = esc_url($action, array('http', 'https')); + $method = Escape::escHtml(strtoupper($method)); + + if ( empty($action) ) { + return ''; + } + // HTML form template $form_template = ' @@ -153,4 +193,30 @@ private function constructOriginExternalForm($action, $method) return $form_template; } + + /** + * External form action must be an absolute http/https URL. + * javascript:, data:, vbscript: and other schemes are rejected. + * + * @param mixed $action + * + * @return bool + */ + private function isAllowedExternalFormAction($action) + { + return is_string($action) && Validate::isUrl($action); + } + + /** + * HTML form method may only be GET or POST. + * + * @param mixed $method + * + * @return bool + */ + private function isAllowedExternalFormMethod($method) + { + return is_string($method) + && in_array(strtolower($method), array('get', 'post'), true); + } } diff --git a/lib/Cleantalk/Common/Validate.php b/lib/Cleantalk/Common/Validate.php index 9fabba8f2..a58df60e5 100644 --- a/lib/Cleantalk/Common/Validate.php +++ b/lib/Cleantalk/Common/Validate.php @@ -82,8 +82,16 @@ public static function isValidFilePath($variable) public static function isUrl($url) { - return ( strpos($url, 'http://') !== false || strpos($url, 'https://') !== false ) && - filter_var($url, FILTER_VALIDATE_URL); + if ( ! is_string($url) || $url === '' ) { + return false; + } + + $scheme = strtolower((string) parse_url($url, PHP_URL_SCHEME)); + + // Allow only http/https. Checking the scheme prevents javascript:/data: bypasses + // that embed "http://" or "https://" in the rest of the string. + return in_array($scheme, array('http', 'https'), true) && + (bool) filter_var($url, FILTER_VALIDATE_URL); } /** diff --git a/tests/Antispam/IntegrationsByHook/TestCleantalkExternalForms.php b/tests/Antispam/IntegrationsByHook/TestCleantalkExternalForms.php new file mode 100644 index 000000000..acd8743dc --- /dev/null +++ b/tests/Antispam/IntegrationsByHook/TestCleantalkExternalForms.php @@ -0,0 +1,106 @@ +integration = new CleantalkExternalForms(); + $this->post_global = $_POST; + $this->server_global = $_SERVER; + $_SERVER['REQUEST_METHOD'] = 'POST'; + $this->resetVariableCaches(); + } + + protected function tearDown(): void + { + $_POST = $this->post_global; + $_SERVER = $this->server_global; + $this->resetVariableCaches(); + } + + private function resetVariableCaches() + { + Post::getInstance()->variables = []; + Server::getInstance()->variables = []; + } + + public function testDoPrepareActionsRejectsJavascriptAction() + { + $_POST = array( + 'cleantalk_hidden_method' => 'POST', + 'cleantalk_hidden_action' => 'javascript:alert(String.fromCharCode(80,83,67,45,88,83,83));void 0', + ); + $this->resetVariableCaches(); + + $this->assertFalse($this->integration->doPrepareActions(null)); + } + + public function testDoPrepareActionsRejectsJavascriptHttpsBypass() + { + $_POST = array( + 'cleantalk_hidden_method' => 'POST', + 'cleantalk_hidden_action' => 'javascript://https://evil.com%0aalert(1)', + ); + $this->resetVariableCaches(); + + $this->assertFalse($this->integration->doPrepareActions(null)); + } + + public function testDoPrepareActionsRejectsDataUri() + { + $_POST = array( + 'cleantalk_hidden_method' => 'POST', + 'cleantalk_hidden_action' => 'data:text/html,', + ); + $this->resetVariableCaches(); + + $this->assertFalse($this->integration->doPrepareActions(null)); + } + + public function testDoPrepareActionsRejectsInvalidMethod() + { + $_POST = array( + 'cleantalk_hidden_method' => 'PUT', + 'cleantalk_hidden_action' => 'https://example.com/form', + ); + $this->resetVariableCaches(); + + $this->assertFalse($this->integration->doPrepareActions(null)); + } + + public function testDoPrepareActionsAllowsHttpHttps() + { + $_POST = array( + 'cleantalk_hidden_method' => 'POST', + 'cleantalk_hidden_action' => 'https://example.com/form', + ); + $this->resetVariableCaches(); + + $this->assertTrue($this->integration->doPrepareActions(null)); + + $_POST['cleantalk_hidden_action'] = 'http://45.137.81.184/'; + $this->resetVariableCaches(); + + $this->assertTrue($this->integration->doPrepareActions(null)); + } +} diff --git a/tests/ApbctWP/ValidateTest.php b/tests/ApbctWP/ValidateTest.php index ce24b9745..54da7a004 100644 --- a/tests/ApbctWP/ValidateTest.php +++ b/tests/ApbctWP/ValidateTest.php @@ -19,5 +19,12 @@ public function testIsUrl() $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl('https://cleantalk.org/some-path/with_parameter=😭')); $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl('ftp://cleantalk.org')); $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl('cleantalk.org')); + $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl('javascript:alert(1)')); + $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl('javascript:alert(String.fromCharCode(80,83,67,45,88,83,83));void 0')); + $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl('javascript://https://evil.com%0aalert(1)')); + $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl("javascript:alert('http://x')")); + $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl('data:text/html,')); + $this->assertFalse(\Cleantalk\ApbctWP\Validate::isUrl('vbscript:msgbox(1)')); + $this->assertTrue(\Cleantalk\ApbctWP\Validate::isUrl('http://45.137.81.184/')); } } diff --git a/tests/Common/HelperTest.php b/tests/Common/HelperTest.php index b240d0a97..65b17355e 100644 --- a/tests/Common/HelperTest.php +++ b/tests/Common/HelperTest.php @@ -12,9 +12,26 @@ public function test_http__multi_request_error() { $this->assertArrayHasKey( 'error', Helper::httpMultiRequest( array(array('https://google.com')) ) ); } + /** + * @group integration + */ public function test_http__multi_request_success() { $res = Helper::httpMultiRequest( array('https://google.com', 'https://apple.com') ); $this->assertIsArray( $res ); + + // Top-level error (e.g. CURL_NOT_INSTALLED) is an array of strings and would + // falsely pass assertContainsOnly('string') without this check. + if ( isset($res['error']) ) { + $this->markTestSkipped('Outbound HTTP multi-request not available in this environment'); + } + + // Per-URL failures come back as arrays (e.g. ['error' => '']). + foreach ( $res as $body ) { + if ( ! is_string($body) ) { + $this->markTestSkipped('Outbound HTTP multi-request not available in this environment'); + } + } + $this->assertContainsOnly( 'string', $res ); }