Big refactor to make sense of code

Use sensible names, and also fix a few bug discovered by altering the
tests with more data and cases (such as the SQLDelete statement). CVE
DataObject is now SecurityAlerts, as not all alerts are assigned CVE
identifiers. To that end if there is another identifier (such as the one
SilverStripe use as a title prefix) then this is set instead. As such the
property is renamed 'Identifier' as opposed to 'CVE'.

_config/packagerelations.yml

@@ -7,9 +7,9 @@ Only:
 Package:
   extensions:
     - PackageSecurityExtension
-CVE:
+SecurityAlert:
   extensions:
-    - CVEExtension
+    - SecurityAlertExtension
 SiteSummary:
   extensions:
     - SiteSummaryExtension

src/Extensions/PackageSecruityExtension.php

@@ -3,20 +3,20 @@
 class PackageSecurityExtension extends DataExtension
 {
     private static $has_many = [
-        'SecurityAlerts' => 'CVE'
+        'SecurityAlerts' => 'SecurityAlert'
     ];
 
     private static $summary_fields = [
-        'listCVEs' => 'Security alerts',
+        'listSecurityAlertIdentifiers' => 'Security alerts',
     ];
 
     /**
-     * Simply returns a comma separated list of active CVE numbers for this record.
+     * 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 listCVEs()
+    public function listSecurityAlertIdentifiers()
     {
-        $alerts = $this->owner->SecurityAlerts()->Column('CVE');
+        $alerts = $this->owner->SecurityAlerts()->Column('Identifier');
         return $alerts ? implode(', ', $alerts) : null;
     }
 

src/Extensions/CVEExtension.php → src/Extensions/SecurityAlertExtension.php

@@ -1,6 +1,6 @@
 <?php
 
-class CVEExtension extends DataExtension
+class SecurityAlertExtension extends DataExtension
 {
     private static $has_one = [
         'PackageRecord' => 'Package'

src/Extensions/SiteSummaryExtension.php

@@ -4,15 +4,15 @@ 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
+     * listing SecurityAlert Identifier 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']);
+        unset($columns['listSecurityAlertIdentifiers']);
     }
 
     /**

src/Jobs/CVECheckJob.php → src/Jobs/SecurityAlertCheckJob.php

@@ -5,30 +5,30 @@
  * @author Peter Thaleikis
  * @license BSD-3-Clause
  */
-class CVECheckJob extends AbstractQueuedJob implements QueuedJob
+class SecurityAlertCheckJob extends AbstractQueuedJob implements QueuedJob
 {
     private static $dependencies = [
-        'CVECheckTask' => '%$' . CVECheckTask::class,
+        'SecurityAlertCheckTask' => '%$' . SecurityAlertCheckTask::class,
     ];
 
     /**
-     * @var CVECheckTask
+     * @var SecurityAlertCheckTask
      */
     protected $checkTask;
 
     /**
-     * @return CVECheckTask
+     * @return SecurityAlertCheckTask
      */
     public function getCheckTask()
     {
         return $this->checkTask;
     }
 
     /**
-     * @param CVECheckTask $checkTask
-     * @return CVECheckJob
+     * @param SecurityAlertCheckTask $checkTask
+     * @return SecurityAlertCheckJob
      */
-    public function setCheckTask(CVECheckTask $checkTask)
+    public function setCheckTask(SecurityAlertCheckTask $checkTask)
     {
         $this->checkTask = $checkTask;
         return $this;

src/Models/CVE.php → src/Models/SecurityAlert.php

@@ -2,14 +2,14 @@
 /**
  * Describes a known security issue of an installed Composer package
  */
-class CVE extends DataObject
+class SecurityAlert extends DataObject
 {
     private static $db = array(
         'PackageName' => 'Varchar(255)',
         'Version' => 'Varchar(255)',
         'Title' => 'Varchar(255)',
         'ExternalLink' => 'Varchar(255)',
-        'CVE' => 'Varchar(255)',
+        'Identifier' => 'Varchar(255)',
     );
 
     private static $summary_fields = array(

src/Tasks/CVECheckTask.php → src/Tasks/SecurityAlertCheckTask.php

@@ -5,7 +5,7 @@
 /**
  * Checks if there are any insecure dependencies.
  */
-class CVECheckTask extends BuildTask
+class SecurityAlertCheckTask extends BuildTask
 {
     /**
      * @var SecurityChecker
@@ -39,6 +39,27 @@ public function setSecurityChecker(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
@@ -52,21 +73,22 @@ public function run($request)
         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 = CVE::get()->filter(array(
+                $vulnerability = SecurityAlert::get()->filter(array(
                     'PackageName' => $package,
                     'Version' => $packageDetails['version'],
-                    'CVE'   => $details['cve'],
+                    'Identifier'   => $identifier,
                 ));
 
                 // Is this vulnerability known? No, lets add it.
                 if ((int) $vulnerability->count() === 0) {
-                    $vulnerability = CVE::create();
+                    $vulnerability = SecurityAlert::create();
                     $vulnerability->PackageName  = $package;
                     $vulnerability->Version      = $packageDetails['version'];
                     $vulnerability->Title        = $details['title'];
                     $vulnerability->ExternalLink = $details['link'];
-                    $vulnerability->CVE          = $details['cve'];
+                    $vulnerability->Identifier   = $identifier;
 
                     $vulnerability->write();
 
@@ -77,7 +99,7 @@ public function run($request)
                     $validEntries = array_merge($validEntries, $vulnerability->column('ID'));
                 }
 
-                if ($vulnerability->hasExtension(CVEExtension::class) &&
+                if ($vulnerability->hasExtension(SecurityAlertExtension::class) &&
                     $vulnerability->PackageRecordID === 0 &&
                     $packageRecord = Package::get()->find('Name', $package)
                 ) {
@@ -87,16 +109,26 @@ public function run($request)
         }
 
         // remove all entries which are resolved (no longer $validEntries)
-        $removeOldCVEs = SQLDelete::create('"CVE"');
+        $removeOldSecurityAlerts = SQLDelete::create('"SecurityAlert"');
         if (empty($validEntries)) {
-            // There were no CVEs listed for our installation - so flush any old data
-            $removeOldCVEs->execute();
+            // There were no SecurityAlerts listed for our installation - so flush any old data
+            $removeOldSecurityAlerts->execute();
         } else {
-            $removable = CVE::get()->exclude(array('ID' => $validEntries));
+            $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()) {
-                // Be careful not to remove all CVEs on the case that entry is valid
-                $removeOldCVEs = $removeOldCVEs->addWhere('"ID"', $removable->column('ID'));
-                $removeOldCVEs->execute();
+                // 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();
             }
         }
 

templates/PackageSecurityAlerts.ss

@@ -2,7 +2,7 @@
 <div class="package-summary__security-alerts">
     <dl class="security-alerts__list">
         <% loop $SecurityAlerts %>
-        <dt><a href="$ExternalLink">$CVE</a></dt>
+        <dt><a href="$ExternalLink">$Identifier</a></dt>
         <dd>$Title</dd>
         <% end_loop %>
     </dl>

tests/SecurityAlertCheckJobTest.php

@@ -0,0 +1,15 @@
+<?php
+
+class SecurityAlertCheckJobTest extends SapphireTest
+{
+    public function testJobCallsTask()
+    {
+        $spy = $this->getMockBuilder(SecurityAlertCheckTask::class)->setMethods(['run'])->getMock();
+        $spy->expects($this->once())->method('run');
+
+        $job = new SecurityAlertCheckJob;
+        $job->setCheckTask($spy);
+
+        $job->process();
+    }
+}