From 241bd1cdeb3119151e35cac1c09fdee086d5277f Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Mon, 22 Feb 2021 09:42:24 +0100 Subject: [PATCH 01/16] =?UTF-8?q?N=C2=B03430=20-=20code=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - during the code review @dflaven preferred the reference rather than the return alternative - typo --- .../itop-portal-base/portal/src/Form/ObjectFormManager.php | 4 +--- dictionaries/en.dictionary.itop.ui.php | 2 +- sources/form/formmanager.class.inc.php | 6 ++---- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php index 88d434496..d93f3debd 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php @@ -1051,7 +1051,7 @@ class ObjectFormManager extends FormManager /** * @inheritDoc */ - public function CheckTransaction($aData) + public function CheckTransaction(&$aData) { $isTransactionValid = \utils::IsTransactionValid($this->oForm->GetTransactionId(), false); //The transaction token is kept in order to preserve BC with ajax forms (the second call would fail if the token is deleted). (The GC will take care of cleaning the token for us later on) if (!$isTransactionValid) { @@ -1066,8 +1066,6 @@ class ObjectFormManager extends FormManager ]; $aData['valid'] = false; } - - return $aData; } /** diff --git a/dictionaries/en.dictionary.itop.ui.php b/dictionaries/en.dictionary.itop.ui.php index 66c9c78bb..5d4a4215f 100644 --- a/dictionaries/en.dictionary.itop.ui.php +++ b/dictionaries/en.dictionary.itop.ui.php @@ -465,7 +465,7 @@ Dict::Add('EN US', 'English', 'English', array( 'UI:Error:InvalidDashboard' => 'Error: invalid dashboard', 'UI:Error:MaintenanceMode' => 'Application is currently in maintenance', 'UI:Error:MaintenanceTitle' => 'Maintenance', - 'UI:Error:InvalidToken' => 'Error: the requested operation have already been performed (CSRF token not found)', + 'UI:Error:InvalidToken' => 'Error: the requested operation has already been performed (CSRF token not found)', 'UI:GroupBy:Count' => 'Count', 'UI:GroupBy:Count+' => 'Number of elements', diff --git a/sources/form/formmanager.class.inc.php b/sources/form/formmanager.class.inc.php index cb6e15f2d..4d87b6a23 100644 --- a/sources/form/formmanager.class.inc.php +++ b/sources/form/formmanager.class.inc.php @@ -175,7 +175,7 @@ abstract class FormManager ), ); - $aData = $this->CheckTransaction($aData); + $this->CheckTransaction($aData); return $aData; } @@ -187,7 +187,7 @@ abstract class FormManager * * @since 2.7.4 3.0.0 N°3430 */ - public function CheckTransaction($aData) + public function CheckTransaction(&$aData) { $isTransactionValid = \utils::IsTransactionValid($this->oForm->GetTransactionId(), false); //The transaction token is kept in order to preserve BC with ajax forms (the second call would fail if the token is deleted). (The GC will take care of cleaning the token for us later on) if (!$isTransactionValid) { @@ -196,8 +196,6 @@ abstract class FormManager ]; $aData['valid'] = false; } - - return $aData; } /** From 6f40bb4c356322f48bedfce40e0f2988a5cdbd2c Mon Sep 17 00:00:00 2001 From: Molkobain Date: Wed, 24 Feb 2021 09:29:42 +0100 Subject: [PATCH 02/16] Change check level to "warning" in order to keep consistency with the others --- setup/setuputils.class.inc.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/setup/setuputils.class.inc.php b/setup/setuputils.class.inc.php index fc69f16bb..890e3390a 100644 --- a/setup/setuputils.class.inc.php +++ b/setup/setuputils.class.inc.php @@ -562,8 +562,7 @@ class SetupUtils clearstatcache(); if (!is_file($sGraphvizPath) || !is_executable($sGraphvizPath)) { //N°3412 avoid shell injection - return new CheckResult(CheckResult::ERROR, - "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); + return new CheckResult(CheckResult::WARNING, "$sGraphvizPath could not be executed: Please make sure it is installed and in the path"); } if (!utils::IsWindowsEnvironment()){ From 5836be713143e3b0bb0633be8c67d34447e978fa Mon Sep 17 00:00:00 2001 From: Molkobain Date: Wed, 24 Feb 2021 09:49:16 +0100 Subject: [PATCH 03/16] Fix unit test --- test/setup/SetupUtilsTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/setup/SetupUtilsTest.php b/test/setup/SetupUtilsTest.php index 47a996bb7..f2f931ce0 100644 --- a/test/setup/SetupUtilsTest.php +++ b/test/setup/SetupUtilsTest.php @@ -47,7 +47,7 @@ class SetupUtilsTest extends ItopTestCase return [ "bash injection" => [ "touch /tmp/toto", - 0, + 1, "could not be executed: Please make sure it is installed and in the path", ], "command ok" => [ From c601082a5e49a188b9d2bd6a0e991b86e7732fbc Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Wed, 24 Feb 2021 12:05:11 +0100 Subject: [PATCH 04/16] 3548 - disable core update if a file integrity problem is detected --- datamodels/2.x/itop-core-update/en.dict.itop-core-update.php | 1 + datamodels/2.x/itop-core-update/fr.dict.itop-core-update.php | 1 + .../2.x/itop-core-update/src/Controller/AjaxController.php | 3 ++- .../2.x/itop-core-update/view/SelectUpdateFile.html.twig | 2 +- .../2.x/itop-core-update/view/SelectUpdateFile.ready.js.twig | 3 +++ 5 files changed, 8 insertions(+), 2 deletions(-) diff --git a/datamodels/2.x/itop-core-update/en.dict.itop-core-update.php b/datamodels/2.x/itop-core-update/en.dict.itop-core-update.php index 72288f7cf..8d3143cba 100644 --- a/datamodels/2.x/itop-core-update/en.dict.itop-core-update.php +++ b/datamodels/2.x/itop-core-update/en.dict.itop-core-update.php @@ -75,6 +75,7 @@ Dict::Add('EN US', 'English', 'English', array( 'iTopUpdate:UI:CanCoreUpdate:Yes' => 'Application can be updated', 'iTopUpdate:UI:CanCoreUpdate:No' => 'Application cannot be updated: %1$s', 'iTopUpdate:UI:CanCoreUpdate:Warning' => 'Warning: application update can fail: %1$s', + 'iTopUpdate:UI:CannotUpdateUseSetup' => 'You must use the setup to update the application.
Some modified files were detected, a partial update cannot be executed.', // Setup Messages 'iTopUpdate:UI:SetupMessage:Ready' => 'Ready to start', diff --git a/datamodels/2.x/itop-core-update/fr.dict.itop-core-update.php b/datamodels/2.x/itop-core-update/fr.dict.itop-core-update.php index 8dae6b928..a0aa2f8fa 100644 --- a/datamodels/2.x/itop-core-update/fr.dict.itop-core-update.php +++ b/datamodels/2.x/itop-core-update/fr.dict.itop-core-update.php @@ -75,6 +75,7 @@ Dict::Add('FR FR', 'French', 'Français', array( 'iTopUpdate:UI:CanCoreUpdate:Yes' => 'L\'application peut être mise à jour', 'iTopUpdate:UI:CanCoreUpdate:No' => 'L\'application ne peut pas être mise à jour : %1$s', 'iTopUpdate:UI:CanCoreUpdate:Warning' => 'Attention : la mise à jour de l\'application peut échouer : %1$s', + 'iTopUpdate:UI:CannotUpdateUseSetup' => 'Vous devez utiliser la page d\'installation pour mettre à jour l\'application.
Des fichiers modifiés ont été détectés, une mise à jour partielle ne peut pas être effectuée.', // Setup Messages 'iTopUpdate:UI:SetupMessage:Ready' => 'Prêt pour l\\installation', diff --git a/datamodels/2.x/itop-core-update/src/Controller/AjaxController.php b/datamodels/2.x/itop-core-update/src/Controller/AjaxController.php index dc33efa7f..8064ea2b0 100644 --- a/datamodels/2.x/itop-core-update/src/Controller/AjaxController.php +++ b/datamodels/2.x/itop-core-update/src/Controller/AjaxController.php @@ -37,7 +37,8 @@ class AjaxController extends Controller } else { - $aParams['sMessage'] = Dict::Format("iTopUpdate:UI:CanCoreUpdate:{$sCanUpdateCore}", $sMessage); + $sLink = utils::GetAbsoluteUrlAppRoot().'setup/'; + $aParams['sMessage'] = Dict::Format('iTopUpdate:UI:CannotUpdateUseSetup', $sLink); } } catch (FileNotExistException $e) { diff --git a/datamodels/2.x/itop-core-update/view/SelectUpdateFile.html.twig b/datamodels/2.x/itop-core-update/view/SelectUpdateFile.html.twig index 09bd5b4ed..eb6f9fbe7 100644 --- a/datamodels/2.x/itop-core-update/view/SelectUpdateFile.html.twig +++ b/datamodels/2.x/itop-core-update/view/SelectUpdateFile.html.twig @@ -54,7 +54,7 @@ -
+
{{ 'iTopUpdate:UI:SelectUpdateFile'|dict_s }}
diff --git a/datamodels/2.x/itop-core-update/view/SelectUpdateFile.ready.js.twig b/datamodels/2.x/itop-core-update/view/SelectUpdateFile.ready.js.twig index f52ac52cb..8b41ce446 100644 --- a/datamodels/2.x/itop-core-update/view/SelectUpdateFile.ready.js.twig +++ b/datamodels/2.x/itop-core-update/view/SelectUpdateFile.ready.js.twig @@ -22,6 +22,9 @@ $.ajax({ } else { + $("#check-update").prop("disabled", true); + $("#file").prop("disabled", true); + $('#form-update-outer').slideUp(600); oRequirements.addClass("message_error"); } } From 9b7cd20d472c2e6ca09ea855c835f5a0b77bd846 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Wed, 24 Feb 2021 16:46:23 +0100 Subject: [PATCH 05/16] =?UTF-8?q?N=C2=B03473=20-=20security=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/dashlet.class.inc.php | 26 +++++++++++++++----------- application/displayblock.class.inc.php | 17 +++++++++++++++-- core/attributedef.class.inc.php | 10 +++++++--- core/dbsearch.class.php | 24 ++++++++++++++++++++++++ dictionaries/en.dictionary.itop.ui.php | 1 + dictionaries/fr.dictionary.itop.ui.php | 3 ++- 6 files changed, 64 insertions(+), 17 deletions(-) diff --git a/application/dashlet.class.inc.php b/application/dashlet.class.inc.php index f43f232bb..42eeca069 100644 --- a/application/dashlet.class.inc.php +++ b/application/dashlet.class.inc.php @@ -459,17 +459,21 @@ EOF $sAttType = $aTargetAttCodes[$sTargetAttCode]; $sExtFieldAttCode = $sTargetAttCode; } - if (is_a($sAttType, 'AttributeLinkedSet', true)) - { - continue; - } - if (is_a($sAttType, 'AttributeFriendlyName', true)) - { - continue; - } - if (is_a($sAttType, 'AttributeOneWayPassword', true)) - { - continue; + + $aForbidenAttType = [ + 'AttributeLinkedSet', + 'AttributeFriendlyName', + + 'iAttributeNoGroupBy', //we cannot only use iAttributeNoGroupBy since this method is also used by the designer who do not have access to the classes' PHP reflection API. So the known classes has to be listed altogether + 'AttributeOneWayPassword', + 'AttributeEncryptedString', + 'AttributePassword', + ]; + foreach ($aForbidenAttType as $sForbidenAttType) { + if (is_a($sAttType, $sForbidenAttType, true)) + { + continue 2; + } } $sLabel = $this->oModelReflection->GetLabel($sClass, $sAttCode); diff --git a/application/displayblock.class.inc.php b/application/displayblock.class.inc.php index 2114a8dfb..1fa904ff3 100644 --- a/application/displayblock.class.inc.php +++ b/application/displayblock.class.inc.php @@ -446,8 +446,21 @@ class DisplayBlock $this->m_oSet = new CMDBObjectSet($this->m_oFilter, $aOrderBy, $aQueryParams); } $this->m_oSet->SetShowObsoleteData($this->m_bShowObsoleteData); - switch($this->m_sStyle) - { + + switch($this->m_sStyle) { + case 'list_search': + case 'list': + break; + default: + // N°3473: except for 'list_search' and 'list' (which have more granularity, see the other switch below), + // refuse to render if the user is not allowed to see the class. + if (! UserRights::IsActionAllowed($this->m_oSet->GetClass(), UR_ACTION_READ, $this->m_oSet) == UR_ALLOWED_YES) { + $sHtml .= $oPage->GetP(Dict::Format('UI:Error:ReadNotAllowedOn_Class', $this->m_oSet->GetClass())); + return $sHtml; + } + } + + switch ($this->m_sStyle) { case 'count': if (isset($aExtraParams['group_by'])) { diff --git a/core/attributedef.class.inc.php b/core/attributedef.class.inc.php index 879fc4ce0..44b165b63 100644 --- a/core/attributedef.class.inc.php +++ b/core/attributedef.class.inc.php @@ -103,6 +103,10 @@ define('LINKSET_EDITMODE_ACTIONS', 2); // Show the usual 'Actions' popup menu define('LINKSET_EDITMODE_INPLACE', 3); // The "linked" objects can be created/modified/deleted in place define('LINKSET_EDITMODE_ADDREMOVE', 4); // The "linked" objects can be added/removed in place +interface iAttributeNoGroupBy +{ + //no method, just a contract on implement +} /** * Attribute definition API, implemented in and many flavours (Int, String, Enum, etc.) @@ -3761,7 +3765,7 @@ class AttributeFinalClass extends AttributeString * * @package iTopORM */ -class AttributePassword extends AttributeString +class AttributePassword extends AttributeString implements iAttributeNoGroupBy { const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; @@ -3837,7 +3841,7 @@ class AttributePassword extends AttributeString * * @package iTopORM */ -class AttributeEncryptedString extends AttributeString +class AttributeEncryptedString extends AttributeString implements iAttributeNoGroupBy { const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; @@ -9217,7 +9221,7 @@ class AttributeSubItem extends AttributeDefinition /** * One way encrypted (hashed) password */ -class AttributeOneWayPassword extends AttributeDefinition +class AttributeOneWayPassword extends AttributeDefinition implements iAttributeNoGroupBy { const SEARCH_WIDGET_TYPE = self::SEARCH_WIDGET_TYPE_RAW; diff --git a/core/dbsearch.class.php b/core/dbsearch.class.php index e59493d80..f9de30d76 100644 --- a/core/dbsearch.class.php +++ b/core/dbsearch.class.php @@ -1186,6 +1186,30 @@ abstract class DBSearch } } } + + if (is_array($aGroupByExpr)) + { + foreach($aGroupByExpr as $sAlias => $oGroupByExp) + { + /** @var \Expression $oGroupByExp */ + + $aFields = $oGroupByExp->ListRequiredFields(); + foreach($aFields as $sFieldAlias) + { + $aMatches = array(); + if (preg_match('/^([^.]+)\\.([^.]+)$/', $sFieldAlias, $aMatches)) + { + $sFieldClass = $this->GetClassName($aMatches[1]); + $oAttDef = MetaModel::GetAttributeDef($sFieldClass, $aMatches[2]); + if ( $oAttDef instanceof iAttributeNoGroupBy) + { + throw new Exception("Grouping on '$sFieldClass' fields is not supported."); + } + } + } + } + } + $oSQLQuery = $oSearch->GetSQLQueryStructure($aAttToLoad, $bGetCount, $aGroupByExpr, null, $aSelectExpr); $oSQLQuery->SetSourceOQL($oSearch->ToOQL()); diff --git a/dictionaries/en.dictionary.itop.ui.php b/dictionaries/en.dictionary.itop.ui.php index 5d4a4215f..6206470ee 100644 --- a/dictionaries/en.dictionary.itop.ui.php +++ b/dictionaries/en.dictionary.itop.ui.php @@ -457,6 +457,7 @@ Dict::Add('EN US', 'English', 'English', array( 'UI:Error:ObjectsAlreadyDeleted' => 'Error: objects have already been deleted!', 'UI:Error:BulkDeleteNotAllowedOn_Class' => 'You are not allowed to perform a bulk delete of objects of class %1$s', 'UI:Error:DeleteNotAllowedOn_Class' => 'You are not allowed to delete objects of class %1$s', + 'UI:Error:ReadNotAllowedOn_Class' => 'You are not allowed to view objects of class %1$s', 'UI:Error:BulkModifyNotAllowedOn_Class' => 'You are not allowed to perform a bulk update of objects of class %1$s', 'UI:Error:ObjectAlreadyCloned' => 'Error: the object has already been cloned!', 'UI:Error:ObjectAlreadyCreated' => 'Error: the object has already been created!', diff --git a/dictionaries/fr.dictionary.itop.ui.php b/dictionaries/fr.dictionary.itop.ui.php index db43f78bf..0bdfdb57b 100644 --- a/dictionaries/fr.dictionary.itop.ui.php +++ b/dictionaries/fr.dictionary.itop.ui.php @@ -439,7 +439,8 @@ Dict::Add('FR FR', 'French', 'Français', array( 'UI:Error:ObjectCannotBeUpdated' => 'Erreur: l\'objet ne peut pas être mis à jour.', 'UI:Error:ObjectsAlreadyDeleted' => 'Erreur: les objets ont déjà été supprimés !', 'UI:Error:BulkDeleteNotAllowedOn_Class' => 'Vous n\'êtes pas autorisé à faire une suppression massive sur les objets de type %1$s', - 'UI:Error:DeleteNotAllowedOn_Class' => 'Vous n\'êtes pas autorisé supprimer des objets de type %1$s', + 'UI:Error:DeleteNotAllowedOn_Class' => 'Vous n\'êtes pas autorisé à supprimer des objets de type %1$s', + 'UI:Error:ReadNotAllowedOn_Class' => 'Vous n\'êtes pas autorisé à voir des objets de type %1$s', 'UI:Error:BulkModifyNotAllowedOn_Class' => 'Vous n\'êtes pas autorisé à faire une modification massive sur les objets de type %1$s', 'UI:Error:ObjectAlreadyCloned' => 'Erreur: l\'objet a déjà été dupliqué !', 'UI:Error:ObjectAlreadyCreated' => 'Erreur: l\'objet a déjà été créé !', From 2276539f24689e215d88219b7d69ecfa8212f6eb Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Wed, 24 Feb 2021 16:47:22 +0100 Subject: [PATCH 06/16] =?UTF-8?q?N=C2=B03430=20-=20code=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- sources/form/formmanager.class.inc.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/sources/form/formmanager.class.inc.php b/sources/form/formmanager.class.inc.php index 4d87b6a23..feeb2359e 100644 --- a/sources/form/formmanager.class.inc.php +++ b/sources/form/formmanager.class.inc.php @@ -183,8 +183,6 @@ abstract class FormManager /** * @param array $aData * - * @return array - * * @since 2.7.4 3.0.0 N°3430 */ public function CheckTransaction(&$aData) From db13c105ad042a9e358409f968acd15635ae1d06 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Wed, 24 Feb 2021 17:38:54 +0100 Subject: [PATCH 07/16] =?UTF-8?q?N=C2=B03473=20-=20PHPdoc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit as requested by @piRGoif --- core/attributedef.class.inc.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/attributedef.class.inc.php b/core/attributedef.class.inc.php index 44b165b63..2e84a8d71 100644 --- a/core/attributedef.class.inc.php +++ b/core/attributedef.class.inc.php @@ -103,6 +103,10 @@ define('LINKSET_EDITMODE_ACTIONS', 2); // Show the usual 'Actions' popup menu define('LINKSET_EDITMODE_INPLACE', 3); // The "linked" objects can be created/modified/deleted in place define('LINKSET_EDITMODE_ADDREMOVE', 4); // The "linked" objects can be added/removed in place +/** + * Attributes implementing this interface won't be accepted as `group by` field + * @since 2.7.4 N°3473 + */ interface iAttributeNoGroupBy { //no method, just a contract on implement From 2763b991429969447ebdef8d69d06acb4119ebee Mon Sep 17 00:00:00 2001 From: Eric Date: Thu, 25 Feb 2021 14:44:16 +0100 Subject: [PATCH 08/16] #1946 Fix Twig templates logging too much --- .../application/TwigBase/Twig/TwigHelper.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/sources/application/TwigBase/Twig/TwigHelper.php b/sources/application/TwigBase/Twig/TwigHelper.php index 9637d54d7..56834d3a8 100644 --- a/sources/application/TwigBase/Twig/TwigHelper.php +++ b/sources/application/TwigBase/Twig/TwigHelper.php @@ -47,19 +47,23 @@ class TwigHelper { $oTwig = self::GetTwigEnvironment($sViewPath); $oPage->add(self::RenderTemplate($oTwig, $aParams, $sTemplateName, $sDefaultType)); - $oPage->add_script(self::RenderTemplate($oTwig, $aParams, $sTemplateName, 'js')); - $oPage->add_ready_script(self::RenderTemplate($oTwig, $aParams, $sTemplateName, 'ready.js')); + $oPage->add_script(self::RenderTemplate($oTwig, $aParams, $sTemplateName, 'js', false)); + $oPage->add_ready_script(self::RenderTemplate($oTwig, $aParams, $sTemplateName, 'ready.js',false)); } /** * @param \Twig\Environment $oTwig - * @param $aParams - * @param $sName - * @param $sTemplateFileExtension + * @param array $aParams + * @param string $sName + * @param string $sTemplateFileExtension + * @param bool $bLogMissingFile * * @return string + * @throws \Twig\Error\LoaderError + * @throws \Twig\Error\RuntimeError + * @throws \Twig\Error\SyntaxError */ - private static function RenderTemplate(Environment $oTwig, $aParams, $sName, $sTemplateFileExtension) + private static function RenderTemplate(Environment $oTwig, $aParams, $sName, $sTemplateFileExtension, $bLogMissingFile = true) { try { @@ -71,7 +75,7 @@ class TwigHelper { IssueLog::Error($e->getMessage()); } - else + elseif ($bLogMissingFile) { IssueLog::Debug($e->getMessage()); } From 77710f1613a2a08d5bce31081b5af1a376c5f310 Mon Sep 17 00:00:00 2001 From: Eric Date: Thu, 25 Feb 2021 17:57:39 +0100 Subject: [PATCH 09/16] Revert "#1946 Fix Twig templates logging too much" This reverts commit 2763b991 --- .../application/TwigBase/Twig/TwigHelper.php | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/sources/application/TwigBase/Twig/TwigHelper.php b/sources/application/TwigBase/Twig/TwigHelper.php index 56834d3a8..9637d54d7 100644 --- a/sources/application/TwigBase/Twig/TwigHelper.php +++ b/sources/application/TwigBase/Twig/TwigHelper.php @@ -47,23 +47,19 @@ class TwigHelper { $oTwig = self::GetTwigEnvironment($sViewPath); $oPage->add(self::RenderTemplate($oTwig, $aParams, $sTemplateName, $sDefaultType)); - $oPage->add_script(self::RenderTemplate($oTwig, $aParams, $sTemplateName, 'js', false)); - $oPage->add_ready_script(self::RenderTemplate($oTwig, $aParams, $sTemplateName, 'ready.js',false)); + $oPage->add_script(self::RenderTemplate($oTwig, $aParams, $sTemplateName, 'js')); + $oPage->add_ready_script(self::RenderTemplate($oTwig, $aParams, $sTemplateName, 'ready.js')); } /** * @param \Twig\Environment $oTwig - * @param array $aParams - * @param string $sName - * @param string $sTemplateFileExtension - * @param bool $bLogMissingFile + * @param $aParams + * @param $sName + * @param $sTemplateFileExtension * * @return string - * @throws \Twig\Error\LoaderError - * @throws \Twig\Error\RuntimeError - * @throws \Twig\Error\SyntaxError */ - private static function RenderTemplate(Environment $oTwig, $aParams, $sName, $sTemplateFileExtension, $bLogMissingFile = true) + private static function RenderTemplate(Environment $oTwig, $aParams, $sName, $sTemplateFileExtension) { try { @@ -75,7 +71,7 @@ class TwigHelper { IssueLog::Error($e->getMessage()); } - elseif ($bLogMissingFile) + else { IssueLog::Debug($e->getMessage()); } From 35155e4b7ab3e06055a58fd33627a0ec717df344 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Fri, 26 Feb 2021 10:06:29 +0100 Subject: [PATCH 10/16] =?UTF-8?q?:bulb:=20N=C2=B03065=20comments=20modific?= =?UTF-8?q?ations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/cmdbsource.class.inc.php | 35 +++++++++++++++++++++-------------- test/core/CMDBSourceTest.php | 2 +- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/core/cmdbsource.class.inc.php b/core/cmdbsource.class.inc.php index 4dcdbec7b..183bd1c71 100644 --- a/core/cmdbsource.class.inc.php +++ b/core/cmdbsource.class.inc.php @@ -1181,18 +1181,23 @@ class CMDBSource } /** - * There may have some differences between DB : for example in MySQL 5.7 we have "INT", while in MariaDB >= 10.2 you get "int DEFAULT - * 'NULL'" + * There may have some differences between DB ! For example in : + * * MySQL 5.7 we have `INT` + * * MariaDB >= 10.2 you get `int DEFAULT 'NULL'` * - * We still do a case sensitive comparison for enum values ! + * We still need to do a case sensitive comparison for enum values ! * * A better solution would be to generate SQL field definitions ({@link GetFieldSpec} method) based on the DB used... But for * now (N°2490 / SF #1756 / PR #91) we did implement this simpler solution * - * @param string $sItopGeneratedFieldType + * @see GetFieldDataTypeAndOptions extracts all info from the SQL field definition + * * @param string $sDbFieldType * + * @param string $sItopGeneratedFieldType + * * @return bool true if same type and options (case sensitive comparison only for type options), false otherwise + * * @throws \CoreException * @since 2.7.0 N°2490 */ @@ -1241,12 +1246,15 @@ class CMDBSource } /** - * @param string $sCompleteFieldType sql field type, for example 'VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT 0' + * @see \self::GetEnumOptions() specific processing for ENUM fields + * + * @param string $sCompleteFieldType sql field type, for example `VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT 0` * * @return string[] consisting of 3 items : - * 1. data type : for example 'VARCHAR' - * 2. type value : for example '255' - * 3. other options : for example ' CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT 0' + * 1. data type : for example `VARCHAR` + * 2. type value : for example `255` + * 3. other options : for example `CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT 0` + * * @throws \CoreException */ private static function GetFieldDataTypeAndOptions($sCompleteFieldType) @@ -1266,18 +1274,17 @@ class CMDBSource } /** - * @since 2.7.4 N°3065 - * Handle ENUM options - * - * @param $sDataType - * @param $sCompleteFieldType - * Example: ENUM('CSP A','CSP (aaaa) M','NA','OEM(ROC)','OPEN(VL)','RETAIL (Boite)') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci + * @param string $sDataType for example `ENUM` + * @param string $sCompleteFieldType Example: + * `ENUM('CSP A','CSP (aaaa) M','NA','OEM(ROC)','OPEN(VL)','RETAIL (Boite)') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci` * * @return string[] consisting of 3 items : * 1. data type : ENUM or enum here * 2. type value : in-between EUM parenthesis * 3. other options : for example ' CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT 0' * @throws \CoreException + * @since 2.7.4 N°3065 specific processing for enum fields : fix no alter table when enum values containing parenthesis + * Handle ENUM options */ private static function GetEnumOptions($sDataType, $sCompleteFieldType) { diff --git a/test/core/CMDBSourceTest.php b/test/core/CMDBSourceTest.php index 132d71292..ee87cf2c4 100644 --- a/test/core/CMDBSourceTest.php +++ b/test/core/CMDBSourceTest.php @@ -106,7 +106,7 @@ class CMDBSourceTest extends ItopTestCase "ENUM('CSP A','CSP M','NA','OEM(ROC)','OPEN(VL)','RETAIL (Boite)') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", "enum('CSP A','CSP M','NA','OEM(ROC)','OPEN(VL)','RETAIL (Boite)') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", ), - //FIXME N°3065 before the fix this returns true :( + // N°3065 before the fix this returned true :( 'ENUM with different values, containing parenthesis' => array( false, "ENUM('value 1 (with parenthesis)','value 2') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", From 13a1d32f56c3d6f469858b21692303db2ac07125 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Fri, 26 Feb 2021 11:47:08 +0100 Subject: [PATCH 11/16] =?UTF-8?q?N=C2=B03453=20-=20portal=20export=20heade?= =?UTF-8?q?r=20fields=20are=20now=20localized?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../portal/src/Controller/ManageBrickController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Controller/ManageBrickController.php b/datamodels/2.x/itop-portal-base/portal/src/Controller/ManageBrickController.php index 615e272f5..147d8ebe7 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Controller/ManageBrickController.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Controller/ManageBrickController.php @@ -252,6 +252,7 @@ class ManageBrickController extends BrickController $oExporter->SetObjectList($oSearch); $oExporter->SetFormat($sFormat); $oExporter->SetChunkSize(EXPORTER_DEFAULT_CHUNK_SIZE); + $oExporter->SetLocalizeOutput(true); $oExporter->SetFields($sFields); $aData = array( From 95a0efedcf39d2c85603d10db7119241fca80cc1 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Mon, 1 Mar 2021 15:28:34 +0100 Subject: [PATCH 12/16] =?UTF-8?q?N=C2=B03728=20-=20security=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/transaction.class.inc.php | 6 +- .../application/privUITransactionFileTest.php | 135 ++++++++++++++++++ 2 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 test/application/privUITransactionFileTest.php diff --git a/application/transaction.class.inc.php b/application/transaction.class.inc.php index eac5ef6c9..d5388976f 100644 --- a/application/transaction.class.inc.php +++ b/application/transaction.class.inc.php @@ -297,11 +297,11 @@ class privUITransactionFile * Cleanup old transactions which have been pending since more than 24 hours * Use filemtime instead of filectime since filectime may be affected by operations on the directory (like changing the access rights) */ - protected static function CleanupOldTransactions() + protected static function CleanupOldTransactions($sTransactionDir = null) { $iLimit = time() - 24*3600; - clearstatcache(); - $aTransactions = glob(APPROOT.'data/transactions/*-*'); + $sPattern = $sTransactionDir ? "$sTransactionDir/*" : APPROOT.'data/transactions/*'; + $aTransactions = glob($sPattern); foreach($aTransactions as $sFileName) { if (filemtime($sFileName) < $iLimit) diff --git a/test/application/privUITransactionFileTest.php b/test/application/privUITransactionFileTest.php new file mode 100644 index 000000000..d15c20ad5 --- /dev/null +++ b/test/application/privUITransactionFileTest.php @@ -0,0 +1,135 @@ +Set('transactions_gc_threshold', 100); + + $iBaseLimit = time() - 24*3600; //24h + + $sBaseDir = sys_get_temp_dir(); + $sDir = "$sBaseDir/privUITransactionFileTest/cleanupOldTransactions"; + if (is_dir($sDir)) { + $this->rm($sDir); + } + mkdir("$sDir", 0777, true); + + for ($i = 0; $i < $iCleanableCreated; $i++) { + touch("$sDir/{$sCleanablePrefix}$i", $iBaseLimit - 10*60); + } + for ($i = 0; $i < $iPreservableCreated; $i++) { + touch("$sDir/{$sPreservablePrefix}$i", $iBaseLimit + 10*60); + } + + $iCleanableCount = count(glob("$sDir/{$sCleanablePrefix}*")); + $iPreservableCount = count(glob("$sDir/{$sPreservablePrefix}*")); + $this->assertEquals($iCleanableCreated, $iCleanableCount); + $this->assertEquals($iPreservableCreated, $iPreservableCount); + + $aArgs = [ + 'sTransactionDir' => "$sDir", + ]; + $oprivUITransactionFile = new privUITransactionFile(); + $this->InvokeNonPublicMethod(get_class($oprivUITransactionFile), 'CleanupOldTransactions', $oprivUITransactionFile, $aArgs); + + $iCleanableCount = count(glob("$sDir/{$sCleanablePrefix}*")); + $iPreservableCount = count(glob("$sDir/{$sPreservablePrefix}*")); + $this->assertEquals(0, $iCleanableCount); + $this->assertEquals($iPreservableCreated, $iPreservableCount); + } + + public function cleanupOldTransactionsProvider() + { + $iBaseLimit = time() - 60 * 10; //ten minutes ago + + $sBaseDir = sys_get_temp_dir(); + $sDir = "$sBaseDir/privUITransactionFileTest/cleanupOldTransactions"; + + return [ + 'linux - no content' => [ + 'iCleanableCreated' => 0, + 'iPreservableCreated' => 0, + 'sCleanablePrefix' => 'cleanable-', + 'sPreservablePrefix' => 'preservable-', + ], + 'linux - cleanable content' => [ + 'iCleanableCreated' => 2, + 'iPreservableCreated' => 0, + 'sCleanablePrefix' => 'cleanable-', + 'sPreservablePrefix' => 'preservable-', + ], + 'linux - preseved content' => [ + 'iCleanableCreated' => 0, + 'iPreservableCreated' => 2, + 'sCleanablePrefix' => 'cleanable-', + 'sPreservablePrefix' => 'preservable-', + ], + 'win - no content' => [ + 'iCleanableCreated' => 0, + 'iPreservableCreated' => 0, + 'sCleanablePrefix' => 'cle', + 'sPreservablePrefix' => 'pre', + ], + 'win - cleanable content' => [ + 'iCleanableCreated' => 2, + 'iPreservableCreated' => 0, + 'sCleanablePrefix' => 'cle', + 'sPreservablePrefix' => 'pre', + ], + 'win - preseved content' => [ + 'iCleanableCreated' => 0, + 'iPreservableCreated' => 2, + 'sCleanablePrefix' => 'cle', + 'sPreservablePrefix' => 'pre', + ], + ]; + } + + public function rm($sDir) { + $aFiles = array_diff(scandir($sDir), ['.','..']); + foreach ($aFiles as $sFile) { + if ((is_dir("$sDir/$sFile"))) { + $this->rm("$sDir/$sFile"); + } else { + unlink("$sDir/$sFile"); + } + } + return rmdir($sDir); + } +} From 0030d5c2b8d0448a6c451a179904975f299578e7 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Mon, 1 Mar 2021 15:30:40 +0100 Subject: [PATCH 13/16] =?UTF-8?q?N=C2=B03764=20-=20add=20transactions=5Fgc?= =?UTF-8?q?=5Fthreshold=20in=20order=20to=20tune=20CSRF=20token=20GC=20loa?= =?UTF-8?q?d?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/transaction.class.inc.php | 8 ++++++++ core/config.class.inc.php | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/application/transaction.class.inc.php b/application/transaction.class.inc.php index d5388976f..8e3afb2ee 100644 --- a/application/transaction.class.inc.php +++ b/application/transaction.class.inc.php @@ -299,6 +299,14 @@ class privUITransactionFile */ protected static function CleanupOldTransactions($sTransactionDir = null) { + $iThreshold = (int) MetaModel::GetConfig()->Get('transactions_gc_threshold'); + $iThreshold = min(100, $iThreshold); + $iThreshold = max(1, $iThreshold); + if ((100 != $iThreshold) && (rand(1, 100) > $iThreshold)) { + return; + } + + clearstatcache(); $iLimit = time() - 24*3600; $sPattern = $sTransactionDir ? "$sTransactionDir/*" : APPROOT.'data/transactions/*'; $aTransactions = glob($sPattern); diff --git a/core/config.class.inc.php b/core/config.class.inc.php index 0947b20b1..5d6e63938 100644 --- a/core/config.class.inc.php +++ b/core/config.class.inc.php @@ -1065,6 +1065,14 @@ class Config 'source_of_value' => '', 'show_in_conf_sample' => false, ), + 'transactions_gc_threshold' => array( + 'type' => 'integer', + 'description' => 'probability in percent for the garbage collector to be triggered (100 mean always)', + 'default' => 10, // added in itop 2.7.4, before the GC was always called + 'value' => '', + 'source_of_value' => '', + 'show_in_conf_sample' => false, + ), 'log_transactions' => array( 'type' => 'bool', 'description' => 'Whether or not to enable the debug log for the transactions.', From cd4b3fdaab1c30db4e791370797410bfbde3c633 Mon Sep 17 00:00:00 2001 From: bruno-ds Date: Mon, 1 Mar 2021 16:27:40 +0100 Subject: [PATCH 14/16] =?UTF-8?q?N=C2=B03764=20-=20fix=20CI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/application/privUITransactionFileTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/application/privUITransactionFileTest.php b/test/application/privUITransactionFileTest.php index d15c20ad5..1b27ebe86 100644 --- a/test/application/privUITransactionFileTest.php +++ b/test/application/privUITransactionFileTest.php @@ -39,7 +39,7 @@ class privUITransactionFileTest extends ItopTestCase */ public function testCleanupOldTransactions($iCleanableCreated, $iPreservableCreated, $sCleanablePrefix, $sPreservablePrefix) { -// MetaModel::GetConfig()->Set('transactions_gc_threshold', 100); + MetaModel::GetConfig()->Set('transactions_gc_threshold', 100); $iBaseLimit = time() - 24*3600; //24h From 5c0e92d51ab155ba2627db9ecead6ffed41b4859 Mon Sep 17 00:00:00 2001 From: odain Date: Mon, 1 Mar 2021 17:06:49 +0100 Subject: [PATCH 15/16] =?UTF-8?q?N=C2=B03065=20-=20Failed=20enum=20compari?= =?UTF-8?q?son=20when=20values=20contains=20parenthesis?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/cmdbsource.class.inc.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/cmdbsource.class.inc.php b/core/cmdbsource.class.inc.php index 183bd1c71..de7840923 100644 --- a/core/cmdbsource.class.inc.php +++ b/core/cmdbsource.class.inc.php @@ -1264,7 +1264,11 @@ class CMDBSource $sDataType = isset($aMatches[1]) ? $aMatches[1] : ''; if (strcasecmp($sDataType, 'ENUM') === 0){ - return self::GetEnumOptions($sDataType, $sCompleteFieldType); + try{ + return self::GetEnumOptions($sDataType, $sCompleteFieldType); + }catch(CoreException $e){ + //do nothing ; especially do not block setup. + } } $sTypeOptions = isset($aMatches[2]) ? $aMatches[3] : ''; From d4607ee8158bf157cbac29bb3be364c342ef3f52 Mon Sep 17 00:00:00 2001 From: odain Date: Tue, 2 Mar 2021 07:33:36 +0100 Subject: [PATCH 16/16] =?UTF-8?q?N=C2=B03065=20-=20Failed=20enum=20compari?= =?UTF-8?q?son=20when=20values=20contains=20parenthesis=20:=20add=20a=20wa?= =?UTF-8?q?rning?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/cmdbsource.class.inc.php | 1 + test/core/CMDBSourceTest.php | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/core/cmdbsource.class.inc.php b/core/cmdbsource.class.inc.php index de7840923..285b42ed8 100644 --- a/core/cmdbsource.class.inc.php +++ b/core/cmdbsource.class.inc.php @@ -1268,6 +1268,7 @@ class CMDBSource return self::GetEnumOptions($sDataType, $sCompleteFieldType); }catch(CoreException $e){ //do nothing ; especially do not block setup. + IssueLog::Warning("enum was not parsed properly: $sCompleteFieldType. it should not happen during setup."); } } diff --git a/test/core/CMDBSourceTest.php b/test/core/CMDBSourceTest.php index ee87cf2c4..229db18b5 100644 --- a/test/core/CMDBSourceTest.php +++ b/test/core/CMDBSourceTest.php @@ -21,6 +21,7 @@ class CMDBSourceTest extends ItopTestCase { protected function setUp() { + parent::setUp(); require_once(APPROOT.'/core/cmdbsource.class.inc.php'); } @@ -112,6 +113,11 @@ class CMDBSourceTest extends ItopTestCase "ENUM('value 1 (with parenthesis)','value 2') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", "enum('value 1 (with parenthesis)','value 3') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", ), + 'ENUM with different values, containing parenthesis' => array( + false, + "ENUM('value 1 ) with parenthesis)','value 2') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", + "enum('value 1 (with parenthesis)','value 3') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci", + ), ); } }