Port AS-689 follow-up hardening to 6.x - #484
Conversation
There was a problem hiding this comment.
LGTM
Please check these AI items:
Blocking
The exact-match override buys no security.
matomo_user.loginis a PRIMARY KEY under a case- and accent-insensitive collation, so two logins differing only by ASCII case cannot both exist — insertingR3davebesider3davefails with a duplicate-key error.isSameLogin()'s ASCII-only fold already rejects everything the collation folds, soisSameLoginExact()only adds rejection of legitimate users. UsingUserIdentity::isSameLogin()inWebServerAuthand in the session guard fixes items 1–3 together and leaves the collation defence intact — on the parent commit a Kelvin-sign LDAP identity authenticates as an existing local superuser, and the ASCII-only fold refuses it. (verified)Existing installs lock out on upgrade. The user row and the live session already hold the LDAP casing, so the first request after the update ends the session and refuses the login — super users included, with no in-app recovery short of editing
config.ini.phpor the database. (verified)New users are provisioned, then permanently locked out. With no Matomo row yet,
authenticate()resolves the empty row before synchronising and never re-compares, so synchronisation creates the user from the LDAP uid and the first request succeeds; every request after that is denied.REMOTE_USER=R3daveagainst LDAP uidr3davegives 200 once and 403 thereafter, fresh sessions included; same withstrip_domain_from_web_auth=1andSHIELD\R3dave. The parent commit serves that user indefinitely, and the new integration test does not cover it — it only uses logins that resolve to an existing user. (verified)
Auth/WebServerAuth.phpis byte-identical on5.x-dev, so #482 carries all of the above. 5.2.7 is not tagged yet, so it can be fixed there first and re-ported. (verified)Medium
A
REMOTE_USERthat strips to the empty string (SHIELD\,@shield.org) destroys a live session:getAssertedLogin()returns''and the guard only checks=== null. Before this branch the same value simply failed auth and left the session alone. (verified)
endSession()wipes$_SESSIONonly whenSession::isSessionStarted(), butSession::start()returns early without setting that flag when a session is already active or headers have been sent —destroyCurrentSession()then regenerates the id and carries the previous user's namespaces across, which is the leak the docblock says the method exists to prevent. The unit test cannot catch it: no session is started there, so the wipe never runs andSessionFingerprint::clear()empties the array on its own. (read)The collision refusal reaches the log only through
logger->debug(), so a locked-out user leaves no trace at the default level, whileUserSynchronizerlogs the equivalent refusal atwarning. (verified)
e0d7a1c to
6415336
Compare
|
@snake14 Can you re-review ? |
6415336 to
1ed97a8
Compare
|
Updated to That test arrives on 6.x with this PR, and #488 (now merged to 5.x, ported in #489) makes Without this, whichever of #484 and #489 merged second would have failed CI on that test. The two also both add bullets to the untagged 6.0.2 changelog section, so they conflict there and nowhere else. I merged both onto 6.x-dev locally to confirm: only that one changelog hunk, and with it resolved the suite is 198 tests, 431 assertions, OK with PHPStan clean. This branch on its own: 196 tests, 426 assertions, OK. |
snake14
left a comment
There was a problem hiding this comment.
Looks good aside from the missing AI checklist items. 👍
Ports #482 and #487 to 6.x-dev. Refs #AS-689.
Rebuilt on the current 6.x-dev tip: the branch previously carried only #482, which included the byte-exact login match that #487 removed, so it should not have merged as it stood. It is now a cherry-pick of all four AS-689 commits from 5.x (
148556b,397a653,e596d61,5778c30), squashed.Changes
UserIdentity::isSameLogin(), applied inAuth\BaseandLdapInterop\UserSynchronizer. The refusal logs atwarningso a locked-out user is visible at the default log level.Auth\WebServerSessionAuthends a Matomo session when the web server starts authenticating a different user, via the sameDI::decoratepattern the Cloud plugin uses.WebServerAuth::getAssertedLogin()is the single answer to who the web server asserted. It returns the value as given — trimming it would letironmanauthenticateironman, which the collation returns for it — and trims only to decide whether anybody was asserted.isCurrentRequestWebServerAuthenticated()and thecreateAppSpecificTokenAuthguard now go through it, so an assertion naming nobody no longer skips Matomo's password confirmation.Differences from the 5.x commits
Three, all deliberate:
*bullets 6.x uses.Base::makeAuthFailure()keepsnulland 6.x's@phpstan-ignore. The 5.x branch passes''there to satisfy PHPStan; 6.x already solved the same warning with an annotation reasoning that core'sLogin\Authpassesnulltoo and theAuthResult@paramis too narrow. I kept 6.x's decision rather than overrule it in a backport, so that one line differs between branches. The newWebServerSessionAuthpasses''since it is new code needing no annotation — say the word if you would rather both used the annotatednull.5.x-devin the touched files is pre-existing 6.x-only work — typed docblocks, other@phpstan-ignores, dropped BC shims — which I left alone.Verification
plugins/LoginLdap/tests/Unit— 196 tests, 426 assertions, OKMatomostandard — no errors on the changed production filesThe integration tests need the LDAP fixture and were not run here; the local core is Matomo 5.14.0-alpha rather than Matomo 6, so CI is the real check.
Still to port
#488 (#AS-730) is open against 5.x and touches
Base::synchronizeLdapUser()in this same file. It will need its own backport once it lands.Checklist