Fix login-only mode for backend logins; add lint job and more tests

.github/workflows/ci.yml

@@ -10,6 +10,26 @@ permissions:
   contents: read
 
 jobs:
+  lint:
+    name: Manifest, language and package checks
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v7
+      - uses: shivammathur/setup-php@v2
+        with:
+          php-version: '8.4'
+          coverage: none
+      - name: XML files are well-formed
+        run: |
+          sudo apt-get install -y -q libxml2-utils > /dev/null
+          find . -name '*.xml' -not -path './.git/*' -print0 | xargs -0 xmllint --noout
+      - name: Language files
+        run: php tests/lint/check-language.php bfstop.xml PLG_SYSTEM_BFSTOP
+      - name: Release zip contains everything the manifest references
+        run: |
+          ./deploy.sh zip
+          php tests/lint/check-zip.php bfstop-*.zip bfstop.xml
+
   unit:
     name: Syntax check and unit tests (PHP ${{ matrix.php }})
     runs-on: ubuntu-latest

deploy.sh

@@ -10,7 +10,7 @@ dstdir=
 # internal variables to be updated when files are added:
 extname=bfstop
 sqlfiles="sql"
-srcfiles="$extname.php helpers $extname.xml $sqlfiles updatescript.php index.html src services"
+srcfiles="$extname.xml $sqlfiles updatescript.php index.html src services"
 langfiles="language"
 docs="CHANGELOG LICENSE.txt README"
 plgtype="system"

src/Extension/Bfstop.php

@@ -470,19 +470,21 @@ private function isPasswordRecoveryRequest()
 
 	/**
 	 * Detects a request that actually submits login credentials, for
-	 * "Login Only" block mode (issue #187): Joomla routes credential
-	 * submission through com_users on both the frontend (task=user.login)
-	 * and the backend (task=login, e.g. the entry_url Joomla itself builds
-	 * for the admin login form) - merely viewing the login form (no task,
-	 * or a display task) doesn't match, so it stays reachable.
+	 * "Login Only" block mode (issue #187): on the frontend, credentials
+	 * are submitted to com_users (task=user.login), the backend login form
+	 * posts to com_login (task=login); com_users with task=login is kept
+	 * for backend entry URLs built that way - merely viewing the login
+	 * form (no task, or a display task) doesn't match, so it stays
+	 * reachable.
 	 */
 	private function isLoginAttemptRequest()
 	{
 		$input = $this->getApplication()->input;
 		$option = $input->getCmd('option', '');
 		$task = $input->getCmd('task', '');
 		$result = (strcmp($option, 'com_users') == 0 &&
-			(strcmp($task, 'user.login') == 0 || strcmp($task, 'login') == 0));
+			(strcmp($task, 'user.login') == 0 || strcmp($task, 'login') == 0)) ||
+			(strcmp($option, 'com_login') == 0 && strcmp($task, 'login') == 0);
 		if ($result)
 		{
 			$this->logger->log('Detected a login-attempt request (task='.$task.')', Log::DEBUG);

tests/Integration/BlockedRequestTest.php

@@ -0,0 +1,164 @@
+<?php
+/*
+ * @package BFStop Plugin (bfstop) for Joomla!
+ * @author Bernhard Froehler
+ * @copyright (C) Bernhard Froehler
+ * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html
+**/
+namespace Codeling\Bfstop\Tests\Integration;
+
+use Codeling\Plugin\System\Bfstop\Helper\DatabaseHelper;
+use Joomla\CMS\Factory;
+
+/**
+ * How the plugin treats requests from a blocked IP address, depending on its
+ * settings: each request runs in a separate process (see fixtures/request.php),
+ * which reads the plugin's settings from the database as a real request does.
+ */
+class BlockedRequestTest extends IntegrationTestCase
+{
+	private const BlockedMessage = 'BFSTOP TEST: blocked';
+	private const Ip = '203.0.113.41';
+
+	private static $originalParams;
+
+	public static function setUpBeforeClass(): void
+	{
+		parent::setUpBeforeClass();
+		$db = Factory::getDbo();
+		$db->setQuery("SELECT params FROM #__extensions WHERE type='plugin' AND element='bfstop'");
+		self::$originalParams = $db->loadResult();
+	}
+
+	public static function tearDownAfterClass(): void
+	{
+		if (self::$originalParams !== null)
+		{
+			$db = Factory::getDbo();
+			$db->setQuery('UPDATE #__extensions SET params='.$db->quote(self::$originalParams).
+				" WHERE type='plugin' AND element='bfstop'");
+			$db->execute();
+		}
+	}
+
+	private function configure(array $params = array())
+	{
+		$this->setPluginParams(json_encode($params + array(
+			'blockMode' => 'full',
+			'blockedMessage' => self::BlockedMessage,
+			'logLevel' => 8, // errors only
+		)));
+	}
+
+	private function block($ip = self::Ip, $minutesAgo = 0, $duration = 60)
+	{
+		return $this->insert('#__bfstop_bannedip', array('ipaddress' => $ip,
+			'crdate' => self::minutesAgo($minutesAgo), 'duration' => $duration), 'id');
+	}
+
+	private function request($query = '', $ip = self::Ip)
+	{
+		$command = escapeshellarg(PHP_BINARY).' '.escapeshellarg(__DIR__.'/fixtures/request.php').' '.
+			escapeshellarg($ip).' '.escapeshellarg($query).' 2>&1';
+		exec($command, $output, $exitCode);
+		$output = implode("\n", $output);
+		$this->assertSame(0, $exitCode, $output);
+		return $output;
+	}
+
+	private function assertBlocked($query = '', $ip = self::Ip)
+	{
+		$output = $this->request($query, $ip);
+		$this->assertStringContainsString(self::BlockedMessage, $output, "request '$query' should be blocked");
+		$this->assertStringNotContainsString('NOT BLOCKED', $output);
+	}
+
+	private function assertNotBlocked($query = '', $ip = self::Ip)
+	{
+		$output = $this->request($query, $ip);
+		$this->assertStringContainsString('NOT BLOCKED', $output, "request '$query' should not be blocked");
+		$this->assertStringNotContainsString(self::BlockedMessage, $output);
+	}
+
+	public function testFullModeBlocksEverything()
+	{
+		$this->configure();
+		$this->block();
+		$this->assertBlocked();
+		$this->assertBlocked('option=com_content&view=article&id=1');
+		$this->assertBlocked('option=com_users&task=user.login');
+		$this->assertNotBlocked('', '203.0.113.42');
+	}
+
+	public function testBlockedMessageCanShowIp()
+	{
+		$this->configure(array('blockedMsgShowIP' => 1));
+		$this->block();
+		$this->assertStringContainsString(self::Ip, $this->request());
+	}
+
+	public function testExpiredBlockNoLongerApplies()
+	{
+		$this->configure();
+		$this->block(self::Ip, 61, 60);
+		$this->assertNotBlocked();
+	}
+
+	public function testPermanentBlockNeverExpires()
+	{
+		$this->configure();
+		// duration 0 = "forever"; 5 years is still within DatabaseHelper::$UNLIMITED_DURATION
+		$this->block(self::Ip, 5 * 365 * 24 * 60, 0);
+		$this->assertBlocked();
+	}
+
+	public function testLoginOnlyModeOnlyRejectsLoginAttempts()
+	{
+		$this->configure(array('blockMode' => 'loginonly'));
+		$this->block();
+		$this->assertNotBlocked();
+		$this->assertNotBlocked('option=com_users&view=login');
+		// frontend login form
+		$this->assertBlocked('option=com_users&task=user.login');
+		// backend login form
+		$this->assertBlocked('option=com_login&task=login');
+	}
+
+	public function testPasswordRecoveryStaysReachable()
+	{
+		// a blocked legitimate user must still be able to reset their password
+		$this->configure();
+		$this->block();
+		$this->assertNotBlocked('option=com_users&view=reset');
+		$this->assertNotBlocked('option=com_users&view=remind');
+		$this->assertBlocked('option=com_users&view=login');
+	}
+
+	public function testValidUnblockTokenGetsThrough()
+	{
+		$this->configure();
+		$blockId = $this->block();
+		$token = (new DatabaseHelper($this->logger))->getNewUnblockToken($blockId, str_repeat('ab', 20));
+		$this->assertNotBlocked('option=com_bfstop&view=tokenunblock&token='.$token);
+		$this->assertBlocked('option=com_bfstop&view=tokenunblock&token='.str_repeat('0', 40));
+		$this->assertBlocked('option=com_bfstop&view=tokenunblock');
+	}
+
+	public function testRejectedRequestsAreCounted()
+	{
+		$this->configure();
+		$blockId = $this->block();
+		$this->assertBlocked();
+		$this->assertBlocked();
+		$this->assertSame(2, (int) $this->queryValue('SELECT attempts FROM #__bfstop_bannedip WHERE id='.$blockId));
+		$this->assertNotNull($this->queryValue('SELECT last_attempt FROM #__bfstop_bannedip WHERE id='.$blockId));
+	}
+
+	public function testUnblockedBlockNoLongerApplies()
+	{
+		$this->configure();
+		$blockId = $this->block();
+		$this->insert('#__bfstop_unblock', array('block_id' => $blockId, 'source' => 0, 'crdate' => self::minutesAgo(0)));
+		$this->assertNotBlocked();
+	}
+}

tests/Integration/GeoHelperTest.php

@@ -0,0 +1,46 @@
+<?php
+/*
+ * @package BFStop Plugin (bfstop) for Joomla!
+ * @author Bernhard Froehler
+ * @copyright (C) Bernhard Froehler
+ * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html
+**/
+
+namespace Codeling\Bfstop\Tests\Integration;
+
+use Codeling\Plugin\System\Bfstop\Helper\GeoHelper;
+use Joomla\CMS\Log\Log;
+
+class GeoHelperTest extends IntegrationTestCase
+{
+	public function testEmptyDbPathReturnsNullSilently()
+	{
+		$logger = $this->logger;
+		$this->assertNull(GeoHelper::getCountryCode($logger, '', '203.0.113.5'));
+		$this->assertNull(GeoHelper::getCityDetails($logger, '', '203.0.113.5'));
+		$this->assertCount(0, $logger->messages);
+	}
+
+	public function testUnreadableDbPathReturnsNullWithWarning()
+	{
+		$logger = $this->logger;
+		$this->assertNull(GeoHelper::getCountryCode($logger, '/nonexistent/GeoLite2-Country.mmdb', '203.0.113.5'));
+		$this->assertTrue($logger->hasMessage(Log::WARNING, 'not readable'));
+	}
+
+	public function testCorruptDbReturnsNullWithWarning()
+	{
+		$file = tempnam(sys_get_temp_dir(), 'bfstop-geo');
+		file_put_contents($file, str_repeat('not a maxmind database', 100));
+		try
+		{
+			$logger = $this->logger;
+			$this->assertNull(GeoHelper::getCountryCode($logger, $file, '203.0.113.5'));
+			$this->assertTrue($logger->hasMessage(Log::WARNING, 'lookup failed'));
+		}
+		finally
+		{
+			unlink($file);
+		}
+	}
+}