From 050e269921ebf093f6f8485dbc386b860e90a969 Mon Sep 17 00:00:00 2001 From: Stephen Abello Date: Tue, 7 Apr 2026 11:53:43 +0200 Subject: [PATCH] =?UTF-8?q?N=C2=B09448=20-=20Fix=20external=20auth=20varia?= =?UTF-8?q?ble=20value?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/loginexternal.class.inc.php | 7 +- core/config.class.inc.php | 75 ++++++++++++++- .../application/LoginExternalTest.php | 95 +++++++++++++++++++ 3 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 tests/php-unit-tests/unitary-tests/application/LoginExternalTest.php diff --git a/application/loginexternal.class.inc.php b/application/loginexternal.class.inc.php index d07869595d..23e0c78501 100644 --- a/application/loginexternal.class.inc.php +++ b/application/loginexternal.class.inc.php @@ -75,13 +75,10 @@ class LoginExternal extends AbstractLoginFSMExtension } /** - * @return bool + * @return bool|mixed */ private function GetAuthUser() { - $sExtAuthVar = MetaModel::GetConfig()->GetExternalAuthenticationVariable(); // In which variable is the info passed ? - eval('$sAuthUser = isset('.$sExtAuthVar.') ? '.$sExtAuthVar.' : false;'); // Retrieve the value - /** @var string $sAuthUser */ - return $sAuthUser; // Retrieve the value + return MetaModel::GetConfig()->GetExternalAuthenticationVariable(); } } diff --git a/core/config.class.inc.php b/core/config.class.inc.php index ec8d44b1af..dcc40cfaae 100644 --- a/core/config.class.inc.php +++ b/core/config.class.inc.php @@ -75,6 +75,7 @@ define('DEFAULT_EXT_AUTH_VARIABLE', '$_SERVER[\'REMOTE_USER\']'); define('DEFAULT_ENCRYPTION_KEY', '@iT0pEncr1pti0n!'); // We'll use a random generated key later (if possible) define('DEFAULT_ENCRYPTION_LIB', 'Mcrypt'); // We'll define the best encryption available later define('DEFAULT_HASH_ALGO', PASSWORD_DEFAULT); + /** * Config * configuration data (this class cannot not be localized, because it is responsible for loading the dictionaries) @@ -869,6 +870,14 @@ class Config 'source_of_value' => '', 'show_in_conf_sample' => false, ], + 'ext_auth_variable' => [ + 'type' => 'string', + 'description' => 'External authentication expression (allowed: $_SERVER[\'key\'], $_COOKIE[\'key\'], $_REQUEST[\'key\'], getallheaders()[\'Header-Name\'])', + 'default' => '', + 'value' => '', + 'source_of_value' => '', + 'show_in_conf_sample' => false, + ], 'login_debug' => [ 'type' => 'bool', 'description' => 'Activate the login FSM debug', @@ -2350,9 +2359,73 @@ class Config return explode('|', $this->m_sAllowedLoginTypes); } + /** + * @return bool|mixed + * @since 3.2.3 return the parsed value instead of an unsecured variable name + */ public function GetExternalAuthenticationVariable() { - return $this->m_sExtAuthVariable; + $sExpression = $this->Get('ext_auth_variable'); + $aParsed = $this->ParseExternalAuthVariableExpression($sExpression); + if ($aParsed === null) { + return false; + } + + $sKey = $aParsed['key']; + switch ($aParsed['type']) { + case 'server': + return $_SERVER[$sKey] ?? false; + case 'cookie': + return $_COOKIE[$sKey] ?? false; + case 'request': + return $_REQUEST[$sKey] ?? false; + case 'header': + if (!function_exists('getallheaders')) { + return false; + } + $aHeaders = getallheaders(); + if (!is_array($aHeaders)) { + return false; + } + return $aHeaders[$sKey] ?? false; + } + return false; + } + + /** + * @param $sExpression + * @return array|null + */ + private function ParseExternalAuthVariableExpression($sExpression) + { + // If it's a configuration parameter it's probably already trimmed, but just in case + $sExpression = trim((string) $sExpression); + if ($sExpression === '') { + return null; + } + + // Match $_SERVER/$_COOKIE/$_REQUEST['key'] with optional whitespace and single/double quotes. + if (preg_match('/^\$_(SERVER|COOKIE|REQUEST)\s*\[\s*(["\'])\s*([^"\']+)\2\s*\]\s*$/', $sExpression, $aMatches) === 1) { + $sContext = strtoupper($aMatches[1]); + $sKey = $aMatches[3]; + return [ + 'type' => strtolower($sContext), + 'key' => $sKey, + 'normalized' => '$_'.$sContext.'[\''.$sKey.'\']', + ]; + } + + // Match getallheaders()['Header-Name'] in a case-insensitive way. + if (preg_match('/^getallheaders\(\)\s*\[\s*(["\'])\s*([^"\']+)\1\s*\]\s*$/i', $sExpression, $aMatches) === 1) { + $sKey = $aMatches[2]; + return [ + 'type' => 'header', + 'key' => $sKey, + 'normalized' => 'getallheaders()[\''.$sKey.'\']', + ]; + } + + return null; } public function GetCSVImportCharsets() diff --git a/tests/php-unit-tests/unitary-tests/application/LoginExternalTest.php b/tests/php-unit-tests/unitary-tests/application/LoginExternalTest.php new file mode 100644 index 0000000000..41e6ce6a89 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/application/LoginExternalTest.php @@ -0,0 +1,95 @@ + + */ + +namespace Combodo\iTop\Test\UnitTest\Application; + +use Combodo\iTop\Test\UnitTest\ItopDataTestCase; +use utils; + +class LoginExternalTest extends ItopDataTestCase +{ + private $oConfig; + private $sOriginalExtAuthVariable; + + protected function setUp(): void + { + parent::setUp(); + require_once APPROOT.'application/loginexternal.class.inc.php'; + $this->oConfig = utils::GetConfig(); + $this->sOriginalExtAuthVariable = $this->oConfig->Get('ext_auth_variable'); + } + + protected function tearDown(): void + { + $this->oConfig->Set('ext_auth_variable', $this->sOriginalExtAuthVariable, 'unit_test'); + parent::tearDown(); + } + + private function CallGetAuthUser() + { + $oLoginExternal = new \LoginExternal(); + $oMethod = new \ReflectionMethod(\LoginExternal::class, 'GetAuthUser'); + $oMethod->setAccessible(true); + return $oMethod->invoke($oLoginExternal); + } + + public function testGetAuthUserFromServerVariable() + { + $_SERVER['REMOTE_USER'] = 'alice'; + $this->oConfig->Set('ext_auth_variable', '$_SERVER[\'REMOTE_USER\']', 'unit_test'); + + $this->assertSame('alice', $this->CallGetAuthUser()); + } + + public function testGetAuthUserFromCookie() + { + $_COOKIE['auth_user'] = 'bob'; + $this->oConfig->Set('ext_auth_variable', '$_COOKIE[\'auth_user\']', 'unit_test'); + + $this->assertSame('bob', $this->CallGetAuthUser()); + } + + public function testGetAuthUserFromRequest() + { + $_REQUEST['auth_user'] = 'carol'; + $this->oConfig->Set('ext_auth_variable', '$_REQUEST[\'auth_user\']', 'unit_test'); + + $this->assertSame('carol', $this->CallGetAuthUser()); + } + + public function testInvalidExpressionReturnsFalse() + { + $this->oConfig->Set('ext_auth_variable', '$_SERVER[\'HTTP_X_CMD\']) ? print(\'x\') : false; //', 'unit_test'); + + $this->assertFalse($this->CallGetAuthUser()); + } + + public function testGetAuthUserFromHeaderWithoutAllowlist() + { + if (!function_exists('getallheaders')) { + $this->markTestSkipped('getallheaders() not available'); + } + $_SERVER['HTTP_X_REMOTE_USER'] = 'CN=header-test'; + $this->oConfig->Set('ext_auth_variable', 'getallheaders()[\'X-Remote-User\']', 'unit_test'); + + $this->assertSame('CN=header-test', $this->CallGetAuthUser()); + } +}