N°9448 - Fix external auth variable value

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();
 	}
 }

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()

js/forms-json-utils.js

@@ -301,112 +301,88 @@ function ValidateField(sFieldId, sPattern, bMandatory, sFormId, nullValue, origi
 	return true; // Do not stop propagation ??
 }
 
-function EvaluateCKEditorValidation(oOptions)
+function ValidateCKEditField(sFieldId, sPattern, bMandatory, sFormId, nullValue, originalValue)
 {
-	let oField = $('#'+oOptions.sFieldId);
+	let oField = $('#'+sFieldId);
 	if (oField.length === 0) {
 		return false;
 	}
 
-	let oCKEditor = CombodoCKEditorHandler.GetInstanceSynchronous('#'+oOptions.sFieldId);
-	let bValid = true;
-	let sExplain = '';
-	let sTextContent;
-	let sTextOriginalContents;
+	let oCKEditor = CombodoCKEditorHandler.GetInstanceSynchronous('#'+sFieldId);
 
+	var bValid;
+	var sExplain = '';
 	if (oField.prop('disabled')) {
 		bValid = true; // disabled fields are not checked
 	} else {
 		// If the CKEditor is not yet loaded, we need to wait for it to be ready
 		// but as we need this function to be synchronous, we need to call it again when the CKEditor is ready
 		if (oCKEditor === undefined){
-			CombodoCKEditorHandler.GetInstance('#'+oOptions.sFieldId).then((oCKEditor) => {
-				oOptions.onRetry();
+			CombodoCKEditorHandler.GetInstance('#'+sFieldId).then((oCKEditor) => {
+				ValidateCKEditField(sFieldId, sPattern, bMandatory, sFormId, nullValue, originalValue);
 			});
-			return false;
+			return;
 		}
-
+		let sTextContent;
 		let sFormattedContent = oCKEditor.getData();
+
+		// Get the contents without the tags
+		// Check if we have a formatted content that is HTML, otherwise we just have plain text, and we can use it directly
 		sTextContent = $(sFormattedContent).length > 0 ? $(sFormattedContent).text() : sFormattedContent;
 
-		if (sTextContent === '')
-		{
+		if (sTextContent === '') {
 			// No plain text, maybe there is just an image
-			let oImg = $(sFormattedContent).find('img');
-			if (oImg.length !== 0)
-			{
+			let oImg = $(sFormattedContent).find("img");
+			if (oImg.length !== 0) {
 				sTextContent = 'image';
 			}
 		}
 
-		let oFormattedOriginalContents = (oOptions.sOriginalValue !== undefined) ? $('<div></div>').html(oOptions.sOriginalValue) : undefined;
-		sTextOriginalContents = (oFormattedOriginalContents !== undefined) ? oFormattedOriginalContents.text() : undefined;
+		// Get the original value without the tags
+		let oFormattedOriginalContents = (originalValue !== undefined) ? $('<div></div>').html(originalValue) : undefined;
+		let sTextOriginalContents = (oFormattedOriginalContents !== undefined) ? oFormattedOriginalContents.text() : undefined;
 
-		if (oOptions.validate !== undefined) {
-			let oValidation = oOptions.validate(sTextContent, sTextOriginalContents);
-			bValid = oValidation.bValid;
-			sExplain = oValidation.sExplain;
+		if (bMandatory && (sTextContent === nullValue)) {
+			bValid = false;
+			sExplain = Dict.S('UI:ValueMustBeSet');
+		} else if ((sTextOriginalContents !== undefined) && (sTextContent === sTextOriginalContents)) {
+			bValid = false;
+			if (sTextOriginalContents === nullValue) {
+				sExplain = Dict.S('UI:ValueMustBeSet');
+			} else {
+				// Note: value change check is not working well yet as the HTML to Text conversion is not exactly the same when done from the PHP value or the CKEditor value.
+				sExplain = Dict.S('UI:ValueMustBeChanged');
+			}
+		} else {
+			bValid = true;
 		}
-
-		// Put an event to check the field when the content changes, remove the event right after as we'll call this same function again, and we don't want to call the event more than once (especially not ^2 times on each call)
+		
+		// Put and event to check the field when the content changes, remove the event right after as we'll call this same function again, and we don't want to call the event more than once (especially not ^2 times on each call)
 		oCKEditor.model.document.once('change:data', (event) => {
-			oOptions.onChange();
+			ValidateCKEditField(sFieldId, sPattern, bMandatory, sFormId, nullValue, originalValue);
 		});
 	}
-
-	ReportFieldValidationStatus(oOptions.sFieldId, oOptions.sFormId, bValid, sExplain);
+	
+	ReportFieldValidationStatus(sFieldId, sFormId, bValid, sExplain);
 	return bValid;
 }
 
-function ValidateCKEditField(sFieldId, sPattern, bMandatory, sFormId, nullValue, originalValue)
-{
-	return EvaluateCKEditorValidation({
-		sFieldId: sFieldId,
-		sFormId: sFormId,
-        sOriginalValue: originalValue,
-		onRetry: function() {
-			ValidateCKEditField(sFieldId, sPattern, bMandatory, sFormId, nullValue, originalValue);
-		},
-		onChange: function() {
-			ValidateCKEditField(sFieldId, sPattern, bMandatory, sFormId, nullValue, originalValue);
-		},
-		validate: function(sTextContent, sTextOriginalContents) {
-			var bValid;
-			var sExplain = '';
-			if (bMandatory && (sTextContent === nullValue)) {
-				bValid = false;
-				sExplain = Dict.S('UI:ValueMustBeSet');
-			} else if ((sTextOriginalContents !== undefined) && (sTextContent === sTextOriginalContents)) {
-				bValid = false;
-				if (sTextOriginalContents === nullValue) {
-					sExplain = Dict.S('UI:ValueMustBeSet');
-				} else {
-					// Note: value change check is not working well yet as the HTML to Text conversion is not exactly the same when done from the PHP value or the CKEditor value.
-					sExplain = Dict.S('UI:ValueMustBeChanged');
-				}
-			} else {
-				bValid = true;
-			}
-			return {bValid: bValid, sExplain: sExplain};
-		}
-	});
-}
 function ResetPwd(id)
 {
-    // Reset the values of the password fields
-    $('#'+id).val('*****');
-    $('#'+id+'_confirm').val('*****');
-    // And reset the flag, to tell it that the password remains unchanged
-    $('#'+id+'_changed').val(0);
-    // Visual feedback, None when it's Ok
-    $('#v_'+id).html('');
+	// Reset the values of the password fields
+	$('#'+id).val('*****');
+	$('#'+id+'_confirm').val('*****');
+	// And reset the flag, to tell it that the password remains unchanged
+	$('#'+id+'_changed').val(0);
+	// Visual feedback, None when it's Ok
+	$('#v_'+id).html('');
 }
 
 // Called whenever the content of a one way encrypted password changes
 function PasswordFieldChanged(id)
 {
-    // Set the flag, to tell that the password changed
-    $('#'+id+'_changed').val(1);
+	// Set the flag, to tell that the password changed
+	$('#'+id+'_changed').val(1);
 }
 
 // Special validation function for one way encrypted password fields
@@ -439,48 +415,37 @@ function ValidatePasswordField(id, sFormId)
 // to determine if the field is empty or not
 function ValidateCaseLogField(sFieldId, bMandatory, sFormId, nullValue, originalValue)
 {
-	return EvaluateCKEditorValidation({
-		sFieldId: sFieldId,
-		sFormId: sFormId,
-		sOriginalValue: originalValue,
-		onRetry: function() {
-			ValidateCaseLogField(sFieldId, bMandatory, sFormId, nullValue, originalValue);
-		},
-		onChange: function() {
-			ValidateCaseLogField(sFieldId, bMandatory, sFormId, nullValue, originalValue);
-		},
-		validate: function(sTextContent, sTextOriginalContents) {
-			var bValid;
-			var sExplain = '';
-			// CaseLog is special: history count matters when deciding if the field is empty
-			var count = $('#'+sFieldId+'_count').val();
+	var bValid = true;
+	var sExplain = '';
+	var sTextContent;
+	
+	if ($('#'+sFieldId).prop('disabled'))
+	{
+		bValid = true; // disabled fields are not checked
+	}
+	else
+	{
+		// Get the contents (with tags)
+		// Note: For CaseLog we can't retrieve the formatted contents from CKEditor (unlike in ValidateCKEditorField() method) because of the place holder.
+		sTextContent = $('#' + sFieldId).val();
+		var count = $('#'+sFieldId+'_count').val();
 
-			if (bMandatory && (count == 0) && (sTextContent === nullValue))
-			{
-				// No previous entry and no content typed
-				bValid = false;
-				sExplain = Dict.S('UI:ValueMustBeSet');
-			}
-			else if ((sTextOriginalContents !== undefined) && (sTextContent === sTextOriginalContents))
-			{
-				bValid = false;
-				if (sTextOriginalContents === nullValue)
-				{
-					sExplain = Dict.S('UI:ValueMustBeSet');
-				}
-				else
-				{
-					// Note: value change check is not working well yet as the HTML to Text conversion is not exactly the same when done from the PHP value or the CKEditor value.
-					sExplain = Dict.S('UI:ValueMustBeChanged');
-				}
-			}
-			else
-			{
-				bValid = true;
-			}
-			return {bValid: bValid, sExplain: sExplain};
+		if (bMandatory && (count == 0) && (sTextContent == nullValue))
+		{
+			// No previous entry and no content typed
+			bValid = false;
+			sExplain = Dict.S('UI:ValueMustBeSet');
 		}
-	});
+		else if ((originalValue != undefined) && (sTextContent == originalValue))
+		{
+			bValid = false;
+			sExplain = Dict.S('UI:ValueMustBeChanged');
+		}
+	}
+	ReportFieldValidationStatus(sFieldId, sFormId, bValid, '' /* sExplain */);
+
+	// We need to check periodically as CKEditor doesn't trigger our events. More details in UIHTMLEditorWidget::Display() @ line 92
+	setTimeout(function(){ValidateCaseLogField(sFieldId, bMandatory, sFormId, nullValue, originalValue);}, 500);
 }
 
 // Validate the inputs depending on the current setting

tests/php-unit-tests/unitary-tests/application/LoginExternalTest.php

@@ -0,0 +1,95 @@
+<?php
+
+/**
+ * Copyright (C) 2010-2024 Combodo SAS
+ *
+ * This file is part of iTop.
+ *
+ * iTop is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * iTop is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with iTop. If not, see <http://www.gnu.org/licenses/>
+ */
+
+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());
+	}
+}