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

Authored by aklapper on May 12 2023, 17:06.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15388: PHP 8.1 "strlen(null)" exceptions block adding 2FA factor

Commits

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

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

2fauser (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 401
Build 401: arc lint + arc unit

Event Timeline

aklapper created this revision.May 12 2023, 17:06

Owners added a reviewer: O1: Blessed Committers.May 12 2023, 17:06

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 12 2023, 17:06

aklapper requested review of this revision.May 12 2023, 17:06

Harbormaster completed remote builds in B401: Diff 724.May 12 2023, 17:06

aklapper mentioned this in T15397: PHP 8.1 "explode(null)" exception blocks rendering "Provide Multi-Factor Credentials" page.May 13 2023, 13:43

valerio.bozzolan mentioned this in Wikimedia Hackaton 2023 Athens Code Sprint.May 18 2023, 10:16

adopt empty string default value, avoiding null

Harbormaster completed remote builds in B436: Diff 778.May 21 2023, 09:57

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.