diff --git a/CHANGELOG b/CHANGELOG index 791e1ef..946f78d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,7 +1,9 @@ 2.0.0 (2026-08-24) - Migrate to Joomla 5/6: PSR-4 namespaced classes (Codeling\Plugin\System\Bfstop), service provider, SubscriberInterface event handling - - Drop Joomla 3/4 compatibility (requires Joomla 5 or 6) + - Drop Joomla 3/4 compatibility (requires Joomla 5 or 6 and PHP 8.1); the + installer refuses older Joomla and PHP versions, and replacing a newer + version of BFStop with an older one - Modernize token generation to use random_bytes()/random_int() - Consolidate all configuration into the component's Settings view (Components -> Brute Force Stop -> Settings); the plugin's Options tab @@ -180,7 +182,9 @@ purge age to 0 for the old behaviour) and limited to 200000 rows - Security: a response is delayed by 60 seconds at most, however the delay settings add up, and the account throttle no longer delays an address - that was just blocked + that was just blocked. "Delay Duration" and "Maximum Adaptive Delay" go + up to 60 seconds (before: 120); a larger value saved by an earlier + version has to be lowered the next time the settings are saved - Security: unblock tokens are stored as hashes; links sent before the update stop working (they are valid for 3 days only) - Security: usernames in log entries and in mails can no longer contain diff --git a/src/Extension/Bfstop.php b/src/Extension/Bfstop.php index 89a603c..fd3225f 100644 --- a/src/Extension/Bfstop.php +++ b/src/Extension/Bfstop.php @@ -39,6 +39,13 @@ class Bfstop extends CMSPlugin implements SubscriberInterface * compares them). The plugin is enabled by the installation, so it runs * with these until an administrator opens the settings. */ + /** + * Values the plugin keeps among its settings for itself: they are not on + * the settings page, so saving that page must keep them (see com_bfstop's + * SettingsModel). + */ + public const InternalSettings = array('addressesCanonical', 'lastPurge'); + public const DefaultSettings = array( 'blockNumber' => 10, 'checkInterval' => 10080, @@ -791,9 +798,7 @@ public function onAfterInitialise($event) // version may be loaded still if (!$this->getBoolParam('addressesCanonical', false)) { - $this->mydb->canonicalizeStoredAddresses(); - $this->mydb->savePluginSetting('addressesCanonical', 1); - $this->params->set('addressesCanonical', 1); + $this->canonicalizeStoredAddressesOnce(); } // periodic maintenance, at most once a day $purgeInterval = 86400; // = 24*60*60 => one day @@ -821,6 +826,51 @@ public function onAfterInitialise($event) } } + /** + * Brings the stored addresses into canonical form, if no other request + * did or does: every request starts out with the settings as they were + * when it began, so after the update all requests arriving before the + * first of them is done would see the setting unset, and each would go + * through the whole block and allow list (and update the same rows) - on + * a busy site with a long list, many at once. The first takes a lock; the + * others go on without waiting (their lookups may miss an entry which + * isn't converted yet, as they would have before the update). Once it has + * the lock, a request checks the stored setting again, as another one may + * have finished in the meantime. If no lock can be taken (no writable + * temporary directory), it is done without one: it can be run more than + * once, it only costs time. + */ + private function canonicalizeStoredAddressesOnce() + { + $lockFile = HtaccessHelper::lockDirectory().'/bfstop-canonicalize-'. + md5((string) $this->getApplication()->get('db').(string) $this->getApplication()->get('dbprefix')).'.lock'; + $handle = @fopen($lockFile, 'c'); + if ($handle !== false && !flock($handle, LOCK_EX | LOCK_NB)) + { + fclose($handle); + $this->logger->log('Another request is converting the stored addresses, not waiting for it', Log::DEBUG); + return; + } + try + { + if (!$this->mydb->getPluginSetting('addressesCanonical', false)) + { + $changed = $this->mydb->canonicalizeStoredAddresses(); + $this->mydb->savePluginSetting('addressesCanonical', 1); + $this->logger->log('Converted '.$changed.' stored address(es) to canonical form', Log::INFO); + } + $this->params->set('addressesCanonical', 1); + } + finally + { + if ($handle !== false) + { + flock($handle, LOCK_UN); + fclose($handle); + } + } + } + /** * The status code of the page shown to a blocked client, or null for none * (which makes it 200). Set with http_response_code(), not as a raw diff --git a/src/Helper/DatabaseHelper.php b/src/Helper/DatabaseHelper.php index cb75441..247396e 100644 --- a/src/Helper/DatabaseHelper.php +++ b/src/Helper/DatabaseHelper.php @@ -915,6 +915,31 @@ public function saveLastPurge($timestamp) * request was started with would undo whatever an administrator saved in * the meantime. */ + /** + * A setting as it is stored now - the plugin's $params are what was stored + * when the request began. + */ + public function getPluginSetting($name, $default = null) + { + try + { + $query = $this->db->getQuery(true) + ->select($this->db->quoteName('params')) + ->from($this->db->quoteName('#__extensions')) + ->where($this->db->quoteName('type').' = '.$this->db->quote('plugin')) + ->where($this->db->quoteName('folder').' = '.$this->db->quote('system')) + ->where($this->db->quoteName('element').' = '.$this->db->quote('bfstop')); + $this->db->setQuery($query); + $params = json_decode((string) $this->db->loadResult(), true); + return (is_array($params) && array_key_exists($name, $params)) ? $params[$name] : $default; + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + return $default; + } + } + public function savePluginSetting($name, $value) { try diff --git a/src/Helper/HtaccessHelper.php b/src/Helper/HtaccessHelper.php index 9150871..e11d001 100644 --- a/src/Helper/HtaccessHelper.php +++ b/src/Helper/HtaccessHelper.php @@ -223,7 +223,7 @@ public function remove403Message() */ private function locked(callable $change) { - $lockFile = $this->lockDirectory().'/bfstop-htaccess-'.md5($this->path).'.lock'; + $lockFile = self::lockDirectory().'/bfstop-htaccess-'.md5($this->path).'.lock'; $handle = @fopen($lockFile, 'c'); if ($handle === false || !flock($handle, LOCK_EX)) { @@ -249,11 +249,12 @@ private function locked(callable $change) } /** - * Joomla's own temporary directory if there is one we can use, not the - * system's: that one is shared with other users of the machine on many - * hosts, who could then hold the lock (or plant a link under its name). + * Where lock files go (also those of the plugin, see Bfstop): Joomla's own + * temporary directory if there is one we can use, not the system's: that + * one is shared with other users of the machine on many hosts, who could + * then hold the lock (or plant a link under its name). */ - private function lockDirectory() + public static function lockDirectory() { try { diff --git a/tests/Integration/BlockedRequestTest.php b/tests/Integration/BlockedRequestTest.php index c67fa3f..a44e7bd 100644 --- a/tests/Integration/BlockedRequestTest.php +++ b/tests/Integration/BlockedRequestTest.php @@ -92,6 +92,34 @@ public function testFullModeBlocksEverything() $this->assertNotBlocked('', '203.0.113.42'); } + public function testStoredAddressesAreConvertedByOneRequestAtATime() + { + // after the update, the first request converts the stored addresses; + // the ones arriving while it is at it must not all do the same + $this->configure(); + $id = $this->block('2001:DB8::41'); + $app = Factory::getApplication(); + $lockFile = HtaccessHelper::lockDirectory().'/bfstop-canonicalize-'. + md5((string) $app->get('db').(string) $app->get('dbprefix')).'.lock'; + $handle = fopen($lockFile, 'c'); + $this->assertTrue(flock($handle, LOCK_EX), 'the test takes the lock of the "first request"'); + try + { + $this->request('', '203.0.113.99'); + $this->assertSame('2001:DB8::41', $this->queryValue('SELECT ipaddress FROM #__bfstop_bannedip WHERE id='.$id), + 'converted although another request holds the lock'); + $this->assertArrayNotHasKey('addressesCanonical', json_decode($this->getPluginParams(), true)); + } + finally + { + flock($handle, LOCK_UN); + fclose($handle); + } + $this->request('', '203.0.113.99'); + $this->assertSame('2001:db8::41', $this->queryValue('SELECT ipaddress FROM #__bfstop_bannedip WHERE id='.$id)); + $this->assertSame(1, json_decode($this->getPluginParams(), true)['addressesCanonical']); + } + public function testBlockIsEnforcedOnlyAfterRouting() { // with SEF URLs option/view are empty until the router ran, so the diff --git a/tests/Integration/ComponentTest.php b/tests/Integration/ComponentTest.php index 2b7556e..a4f298e 100644 --- a/tests/Integration/ComponentTest.php +++ b/tests/Integration/ComponentTest.php @@ -15,6 +15,7 @@ use Codeling\Component\Bfstop\Administrator\Model\BlockModel; use Codeling\Component\Bfstop\Administrator\Model\BlocklistModel; use Codeling\Component\Bfstop\Administrator\Model\FailedloginlistModel; +use Codeling\Component\Bfstop\Administrator\Model\SettingsModel; use Codeling\Component\Bfstop\Administrator\Model\UsernamestatsModel; use Codeling\Component\Bfstop\Site\Model\TokenunblockModel; use Codeling\Plugin\System\Bfstop\Helper\DatabaseHelper; @@ -394,6 +395,72 @@ public function testSettingsAndLogViewsNeedTheAdminPermission() } } + public function testSavingTheSettingsKeepsWhatThePluginStoresForItself() + { + // the form only has the settings; the plugin's own values must survive + // a save, or it would e.g. convert all stored addresses again + $original = $this->getPluginParams(); + try + { + $this->setPluginParams(json_encode(array('blockNumber' => 7, 'lastPurge' => 1700000000, + 'addressesCanonical' => 1))); + $model = $this->model(SettingsModel::class); + $model->getState(); + $this->assertTrue($model->save(array('params' => array('blockNumber' => 8, 'logLevel' => 8))), + implode(', ', $model->getErrors())); + $saved = json_decode($this->getPluginParams(), true); + $this->assertSame(8, (int) $saved['blockNumber']); + $this->assertSame(1700000000, $saved['lastPurge']); + $this->assertSame(1, $saved['addressesCanonical']); + $this->assertSame(\Codeling\Plugin\System\Bfstop\Extension\Bfstop::InternalSettings, + array_values(array_intersect(array_keys($saved), \Codeling\Plugin\System\Bfstop\Extension\Bfstop::InternalSettings))); + // a value given in the data wins (not that the form has them) + $this->assertSame(array('lastPurge' => 5), SettingsModel::keepInternalSettings(array('lastPurge' => 5), + array('lastPurge' => 1700000000))); + } + finally + { + $this->setPluginParams($original); + } + } + + public function testInstallerScriptHintAndVersionCheck() + { + require_once getenv('COM_BFSTOP_ROOT').'/installscript.php'; + $app = \Joomla\CMS\Factory::getApplication(); + $adapter = $this->createMock(\Joomla\CMS\Installer\InstallerAdapter::class); + $adapter->method('getName')->willReturn('com_bfstop'); + $adapter->method('getManifest')->willReturn(simplexml_load_file(getenv('COM_BFSTOP_ROOT').'/bfstop.xml')); + $script = new \com_bfstopInstallerScript($adapter); + $app->getMessageQueue(true); + + // the hint about the settings is for new installations; on an update, + // the plugin's script says what to look at, if anything + $script->postflight('update', $adapter); + $this->assertSame(array(), $app->getMessageQueue(true)); + $script->postflight('install', $adapter); + $this->assertCount(1, $app->getMessageQueue(true)); + + $cache = $this->queryValue("SELECT manifest_cache FROM #__extensions WHERE type='component' AND element='com_bfstop'"); + try + { + $this->assertTrue($script->preflight('update', $adapter)); + $newer = json_decode((string) $cache, true) ?: array(); + $newer['version'] = '99.0.0'; + $this->db->setQuery('UPDATE #__extensions SET manifest_cache='.$this->db->quote(json_encode($newer)). + " WHERE type='component' AND element='com_bfstop'"); + $this->db->execute(); + $this->assertFalse($script->preflight('update', $adapter), 'replaced a newer version'); + } + finally + { + $this->db->setQuery('UPDATE #__extensions SET manifest_cache='.$this->db->quote($cache). + " WHERE type='component' AND element='com_bfstop'"); + $this->db->execute(); + $app->getMessageQueue(true); + } + } + public function testUnsavedSettingsAreWhatTheSettingsPageShows() { // the plugin is enabled by the installation: until the settings are diff --git a/tests/Integration/InstallerScriptTest.php b/tests/Integration/InstallerScriptTest.php new file mode 100644 index 0000000..a792afa --- /dev/null +++ b/tests/Integration/InstallerScriptTest.php @@ -0,0 +1,125 @@ +originalManifestCache = $this->queryValue( + "SELECT manifest_cache FROM #__extensions WHERE type='plugin' AND element='bfstop'"); + Factory::getApplication()->getMessageQueue(true); + } + + protected function tearDown(): void + { + $this->setInstalledVersion(null); + Factory::getApplication()->getMessageQueue(true); + } + + private function setInstalledVersion($version) + { + $cache = $this->originalManifestCache; + if ($version !== null) + { + $decoded = json_decode((string) $cache, true) ?: array(); + $decoded['version'] = $version; + $cache = json_encode($decoded); + } + $this->db->setQuery('UPDATE #__extensions SET manifest_cache='.$this->db->quote($cache). + " WHERE type='plugin' AND element='bfstop'"); + $this->db->execute(); + } + + private function script() + { + $adapter = $this->createMock(InstallerAdapter::class); + $adapter->method('getName')->willReturn('plg_system_bfstop'); + $adapter->method('getManifest')->willReturn(simplexml_load_file(dirname(__DIR__, 2).'/bfstop.xml')); + return array(new \PlgsystembfstopInstallerScript($adapter), $adapter); + } + + private function messages() + { + return array_map(function ($message) { return $message['message']; }, + Factory::getApplication()->getMessageQueue(true)); + } + + private function updateHintShown($fromVersion) + { + $this->setInstalledVersion($fromVersion); + list($script, $adapter) = $this->script(); + $this->assertTrue($script->preflight('update', $adapter)); + $script->postflight('update', $adapter); + $hint = strip_tags(\Joomla\CMS\Language\Text::sprintf('PLG_SYSTEM_BFSTOP_UPDATE_2_0_0_HINT', '')); + foreach ($this->messages() as $message) + { + if (strip_tags($message) === $hint) + { + return true; + } + } + return false; + } + + public function testSettingsMovedHintOnlyForUpdatesFromBefore200() + { + Factory::getApplication()->getLanguage()->load('plg_system_bfstop', JPATH_ADMINISTRATOR); + $this->assertTrue($this->updateHintShown('1.5.2')); + $this->assertFalse($this->updateHintShown('2.0.0'), 'the settings did not move again'); + } + + public function testRefusesToReplaceANewerVersion() + { + $this->setInstalledVersion('99.0.0'); + list($script, $adapter) = $this->script(); + $this->assertFalse($script->preflight('update', $adapter)); + } + + public function testWhitelistTableIsRenamedBeforeTheUpdateSql() + { + // new installations of 1.4.2 kept the old table name; the update SQL + // of 2.0.0 (run between preflight and update) changes the allowlist + if ($this->db->getServerType() !== 'mysql') + { + $this->markTestSkipped('only MySQL installations can be that old'); + } + $this->db->setQuery('RENAME TABLE #__bfstop_allowlist TO #__bfstop_whitelist'); + $this->db->execute(); + try + { + $this->setInstalledVersion('1.4.2'); + list($script, $adapter) = $this->script(); + $this->assertTrue($script->preflight('update', $adapter)); + $tables = array_map('strtolower', $this->db->getTableList()); + $this->assertContains(strtolower($this->db->getPrefix().'bfstop_allowlist'), $tables); + $this->assertNotContains(strtolower($this->db->getPrefix().'bfstop_whitelist'), $tables); + } + finally + { + $tables = array_map('strtolower', $this->db->getTableList()); + if (in_array(strtolower($this->db->getPrefix().'bfstop_whitelist'), $tables, true)) + { + $this->db->setQuery('RENAME TABLE #__bfstop_whitelist TO #__bfstop_allowlist'); + $this->db->execute(); + } + } + } +} diff --git a/updatescript.php b/updatescript.php index 6e3d386..3cd12e2 100644 --- a/updatescript.php +++ b/updatescript.php @@ -9,12 +9,21 @@ use Joomla\CMS\Factory; use Joomla\CMS\Installer\InstallerAdapter; +use Joomla\CMS\Installer\InstallerScript; use Joomla\CMS\Language\Text; use Joomla\CMS\Router\Route; use Joomla\Database\DatabaseInterface; -class PlgsystembfstopInstallerScript +class PlgsystembfstopInstallerScript extends InstallerScript { + // InstallerScript::preflight() refuses to install on anything older (and + // to replace a newer version of the plugin with an older one) + protected $minimumJoomla = '5.0.0'; + protected $minimumPhp = '8.1.0'; + + // the version this update replaces, null for an installation + private $previousVersion = null; + public function __construct(InstallerAdapter $adapter) {} public function install(InstallerAdapter $adapter) @@ -52,11 +61,52 @@ public function uninstall(InstallerAdapter $adapter) // a leftover log must not make the uninstallation fail } } - public function preflight($type, InstallerAdapter $adapter) {} - - public function postflight($type, InstallerAdapter $adapter) + public function preflight($type, $adapter) { + if (!parent::preflight($type, $adapter)) + { + return false; + } if ($type === 'update') + { + $manifest = $this->getItemArray('manifest_cache', '#__extensions', 'name', 'plg_system_bfstop'); + $this->previousVersion = isset($manifest['version']) ? (string) $manifest['version'] : null; + $this->renameWhitelistTable(); + } + return true; + } + + /** + * For version 1.4.2, the whitelist table was renamed to allowlist, but only + * for updates; new installations kept the old name. This has to happen + * before the update SQL, which runs after preflight() but before update() + * and changes the allowlist table (MySQL only - PostgreSQL is supported + * from 2.0.0 on, so no such table exists there). + */ + private function renameWhitelistTable() + { + $db = Factory::getContainer()->get(DatabaseInterface::class); + if ($db->getServerType() !== 'mysql') + { + return; + } + $prefix = $db->getPrefix(); + $tables = array_map('strtolower', $db->getTableList()); + if (in_array(strtolower($prefix.'bfstop_whitelist'), $tables, true) && + !in_array(strtolower($prefix.'bfstop_allowlist'), $tables, true)) + { + $db->setQuery('RENAME TABLE '.$db->quoteName('#__bfstop_whitelist'). + ' TO '.$db->quoteName('#__bfstop_allowlist')); + $db->execute(); + } + } + + public function postflight($type, $adapter) + { + // only for the update from a version before the settings moved, not + // for every later update + if ($type === 'update' && $this->previousVersion !== null && + version_compare($this->previousVersion, '2.0.0', '<')) { $lang = Factory::getApplication()->getLanguage(); $lang->load('plg_system_bfstop', JPATH_ADMINISTRATOR); @@ -69,27 +119,7 @@ public function postflight($type, InstallerAdapter $adapter) public function update(InstallerAdapter $adapter) { - // for version 1.4.2, whitelist was renamed to allowlist, but only for updates; - // for new installs, the old name remained, so let's fix this for all installations - // (MySQL only - PostgreSQL is supported from 2.0.0 on, so no such table exists there): $db = Factory::getContainer()->get(DatabaseInterface::class); - if ($db->getServerType() === 'mysql') - { - try - { - $sql = "SELECT COUNT(*) FROM `#__bfstop_whitelist`"; - $db->setQuery($sql); - $numEntries = ((int)$db->loadResult()); - $sql = "RENAME TABLE `#__bfstop_whitelist` TO `#__bfstop_allowlist`"; - $db->setQuery($sql); - $db->execute(); - } - catch (Exception $e) - { - // if table doesn't exist, there's nothing we need to do -// Log::add("Update ERROR: ".$e->getMessage(), Log::ERROR, 'Update'); - } - } // for 2.0.0, the previously separate blockEnabled/useHtaccess switches // and the (never released) two-value blockMode were unified into a