From 857de7da3b00a4e8cee1dc9f5369b5f9641ab010 Mon Sep 17 00:00:00 2001 From: Dylan Wagstaff Date: Tue, 29 May 2018 16:29:19 +1200 Subject: [PATCH] Add more display output for security alerts Show the information in summary to users looking at the SiteSummary report on screen. This includes the ability to see a tag notifying which modules have alerts issued for them, a form level warning alerting users that some modules have alerts (as they may not display on the first page), and the ability to expand an individual module's information to display the alerts that have been issued (in a summary of title, ID number and externa link) --- .editorconfig | 7 +++++ .travis.yml | 2 -- composer.json | 4 +-- css/securityalerts.css | 7 +++++ javascript/summaryalerts.js | 29 +++++++++++++++++++ src/Extensions/PackageSecruityExtension.php | 32 +++++++++++++++++++++ src/Extensions/SiteSummaryExtension.php | 23 +++++++++++++-- src/Jobs/CVECheckJob.php | 8 +++--- src/Tasks/CVECheckTask.php | 12 ++++---- templates/PackageSecurityAlerts.ss | 24 +++++++--------- templates/SecurityAlertSummary.ss | 4 +++ tests/CVECheckJobTest.php | 2 +- tests/CVECheckTaskTest.php | 14 ++++----- 13 files changed, 130 insertions(+), 38 deletions(-) create mode 100644 css/securityalerts.css create mode 100644 javascript/summaryalerts.js create mode 100644 templates/SecurityAlertSummary.ss diff --git a/.editorconfig b/.editorconfig index 47ae637..77cadaa 100644 --- a/.editorconfig +++ b/.editorconfig @@ -10,6 +10,13 @@ indent_style = space insert_final_newline = true trim_trailing_whitespace = true +[*.md] +trim_trailing_whitespace = false + +[*{.yml,.js}] +indent_size = 2 +indent_style = space + [{*.yml,package.json}] indent_size = 2 diff --git a/.travis.yml b/.travis.yml index 4861d7b..89e6d35 100644 --- a/.travis.yml +++ b/.travis.yml @@ -15,8 +15,6 @@ matrix: include: - php: 5.6 env: DB=MYSQL CORE_RELEASE=3 - - php: 5.6 - env: DB=MYSQL CORE_RELEASE=3.1 - php: 5.6 env: DB=PGSQL CORE_RELEASE=3.2 - php: 5.6 diff --git a/composer.json b/composer.json index db95b4a..9b44349 100644 --- a/composer.json +++ b/composer.json @@ -19,7 +19,7 @@ ], "require": { "php": ">=5.6.0", - "silverstripe/framework": "^3", + "silverstripe/framework": "^3.2", "sensiolabs/security-checker": "^4", "symbiote/silverstripe-queuedjobs": "^2" }, @@ -27,7 +27,7 @@ "spekulatius/silverstripe-composer-security-checker": "self.version" }, "suggest": { - "friendsofsilverstripe/silverstripe-maintenance": "Display advisory information via Site Summary Report", + "bringyourownideas/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/css/securityalerts.css b/css/securityalerts.css new file mode 100644 index 0000000..aa40e9d --- /dev/null +++ b/css/securityalerts.css @@ -0,0 +1,7 @@ +.package-summary__security-alerts { + margin-top: 8px; +} + +.security-alerts__list { + display: none; +} diff --git a/javascript/summaryalerts.js b/javascript/summaryalerts.js new file mode 100644 index 0000000..1d830f7 --- /dev/null +++ b/javascript/summaryalerts.js @@ -0,0 +1,29 @@ +(function($) { + $.entwine('ss', function($) { + $('.package-summary__security-alerts').entwine({ + IsShown: false, + onclick: function(event) { + if ($(event.target).is('strong, strong>span')) { + this.toggleSecurityNotices(); + } + }, + toggleSecurityNotices: function() { + if (this.getIsShown()) { + this.hideSecurityNotices(); + } else { + this.showSecurityNotices(); + } + }, + showSecurityNotices: function() { + this.children('dl').show(); + this.find('strong>span').text('Hide'); + this.setIsShown(true); + }, + hideSecurityNotices: function() { + this.children('dl').hide(); + this.find('strong>span').text('Show'); + this.setIsShown(false); + } + }); + }); +})(jQuery) diff --git a/src/Extensions/PackageSecruityExtension.php b/src/Extensions/PackageSecruityExtension.php index 78712bc..55493fc 100644 --- a/src/Extensions/PackageSecruityExtension.php +++ b/src/Extensions/PackageSecruityExtension.php @@ -10,16 +10,48 @@ class PackageSecurityExtension extends DataExtension 'listCVEs' => 'Security alerts', ]; + /** + * Simply returns a comma separated list of active CVE numbers for this record. + * Used in CSV exports as a type of brief indication (as opposed to full info) + */ public function listCVEs() { $alerts = $this->owner->SecurityAlerts()->Column('CVE'); return $alerts ? implode(', ', $alerts) : null; } + /** + * Renders the SecurityAlerts relationship + * intended for use in the on screen version of the SiteSummary Report. + */ public function listAlerts() { $templates = ['PackageSecurityAlerts']; $this->owner->extend('updateListAlerts', $templates); return $this->owner->renderWith($templates); } + + /** + * updates the badges that render as part of the screen targeted + * summary for this Package + * + * @param array $badges in the format of [title => type] + */ + public function updateBadges(&$badges) + { + if ($this->owner->SecurityAlerts()->exists()) { + $badges['RISK: Security'] = 'warning'; + } + } + + /** + * Appends our own summary info to that of the default output + * of the Package getSummary method. + * + * @param HTMLText $summary + */ + public function updateSummary(&$summary) + { + $summary->setValue($summary . $this->listAlerts()); + } } diff --git a/src/Extensions/SiteSummaryExtension.php b/src/Extensions/SiteSummaryExtension.php index 9224300..7aac9b3 100644 --- a/src/Extensions/SiteSummaryExtension.php +++ b/src/Extensions/SiteSummaryExtension.php @@ -2,14 +2,31 @@ class SiteSummaryExtension extends Extension { + /** + * Update screen bound report columns to remove the text (csv) column + * listing CVE numbers, and include the view assets to render appropriately + * + * @param array $columns Report display columns + */ public function updateColumns(&$columns) { + Requirements::css('silverstripe-composer-security-checker/css/securityalerts.css'); + Requirements::javascript('silverstripe-composer-security-checker/javascript/summaryalerts.js'); unset($columns['listCVEs']); } - public function updateCMSFields(&$fields) + /** + * Update the Package's screen bound summary info with little badges to indicate + * security alerts are present for this package + * + * @param array $alerts a list of alerts to display + */ + public function updateAlerts(&$alerts) { - $summaryInfo = $this->owner->sourceRecords()->renderWith('PackageSecurityAlerts'); - $fields->unshift(LiteralField::create('AlertSummary', $summaryInfo)); + $securityWarnings = $this->owner->sourceRecords()->filter('SecurityAlerts.ID:GreaterThan', 0); + + if ($securityWarnings->exists()) { + $alerts['SecurityAlerts'] = $securityWarnings->renderWith('SecurityAlertSummary'); + } } } diff --git a/src/Jobs/CVECheckJob.php b/src/Jobs/CVECheckJob.php index bb8b0e1..d6c8492 100644 --- a/src/Jobs/CVECheckJob.php +++ b/src/Jobs/CVECheckJob.php @@ -19,16 +19,16 @@ class CVECheckJob extends AbstractQueuedJob implements QueuedJob /** * @return CVECheckTask */ - public function getCVECheckTask() + public function getCheckTask() { return $this->checkTask; } /** - * @param CVECheckTask + * @param CVECheckTask $checkTask * @return CVECheckJob */ - public function setCVECheckTask($checkTask) + public function setCheckTask(CVECheckTask $checkTask) { $this->checkTask = $checkTask; return $this; @@ -52,7 +52,7 @@ public function getJobType() public function process() { // run the task - $task = $this->getCVECheckTask(); + $task = $this->getCheckTask(); $task->run(null); // mark job as completed diff --git a/src/Tasks/CVECheckTask.php b/src/Tasks/CVECheckTask.php index 0e95b45..defdd5d 100644 --- a/src/Tasks/CVECheckTask.php +++ b/src/Tasks/CVECheckTask.php @@ -31,9 +31,9 @@ public function getSecurityChecker() /** * @param SecurityChecker $securityChecker - * @return CVECheckTask $this + * @return $this */ - public function setSecurityChecker($securityChecker) + public function setSecurityChecker(SecurityChecker $securityChecker) { $this->securityChecker = $securityChecker; return $this; @@ -61,7 +61,7 @@ public function run($request) // Is this vulnerability known? No, lets add it. if ((int) $vulnerability->count() === 0) { - $vulnerability = new CVE(); + $vulnerability = CVE::create(); $vulnerability->PackageName = $package; $vulnerability->Version = $packageDetails['version']; $vulnerability->Title = $details['title']; @@ -87,15 +87,15 @@ public function run($request) } // remove all entries which are resolved (no longer $validEntries) - $removeOldCVEs = SQLDelete::create('CVE'); + $removeOldCVEs = SQLDelete::create('"CVE"'); if (empty($validEntries)) { - // There were no CVEs listed for our installation + // There were no CVEs listed for our installation - so flush any old data $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 = $removeOldCVEs->addWhere('"ID"', $removable->column('ID')); $removeOldCVEs->execute(); } } diff --git a/templates/PackageSecurityAlerts.ss b/templates/PackageSecurityAlerts.ss index 170e708..8e1ee49 100644 --- a/templates/PackageSecurityAlerts.ss +++ b/templates/PackageSecurityAlerts.ss @@ -1,13 +1,11 @@ -<% loop $Me %> - <% if $SecurityAlerts %> -
- $Name -
- <% loop $SecurityAlerts %> -
$CVE
-
$Title
- <% end_loop %> -
-
- <% end_if %> -<% end_loop %> +<% if $SecurityAlerts %> +
+ View security alert info +
+ <% loop $SecurityAlerts %> +
$CVE
+
$Title
+ <% end_loop %> +
+
+<% end_if %> diff --git a/templates/SecurityAlertSummary.ss b/templates/SecurityAlertSummary.ss new file mode 100644 index 0000000..5c0d567 --- /dev/null +++ b/templates/SecurityAlertSummary.ss @@ -0,0 +1,4 @@ +

+ Security alert
+ <% if Count > 1 %>Notices have<% else %>A notice has<% end_if %> been issued for $Count of your modules. Review and updating is recommended. +

\ No newline at end of file diff --git a/tests/CVECheckJobTest.php b/tests/CVECheckJobTest.php index 4c69701..07f94c0 100644 --- a/tests/CVECheckJobTest.php +++ b/tests/CVECheckJobTest.php @@ -8,7 +8,7 @@ public function testJobCallsTask() $spy->expects($this->once())->method('run'); $job = new CVECheckJob; - $job->setCVECheckTask($spy); + $job->setCheckTask($spy); $job->process(); } diff --git a/tests/CVECheckTaskTest.php b/tests/CVECheckTaskTest.php index f73a10f..dbd576a 100644 --- a/tests/CVECheckTaskTest.php +++ b/tests/CVECheckTaskTest.php @@ -41,12 +41,12 @@ public function testUpdatesAreSaved() $checkTask->setSecurityChecker($securityCheckerMock); $preCheck = CVE::get(); - $this->assertEquals(0, $preCheck->count(), 'database is empty to begin with'); + $this->assertCount(0, $preCheck, 'database is empty to begin with'); $checkTask->run(null); $postCheck = CVE::get(); - $this->assertEquals(1, $postCheck->count(), 'CVE has been stored'); + $this->assertCount(1, $postCheck, 'CVE has been stored'); } public function testNoDuplicates() @@ -56,17 +56,17 @@ public function testNoDuplicates() $checkTask->setSecurityChecker($securityCheckerMock); $preCheck = CVE::get(); - $this->assertEquals(0, $preCheck->count(), 'database is empty to begin with'); + $this->assertCount(0, $preCheck, 'database is empty to begin with'); $checkTask->run(null); $postCheck = CVE::get(); - $this->assertEquals(1, $postCheck->count(), 'CVE has been stored'); + $this->assertCount(1, $postCheck, 'CVE has been stored'); $checkTask->run(null); $postCheck = CVE::get(); - $this->assertEquals(1, $postCheck->count(), 'The CVE isn\'t stored twice.'); + $this->assertCount(1, $postCheck, 'The CVE isn\'t stored twice.'); } public function testCVERemovals() @@ -78,7 +78,7 @@ public function testCVERemovals() $checkTask->run(null); $preCheck = CVE::get(); - $this->assertEquals(1, $preCheck->count(), 'database has stored CVEs'); + $this->assertCount(1, $preCheck, 'database has stored CVEs'); $securityCheckerMock = $this->getSecurityCheckerMock(true); $checkTask->setSecurityChecker($securityCheckerMock); @@ -86,6 +86,6 @@ public function testCVERemovals() $checkTask->run(null); $postCheck = CVE::get(); - $this->assertEquals(0, $postCheck->count(), 'database is empty to finish with'); + $this->assertCount(0, $postCheck, 'database is empty to finish with'); } }