From dee6e8fb628e3688c42cfb351086c5f0270a870c Mon Sep 17 00:00:00 2001 From: camer0n Date: Sun, 4 May 2025 12:08:42 -0700 Subject: [PATCH] Issue #5487 Ban caching and ban query fixes. Unit tests added. --- e107_admin/admin_log.php | 2 +- e107_handlers/admin_log_class.php | 5 +- e107_handlers/iphandler_class.php | 293 +++++++++++------- .../English/admin/lan_admin_log.php | 2 +- e107_tests/tests/unit/eIPHandlerTest.php | 202 +++++++++++- 5 files changed, 385 insertions(+), 119 deletions(-) diff --git a/e107_admin/admin_log.php b/e107_admin/admin_log.php index d80f3b3fd..6bfeb80a1 100644 --- a/e107_admin/admin_log.php +++ b/e107_admin/admin_log.php @@ -247,7 +247,7 @@ class admin_log_ui extends e_admin_ui $perPage = e107::getConfig()->get('sys_log_perpage'); $this->perPage = vartrue($perPage,10); - $this->prefs['sys_log_perpage']['writeParms'] = array(10=>10, 15=>15, 20=>20, 30=>30, 40=>40, 50=>50); + $this->prefs['sys_log_perpage']['writeParms'] = array(10=>10, 15=>15, 20=>20, 30=>30, 40=>40, 50=>50, 75=>75, 100=>100, 250=>250 ); $this->eventTypes = loadEventTypes('admin_log'); diff --git a/e107_handlers/admin_log_class.php b/e107_handlers/admin_log_class.php index 1e9f44f42..8d3a3c5cb 100644 --- a/e107_handlers/admin_log_class.php +++ b/e107_handlers/admin_log_class.php @@ -364,7 +364,10 @@ class e_admin_log //--------------------------------------- if(($target_logs & LOG_TO_ROLLING) && $this->_roll_log_active) { // Rolling log - + if(getperms('0') && e_REQUEST_HTTP === '/e107_admin/admin_log.php') // Don't log while looking at the log. + { + return; + } // Process source_call info //--------------------------------------- if(is_numeric($source_call) && ($source_call >= 0)) diff --git a/e107_handlers/iphandler_class.php b/e107_handlers/iphandler_class.php index cf3fe66b9..c8f13344e 100644 --- a/e107_handlers/iphandler_class.php +++ b/e107_handlers/iphandler_class.php @@ -764,7 +764,6 @@ class eIPHandler /** * Generate DB query for domain name-related checks * - * If an email address is passed, discards the individual's name * * @param string $email - an email address or domain name string * @param string $fieldName @@ -774,31 +773,51 @@ class eIPHandler * @internal param string $fieldname - if non-empty, each array entry is a comparison with this field * */ - function makeDomainQuery($email, $fieldName = 'banlist_ip') + /** + * Generate DB query for network-level domain bans + * + * @param string $domain - a domain name string (e.g., mydomain.co.uk) + * @param string $fieldName - if non-empty, each array entry is a comparison with this field + * @return array|bool - array of network ban patterns, or false if invalid domain + */ + function makeDomainQuery($domain, $fieldName = 'banlist_ip'): array|bool { - $tp = e107::getParser(); - if (($tv = strrpos('@', $email)) !== FALSE) + + $domain = trim($domain); + + if(strpos($domain, '@') !== false) // this function not intended for email addresses. { - $email = substr($email, $tv+1); + return false; } - $tmp = strtolower($tp -> toDB(trim($email))); - if ($tmp == '') return FALSE; - if (strpos($tmp,'.') === FALSE) return FALSE; - $em = array_reverse(explode('.',$tmp)); + + if($domain === '' || strpos($domain, '.') === false) + { + return false; + } + + + $sanitized_domain = strtolower($domain); + + + $parts = array_reverse(explode('.', $sanitized_domain)); + $line = ''; - $out = array('*@'.$tmp); // First element looks for domain as email address - foreach ($em as $e) + $out = []; + + foreach($parts as $part) { - $line = '.'.$e.$line; - $out[] = '*'.$line; + $line = '.' . $part . $line; + $out[] = '*' . $line; } - if ($fieldName) + + if($fieldName) { - foreach ($out as $k => $v) + foreach($out as $k => $v) { - $out[$k] = '(`'.$fieldName."`='".$v."')"; + $out[$k] = "(`$fieldName`='$v')"; } } + return $out; } @@ -809,26 +828,45 @@ class eIPHandler * @param string $email - email address to process * @param string $fieldname - name of field being searched in DB * - * @return bool|string false if invalid address. Otherwise returns a set of values to check + * @return bool|string|array false if invalid address. Otherwise returns a set of values to check * (Moved in from user_handler.php) */ public function makeEmailQuery($email, $fieldname = 'banlist_ip') { - $tp = e107::getParser(); - $tmp = strtolower($tp -> toDB(trim(substr($email, strrpos($email, "@")+1)))); // Pull out the domain name - if ($tmp == '') return FALSE; - if (strpos($tmp,'.') === FALSE) return FALSE; - $em = array_reverse(explode('.',$tmp)); - $line = ''; - $out = array($fieldname."='*@{$tmp}'"); // First element looks for domain as email address - foreach ($em as $e) - { - $line = '.'.$e.$line; - $out[] = '`'.$fieldname."`='*{$line}'"; - } - return implode(' OR ',$out); - } + $email = trim($email); + if(!filter_var($email, FILTER_VALIDATE_EMAIL)) + { + return $fieldname ? false : []; + } + + $sanitized_email = addslashes($email); + + $domain = strtolower(substr($email, strrpos($email, '@') + 1)); + + if($domain === '' || strpos($domain, '.') === false) + { + return $fieldname ? false : []; + } + + $out = [ + "$sanitized_email", // Specific email (e.g., user@mydomain.co.uk) + "*@$domain" // Domain ban (e.g., *@mydomain.co.uk) + ]; + + if($fieldname) + { + + $out = [ + "`$fieldname`='$sanitized_email'", + "`$fieldname`='*@$domain'" + ]; + + return implode(' OR ', $out); + } + + return $out; + } /** @@ -876,34 +914,26 @@ class eIPHandler if ($ip !== e107::LOCALHOST_IP && ($ip !== e107::LOCALHOST_IP2) && ($ip !== $this->serverIP)) // Check host name, user email to see if banned { - $vals = array(); + $vals = []; + if (e107::getPref('enable_rdns')) { $vals = array_merge($vals, $this->makeDomainQuery($this->get_host_name($ip), '')); } - if ((defined('USEREMAIL') && USEREMAIL)) + if(deftrue('USEREMAIL')) { - // @todo is there point to this? Usually avoid a complete query if we skip it - $vals = array_merge($vals, $this->makeDomainQuery(USEREMAIL, '')); + $vals = array_merge($vals, $this->makeEmailQuery(USEREMAIL, '')); } if (count($vals)) { $vals = array_unique($vals); // Could get identical values from domain name check and email check - if($this->debug) - { - print_a($vals); - } - - $match = "`banlist_ip`='".implode("' OR `banlist_ip`='", $vals)."'"; + $this->checkBan($match); } } - elseif($this->debug) - { - print_a("IP is LocalHost - skipping ban-check"); - } + } } @@ -913,7 +943,6 @@ class eIPHandler * Check the banlist table. $query is used to determine the match. * If $do_return, will always return with ban status - TRUE for OK, FALSE for banned. * If return permitted, will never display a message for a banned user; otherwise will display any message then exit - * @todo consider whether can be simplified * * @param string $query - the 'WHERE' part of the DB query to be executed * @param boolean $show_error - if true, adds a '403 Forbidden' header for a banned user @@ -922,101 +951,131 @@ class eIPHandler */ public function checkBan($query, $show_error = true, $do_return = false) { + $call_id = uniqid(); + $session_key = 'ban_check_' . md5($query); + $session = e107::getSession('eIPHandler'); + + // Long-term cache (360 seconds) + if($session->has($session_key)) + { + $cached = $session->get($session_key); + if(isset($cached['timestamp']) && (time() - $cached['timestamp'] <= 360)) + { + return $cached['result']; + } + } + + // Short-term in-memory cache (1 second) using registry + $cache_key = md5($query); + $current_time = microtime(true); + $recent_checks = e107::getRegistry('core/eIPHandler/checkBan', []); + + if(isset($recent_checks[$cache_key]) && ($current_time - $recent_checks[$cache_key]['time'] <= 1)) + { + // e107::getLog()->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, "DBG", "Using registry cache (Call: $call_id)", "Result: " . ($recent_checks[$cache_key]['result'] ? 'true' : 'false'), false, LOG_TO_ROLLING); + + return $recent_checks[$cache_key]['result']; + } + $sql = e107::getDb(); $pref = e107::getPref(); $tp = e107::getParser(); $log = e107::getLog(); - $log->addEvent(4,__FILE__."|".__FUNCTION__."@".__LINE__,"DBG","Check for Ban",$query,FALSE,LOG_TO_ROLLING); + $caller = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2)[1]; - if ($sql->select('banlist', '*', $query.' ORDER BY `banlist_bantype` DESC')) + $callerFunction = $caller['function']."() in ". basename($caller['file']);; + $log->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, "DBG", "Check for Ban ", $query."\nCall: $call_id\nCaller: $callerFunction", false, LOG_TO_ROLLING); + + $full_query = "SELECT * FROM banlist WHERE $query ORDER BY `banlist_bantype` DESC"; + + $result = true; + $sqlResult = $sql->select('banlist', '*', $query . ' ORDER BY `banlist_bantype` DESC'); + // $log->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, "DBG", "SQL Select Result (Call: $call_id)", "Query: $query, Result: " . ($sqlResult !== false ? 'Found' : 'Not Found'), false, LOG_TO_ROLLING); + + if($sqlResult) { - // Any whitelist entries will be first, because they are positive numbers - so we can answer based on the first DB record read $row = $sql->fetch(); if($row['banlist_bantype'] >= eIPHandler::BAN_TYPE_WHITELIST) { - $log->addEvent(4,__FILE__."|".__FUNCTION__."@".__LINE__,"DBG","Whitelist hit",$query,FALSE,LOG_TO_ROLLING); - return true; // Whitelisted entry - } - // Found banlist entry in table here - if(($row['banlist_banexpires'] > 0) && ($row['banlist_banexpires'] < time())) // Ban has expired - delete from DB + $log->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, "DBG", "Whitelist hit\nCall: $call_id", $query, false, LOG_TO_ROLLING); + + } + elseif(($row['banlist_banexpires'] > 0) && ($row['banlist_banexpires'] < time())) { $sql->delete('banlist', $query); - - $log->addEvent(4,__FILE__."|".__FUNCTION__."@".__LINE__,"DBG","Ban Expired",$row['banlist_ip'],FALSE,LOG_TO_ROLLING); + $log->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, "DBG", "Ban Expired ", $row['banlist_ip']."\nCall: $call_id", false, LOG_TO_ROLLING); $this->regenerateFiles(); - - return true; } - - // User is banned hereafter - just need to sort out the details. - // May need to retrigger ban period - if (!empty($pref['ban_retrigger']) && !empty($pref['ban_durations'][$row['banlist_bantype']])) + else { - $dur = (int) $pref['ban_durations'][$row['banlist_bantype']]; - - $updateQry = array( - 'banlist_banexpires' => (time() + ($dur * 60 * 60)), - 'WHERE' => "banlist_ip ='".$row['banlist_ip']."'" - ); - - $sql->update('banlist', $updateQry); - $this->regenerateFiles(); - - $log->addEvent(4,__FILE__."|".__FUNCTION__."@".__LINE__,"DBG","Retrigger Ban",$row['banlist_ip'],FALSE,LOG_TO_ROLLING); - } - - $log->addEvent(4,__FILE__."|".__FUNCTION__."@".__LINE__,"DBG","Active Ban",$query,FALSE,LOG_TO_ROLLING); - - if ($show_error) - { - header('HTTP/1.1 403 Forbidden', true); - } - - // May want to display a message - if (!empty($pref['ban_messages'])) - { - - if($do_return) // Ban still current here + if(!empty($pref['ban_retrigger']) && !empty($pref['ban_durations'][$row['banlist_bantype']])) { - return false; + $dur = (int) $pref['ban_durations'][$row['banlist_bantype']]; + $updateQry = array( + 'banlist_banexpires' => (time() + ($dur * 60 * 60)), + 'WHERE' => "banlist_ip ='" . $row['banlist_ip'] . "'" + ); + $sql->update('banlist', $updateQry); + $this->regenerateFiles(); + + $log->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, "DBG", "Retrigger Ban ", $row['banlist_ip']."\nCall: $call_id", false, LOG_TO_ROLLING); + } - echo $tp->toHTML(varset($pref['ban_messages'][$row['banlist_bantype']])); // Show message if one set + $log->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, "DBG", "Active Ban ", $query."\nCall: $call_id", false, LOG_TO_ROLLING); + + if($show_error) + { + header('HTTP/1.1 403 Forbidden', true); + } + if(!empty($pref['ban_messages'])) + { + if($do_return) + { + $result = false; + } + else + { + echo $tp->toHTML(varset($pref['ban_messages'][$row['banlist_bantype']])); + } + } + + $log->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, 'BAN_03', 'LAN_AUDIT_LOG_003', $query, false, LOG_TO_ROLLING); + + if($do_return) + { + $result = false; + } + else + { + exit(); + } } - - $log->addEvent(4, __FILE__."|".__FUNCTION__."@".__LINE__, 'BAN_03', 'LAN_AUDIT_LOG_003', $query, FALSE, LOG_TO_ROLLING); - - if($this->debug) - { - echo "
query: ".$query;
-				echo "\nBanned
"; - } - - // added missing if clause - if ($do_return) - { - return false; - } - - exit(); } - if($this->debug) - { - echo "query: ".$query; - echo "
Not Banned "; - } + $log->addEvent(4, __FILE__ . "|" . __FUNCTION__ . "@" . __LINE__, "DBG", "No ban found ", $query."\nCall: $call_id\nCaller: $callerFunction", false, LOG_TO_ROLLING); + // Store in short-term in-memory cache (1 second) using registry + $recent_checks[$cache_key] = [ + 'result' => $result, + 'time' => $current_time + ]; - $log->addEvent(4,__FILE__."|".__FUNCTION__."@".__LINE__,"DBG","No ban found",$query,FALSE,LOG_TO_ROLLING); - return true; // Email address OK + e107::setRegistry('core/eIPHandler/checkBan', $recent_checks); + + // Store in long-term session cache (10 seconds) + $session->set($session_key, [ + 'result' => $result, + 'timestamp' => time() + ]); + + return $result; } - /** * Add an entry to the banlist. $bantype = 1 for manual, 2 for flooding, 4 for multiple logins * Returns TRUE if ban accepted. @@ -1086,7 +1145,7 @@ class eIPHandler $ban_message .= 'Host: '.$this->get_host_name($ban_ip); } // Add using an array - handles DB changes better - $sql->insert('banlist', + if(!$sql->insert('banlist', array( 'banlist_id' => 0, 'banlist_ip' => $ban_ip , @@ -1096,7 +1155,11 @@ class eIPHandler 'banlist_admin' => $ban_user , 'banlist_reason' => $ban_message , 'banlist_notes' => $ban_notes - )); + ))) + { + trigger_error("Error adding ban to banlist table", E_USER_WARNING);; + // dbg("Error adding ban to banlist table"); + } $this->regenerateFiles(); return TRUE; diff --git a/e107_languages/English/admin/lan_admin_log.php b/e107_languages/English/admin/lan_admin_log.php index 3ac93ce19..3eb5412df 100644 --- a/e107_languages/English/admin/lan_admin_log.php +++ b/e107_languages/English/admin/lan_admin_log.php @@ -29,7 +29,7 @@ return [ 'RL_LAN_028' => "Update Filters", 'RL_LAN_029' => "Event type filter", 'RL_LAN_030' => "Admin Log", - 'RL_LAN_031' => "Actions to log", + 'RL_LAN_031' => "User audit actions to log", 'RL_LAN_032' => "Pri", 'RL_LAN_033' => "Further Information", 'RL_LAN_044' => "Log events to display per page", diff --git a/e107_tests/tests/unit/eIPHandlerTest.php b/e107_tests/tests/unit/eIPHandlerTest.php index 6dfd5a515..9d752e3a5 100644 --- a/e107_tests/tests/unit/eIPHandlerTest.php +++ b/e107_tests/tests/unit/eIPHandlerTest.php @@ -21,6 +21,7 @@ class eIPHandlerTest extends \Codeception\Test\Unit try { $this->ip = $this->make('eIPHandler'); + $this->ip->regenerateFiles(); } catch(Exception $e) { @@ -28,6 +29,16 @@ class eIPHandlerTest extends \Codeception\Test\Unit } } + + protected function _after() + { + e107::setRegistry('core/eIPHandler/checkBan', null); + e107::getSession('eIPHandler')->clearData(); + $this->ip->regenerateFiles(); + + } + + /** * Test IPHandler::ipDecode() */ @@ -101,7 +112,7 @@ class eIPHandlerTest extends \Codeception\Test\Unit '-8' => 0 // unknown ); - e107::getConfig()->set('ban_durations', $banDurations)->save(false, true, false); + // e107::getConfig()->set('ban_durations', $banDurations)->save(false, true, false); $result = $this->ip->add_ban(2, "unit test generated ban", '123.123.123.123'); $this::assertTrue($result); @@ -237,5 +248,194 @@ class eIPHandlerTest extends \Codeception\Test\Unit } + public function testMakeEmailQuery() + { + + $email = 'cameron@mydomain.co.uk'; + + // Test with empty $fieldname + $result = $this->ip->makeEmailQuery($email, ''); + $expected = ['cameron@mydomain.co.uk', '*@mydomain.co.uk']; + $this::assertSame($expected, $result); + + + // Test with default $fieldname + $result = $this->ip->makeEmailQuery($email); + $expected = "`banlist_ip`='cameron@mydomain.co.uk' OR `banlist_ip`='*@mydomain.co.uk'"; + $this::assertSame($expected, $result); + + + // Test invalid email + $result = $this->ip->makeEmailQuery('invalid_email', ''); + $expected = []; + $this::assertSame($expected, $result); + + } + + + public function testMakeDomainQuery() + { + + // Test valid domain + $domain = 'mydomain.co.uk'; + $result = $this->ip->makeDomainQuery($domain, ''); + $expected = ['*.uk', '*.co.uk', '*.mydomain.co.uk']; + $this::assertSame($expected, $result); + + + // Test email address + $result = $this->ip->makeDomainQuery('user@mydomain.co.uk', ''); + $expected = false; + $this::assertSame($expected, $result); + + + // Test invalid domain + $result = $this->ip->makeDomainQuery('invalid#domain', ''); + $expected = false; + $this::assertSame($expected, $result); + + + // Test with fieldName + $result = $this->ip->makeDomainQuery('mydomain.co.uk', 'banlist_ip'); + $expected = [ + "(`banlist_ip`='*.uk')", + "(`banlist_ip`='*.co.uk')", + "(`banlist_ip`='*.mydomain.co.uk')" + ]; + + $this::assertSame($expected, $result); + + } + + + // ---- + + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + * @return void + */ + public function testCheckBanNoBan() + { + + $query = "`banlist_ip`='cameron@mydomain.co.uk' OR `banlist_ip`='*@mydomain.co.uk'"; + + // Ensure no ban exists + e107::getDb()->delete('banlist', "`banlist_ip` IN ('cameron@mydomain.co.uk', '*@mydomain.co.uk')"); + $this->ip->regenerateFiles(); + + // Clear session cache + e107::getSession('eIPHandler')->clearData(); + + // Test: no ban + $result = $this->ip->checkBan($query, true, true); + $this::assertTrue($result); + + // Verify session cache is set + $cached = e107::getSession('eIPHandler')->get('ban_check_' . md5($query)); + $this::assertIsArray($cached); + $this::assertTrue($cached['result']); + $this::assertArrayHasKey('timestamp', $cached); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + * @return void + */ + public function testCheckBanActiveBan() + { + + $this->ip->add_ban(1, '', 'cameron@mydomain.co.uk'); + + // Test: active ban + $query = "`banlist_ip`='cameron@mydomain.co.uk' OR `banlist_ip`='*@mydomain.co.uk'"; + $result = $this->ip->checkBan($query, true, true); + $this::assertFalse($result); // ie. banned. + + // Verify session cache is set + $cached = e107::getSession('eIPHandler')->get('ban_check_' . md5($query)); + $this::assertIsArray($cached); + $this::assertFalse($cached['result']); + $this::assertArrayHasKey('timestamp', $cached); + + + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + * @return void + */ + public function testCheckBanThrottling() + { + + $query = "`banlist_ip`='cameron@mydomain.co.uk' OR `banlist_ip`='*@mydomain.co.uk'"; + + // Ensure no ban exists + e107::getDb()->delete('banlist', "`banlist_ip` IN ('cameron@mydomain.co.uk', '*@mydomain.co.uk')"); + + + // Test: multiple calls within 1 second + $startTime = microtime(true); + $result1 = $this->ip->checkBan($query, true, true); + $result2 = $this->ip->checkBan($query, true, true); + $endTime = microtime(true); + + $this::assertTrue($result1); + $this::assertTrue($result2); + $this::assertLessThan(1, $endTime - $startTime, "Throttling test took too long, in-memory cache may not be working"); + + // Verify session cache is set + $cached = e107::getSession('eIPHandler')->get('ban_check_' . md5($query)); + $this::assertIsArray($cached); + $this::assertTrue($cached['result']); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + * @return void + */ + public function testCheckBanCacheExpiration() + { + + $query = "`banlist_ip`='cameron@mydomain.co.uk' OR `banlist_ip`='*@mydomain.co.uk'"; + + // Ensure no ban exists + e107::getDb()->delete('banlist', "`banlist_ip` IN ('cameron@mydomain.co.uk', '*@mydomain.co.uk')"); + $this->ip->regenerateFiles(); + + // Set a cached result (no ban) + e107::getSession('eIPHandler')->set('ban_check_' . md5($query), [ + 'result' => true, + 'timestamp' => time() - 5 // Within 10 seconds + ]); + + // Test: cached result within 10 seconds + $result = $this->ip->checkBan($query, true, true); + $this::assertTrue($result); + + // Simulate cache expiration (11 seconds) + e107::getSession('eIPHandler')->set('ban_check_' . md5($query), [ + 'result' => true, + 'timestamp' => time() - 11 + ]); + + // Insert active ban + e107::getDb()->insert('banlist', [ + 'banlist_ip' => 'cameron@mydomain.co.uk', + 'banlist_bantype' => 1, + 'banlist_banexpires' => 0 + ]); + + // Test: new ban after expiration + $result = $this->ip->checkBan($query, true, true); + $this::assertFalse($result); + + // Cleanup + e107::getDb()->delete('banlist', "`banlist_ip`='cameron@mydomain.co.uk'"); + } }