diff --git a/application/applicationextension.inc.php b/application/applicationextension.inc.php index c86d3cf63..da2c0da94 100644 --- a/application/applicationextension.inc.php +++ b/application/applicationextension.inc.php @@ -1715,6 +1715,11 @@ interface iRestServiceProvider public function ExecOperation($sVersion, $sVerb, $aParams); } +interface iRestInputSanitizer +{ + public function SanitizeJsonInput(string $sJsonInput): string; +} + /** * Minimal REST response structure. Derive this structure to add response data and error codes. * @@ -1806,6 +1811,14 @@ class RestResult * @api */ public $message; + + /** + * Sanitize the content of this result to hide sensitive information + */ + public function SanitizeContent() + { + // The default implementation does nothing + } } /** diff --git a/core/attributedef.class.inc.php b/core/attributedef.class.inc.php index 7134467b7..a3188b016 100644 --- a/core/attributedef.class.inc.php +++ b/core/attributedef.class.inc.php @@ -136,7 +136,7 @@ abstract class AttributeDefinition protected $aCSSClasses; - public function GetType() + public function GetType() { return Dict::S('Core:'.get_class($this)); } @@ -4164,7 +4164,7 @@ class AttributeFinalClass extends AttributeString */ class AttributePassword extends AttributeString implements iAttributeNoGroupBy { - const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; + const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; /** * Useless constructor, but if not present PHP 7.4.0/7.4.1 is crashing :( (N°2329) @@ -4241,7 +4241,7 @@ class AttributePassword extends AttributeString implements iAttributeNoGroupBy */ class AttributeEncryptedString extends AttributeString implements iAttributeNoGroupBy { - const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; + const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; static $sKey = null; // Encryption key used for all encrypted fields static $sLibrary = null; // Encryption library used for all encrypted fields @@ -9973,7 +9973,7 @@ class AttributeSubItem extends AttributeDefinition */ class AttributeOneWayPassword extends AttributeDefinition implements iAttributeNoGroupBy { - const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; + const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; /** * Useless constructor, but if not present PHP 7.4.0/7.4.1 is crashing :( (N°2329) diff --git a/core/restservices.class.inc.php b/core/restservices.class.inc.php index e78224a28..478e0bc44 100644 --- a/core/restservices.class.inc.php +++ b/core/restservices.class.inc.php @@ -156,6 +156,25 @@ class ObjectResult { $this->fields[$sAttCode] = $this->MakeResultValue($oObject, $sAttCode, $bExtendedOutput); } + +public function SanitizeContent() + { + foreach($this->fields as $sAttCode => $value) + { + try{ + $oAttDef = MetaModel::GetAttributeDef($this->class, $sAttCode); + } catch (Exception $e) { // for special cases like ID + continue; + } + if ($oAttDef instanceof iAttributeNoGroupBy) // iAttributeNoGroupBy is equivalent to sensitive attribute + { + $this->fields[$sAttCode] = '******'; + } + { + $this->fields[$sAttCode] = '******'; + } + } + } } @@ -221,6 +240,16 @@ class RestResultWithObjects extends RestResult $sObjKey = get_class($oObject).'::'.$oObject->GetKey(); $this->objects[$sObjKey] = $oObjRes; } + +public function SanitizeContent() + { + parent::SanitizeContent(); + + foreach($this->objects as $sObjKey => $oObjRes) + { + $oObjRes->SanitizeContent(); + } + } } /** @@ -308,7 +337,7 @@ class RestDelete * * @package Core */ -class CoreServices implements iRestServiceProvider +class CoreServices implements iRestServiceProvider, iRestInputSanitizer { /** * Enumerate services delivered by this class @@ -736,6 +765,34 @@ class CoreServices implements iRestServiceProvider } return $oResult; } + + public function SanitizeJsonInput(string $sJsonInput): string + { + $sSanitizedJsonInput = $sJsonInput; + $aJsonData = json_decode($sSanitizedJsonInput, true); + $sOperation = $aJsonData['operation']; + + switch ($sOperation) { + case 'core/check_credentials': + if (isset($aJsonData['password'])) { + $aJsonData['password'] = '*****'; + } + break; + case 'core/update': + case 'core/create': + default : + $sClass = $aJsonData['class']; + foreach ($aJsonData['fields'] as $sAttCode => $value) { + $oAttDef = MetaModel::GetAttributeDef($sClass, $sAttCode); + if ($oAttDef instanceof iAttributeNoGroupBy) // iAttributeNoGroupBy is equivalent to sensitive attribute + { + $aJsonData['fields'][$sAttCode] = '*****'; + } + } + break; + } + return json_encode($aJsonData, JSON_UNESCAPED_SLASHES | JSON_PRETTY_PRINT); + } /** * Helper for object deletion diff --git a/tests/php-unit-tests/unitary-tests/core/Delta/delta_test_sanitize_output.xml b/tests/php-unit-tests/unitary-tests/core/Delta/delta_test_sanitize_output.xml new file mode 100644 index 000000000..2465f3548 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/core/Delta/delta_test_sanitize_output.xml @@ -0,0 +1,12 @@ + + + + + + + encrypted_string + + + + + \ No newline at end of file diff --git a/tests/php-unit-tests/unitary-tests/core/RestServicesSanitizeOutputTest.php b/tests/php-unit-tests/unitary-tests/core/RestServicesSanitizeOutputTest.php new file mode 100644 index 000000000..b819e2699 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/core/RestServicesSanitizeOutputTest.php @@ -0,0 +1,54 @@ + +// + +namespace Combodo\iTop\Test\UnitTest\Core; + +use Combodo\iTop\Test\UnitTest\ItopCustomDatamodelTestCase; +use Group; + +/** + * @runTestsInSeparateProcesses + * @preserveGlobalState disabled + * @backupGlobals disabled + */ +class RestServicesSanitizeOutputTest extends iTopCustomDatamodelTestCase +{ + public function setUp(): void + { + parent::setUp(); + } + + + public function testSanitizeJsonOutput() + { + $oGroup = new Group(); + $oGroup->Set('encrypted_string', "123456"); + $oRestResultWithObject = new \RestResultWithObjects(); + $oRestResultWithObject->AddObject(0, "ok", $oGroup, ['Group' => ['encrypted_string']]); + $oRestResultWithObject->SanitizeContent(); + $this->assertEquals('{"objects":{"Group::-1":{"code":0,"message":"ok","class":"Group","key":-1,"fields":{"encrypted_string":"******"}}},"code":0,"message":null}', json_encode($oRestResultWithObject)); + } + + + + public function GetDatamodelDeltaAbsPath(): string + { + return __DIR__ . "/Delta/delta_test_sanitize_output.xml"; + } +} diff --git a/tests/php-unit-tests/unitary-tests/core/RestServicesTest.php b/tests/php-unit-tests/unitary-tests/core/RestServicesTest.php new file mode 100644 index 000000000..0e013338a --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/core/RestServicesTest.php @@ -0,0 +1,140 @@ + +// + +namespace Combodo\iTop\Test\UnitTest\Core; + +use Combodo\iTop\Test\UnitTest\ItopDataTestCase; +use CoreException; +use CoreServices; +use CoreUnexpectedValue; +use SimpleGraphException; +use UserLocal; + +/** + * @runTestsInSeparateProcesses + * @preserveGlobalState disabled + * @backupGlobals disabled + */ +class RestServicesTest extends ItopDataTestCase +{ + public function setUp(): void + { + parent::setUp(); + } + + /** + * @return void + * @dataProvider providerTestSanitizeJsonInput + */ + public function testSanitizeJsonInput($sJsonData, $sExpectedJsonDataSanitized) + { + $oRS = new CoreServices(); + $sOutputJson = $oRS->SanitizeJsonInput($sJsonData); + $this->assertEquals($sExpectedJsonDataSanitized, $sOutputJson); + } + + public function providerTestSanitizeJsonInput() + { + return [ + 'core/check_credentials' => [ + '{"operation": "core/check_credentials", "user": "admin", "password": "admin"}', + '{ + "operation": "core/check_credentials", + "user": "admin", + "password": "*****" +}' + ], + 'core/update' => [ + '{"operation": "core/update", "comment": "Update user", "class": "UserLocal", "key": {"id":1}, "output_fields": "first_name, password", "fields": {"password" : "123456"}}', + '{ + "operation": "core/update", + "comment": "Update user", + "class": "UserLocal", + "key": { + "id": 1 + }, + "output_fields": "first_name, password", + "fields": { + "password": "*****" + } +}' + ], + 'core/create' => [ + '{"operation": "core/create", "comment": "Create user", "class": "UserLocal", "fields": {"first_name": "John", "last_name": "Doe", "email": "jd@example/com", "password" : "123456"}}', + '{ + "operation": "core/create", + "comment": "Create user", + "class": "UserLocal", + "fields": { + "first_name": "John", + "last_name": "Doe", + "email": "jd@example/com", + "password": "*****" + } +}' + ], + ]; + } + + /** + * @param $sOperation + * @param $aJsonData + * @param $sExpectedJsonDataSanitized + * @return void + * @throws CoreException + * @throws CoreUnexpectedValue + * @throws SimpleGraphException + * @dataProvider providerTestSanitizeJsonOutput + */ + public function testSanitizeJsonOutput($sOperation, $aJsonData, $sExpectedJsonDataSanitized) + { + $oUser = new UserLocal(); + $oUser->Set('password', "123456"); + $oRestResultWithObject = new \RestResultWithObjects(); + $oRestResultWithObject->AddObject(0, "ok", $oUser, ['UserLocal' => ['login', 'password']]); + $oRestResultWithObject->SanitizeContent(); + $this->assertEquals($sExpectedJsonDataSanitized, json_encode($oRestResultWithObject)); + } + + public function providerTestSanitizeJsonOutput() + { + return [ + + 'core/update' => [ + 'core/update', + ['comment' => 'Update user', 'class' => 'UserLocal', 'key' => ['login' => 'my_example'], 'output_fields' => 'password', 'fields' => ['password' => 'opkB!req57']], + '{"objects":{"UserLocal::-1":{"code":0,"message":"ok","class":"UserLocal","key":-1,"fields":{"login":"******","password":"******"}}},"code":0,"message":null}' + ], + 'core/create' => [ + 'core/create', + ['comment' => 'Create user', 'class' => 'UserLocal', 'fields' => ['password' => 'Azertyuiiop*12', 'login' => 'toto', 'profile_list' => [1]]], + '{"objects":{"UserLocal::-1":{"code":0,"message":"ok","class":"UserLocal","key":-1,"fields":{"login":"******","password":"******"}}},"code":0,"message":null}' + ], + 'core/get' => [ + 'core/get', + ['comment' => 'Get user', 'class' => 'UserLocal', 'key' => ['login' => 'my_example'], 'output_fields' => 'first_name, password'], + '{"objects":{"UserLocal::-1":{"code":0,"message":"ok","class":"UserLocal","key":-1,"fields":{"login":"******","password":"******"}}},"code":0,"message":null}' + ], + 'core/check_credentials' => [ + 'core/check_credentials', + ['user' => 'admin', 'password' => 'admin'], + '{"objects":{"UserLocal::-1":{"code":0,"message":"ok","class":"UserLocal","key":-1,"fields":{"login":"******","password":"******"}}},"code":0,"message":null}' ], + ]; + } +} diff --git a/webservices/rest.php b/webservices/rest.php index 14015dc0b..280e35707 100644 --- a/webservices/rest.php +++ b/webservices/rest.php @@ -224,6 +224,13 @@ try /** @var iRestServiceProvider $oRS */ $oRS = $aOpToRestService[$sOperation]['service_provider']; $sProvider = get_class($oRS); + + if ($oRS instanceof iRestInputSanitizer) { + $sSanitizedJsonInput = $oRS->SanitizeJsonInput($sJsonString); + } + else { + $sSanitizedJsonInput = $sJsonString; + } CMDBObject::SetTrackOrigin('webservice-rest'); $oResult = $oRS->ExecOperation($sVersion, $sOperation, $aJsonData); @@ -249,6 +256,7 @@ catch(Exception $e) // $sResponse = json_encode($oResult); + if ($sResponse === false) { $oJsonIssue = new RestResult(); @@ -280,7 +288,7 @@ if (MetaModel::GetConfig()->Get('log_rest_service')) $oLog->SetTrim('userinfo', UserRights::GetUser()); $oLog->Set('version', $sVersion); $oLog->Set('operation', $sOperation); - $oLog->SetTrim('json_input', $sJsonString); + $oLog->SetTrim('json_input', $sSanitizedJsonInput); $oLog->Set('provider', $sProvider); $sMessage = $oResult->message; @@ -290,7 +298,8 @@ if (MetaModel::GetConfig()->Get('log_rest_service')) } $oLog->SetTrim('message', $sMessage); $oLog->Set('code', $oResult->code); - $oLog->SetTrim('json_output', $sResponse); + $oResult->SanitizeContent(); + $oLog->SetTrim('json_output', json_encode($oResult, JSON_UNESCAPED_SLASHES | JSON_PRETTY_PRINT)); $oLog->DBInsertNoReload(); } \ No newline at end of file