From ad134bb344ea413f98d67114791196d178f80f2e Mon Sep 17 00:00:00 2001 From: alexandergull Date: Fri, 18 Sep 2026 12:27:06 +0500 Subject: [PATCH 1/2] Upd. Cleantalk.php. Rotate moderate. Now use CURLOPT_RESOLVE for IP instead of converting IP as string if IP is used as fallback. https://app.doboard.com/1/task/55885 --- lib/Cleantalk/Antispam/Cleantalk.php | 98 ++++++- tests/Antispam/TestCleantalkResolveIP.php | 328 ++++++++++++++++++++++ 2 files changed, 413 insertions(+), 13 deletions(-) create mode 100644 tests/Antispam/TestCleantalkResolveIP.php diff --git a/lib/Cleantalk/Antispam/Cleantalk.php b/lib/Cleantalk/Antispam/Cleantalk.php index 097ca0f7c..99af2cd98 100644 --- a/lib/Cleantalk/Antispam/Cleantalk.php +++ b/lib/Cleantalk/Antispam/Cleantalk.php @@ -5,6 +5,7 @@ use Cleantalk\ApbctWP\Helper; use Cleantalk\ApbctWP\HTTP\Request; use Cleantalk\Common\DNS; +use Cleantalk\Common\TT; /** * Cleantalk base class @@ -105,6 +106,14 @@ class Cleantalk */ private $downServers; + /** + * IPs already picked by the IP fallback during the current request cycle. + * The pool hostname is shared by every node, so only the IP identifies a server there. + * + * @var array + */ + private $down_ips = array(); + /** * Function checks whether it is possible to publish the message * @@ -259,6 +268,7 @@ public function httpRequest($msg) while (($result === false || (is_object($result) && $result->errno != 0)) && $attempt <= $number_of_connection_attempts) { // Getting type of error $type_error = $this->getTypeError($result); + $ip_to_resolve = false; $failed_urls = $this->work_url; if ( ! empty($this->work_url) ) { @@ -266,8 +276,8 @@ public function httpRequest($msg) } if ( ($type_error === 'getaddrinfo_error' || $type_error === 'connection_timeout') && $attempt === 1 ) { - $this->rotateModerateAndUseIP(); - //exit if next sendRequest failed, because change dns->ip is only way to fix errors above + $ip_to_resolve = $this->rotateModerateAndUseIP(); + // Exit if next sendRequest failed, because bypassing DNS is the last retry. $attempt = $attempt + 2; } else { $this->rotateModerate(); @@ -275,7 +285,7 @@ public function httpRequest($msg) $attempt = $attempt + 1; } - $result = $this->sendRequest($msg, $this->work_url, $this->server_timeout); + $result = $this->sendRequest($msg, $this->work_url, $this->server_timeout, $ip_to_resolve); /** @psalm-suppress PossiblyInvalidPropertyFetch */ if ( $result !== false && $result->errno === 0 ) { $this->server_change = true; @@ -341,7 +351,8 @@ public function rotateModerate() } /** - * * @todo Refactor / fix logic errors + * @todo Refactor / fix logic errors + * @return string|false Selected IP address, or false if none was selected. */ public function rotateModerateAndUseIP() { @@ -356,29 +367,58 @@ public function rotateModerateAndUseIP() $servers = $this->getServersIp($url_host); if ( ! $servers ) { - return; + return false; } $apbct->settings['wp__use_builtin_http_api'] = false; // Loop until find work server foreach ( $servers as $server ) { - $dns = Helper::ipResolve($server['ip']); - if ( ! $dns ) { + $ip = TT::getArrayValueAsString($server, 'ip'); + if ( empty($ip) ) { continue; } - $this->work_url = $url_protocol . $server['ip'] . $url_suffix; + // The pool hostname does not distinguish nodes, so dedup the fallback by IP. + if ( in_array($ip, $this->down_ips, true) ) { + continue; + } - // Do not checking previous down server - if ( ! empty($this->downServers) && in_array($this->work_url, $this->downServers) ) { + // A forward-confirmed PTR points to the exact node and is preferred. + // Without a PTR the pool hostname is still correct for SNI and the certificate, + // because the node is pinned by IP via CURLOPT_RESOLVE anyway. + $dns = $this->resolvePtr($ip); + $host = $dns ?: $url_host; + + $work_url = $url_protocol . $host . $url_suffix; + + // Do not check the previous down server. Only a node-specific hostname + // identifies a server, the pool hostname is shared by all of them. + if ( $dns && ! empty($this->downServers) && in_array($work_url, $this->downServers, true) ) { continue; } + $this->work_url = $work_url; + $this->down_ips[] = $ip; $this->server_ttl = $server['ttl']; $this->server_change = true; - break; + + return $ip; } + + return false; + } + + /** + * Seam over the forward-confirmed reverse DNS lookup, overridable in tests. + * + * @param string $ip + * + * @return string|false PTR hostname that resolves back to $ip, false otherwise. + */ + protected function resolvePtr($ip) + { + return Helper::ipResolve($ip); } /** @@ -510,11 +550,12 @@ public function httpPing($host) * @param string|array $data * @param string $url * @param int $server_timeout + * @param string|false $ip_to_resolve IP address to use for the URL hostname without changing the URL. * * @return boolean|CleantalkResponse * @throws \Exception */ - private function sendRequest($data, $url, $server_timeout = 3) + private function sendRequest($data, $url, $server_timeout = 3, $ip_to_resolve = false) { //Cleaning from 'null' values $tmp_data = array(); @@ -552,10 +593,18 @@ private function sendRequest($data, $url, $server_timeout = 3) ? $url . '/' . $this->method_uri : $url; + $options = array('timeout' => $server_timeout); + if ($ip_to_resolve) { + $connection_resolve_string = $this->maybeResolveIPInsteadOfHost($url, $ip_to_resolve); + if ($connection_resolve_string && defined('CURLOPT_RESOLVE')) { + $options[CURLOPT_RESOLVE] = array($connection_resolve_string); + } + } + $result = $http->setUrl($url) ->setData($data) ->setPresets($presets) - ->setOptions(['timeout' => $server_timeout]) + ->setOptions($options) ->request(); $errstr = null; @@ -583,6 +632,29 @@ private function sendRequest($data, $url, $server_timeout = 3) return $response; } + /** + * @param string $url + * @param string $ip_to_resolve + * @return false|string + */ + public function maybeResolveIPInsteadOfHost(string $url, string $ip_to_resolve) + { + if ( + filter_var($ip_to_resolve, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) + ) { + $url_parts = parse_url($url); + $host = TT::getArrayValueAsString($url_parts, 'host'); + $port = TT::getArrayValueAsInt($url_parts, 'port'); + $is_ssl = TT::getArrayValueAsString($url_parts, 'scheme') !== 'http'; + if (!empty($host)) { + // URL may omit the port, then it is defined by the scheme + $port = !empty($port) ? $port : ($is_ssl ? 443 : 80); + return $host . ':' . $port . ':' . $ip_to_resolve; + } + } + return false; + } + /** * Call check_bot API method * diff --git a/tests/Antispam/TestCleantalkResolveIP.php b/tests/Antispam/TestCleantalkResolveIP.php new file mode 100644 index 000000000..a58394a92 --- /dev/null +++ b/tests/Antispam/TestCleantalkResolveIP.php @@ -0,0 +1,328 @@ + PTR hostname. Missing key means the IP has no usable PTR. + */ + public $ptr_map = array(); + + public function getServersIp($host) + { + return $this->servers_fixture; + } + + protected function resolvePtr($ip) + { + return isset($this->ptr_map[$ip]) ? $this->ptr_map[$ip] : false; + } +} + +/** + * Exposes prepared cURL options without performing a network request. + */ +class ApbctResolveOptionsHarness extends CommonRequest +{ + public function prepareOptionsForTest() + { + $convert = new \ReflectionMethod(CommonRequest::class, 'convertOptionsTocURLFormat'); + $convert->setAccessible(true); + $convert->invoke($this); + + $this->appendOptionsObligatory(); + $this->processPresets(); + + return $this->options; + } +} + +/** + * Regression guard for the DNS-failure fallback. + * + * When moderate*.cleantalk.org can not be resolved, the plugin must keep the hostname + * in the URL and pin it to the selected IP via CURLOPT_RESOLVE, so SNI and the TLS + * certificate hostname check keep working. + * + * Historical bugs covered here: + * - the request was sent to https://, which fails with + * "SSL: no alternative certificate subject name matches target host name"; + * - the literal IP request was made with TLS verification switched off; + * - CURLOPT_RESOLVE was never set, because the guard required an explicit port + * that a normal API URL does not have, so the fallback was dead code; + * - an explicit non-default port was overwritten with 443; + * - an IP without a PTR record was skipped, even though it is a valid A record + * of the pool hostname and the fallback pins it by IP anyway; + * - deduplication of failed nodes by URL discards every candidate as soon as the + * URL stops being node-specific. + */ +class TestCleantalkResolveIP extends ApbctTestCase +{ + const IP = '167.71.167.197'; + const HOST = 'moderate2.cleantalk.org'; + const IP_SECOND = '159.69.57.9'; + const HOST_SECOND = 'moderate8.cleantalk.org'; + const POOL_HOST = 'moderate.cleantalk.org'; + const POOL_URL = 'https://moderate.cleantalk.org'; + + /** + * @var Cleantalk + */ + private $ct; + + public function setUp(): void + { + $this->ct = new Cleantalk(); + } + + /** + * The main regression: a normal API URL has no explicit port, so the mapping + * must fall back to the scheme default instead of being skipped. + */ + public function testHttpsUrlWithoutPortIsPinnedToIpOnDefaultPort() + { + $this->assertSame( + self::HOST . ':443:' . self::IP, + $this->ct->maybeResolveIPInsteadOfHost('https://' . self::HOST . '/api2.0', self::IP) + ); + } + + public function testExplicitPortIsPreserved() + { + $this->assertSame( + self::HOST . ':8443:' . self::IP, + $this->ct->maybeResolveIPInsteadOfHost('https://' . self::HOST . ':8443/api2.0', self::IP) + ); + } + + public function testPlainHttpUrlUsesPort80() + { + $this->assertSame( + self::HOST . ':80:' . self::IP, + $this->ct->maybeResolveIPInsteadOfHost('http://' . self::HOST . '/api2.0', self::IP) + ); + } + + /** + * The hostname must stay in the mapping: it is what SNI and the certificate + * check are validated against. + */ + public function testHostnameIsKeptAndNotReplacedByIp() + { + $resolve = $this->ct->maybeResolveIPInsteadOfHost('https://' . self::HOST . '/api2.0', self::IP); + + $this->assertStringStartsWith(self::HOST . ':', $resolve); + $this->assertStringEndsWith(':' . self::IP, $resolve); + } + + /** + * @dataProvider unusableInputProvider + */ + public function testUnusableInputProducesNoMapping($url, $ip) + { + $this->assertFalse($this->ct->maybeResolveIPInsteadOfHost($url, $ip)); + } + + public function unusableInputProvider() + { + return array( + 'not an IP' => array('https://' . self::HOST . '/api2.0', self::HOST), + 'empty IP' => array('https://' . self::HOST . '/api2.0', ''), + 'malformed IP' => array('https://' . self::HOST . '/api2.0', '167.71.167'), + 'IPv6 is not supported by CURLOPT_RESOLVE mapping' => array( + 'https://' . self::HOST . '/api2.0', + '2a03:6f00:1::1', + ), + 'URL without host' => array('/api2.0', self::IP), + 'empty URL' => array('', self::IP), + ); + } + + /** + * Pinning the host to an IP must never weaken TLS: the fallback used to send the + * request with CURLOPT_SSL_VERIFYPEER=false and CURLOPT_SSL_VERIFYHOST=0. + */ + public function testResolveMappingKeepsTlsVerificationEnabled() + { + $resolve = $this->ct->maybeResolveIPInsteadOfHost( + 'https://' . self::HOST . '/api2.0', + self::IP + ); + + $request = new ApbctResolveOptionsHarness(); + $request + ->setUrl('https://' . self::HOST . '/api2.0') + ->setOptions(array('timeout' => 15, CURLOPT_RESOLVE => array($resolve))); + + $options = $request->prepareOptionsForTest(); + + $this->assertSame(array($resolve), $options[CURLOPT_RESOLVE]); + $this->assertTrue($options[CURLOPT_SSL_VERIFYPEER]); + $this->assertSame(2, $options[CURLOPT_SSL_VERIFYHOST]); + $this->assertSame('https://' . self::HOST . '/api2.0', $options[CURLOPT_URL]); + } + + /** + * @param array $servers + * @param array $ptr_map + * @param array $down_servers + * + * @return ApbctRotateModerateStub + */ + private function makeRotateStub(array $servers, array $ptr_map = array(), array $down_servers = array()) + { + $ct = new ApbctRotateModerateStub(); + $ct->server_url = self::POOL_URL; + $ct->servers_fixture = $servers; + $ct->ptr_map = $ptr_map; + + if ($down_servers) { + $property = new \ReflectionProperty(Cleantalk::class, 'downServers'); + $property->setAccessible(true); + $property->setValue($ct, $down_servers); + } + + return $ct; + } + + private function server($ip, $ttl = 300) + { + return array('ip' => $ip, 'host' => self::POOL_HOST, 'ttl' => $ttl); + } + + /** + * A node with a forward-confirmed PTR keeps that node-specific hostname in the URL. + */ + public function testPtrHostnameIsUsedWhenAvailable() + { + $ct = $this->makeRotateStub( + array($this->server(self::IP)), + array(self::IP => self::HOST) + ); + + $this->assertSame(self::IP, $ct->rotateModerateAndUseIP()); + $this->assertSame('https://' . self::HOST, $ct->work_url); + } + + /** + * Regression: an IP without a PTR used to be skipped entirely, which silently + * disabled the fallback. The pool hostname is a valid SNI/certificate name, + * so such an IP must still be usable. + */ + public function testIpWithoutPtrFallsBackToPoolHostnameInsteadOfBeingSkipped() + { + $ct = $this->makeRotateStub(array($this->server(self::IP))); + + $this->assertSame(self::IP, $ct->rotateModerateAndUseIP()); + $this->assertSame(self::POOL_URL, $ct->work_url); + } + + /** + * The first candidate must be taken even if only later ones have a PTR. + */ + public function testFirstIpIsTakenEvenWhenOnlyLaterNodesHavePtr() + { + $ct = $this->makeRotateStub( + array($this->server(self::IP), $this->server(self::IP_SECOND)), + array(self::IP_SECOND => self::HOST_SECOND) + ); + + $this->assertSame(self::IP, $ct->rotateModerateAndUseIP()); + $this->assertSame(self::POOL_URL, $ct->work_url); + } + + /** + * A node that already failed under its own hostname must not be picked again. + */ + public function testNodeSpecificHostnameThatAlreadyFailedIsSkipped() + { + $ct = $this->makeRotateStub( + array($this->server(self::IP), $this->server(self::IP_SECOND)), + array(self::IP => self::HOST, self::IP_SECOND => self::HOST_SECOND), + array('https://' . self::HOST) + ); + + $this->assertSame(self::IP_SECOND, $ct->rotateModerateAndUseIP()); + $this->assertSame('https://' . self::HOST_SECOND, $ct->work_url); + } + + /** + * Regression: the pool hostname is shared by every node, so matching it against + * the list of failed URLs must not discard the candidates. Deduplicating the + * fallback by URL instead of by IP would return false here and kill the retry. + */ + public function testFailedPoolUrlDoesNotDiscardCandidatesWithoutPtr() + { + $ct = $this->makeRotateStub( + array($this->server(self::IP), $this->server(self::IP_SECOND)), + array(), + array(self::POOL_URL) + ); + + $this->assertSame(self::IP, $ct->rotateModerateAndUseIP()); + $this->assertSame(self::POOL_URL, $ct->work_url); + } + + /** + * The same IP must not be handed out twice within one request cycle. + */ + public function testAlreadyUsedIpIsNotReturnedTwice() + { + $ct = $this->makeRotateStub( + array($this->server(self::IP), $this->server(self::IP_SECOND)) + ); + + $this->assertSame(self::IP, $ct->rotateModerateAndUseIP()); + $this->assertSame(self::IP_SECOND, $ct->rotateModerateAndUseIP()); + $this->assertFalse($ct->rotateModerateAndUseIP()); + } + + /** + * getServersIp() returns a null IP when it could not obtain any record. + */ + public function testEntriesWithoutIpAreSkipped() + { + $ct = $this->makeRotateStub( + array($this->server(null), $this->server(''), $this->server(self::IP)) + ); + + $this->assertSame(self::IP, $ct->rotateModerateAndUseIP()); + } + + public function testNoServersMeansNoFallback() + { + $ct = $this->makeRotateStub(array()); + + $this->assertFalse($ct->rotateModerateAndUseIP()); + } + + /** + * End to end of the two halves: whatever hostname the rotation settled on, the + * mapping must pin exactly that hostname to the returned IP. + */ + public function testSelectedIpAndWorkUrlProduceConsistentMapping() + { + foreach (array(array(self::IP => self::HOST), array()) as $ptr_map) { + $ct = $this->makeRotateStub(array($this->server(self::IP)), $ptr_map); + $ip = $ct->rotateModerateAndUseIP(); + + $expected_host = $ptr_map ? self::HOST : self::POOL_HOST; + + $this->assertSame(self::IP, $ip); + $this->assertSame( + $expected_host . ':443:' . self::IP, + $ct->maybeResolveIPInsteadOfHost($ct->work_url, $ip) + ); + } + } +} From 94f9381225fa6e9b9a67b09e05fa76f76e66f0b4 Mon Sep 17 00:00:00 2001 From: alexandergull Date: Mon, 21 Sep 2026 15:46:13 +0500 Subject: [PATCH 2/2] =?UTF-8?q?=E2=84=963=20Probably=20fixed=20CP.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- inc/cleantalk-common.php | 12 + lib/Cleantalk/Antispam/Cleantalk.php | 206 +++++++++++------- .../IntegrationsByClass/Woocommerce.php | 1 + .../ApbctWP/Antispam/ForceProtection.php | 1 + .../ContactsEncoder/ContactsEncoder.php | 1 + tests/Antispam/TestCleantalkResolveIP.php | 55 +++-- 6 files changed, 176 insertions(+), 100 deletions(-) diff --git a/inc/cleantalk-common.php b/inc/cleantalk-common.php index 5967935cb..ae84769ab 100644 --- a/inc/cleantalk-common.php +++ b/inc/cleantalk-common.php @@ -263,6 +263,7 @@ function apbct_base_call($params = array(), $reg_flag = false) $config = ct_get_server(); $ct->server_url = APBCT_MODERATE_URL; $ct->work_url = isset($config['ct_work_url']) && preg_match('/https:\/\/.+/', $config['ct_work_url']) ? $config['ct_work_url'] : null; + $ct->work_ip = isset($config['ct_work_ip']) ? $config['ct_work_ip'] : null; $ct->server_ttl = isset($config['ct_server_ttl']) ? $config['ct_server_ttl'] : null; $ct->server_changed = isset($config['ct_server_changed']) ? $config['ct_server_changed'] : null; @@ -285,6 +286,7 @@ function apbct_base_call($params = array(), $reg_flag = false) 'cleantalk_server', array( 'ct_work_url' => $ct->work_url, + 'ct_work_ip' => $ct->work_ip, 'ct_server_ttl' => $ct->server_ttl, 'ct_server_changed' => time(), ) @@ -330,6 +332,7 @@ function apbct_rotate_moderate() 'cleantalk_server', array( 'ct_work_url' => $ct->work_url, + 'ct_work_ip' => $ct->work_ip, 'ct_server_ttl' => $ct->server_ttl, 'ct_server_changed' => time(), ) @@ -972,6 +975,7 @@ function ct_get_server() if ( ! is_array($ct_server) ) { $ct_server = array( 'ct_work_url' => null, + 'ct_work_ip' => null, 'ct_server_ttl' => null, 'ct_server_changed' => null ); @@ -979,6 +983,12 @@ function ct_get_server() $ct_server['ct_work_url'] = Sanitize::sanitizeCleantalkServerUrl(TT::getArrayValueAsString($ct_server, 'ct_work_url')); + // The node is selected by IP, so a stored value must be a usable IPv4 literal. + $stored_ip = TT::getArrayValueAsString($ct_server, 'ct_work_ip'); + $ct_server['ct_work_ip'] = filter_var($stored_ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) + ? $stored_ip + : null; + return $ct_server; } @@ -1078,6 +1088,7 @@ function ct_send_feedback($feedback_request = null) $config = ct_get_server(); $ct->server_url = APBCT_MODERATE_URL; $ct->work_url = isset($config['ct_work_url']) && preg_match('/http:\/\/.+/', $config['ct_work_url']) ? $config['ct_work_url'] : null; + $ct->work_ip = isset($config['ct_work_ip']) ? $config['ct_work_ip'] : null; $ct->server_ttl = isset($config['ct_server_ttl']) ? $config['ct_server_ttl'] : null; $ct->server_changed = isset($config['ct_server_changed']) ? $config['ct_server_changed'] : null; @@ -1092,6 +1103,7 @@ function ct_send_feedback($feedback_request = null) 'cleantalk_server', array( 'ct_work_url' => $ct->work_url, + 'ct_work_ip' => $ct->work_ip, 'ct_server_ttl' => $ct->server_ttl, 'ct_server_changed' => time(), ) diff --git a/lib/Cleantalk/Antispam/Cleantalk.php b/lib/Cleantalk/Antispam/Cleantalk.php index 99af2cd98..cc448f978 100644 --- a/lib/Cleantalk/Antispam/Cleantalk.php +++ b/lib/Cleantalk/Antispam/Cleantalk.php @@ -51,6 +51,16 @@ class Cleantalk */ public $work_url; + /** + * IP of the node the work url is pinned to. + * + * The URL always carries the pool hostname, so it alone does not identify a node. + * This IP is what actually selects the server, via CURLOPT_RESOLVE. + * + * @var string|null + */ + public $work_ip; + /** * Work url ttl * @var int @@ -256,9 +266,10 @@ private function compressData($data = null) public function httpRequest($msg) { $failed_urls = null; - // Using current server without changing it + // Using current server without changing it. + // work_ip pins the node the URL was resolved to last time, see selectModerateNode(). $result = ! empty($this->work_url) && $this->server_changed + 86400 > time() - ? $this->sendRequest($msg, $this->work_url, $this->server_timeout) + ? $this->sendRequest($msg, $this->work_url, $this->server_timeout, $this->work_ip) : false; // Changing server if no work_url or request has an error @@ -268,9 +279,14 @@ public function httpRequest($msg) while (($result === false || (is_object($result) && $result->errno != 0)) && $attempt <= $number_of_connection_attempts) { // Getting type of error $type_error = $this->getTypeError($result); - $ip_to_resolve = false; - $failed_urls = $this->work_url; + // The URL always carries the pool hostname, so the node that just failed + // is identified by its IP. Keep it out of the next rotation. + if ( ! empty($this->work_ip) && ! in_array($this->work_ip, $this->down_ips, true) ) { + $this->down_ips[] = $this->work_ip; + } + + $failed_urls = $this->describeWorkServer(); if ( ! empty($this->work_url) ) { $this->downServers[] = $this->work_url; } @@ -280,7 +296,7 @@ public function httpRequest($msg) // Exit if next sendRequest failed, because bypassing DNS is the last retry. $attempt = $attempt + 2; } else { - $this->rotateModerate(); + $ip_to_resolve = $this->rotateModerate(); //try change server again if next sendRequest failed $attempt = $attempt + 1; } @@ -292,7 +308,7 @@ public function httpRequest($msg) break; } - $failed_urls .= ', ' . $this->work_url; + $failed_urls .= ', ' . $this->describeWorkServer(); } /** @psalm-suppress PossiblyInvalidArgument */ $response = new CleantalkResponse($result, $failed_urls); @@ -313,51 +329,21 @@ public function httpRequest($msg) } /** - * * @todo Refactor / fix logic errors - */ - public function rotateModerate() - { - // Split server url to parts - preg_match("/^(https?:\/\/)([^\/:]+)(.*)/i", $this->server_url, $matches); - - $url_protocol = isset($matches[1]) ? $matches[1] : ''; - $url_host = isset($matches[2]) ? $matches[2] : ''; - $url_suffix = isset($matches[3]) ? $matches[3] : ''; - - $servers = $this->getServersIp($url_host); - - if ( ! $servers ) { - return; - } - - // Loop until find work server - foreach ( $servers as $server ) { - $dns = Helper::ipResolve($server['ip']); - if ( ! $dns ) { - continue; - } - - $this->work_url = $url_protocol . $dns . $url_suffix; - - // Do not checking previous down server - if ( ! empty($this->downServers) && in_array($this->work_url, $this->downServers) ) { - continue; - } - - $this->server_ttl = $server['ttl']; - $this->server_change = true; - break; - } - } - - /** - * @todo Refactor / fix logic errors - * @return string|false Selected IP address, or false if none was selected. + * Selects the next moderate node that has not failed yet in the current cycle. + * + * The URL always keeps the pool hostname from server_url. It is the only name + * known to match the API certificate, and deriving it from DNS (a PTR record) + * would let a poisoned resolver choose the very name TLS is validated against. + * The node is selected by IP instead and has to be pinned with CURLOPT_RESOLVE. + * + * getServersIp() returns the candidates sorted by ping, so the first usable + * entry is the closest responding node. + * + * @return string|false Selected node IP, false when no candidate is left. */ - public function rotateModerateAndUseIP() + private function selectModerateNode() { // Split server url to parts - global $apbct; preg_match("/^(https?:\/\/)([^\/:]+)(.*)/i", $this->server_url, $matches); $url_protocol = isset($matches[1]) ? $matches[1] : ''; @@ -370,37 +356,20 @@ public function rotateModerateAndUseIP() return false; } - $apbct->settings['wp__use_builtin_http_api'] = false; - // Loop until find work server foreach ( $servers as $server ) { $ip = TT::getArrayValueAsString($server, 'ip'); - if ( empty($ip) ) { - continue; - } - - // The pool hostname does not distinguish nodes, so dedup the fallback by IP. - if ( in_array($ip, $this->down_ips, true) ) { - continue; - } - - // A forward-confirmed PTR points to the exact node and is preferred. - // Without a PTR the pool hostname is still correct for SNI and the certificate, - // because the node is pinned by IP via CURLOPT_RESOLVE anyway. - $dns = $this->resolvePtr($ip); - $host = $dns ?: $url_host; - $work_url = $url_protocol . $host . $url_suffix; - - // Do not check the previous down server. Only a node-specific hostname - // identifies a server, the pool hostname is shared by all of them. - if ( $dns && ! empty($this->downServers) && in_array($work_url, $this->downServers, true) ) { + // The pool hostname is shared by every node, so only the IP identifies one. + // Skipping by URL here would discard all of the candidates at once. + if ( empty($ip) || in_array($ip, $this->down_ips, true) ) { continue; } - $this->work_url = $work_url; + $this->work_url = $url_protocol . $url_host . $url_suffix; + $this->work_ip = $ip; $this->down_ips[] = $ip; - $this->server_ttl = $server['ttl']; + $this->server_ttl = TT::getArrayValueAsInt($server, 'ttl'); $this->server_change = true; return $ip; @@ -410,15 +379,45 @@ public function rotateModerateAndUseIP() } /** - * Seam over the forward-confirmed reverse DNS lookup, overridable in tests. + * Renders the current server for connection reports. * - * @param string $ip + * Every node now shares the pool hostname, so the URL alone no longer tells + * which server was contacted. The pinned IP is appended to keep the report + * able to answer that. * - * @return string|false PTR hostname that resolves back to $ip, false otherwise. + * @return string */ - protected function resolvePtr($ip) + private function describeWorkServer() { - return Helper::ipResolve($ip); + if ( empty($this->work_url) ) { + return ''; + } + + return empty($this->work_ip) + ? $this->work_url + : $this->work_url . ' (' . $this->work_ip . ')'; + } + + /** + * Rotates to the closest responding moderate node. + * + * @return string|false Selected node IP, false when no candidate is left. + */ + public function rotateModerate() + { + return $this->selectModerateNode(); + } + + /** + * Rotates to the closest responding moderate node when DNS for the pool + * hostname is unusable. Identical to rotateModerate(), kept as a separate + * entry point because the caller treats this as the last retry. + * + * @return string|false Selected node IP, false when no candidate is left. + */ + public function rotateModerateAndUseIP() + { + return $this->selectModerateNode(); } /** @@ -594,19 +593,27 @@ private function sendRequest($data, $url, $server_timeout = 3, $ip_to_resolve = : $url; $options = array('timeout' => $server_timeout); - if ($ip_to_resolve) { - $connection_resolve_string = $this->maybeResolveIPInsteadOfHost($url, $ip_to_resolve); - if ($connection_resolve_string && defined('CURLOPT_RESOLVE')) { - $options[CURLOPT_RESOLVE] = array($connection_resolve_string); - } + $connection_resolve_string = $ip_to_resolve + ? $this->maybeResolveIPInsteadOfHost($url, $ip_to_resolve) + : false; + + if ($connection_resolve_string && defined('CURLOPT_RESOLVE')) { + $options[CURLOPT_RESOLVE] = array($connection_resolve_string); } + // CURLOPT_RESOLVE is a cURL option, the WordPress HTTP API silently drops it. + // Without it the hostname would be resolved by DNS again and the node + // selection would be lost, so force the cURL transport for pinned requests. + $http_api_state = $this->disableBuiltInHttpApi(isset($options[CURLOPT_RESOLVE])); + $result = $http->setUrl($url) ->setData($data) ->setPresets($presets) ->setOptions($options) ->request(); + $this->restoreBuiltInHttpApi($http_api_state); + $errstr = null; $response = is_string($result) ? json_decode($result) : false; if ( $result !== false && is_object($response) ) { @@ -655,6 +662,43 @@ public function maybeResolveIPInsteadOfHost(string $url, string $ip_to_resolve) return false; } + /** + * Forces the cURL transport when a request has to be pinned to an IP. + * + * @param bool $needed + * + * @return bool|null Previous setting value, null when nothing was changed. + */ + private function disableBuiltInHttpApi($needed) + { + global $apbct; + + if ( ! $needed || ! isset($apbct->settings['wp__use_builtin_http_api']) ) { + return null; + } + + $previous = $apbct->settings['wp__use_builtin_http_api']; + $apbct->settings['wp__use_builtin_http_api'] = false; + + return $previous; + } + + /** + * @param bool|null $previous Value returned by disableBuiltInHttpApi(). + * + * @return void + */ + private function restoreBuiltInHttpApi($previous) + { + global $apbct; + + if ( $previous === null || ! isset($apbct->settings) ) { + return; + } + + $apbct->settings['wp__use_builtin_http_api'] = $previous; + } + /** * Call check_bot API method * diff --git a/lib/Cleantalk/Antispam/IntegrationsByClass/Woocommerce.php b/lib/Cleantalk/Antispam/IntegrationsByClass/Woocommerce.php index ab1892eb0..0450d09c3 100644 --- a/lib/Cleantalk/Antispam/IntegrationsByClass/Woocommerce.php +++ b/lib/Cleantalk/Antispam/IntegrationsByClass/Woocommerce.php @@ -722,6 +722,7 @@ public function ordersSendFeedback(array $spam_ids, $orders_status = '0') $config = ct_get_server(); $ct->server_url = APBCT_MODERATE_URL; $ct->work_url = isset($config['ct_work_url']) && preg_match('/http:\/\/.+/', $config['ct_work_url']) ? $config['ct_work_url'] : null; + $ct->work_ip = isset($config['ct_work_ip']) ? $config['ct_work_ip'] : null; $ct->server_ttl = isset($config['ct_server_ttl']) ? $config['ct_server_ttl'] : null; $ct->server_changed = isset($config['ct_server_changed']) ? $config['ct_server_changed'] : null; diff --git a/lib/Cleantalk/ApbctWP/Antispam/ForceProtection.php b/lib/Cleantalk/ApbctWP/Antispam/ForceProtection.php index 3ec9ab70b..f034c3c2f 100644 --- a/lib/Cleantalk/ApbctWP/Antispam/ForceProtection.php +++ b/lib/Cleantalk/ApbctWP/Antispam/ForceProtection.php @@ -88,6 +88,7 @@ public function checkBot() $config = ct_get_server(); $ct->server_url = APBCT_MODERATE_URL; $ct->work_url = isset($config['ct_work_url']) && preg_match('/https:\/\/.+/', $config['ct_work_url']) ? $config['ct_work_url'] : null; + $ct->work_ip = isset($config['ct_work_ip']) ? $config['ct_work_ip'] : null; $ct->server_ttl = isset($config['ct_server_ttl']) ? $config['ct_server_ttl'] : null; $ct->server_changed = isset($config['ct_server_changed']) ? $config['ct_server_changed'] : null; $api_response = $ct->checkBot($ct_request); diff --git a/lib/Cleantalk/ApbctWP/ContactsEncoder/ContactsEncoder.php b/lib/Cleantalk/ApbctWP/ContactsEncoder/ContactsEncoder.php index 3cceeda4a..c02956380 100644 --- a/lib/Cleantalk/ApbctWP/ContactsEncoder/ContactsEncoder.php +++ b/lib/Cleantalk/ApbctWP/ContactsEncoder/ContactsEncoder.php @@ -384,6 +384,7 @@ protected function checkRequest() $ct->work_url = preg_match('/https:\/\/.+/', $config_work_url) ? $config_work_url : ''; + $ct->work_ip = TT::getArrayValueAsString($config, 'ct_work_ip'); $ct->server_ttl = TT::getArrayValueAsInt($config, 'ct_server_ttl'); $ct->server_changed = TT::getArrayValueAsInt($config, 'ct_server_changed'); $api_response = $ct->checkBot($ct_request); diff --git a/tests/Antispam/TestCleantalkResolveIP.php b/tests/Antispam/TestCleantalkResolveIP.php index a58394a92..c10260d50 100644 --- a/tests/Antispam/TestCleantalkResolveIP.php +++ b/tests/Antispam/TestCleantalkResolveIP.php @@ -176,34 +176,50 @@ public function testResolveMappingKeepsTlsVerificationEnabled() * @param array $servers * @param array $ptr_map * @param array $down_servers + * @param array $down_ips * * @return ApbctRotateModerateStub */ - private function makeRotateStub(array $servers, array $ptr_map = array(), array $down_servers = array()) - { + private function makeRotateStub( + array $servers, + array $ptr_map = array(), + array $down_servers = array(), + array $down_ips = array() + ) { $ct = new ApbctRotateModerateStub(); $ct->server_url = self::POOL_URL; $ct->servers_fixture = $servers; $ct->ptr_map = $ptr_map; if ($down_servers) { - $property = new \ReflectionProperty(Cleantalk::class, 'downServers'); - $property->setAccessible(true); - $property->setValue($ct, $down_servers); + $this->setPrivate($ct, 'downServers', $down_servers); + } + + if ($down_ips) { + $this->setPrivate($ct, 'down_ips', $down_ips); } return $ct; } + private function setPrivate($object, $name, $value) + { + $property = new \ReflectionProperty(Cleantalk::class, $name); + $property->setAccessible(true); + $property->setValue($object, $value); + } + private function server($ip, $ttl = 300) { return array('ip' => $ip, 'host' => self::POOL_HOST, 'ttl' => $ttl); } /** - * A node with a forward-confirmed PTR keeps that node-specific hostname in the URL. + * The URL must never be derived from DNS: a poisoned resolver would then pick + * the very name TLS is validated against. The pool hostname is the only name + * known to match the API certificate. */ - public function testPtrHostnameIsUsedWhenAvailable() + public function testUrlKeepsPoolHostnameEvenWhenPtrIsAvailable() { $ct = $this->makeRotateStub( array($this->server(self::IP)), @@ -211,7 +227,8 @@ public function testPtrHostnameIsUsedWhenAvailable() ); $this->assertSame(self::IP, $ct->rotateModerateAndUseIP()); - $this->assertSame('https://' . self::HOST, $ct->work_url); + $this->assertSame(self::POOL_URL, $ct->work_url); + $this->assertSame(self::IP, $ct->work_ip); } /** @@ -242,18 +259,20 @@ public function testFirstIpIsTakenEvenWhenOnlyLaterNodesHavePtr() } /** - * A node that already failed under its own hostname must not be picked again. + * A node that already failed must not be picked again. It is identified by IP, + * because every node now shares the pool hostname. */ - public function testNodeSpecificHostnameThatAlreadyFailedIsSkipped() + public function testNodeThatAlreadyFailedIsSkipped() { $ct = $this->makeRotateStub( array($this->server(self::IP), $this->server(self::IP_SECOND)), - array(self::IP => self::HOST, self::IP_SECOND => self::HOST_SECOND), - array('https://' . self::HOST) + array(), + array(), + array(self::IP) ); $this->assertSame(self::IP_SECOND, $ct->rotateModerateAndUseIP()); - $this->assertSame('https://' . self::HOST_SECOND, $ct->work_url); + $this->assertSame(self::POOL_URL, $ct->work_url); } /** @@ -261,7 +280,7 @@ public function testNodeSpecificHostnameThatAlreadyFailedIsSkipped() * the list of failed URLs must not discard the candidates. Deduplicating the * fallback by URL instead of by IP would return false here and kill the retry. */ - public function testFailedPoolUrlDoesNotDiscardCandidatesWithoutPtr() + public function testFailedPoolUrlDoesNotDiscardCandidates() { $ct = $this->makeRotateStub( array($this->server(self::IP), $this->server(self::IP_SECOND)), @@ -307,8 +326,8 @@ public function testNoServersMeansNoFallback() } /** - * End to end of the two halves: whatever hostname the rotation settled on, the - * mapping must pin exactly that hostname to the returned IP. + * End to end of the two halves: the rotation always settles on the pool + * hostname, and the mapping must pin exactly that hostname to the returned IP. */ public function testSelectedIpAndWorkUrlProduceConsistentMapping() { @@ -316,11 +335,9 @@ public function testSelectedIpAndWorkUrlProduceConsistentMapping() $ct = $this->makeRotateStub(array($this->server(self::IP)), $ptr_map); $ip = $ct->rotateModerateAndUseIP(); - $expected_host = $ptr_map ? self::HOST : self::POOL_HOST; - $this->assertSame(self::IP, $ip); $this->assertSame( - $expected_host . ':443:' . self::IP, + self::POOL_HOST . ':443:' . self::IP, $ct->maybeResolveIPInsteadOfHost($ct->work_url, $ip) ); }