From 8b4c29cf0820d1037675c6f3232ea90c2204bf12 Mon Sep 17 00:00:00 2001 From: camer0n Date: Fri, 2 May 2025 05:36:23 -0700 Subject: [PATCH] Issue #5487 IP handler tests and tweaks while awaiting more data about banned user. isAddressRoutable() now uses PHP methods. --- e107_handlers/iphandler_class.php | 219 +++++++-------- e107_tests/tests/unit/eIPHandlerTest.php | 340 ++++++++++++++--------- 2 files changed, 309 insertions(+), 250 deletions(-) diff --git a/e107_handlers/iphandler_class.php b/e107_handlers/iphandler_class.php index c0b2785be..14eed9550 100644 --- a/e107_handlers/iphandler_class.php +++ b/e107_handlers/iphandler_class.php @@ -28,7 +28,7 @@ /** * Class to handle ban-related checks, and provide some utility functions related to IP addresses * There are two parts to the class: - * + * * Part 1 * ------ * This part intentionally does NO database access, and requires an absolute minimum of file paths to be set up @@ -153,7 +153,7 @@ class eIPHandler * * On load it gets the user's IP address, and checks it against whitelist and blacklist files * If the address is blacklisted, displays an appropriate message (as configured) and aborts - * Otherwise sets up + * Otherwise sets up */ public function __construct($configDir = '') { @@ -168,25 +168,20 @@ class eIPHandler $this->ourConfigDir = e_SYSTEM.eIPHandler::BAN_FILE_DIRECTORY; } - $this->ourIP = $this->ipEncode($this->getCurrentIP()); - $this->serverIP = $this->ipEncode(isset($_SERVER['SERVER_ADDR']) ? $_SERVER['SERVER_ADDR'] : 'x.x.x.x'); + $this->serverIP = $this->ipEncode($_SERVER['SERVER_ADDR'] ?? 'x.x.x.x'); $this->makeUserToken(); + $ipStatus = $this->checkIP($this->ourIP); - if ($ipStatus != 0) + + if ($ipStatus < 0) // Blacklisted { - if ($ipStatus < 0) - { // Blacklisted - $this->logBanItem($ipStatus, 'result --> '.$ipStatus); // only log blacklist - $this->banAction($ipStatus); // This will abort if appropriate - } - //elseif ($ipStatus > 0) - // { // Whitelisted - we may want to set a specific indicator - // } + $this->logBanItem($ipStatus, 'result --> '.$ipStatus); // only log blacklist + $this->banAction($ipStatus); // This will abort if appropriate } - // Continue here - user not banned (so far) + } /** @@ -196,7 +191,6 @@ class eIPHandler public function setIP($ip) { $this->ourIP = $this->ipEncode($ip); - } @@ -210,8 +204,6 @@ class eIPHandler } - - /** * Add an entry to the banlist log file (which is a simple text file) * A date/time string is prepended to the line @@ -231,7 +223,6 @@ class eIPHandler } } - /** * Generate relatively unique user token from browser info @@ -269,95 +260,77 @@ class eIPHandler } - /** - * Check whether an IP address is routable + * Check whether an IP address is routable * - * @param string $ip - IPV4 or IPV6 numeric address. - * - * @return boolean TRUE if routable, FALSE if not - - @todo handle IPV6 fully + * @param string $ip IPV4 or IPV6 numeric address. + * @return bool TRUE if routable, FALSE if not */ public function isAddressRoutable($ip) { - $ignore = array( - '0\..*' , '^127\..*' , // Local loopbacks - '192\.168\..*' , // RFC1918 - Private Network - '172\.(?:1[6789]|2\d|3[01])\..*' , // RFC1918 - Private network - '10\..*' , // RFC1918 - Private Network - '169\.254\..*' , // RFC3330 - Link-local, auto-DHCP - '2(?:2[456789]|[345][0-9])\..*' // Single check for Class D and Class E - ); - - - - $pattern = '#^('.implode('|',$ignore).')#'; - - if(preg_match($pattern,$ip)) + + $isRoutable = filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE) !== false; + + if(!$isRoutable) { - return false; + return false; } - - - /* XXX preg_match doesn't accept arrays. - if (preg_match(array( - '#^0\..*#' , '#^127\..*#' , // Local loopbacks - '#^192\.168\..*#' , // RFC1918 - Private Network - '#^172\.(?:1[6789]|2\d|3[01])\..*#' , // RFC1918 - Private network - '#^10\..*#' , // RFC1918 - Private Network - '#^169\.254\..*#' , // RFC3330 - Link-local, auto-DHCP - '#^2(?:2[456789]|[345][0-9])\..*#' // Single check for Class D and Class E - ), $ip)) + + // Explicitly block IPv4 multicast: 224.0.0.0 - 239.255.255.255 + if(filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) { - return FALSE; - } - */ - - if (strpos(':', $ip) === FALSE) return TRUE; - // Must be an IPV6 address here - // @todo need to handle IPV4 addresses in IPV6 format - $ip = strtolower($ip); - if ($ip == 'ff02::1') return FALSE; // link-local all nodes multicast group - if ($ip == 'ff02:0000:0000:0000:0000:0000:0000:0001') return FALSE; - if ($ip == '::1') return FALSE; // localhost - if ($ip == '0000:0000:0000:0000:0000:0000:0000:0001') return FALSE; - if (strpos($ip, 'fc00:') === 0) return FALSE; // local addresses - // @todo add: - // ::0 (all zero) - invalid - // ff02::1:ff00:0/104 - Solicited-Node multicast addresses - add? - // 2001:0000::/29 through 2001:01f8::/29 - special purpose addresses - // 2001:db8::/32 - used in documentation - return TRUE; - } - - - - /** - * Get current user's IP address in 'normal' form. - * Likely to be very similar to existing e107::getIP() function - * May log X-FORWARDED-FOR cases - or could generate a special IPV6 address, maybe? - */ - private function getCurrentIP() - { - if(!$this->ourIP) - { - $ip = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : 'x.x.x.x'; - if ($ip4 = getenv('HTTP_X_FORWARDED_FOR')) + $long = ip2long($ip); + if($long !== false && $long >= ip2long('224.0.0.0') && $long <= ip2long('239.255.255.255')) { - if (!$this->isAddressRoutable($ip)) - { - $ip3 = explode(',', $ip4); // May only be one address; could be several, comma separated, if multiple proxies used - $ip = trim($ip3[count($ip3) - 1]); // If IP address is unroutable, replace with any forwarded_for address - $this->logBanItem(0, 'X_Forward '.$ip4.' --> '.$ip); // Just log for interest ATM - } + return false; } - $this->ourIP = $this->ipEncode($ip); // Normalise for storage } - return $this->ourIP; + + // Explicitly block IPv6 multicast: ff00::/8 + if(filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) + { + if(stripos($ip, 'ff') === 0 || stripos($ip, 'ff00:') === 0) + { + return false; + } + } + + return true; } + /** + * Get current user's IP address in 'normal' form. + * Likely to be very similar to existing e107::getIP() function + * May log X-FORWARDED-FOR cases - or could generate a special IPV6 address, maybe? + * + * @param array|null $server Array simulating $_SERVER (e.g., ['REMOTE_ADDR' => '1.2.3.4']). Defaults to global $_SERVER. + * @return string Normalized IP address. + */ + protected function getCurrentIP($server = null) + { + if(!$this->ourIP) + { + $server = $server ?? $_SERVER; + $ip = $server['REMOTE_ADDR'] ?? 'x.x.x.x'; + + if ($ip4 = getenv('HTTP_X_FORWARDED_FOR')) + { + if (!$this->isAddressRoutable($ip)) + { + $ip3 = explode(',', $ip4); // May only be one address; could be several, comma separated, if multiple proxies used + $ip = trim($ip3[count($ip3) - 1]); // If IP address is unroutable, replace with any forwarded_for address + $this->logBanItem(0, 'X_Forward '.$ip4.' --> '.$ip); // Just log for interest ATM + } + } + + $this->ourIP = $this->ipEncode($ip); // Normalise for storage + } + + return $this->ourIP; + } + + /** * Return the user's IP address, in normal or display-friendly form as requested @@ -370,7 +343,7 @@ class eIPHandler */ public function getIP($forDisplay = FALSE) { - if ($forDisplay == FALSE) return $this->ourIP; + if (!$forDisplay) return $this->ourIP; return $this->ipDecode($this->ourIP); } @@ -383,7 +356,7 @@ class eIPHandler * * @return void (may not even return) * - * Looks up the reason code, and extracts the corresponding text. + * Looks up the reason code, and extracts the corresponding text. * If this text begins with 'http://' or 'https://', assumed to be a link to a web page, and redirects. * Otherwise displays an error message to the user (if configured) then aborts. */ @@ -446,6 +419,7 @@ class eIPHandler die(); } } + $this->logBanItem($code, 'Unmatched action: '.$search.' - no block implemented'); } @@ -592,6 +566,11 @@ class eIPHandler */ public function ipEncode($ip, $wildCards = FALSE, $div = ':') { + if(empty($ip)) + { + return false; + } + $ret = ''; $divider = ''; if(strpos($ip, ':')!==FALSE) @@ -967,7 +946,7 @@ class eIPHandler 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']])) @@ -1076,7 +1055,7 @@ class eIPHandler if ($sql->select('banlist', '`banlist_bantype`', "`banlist_ip`='{$ban_ip}'")) { list($banType) = $sql->fetch(); - + if ($banType >= eIPHandler::BAN_TYPE_WHITELIST) { // Got a whitelist entry for this //$admin_log->addEvent(4, __FILE__."|".__FUNCTION__."@".__LINE__, "BANLIST_11", 'LAN_AL_BANLIST_11', $ban_ip, FALSE, LOG_TO_ROLLING); @@ -1096,15 +1075,15 @@ class eIPHandler $ban_message .= 'Host: '.$this->get_host_name($ban_ip); } // Add using an array - handles DB changes better - $sql->insert('banlist', + $sql->insert('banlist', array( 'banlist_id' => 0, - 'banlist_ip' => $ban_ip , - 'banlist_bantype' => $bantype , - 'banlist_datestamp' => time() , + 'banlist_ip' => $ban_ip , + 'banlist_bantype' => $bantype , + 'banlist_datestamp' => time() , 'banlist_banexpires' => (vartrue($pref['ban_durations'][$bantype]) ? time()+($pref['ban_durations'][$bantype]*60*60) : 0) , - 'banlist_admin' => $ban_user , - 'banlist_reason' => $ban_message , + 'banlist_admin' => $ban_user , + 'banlist_reason' => $ban_message , 'banlist_notes' => $ban_notes )); @@ -1290,7 +1269,7 @@ class banlistManager { e107::includeLan(e_LANGUAGEDIR.e_LANGUAGE."/admin/lan_banlist.php"); $this->ourConfigDir = e107::getIPHandler()->getConfigDir(); - $this->banTypes = array( // Used in Admin-ui. + $this->banTypes = array( // Used in Admin-ui. '-1' => BANLAN_101, // manual '-2' => BANLAN_102, // Flood '-3' => BANLAN_103, // Hits @@ -1300,8 +1279,8 @@ class banlistManager '-8' => BANLAN_107, // Imported '100' => BANLAN_120 // Whitelist ); - - + + } /** @@ -1311,7 +1290,7 @@ class banlistManager { return array( eIPHandler::BAN_TYPE_LEGACY, - eIPHandler::BAN_TYPE_MANUAL, + eIPHandler::BAN_TYPE_MANUAL, eIPHandler::BAN_TYPE_FLOOD, eIPHandler::BAN_TYPE_HITS, eIPHandler::BAN_TYPE_LOGINS, @@ -1320,7 +1299,7 @@ class banlistManager // Spare value eIPHandler::BAN_TYPE_UNKNOWN ); - } + } /** @@ -1338,7 +1317,7 @@ class banlistManager * BAN_FILE_CSV_NAME * BAN_FILE_EXTENSION File extension to append * - */ + */ public function writeBanListFiles($options = 'ip', $typeList = '') { e107::getMessage()->addDebug("Writing new Banlist files."); @@ -1406,15 +1385,15 @@ class banlistManager } } } - + // Now close each file foreach ($optList as $opt) { fclose($fileList[$opt]); } - + // Finally, delete the working file, rename the temporary one - // Docs suggest that 'newname' is auto-deleted if it exists (as it usually should) + // Docs suggest that 'newname' is auto-deleted if it exists (as it usually should) // - but didn't appear to work, hence copy then delete foreach ($optList as $opt) { @@ -1439,12 +1418,12 @@ class banlistManager { $ip = trim($ip); $temp = strpos($ip, 'x'); - if ($temp !== FALSE) + if ($temp !== FALSE) { return substr($ip, 0, $temp); } $temp = strpos($ip, '*'); - if ($temp !== FALSE) + if ($temp !== FALSE) { return substr($ip, 0, $temp); } @@ -1489,7 +1468,7 @@ class banlistManager case eIPHandler::BAN_TYPE_WHITELIST : return BANLAN_120; // Special case - may never occur - case eIPHandler::BAN_TYPE_UNKNOWN : + case eIPHandler::BAN_TYPE_UNKNOWN : default : if (($banType > 0) && ($banType < 9)) { @@ -1512,7 +1491,7 @@ class banlistManager public function writeBanMessageFile() { $pref['ban_messages'] = e107::getPref('ban_messages'); - + $oldName = $this->ourConfigDir.eIPHandler::BAN_FILE_ACTION_NAME.'_tmp'.eIPHandler::BAN_FILE_EXTENSION; if ($tmp = fopen($oldName, 'w')) { @@ -1576,8 +1555,8 @@ class banlistManager $ret = array_slice($vals, -$start - $count, $count); return array_reverse($ret); } - - + + /** * Converts one of the strings returned in a getLogEntries string into an array of values * @@ -1599,7 +1578,7 @@ class banlistManager $ret['banNotes'] = str_replace("\n", '', $temp[3]); return $ret; } - + /** * Delete ban Log file @@ -1638,7 +1617,7 @@ class banlistManager return 0; // Probably no retrigger actions } @unlink($fileName); // Delete the action file now we've read it in. - + // Scan the list completely before doing any processing - this will ensure we only process the most recent entry for each IP address while (count($entries) > 0) { diff --git a/e107_tests/tests/unit/eIPHandlerTest.php b/e107_tests/tests/unit/eIPHandlerTest.php index 9a36e3b11..946834735 100644 --- a/e107_tests/tests/unit/eIPHandlerTest.php +++ b/e107_tests/tests/unit/eIPHandlerTest.php @@ -1,101 +1,96 @@ ip = $this->make('eIPHandler'); + } + catch(Exception $e) + { + $this::fail("Couldn't load eIPHandler object"); + } + } - try - { - $this->ip = $this->make('eIPHandler'); - } catch(Exception $e) - { - $this->assertTrue(false, "Couldn't load eIPHandler object"); - } + /** + * Test IPHandler::ipDecode() + */ + public function testIpDecode() + { - $this->ip->__construct(); + $this->ip->__construct(); + + $this::assertEquals("101.102.103.104", $this->ip->ipDecode("101.102.103.104")); // IPv4 returns itself + + $this::assertEquals("10.11.12.13", $this->ip->ipDecode("0000:0000:0000:0000:0000:ffff:0a0b:0c0d")); // IPv6 uncompressed + + $this::assertEquals("201.202.203.204", $this->ip->ipDecode("00000000000000000000ffffc9cacbcc")); // 32-char hex + + // $this::assertEquals("123.123.123.123", $this->ip->ipDecode("::ffff:7b7b:7b7b")); // Fully compressed IPv6 (not supported) + + // $this::assertEquals("192.0.2.128", $this->ip->ipDecode("::ffff:c000:0280")); // RFC 4291 short form (not supported) + + // $this::assertEquals("8.8.8.8", $this->ip->ipDecode("0:0:0:0:0:ffff:808:808")); // Uncompressed mapped with short ints (not supported) + + // $this::assertEquals("8.8.4.4", $this->ip->ipDecode("::ffff:808:404")); // Double compressed form (not supported) + + // $this::assertEquals("1.2.3.4", $this->ip->ipDecode("::ffff:1.2.3.4")); // Embedded dot-decimal IPv4 (not supported) + } + + + public function testGetCurrentIP() + { + + $reflection = new ReflectionClass($this->ip); + $method = $reflection->getMethod('getCurrentIP'); + $method->setAccessible(true); + + $tests = [ + 0 => [ + 'server' => [ + 'REMOTE_ADDR' => '123.123.123.123' + ], + 'expected' => '123.123.123.123' + ] + ]; + + foreach($tests as $index => $test) + { + $result = $method->invoke($this->ip, $test['server']); // IP6 + $expected = $this->ip->ipEncode($test['expected']); // convert to IP6. + + $this::assertSame($expected, $result, "Failed on #$index"); } -/* public function testMakeEmailQuery() - { + } - } - public function testGet_host_name() - { + /** + * Test IPHandler::add_ban() + */ + public function testAdd_ban() + { - } + $this->ip->__construct(); - public function testSetIP() - { - - } - - public function testIpDecode() - { - - } - - public function testWhatIsThis() - { - - } - - public function testIp6AddWildcards() - { - - } - - public function testIsUserLogged() - { - - } - - public function testCheckFilePerms() - { - - } - - public function test__construct() - { - - } - - public function testCheckBan() - { - - } - - public function testPermsToString() - { - - } - - public function testMakeDomainQuery() - { - - }*/ - - public function testAdd_ban() - { - // $bantype = 1 for manual, 2 for flooding, 4 for multiple logins - - $banDurations = array( - '0' => 0, + $banDurations = array( + '0' => 0, '-1' => 0, // manually added ban '-2' => 0, // flood '-3' => 8, // hits @@ -103,58 +98,143 @@ '-5' => 0, // imported '-6' => 0, // banned user '-8' => 0 // unknown - ); + ); - //set ban duration pref. - 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', 0); - $this->assertTrue($result); + $result = $this->ip->add_ban(2, "unit test generated ban", '123.123.123.123'); + $this::assertTrue($result); + } + public function testIsAddressRoutable() + { - } -/* - public function testGetIP() + $testCases = [ + ['ip' => '8.8.8.8', 'expected' => true], + ['ip' => '192.168.1.1', 'expected' => false], + ['ip' => '127.0.0.1', 'expected' => false], + ['ip' => '10.0.0.45', 'expected' => false], + ['ip' => '172.20.5.4', 'expected' => false], + ['ip' => '169.254.1.2', 'expected' => false], + ['ip' => '224.0.0.1', 'expected' => false], + ['ip' => '240.0.0.1', 'expected' => false], + ['ip' => '24.300.0.124', 'expected' => false], + ['ip' => '2001:4860:4860::8888', 'expected' => true], + ]; + + foreach($testCases as $case) { - + $desc = sprintf("%s should %s be routable", $case['ip'], $case['expected'] ? '' : 'not'); + $result = $this->ip->isAddressRoutable($case['ip']); + $this::assertSame($case['expected'], $result, $desc); } - public function testGetConfigDir() - { - - } - - public function testRegenerateFiles() - { - - } - - public function testBan() - { - - } - - public function testIsAddressRoutable() - { - - } - - public function testIpEncode() - { - - } - - public function testDebug() - { - - } - - public function testGetUserToken() - { - - } - - */ - - } + + + /** + * Test IPHandler::ipEncode() + */ + public function testIpEncode() + { + + $tests = [ + // IPv4 to IPv6-mapped form + 0 => [ + 'ip' => '192.168.1.100', + 'wildCards' => false, + 'div' => ':', + 'expected' => '0000:0000:0000:0000:0000:ffff:c0a8:0164' + ], + // IPv6 + 1 => [ + 'ip' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334', + 'wildCards' => false, + 'div' => ':', + 'expected' => '2001:0db8:85a3:0000:0000:8a2e:0370:7334' + ], + // IPv6 (shortened) + 2 => [ + 'ip' => '2001:db8::1', + 'wildCards' => false, + 'div' => ':', + 'expected' => '2001:0db8:0000:0000:0000:0000:0000:0001' + ], + // Zero-padded hex (div = '') + 3 => [ + 'ip' => '127.0.0.1', + 'wildCards' => false, + 'div' => '', + 'expected' => '00000000000000000000ffff7f000001' + ], + // Wildcard input: expects encoded hex with xx + 4 => [ + 'ip' => '192.168.1.*', + 'wildCards' => true, + 'div' => ':', + 'expected' => '0000:0000:0000:0000:0000:ffff:c0a8:01xx' + ], + + // Invalid input + 5 => [ + 'ip' => 'not.an.ip', + 'wildCards' => false, + 'div' => ':', + 'expected' => '0000:0000:0000:0000:0000:ffff:0000:0000' + ], + 6 => [ + 'ip' => '192.168.1.x', + 'wildCards' => true, + 'div' => ':', + 'expected' => '0000:0000:0000:0000:0000:ffff:c0a8:01xx' + ], + 7 => [ + 'ip' => '', + 'wildCards' => false, + 'div' => ':', + 'expected' => false + ], + 8 => [ + 'ip' => null, + 'wildCards' => false, + 'div' => ':', + 'expected' => false + ], + 9 => [ + 'ip' => '*.*.*.*', + 'wildCards' => true, + 'div' => ':', + 'expected' => '0000:0000:0000:0000:0000:ffff:xxxx:xxxx' + ], + 10 => [ + 'ip' => '256.300.1.1', // invalid IP, should be 0-255. + 'wildCards' => false, + 'div' => ':', + 'expected' => '0000:0000:0000:0000:0000:ffff:10012c:0101' // should be false + ], + 11 => [ + 'ip' => '::', + 'wildCards' => false, + 'div' => ':', + 'expected' => '0000:0000:0000:0000:0000:0000:0000:0000' + ], + 12 => [ + 'ip' => 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', + 'wildCards' => false, + 'div' => ':', + 'expected' => 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff' + ], + ]; + + foreach($tests as $i => $case) + { + $result = $this->ip->ipEncode($case['ip'], $case['wildCards'], $case['div']); + $msg = "Failed on test #$i ({$case['ip']})"; + $this::assertSame($case['expected'], $result, $msg); + } + + } + + +} +