Skip to content

fix: Use strict comparaison - #170

Merged
moodyjmz merged 1 commit into
Euro-Office:mainfrom
CarlSchwan:carl/strict-comparaison
Sep 10, 2026
Merged

moodyjmz merged 1 commit into
Euro-Office:mainfrom
CarlSchwan:carl/strict-comparaison

Conversation

@CarlSchwan

Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

@moodyjmz moodyjmz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving. One of the two lines is a real fix, the other is cosmetic — both correct.

lib/Controller/EditorController.php:372 is a genuine bug fix. PHP compares two numeric strings numerically, so "007" == "7" evaluates to true (verified on 8.x; this repo supports 8.1–8.4). A distinct, authorized user whose UID numerically collided with the current user's was being silently filtered out of the mention/protect list. The current user is still never self-listed, since identical UID strings compare equal under !== too.

lib/Controller/CallbackController.php:200 is cosmetic. It sits inside if ((!empty($user) && !$file->isReadable()) || !$canDownload), which returns 403 unconditionally, and guards only a logger->error() call. The one reachable delta is that a $hashData->userId of "" with no session user now logs where it previously didn't (null == "" is true). No behavioural consequence beyond a log line.

No security or performance dimension either way. Neither line is an access-control decision, and on the filterUser() path the un-excluded user is already gated behind getAccessList($file) in the one branch and $canShare in the other, so nothing is disclosed that wasn't already.

For the record, these were the last two loose comparisons in the PHP source — a per-occurrence grep across the tree leaves only lock != 1 inside a SQL string (lib/KeyManager.php:82) and a != inside a log message (lib/FileVersions.php:171), neither of which is a comparison. So the change is complete for what it claims.

Two follow-ups filed out of this review, neither of which should hold up this PR:

  • #173filterUser() is declared IUser but both callers pass the nullable result of IUserManager::get(). Pre-existing, same function this PR touches.
  • #174 — no static analyser in CI, which is why loose comparisons and nullable-into-non-nullable get found by hand rather than wholesale.

@moodyjmz
moodyjmz merged commit 4211fb9 into Euro-Office:main Sep 10, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants