From 2dc32c25761ffb52feb40f774f6fa4541d1eb6bf Mon Sep 17 00:00:00 2001 From: Jean-Christophe Fillion-Robin Date: Mon, 8 May 2017 15:45:12 -0400 Subject: [PATCH 1/2] BUG: Validate parameter id to prevent XSS --- core/controllers/BrowseController.php | 12 ++++++++++++ core/controllers/FolderController.php | 12 ++++++++++++ core/controllers/ShareController.php | 5 +++++ modules/packages/controllers/PackageController.php | 6 ++++++ modules/scheduler/controllers/RunController.php | 6 ++++++ 5 files changed, 41 insertions(+) diff --git a/core/controllers/BrowseController.php b/core/controllers/BrowseController.php index d4a93c0b7..a15fe1ba4 100644 --- a/core/controllers/BrowseController.php +++ b/core/controllers/BrowseController.php @@ -415,6 +415,12 @@ public function getelementinfoAction() if (!isset($id) || !isset($element)) { throw new Zend_Exception('Please double check the parameters'); } + + $validator = new Zend_Validate_Digits(); + if (!$validator->isValid($id)) { + throw new Zend_Exception('Must specify an id parameter'); + } + $jsonContent = array('type' => $element); switch ($element) { case 'community': @@ -502,6 +508,12 @@ public function getmaxpolicyAction() if (!isset($id) || !isset($type)) { throw new Zend_Exception('Must pass id and type parameters'); } + + $validator = new Zend_Validate_Digits(); + if (!$validator->isValid($id)) { + throw new Zend_Exception('Must specify an id parameter'); + } + switch (strtolower($type)) { case 'folder': $maxpolicy = $this->Folder->getMaxPolicy($id, $this->userSession->Dao); diff --git a/core/controllers/FolderController.php b/core/controllers/FolderController.php index c49447cbb..d7f7f66f2 100644 --- a/core/controllers/FolderController.php +++ b/core/controllers/FolderController.php @@ -48,6 +48,12 @@ public function getnameAction() if (!isset($folderId)) { throw new Zend_Exception('Must pass id parameter'); } + + $validator = new Zend_Validate_Digits(); + if (!$validator->isValid($folderId)) { + throw new Zend_Exception('Must specify an id parameter'); + } + $folder = $this->Folder->load($folderId); if (!$folder) { throw new Zend_Exception('Invalid folderId', 404); @@ -69,6 +75,12 @@ public function javachildrenAction() if (!isset($folderId)) { throw new Zend_Exception('Must pass id parameter'); } + + $validator = new Zend_Validate_Digits(); + if (!$validator->isValid($folderId)) { + throw new Zend_Exception('Must specify an id parameter'); + } + $folder = $this->Folder->load($folderId); if (!$folder) { throw new Zend_Exception('Invalid folderId', 404); diff --git a/core/controllers/ShareController.php b/core/controllers/ShareController.php index edca1de82..dc9cfef11 100644 --- a/core/controllers/ShareController.php +++ b/core/controllers/ShareController.php @@ -297,6 +297,11 @@ public function linksAction() $type = $this->getParam('type'); $id = $this->getParam('id'); + $validator = new Zend_Validate_Digits(); + if (!$validator->isValid($id)) { + throw new Zend_Exception('Must specify an id parameter'); + } + switch ($type) { case 'folder': $dao = $this->Folder->load($id); diff --git a/modules/packages/controllers/PackageController.php b/modules/packages/controllers/PackageController.php index 8b2a0f23b..4ee407810 100644 --- a/modules/packages/controllers/PackageController.php +++ b/modules/packages/controllers/PackageController.php @@ -34,6 +34,12 @@ public function manageAction() if (!isset($packageId)) { throw new Zend_Exception('Must specify an id parameter'); } + + $validator = new Zend_Validate_Digits(); + if (!$validator->isValid($packageId)) { + throw new Zend_Exception('Must specify an id parameter'); + } + $package = $this->Packages_Package->load($packageId); if (!$package) { throw new Zend_Exception('Invalid package id'); diff --git a/modules/scheduler/controllers/RunController.php b/modules/scheduler/controllers/RunController.php index 6094cf3bd..828c2023c 100644 --- a/modules/scheduler/controllers/RunController.php +++ b/modules/scheduler/controllers/RunController.php @@ -43,6 +43,12 @@ public function indexAction() $this->Setting->setConfig('lastrun', $startTime, $this->moduleName); $id = $this->getParam('id'); + + $validator = new Zend_Validate_Digits(); + if (!$validator->isValid($id)) { + throw new Zend_Exception('Must specify an id parameter'); + } + if (isset($id)) { $jobs = $this->Scheduler_Job->load($id); if ($jobs == false) { From 771d2201626b2475533a418a2112f63ec7937e53 Mon Sep 17 00:00:00 2001 From: mgrauer Date: Mon, 8 May 2017 19:51:22 +0000 Subject: [PATCH 2/2] Apply fixes from StyleCI --- core/controllers/ImportController.php | 1 - core/controllers/components/ApihelperComponent.php | 1 - .../controllers/components/DownloadBitstreamComponent.php | 2 +- core/database/upgrade/3.2.6.php | 1 - core/models/base/ItemModelBase.php | 1 - core/tests/models/base/FeedModelTest.php | 8 ++++---- modules/pvw/controllers/components/ParaviewComponent.php | 1 - modules/pvw/models/pdo/InstanceModel.php | 2 +- modules/scheduler/models/pdo/JobModel.php | 1 - modules/thumbnailcreator/database/upgrade/1.0.2.php | 1 - modules/tracker/controllers/ProducerController.php | 1 - modules/tracker/models/base/ScalarModelBase.php | 1 - modules/tracker/models/base/TrendModelBase.php | 1 - 13 files changed, 6 insertions(+), 16 deletions(-) diff --git a/core/controllers/ImportController.php b/core/controllers/ImportController.php index 1c6433c3a..5f0b91f1b 100644 --- a/core/controllers/ImportController.php +++ b/core/controllers/ImportController.php @@ -143,7 +143,6 @@ private function _recursiveParseDirectory($path, $currentdir) } if ($fileInfo->isDir()) { // we have a directory - // If the the directory actually doesn't exist at this point, // skip it. if (!file_exists($fileInfo->getPathName())) { diff --git a/core/controllers/components/ApihelperComponent.php b/core/controllers/components/ApihelperComponent.php index befb0f516..4d74c7bcf 100644 --- a/core/controllers/components/ApihelperComponent.php +++ b/core/controllers/components/ApihelperComponent.php @@ -224,7 +224,6 @@ public function setMetadata($item, $type, $element, $qualifier, $value, $revisio ); foreach ($modules as $retval) { if ($retval['status'] === true) { // module has handled the event, so we don't have to - return; } } diff --git a/core/controllers/components/DownloadBitstreamComponent.php b/core/controllers/components/DownloadBitstreamComponent.php index 459f72bc8..df8edf4c2 100644 --- a/core/controllers/components/DownloadBitstreamComponent.php +++ b/core/controllers/components/DownloadBitstreamComponent.php @@ -159,7 +159,7 @@ public function download($bitstream, $offset = 0, $incrementDownload = false) * * @param string $key Environment variable name * @return string Environment variable setting - * @link http://book.cakephp.org/view/1130/env + * @see http://book.cakephp.org/view/1130/env */ function env($key) { diff --git a/core/database/upgrade/3.2.6.php b/core/database/upgrade/3.2.6.php index 14996ac34..49b616971 100644 --- a/core/database/upgrade/3.2.6.php +++ b/core/database/upgrade/3.2.6.php @@ -83,7 +83,6 @@ private function _moveThumbnailToAssetstore($thumbnail) $oldpath = BASE_PATH.'/'.$thumbnail; if (!file_exists($oldpath)) { //thumbnail file no longer exists, so we remove its reference - return; } diff --git a/core/models/base/ItemModelBase.php b/core/models/base/ItemModelBase.php index 71be5d2c2..c7589e29b 100644 --- a/core/models/base/ItemModelBase.php +++ b/core/models/base/ItemModelBase.php @@ -143,7 +143,6 @@ public function copyItemPolicies($itemdao, $referenceItemdao, $feeddao = null) } if ($feeddao != null && $feeddao instanceof FeedDao) { - /** @var FeedpolicygroupModel $FeedpolicygroupModel */ $FeedpolicygroupModel = MidasLoader::loadModel('Feedpolicygroup'); foreach ($groupPolicies as $key => $policy) { diff --git a/core/tests/models/base/FeedModelTest.php b/core/tests/models/base/FeedModelTest.php index 22976e277..a52df67f8 100644 --- a/core/tests/models/base/FeedModelTest.php +++ b/core/tests/models/base/FeedModelTest.php @@ -73,13 +73,13 @@ public function testCreateFeed() $itemFile = $this->loadData('Item', 'default'); $feed = $this->Feed->createFeed($usersFile[0], MIDAS_FEED_CREATE_COMMUNITY, $communityFile[0]); $this->assertEquals(true, $feed->saved); - $feed = $this->Feed->createFeed($usersFile [0], MIDAS_FEED_CREATE_FOLDER, $folderFile[0]); + $feed = $this->Feed->createFeed($usersFile[0], MIDAS_FEED_CREATE_FOLDER, $folderFile[0]); $this->assertEquals(true, $feed->saved); - $feed = $this->Feed->createFeed($usersFile [0], MIDAS_FEED_CREATE_ITEM, $itemFile[0]); + $feed = $this->Feed->createFeed($usersFile[0], MIDAS_FEED_CREATE_ITEM, $itemFile[0]); $this->assertEquals(true, $feed->saved); - $feed = $this->Feed->createFeed($usersFile [0], MIDAS_FEED_CREATE_USER, $usersFile[0]); + $feed = $this->Feed->createFeed($usersFile[0], MIDAS_FEED_CREATE_USER, $usersFile[0]); $this->assertEquals(true, $feed->saved); - $this->Feed->addCommunity($feed, $communityFile [0]); + $this->Feed->addCommunity($feed, $communityFile[0]); $communities = $feed->getCommunities(); if (($communities[0]->getKey() != $communityFile[0]->getKey())) { $this->fail('Unable to add dao'); diff --git a/modules/pvw/controllers/components/ParaviewComponent.php b/modules/pvw/controllers/components/ParaviewComponent.php index f27c1be8c..ea2050d90 100644 --- a/modules/pvw/controllers/components/ParaviewComponent.php +++ b/modules/pvw/controllers/components/ParaviewComponent.php @@ -208,7 +208,6 @@ private function _getNextOpenPort() } } elseif (!UtilityComponent::isPortListening($portEntry) ) { // single port check - return $portEntry; } } diff --git a/modules/pvw/models/pdo/InstanceModel.php b/modules/pvw/models/pdo/InstanceModel.php index 2df090e15..34583f412 100644 --- a/modules/pvw/models/pdo/InstanceModel.php +++ b/modules/pvw/models/pdo/InstanceModel.php @@ -31,7 +31,7 @@ public function getAll() $rows = $this->database->fetchAll($this->database->select()->setIntegrityCheck(false)); $daos = array(); foreach ($rows as $row) { - $daos [] = $this->initDao('Instance', $row, $this->moduleName); + $daos[] = $this->initDao('Instance', $row, $this->moduleName); } return $daos; diff --git a/modules/scheduler/models/pdo/JobModel.php b/modules/scheduler/models/pdo/JobModel.php index d78f4b7d1..fe279b0da 100644 --- a/modules/scheduler/models/pdo/JobModel.php +++ b/modules/scheduler/models/pdo/JobModel.php @@ -77,7 +77,6 @@ public function getJobsToRun($limit = 1000) $minPriority = MIDAS_EVENT_PRIORITY_LOW; if (!empty($load)) { if ($load[0] > 80 || $load[1] > 80) { // don't run anything - return array(); } $minPriority = MIDAS_EVENT_PRIORITY_HIGH; diff --git a/modules/thumbnailcreator/database/upgrade/1.0.2.php b/modules/thumbnailcreator/database/upgrade/1.0.2.php index 120789f1f..72f0104b8 100644 --- a/modules/thumbnailcreator/database/upgrade/1.0.2.php +++ b/modules/thumbnailcreator/database/upgrade/1.0.2.php @@ -75,7 +75,6 @@ private function _moveThumbnailToAssetstore($thumbnail) $oldpath = BASE_PATH.'/'.$thumbnail; if (!file_exists($oldpath)) { //thumbnail file no longer exists, so we remove its reference - return; } diff --git a/modules/tracker/controllers/ProducerController.php b/modules/tracker/controllers/ProducerController.php index 1400bb42b..65f56bf25 100644 --- a/modules/tracker/controllers/ProducerController.php +++ b/modules/tracker/controllers/ProducerController.php @@ -133,7 +133,6 @@ public function viewAction() 'text' => $producerDao->getDisplayName(), 'icon' => $this->view->baseUrl('core/public/images/icons/cog_go.png'), ), - ); $this->Component->Breadcrumb->setBreadcrumbHeader($breadcrumbs, $this->view); } diff --git a/modules/tracker/models/base/ScalarModelBase.php b/modules/tracker/models/base/ScalarModelBase.php index 1f93098a3..2a68ff1ad 100644 --- a/modules/tracker/models/base/ScalarModelBase.php +++ b/modules/tracker/models/base/ScalarModelBase.php @@ -89,7 +89,6 @@ abstract public function getOtherScalarsFromSubmission($scalarDao); */ public function addToTrend($trendDao, $submissionDao, $value) { - /** @var Tracker_ScalarDao $scalarDao */ $scalarDao = MidasLoader::newDao('ScalarDao', $this->moduleName); diff --git a/modules/tracker/models/base/TrendModelBase.php b/modules/tracker/models/base/TrendModelBase.php index 717280afa..ac8556414 100644 --- a/modules/tracker/models/base/TrendModelBase.php +++ b/modules/tracker/models/base/TrendModelBase.php @@ -134,7 +134,6 @@ public function createIfNeeded($producerId, $metricName, $configItemId, $testDat $trendDao = $this->getMatch($producerId, $metricName, $configItemId, $testDatasetId, $truthDatasetId); if ($trendDao === false) { - /** @var Tracker_TrendGroupModel $trendGroupModel */ $trendGroupModel = MidasLoader::loadModel('Trendgroup', $this->moduleName);