Paths

Table of Contentst

Differential D25230

Fix PHP 8.1 "strlen(null)" exception trying to create an empty Menu Item
ClosedPublic
Actions

Authored by aklapper on May 14 2023, 10:17.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15398: PHP 8.1 "strlen(null)" exception trying to create an empty Menu Item

Commits

rPb1edfea09bad: Fix PHP 8.1 "strlen(null)" exception trying to create an empty Menu Item

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 T15398

Test Plan

Applied this change and "Edit Menu Item" overlay dialog correctly shows "You must choose a project" resp. "You must choose a room." error message, now without an exception.

Go to /home/menu/configure/custom/ and also to /home/menu/configure/global/ and try:

Add Label with name 0 / 1 / 1.123 etc.
Add Label, Save again
Add Link, Save again
Add Application, Save again
Add Divider, Save again
Add Conpherence, Save again
Add Link, Save Again
Add Motivator (Cat Facts), Save again
Add Form, Save again
Order Items
Activate/Disable Built-in Home (unrelated)
Remove all the above

Also, tried on a Project menu.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 14 2023, 10:17

Owners added a reviewer: O1: Blessed Committers.May 14 2023, 10:17

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

aklapper requested review of this revision.May 14 2023, 10:17

Harbormaster completed remote builds in B415: Diff 739.May 14 2023, 10:17

TBH, I'm confused about $xaction being treated as an array in line 126.

I was able to track it down to PhabricatorProfileMenuEditor::validateTransaction(), which is using "regular" transactions and converts them to an array in case the xaction is of type PhabricatorProfileMenuItemConfigurationTransaction::TYPE_PROPERTY.

At this point, I think the content might be generated in javascript, which means all bets are off.

To complete the testing, please try creating/moving/hiding/showing these kinds of menu items:

"Label" menu item with a label 0 and something else that might be converted to an int
a "Link" item
a "Divider" item

That should cover all complex/surprise possible values of $xaction['new'].

Thanks!

Just to note that, if we will be able to crash this, here is the candidate resolution:

https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/storage/PhabricatorModularTransactionType.php$347-358

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

valerio.bozzolan edited the test plan for this revision. (Show Details)May 21 2023, 07:19

valerio.bozzolan edited the test plan for this revision. (Show Details)May 21 2023, 07:23

valerio.bozzolan edited the test plan for this revision. (Show Details)May 21 2023, 07:30

Thanks avivey, I share the same concerns. Hoping to be useful I've followed these tips to expand the test plan a bit.

I tested this locally. Kind of re-ordering stuff in Personal / Global menus and do fuzzy tests. Limited examples:

Phorge menu items tests.png (885×717 px, 54 KB)

Phorge home menu test.png (440×222 px, 11 KB)

I'm also available to hot-patch this immediately if somebody will report any future issue with any unknown alien corner case.

Thanks

sgtm

This revision is now accepted and ready to land.May 21 2023, 07:41

Closed by commit rPb1edfea09bad: Fix PHP 8.1 "strlen(null)" exception trying to create an empty Menu Item. · Explain WhyMay 23 2023, 10:00

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rPb1edfea09bad: Fix PHP 8.1 "strlen(null)" exception trying to create an empty Menu Item.

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

search/

menuitem/

PhabricatorProfileMenuItem.php

2 lines

O1: Blessed Committers

Diff 791

View Options

src/applications/search/menuitem/PhabricatorProfileMenuItem.php

Fix PHP 8.1 "strlen(null)" exception trying to create an empty Menu ItemClosedPublicActions