Fix login-only mode for backend logins; add lint job and more tests - #228
Merged
Merged
Conversation
The backend login form posts option=com_login&task=login, but isLoginAttemptRequest() only recognized com_users, so in "Login Only" block mode a blocked IP could keep submitting backend login attempts. Found by the new integration tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013VoDNPckWPWBjxuJZhFPux
bfstop.php and helpers/ were removed in the 2.0.0 migration to namespaced classes; zip only warned about them, but CI builds the zip now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013VoDNPckWPWBjxuJZhFPux
Lint job (new): all XML files well-formed, language files parseable, prefixed with PLG_SYSTEM_BFSTOP_ and in the folder matching their language tag, manifest-referenced language files present (missing translations are reported as warnings), and the zip built by deploy.sh containing everything the manifest references. New integration tests: - BlockedRequestTest: requests from a blocked IP in full and login-only mode (incl. the backend login form, see previous fix), block expiry and permanent blocks, password recovery and unblock tokens getting through, rejected requests being counted, lifted blocks - IpHelperAddressTest: proxy header handling with the settings read from the plugin params, i.e. that a forwarding header is only trusted from the configured proxy - RiskHelperTest, HtaccessHelperTest, GeoHelperTest - IpValidateHelperTest and (unit) IpRangeHelperTest for the component fixtures/request.php takes an optional query string with request parameters; RecordingLogger additionally keeps all messages. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013VoDNPckWPWBjxuJZhFPux
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on the test suite and CI from #226/#227. It adds a lint job and tests for areas the suite didn't cover yet. Those tests turned up one bug, which this PR also fixes.
Bug fix: "Login Only" block mode didn't protect the backend
The backend login form posts
option=com_login&task=login, butisLoginAttemptRequest()only recognizedcom_users. So in login-only mode, a blocked IP could keep submitting backend passwords. The frontend was not affected.BlockedRequestTest::testLoginOnlyModeOnlyRejectsLoginAttemptsfails without the fix.Other changes
deploy.sh: stopped listingbfstop.phpandhelpers/, which no longer exist.lintjob, which runs:xmllinton all XML filestests/lint/check-language.php: every language file parses the way Joomla parses it, keys have thePLG_SYSTEM_BFSTOP_prefix, each file sits in its language-tag folder, and every file the manifest lists exists. Missing translations are reported as warnings only.tests/lint/check-zip.php: the zip fromdeploy.sh zipcontains everything the manifest referencesBlockedRequestTest: requests from a blocked IP in full and login-only mode, block expiry, permanent blocks, password recovery and unblock tokens getting through, rejected requests being counted, lifted blocksIpHelperAddressTest: a proxy header is only trusted when the request comes from the configured proxy (protection against spoofing)RiskHelperTest,HtaccessHelperTest,GeoHelperTestIpValidateHelperTest, plus the unit testIpRangeHelperTest, which runs only whenCOM_BFSTOP_ROOTis setfixtures/request.phpnow takes an optional query string of request parameters.RecordingLoggernow also keeps a list of all messages; its existing error checks work as before.Known issue, not fixed here
With search-engine-friendly URLs on (Joomla's default), a blocked user can't reach the password-reset page.
isPasswordRecoveryRequest()runs inonAfterInitialise, before Joomla has routed the request, sooption/vieware still empty for a URL like/index.php/component/users/reset. Joomla also redirects the non-SEF reset link to the SEF one. I confirmed this over HTTP against a real site. The in-process harness passes request parameters directly, so it can't reproduce this. Fixing it probably means moving that check toonAfterRoute. That's a design change, so it's left for a separate PR.Companion PR
codeling/com_bfstop has a branch of the same name, and its CI uses this branch through the existing same-name lookup. That PR fixes the component's language keys and adds the same lint job.
Testing
Run locally with PHPUnit 11 against the harness's own
tests/ci/install-joomla.sh:Each run has one skip: the read-only file test, because the sandbox runs as root.
🤖 Generated with Claude Code
https://claude.ai/code/session_013VoDNPckWPWBjxuJZhFPux
Generated by Claude Code