diff --git a/.editorconfig b/.editorconfig index 47ae637..0a8a30b 100644 --- a/.editorconfig +++ b/.editorconfig @@ -10,6 +10,10 @@ indent_style = space insert_final_newline = true trim_trailing_whitespace = true +[*{.yml,.js}] +indent_size = 2 +indent_style = space + [{*.yml,package.json}] indent_size = 2 diff --git a/.travis.yml b/.travis.yml index 4861d7b..f0ca414 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,19 +4,10 @@ sudo: false language: php -php: - - 5.6 - - 7.0 - -env: - - DB=MYSQL CORE_RELEASE=3.1 - 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 @@ -27,8 +18,10 @@ matrix: env: DB=MYSQL CORE_RELEASE=3.5 - php: 5.6 env: DB=MYSQL CORE_RELEASE=3.6 - allow_failures: - php: 7.0 + env: DB=MYSQL CORE_RELEASE=3.6 + - php: 7.1 + env: DB=MYSQL CORE_RELEASE=3.6 before_script: - composer self-update || true diff --git a/_config/packagerelations.yml b/_config/packagerelations.yml new file mode 100644 index 0000000..165bc7d --- /dev/null +++ b/_config/packagerelations.yml @@ -0,0 +1,15 @@ +--- +Name: packagerelations +after: '#updatecheckerextensions' +Only: + moduleexists: silverstripe-maintenance +--- +Package: + extensions: + - PackageSecurityExtension +SecurityAlert: + extensions: + - SecurityAlertExtension +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..9b44349 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.2", + "sensiolabs/security-checker": "^4", + "symbiote/silverstripe-queuedjobs": "^2" }, "replace": { "spekulatius/silverstripe-composer-security-checker": "self.version" }, "suggest": { + "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..6781a7d --- /dev/null +++ b/css/securityalerts.css @@ -0,0 +1,8 @@ +.security-alerts__list { + margin-top: 8px; + display: none; +} + +.security-alerts__toggler { + cursor: pointer; +} diff --git a/javascript/summaryalerts.js b/javascript/summaryalerts.js new file mode 100644 index 0000000..567af2e --- /dev/null +++ b/javascript/summaryalerts.js @@ -0,0 +1,33 @@ +(function($) { + $.entwine('ss', function($) { + $('.package-summary__security-alerts').entwine({ + IsShown: false, + toggleSecurityNotices: function() { + if (this.getIsShown()) { + this.hideSecurityNotices(); + } else { + this.showSecurityNotices(); + } + }, + showSecurityNotices: function() { + this.getAlertList().show(); + this.setIsShown(true); + }, + hideSecurityNotices: function() { + this.getAlertList().hide(); + this.setIsShown(false); + }, + getAlertList: function() { + return this.children('.security-alerts__list'); + } + }); + $('.security-alerts__toggler').entwine({ + onclick: function(event) { + this.parent() + .nextAll('.package-summary__security-alerts') + .toggleSecurityNotices(); + event.preventDefault(); + } + }); + }); +})(jQuery) diff --git a/lang/en.yml b/lang/en.yml new file mode 100644 index 0000000..93368f2 --- /dev/null +++ b/lang/en.yml @@ -0,0 +1,7 @@ +en: + PackageSecurityExtension: + BADGE_SECURITY: 'RISK: Security' + SecurityAlertSummary: + TITLE: 'Security alert' + NOTICE_ONE: 'A notice has been issued for 1 of your modules. Review and updating is recommended.' + NOTICE_MANY: 'Notices have been issued for {count} of your modules. Review and updating is recommended.' 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/PackageSecruityExtension.php b/src/Extensions/PackageSecruityExtension.php new file mode 100644 index 0000000..19a8537 --- /dev/null +++ b/src/Extensions/PackageSecruityExtension.php @@ -0,0 +1,60 @@ + 'SecurityAlert' + ]; + + private static $summary_fields = [ + 'listSecurityAlertIdentifiers' => 'Security alerts', + ]; + + /** + * Simply returns a comma separated list of active SecurityAlert Identifiers for this record. + * Used in CSV exports as a type of brief indication (as opposed to full info) + */ + public function listSecurityAlertIdentifiers() + { + $alerts = $this->owner->SecurityAlerts()->Column('Identifier'); + 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 ArrayList $badges + */ + public function updateBadges(&$badges) + { + if ($this->owner->SecurityAlerts()->exists()) { + $badges->push(ArrayData::create([ + 'Title' => _t(__CLASS__ . '.BADGE_SECURITY', 'RISK: Security'), + 'Type' => 'warning security-alerts__toggler', + ])); + } + } + + /** + * 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/SecurityAlertExtension.php b/src/Extensions/SecurityAlertExtension.php new file mode 100644 index 0000000..3e8ab6e --- /dev/null +++ b/src/Extensions/SecurityAlertExtension.php @@ -0,0 +1,8 @@ + 'Package' + ]; +} diff --git a/src/Extensions/SiteSummaryExtension.php b/src/Extensions/SiteSummaryExtension.php new file mode 100644 index 0000000..951f5c1 --- /dev/null +++ b/src/Extensions/SiteSummaryExtension.php @@ -0,0 +1,32 @@ +owner->sourceRecords()->filter('SecurityAlerts.ID:GreaterThan', 0); + + if ($securityWarnings->exists()) { + $alerts['SecurityAlerts'] = $securityWarnings->renderWith('SecurityAlertSummary'); + } + } +} diff --git a/src/Jobs/SecurityAlertCheckJob.php b/src/Jobs/SecurityAlertCheckJob.php new file mode 100644 index 0000000..b57ed0c --- /dev/null +++ b/src/Jobs/SecurityAlertCheckJob.php @@ -0,0 +1,61 @@ + '%$' . SecurityAlertCheckTask::class, + ]; + + /** + * @var SecurityAlertCheckTask + */ + protected $checkTask; + + /** + * @return SecurityAlertCheckTask + */ + public function getCheckTask() + { + return $this->checkTask; + } + + /** + * @param SecurityAlertCheckTask $checkTask + * @return SecurityAlertCheckJob + */ + public function setCheckTask(SecurityAlertCheckTask $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->getCheckTask(); + $task->run(null); + + // mark job as completed + $this->isComplete = true; + } +} diff --git a/src/Models/SecurityAlert.php b/src/Models/SecurityAlert.php new file mode 100644 index 0000000..51179f3 --- /dev/null +++ b/src/Models/SecurityAlert.php @@ -0,0 +1,20 @@ + 'Varchar(255)', + 'Version' => 'Varchar(255)', + 'Title' => 'Varchar(255)', + 'ExternalLink' => 'Varchar(255)', + 'Identifier' => 'Varchar(255)', + ); + + private static $summary_fields = array( + 'PackageName', + 'Version', + 'Title', + ); +} diff --git a/src/Tasks/SecurityAlertCheckTask.php b/src/Tasks/SecurityAlertCheckTask.php new file mode 100644 index 0000000..421e89d --- /dev/null +++ b/src/Tasks/SecurityAlertCheckTask.php @@ -0,0 +1,150 @@ + '%$' . 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 $this + */ + public function setSecurityChecker(SecurityChecker $securityChecker) + { + $this->securityChecker = $securityChecker; + return $this; + } + + /** + * Most SilverStripe issued alerts are _not_ assiged CVEs. + * However they have their own identifier in the form of a + * prefix to the title - we can use this instead of a CVE ID. + * + * @param string $cve + * @param string $title + * + * @return string + */ + protected function discernIdentifier($cve, $title) + { + $identifier = $cve; + if (!$identifier || $identifier === '~') { + $identifier = explode(':', $title); + $identifier = array_shift($identifier); + } + $this->extend('updateIdentifier', $identifier, $cve, $title); + return $identifier; + } + + 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) { + $identifier = $this->discernIdentifier($details['cve'], $details['title']); + // check if this vulnerability is already known + $vulnerability = SecurityAlert::get()->filter(array( + 'PackageName' => $package, + 'Version' => $packageDetails['version'], + 'Identifier' => $identifier, + )); + + // Is this vulnerability known? No, lets add it. + if ((int) $vulnerability->count() === 0) { + $vulnerability = SecurityAlert::create(); + $vulnerability->PackageName = $package; + $vulnerability->Version = $packageDetails['version']; + $vulnerability->Title = $details['title']; + $vulnerability->ExternalLink = $details['link']; + $vulnerability->Identifier = $identifier; + + $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(SecurityAlertExtension::class) && + $vulnerability->PackageRecordID === 0 && + $packageRecord = Package::get()->find('Name', $package) + ) { + $vulnerability->PackageRecordID = $packageRecord->ID; + } + } + } + + // remove all entries which are resolved (no longer $validEntries) + $removeOldSecurityAlerts = SQLDelete::create('"SecurityAlert"'); + if (empty($validEntries)) { + // There were no SecurityAlerts listed for our installation - so flush any old data + $removeOldSecurityAlerts->execute(); + } else { + $removable = SecurityAlert::get()->exclude(array('ID' => $validEntries)); + // Be careful not to remove all SecurityAlerts on the case that every entry is valid + if ($removable->exists()) { + // SQLConditionalExpression does not support IN() syntax via addWhere + // so we have to build this up manually + $convertIDsToQuestionMarks = function ($id) { + return '?'; + }; + $queryArgs = $removable->column('ID'); + $paramPlaceholders = implode(',', array_map($convertIDsToQuestionMarks, $queryArgs)); + + $removeOldSecurityAlerts = $removeOldSecurityAlerts->addWhere([ + '"ID" IN(' . $paramPlaceholders . ')' => $queryArgs + ]); + $removeOldSecurityAlerts->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..3eda405 --- /dev/null +++ b/templates/PackageSecurityAlerts.ss @@ -0,0 +1,10 @@ +<% if $SecurityAlerts %> +
+
+ <% loop $SecurityAlerts %> +
$Identifier
+
$Title
+ <% end_loop %> +
+
+<% end_if %> diff --git a/templates/SecurityAlertSummary.ss b/templates/SecurityAlertSummary.ss new file mode 100644 index 0000000..66a317d --- /dev/null +++ b/templates/SecurityAlertSummary.ss @@ -0,0 +1,8 @@ +

+ <%t SecurityAlertSummary.TITLE "Security alert" %>
+ <% if Count > 1 %> + <%t SecurityAlertSummary.NOTICE_MANY "Notices have been issued for {count} of your modules. Review and updating is recommended." count=$Count %> + <% else %> + <%t SecurityAlertSummary.NOTICE_ONE "A notice has been issued for {count} of your modules. Review and updating is recommended." count=$Count %> + <% end_if %> +

\ No newline at end of file diff --git a/tests/.gitkeep b/tests/.gitkeep deleted file mode 100644 index e69de29..0000000 diff --git a/tests/SecurityAlertCheckJobTest.php b/tests/SecurityAlertCheckJobTest.php new file mode 100644 index 0000000..828e71f --- /dev/null +++ b/tests/SecurityAlertCheckJobTest.php @@ -0,0 +1,15 @@ +getMockBuilder(SecurityAlertCheckTask::class)->setMethods(['run'])->getMock(); + $spy->expects($this->once())->method('run'); + + $job = new SecurityAlertCheckJob; + $job->setCheckTask($spy); + + $job->process(); + } +} diff --git a/tests/SecurityAlertCheckTaskTest.php b/tests/SecurityAlertCheckTaskTest.php new file mode 100644 index 0000000..63d2d18 --- /dev/null +++ b/tests/SecurityAlertCheckTaskTest.php @@ -0,0 +1,140 @@ +getMockBuilder(SecurityChecker::class)->setMethods(['check'])->getMock(); + $securityCheckerMock->expects($this->any())->method('check')->will($this->returnValue( + $empty ? [] : json_decode($mockOutput, true) + )); + + return $securityCheckerMock; + } + + public function setUp() + { + parent::setUp(); + $securityCheckerMock = $this->getSecurityCheckerMock(); + $checkTask = new SecurityAlertCheckTask; + $checkTask->setSecurityChecker($securityCheckerMock); + $this->checkTask = $checkTask; + } + + public function testUpdatesAreSaved() + { + $checkTask = $this->checkTask; + + $preCheck = SecurityAlert::get(); + $this->assertCount(0, $preCheck, 'database is empty to begin with'); + + $checkTask->run(null); + + $postCheck = SecurityAlert::get(); + $this->assertCount(6, $postCheck, 'SecurityAlert has been stored'); + } + + public function testNoDuplicates() + { + $checkTask = $this->checkTask; + + $checkTask->run(null); + + $postCheck = SecurityAlert::get(); + $this->assertCount(6, $postCheck, 'SecurityAlert has been stored'); + + $checkTask->run(null); + + $postCheck = SecurityAlert::get(); + $this->assertCount(6, $postCheck, 'The SecurityAlert isn\'t stored twice.'); + } + + public function testSecurityAlertRemovals() + { + $checkTask = $this->checkTask; + + $checkTask->run(null); + + $preCheck = SecurityAlert::get(); + $this->assertCount(6, $preCheck, 'database has stored SecurityAlerts'); + + $securityCheckerMock = $this->getSecurityCheckerMock(true); + $checkTask->setSecurityChecker($securityCheckerMock); + + $checkTask->run(null); + + $postCheck = SecurityAlert::get(); + $this->assertCount(0, $postCheck, 'database is empty to finish with'); + } + + public function testIdentifierSetsFromTitleIfCVEIsNotSet() + { + $checkTask = $this->checkTask; + $checkTask->run(null); + $frameworkAlert = SecurityAlert::get() + ->filter('PackageName', 'silverstripe/framework') + ->first(); + $this->assertNotEmpty($frameworkAlert->Identifier); + $this->assertRegExp('/^SS-201[56]-\d{3}$/', $frameworkAlert->Identifier); + } +}