Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exceptions which block adding 2FA factor
ClosedPublic

Authored by aklapper on May 12 2023, 17:06.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 06:08
Unknown Object (File)
Sun, Mar 24, 06:37
Unknown Object (File)
Wed, Mar 20, 10:40
Unknown Object (File)
Tue, Mar 12, 15:02
Unknown Object (File)
Fri, Mar 8, 12:27
Unknown Object (File)
Feb 25 2024, 07:38
Unknown Object (File)
Feb 25 2024, 07:38
Unknown Object (File)
Feb 25 2024, 07:38

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.

Closes T15388

Test Plan

Applied these two changes; afterwards managed to add a 2FA factor and /settings/user/username/page/multifactor/?id=1 correctly listed mys Authentication Factors.

Additional tests:

  • Unset any eventual personal MFA
  • Setup a personal MFA
  • Login/Logout using the MFA
  • Remove a personal MFA
  • Setup a personal enroll message from /auth/mfa/1/
  • Setup a personal MFA
  • Login/Logout using the MFA
  • (then cleanup removing your test MFA)

Diff Detail

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

Event Timeline

adopt empty string default value, avoiding null

valerio.bozzolan edited the test plan for this revision. (Show Details)

I'm quite sure we do not cause any nuclear implosion here. Added some things in the test plan

Thank you again!

src/applications/auth/factor/PhabricatorAuthFactor.php
418

✅ I verified the above line

The method AphrontRequest::getStr($something, '') assures that we only receive something that is never null. Since internally it does an isset() lookup against the potentially returned value, excluding null.

src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
260

✅ I verified the above line that is similar to D25218

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

This revision is now accepted and ready to land.May 21 2023, 10:08