From bdf11e32a76be70252e3d42d5223f91009dcf70e Mon Sep 17 00:00:00 2001 From: Molkobain Date: Wed, 24 Aug 2022 10:47:40 +0200 Subject: [PATCH 1/8] =?UTF-8?q?N=C2=B05318=20-=20Security=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/cmdbchangeop.class.inc.php | 2 +- .../Component/FieldBadge/FieldBadgeUIBlockFactory.php | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/core/cmdbchangeop.class.inc.php b/core/cmdbchangeop.class.inc.php index 537ca8d00..6dbcf3f75 100644 --- a/core/cmdbchangeop.class.inc.php +++ b/core/cmdbchangeop.class.inc.php @@ -78,7 +78,7 @@ class CMDBChangeOp extends DBObject implements iCMDBChangeOp } /** - * Describe (as a text string) the modifications corresponding to this change + * @inheritDoc */ public function GetDescription() { diff --git a/sources/application/UI/Base/Component/FieldBadge/FieldBadgeUIBlockFactory.php b/sources/application/UI/Base/Component/FieldBadge/FieldBadgeUIBlockFactory.php index 353254153..e8cd1c94e 100644 --- a/sources/application/UI/Base/Component/FieldBadge/FieldBadgeUIBlockFactory.php +++ b/sources/application/UI/Base/Component/FieldBadge/FieldBadgeUIBlockFactory.php @@ -10,6 +10,7 @@ namespace Combodo\iTop\Application\UI\Base\Component\FieldBadge; use Combodo\iTop\Application\UI\Base\AbstractUIBlockFactory; use ormStyle; +use utils; /** * Class FieldBadgeUIBlockFactory @@ -36,6 +37,10 @@ class FieldBadgeUIBlockFactory extends AbstractUIBlockFactory { $oBadge = null; $sHtml = ''; + + // N°5318 - Sanitize value manually as this UIBlock is not using a proper TWIG template 😥 + $sValueForHtml = utils::EscapeHtml($sValue); + if ($oStyle) { $sStyleClass = $oStyle->GetStyleClass(); $sPrimaryColor = $oStyle->GetMainColor(); @@ -47,12 +52,12 @@ class FieldBadgeUIBlockFactory extends AbstractUIBlockFactory if (!is_null($sDecorationClasses) && !empty($sDecorationClasses)) { $sHtml .= ""; } - $sHtml .= "$sValue"; + $sHtml .= "$sValueForHtml"; } } if (!$oBadge) { $oBadge = new FieldBadge(); - $sHtml .= "$sValue"; + $sHtml .= "$sValueForHtml"; } $oBadge->AddHtml($sHtml); return $oBadge; From 0b46ab9e48b5189c867966dfb5f9b6decc95cda3 Mon Sep 17 00:00:00 2001 From: odain Date: Wed, 24 Aug 2022 11:02:08 +0200 Subject: [PATCH 2/8] ci: add phpunit defaultProfiles annotation + tag some tests with addition annotations to ease exclusion/inclusion mechanism --- test/application/privUITransactionFileTest.php | 1 + test/core/CMDBSource/TransactionsTest.php | 3 ++- test/core/OQLParserTest.php | 1 + test/core/UserRightsTest.php | 7 ++++--- test/synchro/DataSynchroTest.php | 1 + test/webservices/RestTest.php | 1 + 6 files changed, 10 insertions(+), 4 deletions(-) diff --git a/test/application/privUITransactionFileTest.php b/test/application/privUITransactionFileTest.php index f7960cbe6..cbdf782b2 100644 --- a/test/application/privUITransactionFileTest.php +++ b/test/application/privUITransactionFileTest.php @@ -26,6 +26,7 @@ use Combodo\iTop\Test\UnitTest\ItopDataTestCase; * * @covers utils * @group sampleDataNeeded + * @group defaultProfiles */ class privUITransactionFileTest extends ItopDataTestCase { diff --git a/test/core/CMDBSource/TransactionsTest.php b/test/core/CMDBSource/TransactionsTest.php index e7c4a11e1..00f9872d8 100644 --- a/test/core/CMDBSource/TransactionsTest.php +++ b/test/core/CMDBSource/TransactionsTest.php @@ -18,6 +18,7 @@ use MetaModel; * @backupGlobals disabled * * @group itopRequestMgmt + * @group specificOrgInSampleData * Class TransactionsTest * * @package Combodo\iTop\Test\UnitTest\Core @@ -248,4 +249,4 @@ class TransactionsTest extends ItopTestCase "History 13" => ['iFailAt' => 15, 'bIsModified' => true], ]; } -} \ No newline at end of file +} diff --git a/test/core/OQLParserTest.php b/test/core/OQLParserTest.php index afeddc664..63cd2c626 100644 --- a/test/core/OQLParserTest.php +++ b/test/core/OQLParserTest.php @@ -29,6 +29,7 @@ class OQLParserTest extends ItopDataTestCase * @group iTopChangeMgt * @group itopConfigMgmt * @group itopRequestMgmt + * @group specificOrgInSampleData * @dataProvider NestedQueryProvider * * @param $sQuery diff --git a/test/core/UserRightsTest.php b/test/core/UserRightsTest.php index 0c54ebf8f..c3043d85e 100644 --- a/test/core/UserRightsTest.php +++ b/test/core/UserRightsTest.php @@ -40,6 +40,7 @@ use utils; /** * @group itopRequestMgmt * @group userRights + * @group defaultProfiles * * @runTestsInSeparateProcesses * @preserveGlobalState disabled @@ -486,7 +487,7 @@ class UserRightsTest extends ItopDataTestCase // logout $_SESSION = []; } - + public function NonAdminCanListOwnProfilesProvider(): array { return [ @@ -495,7 +496,7 @@ class UserRightsTest extends ItopDataTestCase ]; } /** - *@dataProvider NonAdminCannotListAdminProfilesProvider + *@dataProvider NonAdminCannotListAdminProfilesProvider */ public function testNonAdminCannotListAdminProfiles($bHideAdministrators, $iExpectedCount) { @@ -518,7 +519,7 @@ class UserRightsTest extends ItopDataTestCase // logout $_SESSION = []; } - + public function NonAdminCannotListAdminProfilesProvider(): array { return [ diff --git a/test/synchro/DataSynchroTest.php b/test/synchro/DataSynchroTest.php index c3dea91f4..29af23830 100644 --- a/test/synchro/DataSynchroTest.php +++ b/test/synchro/DataSynchroTest.php @@ -30,6 +30,7 @@ use utils; * * @package Combodo\iTop\Test\UnitTest\Synchro * @group dataSynchro + * @group defaultProfiles * * @runTestsInSeparateProcesses * @preserveGlobalState disabled diff --git a/test/webservices/RestTest.php b/test/webservices/RestTest.php index 8e3c95018..d71cefa86 100644 --- a/test/webservices/RestTest.php +++ b/test/webservices/RestTest.php @@ -9,6 +9,7 @@ use Exception; /** * @group itopRequestMgmt * @group restApi + * @group defaultProfiles * * @runTestsInSeparateProcesses * @preserveGlobalState disabled From f923ac879f7e771301595d141a1f0dd0a46b6841 Mon Sep 17 00:00:00 2001 From: odain Date: Wed, 24 Aug 2022 12:07:04 +0200 Subject: [PATCH 3/8] ci: tag some tests with addition annotations to ease exclusion/inclusion mechanism --- test/core/DBSearchTest.php | 1 + test/core/GetSelectFilterTest.php | 21 +++++++++++---------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/test/core/DBSearchTest.php b/test/core/DBSearchTest.php index 0ef928602..dded9f97b 100644 --- a/test/core/DBSearchTest.php +++ b/test/core/DBSearchTest.php @@ -528,6 +528,7 @@ class DBSearchTest extends ItopDataTestCase /** * @dataProvider GetFirstResultProvider + * @group specificOrgInSampleData * * @param string $sOql query to test * @param bool $bMustHaveOneResultMax arg passed to the tested function diff --git a/test/core/GetSelectFilterTest.php b/test/core/GetSelectFilterTest.php index 449d53c47..a3db7e145 100644 --- a/test/core/GetSelectFilterTest.php +++ b/test/core/GetSelectFilterTest.php @@ -12,8 +12,9 @@ use utils; /** - * @group getSelectFilterTest + * @group getSelectFilterTest * @group sampleDataNeeded + * @group specificOrgInSampleData * Class GetSelectFilterTest * * @runTestsInSeparateProcesses @@ -35,9 +36,9 @@ class GetSelectFilterTest extends ItopDataTestCase $oRestProfile = MetaModel::GetObjectFromOQL("SELECT URP_Profiles WHERE name = :name", array('name' => 'REST Services User'), true); $oAdminProfile = MetaModel::GetObjectFromOQL("SELECT URP_Profiles WHERE name = :name", array('name' => 'Administrator'), true); - + $this->sLogin = "getselectfilter-user-" . date('dmYHis'); - + // Ensure that we have at least one administrator account if (is_object($oRestProfile) && is_object($oAdminProfile)) { @@ -45,7 +46,7 @@ class GetSelectFilterTest extends ItopDataTestCase $this->AddProfileToUser($this->oUser, $oAdminProfile->GetKey()); } } - + public function testGetSelectFilter() { $oUserRights = new UserRightsProfile(); @@ -64,9 +65,9 @@ class GetSelectFilterTest extends ItopDataTestCase //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // Default behavior: Administrators, Administrator profile and URP_UserProfile related to administrators are visible // via GetSelectFilter - + $oConfig->Set('security.hide_administrators', false); - + $oFilterProfiles = $oUserRights->GetSelectFilter($this->oUser, 'URP_Profiles'); if ($oFilterProfiles === true) { @@ -83,7 +84,7 @@ class GetSelectFilterTest extends ItopDataTestCase } } $this->assertEquals($bAdminProfileFound, true); - + foreach($aUserLocalAncestors as $sUserClass) { $bAdminUserFound = false; @@ -103,7 +104,7 @@ class GetSelectFilterTest extends ItopDataTestCase } $this->assertEquals($bAdminUserFound, true); } - + $oFilterLnkProfiles = $oUserRights->GetSelectFilter($this->oUser, 'URP_UserProfile'); if ($oFilterLnkProfiles === true) { @@ -160,6 +161,6 @@ class GetSelectFilterTest extends ItopDataTestCase $this->assertNotEquals($oLnk->Get('userid'), $this->oUser->GetKey()); $this->assertNotEquals($oLnk->Get('profileid'), 1); } - + } -} \ No newline at end of file +} From 7a6a3d1ac0693f072681ae9c30aff18277e3d3ad Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Wed, 24 Aug 2022 14:41:32 +0200 Subject: [PATCH 4/8] Integration tests : move and comment itop-community group --- test/integration/iTopModulesPhpVersionChecklistTest.php | 7 ++++--- test/integration/iTopModulesXmlVersionChecklistTest.php | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/test/integration/iTopModulesPhpVersionChecklistTest.php b/test/integration/iTopModulesPhpVersionChecklistTest.php index ffc711d33..9c66bda8e 100644 --- a/test/integration/iTopModulesPhpVersionChecklistTest.php +++ b/test/integration/iTopModulesPhpVersionChecklistTest.php @@ -22,8 +22,6 @@ use utils; /** - * @group itop-community - * * @covers iTopDesignFormat * * @package Combodo\iTop\Test\UnitTest\Setup @@ -31,9 +29,12 @@ use utils; class iTopModulesPhpVersionIntegrationTest extends ItopTestCase { /** - * Verify if the datamodel.*.xml files refer to the current itop version + * Verify if `module.*.php` files contained in `datamodels/1.x` or `datamodels/2.x` refers to the current itop version * This is an integration test * + * As ess and pro targets are copying modules into datamodels/2.x this test can only be run on a community target ! + * + * @group itop-community * @group skipPostBuild * * @dataProvider iTopModulesPhpVersionProvider diff --git a/test/integration/iTopModulesXmlVersionChecklistTest.php b/test/integration/iTopModulesXmlVersionChecklistTest.php index b6dfcb289..3cdb3cc25 100644 --- a/test/integration/iTopModulesXmlVersionChecklistTest.php +++ b/test/integration/iTopModulesXmlVersionChecklistTest.php @@ -21,8 +21,6 @@ use iTopDesignFormat; /** - * @group itop-community - * * @covers iTopDesignFormat * * @package Combodo\iTop\Test\UnitTest\Setup @@ -38,9 +36,12 @@ class iTopModulesXmlVersionIntegrationTest extends ItopTestCase /** - * Verify if the datamodel.*.xml files refer to the latest version of the design + * Verify if the `datamodels/2.x/datamodel.*.xml` files refer to the latest version of the design * This is an integration test * + * As ess and pro targets are copying modules into datamodels/2.x this test can only be run on a community target ! + * + * @group itop-community * @group skipPostBuild * * @dataProvider DatamodelItopXmlVersionProvider From 06c3e295d6bbde587ec261a764d72b4c4ab4f824 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Wed, 24 Aug 2022 14:54:02 +0200 Subject: [PATCH 5/8] Fix iTopXmlVersionIntegrationTest * remove annotations for data tests : we are only comparing constants * remove itop-community group : this test can be run whatever target was used to build * remove wrong `@covers` annotation --- test/integration/iTopXmlVersionChecklistTest.php | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/integration/iTopXmlVersionChecklistTest.php b/test/integration/iTopXmlVersionChecklistTest.php index d06c1fbf3..6a624a2c3 100644 --- a/test/integration/iTopXmlVersionChecklistTest.php +++ b/test/integration/iTopXmlVersionChecklistTest.php @@ -16,18 +16,9 @@ namespace Combodo\iTop\Test\UnitTest\Integration; use Combodo\iTop\Test\UnitTest\ItopTestCase; -use iTopDesignFormat; /** - * - * @runTestsInSeparateProcesses - * @preserveGlobalState disabled - * @backupGlobals disabled - * @group itop-community - * - * @covers iTopDesignFormat - * * @package Combodo\iTop\Test\UnitTest\Setup */ class iTopXmlVersionIntegrationTest extends ItopTestCase From d78a25ee4edd9e1b2eb7ecb5b7a643180b515775 Mon Sep 17 00:00:00 2001 From: Stephen Abello Date: Wed, 24 Aug 2022 16:33:54 +0200 Subject: [PATCH 6/8] =?UTF-8?q?N=C2=B05462=20Add=20a=20setup=20check=20to?= =?UTF-8?q?=20verify=20if=20directory-level=20configuration=20files=20(.ht?= =?UTF-8?q?access=20and=20web.config)=20are=20used=20by=20the=20server?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/permissions-test-folder/.htaccess | 13 +++++++++++++ .../permissions-test-file | 0 setup/permissions-test-folder/web.config | 13 +++++++++++++ setup/setup.js | 13 +++++++++++++ setup/wizardsteps.class.inc.php | 1 + 5 files changed, 40 insertions(+) create mode 100644 setup/permissions-test-folder/.htaccess create mode 100644 setup/permissions-test-folder/permissions-test-subfolder/permissions-test-file create mode 100644 setup/permissions-test-folder/web.config diff --git a/setup/permissions-test-folder/.htaccess b/setup/permissions-test-folder/.htaccess new file mode 100644 index 000000000..782472c78 --- /dev/null +++ b/setup/permissions-test-folder/.htaccess @@ -0,0 +1,13 @@ +# Apache 2.4 + +Require all denied + + +# Apache 2.2 + +deny from all +Satisfy All + + +# Apache 2.2 and 2.4 +IndexIgnore * diff --git a/setup/permissions-test-folder/permissions-test-subfolder/permissions-test-file b/setup/permissions-test-folder/permissions-test-subfolder/permissions-test-file new file mode 100644 index 000000000..e69de29bb diff --git a/setup/permissions-test-folder/web.config b/setup/permissions-test-folder/web.config new file mode 100644 index 000000000..58c9c3ac3 --- /dev/null +++ b/setup/permissions-test-folder/web.config @@ -0,0 +1,13 @@ + + + + + + + + + + + + + \ No newline at end of file diff --git a/setup/setup.js b/setup/setup.js index b83719322..62ae1b733 100644 --- a/setup/setup.js +++ b/setup/setup.js @@ -51,4 +51,17 @@ function ExecuteStep(sStep) $('#wiz_form').data('installation_status', 'error'); WizardUpdateButtons(); } ); +} + +function CheckDirectoryConfFilesPermissions(sWikiVersion){ + $.ajax('permissions-test-folder/permissions-test-subfolder/permissions-test-file', + { + statusCode: { + 200: function() { + $('#details').prepend('
Security issue: iTop is bundled with directory-level configuration files. You must check that those files will be read by your web server (eg.' + + 'AllowOverride directive should be set to All for Apache HTTP Server) see documentation.
'); + $(' and 1 Security issue').insertBefore('h2.message button:first'); + } + } + }); } \ No newline at end of file diff --git a/setup/wizardsteps.class.inc.php b/setup/wizardsteps.class.inc.php index 805a7de9e..81ff1c9c9 100644 --- a/setup/wizardsteps.class.inc.php +++ b/setup/wizardsteps.class.inc.php @@ -161,6 +161,7 @@ HTML $oPage->p('Sorry, the installation cannot continue. Please fix the errors and reload this page to launch the installation again.'); $oPage->p(''); } + $oPage->add_ready_script('CheckDirectoryConfFilesPermissions("'.utils::GetItopVersionWikiSyntax().'")'); } public function CanMoveForward() From 14facb4d6c2dba25d8ac8706ad5c962775a5566b Mon Sep 17 00:00:00 2001 From: Stephen Abello Date: Wed, 24 Aug 2022 16:33:54 +0200 Subject: [PATCH 7/8] =?UTF-8?q?N=C2=B05462=20Add=20a=20setup=20check=20to?= =?UTF-8?q?=20verify=20if=20directory-level=20configuration=20files=20(.ht?= =?UTF-8?q?access=20and=20web.config)=20are=20used=20by=20the=20server?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- setup/permissions-test-folder/.htaccess | 13 +++++++++++++ .../permissions-test-file | 0 setup/permissions-test-folder/web.config | 13 +++++++++++++ setup/setup.js | 13 +++++++++++++ setup/wizardsteps.class.inc.php | 1 + 5 files changed, 40 insertions(+) create mode 100644 setup/permissions-test-folder/.htaccess create mode 100644 setup/permissions-test-folder/permissions-test-subfolder/permissions-test-file create mode 100644 setup/permissions-test-folder/web.config diff --git a/setup/permissions-test-folder/.htaccess b/setup/permissions-test-folder/.htaccess new file mode 100644 index 000000000..782472c78 --- /dev/null +++ b/setup/permissions-test-folder/.htaccess @@ -0,0 +1,13 @@ +# Apache 2.4 + +Require all denied + + +# Apache 2.2 + +deny from all +Satisfy All + + +# Apache 2.2 and 2.4 +IndexIgnore * diff --git a/setup/permissions-test-folder/permissions-test-subfolder/permissions-test-file b/setup/permissions-test-folder/permissions-test-subfolder/permissions-test-file new file mode 100644 index 000000000..e69de29bb diff --git a/setup/permissions-test-folder/web.config b/setup/permissions-test-folder/web.config new file mode 100644 index 000000000..58c9c3ac3 --- /dev/null +++ b/setup/permissions-test-folder/web.config @@ -0,0 +1,13 @@ + + + + + + + + + + + + + \ No newline at end of file diff --git a/setup/setup.js b/setup/setup.js index bdec79eea..0556d9c71 100644 --- a/setup/setup.js +++ b/setup/setup.js @@ -53,4 +53,17 @@ function ExecuteStep(sStep) } ); } +function CheckDirectoryConfFilesPermissions(sWikiVersion){ + $.ajax('permissions-test-folder/permissions-test-subfolder/permissions-test-file', + { + statusCode: { + 200: function() { + $('#details').prepend('
Security issue: iTop is bundled with directory-level configuration files. You must check that those files will be read by your web server (eg. ' + + 'AllowOverride directive should be set to All for Apache HTTP Server) see documentation.
'); + $(' and 1 Security issue').insertBefore('h2.message button:first'); + } + } + }); +} + CombodoTooltip.InitAllNonInstantiatedTooltips(); \ No newline at end of file diff --git a/setup/wizardsteps.class.inc.php b/setup/wizardsteps.class.inc.php index eb4cba961..dcaa4c13a 100644 --- a/setup/wizardsteps.class.inc.php +++ b/setup/wizardsteps.class.inc.php @@ -165,6 +165,7 @@ HTML $oPage->p('Sorry, the installation cannot continue. Please fix the errors and reload this page to launch the installation again.'); $oPage->p(''); } + $oPage->add_ready_script('CheckDirectoryConfFilesPermissions("'.utils::GetItopVersionWikiSyntax().'")'); } public function CanMoveForward() From 02a0969b530e4e6e5eda3f854ebb3d41a9f2e42b Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Mon, 29 Aug 2022 12:31:45 +0200 Subject: [PATCH 8/8] =?UTF-8?q?N=C2=B05414=20Fix=20undefined=20constant=20?= =?UTF-8?q?error=20in=20notification=20with=20wrong=20placeholder=20Fix=20?= =?UTF-8?q?regression=20introduced=20in=2033c2168=20(#282)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/metamodel.class.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/metamodel.class.php b/core/metamodel.class.php index 744bfb26c..d523fcc87 100644 --- a/core/metamodel.class.php +++ b/core/metamodel.class.php @@ -7379,7 +7379,7 @@ abstract class MetaModel } IssueLog::Debug( 'Invalid placeholder in notification, no replacement will occur!', - LogChannels::NOTIFICATION, + LogChannels::NOTIFICATIONS, $aContext ); } @@ -7404,7 +7404,7 @@ abstract class MetaModel catch (Exception $e) { IssueLog::Debug( 'Invalid placeholder in notification, no replacement will occur !', - LogChannels::NOTIFICATION, + LogChannels::NOTIFICATIONS, [ 'placeholder' => $sPlaceholderAttCode, 'replace' => $replace,