N°9392 - wizard helper linkset data validation improvement

- greptile feedbacks

application/wizardhelper.class.inc.php

@@ -35,10 +35,14 @@ class WizardHelper
 	public function __construct()
 	{
 	}
+
 	/**
 	 * Constructs the PHP target object from the parameters sent to the web page by the wizard
+	 *
 	 * @param boolean $bReadUploadedFiles True to also read any uploaded file (for blob/document fields)
+	 *
 	 * @return object
+	 * @throws CoreException
 	 */
 	public function GetTargetObject($bReadUploadedFiles = false)
 	{
@@ -57,7 +61,14 @@ class WizardHelper
 					// assumes this is handled as an array of objects
 					// thus encoded in json like: [ { name:'link1', 'id': 123}, { name:'link2', 'id': 124}...]
 					if (!is_array($value)) {
-						$aData = json_decode($value, true); // true means decode as a hash array (not an object)
+						try{
+							$aData = json_decode($value, associative: true,  flags: JSON_THROW_ON_ERROR); // true means decode as a hash array (not an object)
+						} catch (JsonException $e) {
+							throw new CoreException('WizardHelper GetTargetObject: invalid data for linked set attribute '.$sAttCode.' of class '.$this->m_aData['m_sClass'], [
+								'att_code' => $sAttCode,
+								'linked_class' => $this->m_aData['m_sClass'],
+							], 'Invalid JSON data', $e);
+						}
 					} else {
 						$aData = $value;
 					}
@@ -66,9 +77,6 @@ class WizardHelper
 					$aFields = $this->GetLinkedWizardStructure($oAttDef);
 					$sLinkedClass = $oAttDef->GetLinkedClass();
 					$aLinkedObjectsArray = [];
-					if (!is_array($aData)) {
-						echo("aData: '$aData' (value: '$value')\n");
-					}
 					foreach ($aData as $aLinkedObject) {
 						$oLinkedObj = MetaModel::NewObject($sLinkedClass);
 						foreach ($aFields as $sLinkedAttCode) {

sources/Controller/Links/LinkSetController.php

@@ -17,11 +17,9 @@ use Combodo\iTop\Service\Base\ObjectRepository;
 use Combodo\iTop\Service\Router\Router;
 use CoreException;
 use DBObject;
-use DBObjectSet;
 use Dict;
 use Exception;
 use MetaModel;
-use SecurityException;
 use UserRights;
 use utils;
 
@@ -60,11 +58,7 @@ class LinkSetController extends AbstractController
 		// check transaction id
 		if (utils::IsTransactionValid($sTransactionId, false)) {
 			try {
-				$oLinkedObject = MetaModel::GetObject($sLinkedObjectClass, $sLinkedObjectObjectKey);
-				if (UserRights::IsActionAllowed(get_class($oLinkedObject), UR_ACTION_DELETE, DBObjectSet::FromObject($oLinkedObject)) !== UR_ALLOWED_YES) {
-					throw new SecurityException('You are not allowed to delete this object.');
-				}
-				$oDeletionPlan = $oLinkedObject->DBDelete();
+				$oDeletionPlan = MetaModel::GetObject($sLinkedObjectClass, $sLinkedObjectObjectKey)->DBDelete();
 				$bOperationSuccess = (count($oDeletionPlan->GetIssues()) === 0);
 				if (!$bOperationSuccess) {
 					$sErrorMessage = json_encode($oDeletionPlan->GetIssues());
@@ -109,9 +103,6 @@ class LinkSetController extends AbstractController
 		if (utils::IsTransactionValid($sTransactionId, false)) {
 			try {
 				$oLinkedObject = MetaModel::GetObject($sLinkedObjectClass, $sLinkedObjectKey);
-				if (UserRights::IsActionAllowed($sLinkedObjectClass, UR_ACTION_MODIFY, DBObjectSet::FromObject($oLinkedObject)) !== UR_ALLOWED_YES) {
-					throw new SecurityException('User not allowed to modify this object');
-				}
 				$oLinkedObject->Set($sExternalKeyAttCode, null);
 				$oLinkedObject->DBWrite();
 				$bOperationSuccess = true;

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

@@ -0,0 +1,45 @@
+<?php
+
+/**
+ * Copyright (C) 2018 Dennis Lassiter
+ *
+ * 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 WizardHelper;
+use CoreException;
+
+/**
+ * @covers WizardHelper
+ */
+class wizardHelperTest extends ItopDataTestCase
+{
+
+	public function testGetTargetObjectWithInvalidLinkSetDataValue()
+	{
+		// test case from N°9392 - [SECU] Stored XSS in subnets_list Parameter
+		$sWizardHelperJSON = '{"m_sClass":"VLAN","m_oFieldsMap":{"vlan_tag":"2_vlan_tag","org_id":"2_org_id","description":"2_description","subnets_list":"2_subnets_list","physicalinterfaces_list":"2_physicalinterfaces_list"},"m_oCurrentValues":{"vlan_tag":null,"org_id":null,"description":null,"subnets_list":"[]oszzt<img src=a onerror=alert(1)>l053a","physicalinterfaces_list":"[]"},"m_aDefaultValueRequested":[],"m_aAllowedValuesRequested":[],"m_oDefaultValue":{},"m_oAllowedValues":{},"m_aStaticValues":[],"m_iFieldsCount":5,"m_sFormPrefix":"","m_sState":"","m_bReturnNotEditableFields":false,"m_sWizHelperJsVarName":null}';
+		$this->expectException(CoreException::class);
+		$this->expectExceptionMessageMatches('/^WizardHelper GetTargetObject: invalid data for linked set attribute/');
+		$oWizardHelper = WizardHelper::FromJSON($sWizardHelperJSON);
+		$oWizardHelper->GetTargetObject();
+	}
+
+}