From 57016420a7a78b88b50ffc675f7bea7701860fc5 Mon Sep 17 00:00:00 2001 From: Dylan Wagstaff Date: Fri, 25 May 2018 16:06:25 +1200 Subject: [PATCH] API refactor module to be more independent As part of a greater piece of work with site summaries (cf. bringyourownideas/silverstripe-maintenance). This commit tidies class definitions and makes the import slightly more efficient. Abstractions are added to relate to the Package object from the aforementioned module. Unit tests have been added to ensure tidy standards in future maintenance. --- _config/packagerelations.yml | 15 +++ code/jobs/CheckComposerSecurityJob.php | 60 --------- code/models/ComposerSecurityVulnerability.php | 44 ------- code/tasks/CheckComposerSecurityTask.php | 102 --------------- composer.json | 8 +- phpcs.xml.dist | 10 ++ src/Extensions/CVEExtension.php | 8 ++ src/Extensions/PackageSecruityExtension.php | 25 ++++ src/Extensions/SiteSummaryExtension.php | 15 +++ src/Jobs/CVECheckJob.php | 61 +++++++++ src/Models/CVE.php | 20 +++ src/Tasks/CVECheckTask.php | 118 ++++++++++++++++++ templates/PackageSecurityAlerts.ss | 13 ++ tests/.gitkeep | 0 tests/CVECheckJobTest.php | 15 +++ tests/CVECheckTaskTest.php | 91 ++++++++++++++ 16 files changed, 395 insertions(+), 210 deletions(-) create mode 100644 _config/packagerelations.yml delete mode 100644 code/jobs/CheckComposerSecurityJob.php delete mode 100644 code/models/ComposerSecurityVulnerability.php delete mode 100644 code/tasks/CheckComposerSecurityTask.php create mode 100644 phpcs.xml.dist create mode 100644 src/Extensions/CVEExtension.php create mode 100644 src/Extensions/PackageSecruityExtension.php create mode 100644 src/Extensions/SiteSummaryExtension.php create mode 100644 src/Jobs/CVECheckJob.php create mode 100644 src/Models/CVE.php create mode 100644 src/Tasks/CVECheckTask.php create mode 100644 templates/PackageSecurityAlerts.ss delete mode 100644 tests/.gitkeep create mode 100644 tests/CVECheckJobTest.php create mode 100644 tests/CVECheckTaskTest.php diff --git a/_config/packagerelations.yml b/_config/packagerelations.yml new file mode 100644 index 0000000..93e8c73 --- /dev/null +++ b/_config/packagerelations.yml @@ -0,0 +1,15 @@ +--- +Name: packagerelations +after: '#updatecheckerextensions' +Only: + moduleexists: silverstripe-maintenance +--- +Package: + extensions: + - PackageSecurityExtension +CVE: + extensions: + - CVEExtension +SiteSummary: + extensions: + - SiteSummaryExtension diff --git a/code/jobs/CheckComposerSecurityJob.php b/code/jobs/CheckComposerSecurityJob.php deleted file mode 100644 index c5778e9..0000000 --- a/code/jobs/CheckComposerSecurityJob.php +++ /dev/null @@ -1,60 +0,0 @@ -totalSteps = 1; - - return QueuedJob::QUEUED; - } - - /** - * init - */ - public function setup() - { - // create the instance of the task - $this->task = new CheckComposerSecurityTask(); - } - - /** - * processes the task as a job - */ - public function process() - { - // run the task - $this->task->run(new SS_HTTPRequest('GET', '/')); - - // mark job as completed - $this->isComplete = true; - } -} diff --git a/code/models/ComposerSecurityVulnerability.php b/code/models/ComposerSecurityVulnerability.php deleted file mode 100644 index a087c17..0000000 --- a/code/models/ComposerSecurityVulnerability.php +++ /dev/null @@ -1,44 +0,0 @@ - 'Varchar(255)', - 'Version' => 'Varchar(255)', - 'Title' => 'Varchar(255)', - 'ExternalLink' => 'Varchar(255)', - 'CVE' => 'Varchar(255)', - ); - - /** - * @var array - */ - private static $summary_fields = array( - 'Package', - 'Version', - 'Title', - ); - - /** - * name of the related job - * - * @var string - */ - public $jobName = 'CheckComposerSecurityJob'; - - /** - * self update on dev/build - */ - public function requireDefaultRecords() - { - parent::requireDefaultRecords(); - - // add a queuedjob on dev/build - singleton('QueuedJobService')->queueJob(new CheckComposerSecurityJob()); - } -} diff --git a/code/tasks/CheckComposerSecurityTask.php b/code/tasks/CheckComposerSecurityTask.php deleted file mode 100644 index f8ab150..0000000 --- a/code/tasks/CheckComposerSecurityTask.php +++ /dev/null @@ -1,102 +0,0 @@ -check($this->getPathToComposerlock()); - - // Are there any issues known? If so save the information in the database - if (is_array($alerts) && !empty($alerts)) { - // go through all alerts for packages - each can contain multiple issues - foreach ($alerts as $package => $packageDetails) { - // go through each individual known security issue - foreach ($packageDetails['advisories'] as $details) { - // check if this vulnerability is already known - $vulnerability = ComposerSecurityVulnerability::get()->filter(array( - 'Package' => $package, - 'Version' => $packageDetails['version'], - 'Title' => $details['title'], - )); - - // Is this vulnerability known? No, lets add it. - if ((int) $vulnerability->count() === 0) { - $vulnerability = new ComposerSecurityVulnerability(); - $vulnerability->Package = $package; - $vulnerability->Version = $packageDetails['version']; - $vulnerability->Title = $details['title']; - $vulnerability->ExternalLink = $details['link']; - $vulnerability->CVE = $details['cve']; - $vulnerability->write(); - - // add the new entries to the list of the remaining entries - $remainingEntries[] = $vulnerability->ID; - } else { - // add all matching known vulnerabilities - this way we can keep those. - $remainingEntries = array_merge($remainingEntries, $vulnerability->column('ID')); - } - } - } - } - - // remove all entries which are resolved (not in $remainingEntries) - foreach (ComposerSecurityVulnerability::get()->exclude(array('ID' => $remainingEntries)) as $vulnerability) { - $vulnerability->delete(); - } - - // notify that the task finished. - $this->message('The task finished running. You can find the updated information in the database now.'); - } - - /** - * @var boolean - */ - protected function isCLI() - { - return (PHP_SAPI === 'cli'); - } - - /** - * @return string - */ - protected function getPathToComposerlock() - { - return (($this->isCLI()) ? '..' : $_SERVER['DOCUMENT_ROOT']) . '/composer.lock'; - } - - /** - * prints a message during the run of the task - * - * @param string $text - */ - protected function message($text) - { - if (!$this->isCLI()) { - $text = '

' . $text . '

' . PHP_EOL; - } - - echo $text . PHP_EOL; - } -} diff --git a/composer.json b/composer.json index 1d49f6f..db95b4a 100644 --- a/composer.json +++ b/composer.json @@ -19,15 +19,15 @@ ], "require": { "php": ">=5.6.0", - "silverstripe/framework": "^3.0", - "sensiolabs/security-checker": "^3.0", - "symbiote/silverstripe-queuedjobs": "^2.8", - "friendsofsilverstripe/silverstripe-maintenance": "^0.3" + "silverstripe/framework": "^3", + "sensiolabs/security-checker": "^4", + "symbiote/silverstripe-queuedjobs": "^2" }, "replace": { "spekulatius/silverstripe-composer-security-checker": "self.version" }, "suggest": { + "friendsofsilverstripe/silverstripe-maintenance": "Display advisory information via Site Summary Report", "roave/security-advisories": "Raises a conflict on attempt to install a vulnerable module" }, "support": { diff --git a/phpcs.xml.dist b/phpcs.xml.dist new file mode 100644 index 0000000..6f34b9b --- /dev/null +++ b/phpcs.xml.dist @@ -0,0 +1,10 @@ + + + CodeSniffer ruleset for SilverStripe coding conventions. + + + + + + + diff --git a/src/Extensions/CVEExtension.php b/src/Extensions/CVEExtension.php new file mode 100644 index 0000000..d3a708f --- /dev/null +++ b/src/Extensions/CVEExtension.php @@ -0,0 +1,8 @@ + 'Package' + ]; +} diff --git a/src/Extensions/PackageSecruityExtension.php b/src/Extensions/PackageSecruityExtension.php new file mode 100644 index 0000000..78712bc --- /dev/null +++ b/src/Extensions/PackageSecruityExtension.php @@ -0,0 +1,25 @@ + 'CVE' + ]; + + private static $summary_fields = [ + 'listCVEs' => 'Security alerts', + ]; + + public function listCVEs() + { + $alerts = $this->owner->SecurityAlerts()->Column('CVE'); + return $alerts ? implode(', ', $alerts) : null; + } + + public function listAlerts() + { + $templates = ['PackageSecurityAlerts']; + $this->owner->extend('updateListAlerts', $templates); + return $this->owner->renderWith($templates); + } +} diff --git a/src/Extensions/SiteSummaryExtension.php b/src/Extensions/SiteSummaryExtension.php new file mode 100644 index 0000000..9224300 --- /dev/null +++ b/src/Extensions/SiteSummaryExtension.php @@ -0,0 +1,15 @@ +owner->sourceRecords()->renderWith('PackageSecurityAlerts'); + $fields->unshift(LiteralField::create('AlertSummary', $summaryInfo)); + } +} diff --git a/src/Jobs/CVECheckJob.php b/src/Jobs/CVECheckJob.php new file mode 100644 index 0000000..bb8b0e1 --- /dev/null +++ b/src/Jobs/CVECheckJob.php @@ -0,0 +1,61 @@ + '%$' . CVECheckTask::class, + ]; + + /** + * @var CVECheckTask + */ + protected $checkTask; + + /** + * @return CVECheckTask + */ + public function getCVECheckTask() + { + return $this->checkTask; + } + + /** + * @param CVECheckTask + * @return CVECheckJob + */ + public function setCVECheckTask($checkTask) + { + $this->checkTask = $checkTask; + return $this; + } + + public function getTitle() + { + return _t( + __CLASS__ . '.Title', + 'Check if any composer managed modules have known security vulnerabilities.' + ); + } + + public function getJobType() + { + $this->totalSteps = 1; + + return QueuedJob::QUEUED; + } + + public function process() + { + // run the task + $task = $this->getCVECheckTask(); + $task->run(null); + + // mark job as completed + $this->isComplete = true; + } +} diff --git a/src/Models/CVE.php b/src/Models/CVE.php new file mode 100644 index 0000000..7a0bf56 --- /dev/null +++ b/src/Models/CVE.php @@ -0,0 +1,20 @@ + 'Varchar(255)', + 'Version' => 'Varchar(255)', + 'Title' => 'Varchar(255)', + 'ExternalLink' => 'Varchar(255)', + 'CVE' => 'Varchar(255)', + ); + + private static $summary_fields = array( + 'PackageName', + 'Version', + 'Title', + ); +} diff --git a/src/Tasks/CVECheckTask.php b/src/Tasks/CVECheckTask.php new file mode 100644 index 0000000..0e95b45 --- /dev/null +++ b/src/Tasks/CVECheckTask.php @@ -0,0 +1,118 @@ + '%$' . SecurityChecker::class, + ]; + + protected $title = 'Composer security checker'; + + protected $description = + 'Checks if any modules managed through composer have known security vulnerabilities at the used version.'; + + /** + * @return SecurityChecker + */ + public function getSecurityChecker() + { + return $this->securityChecker; + } + + /** + * @param SecurityChecker $securityChecker + * @return CVECheckTask $this + */ + public function setSecurityChecker($securityChecker) + { + $this->securityChecker = $securityChecker; + return $this; + } + + public function run($request) + { + // to keep the list up to date while removing resolved issues we keep all of found issues + $validEntries = array(); + + // use the security checker of + $checker = $this->getSecurityChecker(); + $alerts = $checker->check(BASE_PATH . DIRECTORY_SEPARATOR . 'composer.lock'); + + // go through all alerts for packages - each can contain multiple issues + foreach ($alerts as $package => $packageDetails) { + // go through each individual known security issue + foreach ($packageDetails['advisories'] as $details) { + // check if this vulnerability is already known + $vulnerability = CVE::get()->filter(array( + 'PackageName' => $package, + 'Version' => $packageDetails['version'], + 'CVE' => $details['cve'], + )); + + // Is this vulnerability known? No, lets add it. + if ((int) $vulnerability->count() === 0) { + $vulnerability = new CVE(); + $vulnerability->PackageName = $package; + $vulnerability->Version = $packageDetails['version']; + $vulnerability->Title = $details['title']; + $vulnerability->ExternalLink = $details['link']; + $vulnerability->CVE = $details['cve']; + + $vulnerability->write(); + + // add the new entries to the list of valid entries + $validEntries[] = $vulnerability->ID; + } else { + // add existing vulnerabilities (probably just 1) to the list of valid entries + $validEntries = array_merge($validEntries, $vulnerability->column('ID')); + } + + if ($vulnerability->hasExtension(CVEExtension::class) && + $vulnerability->PackageRecordID === 0 && + $packageRecord = Package::get()->find('Name', $package) + ) { + $vulnerability->PackageRecordID = $packageRecord->ID; + } + } + } + + // remove all entries which are resolved (no longer $validEntries) + $removeOldCVEs = SQLDelete::create('CVE'); + if (empty($validEntries)) { + // There were no CVEs listed for our installation + $removeOldCVEs->execute(); + } else { + $removable = CVE::get()->exclude(array('ID' => $validEntries)); + if ($removable->exists()) { + // Be careful not to remove all CVEs on the case that entry is valid + $removeOldCVEs = $removeOldCVEs->addWhere('ID', $removable->column('ID')); + $removeOldCVEs->execute(); + } + } + + // notify that the task finished. + $this->output('The task finished running. You can find the updated information in the database now.'); + } + + /** + * prints a message during the run of the task + * + * @param string $text + */ + protected function output($text) + { + if (!SapphireTest::is_running_test()) { + echo Director::is_cli() ? $text . PHP_EOL : "

$text

\n"; + } + } +} diff --git a/templates/PackageSecurityAlerts.ss b/templates/PackageSecurityAlerts.ss new file mode 100644 index 0000000..170e708 --- /dev/null +++ b/templates/PackageSecurityAlerts.ss @@ -0,0 +1,13 @@ +<% loop $Me %> + <% if $SecurityAlerts %> +
+ $Name +
+ <% loop $SecurityAlerts %> +
$CVE
+
$Title
+ <% end_loop %> +
+
+ <% end_if %> +<% end_loop %> diff --git a/tests/.gitkeep b/tests/.gitkeep deleted file mode 100644 index e69de29..0000000 diff --git a/tests/CVECheckJobTest.php b/tests/CVECheckJobTest.php new file mode 100644 index 0000000..4c69701 --- /dev/null +++ b/tests/CVECheckJobTest.php @@ -0,0 +1,15 @@ +getMockBuilder(CVECheckTask::class)->setMethods(['run'])->getMock(); + $spy->expects($this->once())->method('run'); + + $job = new CVECheckJob; + $job->setCVECheckTask($spy); + + $job->process(); + } +} diff --git a/tests/CVECheckTaskTest.php b/tests/CVECheckTaskTest.php new file mode 100644 index 0000000..f73a10f --- /dev/null +++ b/tests/CVECheckTaskTest.php @@ -0,0 +1,91 @@ +getMockBuilder(SecurityChecker::class)->setMethods(['check'])->getMock(); + $securityCheckerMock->expects($this->any())->method('check')->will($this->returnValue( + $empty ? [] : json_decode($mockOutput, true) + )); + + return $securityCheckerMock; + } + + public function testUpdatesAreSaved() + { + $securityCheckerMock = $this->getSecurityCheckerMock(); + $checkTask = new CVECheckTask; + $checkTask->setSecurityChecker($securityCheckerMock); + + $preCheck = CVE::get(); + $this->assertEquals(0, $preCheck->count(), 'database is empty to begin with'); + + $checkTask->run(null); + + $postCheck = CVE::get(); + $this->assertEquals(1, $postCheck->count(), 'CVE has been stored'); + } + + public function testNoDuplicates() + { + $securityCheckerMock = $this->getSecurityCheckerMock(); + $checkTask = new CVECheckTask; + $checkTask->setSecurityChecker($securityCheckerMock); + + $preCheck = CVE::get(); + $this->assertEquals(0, $preCheck->count(), 'database is empty to begin with'); + + $checkTask->run(null); + + $postCheck = CVE::get(); + $this->assertEquals(1, $postCheck->count(), 'CVE has been stored'); + + $checkTask->run(null); + + $postCheck = CVE::get(); + $this->assertEquals(1, $postCheck->count(), 'The CVE isn\'t stored twice.'); + } + + public function testCVERemovals() + { + $securityCheckerMock = $this->getSecurityCheckerMock(); + $checkTask = new CVECheckTask; + $checkTask->setSecurityChecker($securityCheckerMock); + + $checkTask->run(null); + + $preCheck = CVE::get(); + $this->assertEquals(1, $preCheck->count(), 'database has stored CVEs'); + + $securityCheckerMock = $this->getSecurityCheckerMock(true); + $checkTask->setSecurityChecker($securityCheckerMock); + + $checkTask->run(null); + + $postCheck = CVE::get(); + $this->assertEquals(0, $postCheck->count(), 'database is empty to finish with'); + } +}