Paths

Table of Contentst

-
src/applications/auth/provider/
-
applications/
-
auth/
-
provider/
-
PhabricatorLDAPAuthProvider.php

Differential D25748

Fix PHP 8.1 "strlen(null)" exception on LDAP login without password
ClosedPublic
Actions

Authored by aklapper on Jul 26 2024, 14:46.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15893: PHP 8.1 "strlen(null)" exception for LDAP login without password

Commits

rP7909f6a91937: Fix PHP 8.1 "strlen(null)" exception on LDAP login without password

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.

Note: This patch also corrects two further strlen() occurrences with the same pattern.

ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php:145]

Closes T15893

Test Plan

Create an LDAP user without setting their password; try to log into Phabricator with that user via the LDAP auth provider.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Jul 26 2024, 14:46

Owners added a reviewer: O1: Blessed Committers.Jul 26 2024, 14:46

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJul 26 2024, 14:46

aklapper requested review of this revision.Jul 26 2024, 14:46

Harbormaster completed remote builds in B1458: Diff 2275.Jul 26 2024, 14:46

Thaaaanks. I wonder why $has_password has a dedicated variable, and instead $has_username has not 🤔

Feel free to introduce $has_username if your moon isn't full right now 👍 both for readability and micro-optimization.

This revision is now accepted and ready to land.Jul 26 2024, 16:09

The $password variable is not a string anymore in the line after. I guess it's not much more expensive to check if the string $username is nonempty versus comparing to a boolean value

Yup. Laaaand

Closed by commit rP7909f6a91937: Fix PHP 8.1 "strlen(null)" exception on LDAP login without password. · Explain WhyJul 27 2024, 04:47

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rP7909f6a91937: Fix PHP 8.1 "strlen(null)" exception on LDAP login without password.

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

auth/

provider/

PhabricatorLDAPAuthProvider.php

6 lines

O1: Blessed Committers

Diff 2276

View Options

src/applications/auth/provider/PhabricatorLDAPAuthProvider.php

This file was added.

				<?php

				/**
				* Wrapper for the PhutilURI class to be aware of the relative/absolute context
				* and maybe other things in the future.
				*/
				final class PhutilURIGoodie extends Phobject {

				/**
				* Dependency injection with the URI object.
				* @var PhutilURI
				*/
				private $uriObj;

				/**
				* The original URI, untouched.
				* @var string
				*/
				private $uriOriginalStr;

				/**
				* Base URI of Phorge itself, as handy object.
				* The base URI is supposed to never change across requests. So, static.
				* Possible values:
				* - null: Startup. Empty cache.
				* - PhutilURI: Cached value.
				* - false: Cached no value.
				* @var PhutilURI\|null\|false
				*/
				private static $siteBaseUriObj;

				/**
				* Constructor by string.
				* @param string $uri Partial or complete URI.
				*/
				public function __construct($uri) {
				$this->uriObj = new PhutilURI($uri);
				$this->uriOriginalStr = $uri;
				}

				/**
				* Get the original URI as string.
				* @return string
				*/
				public function getOriginalURI() {
				return $this->uriOriginalStr;
				}

				/**
				* Get the traditional URI object.
				* @return PhutilURI
				*/
				public function getURI() {
				return $this->uriObj;
				}

				/**
				* Check whenever an URI is just a simple HTML anchor.
				* It MUST return false if the anchor is about another page or website.
				* @return bool
				*/
				public function isAnchor() {
				return $this->isStartingWithChar('#');
				}

				/**
				* Check whenever an URI starts with a slash (no protocol, etc.)
				* @return bool
				*/
				public function isStartingWithSlash() {
				return $this->isStartingWithChar('/');
				}

				/**
				* Check if the URI points to Phorge itself.
				* This is a simple compromise between performance and a sane logic.
				* At the moment we don't read the crystal ball, so these are
				* considered different websites:
				* - http://example.com (the input)
				* - https://example.com (the base-uri)
				* @return bool
				*/
				public function isInternal() {

				valerio.bozzolanAuthorUnsubmitted Done Inline Actions Maybe rename from `isInternal` to `isSelfURI` and just return `PhabricatorEnv::isSelfURI($uri_string)` valerio.bozzolan: Maybe rename from `isInternal` to `isSelfURI` and just return `PhabricatorEnv::isSelfURI…
				// If this URL has not a domain, indeed its the current domain.
				// Probably this is a simple anchor, or just '/something/' etc.
				$uri = $this->uriObj;
				if ($uri->getProtocol() === '' && $uri->getDomain() === '') {
				return true;
				}

				// Assume a safe default in case of missing base URI.
				$base = self::baseURIObj();
				if (!$base) {
				return false;
				}

				return $uri->getDomain() === $base->getDomain()
				&& $uri->getProtocol() === $base->getProtocol()
				&& $uri->getPort() === $base->getPort();
				}

				/**
				* Get a fresh URI object, cached.
				* This is safe to be called a lot of times.
				* @return PhutilURI\|null
				*/
				public static function baseURIObj() {
				if (self::$siteBaseUriObj === null) {
				self::$siteBaseUriObj = self::baseURIObjUncached();
				}
				return self::$siteBaseUriObj;
				}

				/**
				* Get a fresh URI object (if any), uncached.
				* @return PhutilURI\|false
				*/
				private static function baseURIObjUncached() {
				$base = PhabricatorEnv::getEnvConfigIfExists('phabricator.base-uri');
				if ($base) {
				return new PhutilURI($base);
				}
				return false;
				}

				/**
				* Check whenever the URI starts with the provided character.
				* @param string $char String that MUST be of length = 1.
				* @return boolean
				*/
				private function isStartingWithChar($char) {
				return strncmp($this->uriOriginalStr, $char, 1) === 0;
				}

				}

Fix PHP 8.1 "strlen(null)" exception on LDAP login without passwordClosedPublicActions