Skip to content

Port AS-689 follow-up hardening to 6.x - #484

Merged
AltamashShaikh merged 1 commit into
6.x-devfrom
port-as689-followup-6x
Sep 10, 2026
Merged

Port AS-689 follow-up hardening to 6.x#484
AltamashShaikh merged 1 commit into
6.x-devfrom
port-as689-followup-6x

Conversation

@AltamashShaikh

@AltamashShaikh AltamashShaikh commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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

  • The login comparison no longer folds Unicode, replaced with an ASCII-only fold in the new UserIdentity::isSameLogin(), applied in Auth\Base and LdapInterop\UserSynchronizer. The refusal logs at warning so a locked-out user is visible at the default log level.
  • Web server auth resolves the login before synchronizing, so a login resolving to a different existing user is rejected before sync writes to that user's row.
  • Auth\WebServerSessionAuth ends a Matomo session when the web server starts authenticating a different user, via the same DI::decorate pattern 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 let ironman authenticate ironman, which the collation returns for it — and trims only to decide whether anybody was asserted. isCurrentRequestWebServerAuthenticated() and the createAppSpecificTokenAuth guard 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:

  1. Version and changelog folded into the untagged 6.0.2 section, using the * bullets 6.x uses.
  2. Base::makeAuthFailure() keeps null and 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's Login\Auth passes null too and the AuthResult @param is too narrow. I kept 6.x's decision rather than overrule it in a backport, so that one line differs between branches. The new WebServerSessionAuth passes '' since it is new code needing no annotation — say the word if you would rather both used the annotated null.
  3. Everything else that still differs between this branch and 5.x-dev in 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, OK
  • PHPStan level 5 — no errors
  • PHPCS Matomo standard — no errors on the changed production files

The 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

  • [✔] 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?

@snake14 snake14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Please check these AI items:

Blocking

  1. The exact-match override buys no security. matomo_user.login is a PRIMARY KEY under a case- and accent-insensitive collation, so two logins differing only by ASCII case cannot both exist — inserting R3dave beside r3dave fails with a duplicate-key error. isSameLogin()'s ASCII-only fold already rejects everything the collation folds, so isSameLoginExact() only adds rejection of legitimate users. Using UserIdentity::isSameLogin() in WebServerAuth and 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)

  2. 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.php or the database. (verified)

  3. 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=R3dave against LDAP uid r3dave gives 200 once and 403 thereafter, fresh sessions included; same with strip_domain_from_web_auth=1 and SHIELD\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)

  4. Auth/WebServerAuth.php is byte-identical on 5.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

  1. A REMOTE_USER that 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)

  2. endSession() wipes $_SESSION only when Session::isSessionStarted(), but Session::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 and SessionFingerprint::clear() empties the array on its own. (read)

  3. The collision refusal reaches the log only through logger->debug(), so a locked-out user leaves no trace at the default level, while UserSynchronizer logs the equivalent refusal at warning. (verified)

@AltamashShaikh

Copy link
Copy Markdown
Contributor Author

@snake14 Can you re-review ?

Ports #482 and #487 together: the login comparison no longer folds
Unicode, web server auth resolves the login before synchronizing, a
Matomo session ends when the web server starts authenticating a
different user, and one call answers who the web server asserted.
@AltamashShaikh

Copy link
Copy Markdown
Contributor Author

Updated to 1ed97a8, which adds the f3214e5 adaptation from #488 to tests/Unit/WebServerAuthLoginResolutionTest.php.

That test arrives on 6.x with this PR, and #488 (now merged to 5.x, ported in #489) makes Base::synchronizeLdapUser() read the user row back after access synchronization. The test's UserModel::getUser() stub returned a fixed value, so in the provisioning case it kept reporting no user after synchronization had created one and the re-read failed the login. The stub now reports the user once synchronization creates it.

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 snake14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good aside from the missing AI checklist items. 👍

@AltamashShaikh
AltamashShaikh merged commit bb60214 into 6.x-dev Sep 10, 2026
22 of 23 checks passed
@AltamashShaikh
AltamashShaikh deleted the port-as689-followup-6x branch September 10, 2026 10:17
AltamashShaikh added a commit that referenced this pull request Sep 10, 2026
Ports #488. The commit that adapts WebServerAuthLoginResolutionTest is
left out: that test arrives on 6.x with #484, which carries the adapted
version instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants