Strpos() throwing in PhabricatorAuthPasswordEngine

Reproduction Instructions
Run a phabricator instance using PHP 7.4 on a domain name having a numeric part.
Try to set a password for a user, e.g. by setting a VCS password for a bot user.

This will throw a RuntimeException in strpos() because one of the $terms used a needles will be an integer, not a string.

Full stack trace:

"PHP message: [2020-11-02 19:41:38] EXCEPTION: (RuntimeException) strpos(): Non-string needles will be interpreted as strings in the future. Use an explicit chr() call to preserve the current behavior at [<arcanist>/src/error/PhutilErrorHandler.php:263]
PHP message: arcanist(head=stable, ref.master=c04f141ab023, ref.stable=ac54d61d7af2), phabricator(head=stable, ref.master=5dc4e76eb9f8, ref.stable=86ad69863930)
PHP message:   #0 <#2> PhutilErrorHandler::handleError called at [<arcanist>/src/error/PhutilErrorHandler.php:263]
PHP message:   #1 <#2> strpos called at [<phabricator>/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php:185]
PHP message:   #2 <#2> PhabricatorAuthPasswordEngine::checkNewPassword called at [<phabricator>/src/applications/diffusion/panel/DiffusionSetPasswordSettingsPanel.php:93]
PHP message:   #3 <#2> DiffusionSetPasswordSettingsPanel::processRequest called at [<phabricator>/src/applications/settings/controller/PhabricatorSettingsMainController.php:107]
PHP message:   #4 <#2> PhabricatorSettingsMainController::handleRequest called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:288]
PHP message:   #5 phlog called at [<phabricator>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
PHP message:   #6 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:752]
PHP message:   #7 AphrontApplicationConfiguration::handleThrowable called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:300]
PHP message:   #8 AphrontApplicationConfiguration::processRequest called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:208]
PHP message:   #9 AphrontApplicationConfiguration::runHTTPRequest called at [<phabricator>/webroot/index.php:35]"

I actually prepared a diff, but I couldn’t find instructions on how to run unit tests (and I’m not sure I wouldn’t have been stopped at some point due to a lack of permissions):

The commit:

diff --git a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
index f763b0987f..603bd94e10 100644
--- a/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthPasswordEngine.php
@@ -172,7 +172,7 @@ final class PhabricatorAuthPasswordEngine
     // Normalize terms for comparison.
     $normal_map = array();
     foreach ($terms_map as $term => $source) {
-      $term = phutil_utf8_strtolower($term);
+      $term = (string)phutil_utf8_strtolower($term);
       $normal_map[$term] = $source;
     }

And the diff I wrote:

fix: Fix use of non-string values for strpos()’ $needle value.

Summary:
Terms generated from the blocklist and compared to passwords might be of type other than string.
The phutil_utf8_* family of functions do not cast to string either, which makes recent versions
of PHP (e.g. 7.4) throw a RuntimeException:

strpos(): Non-string needles will be interpreted as strings in the future. Use an explicit chr() call to preserve the current behavior

This happens e.g. when defining a VCS password for a user and when part of your phabricator instance’s domain is a numeric value.
For example:

array() {
  ["cator.1024.lu"]=> string(13) "cator.1024.lu"
  ["cator"]=> string(13) "cator.1024.lu"
  [1024]=> string(13) "cator.1024.lu"
}

Note: I think it could be debated whether functions named e.g. phutil_utf8_strtolower shouldn’t return strings since their names kind of imply that they would. So instead of casting to a string value here, perhaps this should happen in utils/utf8.php` instead.

Phabricator/Arcanist Version

Phabricator Version Information	
Library	Version	Date	Branchpoint
phabricator	86ad69863930	Mon, Oct 19	b2e96df3a3be
arcanist	ac54d61d7af2	Mon, Oct 19	4b3baca999a4
Other Version Information	
Binary	Version	Path
php	7.4.7	fpm-fcgi

I couldn’t find instructions on how to run unit tests

$ arc unit


I’m a little confused here, because I don’t think the patch works as written (did you actually get a chance to test it?). As far as I can tell, phutil_utf8_strtolower() can already only ever return a string, so adding an explicit (string) cast never has any effect: the patch always casts a string to a string.

The problem isn’t with phutil_utf8_strtolower(), but with PHP implicitly casting the numeric array key into an integer. Here’s a script which demonstrates the behavior:

$ cat test.php 
<?php

$map = array();
$map[1] = true;
$map["2"] = true;

foreach ($map as $k => $v) {
  var_dump($k);
}

This produces:

$ php -f test.php 
int(1)
int(2)

Note that “2” was inserted into the array as a string, but is an integer when retrieved. This behavior appears consistent across all versions of PHP:

https://3v4l.org/eCqZp

Instead, I think the fix should be something like:

  • cast the term to a string after selecting it from the array; or
  • store the term as a value rather than a key, so PHP never performs an implicit cast; or
  • perhaps provide some StringMap primitive which guarantees string keys.

I’ve made a note about this in https://secure.phabricator.com/T2312. In the general case, we can’t detect this problem (“insertion of a string into an array key causes implicit conversion to integer and discards type”) without modifying the PHP runtime, but I think I favor introducing typed map primitives as a solution.

For now, I added a phutil_string_cast(...) after we pull $term out of the array, in https://secure.phabricator.com/D21487. I believe this will fix the immediate issue. This change is now in master.

And thanks for the report!