Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" and "explode()" exceptions which block rendering Administrator Account Creation page
ClosedPublic

Authored by aklapper on May 2 2023, 18:55.
Tags
None
Referenced Files
F2071683: D25175.diff
Fri, Mar 29, 07:42
Unknown Object (File)
Tue, Mar 26, 08:37
Unknown Object (File)
Tue, Mar 26, 08:37
Unknown Object (File)
Tue, Mar 26, 08:37
Unknown Object (File)
Tue, Mar 26, 07:57
Unknown Object (File)
Tue, Mar 26, 07:28
Unknown Object (File)
Sun, Mar 24, 06:26
Unknown Object (File)
Wed, Mar 20, 10:31

Details

Summary

strlen() was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts phutil_nonempty_string() as a replacement.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Similarly, explode(string $separator, string $string, int $limit) does not accept
passing null instead of an actual string as input parameter either anymore.

Closes T15284

Test Plan

Applied these two changes. Afterwards, admin user account was created and Phorge homepage rendered in web browser on a fresh installation.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25175
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 404
Build 404: arc lint + arc unit

Event Timeline

aklapper requested review of this revision.May 2 2023, 18:55

I am not sure about the assumption I made about the false return value of isRawCacheDataValid().

Thanks for this patch

To me it seems that your assumption is totally legit

I tested this locally and this does not introduce nuclear implosions

I accept as behalf of myself and not as O1 since I shared some strong positive facts but Phorge probably deserves a stronger opinion

src/applications/auth/constants/PhabricatorCookies.php
167

✅ I logged this variable with phlog() to verify that $cookie just assumes values like null or strings like "1683107655,/" etc.

The function phutil_nonempty_string() will report any alien type, and that is OK.

src/applications/people/cache/PhabricatorUserProfileImageCacheType.php
94–96

Before PHP 8.1 this was happening, when $data was null:

$parts = explode(',', null, 2);                      // array('')
$version = reset($parts);                            // string(0) ""
return ($version === $this->getCacheVersion($user)); // false

Also note that $this->getCacheVersion($user) seems that it never returns an empty string so this is really always false when $data is null

99–107

Internal note: this method never returns an empty string or null

valerio.bozzolan retitled this revision from Fix PHP 8.1 "strlen(null)" and "explode" exceptions which block rendering Administrator Account Creation page to Fix PHP 8.1 "strlen(null)" and "explode()" exceptions which block rendering Administrator Account Creation page.May 4 2023, 12:34
valerio.bozzolan edited the summary of this revision. (Show Details)
src/applications/people/cache/PhabricatorUserProfileImageCacheType.php
99–100

Thanks. Also here maybe better a minimal strict short-circuit check like

if ($data === null) {
  return false;
}

To avoid type juggling with string "0" etc.

Add minimal strict short-circuit check as proposed by Valerio

Thanks for updating

I tested this intensively locally doing login / logout etc. without any nuclear implosion

Thanks for this patch that seems 100% legit to me

sgtm

This revision is now accepted and ready to land.May 19 2023, 14:31