From c013e6ce1d25fe0f2b9ec416d51c674e8ef2bb44 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 19:45:44 +0000 Subject: [PATCH 01/12] Test that the unblock token only passes the unblock view A valid token must open the unblock view and nothing else: other tasks or components (also in other spellings) and non-string tokens stay blocked, and the token of another address's block is no pass. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- tests/Integration/BlockedRequestTest.php | 36 ++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/Integration/BlockedRequestTest.php b/tests/Integration/BlockedRequestTest.php index af8a77e..3341592 100644 --- a/tests/Integration/BlockedRequestTest.php +++ b/tests/Integration/BlockedRequestTest.php @@ -259,6 +259,42 @@ public function testExpiredUnblockTokenIsNoPass() $this->assertBlocked('option=com_bfstop&view=tokenunblock&token='.str_repeat('ef', 20)); } + /** + * The pass a valid token gives is for the unblock view and nothing else: + * whatever else a request carries (another task or component, in any + * spelling - Joomla matches the task case-insensitively, and the backend + * decides on the component itself - or a token which isn't a plain + * string) must still be rejected. The one request which gets through is + * the unblock view itself. + */ + public function testUnblockTokenPassIsLimitedToTheUnblockView() + { + $this->configure(); + $blockId = $this->block(); + $token = (new DatabaseHelper($this->logger))->getNewUnblockToken($blockId, str_repeat('ab', 20)); + $this->assertBlocked('option=com_users&task=user.LOGIN&view=tokenunblock&token='.$token); + $this->assertBlocked('option=com_login&task=login&view=tokenunblock&token='.$token); + $this->assertBlocked('option=com_bfstop&task=user.login&view=tokenunblock&token='.$token); + $this->assertBlocked('option=COM_BFSTOP&view=tokenunblock&token='.$token); + $this->assertBlocked('option=com_bfstop&view=TOKENUNBLOCK&token='.$token); + $this->assertBlocked('option=com_bfstop&view=tokenunblock&token[]='.$token); + $this->assertNotBlocked('option=com_bfstop&view=tokenunblock&token='.$token); + } + + /** + * A token is bound to the blocks it was issued for: a valid token of + * another address's block doesn't open the unblock view, whatever the + * request looks like otherwise. + */ + public function testUnblockTokenOfAnotherAddressIsNoPassForTheUnblockView() + { + $this->configure(); + $this->block(); + $otherBlockId = $this->block('203.0.113.99'); + $otherToken = (new DatabaseHelper($this->logger))->getNewUnblockToken($otherBlockId, str_repeat('cd', 20)); + $this->assertBlocked('option=com_bfstop&view=tokenunblock&token='.$otherToken); + } + public function testRejectedRequestsAreCounted() { $this->configure(); From 8b3af124f3677a1fc197908b94e822ef8737515e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 20:46:27 +0000 Subject: [PATCH 02/12] Track the bundled GeoIP reader as a dependency and update it to 1.14.0 The MaxMind DB reader shipped in src/Helper/Geo is pinned in composer.json/ composer.lock, generated from there by tools/vendor-geoip.php (which only changes namespace, header and _JEXEC guard), and CI fails if the copy deviates from the lock. Dependabot and a weekly job report new releases and advisories. The reader is updated from 1.13.0 to 1.14.0, which limits the decoding of malformed or hostile database files. A test database writer lets the unit tests do real lookups with the vendored reader. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- .github/dependabot.yml | 16 ++ .github/workflows/ci.yml | 8 + .github/workflows/dependencies.yml | 43 ++++ .gitignore | 2 + CHANGELOG | 7 + README | 20 ++ SECURITY.md | 10 + composer.json | 20 ++ composer.lock | 89 ++++++++ src/Helper/Geo/Reader.php | 38 +++- src/Helper/Geo/Reader/Decoder.php | 205 +++++++++++++++-- .../Geo/Reader/InvalidDatabaseException.php | 2 +- src/Helper/Geo/Reader/Metadata.php | 2 +- src/Helper/Geo/Reader/Util.php | 8 +- tests/Support/MmdbBuilder.php | 114 ++++++++++ tests/Unit/GeoReaderTest.php | 86 +++++++ tools/vendor-geoip.php | 215 ++++++++++++++++++ 17 files changed, 849 insertions(+), 36 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/dependencies.yml create mode 100644 .gitignore create mode 100644 composer.json create mode 100644 composer.lock create mode 100644 tests/Support/MmdbBuilder.php create mode 100644 tests/Unit/GeoReaderTest.php create mode 100644 tools/vendor-geoip.php diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..9aba745 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,16 @@ +version: 2 +updates: + # the vendored GeoIP reader (src/Helper/Geo): a new release of + # maxmind-db/reader gets a pull request; its CI run fails until + # `composer install && composer vendor-geoip` has been run and the result + # committed - see README, "GeoIP reader" + - package-ecosystem: composer + directory: / + schedule: + interval: weekly + labels: [dependencies, geoip-reader] + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + labels: [dependencies] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e73a288..9f8bb16 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,6 +25,14 @@ jobs: 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: Composer manifest is valid + run: composer validate --strict + - name: Vendored GeoIP reader is the version locked in composer.lock + # src/Helper/Geo is generated by tools/vendor-geoip.php; if this fails + # after a dependency update, run `composer install && composer vendor-geoip` + run: | + composer install --no-interaction --no-progress + composer check-geoip - name: Release zip contains everything the manifest references run: | ./deploy.sh zip diff --git a/.github/workflows/dependencies.yml b/.github/workflows/dependencies.yml new file mode 100644 index 0000000..92f61d8 --- /dev/null +++ b/.github/workflows/dependencies.yml @@ -0,0 +1,43 @@ +name: Dependency check + +# The plugin ships a copy of the MaxMind DB reader (src/Helper/Geo, see README). +# This weekly job reports a newer release, and security advisories against the +# locked version, even if Dependabot is switched off. + +on: + schedule: + - cron: '23 5 * * 1' + workflow_dispatch: + +permissions: + contents: read + issues: write + +jobs: + geoip-reader: + name: maxmind-db/reader is current and free of advisories + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: shivammathur/setup-php@v2 + with: + php-version: '8.4' + coverage: none + - run: composer install --no-interaction --no-progress + - name: Vendored copy is the locked version + run: composer check-geoip + - name: Security advisories + run: composer audit + - name: Newer release available? + run: | + composer outdated --direct --strict maxmind-db/reader + - name: Open an issue + if: failure() + env: + GH_TOKEN: ${{ github.token }} + run: | + title="Update the vendored GeoIP reader (maxmind-db/reader)" + if [ -z "$(gh issue list --state open --search "in:title $title" --json number --jq '.[0].number')" ]; then + gh issue create --title "$title" --label dependencies \ + --body "The weekly dependency check failed: maxmind-db/reader has a newer release, a security advisory, or src/Helper/Geo differs from composer.lock. See the run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID. To update: \`composer update maxmind-db/reader && composer vendor-geoip\`, review the diff, run the tests, release both extensions." + fi diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..400b527 --- /dev/null +++ b/.gitignore @@ -0,0 +1,2 @@ +/vendor/ +*.zip diff --git a/CHANGELOG b/CHANGELOG index ed7b5a7..01de054 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,10 @@ +2.0.1 (unreleased) + - Security: the bundled MaxMind DB reader is updated from 1.13.0 to 1.14.0 + (limits for the decoding of malformed or hostile database files). Its + version is now tracked in composer.json/composer.lock; src/Helper/Geo is + generated from it (tools/vendor-geoip.php), CI verifies that, and + Dependabot plus a weekly job report new releases and advisories + 2.0.0 (2026-08-24) - Migrate to Joomla 5/6: PSR-4 namespaced classes (Codeling\Plugin\System\Bfstop), service provider, SubscriberInterface event handling diff --git a/README b/README index 929cd7e..c92b7ea 100644 --- a/README +++ b/README @@ -19,3 +19,23 @@ https://github.com/codeling/bfstop For any further questions, don't hesitate to contact me under info@bfstop.de +GeoIP reader (third-party code, keep it up to date!) +---------------------------------------------------- +The optional GeoIP features (country risk score, "Information for IP Address") +use a copy of the MaxMind DB reader (maxmind-db/reader, Apache-2.0), shipped in +src/Helper/Geo, because a Joomla! extension cannot rely on composer at install +time. It parses a database file with untrusted-format binary data, so it has to +be updated whenever MaxMind publishes a new release - releases of this plugin +should never ship an outdated copy. + + * Which version is shipped: composer.lock (package maxmind-db/reader); the + header of src/Helper/Geo/Reader.php names it too. + * How it is kept current: Dependabot and a weekly GitHub Actions job + (.github/workflows/dependencies.yml) report new releases and security + advisories; CI fails if src/Helper/Geo is not exactly the locked version. + * How to update: composer update maxmind-db/reader && composer vendor-geoip, + review the diff, run the tests, release plugin and component. + * The GeoLite2/GeoIP2 *database* (.mmdb) is separate: you download it and + configure its path in the component's settings. Keep it current as well + (MaxMind's licence requires updates; monthly at least), e.g. with + geoipupdate. diff --git a/SECURITY.md b/SECURITY.md index 6350ec2..e90f7df 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -35,3 +35,13 @@ Settings are changed by users with the `core.admin` permission for `com_bfstop`. They can, among other things, choose the directory in which the plugin writes its `.htaccess` file, so grant that permission only to people you trust with the web server's configuration for your site. + +## Third-party code + +The plugin ships a copy of MaxMind's `maxmind-db/reader` in `src/Helper/Geo` +(the exact version is pinned in `composer.lock`). A weekly CI job and +Dependabot report new releases and security advisories, and CI fails if the +copy deviates from the pinned version. Releases are only made with the latest +reader; see the "GeoIP reader" section of the README for how to update it. If +you notice that the shipped version is behind, please report it like any other +vulnerability. diff --git a/composer.json b/composer.json new file mode 100644 index 0000000..1334886 --- /dev/null +++ b/composer.json @@ -0,0 +1,20 @@ +{ + "name": "codeling/bfstop", + "description": "Brute Force Stop for Joomla! - this file only tracks the third-party code vendored into the plugin package (src/Helper/Geo), see README", + "type": "project", + "license": "GPL-3.0-or-later", + "require": { + "php": ">=8.1", + "maxmind-db/reader": "^1.14.0" + }, + "config": { + "sort-packages": true, + "platform": { + "php": "8.1.0" + } + }, + "scripts": { + "vendor-geoip": "@php tools/vendor-geoip.php", + "check-geoip": "@php tools/vendor-geoip.php --check" + } +} diff --git a/composer.lock b/composer.lock new file mode 100644 index 0000000..b271652 --- /dev/null +++ b/composer.lock @@ -0,0 +1,89 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state", + "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", + "This file is @generated automatically" + ], + "content-hash": "d9f1ceb99c1383647fb2b2cf3b25fd40", + "packages": [ + { + "name": "maxmind-db/reader", + "version": "v1.14.0", + "source": { + "type": "git", + "url": "https://github.com/maxmind/MaxMind-DB-Reader-php.git", + "reference": "f3c92f68b3bec42a9aa780368399e03dc6b91e89" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/maxmind/MaxMind-DB-Reader-php/zipball/f3c92f68b3bec42a9aa780368399e03dc6b91e89", + "reference": "f3c92f68b3bec42a9aa780368399e03dc6b91e89", + "shasum": "" + }, + "require": { + "php": ">=7.2" + }, + "conflict": { + "ext-maxminddb": "<1.14.0 || >=2.0.0" + }, + "require-dev": { + "friendsofphp/php-cs-fixer": "3.*", + "phpstan/phpstan": "*", + "phpunit/phpunit": ">=8.0.0,<10.0.0", + "squizlabs/php_codesniffer": "4.*", + "symfony/polyfill-php80": "^1.33" + }, + "suggest": { + "ext-bcmath": "bcmath or gmp is required for decoding larger integers with the pure PHP decoder", + "ext-gmp": "bcmath or gmp is required for decoding larger integers with the pure PHP decoder", + "ext-maxminddb": "A C-based database decoder that provides significantly faster lookups", + "maxmind-db/reader-ext": "C extension for significantly faster IP lookups (install via PIE: pie install maxmind-db/reader-ext)" + }, + "type": "library", + "autoload": { + "psr-4": { + "MaxMind\\Db\\": "src/MaxMind/Db" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "Apache-2.0" + ], + "authors": [ + { + "name": "Gregory J. Oschwald", + "email": "goschwald@maxmind.com", + "homepage": "https://www.maxmind.com/en/home" + } + ], + "description": "MaxMind DB Reader API", + "homepage": "https://github.com/maxmind/MaxMind-DB-Reader-php", + "keywords": [ + "database", + "geoip", + "geoip2", + "geolocation", + "maxmind" + ], + "support": { + "issues": "https://github.com/maxmind/MaxMind-DB-Reader-php/issues", + "source": "https://github.com/maxmind/MaxMind-DB-Reader-php/tree/v1.14.0" + }, + "time": "2026-09-10T22:02:38+00:00" + } + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": {}, + "prefer-stable": false, + "prefer-lowest": false, + "platform": { + "php": ">=8.1" + }, + "platform-dev": {}, + "platform-overrides": { + "php": "8.1.0" + }, + "plugin-api-version": "2.6.0" +} diff --git a/src/Helper/Geo/Reader.php b/src/Helper/Geo/Reader.php index b2ca452..caea3b8 100644 --- a/src/Helper/Geo/Reader.php +++ b/src/Helper/Geo/Reader.php @@ -5,12 +5,14 @@ * @copyright (C) Bernhard Froehler * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html * - * This file is vendored from maxmind-db/reader + * This file is vendored from maxmind-db/reader 1.14.0 * (https://github.com/maxmind/MaxMind-DB-Reader-php), used here as a small, * dependency-free (core PHP only) reader for MaxMind .mmdb GeoIP databases, * so BFStop can look up an IP's country/city locally instead of relying on a - * third-party web API (see issues #76 and #169). Only the namespace has been - * changed; the decoding logic is unmodified. + * third-party web API (see issues #76 and #169). Only the namespace, this + * header and the _JEXEC guard have been changed; the decoding logic is + * unmodified. Do not edit by hand: tools/vendor-geoip.php regenerates it from + * composer.lock (see README, "GeoIP reader"). * * Original work Copyright (C) MaxMind, Inc., licensed under the * Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0). @@ -63,6 +65,11 @@ class Reader */ private $fileHandle; + /** + * @var bool + */ + private $lookupInProgress = false; + /** * @var int */ @@ -136,7 +143,7 @@ public function __construct(string $database) * * @param string $ipAddress the IP address to look up * - * @throws \BadMethodCallException if this method is called on a closed database + * @throws \BadMethodCallException if the database is closed or another lookup is in progress * @throws \InvalidArgumentException if something other than a single IP address is passed to the method * @throws InvalidDatabaseException * if the database is invalid or there is an error reading @@ -161,7 +168,7 @@ public function get(string $ipAddress) * * @param string $ipAddress the IP address to look up * - * @throws \BadMethodCallException if this method is called on a closed database + * @throws \BadMethodCallException if the database is closed or another lookup is in progress * @throws \InvalidArgumentException if something other than a single IP address is passed to the method * @throws InvalidDatabaseException * if the database is invalid or there is an error reading @@ -184,12 +191,25 @@ public function getWithPrefixLen(string $ipAddress): array ); } - [$pointer, $prefixLen] = $this->findAddressInTree($ipAddress); - if ($pointer === 0) { - return [null, $prefixLen]; + if ($this->lookupInProgress) { + throw new \BadMethodCallException( + 'A lookup is already in progress on this reader. Use a separate reader for nested lookups.' + ); } + // A stream wrapper can call back into this reader during a read. + // Reject nested lookups before they can move the shared stream. + $this->lookupInProgress = true; + + try { + [$pointer, $prefixLen] = $this->findAddressInTree($ipAddress); + if ($pointer === 0) { + return [null, $prefixLen]; + } - return [$this->resolveDataPointer($pointer), $prefixLen]; + return [$this->resolveDataPointer($pointer), $prefixLen]; + } finally { + $this->lookupInProgress = false; + } } /** diff --git a/src/Helper/Geo/Reader/Decoder.php b/src/Helper/Geo/Reader/Decoder.php index 7d46f50..9e99d8f 100644 --- a/src/Helper/Geo/Reader/Decoder.php +++ b/src/Helper/Geo/Reader/Decoder.php @@ -5,7 +5,7 @@ * @copyright (C) Bernhard Froehler * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html * - * Vendored from maxmind-db/reader (see Geo/Reader.php for details). + * Vendored from maxmind-db/reader 1.14.0 (see Geo/Reader.php for details). * Original work Copyright (C) MaxMind, Inc., licensed under the * Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0). **/ @@ -42,6 +42,29 @@ class Decoder */ private $switchByteOrder; + /** + * Remaining decoded-value allowance for the current decode() call. + * + * @var int + */ + private $budget = 0; + + /** + * Remaining string and bytes payload allowance for the current decode() + * call. + * + * @var int + */ + private $byteBudget = 0; + + /** + * Stream position after the last read of the current decode() call, or -1 + * when unknown. + * + * @var int + */ + private $position = -1; + private const _EXTENDED = 0; private const _POINTER = 1; private const _UTF8_STRING = 2; @@ -59,6 +82,30 @@ class Decoder private const _BOOLEAN = 14; private const _FLOAT = 15; + // Per-lookup decode limits recommended by the MaxMind DB specification. The + // depth limit stops pointer cycles and over-deep data. The value limit + // stops a pointer fan-out, where nested pointers to shared targets would + // otherwise cost 2**depth decode operations. The count follows the + // specification's flat rule: the root is one value, each array and map + // charges its declared children (a map entry costs two, key and value), + // and a pointer costs nothing beyond the value it resolves to, which its + // container already charged. + private const MAX_DEPTH = 512; + private const MAX_VALUES = 1 << 16; + + // The value limit alone does not stop payload amplification: an array of + // pointers to one large string or bytes value keeps the value count low + // while forcing the reader to copy the target once per pointer. This + // second limit bounds the total string and bytes payload copied for one + // lookup to 2 MiB. Re-decoding a shared target charges its payload again. + private const MAX_PAYLOAD_BYTES = 1 << 21; + + // A fixed-width scalar (a float, double, or integer) never needs more than + // 16 bytes (the width of a uint128). A larger declared size is either + // corrupt or an attempt to amplify the read of an oversized variable-length + // integer, so it is rejected before the bytes are materialized. + private const MAX_SCALAR_BYTES = 16; + /** * @param resource $fileStream */ @@ -80,7 +127,25 @@ public function __construct( */ public function decode(int $offset): array { - $ctrlByte = \ord(Util::read($this->fileStream, $offset, 1)); + // Reset both budgets for each lookup. Charge the root value here and + // container children when entering each container. Reader prevents + // nested lookups from moving its stream during a decode. + $this->budget = self::MAX_VALUES - 1; + $this->byteBudget = self::MAX_PAYLOAD_BYTES; + + // Other readers of the stream, such as the search tree walk, may have + // moved it since the last call. + $this->position = -1; + + return $this->decodeWithBudget($offset, 0); + } + + /** + * @return array + */ + private function decodeWithBudget(int $offset, int $depth, bool $allowPointer = true): array + { + $ctrlByte = \ord($this->read($offset, 1)); ++$offset; $type = $ctrlByte >> 5; @@ -89,6 +154,12 @@ public function decode(int $offset): array // use the size to determine the length of the pointer and then follow // it. if ($type === self::_POINTER) { + if (!$allowPointer) { + throw new InvalidDatabaseException( + 'The MaxMind DB file contains a pointer to another pointer' + ); + } + [$pointer, $offset] = $this->decodePointer($ctrlByte, $offset); // for unit testing @@ -96,13 +167,22 @@ public function decode(int $offset): array return [$pointer]; } - [$result] = $this->decode($pointer); + if ($depth >= self::MAX_DEPTH) { + throw new InvalidDatabaseException( + 'The MaxMind DB file exceeds the maximum depth' + ); + } + + // The value at the pointer's position was charged by its containing + // array or map, so the target costs nothing more. Only the depth + // grows. + [$result] = $this->decodeWithBudget($pointer, $depth + 1, false); return [$result, $offset]; } if ($type === self::_EXTENDED) { - $nextByte = \ord(Util::read($this->fileStream, $offset, 1)); + $nextByte = \ord($this->read($offset, 1)); $type = $nextByte + 7; @@ -120,7 +200,7 @@ public function decode(int $offset): array [$size, $offset] = $this->sizeFromCtrlByte($ctrlByte, $offset); - return $this->decodeByType($type, $offset, $size); + return $this->decodeByType($type, $offset, $size, $depth); } /** @@ -128,27 +208,54 @@ public function decode(int $offset): array * * @return array{0:mixed, 1:int} */ - private function decodeByType(int $type, int $offset, int $size): array + private function decodeByType(int $type, int $offset, int $size, int $depth): array { switch ($type) { case self::_MAP: - return $this->decodeMap($size, $offset); + return $this->decodeMap($size, $offset, $depth); case self::_ARRAY: - return $this->decodeArray($size, $offset); + return $this->decodeArray($size, $offset, $depth); case self::_BOOLEAN: return [$this->decodeBoolean($size), $offset]; + + case self::_BYTES: + case self::_UTF8_STRING: + // A string or bytes value is copied into a native string, so N + // pointers to one large value copy N times its length. Charge + // the payload against the byte budget wherever it is decoded, + // including inline inside a pointed-to container, so a shared + // target recharges each time it is followed. Compare before + // subtracting so an oversized declared size cannot drive the + // budget negative. A total exactly at the limit is allowed. + if ($size > $this->byteBudget) { + throw new InvalidDatabaseException( + 'The MaxMind DB file exceeds the maximum payload size' + ); + } + $this->byteBudget -= $size; + + return [$this->read($offset, $size), $offset + $size]; + } + + // The remaining valid types are fixed-width scalars, none wider than a + // uint128. A few other control bytes also reach here: the container + // (12) and end-marker (13) types, and any unknown extended type. The + // size guard below rejects one that declares an oversized size, and the + // default case at the end of the switch rejects the rest. Reject an + // oversized declared size before materializing the bytes, so an + // oversized variable-length integer cannot amplify the read. + if ($size > self::MAX_SCALAR_BYTES) { + throw new InvalidDatabaseException( + "The MaxMind DB file's data section contains bad data (unknown data type or corrupt data)" + ); } $newOffset = $offset + $size; - $bytes = Util::read($this->fileStream, $offset, $size); + $bytes = $this->read($offset, $size); switch ($type) { - case self::_BYTES: - case self::_UTF8_STRING: - return [$bytes, $newOffset]; - case self::_DOUBLE: $this->verifySize(8, $size); @@ -175,6 +282,36 @@ private function decodeByType(int $type, int $offset, int $size): array } } + /** + * Reads from the stream, seeking only when the read does not continue where + * the previous one ended. + * + * @param int<0, max> $numberOfBytes + */ + private function read(int $offset, int $numberOfBytes): string + { + if ($numberOfBytes === 0) { + return ''; + } + + $stream = $this->fileStream; + if ($offset !== $this->position && fseek($stream, $offset) !== 0) { + $this->position = -1; + + throw new InvalidDatabaseException('The MaxMind DB file contains bad data'); + } + + $value = fread($stream, $numberOfBytes); + if ($value === false || \strlen($value) !== $numberOfBytes) { + $this->position = -1; + + throw new InvalidDatabaseException('The MaxMind DB file contains bad data'); + } + $this->position = $offset + $numberOfBytes; + + return $value; + } + private function verifySize(int $expected, int $actual): void { if ($expected !== $actual) { @@ -184,15 +321,40 @@ private function verifySize(int $expected, int $actual): void } } + /** + * Charges declared children before decoding them. An oversized container + * fails before any child is read. Each visit to a shared container charges + * its children again, which bounds pointer fan-out. + */ + private function enterContainer( + int $size, + int $depth, + int $valuesPerEntry = 1 + ): void { + if ($depth >= self::MAX_DEPTH) { + throw new InvalidDatabaseException( + 'The MaxMind DB file exceeds the maximum depth' + ); + } + if ($size > intdiv($this->budget, $valuesPerEntry)) { + throw new InvalidDatabaseException( + 'The MaxMind DB file exceeds the maximum number of values' + ); + } + $this->budget -= $size * $valuesPerEntry; + } + /** * @return array{0:array, 1:int} */ - private function decodeArray(int $size, int $offset): array + private function decodeArray(int $size, int $offset, int $depth): array { + $this->enterContainer($size, $depth); + $array = []; for ($i = 0; $i < $size; ++$i) { - [$value, $offset] = $this->decode($offset); + [$value, $offset] = $this->decodeWithBudget($offset, $depth + 1); $array[] = $value; } @@ -270,13 +432,16 @@ private function decodeInt32(string $bytes, int $size): int /** * @return array{0:array, 1:int} */ - private function decodeMap(int $size, int $offset): array + private function decodeMap(int $size, int $offset, int $depth): array { + // A map entry decodes a key and a value, so it costs two values. + $this->enterContainer($size, $depth, 2); + $map = []; for ($i = 0; $i < $size; ++$i) { - [$key, $offset] = $this->decode($offset); - [$value, $offset] = $this->decode($offset); + [$key, $offset] = $this->decodeWithBudget($offset, $depth + 1); + [$value, $offset] = $this->decodeWithBudget($offset, $depth + 1); $map[$key] = $value; } @@ -290,7 +455,7 @@ private function decodePointer(int $ctrlByte, int $offset): array { $pointerSize = (($ctrlByte >> 3) & 0x3) + 1; - $buffer = Util::read($this->fileStream, $offset, $pointerSize); + $buffer = $this->read($offset, $pointerSize); $offset += $pointerSize; switch ($pointerSize) { @@ -416,7 +581,7 @@ private function sizeFromCtrlByte(int $ctrlByte, int $offset): array } $bytesToRead = $size - 28; - $bytes = Util::read($this->fileStream, $offset, $bytesToRead); + $bytes = $this->read($offset, $bytesToRead); if ($size === 29) { $size = 29 + \ord($bytes); diff --git a/src/Helper/Geo/Reader/InvalidDatabaseException.php b/src/Helper/Geo/Reader/InvalidDatabaseException.php index 6132048..cf30bbd 100644 --- a/src/Helper/Geo/Reader/InvalidDatabaseException.php +++ b/src/Helper/Geo/Reader/InvalidDatabaseException.php @@ -5,7 +5,7 @@ * @copyright (C) Bernhard Froehler * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html * - * Vendored from maxmind-db/reader (see Geo/Reader.php for details). + * Vendored from maxmind-db/reader 1.14.0 (see Geo/Reader.php for details). * Original work Copyright (C) MaxMind, Inc., licensed under the * Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0). **/ diff --git a/src/Helper/Geo/Reader/Metadata.php b/src/Helper/Geo/Reader/Metadata.php index 5b16ed6..12da363 100644 --- a/src/Helper/Geo/Reader/Metadata.php +++ b/src/Helper/Geo/Reader/Metadata.php @@ -5,7 +5,7 @@ * @copyright (C) Bernhard Froehler * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html * - * Vendored from maxmind-db/reader (see Geo/Reader.php for details). + * Vendored from maxmind-db/reader 1.14.0 (see Geo/Reader.php for details). * Original work Copyright (C) MaxMind, Inc., licensed under the * Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0). **/ diff --git a/src/Helper/Geo/Reader/Util.php b/src/Helper/Geo/Reader/Util.php index 4368d45..e00224b 100644 --- a/src/Helper/Geo/Reader/Util.php +++ b/src/Helper/Geo/Reader/Util.php @@ -5,7 +5,7 @@ * @copyright (C) Bernhard Froehler * @license GNU/GPLv3 http://www.gnu.org/licenses/gpl-3.0.html * - * Vendored from maxmind-db/reader (see Geo/Reader.php for details). + * Vendored from maxmind-db/reader 1.14.0 (see Geo/Reader.php for details). * Original work Copyright (C) MaxMind, Inc., licensed under the * Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0). **/ @@ -30,10 +30,8 @@ public static function read($stream, int $offset, int $numberOfBytes): string if (fseek($stream, $offset) === 0) { $value = fread($stream, $numberOfBytes); - // We check that the number of bytes read is equal to the number - // asked for. We use ftell as getting the length of $value is - // much slower. - if ($value !== false && ftell($stream) - $offset === $numberOfBytes) { + // Check that the number of bytes read is the number asked for. + if ($value !== false && \strlen($value) === $numberOfBytes) { return $value; } } diff --git a/tests/Support/MmdbBuilder.php b/tests/Support/MmdbBuilder.php new file mode 100644 index 0000000..31d3f18 --- /dev/null +++ b/tests/Support/MmdbBuilder.php @@ -0,0 +1,114 @@ + record (nested + * arrays of strings and integers); the networks must + * not overlap + * @return string the contents of the database file + */ + public static function build(array $entries): string + { + $nodes = array(array(null, null)); + $data = ''; + foreach ($entries as $cidr => $record) + { + [$ip, $bits] = explode('/', $cidr); + $bits = (int) $bits; + $address = inet_pton($ip); + $offset = strlen($data); + $data .= self::encode($record); + $node = 0; + for ($i = 0; $i < $bits; ++$i) + { + $bit = (ord($address[$i >> 3]) >> (7 - ($i & 7))) & 1; + if ($i === $bits - 1) + { + $nodes[$node][$bit] = array('data' => $offset); + break; + } + if (!is_int($nodes[$node][$bit])) + { + $nodes[] = array(null, null); + $nodes[$node][$bit] = count($nodes) - 1; + } + $node = $nodes[$node][$bit]; + } + } + $nodeCount = count($nodes); + $tree = ''; + foreach ($nodes as $node) + { + foreach ($node as $record) + { + $value = $record === null ? $nodeCount + : (is_int($record) ? $record : $nodeCount + 16 + $record['data']); + $tree .= substr(pack('N', $value), 1); + } + } + $metadata = array( + 'binary_format_major_version' => 2, + 'binary_format_minor_version' => 0, + 'build_epoch' => 1, + 'database_type' => 'BFStop-Test', + 'description' => array('en' => 'test database'), + 'ip_version' => 4, + 'languages' => array('en'), + 'node_count' => $nodeCount, + 'record_size' => 24, + ); + return $tree.str_repeat("\0", 16).$data."\xAB\xCD\xEFMaxMind.com".self::encode($metadata); + } + + private static function size(int $type, int $size): string + { + if ($size >= 29) + { + throw new \InvalidArgumentException('values this big are not needed by the tests'); + } + return chr(($type << 5) | $size); + } + + private static function encode($value): string + { + if (is_string($value)) + { + return self::size(2, strlen($value)).$value; + } + if (is_int($value)) + { + $bytes = $value === 0 ? '' : ltrim(pack('N', $value), "\0"); + return self::size(6, strlen($bytes)).$bytes; + } + if (is_array($value) && array_is_list($value)) + { + // extended type 11 (array): type 0 in the control byte, then type - 7 + $out = chr(count($value))."\x04"; + foreach ($value as $item) + { + $out .= self::encode($item); + } + return $out; + } + $out = self::size(7, count($value)); + foreach ($value as $key => $item) + { + $out .= self::encode((string) $key).self::encode($item); + } + return $out; + } +} diff --git a/tests/Unit/GeoReaderTest.php b/tests/Unit/GeoReaderTest.php new file mode 100644 index 0000000..b4d6d50 --- /dev/null +++ b/tests/Unit/GeoReaderTest.php @@ -0,0 +1,86 @@ +file = tempnam(sys_get_temp_dir(), 'bfstop-mmdb'); + file_put_contents($this->file, MmdbBuilder::build(array( + '203.0.113.0/24' => array('country' => array('iso_code' => 'DE', 'names' => array('en' => 'Germany')), + 'location' => array('latitude' => 5, 'longitude' => 7)), + '198.51.100.0/25' => array('country' => array('iso_code' => 'AT')), + ))); + } + + protected function tearDown(): void + { + @unlink($this->file); + } + + public function testFindsTheRecordOfAnAddress() + { + $reader = new Reader($this->file); + $record = $reader->get('203.0.113.77'); + $this->assertSame('DE', $record['country']['iso_code']); + $this->assertSame('Germany', $record['country']['names']['en']); + $this->assertSame(5, $record['location']['latitude']); + $this->assertSame('AT', $reader->get('198.51.100.9')['country']['iso_code']); + $reader->close(); + } + + public function testAddressesWithoutRecordGiveNull() + { + $reader = new Reader($this->file); + $this->assertNull($reader->get('198.51.100.200')); + $this->assertNull($reader->get('192.0.2.1')); + $reader->close(); + } + + public function testReportsTheDatabaseMetadata() + { + $reader = new Reader($this->file); + $this->assertSame('BFStop-Test', $reader->metadata()->databaseType); + $this->assertSame(4, $reader->metadata()->ipVersion); + $reader->close(); + } + + public function testRejectsAddressesOfAnotherFamily() + { + $reader = new Reader($this->file); + $this->expectException(\InvalidArgumentException::class); + $reader->get('2001:db8::1'); + } + + public function testRejectsGarbage() + { + file_put_contents($this->file, str_repeat('not a maxmind database', 50)); + $this->expectException(InvalidDatabaseException::class); + new Reader($this->file); + } + + public function testRejectsATruncatedDatabase() + { + file_put_contents($this->file, substr(file_get_contents($this->file), 0, 40)); + $this->expectException(InvalidDatabaseException::class); + new Reader($this->file); + } +} diff --git a/tools/vendor-geoip.php b/tools/vendor-geoip.php new file mode 100644 index 0000000..5423a03 --- /dev/null +++ b/tools/vendor-geoip.php @@ -0,0 +1,215 @@ + $mainHeader); +foreach (glob($source.'/Reader/*.php') as $file) +{ + $files['Reader/'.basename($file)] = $shortHeader; +} +if (!is_file($source.'/Reader.php') || count($files) < 2) +{ + fail("No reader sources found in $source."); +} +foreach ($files as $relative => $header) +{ + $generated[$relative] = transform(file_get_contents($source.'/'.$relative), $header, $relative); +} +$generated['LICENSE'] = file_get_contents($vendorDir.'/maxmind-db/reader/LICENSE'); + +$differences = array(); +foreach ($generated as $relative => $content) +{ + $path = $target.'/'.$relative; + if (!is_file($path) || file_get_contents($path) !== $content) + { + $differences[] = $relative; + if (!$check) + { + if (!is_dir(dirname($path))) + { + mkdir(dirname($path), 0777, true); + } + file_put_contents($path, $content); + } + } +} +// files which upstream no longer has +$existing = array_merge(glob($target.'/*.php'), glob($target.'/Reader/*.php')); +foreach ($existing as $path) +{ + $relative = substr($path, strlen($target) + 1); + if (!isset($generated[$relative])) + { + $differences[] = "$relative (not in maxmind-db/reader $version)"; + if (!$check) + { + unlink($path); + } + } +} + +if ($check) +{ + if ($differences) + { + fwrite(STDERR, "src/Helper/Geo is not maxmind-db/reader $version as locked in composer.lock:\n ". + implode("\n ", $differences)."\nRun 'composer vendor-geoip' and commit the result.\n"); + exit(1); + } + echo "src/Helper/Geo is maxmind-db/reader $version, as locked in composer.lock.\n"; + exit(0); +} +echo $differences + ? "Updated src/Helper/Geo to maxmind-db/reader $version: ".implode(', ', $differences)."\n" + : "src/Helper/Geo already is maxmind-db/reader $version.\n"; From 792d0c23db46c40279dcaa43dc4e156bd9902218 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 20:48:39 +0000 Subject: [PATCH 03/12] Harden request parameter handling and what visitors can write to logs and mails - Request parameters the block logic compares are read as strings; array values (option[]=x) no longer end in a TypeError and count as 'something else', never as 'not given'. - A failed login without username in the credentials is recorded instead of raising a warning. - A block that could not be stored is not announced or mailed, and does not touch the .htaccess file. - Control characters in usernames (and therefore line breaks) are replaced in log entries and in the mailed lists of failed logins, so a visitor cannot forge lines there. - Remove DatabaseHelper::myCheckDBError(), which used removed Joomla 3 API. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- src/Extension/Bfstop.php | 45 +++++++++++++++++------- src/Helper/DatabaseHelper.php | 13 ++----- src/Helper/LoggerHelper.php | 12 ++++++- src/Helper/NotifierHelper.php | 2 +- tests/Integration/BlockedRequestTest.php | 16 +++++++++ tests/Integration/DatabaseHelperTest.php | 8 +++++ tests/Integration/PluginEventsTest.php | 9 +++++ tests/Unit/SingleLineTest.php | 27 ++++++++++++++ 8 files changed, 106 insertions(+), 26 deletions(-) create mode 100644 tests/Unit/SingleLineTest.php diff --git a/src/Extension/Bfstop.php b/src/Extension/Bfstop.php index 95aa6d4..f12562d 100644 --- a/src/Extension/Bfstop.php +++ b/src/Extension/Bfstop.php @@ -64,6 +64,19 @@ private function getStringParam($paramName, $default) return $this->params->get($paramName, $default); } + /** + * A request parameter as a string: "option[]=x" and the like give arrays, + * which strcmp() and friends don't take (a TypeError on PHP 8, i.e. an + * error page instead of the intended answer). Such a value is returned as + * a NUL character: not empty - so it doesn't pass for "parameter not + * given" - and not equal to anything the parameter can legitimately be. + */ + private function requestString($name, $filter = 'cmd') + { + $value = $this->getApplication()->input->get($name, '', $filter); + return is_string($value) ? $value : "\0"; + } + private static function endsWith($haystack, $needle) { $length = strlen($needle); @@ -155,6 +168,15 @@ private function block($logEntry, $duration) $this->getRealDurationFromDBDuration($this->getIntParam('checkInterval', NotifierHelper::$ONE_DAY)), $logEntry->ipaddress, $logEntry->username, $logEntry->logtime); $id = $this->mydb->blockIP($logEntry, $duration, $usehtaccess, $htaccessPath); + if ($id < 1) + { + // nothing is blocked: neither tell the administrators that + // something was, nor mail an unblock link for a block which + // doesn't exist + $this->logger->log('Could not block IP address '.$logEntry->ipaddress. + ', see the previous errors', Log::ERROR); + return; + } $this->logger->log('Inserted IP address '.$logEntry->ipaddress. ' into block list', Log::INFO); @@ -461,7 +483,7 @@ public function onUserLoginFailure($event) $this->logger->log('Ignoring failed login by allowed address '.$ipAddress, Log::INFO); return; } - $username = mb_strimwidth($user['username'], 0, 150, "..."); + $username = mb_strimwidth((string) ($user['username'] ?? ''), 0, 150, "..."); $riskScore = RiskHelper::computeScore($this->mydb, $this->logger, $this->params, $ipAddress, $username); $logEntry = new \stdClass(); @@ -518,7 +540,7 @@ public function onUserLogin($event) } $info = new \stdClass(); $info->ipaddress = IpHelper::getAddress($this->logger); - $info->username = $user['username']; + $info->username = (string) ($user['username'] ?? ''); $this->logger->log('Successful login by '.$info->username. ' from IP address '.$info->ipaddress, Log::DEBUG); $this->mydb->successfulLogin($info, $this->trackedAddress($info->ipaddress)); @@ -536,14 +558,13 @@ public function onUserLogin($event) */ private function isUnblockRequest($blockIds) { - $input = $this->getApplication()->input; - if (strcmp($input->getCmd('option', ''), 'com_bfstop') != 0 || - strcmp($input->getCmd('view', ''), 'tokenunblock') != 0 || - $input->getCmd('task', '') !== '') + if (strcmp($this->requestString('option'), 'com_bfstop') != 0 || + strcmp($this->requestString('view'), 'tokenunblock') != 0 || + $this->requestString('task') !== '') { return false; } - $token = $input->getString('token', ''); + $token = $this->requestString('token', 'string'); $result = $this->mydb->unblockTokenValidForBlocks($token, $blockIds); if ($result) { @@ -567,9 +588,8 @@ private function isUnblockRequest($blockIds) */ private function isPasswordRecoveryRequest() { - $input = $this->getApplication()->input; - $option = $input->getCmd('option', ''); - $view = $input->getCmd('view', ''); + $option = $this->requestString('option'); + $view = $this->requestString('view'); $result = (strcmp($option, 'com_users') == 0 && (strcmp($view, 'reset') == 0 || strcmp($view, 'remind') == 0)); if ($result) @@ -593,9 +613,8 @@ private function isPasswordRecoveryRequest() */ private function isLoginAttemptRequest() { - $input = $this->getApplication()->input; - $option = $input->getCmd('option', ''); - $task = $input->getCmd('task', ''); + $option = $this->requestString('option'); + $task = $this->requestString('task'); $result = (strcmp($option, 'com_users') == 0 && (strcmp($task, 'user.login') == 0 || strcmp($task, 'login') == 0)) || (strcmp($option, 'com_login') == 0 && strcmp($task, 'login') == 0); diff --git a/src/Helper/DatabaseHelper.php b/src/Helper/DatabaseHelper.php index dcf0d7e..3a66deb 100644 --- a/src/Helper/DatabaseHelper.php +++ b/src/Helper/DatabaseHelper.php @@ -76,15 +76,6 @@ public function __construct(LoggerHelper $logger) $this->logger = $logger; } - public function myCheckDBError() - { - $errNum = $this->db->getErrorNum(); - if ($errNum != 0) - { - $this->logger->log("Database error (#$errNum) occured: ".$this->db->getErrorMsg(), Log::ERROR); - } - } - public function eventsInInterval( $interval, $time, @@ -188,7 +179,7 @@ public function getFormattedFailedList($ipAddress, $curTime, $interval) str_repeat("-", 97)."\n"; foreach ($entries as $entry) { - $result .= str_pad($entry->username, 25)." ". + $result .= str_pad(LoggerHelper::singleLine($entry->username), 25)." ". str_pad($entry->ipaddress, 15)." ". str_pad($entry->logtime, 20)." ". str_pad($this->getClientString($entry->origin), 8)."\n"; @@ -310,7 +301,7 @@ public function blockIP($logEntry, $duration, $usehtaccess, $htaccessPath) if (!$this->db->insertObject('#__bfstop_bannedip', $blockEntry, 'id')) { $this->logger->log('Insert block entry failed!', Log::ERROR); - $blockEntry->id = -1; + return -1; } $this->setFailedLoginHandled($logEntry, false); if ($usehtaccess) diff --git a/src/Helper/LoggerHelper.php b/src/Helper/LoggerHelper.php index 01dd99c..f7a4c18 100644 --- a/src/Helper/LoggerHelper.php +++ b/src/Helper/LoggerHelper.php @@ -201,6 +201,16 @@ public static function deleteLogFiles() return $deleted; } + /** + * $text with control characters (line breaks above all) replaced by spaces: + * values a visitor chooses - like the username of a failed login - must not + * be able to forge further lines in the log or in a mail. + */ + public static function singleLine($text) + { + return preg_replace('/[\x00-\x1f\x7f]+/', ' ', (string) $text); + } + public function isEnabled($priority = Log::ERROR) { return $priority <= $this->log_level; @@ -210,7 +220,7 @@ public function log($msg, $priority) { if ($this->isEnabled($priority)) { - Log::add($msg, $priority, self::LogCategory); + Log::add(self::singleLine($msg), $priority, self::LogCategory); } } } diff --git a/src/Helper/NotifierHelper.php b/src/Helper/NotifierHelper.php index 60a5478..966680b 100644 --- a/src/Helper/NotifierHelper.php +++ b/src/Helper/NotifierHelper.php @@ -95,7 +95,7 @@ public function getFailedLoginBody($logEntry) $this->getSiteName(), Uri::root())."\n"; $bodys .= str_pad(Text::_('PLG_SYSTEM_BFSTOP_USERNAME').":", 15). - $logEntry->username."\n"; + LoggerHelper::singleLine($logEntry->username)."\n"; $bodys .= str_pad(Text::_('PLG_SYSTEM_BFSTOP_IPADDRESS').":", 15). $logEntry->ipaddress."\n"; $bodys .= str_pad(Text::_('PLG_SYSTEM_BFSTOP_DATETIME').":", 15). diff --git a/tests/Integration/BlockedRequestTest.php b/tests/Integration/BlockedRequestTest.php index 3341592..6bf1047 100644 --- a/tests/Integration/BlockedRequestTest.php +++ b/tests/Integration/BlockedRequestTest.php @@ -295,6 +295,22 @@ public function testUnblockTokenOfAnotherAddressIsNoPassForTheUnblockView() $this->assertBlocked('option=com_bfstop&view=tokenunblock&token='.$otherToken); } + /** + * Parameters given as arrays ("option[]=x") are no valid values: they must + * neither break the plugin nor get a request past the block. + */ + public function testArrayParametersDoNotBreakTheBlock() + { + $this->configure(); + $blockId = $this->block(); + $token = (new DatabaseHelper($this->logger))->getNewUnblockToken($blockId, str_repeat('ab', 20)); + $this->assertBlocked('option[]=com_bfstop&view=tokenunblock&token='.$token); + $this->assertBlocked('option=com_bfstop&view[]=tokenunblock&token='.$token); + $this->assertBlocked('option=com_bfstop&view=tokenunblock&task[]=display&token='.$token); + $this->assertBlocked('option[]=com_users&view[]=reset'); + $this->assertBlocked('option[]=com_users&task[]=user.login'); + } + public function testRejectedRequestsAreCounted() { $this->configure(); diff --git a/tests/Integration/DatabaseHelperTest.php b/tests/Integration/DatabaseHelperTest.php index c8c386a..2fb22b8 100644 --- a/tests/Integration/DatabaseHelperTest.php +++ b/tests/Integration/DatabaseHelperTest.php @@ -350,4 +350,12 @@ public function testTrimUsernameStatsKeepsTheMostAttackedAndMostRecent() $this->db->setQuery('SELECT username FROM #__bfstop_username_stats ORDER BY username'); $this->assertSame(array('few-recent', 'many-old', 'some'), $this->db->loadColumn()); } + + public function testLineBreaksInUsernamesDoNotForgeLinesInTheMailedList() + { + $this->failedLogin('203.0.113.5', "bob\n[forged] 203.0.113.99", 1); + $list = $this->helper->getFormattedFailedList('203.0.113.5', self::minutesAgo(0), 60); + // header, separator and the one entry + $this->assertCount(3, array_filter(explode("\n", $list), 'strlen')); + } } diff --git a/tests/Integration/PluginEventsTest.php b/tests/Integration/PluginEventsTest.php index edc1230..a104fc9 100644 --- a/tests/Integration/PluginEventsTest.php +++ b/tests/Integration/PluginEventsTest.php @@ -107,6 +107,15 @@ public function testBlocksAfterTooManyFailedLogins() $this->assertSame($logErrorsBefore, $this->pluginLogErrors(), 'plugin logged errors'); } + public function testFailedLoginWithoutUsernameIsRecorded() + { + // some authentication flows hand over credentials without a username + $_SERVER['REMOTE_ADDR'] = '203.0.113.23'; + $_SERVER['HTTP_USER_AGENT'] = 'Mozilla/5.0'; + $this->dispatch('onUserLoginFailure', array(array('status' => 4), array())); + $this->assertSame(1, (int) $this->queryValue("SELECT COUNT(*) FROM #__bfstop_failedlogin WHERE ipaddress='203.0.113.23'")); + } + public function testSubnetBlockAndAllowList() { $this->insert('#__bfstop_bannedip', array('ipaddress' => '198.51.100.0/24', 'crdate' => self::minutesAgo(0), 'duration' => 0)); diff --git a/tests/Unit/SingleLineTest.php b/tests/Unit/SingleLineTest.php new file mode 100644 index 0000000..4649960 --- /dev/null +++ b/tests/Unit/SingleLineTest.php @@ -0,0 +1,27 @@ +assertSame('bob 2020-01-01T00:00:00+00:00 INFO forged', + LoggerHelper::singleLine("bob\r\n2020-01-01T00:00:00+00:00\tINFO\x00forged")); + $this->assertSame(' ', LoggerHelper::singleLine("\n\n\r")); + } + + public function testOrdinaryTextIsKept() + { + $this->assertSame('Zoë 管理者 ', LoggerHelper::singleLine('Zoë 管理者 ')); + $this->assertSame('', LoggerHelper::singleLine(null)); + } +} From baec11080d05617791d13cb5b6f251b6d1acc844 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 20:51:24 +0000 Subject: [PATCH 04/12] Keep .htaccess blocking temporary, small and consistent - Blocks that ran out or were lifted are taken out of the .htaccess file (on each new block and in the daily maintenance); before, a timed block stayed in the file forever and the file grew with every address ever blocked. Entries an administrator added by hand are left alone. - The block section is limited to 2000 lines; further blocks still apply in the database. - The file is replaced as a whole (temporary file and rename, permissions and symbolic links kept) instead of being truncated and rewritten, so a request or a crash in between cannot see half a configuration file. - The lock file lives in Joomla's tmp directory, not the shared system one. - The backend list only shows lines that are addresses or subnets. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- src/Extension/Bfstop.php | 52 ++++++++++++-- src/Helper/DatabaseHelper.php | 24 +++++++ src/Helper/HtaccessHelper.php | 86 ++++++++++++++++++++++-- tests/Integration/BlockedRequestTest.php | 25 +++++++ tests/Integration/DatabaseHelperTest.php | 19 ++++++ tests/Integration/HtaccessHelperTest.php | 52 ++++++++++++++ 6 files changed, 248 insertions(+), 10 deletions(-) diff --git a/src/Extension/Bfstop.php b/src/Extension/Bfstop.php index f12562d..0e86e64 100644 --- a/src/Extension/Bfstop.php +++ b/src/Extension/Bfstop.php @@ -11,6 +11,7 @@ defined('_JEXEC') or die; use Codeling\Plugin\System\Bfstop\Helper\DatabaseHelper; +use Codeling\Plugin\System\Bfstop\Helper\HtaccessHelper; use Codeling\Plugin\System\Bfstop\Helper\IpHelper; use Codeling\Plugin\System\Bfstop\Helper\LoggerHelper; use Codeling\Plugin\System\Bfstop\Helper\NotifierHelper; @@ -155,11 +156,11 @@ private function block($logEntry, $duration) } } $usehtaccess = $this->getStringParam('blockMode', 'full') === 'htaccess'; - $htaccessPath = $this->getStringParam('htaccessPath', JPATH_ROOT); - if ($htaccessPath === "") + $htaccessPath = $this->getHtaccessPath(); + if ($usehtaccess) { - $this->logger->log('htaccessPath empty, setting it to '.JPATH_ROOT, Log::INFO); - $htaccessPath = JPATH_ROOT; + // the file only holds what is still blocked + $this->removeLiftedHtaccessBlocks(); } // has to be found out before blocking, which marks these failed logins // as handled @@ -254,6 +255,45 @@ private function whyNoUnblockMail($logEntry) : 'the user has never logged in from this IP address'; } + private function getHtaccessPath() + { + $htaccessPath = $this->getStringParam('htaccessPath', JPATH_ROOT); + if ($htaccessPath === "") + { + $this->logger->log('htaccessPath empty, setting it to '.JPATH_ROOT, Log::INFO); + $htaccessPath = JPATH_ROOT; + } + return $htaccessPath; + } + + /** + * With blocking through .htaccess the web server holds back a blocked + * address; it doesn't know that a block has run out or was lifted, so the + * entry has to be taken out of the file here, or the block would be + * permanent (and the file would grow with every address ever blocked). + * Entries without a block in the database - the ones an administrator + * added to the file by hand - are left alone. + */ + private function removeLiftedHtaccessBlocks() + { + $addresses = $this->mydb->getAddressesWithoutActiveBlock(); + if (count($addresses) === 0) + { + return; + } + $htaccess = new HtaccessHelper($this->getHtaccessPath(), $this->logger); + $present = $htaccess->getDeniedIPs(); + foreach ($addresses as $address) + { + if (in_array($address, $present, true)) + { + $this->logger->log('Block of '.$address.' is over, removing it from '. + $htaccess->getFileName(), Log::INFO); + $htaccess->undenyIP($address); + } + } + } + private function getRealDurationFromDBDuration($duration) { return ($duration <= 0) @@ -647,6 +687,10 @@ public function onAfterInitialise($event) $this->mydb->trimUsernameStats(); $this->mydb->pruneKnownIps(); $this->mydb->purgeExpiredUnblockTokens(); + if ($this->getStringParam('blockMode', 'full') === 'htaccess') + { + $this->removeLiftedHtaccessBlocks(); + } LoggerHelper::pruneByAge($this->getIntParam('logKeepDays', LoggerHelper::DefaultKeepDays)); $this->params->set('lastPurge', $now); $this->mydb->saveLastPurge($now); diff --git a/src/Helper/DatabaseHelper.php b/src/Helper/DatabaseHelper.php index 3a66deb..c05adf2 100644 --- a/src/Helper/DatabaseHelper.php +++ b/src/Helper/DatabaseHelper.php @@ -254,6 +254,30 @@ public function getActiveBlockIds($ipaddress) return $ids; } + /** + * The addresses which have been blocked, but are not any more: the block + * has run out or was lifted, and no other block of the same address is + * active. For blocking through the web server's configuration, which + * knows nothing about durations. + */ + public function getAddressesWithoutActiveBlock() + { + try + { + $now = $this->db->quote(date("Y-m-d H:i:s")); + $active = "(c.duration=0 OR ".$this->addMinutesSql('c.crdate', 'c.duration')." >= $now)". + " AND NOT EXISTS (SELECT 1 FROM #__bfstop_unblock cu WHERE cu.block_id = c.id)"; + $this->db->setQuery("SELECT DISTINCT b.ipaddress FROM #__bfstop_bannedip b WHERE NOT EXISTS ". + "(SELECT 1 FROM #__bfstop_bannedip c WHERE c.ipaddress = b.ipaddress AND $active)"); + return $this->db->loadColumn(); + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + return array(); + } + } + public function isIPBlocked($ipaddress) { return (count($this->getActiveBlockIds($ipaddress)) > 0); diff --git a/src/Helper/HtaccessHelper.php b/src/Helper/HtaccessHelper.php index 2fa6ed9..9150871 100644 --- a/src/Helper/HtaccessHelper.php +++ b/src/Helper/HtaccessHelper.php @@ -10,6 +10,7 @@ defined('_JEXEC') or die; +use Joomla\CMS\Factory; use Joomla\CMS\Log\Log; /** @@ -30,6 +31,15 @@ class HtaccessHelper public const BlockPrefix = 'Require not ip '; + /** + * The most addresses the block section holds. Apache reads the file again + * for every request, and a visitor with many addresses can get many of + * them blocked: without a limit, that slows down the whole site. Blocks + * beyond it still apply in the database - for requests that get as far as + * Joomla. + */ + public const MaxBlockLines = 2000; + /** * Construct class with given $path. * @@ -97,12 +107,20 @@ public function getDeniedIPs() { $lines = $this->getLines(self::BlockPrefix); - foreach ($lines as $key => $line) + $addresses = array(); + foreach ($lines as $line) { - $lines[$key] = substr($line, strlen(self::BlockPrefix)); + // the list is shown in the backend, and what it holds is whatever + // somebody wrote into the file: only addresses and subnets are + // passed on + $address = substr($line, strlen(self::BlockPrefix)); + if (IpHelper::isValidIpOrSubnet($address)) + { + $addresses[] = $address; + } } - return $lines; + return $addresses; } /** @@ -205,7 +223,7 @@ public function remove403Message() */ private function locked(callable $change) { - $lockFile = sys_get_temp_dir().'/bfstop-htaccess-'.md5($this->path).'.lock'; + $lockFile = $this->lockDirectory().'/bfstop-htaccess-'.md5($this->path).'.lock'; $handle = @fopen($lockFile, 'c'); if ($handle === false || !flock($handle, LOCK_EX)) { @@ -230,6 +248,28 @@ 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). + */ + private function lockDirectory() + { + try + { + $tmp = (string) Factory::getApplication()->get('tmp_path'); + if ($tmp !== '' && is_dir($tmp) && is_writable($tmp)) + { + return rtrim($tmp, '/\\'); + } + } + catch (\Throwable $e) + { + // no application (e.g. in a script): use the system's + } + return sys_get_temp_dir(); + } + /** * Return array of (prefixed) lines from .htaccess. * @@ -283,7 +323,17 @@ private function getLines($prefixes = false, $onlyBody = false, $exceptPrefix = */ private function addLine($line) { - $insertion = array_merge($this->getHeader(), $this->getLines(false, true), array($line), $this->getFooter()); + $existing = $this->getLines(false, true); + if (!in_array($line, $existing, true) && count($existing) >= self::MaxBlockLines) + { + if (!is_null($this->logger)) + { + $this->logger->log("Not adding '$line' to .htaccess: it holds ".count($existing). + " lines already", Log::ERROR); + } + return false; + } + $insertion = array_merge($this->getHeader(), $existing, array($line), $this->getFooter()); return $this->insert(array_unique($insertion)); } @@ -327,6 +377,30 @@ private function removeLine($line, $prefix = false) return $this->insert($insertion); } + /** + * Replaces the file as a whole: the new content is written next to it and + * renamed over it. Writing into the file itself would truncate it first, + * and a request served (or a crash) in between would see a part of it - for + * a configuration file of the web server, an error page for the whole site. + * Falls back to writing into the file if the directory doesn't allow + * creating files, which hosts which only make the file itself writable need. + */ + private function write($content) + { + $target = is_link($this->path) ? (realpath($this->path) ?: $this->path) : $this->path; + $temporary = $target.'.bfstop-'.bin2hex(random_bytes(6)); + if (@file_put_contents($temporary, $content) === strlen($content)) + { + @chmod($temporary, file_exists($target) ? (fileperms($target) & 0777) : 0644); + if (@rename($temporary, $target)) + { + return true; + } + } + @unlink($temporary); + return file_put_contents($target, $content, LOCK_EX) !== false; + } + private static $marker = 'BFStop Blocks'; /** @@ -454,7 +528,7 @@ private function insert($insertion) $newContent = $beginContent . $newContent; } - return file_put_contents($this->path, $newContent, LOCK_EX); + return $this->write($newContent); } if (!is_null($this->logger)) { diff --git a/tests/Integration/BlockedRequestTest.php b/tests/Integration/BlockedRequestTest.php index 6bf1047..12e89b8 100644 --- a/tests/Integration/BlockedRequestTest.php +++ b/tests/Integration/BlockedRequestTest.php @@ -9,6 +9,7 @@ use Codeling\Plugin\System\Bfstop\Extension\Bfstop; use Codeling\Plugin\System\Bfstop\Helper\DatabaseHelper; +use Codeling\Plugin\System\Bfstop\Helper\HtaccessHelper; use Joomla\CMS\Factory; /** @@ -133,6 +134,30 @@ public function testDailyMaintenancePrunesAndRemembersWhenItRan() $this->assertSame(7, $saved['blockNumber'], 'the other settings are left alone'); } + public function testMaintenanceTakesLiftedBlocksOutOfTheHtaccessFile() + { + $dir = sys_get_temp_dir().'/bfstop-maintenance-'.bin2hex(random_bytes(4)); + mkdir($dir); + try + { + $this->configure(array('blockMode' => 'htaccess', 'htaccessPath' => $dir)); + $this->block('203.0.113.50', 120, 60); // ran out + $this->block('203.0.113.51', 10, 60); // still active + $helper = new HtaccessHelper($dir, $this->logger); + foreach (array('203.0.113.50', '203.0.113.51', '203.0.113.52') as $ip) + { + $helper->denyIP($ip); // .52: added by hand, no block in the database + } + $this->assertNotBlocked(); + $this->assertSame(array('203.0.113.51', '203.0.113.52'), (new HtaccessHelper($dir, $this->logger))->getDeniedIPs()); + } + finally + { + @unlink($dir.'/.htaccess'); + @rmdir($dir); + } + } + public function testMaintenanceRunsOnlyOncePerDay() { $this->configure(array('lastPurge' => time() - 3600)); diff --git a/tests/Integration/DatabaseHelperTest.php b/tests/Integration/DatabaseHelperTest.php index 2fb22b8..95a6065 100644 --- a/tests/Integration/DatabaseHelperTest.php +++ b/tests/Integration/DatabaseHelperTest.php @@ -358,4 +358,23 @@ public function testLineBreaksInUsernamesDoNotForgeLinesInTheMailedList() // header, separator and the one entry $this->assertCount(3, array_filter(explode("\n", $list), 'strlen')); } + + public function testAddressesWithoutActiveBlock() + { + $block = function ($ip, $minutesAgo, $duration) + { + return $this->insert('#__bfstop_bannedip', array('ipaddress' => $ip, + 'crdate' => self::minutesAgo($minutesAgo), 'duration' => $duration), 'id'); + }; + $block('203.0.113.1', 120, 60); // ran out + $block('203.0.113.2', 10, 60); // active + $block('203.0.113.3', 5000, 0); // permanent + $lifted = $block('203.0.113.4', 10, 60); // lifted by an administrator + $this->insert('#__bfstop_unblock', array('block_id' => $lifted, 'source' => 0, 'crdate' => self::minutesAgo(1))); + $block('203.0.113.5', 120, 60); // ran out, but blocked again since + $block('203.0.113.5', 10, 60); + $addresses = $this->helper->getAddressesWithoutActiveBlock(); + sort($addresses); + $this->assertSame(array('203.0.113.1', '203.0.113.4'), $addresses); + } } diff --git a/tests/Integration/HtaccessHelperTest.php b/tests/Integration/HtaccessHelperTest.php index 3c7ae5e..915c8f8 100644 --- a/tests/Integration/HtaccessHelperTest.php +++ b/tests/Integration/HtaccessHelperTest.php @@ -225,4 +225,56 @@ public function testReadOnlyFileIsReported() $this->assertTrue($logger->hasMessage(Log::ERROR, 'not writable')); $logger->errors = array(); // expected } + + public function testWritingReplacesTheFileWithoutLeavingTemporaryFiles() + { + file_put_contents($this->dir.'/.htaccess', "RewriteEngine On\n"); + chmod($this->dir.'/.htaccess', 0640); + $this->assertTrue($this->helper()->denyIP('203.0.113.5')); + $this->assertStringContainsString('Require not ip 203.0.113.5', $this->content()); + $this->assertStringContainsString('RewriteEngine On', $this->content()); + $this->assertSame(0640, fileperms($this->dir.'/.htaccess') & 0777, 'the permissions are kept'); + $this->assertSame(array('.htaccess'), array_values(array_diff(scandir($this->dir), array('.', '..')))); + } + + public function testWritingKeepsASymbolicLink() + { + $real = $this->dir.'/real-htaccess'; + file_put_contents($real, "RewriteEngine On\n"); + symlink($real, $this->dir.'/.htaccess'); + try + { + $this->assertTrue($this->helper()->denyIP('203.0.113.5')); + $this->assertTrue(is_link($this->dir.'/.htaccess')); + $this->assertStringContainsString('Require not ip 203.0.113.5', file_get_contents($real)); + } + finally + { + unlink($this->dir.'/.htaccess'); + unlink($real); + } + } + + public function testTheBlockSectionHasALimit() + { + $lines = array('', 'Require all granted'); + for ($i = 0; $i < HtaccessHelper::MaxBlockLines; ++$i) + { + $lines[] = 'Require not ip 10.'.intdiv($i, 250).'.'.($i % 250).'.1'; + } + $lines[] = ''; + file_put_contents($this->dir.'/.htaccess', "# BEGIN BFStop Blocks\n".implode("\n", $lines)."\n# END BFStop Blocks\n"); + $this->assertFalse($this->helper(new RecordingLogger())->denyIP('203.0.113.5')); + $this->assertStringNotContainsString('203.0.113.5', $this->content()); + // an address which is in there already is no new line + $this->assertTrue($this->helper()->denyIP('10.0.0.1')); + } + + public function testOnlyAddressesAreListed() + { + file_put_contents($this->dir.'/.htaccess', "# BEGIN BFStop Blocks\n\nRequire all granted\n". + "Require not ip 203.0.113.5\nRequire not ip \">\nRequire not ip 2001:db8::/32\n". + "Require not ip 203.0.113.0/99\n\n# END BFStop Blocks\n"); + $this->assertSame(array('203.0.113.5', '2001:db8::/32'), $this->helper()->getDeniedIPs()); + } } From 3c33a9ab0058f6bcfaa7c431d9105dbbaa3e0651 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 20:52:50 +0000 Subject: [PATCH 05/12] Bound the data a visitor can make the plugin keep, and the delays it adds - Failed logins are purged after 4 weeks unless another age is set (0 still means never), and never exceed 200000 rows; before, the default kept every failed login for ever. - However the delay settings add up, a response is held back for 60 seconds at most, and it is a single wait: the account throttle no longer sleeps for an address that was just blocked. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- src/Extension/Bfstop.php | 40 +++++++++++++++----- src/Helper/DatabaseHelper.php | 47 ++++++++++++++++++++++++ tests/Integration/DatabaseHelperTest.php | 15 ++++++++ tests/Integration/FailedLoginTest.php | 26 +++++++++++++ 4 files changed, 118 insertions(+), 10 deletions(-) diff --git a/src/Extension/Bfstop.php b/src/Extension/Bfstop.php index 0e86e64..ca6e942 100644 --- a/src/Extension/Bfstop.php +++ b/src/Extension/Bfstop.php @@ -344,12 +344,14 @@ private function blockIfTooManyAttempts($logEntry, $riskScore = 0) * per-IP threshold can ever detect on its own. The account itself is * never locked - only wrong attempts get progressively expensive, so a * correct password still logs the real owner in immediately. + * + * @return int the seconds to delay the response by */ - private function accountThrottleIfNeeded($logEntry) + private function accountThrottleDelay($logEntry) { if (!$this->getBoolParam('accountThrottleEnabled', true)) { - return; + return 0; } $checkInterval = $this->getIntParam('accountCheckInterval', 60); $accountBlockNumber = $this->getIntParam('accountBlockNumber', 20); @@ -357,17 +359,17 @@ private function accountThrottleIfNeeded($logEntry) $checkInterval, $logEntry->username, $logEntry->logtime); if ($numberOfFailedLogins < $accountBlockNumber) { - return; + return 0; } - $throttleDelay = $this->getIntParam('accountThrottleDelay', 5); + $throttleDelay = max(0, $this->getIntParam('accountThrottleDelay', 5)); if ($throttleDelay > 0) { $this->logger->log('Account-level throttle triggered for username \''. $logEntry->username.'\' ('.$numberOfFailedLogins. ' failed attempts across all IPs within '.$checkInterval. ' minutes), adding '.$throttleDelay.'s delay', Log::INFO); - sleep($throttleDelay); } + return $throttleDelay; } private function init() @@ -550,10 +552,9 @@ public function onUserLoginFailure($event) $maxNumber = $this->getIntParam('notifyFailedNumber', 0); $this->notifier->failedLogin($logEntry, $maxNumber); $this->blockIfTooManyAttempts($logEntry, $riskScore); - $this->accountThrottleIfNeeded($logEntry); - $delayDuration = $this->determineDelayDuration($riskScore); - if ($delayDuration != 0) + $delayDuration = $this->accountThrottleDelay($logEntry) + $this->determineDelayDuration($riskScore); + if ($delayDuration > 0) { // a blocked address gets nothing more out of a slow response; // don't let it hold on to a worker (attackers could otherwise use @@ -565,11 +566,29 @@ public function onUserLoginFailure($event) } else { - sleep((int) round($delayDuration)); + $seconds = self::cappedDelay($delayDuration); + if ($seconds < (int) round($delayDuration)) + { + $this->logger->log('Delay of '.round($delayDuration).'s cut to '. + $seconds.'s', Log::WARNING); + } + sleep($seconds); } } } + /** + * Every second of delay occupies a PHP worker, which is what an attacker + * with many addresses would like to run out of - so however the delays of + * the settings add up, one response is never held back longer than this. + */ + public const MaxDelaySeconds = 60; + + public static function cappedDelay($seconds) + { + return max(0, min((int) round($seconds), self::MaxDelaySeconds)); + } + public function onUserLogin($event) { $user = $event->getArgument(0); @@ -678,12 +697,13 @@ public function onAfterInitialise($event) $now = time(); if ($now > ($lastPurge + $purgeInterval)) { - $purgeAge = $this->getIntParam('deleteOld', 0); + $purgeAge = $this->getIntParam('deleteOld', DatabaseHelper::$DEFAULT_PURGE_WEEKS); if ($purgeAge > 0) { $this->mydb->purgeOldEntries($purgeAge); } // regardless of the purge age setting: these are not deleted by age + $this->mydb->trimFailedLogins(); $this->mydb->trimUsernameStats(); $this->mydb->pruneKnownIps(); $this->mydb->purgeExpiredUnblockTokens(); diff --git a/src/Helper/DatabaseHelper.php b/src/Helper/DatabaseHelper.php index c05adf2..41fa475 100644 --- a/src/Helper/DatabaseHelper.php +++ b/src/Helper/DatabaseHelper.php @@ -27,6 +27,16 @@ class DatabaseHelper // TokenunblockModel::TokenValidDays must stay in sync with this public static $UNBLOCK_TOKEN_VALID_DAYS = 3; + // failed logins are purged after this many weeks unless the administrator + // chose another time (0 = never). Keeping them for ever is not needed to + // protect anything, and they are personal data (addresses, usernames) + public static $DEFAULT_PURGE_WEEKS = 4; + + // upper bound for the rows of the failed logins, however the purge age is + // set: a visitor can add one for every request, from as many addresses as + // they like + public static $FAILED_LOGIN_MAX_ROWS = 200000; + // upper bound for the rows of the username statistics, which (unlike // the failed logins) are not purged by age: an attacker can make up as // many usernames as they like @@ -936,6 +946,43 @@ public function pruneKnownIps($maxAgeDays = null, $maxRows = null) } } + /** + * Keeps the failed logins below $maxRows by deleting the oldest ones. + */ + public function trimFailedLogins($maxRows = null) + { + $maxRows = $maxRows ?? self::$FAILED_LOGIN_MAX_ROWS; + try + { + for ($round = 0; $round < 1000; ++$round) + { + $this->db->setQuery('SELECT COUNT(*) FROM #__bfstop_failedlogin'); + $excess = ((int) $this->db->loadResult()) - $maxRows; + if ($excess <= 0) + { + return; + } + $query = $this->db->getQuery(true) + ->select($this->db->quoteName('id')) + ->from($this->db->quoteName('#__bfstop_failedlogin')) + ->order($this->db->quoteName('id').' ASC'); + $this->db->setQuery($query, 0, min($excess, 5000)); + $ids = $this->db->loadColumn(); + if (count($ids) === 0) + { + return; + } + $this->db->setQuery('DELETE FROM #__bfstop_failedlogin WHERE id IN ('. + implode(',', array_map('intval', $ids)).')'); + $this->db->execute(); + } + } + catch (\Exception $e) + { + $this->logger->log("Database exception occured: ".$e->getMessage(), Log::ERROR); + } + } + /** * Keeps the username statistics below $maxRows by deleting the entries of * the usernames with the fewest attempts, the longest ago first. diff --git a/tests/Integration/DatabaseHelperTest.php b/tests/Integration/DatabaseHelperTest.php index 95a6065..a1ea29e 100644 --- a/tests/Integration/DatabaseHelperTest.php +++ b/tests/Integration/DatabaseHelperTest.php @@ -377,4 +377,19 @@ public function testAddressesWithoutActiveBlock() sort($addresses); $this->assertSame(array('203.0.113.1', '203.0.113.4'), $addresses); } + + public function testFailedLoginsAreLimited() + { + for ($i = 0; $i < 12; ++$i) + { + $this->failedLogin('203.0.113.5', 'user'.$i, 100 - $i); + } + $this->helper->trimFailedLogins(20); + $this->assertSame(12, (int) $this->queryValue('SELECT COUNT(*) FROM #__bfstop_failedlogin')); + $this->helper->trimFailedLogins(5); + $this->assertSame(5, (int) $this->queryValue('SELECT COUNT(*) FROM #__bfstop_failedlogin')); + // the newest are kept + $this->db->setQuery('SELECT username FROM #__bfstop_failedlogin ORDER BY id'); + $this->assertSame(array('user7', 'user8', 'user9', 'user10', 'user11'), $this->db->loadColumn()); + } } diff --git a/tests/Integration/FailedLoginTest.php b/tests/Integration/FailedLoginTest.php index 39546ae..28689f2 100644 --- a/tests/Integration/FailedLoginTest.php +++ b/tests/Integration/FailedLoginTest.php @@ -7,6 +7,7 @@ **/ namespace Codeling\Bfstop\Tests\Integration; +use Codeling\Plugin\System\Bfstop\Extension\Bfstop; use Joomla\CMS\Factory; /** @@ -132,6 +133,31 @@ public function testBlockedAddressIsNotDelayed() $this->assertSame(self::Ip, $this->queryValue('SELECT ipaddress FROM #__bfstop_bannedip')); } + public function testAccountThrottleDoesNotDelayABlockedAddress() + { + // the account has been attacked from elsewhere: every further failed + // login for it would be delayed, but this one got its address blocked + $this->configure(array('blockNumber' => 1, 'accountThrottleEnabled' => 1, 'accountBlockNumber' => 2, + 'accountThrottleDelay' => 5)); + foreach (array('203.0.113.60', '203.0.113.61', '203.0.113.62') as $ip) + { + $this->insert('#__bfstop_failedlogin', array('username' => 'Admin', 'ipaddress' => $ip, + 'logtime' => self::minutesAgo(1), 'origin' => 0, 'handled' => 0)); + } + $start = microtime(true); + $this->failedLogin('Admin'); + $this->assertLessThan(4.0, microtime(true) - $start); + $this->assertSame(self::Ip, $this->queryValue('SELECT ipaddress FROM #__bfstop_bannedip')); + } + + public function testDelaysAreCapped() + { + $this->assertSame(0, Bfstop::cappedDelay(0)); + $this->assertSame(0, Bfstop::cappedDelay(-5)); + $this->assertSame(7, Bfstop::cappedDelay(6.6)); + $this->assertSame(Bfstop::MaxDelaySeconds, Bfstop::cappedDelay(100000)); + } + public function testExistingAndCommonUsernamesAreStoredReadably() { $this->configure(); From 254899169a844810f7df37cbccb7439453980f1d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 20:55:21 +0000 Subject: [PATCH 06/12] Store only a hash of the unblock tokens The table no longer holds what an unblock link needs, so a leaked backup or read-only SQL access doesn't give working links (they are bound to the blocked address and valid for 3 days, but there is no reason to keep them in the clear). The component's unblock page hashes the same way. Links sent before the update stop working; they live 3 days at most. Needs the matching com_bfstop change (branch security-review-fixes). Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- src/Helper/DatabaseHelper.php | 24 ++++++++++++++++++----- tests/Integration/BlockedRequestTest.php | 2 +- tests/Integration/DatabaseHelperTest.php | 18 +++++++++++++++-- tests/Integration/IntegrationTestCase.php | 11 ++++++++++- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/Helper/DatabaseHelper.php b/src/Helper/DatabaseHelper.php index 41fa475..94db6ed 100644 --- a/src/Helper/DatabaseHelper.php +++ b/src/Helper/DatabaseHelper.php @@ -354,14 +354,28 @@ public function blockIP($logEntry, $duration, $usehtaccess, $htaccessPath) } /** + * What is stored of an unblock token. The token itself is a secret which + * only the mail holds: whoever reads the table (a backup, an SQL injection + * somewhere else on the site) must not get working links out of it. The + * token has 160 random bits, so a plain hash is enough; it is cut to the + * 40 characters of the column. com_bfstop's TokenunblockModel uses this too. + */ + public static function hashToken($token) + { + return substr(hash('sha256', (string) $token), 0, 40); + } + + /** + * @param string $token the token to put into the link; only its hash is stored * @param string|null $username the user the link is sent to, if any + * @return string|null $token, or null if it could not be stored */ public function getNewUnblockToken($id, $token, $username = null) { try { $tokenEntry = new \stdClass(); - $tokenEntry->token = $token; + $tokenEntry->token = self::hashToken($token); $tokenEntry->block_id = $id; $tokenEntry->crdate = date("Y-m-d H:i:s"); $tokenEntry->username = $username; @@ -369,9 +383,9 @@ public function getNewUnblockToken($id, $token, $username = null) { // maybe check if duplicate token (=PRIMARY KEY violation) and retry? $this->logger->log('Insert unblock token failed!', Log::ERROR); - $tokenEntry->token = null; + return null; } - return $tokenEntry->token; + return $token; } catch (\Exception $e) { @@ -395,7 +409,7 @@ public function unblockTokenValidForBlocks($token, array $blockIds) try { $sql = "SELECT block_id, crdate FROM #__bfstop_unblock_token WHERE token=". - $this->db->quote($token); + $this->db->quote(self::hashToken($token)); $this->db->setQuery($sql); $row = $this->db->loadAssoc(); if ($row === null || !in_array((int) $row['block_id'], $blockIds, true)) @@ -439,7 +453,7 @@ public function unblockTokenExists($token) try { $sql = "SELECT token FROM #__bfstop_unblock_token WHERE token=". - $this->db->quote($token); + $this->db->quote(self::hashToken($token)); $this->db->setQuery($sql); $result = $this->db->loadResult(); return $result != null; diff --git a/tests/Integration/BlockedRequestTest.php b/tests/Integration/BlockedRequestTest.php index 12e89b8..bcd6ddc 100644 --- a/tests/Integration/BlockedRequestTest.php +++ b/tests/Integration/BlockedRequestTest.php @@ -128,7 +128,7 @@ public function testDailyMaintenancePrunesAndRemembersWhenItRan() 'crdate' => self::minutesAgo(60))); $this->assertNotBlocked(); $this->assertSame(array('203.0.113.2'), $this->knownIps()); - $this->assertSame(array(str_repeat('cd', 20)), $this->db->setQuery('SELECT token FROM #__bfstop_unblock_token')->loadColumn()); + $this->assertSame(array(DatabaseHelper::hashToken(str_repeat('cd', 20))), $this->db->setQuery('SELECT token FROM #__bfstop_unblock_token')->loadColumn()); $saved = json_decode($this->getPluginParams(), true); $this->assertGreaterThan(time() - 60, $saved['lastPurge']); $this->assertSame(7, $saved['blockNumber'], 'the other settings are left alone'); diff --git a/tests/Integration/DatabaseHelperTest.php b/tests/Integration/DatabaseHelperTest.php index a1ea29e..9c4d777 100644 --- a/tests/Integration/DatabaseHelperTest.php +++ b/tests/Integration/DatabaseHelperTest.php @@ -167,7 +167,10 @@ public function testAllowList() public function testUnblockToken() { $this->assertSame('tokenA', $this->helper->getNewUnblockToken(1, 'tokenA')); - $this->assertNull($this->queryValue("SELECT username FROM #__bfstop_unblock_token WHERE token='tokenA'")); + $this->assertNull($this->queryValue("SELECT username FROM #__bfstop_unblock_token WHERE token='".DatabaseHelper::hashToken('tokenA')."'")); + // only the hash is stored: the table doesn't hold what the link needs + $this->assertSame(0, (int) $this->queryValue("SELECT COUNT(*) FROM #__bfstop_unblock_token WHERE token='tokenA'")); + $this->assertSame(1, (int) $this->queryValue('SELECT COUNT(*) FROM #__bfstop_unblock_token WHERE block_id=1')); $this->assertFalse($this->helper->hasCurrentUnblockToken('Someone')); $this->assertSame('tokenU', $this->helper->getNewUnblockToken(2, 'tokenU', 'Someone')); $this->assertTrue($this->helper->hasCurrentUnblockToken('someone'), 'case-insensitive'); @@ -179,6 +182,17 @@ public function testUnblockToken() $this->assertFalse($this->helper->unblockTokenExists('tokenB')); } + public function testTokenHashFitsTheColumnAndIsStable() + { + $hash = DatabaseHelper::hashToken('abc'); + $this->assertMatchesRegularExpression('/^[0-9a-f]{40}$/', $hash); + $this->assertSame($hash, DatabaseHelper::hashToken('abc')); + $this->assertNotSame($hash, DatabaseHelper::hashToken('abd')); + // a stored hash is no token which unblocks: it would be hashed again + $this->helper->getNewUnblockToken(1, 'tokenA'); + $this->assertFalse($this->helper->unblockTokenExists(DatabaseHelper::hashToken('tokenA'))); + } + public function testKnownIpUsername() { $login = (object) array('ipaddress' => '203.0.113.5', 'username' => 'bob'); @@ -279,7 +293,7 @@ public function testPurgeExpiredUnblockTokens() 'crdate' => self::minutesAgo(3 * 24 * 60 - 5))); $this->helper->purgeExpiredUnblockTokens(); $this->db->setQuery('SELECT token FROM #__bfstop_unblock_token'); - $this->assertSame(array('tokenNew'), $this->db->loadColumn()); + $this->assertSame(array(DatabaseHelper::hashToken('tokenNew')), $this->db->loadColumn()); } public function testDnsCache() diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index 491b05c..1e3e8e4 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -7,6 +7,7 @@ **/ namespace Codeling\Bfstop\Tests\Integration; +use Codeling\Plugin\System\Bfstop\Helper\DatabaseHelper; use Codeling\Plugin\System\Bfstop\Helper\LoggerHelper; use Joomla\CMS\Factory; use Joomla\CMS\Log\Log; @@ -108,9 +109,17 @@ protected static function minutesAgo($minutes) return date('Y-m-d H:i:s', time() - $minutes * 60); } - /** inserts a row, returns the new id if $key is given */ + /** + * inserts a row, returns the new id if $key is given. For unblock tokens + * the row is given with the token as it is in the mail; what the table + * holds is its hash (see DatabaseHelper::hashToken()) + */ protected function insert($table, array $row, $key = null) { + if ($table === '#__bfstop_unblock_token' && isset($row['token'])) + { + $row['token'] = DatabaseHelper::hashToken($row['token']); + } $object = (object) $row; $this->db->insertObject($table, $object, $key); return $key ? (int) $object->$key : null; From 0148dc8222dce7dc72e72c06bb693726cf50f66d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 20:58:15 +0000 Subject: [PATCH 07/12] Make unsaved settings behave like the settings page says; changelog The plugin is enabled by the installation and runs with its built-in defaults until the settings are saved; they differed from the defaults the settings page shows (blockNumber 15 vs 10, checkInterval 1 day vs 1 week, ...). One table, checked against com_bfstop's settings.xml by a test, now holds them. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- CHANGELOG | 19 ++++ src/Extension/Bfstop.php | 34 +++++-- tests/Integration/ComponentTest.php | 148 +++++++++++++++++++++++++++- 3 files changed, 191 insertions(+), 10 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 01de054..c0c3a14 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,25 @@ version is now tracked in composer.json/composer.lock; src/Helper/Geo is generated from it (tools/vendor-geoip.php), CI verifies that, and Dependabot plus a weekly job report new releases and advisories + - Blocking through .htaccess: blocks that ran out or were lifted are now + removed from the file (before, a timed block stayed there for ever and + the file grew with every blocked address); at most 2000 lines are kept; + the file is replaced atomically instead of being rewritten in place; the + lock file moved from the system's temp directory to Joomla's + - Failed logins are purged after 4 weeks by default (set the purge age to 0 + for the old behaviour) and limited to 200000 rows + - A response is delayed by 60 seconds at most, however the delay settings + add up, and the account throttle no longer delays a blocked address + - Unblock tokens are stored as hashes; links sent before the update stop + working (they are valid for 3 days only) + - Usernames in log entries and in mails can no longer contain line breaks + - Array-valued request parameters ("option[]=x") no longer cause a + TypeError in the check of blocked requests + - Settings which have not been saved yet now have the values the settings + page shows (block after 10 attempts instead of 15, ...) + - Component: all list settings are validated against their options, the + notification addresses against the email format; messages escape what was + typed in; settings and log need the core.admin permission 2.0.0 (2026-08-24) - Migrate to Joomla 5/6: PSR-4 namespaced classes (Codeling\Plugin\System\Bfstop), diff --git a/src/Extension/Bfstop.php b/src/Extension/Bfstop.php index ca6e942..ecccf1a 100644 --- a/src/Extension/Bfstop.php +++ b/src/Extension/Bfstop.php @@ -33,6 +33,22 @@ class Bfstop extends CMSPlugin implements SubscriberInterface // users who logged in from the blocked address before) private const NotifyBlockedAnyAddress = 2; + /** + * What the settings are while they have not been saved: the same as the + * settings page shows for them (com_bfstop's forms/settings.xml - a test + * compares them). The plugin is enabled by the installation, so it runs + * with these until an administrator opens the settings. + */ + public const DefaultSettings = array( + 'blockNumber' => 10, + 'checkInterval' => 10080, + 'maxBlocksBefore' => 3, + 'notifyUsePasswordReminder' => 1, + 'adaptiveDelayMax' => 0, + 'adaptiveDelayThresholdMin' => 50, + 'adaptiveDelayThresholdMax' => 200, + ); + private LoggerHelper $logger; private NotifierHelper $notifier; private DatabaseHelper $mydb; @@ -125,7 +141,7 @@ private function block($logEntry, $duration) ' is already blocked!', Log::ERROR); return; } - $maxBlocksBefore = $this->getIntParam('maxBlocksBefore', 0); + $maxBlocksBefore = $this->getIntParam('maxBlocksBefore', self::DefaultSettings['maxBlocksBefore']); $progressiveEnabled = $this->getBoolParam('progressiveBlockDuration', false); if ($maxBlocksBefore > 0 || $progressiveEnabled) { @@ -166,7 +182,7 @@ private function block($logEntry, $duration) // as handled $targetedOtherAccounts = $this->getBoolParam('notifyBlockedUser', false) && $this->mydb->hasFailedLoginsForOtherAccounts( - $this->getRealDurationFromDBDuration($this->getIntParam('checkInterval', NotifierHelper::$ONE_DAY)), + $this->getRealDurationFromDBDuration($this->getIntParam('checkInterval', self::DefaultSettings['checkInterval'])), $logEntry->ipaddress, $logEntry->username, $logEntry->logtime); $id = $this->mydb->blockIP($logEntry, $duration, $usehtaccess, $htaccessPath); if ($id < 1) @@ -311,7 +327,7 @@ private function getRealDurationFromDBDuration($duration) */ private function determineEffectiveBlockNumber($riskScore) { - $blockNumber = $this->getIntParam('blockNumber', 15); + $blockNumber = $this->getIntParam('blockNumber', self::DefaultSettings['blockNumber']); $reductionPerPoint = $this->getIntParam('riskBlockNumberReductionPerPoint', 1); $minBlockNumber = $this->getIntParam('riskMinBlockNumber', 2); $effective = (int) round($blockNumber - $riskScore * $reductionPerPoint); @@ -323,7 +339,7 @@ private function blockIfTooManyAttempts($logEntry, $riskScore = 0) $blockInterval = $this->getIntParam('blockDuration', NotifierHelper::$ONE_DAY); $maxNumber = $this->determineEffectiveBlockNumber($riskScore); $checkInterval = $this->getRealDurationFromDBDuration( - $this->getIntParam('checkInterval', NotifierHelper::$ONE_DAY)); + $this->getIntParam('checkInterval', self::DefaultSettings['checkInterval'])); if ($this->mydb->getNumberOfFailedLogins( $checkInterval, $logEntry->ipaddress, @@ -389,7 +405,7 @@ private function notifyOfRemainingAttempts($logEntry, $riskScore = 0) // remaining attempts notification only makes sense if we // actually block $notifyRemaining = $this->getBoolParam('notifyRemainingAttempts', false); - $passwordReminder = $this->getIntParam('notifyUsePasswordReminder', -1); + $passwordReminder = $this->getIntParam('notifyUsePasswordReminder', self::DefaultSettings['notifyUsePasswordReminder']); if ($this->getStringParam('blockMode', 'full') === 'off' || (!$notifyRemaining && !($passwordReminder == -1 || $passwordReminder > 0))) @@ -399,7 +415,7 @@ private function notifyOfRemainingAttempts($logEntry, $riskScore = 0) } $allowedAttempts = $this->determineEffectiveBlockNumber($riskScore); $checkInterval = $this->getRealDurationFromDBDuration( - $this->getIntParam('checkInterval', NotifierHelper::$ONE_DAY)); + $this->getIntParam('checkInterval', self::DefaultSettings['checkInterval'])); $numberOfFailedLogins = $this->mydb->getNumberOfFailedLogins( $checkInterval, $logEntry->ipaddress, $logEntry->logtime); @@ -454,9 +470,9 @@ private function determineBaseDelayDuration() $adaptive = $this->getBoolParam('adaptiveDelay', false); if ($adaptive) { - $maxDelay = $this->getIntParam('adaptiveDelayMax', 60); - $lowThreshold = $this->getIntParam('adaptiveDelayThresholdMin', 50); - $highThreshold = $this->getIntParam('adaptiveDelayThresholdMax', 1000); + $maxDelay = $this->getIntParam('adaptiveDelayMax', self::DefaultSettings['adaptiveDelayMax']); + $lowThreshold = $this->getIntParam('adaptiveDelayThresholdMin', self::DefaultSettings['adaptiveDelayThresholdMin']); + $highThreshold = $this->getIntParam('adaptiveDelayThresholdMax', self::DefaultSettings['adaptiveDelayThresholdMax']); if ($lowThreshold > $highThreshold) { $tmp = $lowThreshold; diff --git a/tests/Integration/ComponentTest.php b/tests/Integration/ComponentTest.php index 2f19a34..723c285 100644 --- a/tests/Integration/ComponentTest.php +++ b/tests/Integration/ComponentTest.php @@ -8,6 +8,8 @@ namespace Codeling\Bfstop\Tests\Integration; use Codeling\Component\Bfstop\Administrator\Controller\DisplayController; +use Codeling\Component\Bfstop\Administrator\Helper\IpValidateHelper; +use Codeling\Component\Bfstop\Administrator\Helper\UnblockHelper; use Codeling\Component\Bfstop\Administrator\Model\AllowModel; use Codeling\Component\Bfstop\Administrator\Model\AllowlistModel; use Codeling\Component\Bfstop\Administrator\Model\BlockModel; @@ -218,7 +220,10 @@ public function testTokenUnblockForBlockedIpv6Network() private function formRuleResult($ruleFile, $class, $value, $blockMode = 'full') { - require_once getenv('COM_BFSTOP_ROOT').'/admin/rules/'.$ruleFile; + if (!class_exists($class, false)) + { + require_once getenv('COM_BFSTOP_ROOT').'/admin/rules/'.$ruleFile; + } $rule = new $class(); return $rule->test(new \SimpleXMLElement(''), $value, null, new \Joomla\Registry\Registry(array('params' => array('blockMode' => $blockMode)))); @@ -260,4 +265,145 @@ public function testWarnsAboutAdminUserCaseInsensitively() $controller->warnIfAdminUserExists(); $this->assertCount(1, $app->getMessageQueue(true)); } + + public function testMessagesEscapeWhatWasTypedIn() + { + // Joomla shows the messages as HTML + $app = Factory::getApplication(); + Factory::getLanguage()->load('com_bfstop', JPATH_ADMINISTRATOR.'/components/com_bfstop'); + $app->getMessageQueue(true); + $evil = ''; + $this->assertFalse(IpValidateHelper::validIPRange($evil)); + $this->assertFalse(IpValidateHelper::validIPRange('203.0.113.0/'.$evil)); + IpValidateHelper::validIPRange('10.0.0.1'); // private: a warning with the address + $messages = ''; + foreach ($app->getMessageQueue(true) as $message) + { + $messages .= $message['message']; + } + $this->assertStringNotContainsString(''; $this->assertFalse(IpValidateHelper::validIPRange($evil)); diff --git a/tests/Integration/FailedLoginTest.php b/tests/Integration/FailedLoginTest.php index 28689f2..2226cb1 100644 --- a/tests/Integration/FailedLoginTest.php +++ b/tests/Integration/FailedLoginTest.php @@ -25,7 +25,7 @@ class FailedLoginTest extends IntegrationTestCase public static function setUpBeforeClass(): void { parent::setUpBeforeClass(); - $db = Factory::getDbo(); + $db = Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $db->setQuery("SELECT params FROM #__extensions WHERE type='plugin' AND element='bfstop'"); self::$originalParams = $db->loadResult(); } @@ -34,7 +34,7 @@ public static function tearDownAfterClass(): void { if (self::$originalParams !== null) { - $db = Factory::getDbo(); + $db = Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $db->setQuery('UPDATE #__extensions SET params='.$db->quote(self::$originalParams). " WHERE type='plugin' AND element='bfstop'"); $db->execute(); diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php index 1e3e8e4..51ede3f 100644 --- a/tests/Integration/IntegrationTestCase.php +++ b/tests/Integration/IntegrationTestCase.php @@ -79,7 +79,7 @@ public static function setUpBeforeClass(): void protected function setUp(): void { - $this->db = Factory::getDbo(); + $this->db = Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $this->logger = new RecordingLogger(); $this->emptyTables(); } diff --git a/tests/Integration/IpHelperAddressTest.php b/tests/Integration/IpHelperAddressTest.php index 83331d3..cacda63 100644 --- a/tests/Integration/IpHelperAddressTest.php +++ b/tests/Integration/IpHelperAddressTest.php @@ -26,7 +26,7 @@ class IpHelperAddressTest extends IntegrationTestCase public static function setUpBeforeClass(): void { parent::setUpBeforeClass(); - $db = \Joomla\CMS\Factory::getDbo(); + $db = \Joomla\CMS\Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $db->setQuery("SELECT params FROM #__extensions WHERE type='plugin' AND element='bfstop'"); self::$originalParams = $db->loadResult(); } @@ -35,7 +35,7 @@ public static function tearDownAfterClass(): void { if (self::$originalParams !== null) { - $db = \Joomla\CMS\Factory::getDbo(); + $db = \Joomla\CMS\Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $db->setQuery('UPDATE #__extensions SET params='.$db->quote(self::$originalParams). " WHERE type='plugin' AND element='bfstop'"); $db->execute(); diff --git a/tests/Integration/PluginEventsTest.php b/tests/Integration/PluginEventsTest.php index a104fc9..7b7fbb3 100644 --- a/tests/Integration/PluginEventsTest.php +++ b/tests/Integration/PluginEventsTest.php @@ -35,7 +35,7 @@ class PluginEventsTest extends IntegrationTestCase public static function setUpBeforeClass(): void { parent::setUpBeforeClass(); - $db = Factory::getDbo(); + $db = Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $db->setQuery("SELECT params FROM #__extensions WHERE type='plugin' AND element='bfstop'"); self::$originalParams = $db->loadResult(); $db->setQuery('UPDATE #__extensions SET params='.$db->quote(json_encode(self::Params)). @@ -51,7 +51,7 @@ public static function tearDownAfterClass(): void { if (self::$originalParams !== null) { - $db = Factory::getDbo(); + $db = Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $db->setQuery('UPDATE #__extensions SET params='.$db->quote(self::$originalParams). " WHERE type='plugin' AND element='bfstop'"); $db->execute(); diff --git a/tests/Integration/UnblockPageHttpTest.php b/tests/Integration/UnblockPageHttpTest.php index 68f2cf9..bcc0340 100644 --- a/tests/Integration/UnblockPageHttpTest.php +++ b/tests/Integration/UnblockPageHttpTest.php @@ -32,7 +32,7 @@ public static function setUpBeforeClass(): void { self::markTestSkipped('COM_BFSTOP_ROOT not set, see tests/README.md'); } - $db = Factory::getDbo(); + $db = Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $db->setQuery("SELECT params FROM #__extensions WHERE type='plugin' AND element='bfstop'"); self::$originalParams = $db->loadResult(); } @@ -41,7 +41,7 @@ public static function tearDownAfterClass(): void { if (self::$originalParams !== null) { - $db = Factory::getDbo(); + $db = Factory::getContainer()->get(\Joomla\Database\DatabaseInterface::class); $db->setQuery('UPDATE #__extensions SET params='.$db->quote(self::$originalParams). " WHERE type='plugin' AND element='bfstop'"); $db->execute(); diff --git a/updatescript.php b/updatescript.php index 3e0b7d4..84e1f1c 100644 --- a/updatescript.php +++ b/updatescript.php @@ -11,6 +11,7 @@ use Joomla\CMS\Installer\InstallerAdapter; use Joomla\CMS\Language\Text; use Joomla\CMS\Router\Route; +use Joomla\Database\DatabaseInterface; class PlgsystembfstopInstallerScript { @@ -21,7 +22,7 @@ public function install(InstallerAdapter $adapter) // plugins install disabled by default, but bfstop only does anything // useful while running, and all its settings now live in the // component - so there's nothing left to configure before enabling it. - $db = Factory::getDbo(); + $db = Factory::getContainer()->get(DatabaseInterface::class); $query = $db->getQuery(true) ->update($db->quoteName('#__extensions')) ->set($db->quoteName('enabled') . ' = 1') @@ -71,7 +72,7 @@ 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::getDbo(); + $db = Factory::getContainer()->get(DatabaseInterface::class); if ($db->getServerType() === 'mysql') { try From c169f8645f368576ac8320ccf1c8461e051f88cc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 22:05:15 +0000 Subject: [PATCH 10/12] Stop using the other deprecated Joomla APIs Factory::getConfig/getMailer/getLanguage and $app->input are replaced by the application's get(), the MailerFactory of the container, getLanguage() and getInput(). A test (together with the component's sources) fails if a deprecated call comes back: the database accessors replaced earlier, getConfig, getSession, getLanguage, getDocument, getMailer, getUser, getCache, getCfg, Table::getInstance, $_db, View::get() and addStyleSheet(). Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- CHANGELOG | 4 ++ src/Extension/Bfstop.php | 2 +- src/Helper/NotifierHelper.php | 10 +--- tests/Integration/ComponentTest.php | 16 +++++ tests/Unit/DeprecatedApiTest.php | 93 +++++++++++++++++++++++++++++ updatescript.php | 2 +- 6 files changed, 118 insertions(+), 9 deletions(-) create mode 100644 tests/Unit/DeprecatedApiTest.php diff --git a/CHANGELOG b/CHANGELOG index c0c3a14..13cafd8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -20,6 +20,10 @@ TypeError in the check of blocked requests - Settings which have not been saved yet now have the values the settings page shows (block after 10 attempts instead of 15, ...) + - Internal: the Joomla! APIs deprecated in 4.3-5.3 and removed in Joomla! 7 + are no longer used (Factory::getDbo/getConfig/getSession/getLanguage/ + getDocument/getMailer, $_db, Table::getInstance, View::get(), + addStyleSheet(), getCfg()); a test fails if one comes back - Component: all list settings are validated against their options, the notification addresses against the email format; messages escape what was typed in; settings and log need the core.admin permission diff --git a/src/Extension/Bfstop.php b/src/Extension/Bfstop.php index ecccf1a..c6ae943 100644 --- a/src/Extension/Bfstop.php +++ b/src/Extension/Bfstop.php @@ -90,7 +90,7 @@ private function getStringParam($paramName, $default) */ private function requestString($name, $filter = 'cmd') { - $value = $this->getApplication()->input->get($name, '', $filter); + $value = $this->getApplication()->getInput()->get($name, '', $filter); return is_string($value) ? $value : "\0"; } diff --git a/src/Helper/NotifierHelper.php b/src/Helper/NotifierHelper.php index 966680b..c36fe03 100644 --- a/src/Helper/NotifierHelper.php +++ b/src/Helper/NotifierHelper.php @@ -13,6 +13,7 @@ use Joomla\CMS\Factory; use Joomla\CMS\Language\Text; use Joomla\CMS\Log\Log; +use Joomla\CMS\Mail\MailerFactoryInterface; use Joomla\CMS\Uri\Uri; class NotifierHelper @@ -50,12 +51,7 @@ public function getNotifyAddresses() public function getSiteName() { - $config = Factory::getConfig(); - $siteName = $config->get('sitename'); // Joomla! 3.x - $siteName = (strcmp($siteName, '') == 0) - ? $config->get('config.sitename') - : $siteName; - return $siteName; + return (string) Factory::getApplication()->get('sitename'); } public function isNotificationAllowed($logtime, $maxNumber, @@ -112,7 +108,7 @@ public function sendMail($subject, $body, $emailAddresses) $this->logger->log("sendMail called with invalid argument: ".print_r($emailAddresses, true), Log::ERROR); return false; } - $mail = Factory::getMailer(); + $mail = Factory::getContainer()->get(MailerFactoryInterface::class)->createMailer(); $mail->setSubject($subject); $mail->setBody($body); foreach ($emailAddresses as $recipient) diff --git a/tests/Integration/ComponentTest.php b/tests/Integration/ComponentTest.php index dd3bc5a..74e5f03 100644 --- a/tests/Integration/ComponentTest.php +++ b/tests/Integration/ComponentTest.php @@ -406,4 +406,20 @@ public function testUnsavedSettingsAreWhatTheSettingsPageShows() $this->assertSame((string) $default, (string) $field[0]['default'], $name); } } + + public function testEditViewsRegisterTheirStylesheetsAsRealFiles() + { + // a path with a leading slash would become "//administrator/...", a link to another host + foreach (array('Allow' => 'block', 'Block' => 'block', 'Htblock' => 'htblock') as $view => $folder) + { + $source = file_get_contents(getenv('COM_BFSTOP_ROOT').'/admin/src/View/'.$view.'/HtmlView.php'); + $this->assertSame(1, preg_match("#registerAndUseStyle\(\s*'([\w.]+)',\s*'([^']+)'\)#", $source, $m), $view); + $this->assertFileExists(getenv('COM_BFSTOP_ROOT').'/admin/'.substr($m[2], strlen('administrator/components/com_bfstop/')), $view); + $wa = new \Joomla\CMS\WebAsset\WebAssetManager(new \Joomla\CMS\WebAsset\WebAssetRegistry()); + $wa->registerAndUseStyle($m[1], $m[2]); + $uri = $wa->getAsset('style', $m[1])->getUri(); + $this->assertStringStartsWith('/administrator/components/com_bfstop/tmpl/', $uri, $view); + $this->assertStringEndsWith('/edit.css', $uri, $view); + } + } } diff --git a/tests/Unit/DeprecatedApiTest.php b/tests/Unit/DeprecatedApiTest.php new file mode 100644 index 0000000..ff5504e --- /dev/null +++ b/tests/Unit/DeprecatedApiTest.php @@ -0,0 +1,93 @@ + what to use */ + private const Deprecated = array( + '/Factory::getDbo\(/' => 'the DatabaseInterface of the container (or $this->getDatabase() in a model)', + '/Factory::getConfig\(/' => 'Factory::getApplication()->get()', + '/Factory::getSession\(/' => '$app->getSession()', + '/Factory::getLanguage\(/' => '$app->getLanguage()', + '/Factory::getDocument\(/' => '$app->getDocument() (in a view: $this->getDocument())', + '/Factory::getMailer\(/' => 'MailerFactoryInterface from the container', + '/Factory::getUser\(/' => '$app->getIdentity()', + '/Factory::getCache\(/' => 'CacheControllerFactoryInterface from the container', + '/Factory::getDate\(/' => 'new Joomla\CMS\Date\Date()', + '/->getCfg\(/' => '->get()', + '/->_db\b/' => '$this->getDatabase()', + '/Table::getInstance\(/' => 'the MVC factory (createTable) or the table class itself', + '/->addStyleSheet\(|->addScript\(|->addScriptDeclaration\(|->addStyleDeclaration\(/' => 'the WebAssetManager', + '/\$this->get\(\'/' => '$this->getModel()->getXyz() in a view', + '/(Factory::getApplication\(\)|\$app|\$application|\$this->getApplication\(\))->input\b/' => '->getInput()', + ); + + private function sources() + { + $roots = array(dirname(__DIR__, 2).'/src', dirname(__DIR__, 2).'/services', dirname(__DIR__, 2).'/updatescript.php'); + if (getenv('COM_BFSTOP_ROOT')) + { + foreach (array('admin', 'site') as $dir) + { + $roots[] = getenv('COM_BFSTOP_ROOT').'/'.$dir; + } + $roots[] = getenv('COM_BFSTOP_ROOT').'/installscript.php'; + } + foreach ($roots as $root) + { + if (is_file($root)) + { + yield $root; + continue; + } + foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($root, \FilesystemIterator::SKIP_DOTS)) as $file) + { + // the vendored MaxMind reader is not ours to modernize + if ($file->getExtension() === 'php' && strpos($file->getPathname(), '/Helper/Geo/') === false) + { + yield $file->getPathname(); + } + } + } + } + + public function testNoDeprecatedApiIsUsed() + { + $found = array(); + $files = 0; + foreach ($this->sources() as $path) + { + ++$files; + foreach (file($path) as $number => $line) + { + // comments may name the old forms + if (preg_match('#^\s*(//|\*|/\*)#', $line)) + { + continue; + } + foreach (self::Deprecated as $pattern => $replacement) + { + if (preg_match($pattern, $line)) + { + $found[] = basename(dirname($path)).'/'.basename($path).':'.($number + 1).' use '.$replacement; + } + } + } + } + $this->assertGreaterThan(20, $files, 'the sources were not found'); + $this->assertSame(array(), $found, "deprecated API:\n".implode("\n", $found)); + } +} diff --git a/updatescript.php b/updatescript.php index 84e1f1c..6e3d386 100644 --- a/updatescript.php +++ b/updatescript.php @@ -58,7 +58,7 @@ public function postflight($type, InstallerAdapter $adapter) { if ($type === 'update') { - $lang = Factory::getLanguage(); + $lang = Factory::getApplication()->getLanguage(); $lang->load('plg_system_bfstop', JPATH_ADMINISTRATOR); Factory::getApplication()->enqueueMessage( Text::sprintf('PLG_SYSTEM_BFSTOP_UPDATE_2_0_0_HINT', Route::_('index.php?option=com_bfstop&view=settings', false)), From f4da0e89aa85ec8b765835e7350122b87f239660 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 22:06:09 +0000 Subject: [PATCH 11/12] Fix the deprecated-API test for the unit job, which runs without the component The sanity check expected more than 20 source files, but the unit job of the plugin repository only has the plugin's dozen. It now expects 8 with the plugin alone and 40 with the component checked out as well. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- tests/Unit/DeprecatedApiTest.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/Unit/DeprecatedApiTest.php b/tests/Unit/DeprecatedApiTest.php index ff5504e..d4f5644 100644 --- a/tests/Unit/DeprecatedApiTest.php +++ b/tests/Unit/DeprecatedApiTest.php @@ -87,7 +87,9 @@ public function testNoDeprecatedApiIsUsed() } } } - $this->assertGreaterThan(20, $files, 'the sources were not found'); + // the plugin alone has about a dozen files (the unit tests run without + // the component), with the component there are over fifty + $this->assertGreaterThan(getenv('COM_BFSTOP_ROOT') ? 40 : 8, $files, 'the sources were not found'); $this->assertSame(array(), $found, "deprecated API:\n".implode("\n", $found)); } } From f3d5018e1b30110c368fc99ac4d792b33a0e1443 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 22:08:20 +0000 Subject: [PATCH 12/12] Changelog: list the security review changes under 2.0.0, which is not released yet Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh --- CHANGELOG | 59 +++++++++++++++++++++++++++---------------------------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 13cafd8..3d4cff6 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,33 +1,3 @@ -2.0.1 (unreleased) - - Security: the bundled MaxMind DB reader is updated from 1.13.0 to 1.14.0 - (limits for the decoding of malformed or hostile database files). Its - version is now tracked in composer.json/composer.lock; src/Helper/Geo is - generated from it (tools/vendor-geoip.php), CI verifies that, and - Dependabot plus a weekly job report new releases and advisories - - Blocking through .htaccess: blocks that ran out or were lifted are now - removed from the file (before, a timed block stayed there for ever and - the file grew with every blocked address); at most 2000 lines are kept; - the file is replaced atomically instead of being rewritten in place; the - lock file moved from the system's temp directory to Joomla's - - Failed logins are purged after 4 weeks by default (set the purge age to 0 - for the old behaviour) and limited to 200000 rows - - A response is delayed by 60 seconds at most, however the delay settings - add up, and the account throttle no longer delays a blocked address - - Unblock tokens are stored as hashes; links sent before the update stop - working (they are valid for 3 days only) - - Usernames in log entries and in mails can no longer contain line breaks - - Array-valued request parameters ("option[]=x") no longer cause a - TypeError in the check of blocked requests - - Settings which have not been saved yet now have the values the settings - page shows (block after 10 attempts instead of 15, ...) - - Internal: the Joomla! APIs deprecated in 4.3-5.3 and removed in Joomla! 7 - are no longer used (Factory::getDbo/getConfig/getSession/getLanguage/ - getDocument/getMailer, $_db, Table::getInstance, View::get(), - addStyleSheet(), getCfg()); a test fails if one comes back - - Component: all list settings are validated against their options, the - notification addresses against the email format; messages escape what was - typed in; settings and log need the core.admin permission - 2.0.0 (2026-08-24) - Migrate to Joomla 5/6: PSR-4 namespaced classes (Codeling\Plugin\System\Bfstop), service provider, SubscriberInterface event handling @@ -182,6 +152,35 @@ are validated (no stream wrappers like phar://, a .mmdb file name) - Added SECURITY.md with the supported versions and how to report vulnerabilities + - Security: the bundled MaxMind DB reader (maxmind-db/reader, now 1.14.0, + which limits the decoding of malformed or hostile database files) is + tracked in composer.json/composer.lock; src/Helper/Geo is generated from + it (tools/vendor-geoip.php), CI verifies that, and Dependabot plus a + weekly job report new releases and security advisories + - Security: blocking through .htaccess removes blocks that ran out or were + lifted from the file (a timed block used to stay there for ever, and the + file grew with every blocked address); at most 2000 lines are kept; the + file is replaced atomically instead of being rewritten in place; the lock + file lives in Joomla's tmp directory instead of the system's + - Security: failed logins are purged after 4 weeks by default (set the + 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 + - 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 + line breaks; array-valued request parameters ("option[]=x") no longer + cause a TypeError in the check of blocked requests + - Settings which have not been saved yet now have the values the settings + page shows (block after 10 attempts, ...) + - Component: all list settings are validated against their options, the + notification addresses against the email format; messages escape what was + typed in; the settings and log views need the core.admin permission + - Internal: the Joomla! APIs deprecated in 4.3-5.3 and removed in Joomla! 7 + are no longer used (Factory::getDbo/getConfig/getSession/getLanguage/ + getDocument/getMailer, $_db, Table::getInstance, View::get(), + addStyleSheet(), getCfg()); a test fails if one comes back 1.5.2 (2024-02-18) - Note: Only component changes, no plugin changes