From a5b8728037d50917ea819aef093123d65696bb58 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 22:16:29 +0000 Subject: [PATCH 1/4] Convert the stored addresses in one request, not in all that arrive at once After the update, the block and allow list entries are brought into canonical form on the first request. Every request starts with the settings as they were when it began, though, so all requests arriving before the first one was done saw the flag unset and each went through the whole list, updating the same rows - on a busy site with a long list, many at once. The conversion now takes a lock (in Joomla's tmp directory, like the .htaccess changes); requests which don't get it go on without waiting, and the one which does checks the stored flag again first. Without a usable lock directory it runs as before. Also adds Bfstop::InternalSettings, the settings the plugin keeps for itself (addressesCanonical, lastPurge), which com_bfstop's settings page has to keep when it saves. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AFJFMMUpch4LGexEiXN8Nc --- src/Extension/Bfstop.php | 56 ++++++++++++++++++++++-- src/Helper/DatabaseHelper.php | 25 +++++++++++ src/Helper/HtaccessHelper.php | 11 ++--- tests/Integration/BlockedRequestTest.php | 28 ++++++++++++ 4 files changed, 112 insertions(+), 8 deletions(-) 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 From 920c08320efd38a711716da4d1ee1f881e8cb005 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 22:17:38 +0000 Subject: [PATCH 2/4] Installer script: check versions, rename whitelist before the update SQL - The script extends Joomla's InstallerScript, whose preflight() refuses Joomla before 5.0 and PHP before 8.1 (the plugin needs them: a site on Joomla 4 could install it by hand) and replacing a newer version of the plugin with an older one. - The "configuration has moved to the component" hint is only shown when updating from a version before 2.0.0, not on every later update. - The rename of #__bfstop_whitelist to #__bfstop_allowlist (installations made with 1.4.2 kept the old name) moved from update() to preflight(): Joomla runs the update SQL between the two, and 2.0.0.sql changes the allowlist table, so such a site couldn't be updated. The rename now only happens if the old table is there and the new one isn't, instead of trying and ignoring the error. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AFJFMMUpch4LGexEiXN8Nc --- CHANGELOG | 4 +- tests/Integration/InstallerScriptTest.php | 125 ++++++++++++++++++++++ updatescript.php | 78 +++++++++----- 3 files changed, 182 insertions(+), 25 deletions(-) create mode 100644 tests/Integration/InstallerScriptTest.php diff --git a/CHANGELOG b/CHANGELOG index 791e1ef..d2150be 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 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 From 662aa901ebd90a0e37eaf9419df1eb8912b31706 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 22:20:23 +0000 Subject: [PATCH 3/4] Test com_bfstop's settings save and installer script The settings page must keep the values the plugin stores for itself (Bfstop::InternalSettings), and the component's installer script shows its hint on installation only and refuses to replace a newer version. Needs the com_bfstop branch of the same name. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AFJFMMUpch4LGexEiXN8Nc --- tests/Integration/ComponentTest.php | 67 +++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) 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 From 4b228c77d0bbc5ef231488821e423f8200de12ce Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 5 Nov 2026 22:20:29 +0000 Subject: [PATCH 4/4] Changelog: the delay settings go up to 60 seconds Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AFJFMMUpch4LGexEiXN8Nc --- CHANGELOG | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index d2150be..946f78d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -182,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