fix: treat a request parameter that is not a string as absent in auth

Saving a user from the web ui took the whole auth path down:

  PHP Fatal error: Uncaught TypeError: strcasecmp(): Argument #1 ($string1)
  must be of type string, array given in includes/auth.php:197
  #0 auth.php(197): strcasecmp()
  #1 auth.php(528): getAuthUser()
  #2 auth.php(687): userFromSession()

reached from ?view=user&uid=2. That page's form posts user[Username],
user[Password], user[Name] and the rest, so $_REQUEST['user'] is an array on
every save from it, and getAuthUser() read that parameter as the username to
filter on and handed it to strcasecmp(). Under PHP 8 a string function given an
array is a TypeError rather than a warning, so the request died with a 500.

The same shape arrives from anyone who cares to send it, and not only on a page
that needs a session. userFromSession() reads user, pass, username, password
and auth straight out of the request, and the credential branches run before
anyone is logged in, so ?username[]=x&password[]=y reaches validateUser() with
arrays on an install that has never seen the caller before.

requestString() returns a parameter only when it is a string and null
otherwise, which is what the callers already do with a parameter that was not
sent. An array is not a username, a password or an auth hash.

master no longer has the strcasecmp line the report names, so it does not fatal
in that exact spot, but it reads the same unvalidated values: $filterUser is
bound as a query parameter and the credentials still reach validateUser(). This
fixes the class rather than the one line, and backports to 1.38 where the
reported line lives.

The test lifts requestString() out of auth.php and evaluates it alone, because
including auth.php needs a database; test_auth_no_include_side_effects.php
sidesteps the same dependency the same way. 8 cases, covering the form's array,
the login parameters, a nested array and the strings that must still pass
through. 5 of them fail without this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 1f4aa1a16c1b63fdf34a6f2c6aef6590dbd45acd)

tests/php/test_auth_request_string.php

@@ -0,0 +1,83 @@
+<?php
+// Tests requestString() in web/includes/auth.php: the request parameters the
+// auth path reads have to be strings before anything does string work on them.
+//
+// The reported failure was a fatal, not a wrong answer:
+//
+//   PHP Fatal error: Uncaught TypeError: strcasecmp(): Argument #1 ($string1)
+//   must be of type string, array given in includes/auth.php:197
+//   #0 auth.php(197): strcasecmp()
+//   #1 auth.php(528): getAuthUser()
+//   #2 auth.php(687): userFromSession()
+//
+// reached from ?view=user&uid=2. That page's form posts user[Username],
+// user[Password] and the rest, so $_REQUEST['user'] is an array on every save,
+// and getAuthUser() handed it straight to strcasecmp(). The same shape arrives
+// from anyone who asks for it: ?username[]=x&password[]=y reaches the login
+// path with no session at all.
+//
+// Run: php tests/php/test_auth_request_string.php
+//
+// requestString() is pure, but including auth.php requires a database --
+// User.php pulls in database.php, which connects at include time -- so the
+// function is lifted out of the file and evaluated on its own, the same
+// dodge test_auth_no_include_side_effects.php makes for the same reason.
+$auth_src = file_get_contents(__DIR__.'/../../web/includes/auth.php');
+if ($auth_src === false) {
+  echo "FAIL could not read auth.php\n";
+  exit(1);
+}
+if (!preg_match('/^function requestString\(.*?^}$/ms', $auth_src, $matches)) {
+  echo "FAIL requestString() not found in auth.php\n";
+  exit(1);
+}
+eval($matches[0]);
+
+$failures = 0;
+$passes = 0;
+
+function check($name, $got, $expected) {
+  global $failures, $passes;
+  $got_str = var_export($got, true);
+  $expected_str = var_export($expected, true);
+  if ($got === $expected) {
+    $passes++;
+    echo "ok   $name\n";
+  } else {
+    $failures++;
+    echo "FAIL $name: expected $expected_str, got $got_str\n";
+  }
+}
+
+// A parameter the client did send, as a string, comes back unchanged.
+$_REQUEST = array('user' => 'admin');
+check('string passes through', requestString('user'), 'admin');
+
+// The empty string is still a string: callers decide what empty means, and the
+// login paths already test for it with empty().
+$_REQUEST = array('user' => '');
+check('empty string is kept', requestString('user'), '');
+
+// A parameter that was never sent.
+$_REQUEST = array();
+check('missing parameter is null', requestString('user'), null);
+
+// The shape the user edit form posts. This is the reported crash.
+$_REQUEST = array('user' => array('Username' => 'admin', 'Password' => 'secret'));
+check('form array is refused', requestString('user'), null);
+
+// The shape an attacker sends, on the paths that need no session.
+$_REQUEST = array('username' => array('admin'), 'password' => array('x'));
+check('username array is refused', requestString('username'), null);
+check('password array is refused', requestString('password'), null);
+
+// An auth hash is used to look up a user and compared against the session copy.
+$_REQUEST = array('auth' => array('deadbeef'));
+check('auth array is refused', requestString('auth'), null);
+
+// Anything else PHP can put in a request parameter.
+$_REQUEST = array('user' => array(array('nested')));
+check('nested array is refused', requestString('user'), null);
+
+echo "\n$passes passed, $failures failed\n";
+exit($failures ? 1 : 0);

web/includes/auth.php

@@ -29,6 +29,25 @@ require_once('Role_Monitor_Permission.php');
 require_once(__DIR__.'/../vendor/autoload.php');
 use \Firebase\JWT\JWT;
 
+// A request parameter as a string, or null when the client did not send one it
+// can be used as. Request values are whatever arrived: "?user[]=x" makes
+// $_REQUEST['user'] an array, and the first string operation on an array is a
+// fatal TypeError under PHP 8.
+//
+// Not only hostile input. The user edit form posts user[Username],
+// user[Password] and the rest, so $_REQUEST['user'] is an array on every save
+// from that page, which was enough to take the whole auth path down with
+// "strcasecmp(): Argument #1 must be of type string, array given".
+//
+// An array is not a username, a password or an auth hash, so callers treat it
+// the same as a parameter that was never sent.
+function requestString($key) {
+  if (!isset($_REQUEST[$key])) {
+    return null;
+  }
+  return is_string($_REQUEST[$key]) ? $_REQUEST[$key] : null;
+}
+
 function password_type($password) {
   if (!$password || $password === '') {
     return 'plain';
@@ -226,7 +245,8 @@ function getAuthUser($auth) {
     // Prefer the username from the URL (matches what zms uses) so PHP and the
     // C++ side query the same row. Fall back to the session username for
     // page-internal calls that don't carry user= on the URL.
-    $requestedUser = !empty($_REQUEST['user']) ? $_REQUEST['user'] : null;
+    $requestedUser = requestString('user');
+    if ($requestedUser === '') $requestedUser = null;
     $sessionUser = isset($_SESSION['username']) ? $_SESSION['username'] : null;
     $filterUser = $requestedUser !== null ? $requestedUser : $sessionUser;
 
@@ -640,28 +660,33 @@ function zm_authenticate_request() {
         zm_session_start();
       }
 
-      if (ZM_AUTH_HASH_LOGINS && empty($user) && !empty($_REQUEST['auth'])) {
-        $user = getAuthUser($_REQUEST['auth']);
+      $requestAuth = requestString('auth');
+      $requestUser = requestString('user');
+      $requestPass = requestString('pass');
+      $requestUsername = requestString('username');
+      $requestPassword = requestString('password');
+      if (ZM_AUTH_HASH_LOGINS && empty($user) && !empty($requestAuth)) {
+        $user = getAuthUser($requestAuth);
         if ($user) {
           $remoteAddr = ZM_AUTH_HASH_IPS ? $_SESSION['remoteAddr'] : '';
-          if (isset($_SESSION['AuthHash'.$remoteAddr]) and ($_SESSION['AuthHash'.$remoteAddr] != $_REQUEST['auth'])) {
+          if (isset($_SESSION['AuthHash'.$remoteAddr]) and ($_SESSION['AuthHash'.$remoteAddr] != $requestAuth)) {
             unset($_SESSION['AuthHashGeneratedAt']);
             unset($_SESSION['AuthHash'.$remoteAddr]);
           }
           $_SESSION['username'] = $user->Username();
         }
-      } else if (!(empty($_REQUEST['user']) or empty($_REQUEST['pass']))) {
+      } else if (!(empty($requestUser) or empty($requestPass))) {
         # The shortened versions are used in auth_relay = PLAIN
-        $ret = validateUser($_REQUEST['user'], $_REQUEST['pass']);
+        $ret = validateUser($requestUser, $requestPass);
         if (!$ret[0]) {
           ZM\Warning($ret[1]);
           $user = null; // null, not unset: in a function unset() drops only the local binding
           return;
         }
         $user = $ret[0];
-      } else if (!(empty($_REQUEST['username']) or empty($_REQUEST['password']))) {
+      } else if (!(empty($requestUsername) or empty($requestPassword))) {
         # Longer versions are used on login page
-        $ret = validateUser($_REQUEST['username'], $_REQUEST['password']);
+        $ret = validateUser($requestUsername, $requestPassword);
         if (!$ret[0]) {
           ZM\Warning($ret[1]);
           $user = null; // null, not unset: in a function unset() drops only the local binding
@@ -710,8 +735,8 @@ function zm_authenticate_request() {
         # Drop the pre-auth session and issue a fresh id in a single Set-Cookie
         zm_session_regenerate_id_login();
 
-        $username = $_REQUEST['username'];
-        $password = $_REQUEST['password'];
+        $username = $requestUsername;
+        $password = $requestPassword;
 
         ZM\Info("Login successful for user \"$username\"");
         #ZM\Audit("user=$username action=login id=".$user->Id()." from=".($_SERVER['REMOTE_ADDR'] ?? 'local'));