Skip to content

Commit

Permalink
Security advisories: store severity (#1395)
Browse files Browse the repository at this point in the history 
* RemoteSecurityAdvisory: transform to readonly properties

* RemoteSecurityAdvisory: store severity from GitHub database

* SecurityAdvisory: clarify how/why we store the severity always from GitHub

* SecurityAdvisory: return severity as part of the API response

* SecurityAdvisory: display severity in the list of package advisories

* SecurityAdvisory: extract list into shared template

* Fix template variable name

---------

Co-authored-by: Jordi Boggiano <j.boggiano@seld.be>
  • Loading branch information
@glaubinix @Seldaek
glaubinix and Seldaek authored Oct 27, 2023
1 parent 2298fd4 commit 35223c1
Show file tree
Hide file tree
Showing 20 changed files with 433 additions and 278 deletions.
6 changes: 3 additions & 3 deletions src/Controller/PackageController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1476,7 +1476,7 @@ public function securityAdvisoriesAction(Request $request, string $name): Respon
{
/** @var SecurityAdvisoryRepository $repo */
$repo = $this->getEM()->getRepository(SecurityAdvisory::class);
$securityAdvisories = $repo->getPackageSecurityAdvisories($name);
$securityAdvisories = $repo->findByPackageName($name);

$data = [];
$data['name'] = $name;
Expand All @@ -1491,7 +1491,7 @@ public function securityAdvisoriesAction(Request $request, string $name): Respon
$versionParser = new VersionParser();
foreach ($securityAdvisories as $advisory) {
try {
$affectedVersionConstraint = $versionParser->parseConstraints($advisory['affectedVersions']);
$affectedVersionConstraint = $versionParser->parseConstraints($advisory->getAffectedVersions());
} catch (UnexpectedValueException) {
// ignore parsing errors, advisory must be invalid
continue;
Expand Down Expand Up @@ -1528,7 +1528,7 @@ public function securityAdvisoryAction(Request $request, string $id): Response
throw new NotFoundHttpException();
}

return $this->render('package/security_advisory.html.twig', ['advisories' => $securityAdvisories, 'id' => $id]);
return $this->render('package/security_advisory.html.twig', ['securityAdvisories' => $securityAdvisories, 'id' => $id]);
}

private function createAddMaintainerForm(Package $package): FormInterface
Expand Down
101 changes: 67 additions & 34 deletions src/Entity/SecurityAdvisory.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use App\SecurityAdvisory\AdvisoryIdGenerator;
use App\SecurityAdvisory\AdvisoryParser;
use App\SecurityAdvisory\FriendsOfPhpSecurityAdvisoriesSource;
use App\SecurityAdvisory\Severity;
use DateTimeImmutable;
use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
Expand Down Expand Up @@ -70,6 +71,9 @@ class SecurityAdvisory
#[ORM\Column(type: 'string', nullable: true)]
private string|null $composerRepository = null;

#[ORM\Column(nullable: true)]
private Severity|null $severity = null;

/**
* @var Collection<int, SecurityAdvisorySource>&Selectable<int, SecurityAdvisorySource>
*/
Expand All @@ -86,44 +90,57 @@ public function __construct(RemoteSecurityAdvisory $advisory, string $source)
$this->updatedAt = new DateTimeImmutable();

$this->copyAdvisory($advisory, true);
$this->addSource($this->remoteId, $source);
$this->addSource($advisory->id, $source, $advisory->severity);
}

public function updateAdvisory(RemoteSecurityAdvisory $advisory): void
{
if (!in_array($advisory->getSource(), [null, $this->source], true)) {
$this->findSecurityAdvisorySource($advisory->source)?->update($advisory);

$now = new DateTimeImmutable();
if (!$this->severity && $advisory->severity) {
$this->updatedAt = $now;
$this->severity = $advisory->severity;
}

if (!in_array($advisory->source, [null, $this->source], true)) {
return;
}

if (
$this->remoteId !== $advisory->getId() ||
$this->packageName !== $advisory->getPackageName() ||
$this->title !== $advisory->getTitle() ||
$this->link !== $advisory->getLink() ||
$this->cve !== $advisory->getCve() ||
$this->affectedVersions !== $advisory->getAffectedVersions() ||
$this->reportedAt != $advisory->getDate() ||
$this->composerRepository !== $advisory->getComposerRepository()
$this->remoteId !== $advisory->id ||
$this->packageName !== $advisory->packageName ||
$this->title !== $advisory->title ||
$this->link !== $advisory->link ||
$this->cve !== $advisory->cve ||
$this->affectedVersions !== $advisory->affectedVersions ||
$this->reportedAt != $advisory->date ||
$this->composerRepository !== $advisory->composerRepository ||
($this->severity !== $advisory->severity && $advisory->severity)
) {
$this->updatedAt = new DateTimeImmutable();
$this->updatedAt = $now;
}

$this->copyAdvisory($advisory, false);
}

private function copyAdvisory(RemoteSecurityAdvisory $advisory, bool $initialCopy): void
{
$this->remoteId = $advisory->getId();
$this->packageName = $advisory->getPackageName();
$this->title = $advisory->getTitle();
$this->link = $advisory->getLink();
$this->cve = $advisory->getCve();
$this->affectedVersions = $advisory->getAffectedVersions();
$this->composerRepository = $advisory->getComposerRepository();
$this->remoteId = $advisory->id;
$this->packageName = $advisory->packageName;
$this->title = $advisory->title;
$this->link = $advisory->link;
$this->cve = $advisory->cve;
$this->affectedVersions = $advisory->affectedVersions;
$this->composerRepository = $advisory->composerRepository;

// only update if the date is different to avoid ending up with a new datetime object which doctrine will want to update in the DB for nothing
if ($initialCopy || $this->reportedAt != $advisory->getDate()) {
$this->reportedAt = $advisory->getDate();
if ($initialCopy || $this->reportedAt != $advisory->date) {
$this->reportedAt = $advisory->date;
}

if ($initialCopy && $advisory->severity) {
$this->severity = $advisory->severity;
}
}

Expand Down Expand Up @@ -179,52 +196,52 @@ public function getSource(): string
public function calculateDifferenceScore(RemoteSecurityAdvisory $advisory): int
{
// Regard advisories where CVE + package name match as identical as the remaining data on GitHub and FriendsOfPhp can be quite different
if ($advisory->getCve() === $this->getCve() && $advisory->getPackageName() === $this->getPackageName()) {
if ($advisory->cve === $this->getCve() && $advisory->packageName === $this->getPackageName()) {
return 0;
}

$score = 0;
if ($advisory->getId() !== $this->getRemoteId() && $this->getSource() === $advisory->getSource()) {
if ($advisory->id !== $this->getRemoteId() && $this->getSource() === $advisory->source) {
$score++;
}

if ($advisory->getPackageName() !== $this->getPackageName()) {
if ($advisory->packageName !== $this->getPackageName()) {
$score += 99;
}

if ($advisory->getTitle() !== $this->getTitle()) {
if ($advisory->title !== $this->getTitle()) {
$increase = 1;

// Do not increase the score if the title was just renamed to add a CVE e.g. from CVE-2022-xxx to CVE-2022-99999999
if (AdvisoryParser::titleWithoutCve($this->getTitle()) === AdvisoryParser::titleWithoutCve($advisory->getTitle())) {
if (AdvisoryParser::titleWithoutCve($this->getTitle()) === AdvisoryParser::titleWithoutCve($advisory->title)) {
$increase = 0;
}

$score += $increase;
}

if ($advisory->getLink() !== $this->getLink() && !in_array($this->getLink(), $advisory->getReferences(), true)) {
if ($advisory->link !== $this->getLink() && !in_array($this->getLink(), $advisory->references, true)) {
$score++;
}

if ($advisory->getCve() !== $this->getCve()) {
if ($advisory->cve !== $this->getCve()) {
$score++;

// CVE ID changed from not null to different not-null value
if ($advisory->getCve() !== null && $this->getCve() !== null) {
if ($advisory->cve !== null && $this->getCve() !== null) {
$score += 99;
}
}

if ($advisory->getAffectedVersions() !== $this->getAffectedVersions()) {
if ($advisory->affectedVersions !== $this->getAffectedVersions()) {
$score++;
}

if ($advisory->getComposerRepository() !== $this->composerRepository) {
if ($advisory->composerRepository !== $this->composerRepository) {
$score++;
}

if ($advisory->getDate() != $this->reportedAt) {
if ($advisory->date != $this->reportedAt) {
$score++;
}

Expand All @@ -241,15 +258,20 @@ private function assignPackagistAdvisoryId(): void
$this->packagistAdvisoryId = AdvisoryIdGenerator::generate();
}

public function getSeverity(): ?Severity
{
return $this->severity;
}

public function hasSources(): bool
{
return !$this->sources->isEmpty();
}

public function addSource(string $remoteId, string $source): void
public function addSource(string $remoteId, string $source, Severity|null $severity): void
{
if (null === $this->getSourceRemoteId($source)) {
$this->sources->add(new SecurityAdvisorySource($this, $remoteId, $source));
$this->sources->add(new SecurityAdvisorySource($this, $remoteId, $source, $severity));

// FriendsOfPhp source is curated by PHP developer, trust that data over data from GitHub
if ($source === FriendsOfPhpSecurityAdvisoriesSource::SOURCE_NAME) {
Expand Down Expand Up @@ -300,7 +322,18 @@ public function getSourceRemoteId(string $source): ?string
public function setupSource(): void
{
if (!$this->getSourceRemoteId($this->source)) {
$this->addSource($this->remoteId, $this->source);
$this->addSource($this->remoteId, $this->source, null);
}
}

public function findSecurityAdvisorySource(string $search): ?SecurityAdvisorySource
{
foreach ($this->sources as $source) {
if ($source->getSource() === $search) {
return $source;
}
}

return null;
}
}
18 changes: 17 additions & 1 deletion src/Entity/SecurityAdvisoryRepository.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,22 @@ public function findByRemoteId(string $source, string $id): array
->getResult();
}

/**
* @return list<SecurityAdvisory>
*/
public function findByPackageName(string $packageName): array
{
return $this
->createQueryBuilder('a')
->addSelect('s')
->leftJoin('a.sources', 's')
->where('a.packageName = :packageName')
->orderBy('a.reportedAt', 'DESC')
->setParameters(['packageName' => $packageName])
->getQuery()
->getResult();
}

/**
* @return array<string, array{advisoryId: string, packageName: string, remoteId: string, title: string, link: string|null, cve: string|null, affectedVersions: string, sources: array<array{name: string, remoteId: string}>, reportedAt: string, composerRepository: string|null}>
*/
Expand Down Expand Up @@ -136,7 +152,7 @@ public function searchSecurityAdvisories(array $packageNames, int $updatedSince)
}

if (!$useCache || $filterByNames) {
$sql = 'SELECT s.packagistAdvisoryId as advisoryId, s.packageName, s.remoteId, s.title, s.link, s.cve, s.affectedVersions, s.source, s.reportedAt, s.composerRepository, sa.source sourceSource, sa.remoteId sourceRemoteId
$sql = 'SELECT s.packagistAdvisoryId as advisoryId, s.packageName, s.remoteId, s.title, s.link, s.cve, s.affectedVersions, s.source, s.reportedAt, s.composerRepository, sa.source sourceSource, sa.remoteId sourceRemoteId, s.severity
FROM security_advisory s
INNER JOIN security_advisory_source sa ON sa.securityAdvisory_id=s.id
WHERE s.updatedAt >= :updatedSince '.
Expand Down
18 changes: 17 additions & 1 deletion src/Entity/SecurityAdvisorySource.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

namespace App\Entity;

use App\SecurityAdvisory\RemoteSecurityAdvisory;
use App\SecurityAdvisory\Severity;
use Doctrine\ORM\Mapping as ORM;

#[ORM\Entity]
Expand All @@ -32,11 +34,15 @@ class SecurityAdvisorySource
#[ORM\Column(type: 'string')]
private string $source;

public function __construct(SecurityAdvisory $securityAdvisory, string $remoteId, string $source)
#[ORM\Column(nullable: true)]
private Severity|null $severity;

public function __construct(SecurityAdvisory $securityAdvisory, string $remoteId, string $source, Severity|null $severity)
{
$this->securityAdvisory = $securityAdvisory;
$this->remoteId = $remoteId;
$this->source = $source;
$this->severity = $severity;
}

public function getRemoteId(): string
Expand All @@ -48,4 +54,14 @@ public function getSource(): string
{
return $this->source;
}

public function getSeverity(): ?Severity
{
return $this->severity;
}

public function update(RemoteSecurityAdvisory $advisory): void
{
$this->severity = $advisory->severity;
}
}
2 changes: 2 additions & 0 deletions src/SecurityAdvisory/GitHubSecurityAdvisoriesSource.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ public function getAdvisories(ConsoleIO $io): ?RemoteSecurityAdvisoryCollection
$this->providerManager->packageExists($packageName) ? SecurityAdvisory::PACKAGIST_ORG : null,
$references,
self::SOURCE_NAME,
Severity::fromGitHub($node['advisory']['severity']),
);
}

Expand Down Expand Up @@ -157,6 +158,7 @@ private function getQuery(string $after = ''): string
permalink,
publishedAt,
withdrawnAt,
severity,
identifiers {
type,
value
Expand Down
Loading

0 comments on commit 35223c1

Please sign in to comment.