Withdraw security advisories instead of deleting them - #1816
Conversation
glaubinix
left a comment
There was a problem hiding this comment.
We probably need to treat withdrawn advisories in a special way in the resolver. The difference score should probably only match if either packageName + CVE match or packageName + remoteId match. This way we avoid accidentally matching old withdrawn advisories with new advisories. The risk here is that if an advisory was published by accident, people start ignoring it because its bogus, the advisory then gets withdrawn, later someone identifies a very similar security issue, an advisory gets published that by accident matches with the withdrawn one and because people ignored the old advisoryID, they will never know about the newly found issue.
|
|
||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we want to specifically mark withdrawn advisories that get unwithdrawn? Currently they are flagged as edited
There was a problem hiding this comment.
Added dedicated event for this.
| $active = $this->getEM()->getRepository(SecurityAdvisory::class)->getPackageSecurityAdvisories('acme/orphan'); | ||
| $this->assertSame([], $active); | ||
| } | ||
|
|
There was a problem hiding this comment.
There are now scenarios where the worker will end up throwing unrecoverable unique constraint violations. They are probably unlikely to happen but should still be addressed to make sure we don't end up with a crashing worker.
/**
* Same reassignment as above, but the advisory that inherits the CVE was inserted first, so
* Doctrine computes its change set (and runs its UPDATE) before the withdrawal UPDATE. Both
* land in the same flush, so without an intermediate flush the outcome hangs on entity order.
*/
public function testWithdrawnAdvisoryFreesCveForReassignmentWhateverTheEntityOrder(): void
{
$survivor = new SecurityAdvisory($this->remoteAdvisory('GHSA-live-2222-2222', 'acme/reordered', 'CVE-2024-40004'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$withdrawn = new SecurityAdvisory($this->remoteAdvisory('GHSA-stale-3333-3333', 'acme/reordered', 'CVE-2024-50005'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$this->store($survivor, $withdrawn);
$collection = new RemoteSecurityAdvisoryCollection(
[$this->remoteAdvisory('GHSA-live-2222-2222', 'acme/reordered', 'CVE-2024-50005')],
['acme/reordered' => ['GHSA-stale-3333-3333' => true]],
);
$this->runWorkerWithSource($collection);
$this->getEM()->clear();
$advisories = $this->getEM()->getRepository(SecurityAdvisory::class)->findByPackageName('acme/reordered');
$this->assertCount(2, $advisories);
$this->assertSame('CVE-2024-50005', $this->activeAdvisory($advisories)->getCve());
}
/**
* The withdrawn advisory keeps its CVE and the source publishes a brand new advisory carrying
* that same CVE in the same run. removeWithdrawn() leaves the withdrawn advisory out of the
* list handed to resolve(), so the replacement is persisted as a new entity, and Doctrine
* commits inserts before updates.
*/
public function testWithdrawnAdvisoryFreesCveForAReplacementInsertedInTheSameRun(): void
{
$withdrawn = new SecurityAdvisory($this->remoteAdvisory('GHSA-gone-4444-4444', 'acme/replaced', 'CVE-2024-60006'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$this->store($withdrawn);
$collection = new RemoteSecurityAdvisoryCollection(
[$this->remoteAdvisory('GHSA-fresh-5555-5555', 'acme/replaced', 'CVE-2024-60006')],
['acme/replaced' => ['GHSA-gone-4444-4444' => true]],
);
$this->runWorkerWithSource($collection);
$this->getEM()->clear();
$advisories = $this->getEM()->getRepository(SecurityAdvisory::class)->findByPackageName('acme/replaced');
$this->assertCount(2, $advisories);
$this->assertSame('GHSA-fresh-5555-5555', $this->activeAdvisory($advisories)->getRemoteId());
}
/**
* @param SecurityAdvisory[] $advisories
*/
private function activeAdvisory(array $advisories): SecurityAdvisory
{
$active = array_values(array_filter($advisories, static fn (SecurityAdvisory $a) => !$a->isWithdrawn()));
$this->assertCount(1, $active);
return $active[0];
}
There was a problem hiding this comment.
Great observation. Fixed this by not allowing to un-withdraw a withdrawn advisory when an active one with same CVE exists.
There was a problem hiding this comment.
This only half fixed the unique constraint violation. Can still reproduce it via
public function testCveReassignedFromAnAdvisoryTheSourceStoppedListing(): void
{
$survivor = new SecurityAdvisory($this->remoteAdvisory('GHSA-survivor-0000', 'acme/dropped', 'CVE-2024-70007'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$dropped = new SecurityAdvisory($this->remoteAdvisory('GHSA-dropped-1111', 'acme/dropped', 'CVE-2024-80008'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$this->store($survivor, $dropped);
// No withdrawn map entry: GHSA-dropped-1111 is just absent from the source's response.
$collection = new RemoteSecurityAdvisoryCollection(
[$this->remoteAdvisory('GHSA-survivor-0000', 'acme/dropped', 'CVE-2024-80008')],
);
$this->runWorkerWithSource($collection);
$this->getEM()->clear();
$advisories = $this->getEM()->getRepository(SecurityAdvisory::class)->findByPackageName('acme/dropped');
$this->assertCount(2, $advisories);
$this->assertSame('CVE-2024-80008', $this->activeAdvisory($advisories)->getCve());
$this->assertSame('GHSA-survivor-0000', $this->activeAdvisory($advisories)->getRemoteId());
}
There was a problem hiding this comment.
Additionally holding back withdrawn advisories from being unwithdrawn can now end up in scenarios where active advisories stay withdrawn for an additional run as demonstrated in the below SecurityAdvisoryResolverTest
public function testResolveUnWithdrawsWhenTheAdvisoryHoldingTheCveIsWithdrawnInTheSameRun(): void
{
$date = new \DateTimeImmutable('2024-01-01');
$droppedByTheSource = new SecurityAdvisory(
new RemoteSecurityAdvisory('ghsa-a', 'Advisory A', 'acme/package', '^1.0', 'https://example.org/a', 'CVE-2024-1111', $date, null, [], 'test', null),
'test',
);
$reportedAsLiveAgain = new SecurityAdvisory(
new RemoteSecurityAdvisory('ghsa-b', 'Advisory B', 'acme/package', '^1.0', 'https://example.org/b', 'CVE-2024-1111', $date, null, [], 'test', null),
'test',
);
$reportedAsLiveAgain->withdraw();
$collection = new RemoteSecurityAdvisoryCollection([
new RemoteSecurityAdvisory('ghsa-b', 'Advisory B', 'acme/package', '^1.0', 'https://example.org/b', 'CVE-2024-1111', $date, null, [], 'test', null),
]);
[$new, $withdrawn] = $this->resolver->resolve([$droppedByTheSource, $reportedAsLiveAgain], $collection, 'test');
$this->assertSame([], $new);
$this->assertSame([$droppedByTheSource], $withdrawn);
$this->assertTrue($droppedByTheSource->isWithdrawn());
$this->assertFalse($reportedAsLiveAgain->isWithdrawn(), 'the advisory the source reports as live must not stay withdrawn once the CVE holder is withdrawn in the same run');
}
There was a problem hiding this comment.
You're right - the first fix only covered withdrawals coming from removeWithdrawn(). In your repro the withdrawal happens inside resolve() (the advisory just drops out of the feed, no withdrawn‑map entry) after the CVE has already been reassigned to the survivor, so both the freeing UPDATE and the claiming UPDATE land in one flush and the order isn't dependency‑aware.
Fixed it by splitting the resolver into phases:
planResolve()- pure classification, no entity mutationapplyWithdrawals()- withdraw the advisories the source no longer listsapplyMatches()- apply the remote data to matches and build new advisories
The worker now flushes between applyWithdrawals() and applyMatches(), so freed (packageName, activeCve) keys are committed before anything reuses them. resolve() stays as a thin wrapper for the unit tests. The extra flush is guarded by a count check, so a run with no withdrawals is still a single flush as before.
Thanks for providing a regression test testCveReassignedFromAnAdvisoryTheSourceStoppedListing - added it together with my changes and it passes now (verified it fails without the split).
There was a problem hiding this comment.
Scenarios like these are not very likely but they can still potentially happen and cause unique constraint violations
public function testCveHandedBackToAWithdrawnAdvisoryTheSourceListsAgain(): void
{
$reinstatedFirst = new SecurityAdvisory($this->remoteAdvisory('GHSA-a1-back-0000', 'acme/swap-first', 'CVE-2024-90001'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$reinstatedFirst->withdraw();
$holderFirst = new SecurityAdvisory($this->remoteAdvisory('GHSA-b1-hold-1111', 'acme/swap-first', 'CVE-2024-90001'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$holderSecond = new SecurityAdvisory($this->remoteAdvisory('GHSA-a2-hold-2222', 'acme/swap-second', 'CVE-2024-90003'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$reinstatedSecond = new SecurityAdvisory($this->remoteAdvisory('GHSA-b2-back-3333', 'acme/swap-second', 'CVE-2024-90003'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$reinstatedSecond->withdraw();
$this->store($reinstatedFirst, $holderFirst, $holderSecond, $reinstatedSecond);
$collection = new RemoteSecurityAdvisoryCollection([
$this->remoteAdvisory('GHSA-b1-hold-1111', 'acme/swap-first', 'CVE-2024-90002'),
$this->remoteAdvisory('GHSA-a1-back-0000', 'acme/swap-first', 'CVE-2024-90001'),
$this->remoteAdvisory('GHSA-a2-hold-2222', 'acme/swap-second', 'CVE-2024-90004'),
$this->remoteAdvisory('GHSA-b2-back-3333', 'acme/swap-second', 'CVE-2024-90003'),
]);
$this->runWorkerWithSource($collection);
}
| <h4 class="font-bold"> | ||
| {% if advisory.severity %}[{{ advisory.severity.value|upper }}]{% endif %} | ||
| <a href="{{ advisory.link }}">{{ advisory.title }}</a> | ||
| {% if advisory.withdrawnAt %}<span class="label label-default">Withdrawn {{ advisory.withdrawnAt|date('Y-m-d') }}</span>{% endif %} |
There was a problem hiding this comment.
If a package has withdrawn advisories then the count on the package view page and the count here don't match anymore. Especially for packages with lots of advisories, it might be hard to spot why this is the case. Any ideas how to better surface this?
There was a problem hiding this comment.
Decided to show only active advisories.
There was a problem hiding this comment.
The reason why we no longer want to delete withdrawn advisories is so that people can still find them. Would try to find another solution here like for instance displaying them at the bottom in a separate table
- Resolver: never resurrect a withdrawn advisory via a fuzzy match, and keep it withdrawn when another active advisory already holds its CVE - Worker: flush withdrawals before resolve() reuses the freed CVE key - Audit: dedicated 'reinstated' event for un-withdrawal; drop dead preRemove - Advisories tab excludes withdrawn so its count matches the package page - Migration: mark already-sourceless advisories as withdrawn
| <h4 class="font-bold"> | ||
| {% if advisory.severity %}[{{ advisory.severity.value|upper }}]{% endif %} | ||
| <a href="{{ advisory.link }}">{{ advisory.title }}</a> | ||
| {% if advisory.withdrawnAt %}<span class="label label-default">Withdrawn {{ advisory.withdrawnAt|date('Y-m-d') }}</span>{% endif %} |
There was a problem hiding this comment.
The reason why we no longer want to delete withdrawn advisories is so that people can still find them. Would try to find another solution here like for instance displaying them at the bottom in a separate table
| if ($event->hasChangedField('withdrawnAt')) { | ||
| $this->buffered[] = null !== $event->getNewValue('withdrawnAt') | ||
| ? AuditRecord::securityAdvisoryWithdrawn($advisory, $this->getUser(), $this->getPackageId($advisory)) | ||
| : AuditRecord::securityAdvisoryUnwithdrawn($advisory, $this->getUser(), $this->getPackageId($advisory)); |
There was a problem hiding this comment.
Does unwithdrawn also need to show which fields changed to have the full audit trace?
There was a problem hiding this comment.
Fixed: AuditRecord::securityAdvisoryUnwithdrawn() now takes the Doctrine change set and stores the same changes diff as securityAdvisoryEdited, and SecurityAdvisoryUnwithdrawnDisplay / its template render it. withdrawnAt itself is filtered out of the diff (in getSecurityAdvisoryChanges(), alongside updatedAt) since the flip is implied by the record type. SecurityAdvisoryAuditRecordTest now asserts a co‑changed field shows up in the un‑withdraw record.
| $active = $this->getEM()->getRepository(SecurityAdvisory::class)->getPackageSecurityAdvisories('acme/orphan'); | ||
| $this->assertSame([], $active); | ||
| } | ||
|
|
There was a problem hiding this comment.
This only half fixed the unique constraint violation. Can still reproduce it via
public function testCveReassignedFromAnAdvisoryTheSourceStoppedListing(): void
{
$survivor = new SecurityAdvisory($this->remoteAdvisory('GHSA-survivor-0000', 'acme/dropped', 'CVE-2024-70007'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$dropped = new SecurityAdvisory($this->remoteAdvisory('GHSA-dropped-1111', 'acme/dropped', 'CVE-2024-80008'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$this->store($survivor, $dropped);
// No withdrawn map entry: GHSA-dropped-1111 is just absent from the source's response.
$collection = new RemoteSecurityAdvisoryCollection(
[$this->remoteAdvisory('GHSA-survivor-0000', 'acme/dropped', 'CVE-2024-80008')],
);
$this->runWorkerWithSource($collection);
$this->getEM()->clear();
$advisories = $this->getEM()->getRepository(SecurityAdvisory::class)->findByPackageName('acme/dropped');
$this->assertCount(2, $advisories);
$this->assertSame('CVE-2024-80008', $this->activeAdvisory($advisories)->getCve());
$this->assertSame('GHSA-survivor-0000', $this->activeAdvisory($advisories)->getRemoteId());
}
| $active = $this->getEM()->getRepository(SecurityAdvisory::class)->getPackageSecurityAdvisories('acme/orphan'); | ||
| $this->assertSame([], $active); | ||
| } | ||
|
|
There was a problem hiding this comment.
Additionally holding back withdrawn advisories from being unwithdrawn can now end up in scenarios where active advisories stay withdrawn for an additional run as demonstrated in the below SecurityAdvisoryResolverTest
public function testResolveUnWithdrawsWhenTheAdvisoryHoldingTheCveIsWithdrawnInTheSameRun(): void
{
$date = new \DateTimeImmutable('2024-01-01');
$droppedByTheSource = new SecurityAdvisory(
new RemoteSecurityAdvisory('ghsa-a', 'Advisory A', 'acme/package', '^1.0', 'https://example.org/a', 'CVE-2024-1111', $date, null, [], 'test', null),
'test',
);
$reportedAsLiveAgain = new SecurityAdvisory(
new RemoteSecurityAdvisory('ghsa-b', 'Advisory B', 'acme/package', '^1.0', 'https://example.org/b', 'CVE-2024-1111', $date, null, [], 'test', null),
'test',
);
$reportedAsLiveAgain->withdraw();
$collection = new RemoteSecurityAdvisoryCollection([
new RemoteSecurityAdvisory('ghsa-b', 'Advisory B', 'acme/package', '^1.0', 'https://example.org/b', 'CVE-2024-1111', $date, null, [], 'test', null),
]);
[$new, $withdrawn] = $this->resolver->resolve([$droppedByTheSource, $reportedAsLiveAgain], $collection, 'test');
$this->assertSame([], $new);
$this->assertSame([$droppedByTheSource], $withdrawn);
$this->assertTrue($droppedByTheSource->isWithdrawn());
$this->assertFalse($reportedAsLiveAgain->isWithdrawn(), 'the advisory the source reports as live must not stay withdrawn once the CVE holder is withdrawn in the same run');
}
- Resolver split into planResolve/applyWithdrawals/applyMatches so withdrawals flush before any advisory reuses a freed (packageName, cve) key, fixing the constraint violation when the source drops an advisory it reassigned the CVE from - Un-withdrawn advisories re-match in the same run once the CVE holder is withdrawn - Un-withdraw audit record now carries the field changeset - Withdrawn advisories shown in a collapsible section on the package advisories tab - Severity rendered as a coloured label instead of [BRACKETS]
…e-cves # Conflicts: # css/app.scss # tests/Controller/PackageControllerTest.php
glaubinix
left a comment
There was a problem hiding this comment.
The more I like at the PR the more things I noticed that don't fully work as expected. I think generally, if you are using AI to write the changes it would be helpful to have AI run a few rounds of review and verify that it covers any (additional) edge cases.
For instance:
Given that you have an advisory that is in two difference sources. The advisory is then withdrawn from one source but not the other. Based on what I see in the code it looks like you want to remove one of the sources? Based on the test below this doesn't happen yet. Overall unsure if this is a good idea. I think it might be better to store on the source when it was withdrawn, then we can display this in the UI that the advisory was withdrawn in source X but not source Y. This way we truly never delete any data. I believe this info is also missing from the audit log.
public function testDetachedSourceIsRemovedFromTheDatabase(): void
{
$advisory = new SecurityAdvisory($this->remoteAdvisory('GHSA-shared-0000', 'acme/two-sources', 'CVE-2024-12001'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$advisory->addSource('shared/2024/12001', FriendsOfPhpSecurityAdvisoriesSource::SOURCE_NAME, null);
$this->store($advisory);
// GitHub withdraws its copy while FriendsOfPHP keeps listing the advisory.
$this->runWorkerWithSource(new RemoteSecurityAdvisoryCollection([], ['acme/two-sources' => ['GHSA-shared-0000' => true]]));
$this->getEM()->clear();
$advisories = $this->getEM()->getRepository(SecurityAdvisory::class)->findByPackageName('acme/two-sources');
$this->assertCount(1, $advisories);
$this->assertFalse($advisories[0]->isWithdrawn());
$this->assertNull($advisories[0]->getSourceRemoteId(GitHubSecurityAdvisoriesSource::SOURCE_NAME), 'the detached source must be gone from the database, not only from the entity');
$this->assertCount(1, $advisories[0]->getSources());
}
| $active = $this->getEM()->getRepository(SecurityAdvisory::class)->getPackageSecurityAdvisories('acme/orphan'); | ||
| $this->assertSame([], $active); | ||
| } | ||
|
|
There was a problem hiding this comment.
Scenarios like these are not very likely but they can still potentially happen and cause unique constraint violations
public function testCveHandedBackToAWithdrawnAdvisoryTheSourceListsAgain(): void
{
$reinstatedFirst = new SecurityAdvisory($this->remoteAdvisory('GHSA-a1-back-0000', 'acme/swap-first', 'CVE-2024-90001'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$reinstatedFirst->withdraw();
$holderFirst = new SecurityAdvisory($this->remoteAdvisory('GHSA-b1-hold-1111', 'acme/swap-first', 'CVE-2024-90001'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$holderSecond = new SecurityAdvisory($this->remoteAdvisory('GHSA-a2-hold-2222', 'acme/swap-second', 'CVE-2024-90003'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$reinstatedSecond = new SecurityAdvisory($this->remoteAdvisory('GHSA-b2-back-3333', 'acme/swap-second', 'CVE-2024-90003'), GitHubSecurityAdvisoriesSource::SOURCE_NAME);
$reinstatedSecond->withdraw();
$this->store($reinstatedFirst, $holderFirst, $holderSecond, $reinstatedSecond);
$collection = new RemoteSecurityAdvisoryCollection([
$this->remoteAdvisory('GHSA-b1-hold-1111', 'acme/swap-first', 'CVE-2024-90002'),
$this->remoteAdvisory('GHSA-a1-back-0000', 'acme/swap-first', 'CVE-2024-90001'),
$this->remoteAdvisory('GHSA-a2-hold-2222', 'acme/swap-second', 'CVE-2024-90004'),
$this->remoteAdvisory('GHSA-b2-back-3333', 'acme/swap-second', 'CVE-2024-90003'),
]);
$this->runWorkerWithSource($collection);
}
|
Thank you @glaubinix for your review again. When I picked up this ticket, I was under impression it will be a simple change but now that you shared some of the possible use and edge cases, it is definitely not a straightforward feature.
Because of this, I had to also add new events when a new source reports the advisory and withdraws it. This allows more verbose audit log and also presenting both publish and withdraw date for sources and advisories itself, including display of partially withdrawn advisories. |
|
@IonBazan there is a reason, we haven't tackled this yet :) I haven't reviewed your latest changes but overall, the PR has grown significantly for something that is not going to happen very often. I would maybe pause work on this for now. I'll check with the rest of the team to see which problems we want to solve here and if there maybe is another way to address those problems. |
|
Understandable, I was also surprised to see 2k+ diff for something that sounded like adding a column 😂 |

Security advisories withdrawn at the source (GitHub) or no longer reported by it (e.g. removed from FriendsOfPHP) were being hard-deleted, losing all history. This keeps them in the database and marks them with a
withdrawnAttimestamp instead.Changes:
SecurityAdvisory: new nullablewithdrawnAt+withdraw()/isWithdrawn(). Re-matching a withdrawn advisory against live source data clears it automatically.SecurityAdvisoryResolver/SecurityAdvisoryWorker: both removal paths now mark advisories withdrawn instead of deleting the entity and its sources; worker no longer needs the two-phase delete/flush.SecurityAdvisoryRepository: active-listing queries (composer audit/API, metadata dumper) now exclude withdrawn advisories; package/advisory detail pages still show them (with a "Withdrawn" badge) for historical lookups.SecurityAdvisoryAuditListener: withdrawal audit record now fires on update instead of delete.(packageName, cve)uniqueness via a generatedactiveCvecolumn (NULLwhen withdrawn) so a withdrawn row no longer blocks a later advisory from reusing the same CVE.Fixes #1770