Uses the same login comparison everywhere

[AltamashShaikh]

Addresses review feedback on #484, applied to 5.x first since 5.2.7 is not tagged yet. #484 will be re-ported from this. Refs #AS-689.

The exact match is removed

WebServerAuth and the session guard used UserIdentity::isSameLoginExact(). The reasoning was that upstream authenticators treat logins differing in ASCII case as separate principals, so REMOTE_USER=karen should not authenticate as stored superuser Karen.

That defence is worth less than it costs. matomo_user.login is a primary key under a case- and accent-insensitive collation, so karen and Karen cannot both exist as Matomo users. For the attack to be possible, the upstream would have to treat them as different people while the directory this plugin authenticates against treats them as one entry — LDAP uid matching is case-insensitive, so it does not. The ASCII fold still refuses everything the collation folds beyond ASCII case, which is the reported issue: a Kelvin-sign identity resolving to an existing local superuser is rejected.

Against that, the exact match broke two things:

• Existing installs locked out on upgrade. Where the stored login is cased differently from REMOTE_USER, the first request after the update ended the session and refused the login, superusers included, with no in-app recovery.
• New users were provisioned and then locked out. Base::synchronizeLdapUser() assigns $this->userForLogin directly, so after synchronization getUserForLogin() returns the cached row without re-comparing and the first request succeeded. Every request after that resolved the now-existing row and was refused.

Both call sites now use UserIdentity::isSameLogin(), and isSameLoginExact() is gone.

Also fixed

• A REMOTE_USER that strips to the empty string (SHIELD\, @shield.org) destroyed a live session: getAssertedLogin() returns '' and the guard only tested === null. Such a value is now treated as no assertion, as it was before this work.
• endSession() wiped $_SESSION only when Session::isSessionStarted(). That flag stays false whenever Session::start() returned early — a session already active, or headers sent — while $_SESSION is populated regardless, so destroyCurrentSession() carried the previous user's namespaces across. The wipe is now unconditional.
• The refusal reached the log only through logger->debug(), so a locked-out user left no trace at the default level while UserSynchronizer logs the equivalent at warning. It now logs at warning too.
• new AuthResult(AuthResult::FAILURE, null, null) passes '' for both, which AuthResult documents as string. This is the change Port AS-689 follow-up hardening to 6.x #484 already carries; making it here keeps the branches identical.

Tests

tests/Unit/WebServerAuthLoginResolutionTest.php is new and covers the two lockouts directly — I confirmed both of its positive cases fail when the exact match is reinstated and pass with the fold, and that the three collation-collision cases (accent, Kelvin sign, trailing space) are refused either way.

The integration test's ascii case difference row moved from the refusal provider to its own passing test, and a provisioning test was added there too. Neither integration test could be run locally — the LDAP fixture is unavailable here, and five pre-existing tests in that file fail for the same reason on an untouched 5.x-dev.

Plugin unit suite: 160 tests, 364 assertions, OK. PHPCS clean on the changed production files. PHPStan reports nothing in the changed files; the 20 errors it reports across 9 other files are pre-existing on 5.x-dev, which has a phpstan.neon but no PHPStan workflow.

Checklist

• [✔] Tested locally or on demo2/demo3?
• [✔] New test case added/updated?
• [✔] Are all newly added texts included via translation?
• [NA] Are text sanitized properly? (Eg use of v-text v/s v-html for vue)
• [✔] Version bumped?
• [✔] I have understood, reviewed, and tested all AI outputs before use
• [✔] All AI instructions respect security, IP, and privacy rules
• [✖] Documentation updated?

[AltamashShaikh]

@snake14 Can you check this one ?

[snake14]

Looking good. Please check whether these AI items are worth addressing:

1. Nothing tests the endSession() change. setUp() seeds $_SESSION with only the fingerprint key, which SessionFingerprint::clear() removes anyway, so assertSessionWasEnded() passes with or without the unconditional wipe — I restored the isSessionStarted() gate and the suite stayed 12/12 green. One extra key in setUp() that clear() does not touch would cover it.

2. A trailing space in REMOTE_USER is still an unrecoverable lockout, in the same shape as the SHIELD\ case. The guard trims only for the emptiness test, then compares the untrimmed value, so 'ironman ' ends the session and WebServerAuth refuses it too — and since REMOTE_USER is non-empty the login form is never reached, so it repeats on every request. Confirmed over real HTTP. Pre-existing rather than introduced here, so happy for it to be a separate ticket, but it is the same class of problem: either trim() once in getAssertedLogin() so both sides agree, or note the residual case in the guard's comment.

3. Base::isSameLogin() is now a dead seam. Nothing overrides it any more and the other two call sites already use UserIdentity::isSameLogin() directly, so deleting it would make the comparison literally the same everywhere.

4. Base::makeAuthFailure() still passes null where AuthResult documents a string — the same fix you made in WebServerSessionAuth, and PHPStan already flags it at Auth/Base.php:397.

5. Minor: the comment above the $_SESSION wipe only holds for the "session already active" case, not the headers_sent() one; and the 5.2.7 changelog heading is missing the space before the dash.

[AltamashShaikh]

All six addressed in e596d61. Thanks — the first one was the important catch.

The endSession() test was vacuous. Confirmed: setUp() seeded only SessionFingerprint::USER_NAME_SESSION_VAR_NAME, which clear() removes, so assertEquals(array(), $_SESSION) held either way. The fixture now also carries a Piwik_Login namespace that clear() leaves alone, and assertSessionWasEnded() asserts it is gone while assertSessionWasKept() asserts it survives. I re-ran your check — restoring the isSessionStarted() gate now fails four tests rather than none.

Trailing space. Fixed here rather than deferred, since it is an unrecoverable lockout and the fix is one line. getAssertedLogin() now trims after stripping the domain and returns null when nothing is left, so SHIELD\, @shield.org and " " all report "nobody" and a padded ironman resolves to ironman. That makes the guard and WebServerAuth agree on every value by construction, so the trim() in the guard's emptiness test is gone along with the residual case its comment would have had to describe. Covered by data providers on both sides. Say the word if you would still rather this were its own ticket and I will split it out.

Base::isSameLogin() deleted; getUserForLogin() calls UserIdentity::isSameLogin() directly, so all three call sites are now literally the same comparison.

Base::makeAuthFailure() passes ''. PHPStan on this branch is down from 20 errors to 17, and Auth/Base.php:397 is gone; the one remaining in that file is getTokenAuth() at line 175, which is pre-existing and about the property's nullability rather than this work, so I left it.

Minor. The wipe comment now only claims the "session already active" case. Changelog heading spacing fixed.

Unit suite 164 tests / 387 assertions. The integration tests still cannot run here — no LDAP fixture on this machine, where five pre-existing tests in that file fail on an untouched 5.x-dev too.

[snake14]

Thank you @AltamashShaikh Please check these AI items:

Blocking

1. Auth/WebServerAuth.php:171-177 trims REMOTE_USER before identity validation. Thus ironman or ironman becomes ironman and authenticates that account, bypassing the existing trailing-space/collation collision protection. The new whitespace-success tests codify this regression. Preserve the raw value for identity comparison and only use trimming to detect an empty assertion.

2. getAssertedLogin() now returns null for values such as SHIELD\ and whitespace, so WebServerSessionAuth keeps the existing session (Auth/WebServerSessionAuth.php:91-103). However, isCurrentRequestWebServerAuthenticated() still treats any non-empty raw REMOTE_USER as authenticated (Auth/WebServerAuth.php:180-183). LoginLdap::confirmCurrentUserPassword() then skips password verification, creating a fail-open password-confirmation bypass for malformed assertions. Use the same canonical assertion check everywhere and add regression coverage.

[AltamashShaikh]

Both blocking items were right — fixed in 5778c30.

1. Trimming the identity was a regression, and my own doing. The previous round flagged the trailing-space case as a lockout and offered "trim once in getAssertedLogin() so both sides agree, or note the residual case in the guard's comment". I took the trim, which quietly re-opened the collation collision for whitespace: under a PAD SPACE collation getUser('ironman ') returns ironman, and trimming turned the comparison that refuses it into one that accepts it. The whitespace-success tests I added codified exactly that, as you say.

getAssertedLogin() now returns the value the web server gave, and trims only to decide whether anybody was asserted at all. So SHIELD\, @shield.org and " " still name nobody, while ironman stays ironman and is refused. The whitespace cases moved from the success providers to the refusal providers on both sides.

That leaves the residual you offered as the alternative: a REMOTE_USER differing from the session's user only by surrounding whitespace ends the session and cannot then authenticate. It is the same outcome as any other login WebServerAuth refuses, and it is now stated in the guard's comment rather than papered over. Happy to raise it separately if you would rather it were handled than documented.

2. Fail-open on malformed assertions. Confirmed. isCurrentRequestWebServerAuthenticated() tested !empty($_SERVER['REMOTE_USER']) while getAssertedLogin() had started reporting the same values as naming nobody, so the two disagreed and password confirmation was skipped for an assertion that authenticates nobody. It now goes through getAssertedLogin() !== null.

LoginLdap::onApiRequestDispatch() had its own copy of the old test, so the createAppSpecificTokenAuth guard would have disagreed too — it now calls the same helper. The three existing callers (API.php:181, config/config.php:18, Controller.php:79) already did.

Coverage. tests/Unit/WebServerAuthAssertionTest.php is new and pins both: getAssertedLogin() returns null for every assertion naming nobody and returns the login untrimmed for those naming somebody, and isCurrentRequestWebServerAuthenticated() agrees with it in both directions. I checked it earns its place — reinstating the trim and the old !empty() check fails 8 tests across the suite.

179 tests, 407 assertions. PHPCS clean.

[snake14]

Looking good @AltamashShaikh . Can you please check whether this is really a blocking issue?

Blocking

• UserIdentity::isSameLoginExact() is removed from the public class without deprecation or a compatibility wrapper (UserIdentity.php:30). It was introduced in released version 5.2.6, so external plugins calling it will fail with an undefined-method error after upgrading. Keep a deprecated wrapper until a breaking release.

[AltamashShaikh]

@snake14 isSameLoginExact was added recently so safe to remove it. Can you please re-review ?

[snake14]

LGTM 👍