Skip to content

Commit

Permalink
FIX Make task work
Browse files Browse the repository at this point in the history
Not sure how it was supposed to work before this fix.
Looking at the earliest stable release in the constraint
for https://github.com/sensiolabs/security-checker/tree/v5.0.0,
it mentions the use of json_decode() there.

We've previously looped over a JSON string,
and never created any security entries.

The bugfix around strict "=== 0" checks
might be a regression from framework, maybe
we used to set those foreign keys to 0,
and now we're leaving them null?
Either way, it's unnecessarily strict for this case.

Fixed the mocks, since they weren't using the actual API provided by the sensiolabs module
  • Loading branch information
chillu authored and robbieaverill committed Mar 19, 2019
1 parent fec6924 commit f8b72c6
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 7 deletions.
16 changes: 10 additions & 6 deletions src/Tasks/SecurityAlertCheckTask.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,22 +79,25 @@ public function run($request)

// use the security checker of
$checker = $this->getSecurityChecker();
$alerts = $checker->check(BASE_PATH . DIRECTORY_SEPARATOR . 'composer.lock');
$result = $checker->check(BASE_PATH . DIRECTORY_SEPARATOR . 'composer.lock');
$alerts = json_decode((string) $result, true);

// 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']);
$vulnerability = null;

// check if this vulnerability is already known
$vulnerability = SecurityAlert::get()->filter(array(
$existingVulns = SecurityAlert::get()->filter(array(
'PackageName' => $package,
'Version' => $packageDetails['version'],
'Identifier' => $identifier,
));

// Is this vulnerability known? No, lets add it.
if ((int) $vulnerability->count() === 0) {
if (!$existingVulns->Count()) {
$vulnerability = SecurityAlert::create();
$vulnerability->PackageName = $package;
$vulnerability->Version = $packageDetails['version'];
Expand All @@ -108,17 +111,18 @@ public function run($request)
$validEntries[] = $vulnerability->ID;
} else {
// add existing vulnerabilities (probably just 1) to the list of valid entries
$validEntries = array_merge($validEntries, $vulnerability->column('ID'));
$validEntries = array_merge($validEntries, $existingVulns->column('ID'));
}

// Relate this vulnerability to an existing Package, if the
// bringyourownideas/silverstripe-maintenance module is installed
if ($vulnerability->hasExtension(SecurityAlertExtension::class)
if ($vulnerability && $vulnerability->hasExtension(SecurityAlertExtension::class)
&& class_exists(Package::class)
&& $vulnerability->PackageRecordID === 0
&& !$vulnerability->PackageRecordID
&& $packageRecord = Package::get()->find('Name', $package)
) {
$vulnerability->PackageRecordID = $packageRecord->ID;
$vulnerability->write();
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion tests/SecurityAlertCheckTaskTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use BringYourOwnIdeas\SecurityChecker\Models\SecurityAlert;
use BringYourOwnIdeas\SecurityChecker\Tasks\SecurityAlertCheckTask;
use SensioLabs\Security\Result;
use SensioLabs\Security\SecurityChecker;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Dev\SapphireTest;
Expand Down Expand Up @@ -99,7 +100,7 @@ protected function getSecurityCheckerMock($empty = false)

$securityCheckerMock = $this->getMockBuilder(SecurityChecker::class)->setMethods(['check'])->getMock();
$securityCheckerMock->expects($this->any())->method('check')->will($this->returnValue(
$empty ? [] : json_decode($mockOutput, true)
$empty ? new Result(0, '{}', 'json') : new Result(6, $mockOutput, 'json')
));

return $securityCheckerMock;
Expand Down

0 comments on commit f8b72c6

Please sign in to comment.