Skip to content

Security review fixes (plugin): GeoIP reader tracking, .htaccess, data bounds, token hashing - #236

Open
codeling wants to merge 12 commits into
mainfrom
security-review-fixes
Open

Security review fixes (plugin): GeoIP reader tracking, .htaccess, data bounds, token hashing#236
codeling wants to merge 12 commits into
mainfrom
security-review-fixes

Conversation

@codeling

@codeling codeling commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes from a security review of plugin and component. The matching component changes are in codeling/com_bfstop (branch security-review-fixes; CI of each repo checks out the other repo's branch of the same name). This branch also contains the two token-exemption tests from #235.

Bundled GeoIP reader (src/Helper/Geo)

  • Tracked in composer.json/composer.lock; tools/vendor-geoip.php generates the copy from the lock (only namespace, header and _JEXEC guard change - verified to reproduce the shipped 1.13.0 files). composer check-geoip runs in CI and fails if the copy deviates.
  • Updated 1.13.0 -> 1.14.0 (decode limits for malformed/hostile databases).
  • Dependabot (composer, actions) and a weekly job (dependencies.yml: advisories, newer release, opens an issue). README and SECURITY.md document how to update, and that the .mmdb database needs regular updates too.
  • Unit tests do real lookups using a small MMDB writer (tests/Support/MmdbBuilder.php).

.htaccess blocking

  • Lifted/expired blocks are removed from the file (new block + daily maintenance); before, timed blocks were permanent and the file grew without bound. Manual entries are untouched. Cap of 2000 lines.
  • Atomic replace (temp file + rename; permissions and symlinks kept), lock file in Joomla's tmp dir, backend list shows valid addresses/subnets only.

Bounded data and delays

  • Failed logins purged after 4 weeks by default (0 = never still possible), max 200000 rows.
  • One delay per response, 60 s at most; no account-throttle delay for an address that was just blocked.

Smaller hardening

  • Unblock tokens are stored as hashes (needs the com_bfstop change; links sent before the update stop working, they live 3 days).
  • Request parameters compared by the block logic are read as strings (array values no longer raise a TypeError); control characters in usernames are removed from log entries and mails; missing username in credentials; no mail/notification for a block that could not be stored; dead Joomla 3 code removed.
  • Unsaved settings equal what the settings page shows (one table, checked by a test against settings.xml).

Not in this PR: the matching of requests against exemptions other than the unblock token was not part of this work; per-request CIDR query cost; pinning GitHub Actions to SHAs and checksum verification of the Joomla download in CI.

Testing

Full integration + unit suite against Joomla 5.4.8 on PostgreSQL 16 with this branch and the com_bfstop branch: 322 tests, all passing (1 skipped: the read-only-file test, because it ran as root). New tests were checked to fail against the unfixed code where applicable. Not run: MySQL/MariaDB, Joomla 6, PHP 8.1.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh

Generated by Claude Code

claude added 8 commits October 7, 2026 19:45
@claude
Test that the unblock token only passes the unblock view
c013e6c 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Track the bundled GeoIP reader as a dependency and update it to 1.14.0
8b3af12 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Harden request parameter handling and what visitors can write to logs…
792d0c2 
… 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Keep .htaccess blocking temporary, small and consistent
baec110 
- 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Bound the data a visitor can make the plugin keep, and the delays it …
3c33a9a 
…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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Store only a hash of the unblock tokens
2548991 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Make unsaved settings behave like the settings page says; changelog
0148dc8 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
README: say only what is known about the GeoIP database updates
98940c0 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@codeling codeling mentioned this pull request Oct 7, 2026
Open
claude added 4 commits October 7, 2026 21:06
@claude
Use the database service instead of the deprecated Factory::getDbo()
1bc8a92 
Factory::getDbo() is deprecated and removed in Joomla 7: the plugin's
DatabaseHelper and the installer script get the DatabaseInterface from the
container. The tests do the same, and no longer depend on the component being
copied into the site by hand to find its language files.

Needs no change in com_bfstop to work, but see its branch of the same name.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Stop using the other deprecated Joomla APIs
c169f86 
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Fix the deprecated-API test for the unit job, which runs without the …
f4da0e8 
…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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
@claude
Changelog: list the security review changes under 2.0.0, which is not…
f3d5018 
… released yet

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhhhdjhErC9ZiaS4HyRjsh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@codeling @claude