diff --git a/CHANGELOG.md b/CHANGELOG.md index ca491977..37bc7d2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- [#175](https://github.com/itk-dev/devops_itksites/pull/175) + Store long advisory constraints, titles and links in full, so one long + advisory no longer blocks the others, and mark detection results with the + advisories they could not store - [#174](https://github.com/itk-dev/devops_itksites/pull/174) Show detection result errors and warnings as readable messages instead of JSON, so long messages are no longer cut off diff --git a/migrations/Version20261104214553.php b/migrations/Version20261104214553.php new file mode 100644 index 00000000..c37cea76 --- /dev/null +++ b/migrations/Version20261104214553.php @@ -0,0 +1,36 @@ +addSql('ALTER TABLE advisory CHANGE affected_versions affected_versions LONGTEXT NOT NULL, CHANGE title title LONGTEXT NOT NULL, CHANGE link link LONGTEXT NOT NULL'); + // Existing rows get an empty list before the column becomes NOT NULL. + $this->addSql('ALTER TABLE detection_result ADD processing_errors JSON DEFAULT NULL'); + $this->addSql("UPDATE detection_result SET processing_errors = '[]'"); + $this->addSql('ALTER TABLE detection_result MODIFY processing_errors JSON NOT NULL'); + } + + public function down(Schema $schema): void + { + $this->addSql('ALTER TABLE advisory CHANGE affected_versions affected_versions VARCHAR(500) NOT NULL, CHANGE title title VARCHAR(255) NOT NULL, CHANGE link link VARCHAR(255) NOT NULL'); + $this->addSql('ALTER TABLE detection_result DROP processing_errors'); + } + + public function isTransactional(): bool + { + return false; + } +} diff --git a/src/Controller/Admin/DetectionResultCrudController.php b/src/Controller/Admin/DetectionResultCrudController.php index dc085e8f..e39143a6 100644 --- a/src/Controller/Admin/DetectionResultCrudController.php +++ b/src/Controller/Admin/DetectionResultCrudController.php @@ -59,6 +59,8 @@ public function configureFields(string $pageName): iterable ->setTemplatePath('EasyAdminBundle/Fields/harvester_messages.html.twig'); yield Field::new('warnings')->onlyOnDetail() ->setTemplatePath('EasyAdminBundle/Fields/harvester_messages.html.twig'); + yield Field::new('processingErrors')->onlyOnDetail() + ->setTemplatePath('EasyAdminBundle/Fields/sources.html.twig'); yield Field::new('prettyData')->onlyOnDetail()->setLabel('Data') ->setTemplatePath('EasyAdminBundle/Fields/code_highlight.html.twig') ->setCustomOption('language', 'json'); diff --git a/src/Entity/Advisory.php b/src/Entity/Advisory.php index 9811edc4..47d76d88 100644 --- a/src/Entity/Advisory.php +++ b/src/Entity/Advisory.php @@ -9,6 +9,7 @@ use Doctrine\Common\Collections\Collection; use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping as ORM; +use Symfony\Component\Validator\Constraints as Assert; #[ORM\Entity(repositoryClass: AdvisoryRepository::class)] class Advisory extends AbstractBaseEntity implements \Stringable @@ -20,18 +21,20 @@ class Advisory extends AbstractBaseEntity implements \Stringable private const string DRUPAL_ADVISORY_URL_PATTERN = 'https://www.drupal.org/%s'; #[ORM\Column(length: 255, unique: true)] + #[Assert\Length(max: 255)] private ?string $advisoryId = null; - #[ORM\Column(length: 500)] + #[ORM\Column(type: Types::TEXT)] private ?string $affectedVersions = null; - #[ORM\Column(length: 255)] + #[ORM\Column(type: Types::TEXT)] private ?string $title = null; #[ORM\Column(length: 50, nullable: true)] + #[Assert\Length(max: 50)] private ?string $cve = null; - #[ORM\Column(length: 255)] + #[ORM\Column(type: Types::TEXT)] private ?string $link = null; #[ORM\Column] diff --git a/src/Entity/DetectionResult.php b/src/Entity/DetectionResult.php index cae5cc51..3810c949 100644 --- a/src/Entity/DetectionResult.php +++ b/src/Entity/DetectionResult.php @@ -108,6 +108,14 @@ class DetectionResult extends AbstractBaseEntity implements \Stringable )] private array $warnings = []; + /** + * Why parts of the result could not be processed, e.g. advisories that were skipped. Set by ITKsites, not the harvester. + * + * @var list + */ + #[ORM\Column(type: Types::JSON)] + private array $processingErrors = []; + #[ORM\Column(type: Types::STRING, length: 255, unique: true)] private string $hash; @@ -219,6 +227,27 @@ public function setWarnings(array $warnings): self return $this; } + /** @return list */ + public function getProcessingErrors(): array + { + return $this->processingErrors; + } + + /** @param list $processingErrors */ + public function setProcessingErrors(array $processingErrors): self + { + $this->processingErrors = $processingErrors; + + return $this; + } + + public function addProcessingError(string $processingError): self + { + $this->processingErrors[] = $processingError; + + return $this; + } + public function getHash(): ?string { return $this->hash; diff --git a/src/Handler/DockerImageHandler.php b/src/Handler/DockerImageHandler.php index e6b64132..2c55c76e 100644 --- a/src/Handler/DockerImageHandler.php +++ b/src/Handler/DockerImageHandler.php @@ -61,7 +61,9 @@ public function handleResult(DetectionResult $detectionResult): void $this->packageVersionFactory->setPackageVersions($site->getInstallation(), $container->packages->installed); } if (isset($container->audit) && is_object($container->audit)) { - $this->advisoryFactory->setAdvisories($site->getInstallation(), $container->audit); + foreach ($this->advisoryFactory->setAdvisories($site->getInstallation(), $container->audit) as $skipped) { + $detectionResult->addProcessingError($skipped); + } } if (isset($container->drupal) && is_object($container->drupal)) { $this->moduleVersionFactory->setModuleVersions($site->getInstallation(), $container->drupal); diff --git a/src/MessageHandler/ProcessDetectionResultHandler.php b/src/MessageHandler/ProcessDetectionResultHandler.php index a403a936..bed5c13b 100644 --- a/src/MessageHandler/ProcessDetectionResultHandler.php +++ b/src/MessageHandler/ProcessDetectionResultHandler.php @@ -26,6 +26,9 @@ public function __invoke(ProcessDetectionResult $message): void $detectionResult = $this->detectionResultRepository->find($message->detectionResultId); if (null !== $detectionResult) { + // Each run records its own problems, so a replay shows the current state. + $detectionResult->setProcessingErrors([]); + /** @var DetectionResultHandlerInterface $handler */ foreach ($this->resultHandlers as $handler) { if ($handler->supportsType($detectionResult->getType())) { diff --git a/src/Service/AdvisoryFactory.php b/src/Service/AdvisoryFactory.php index beb6ccdf..7530fb01 100644 --- a/src/Service/AdvisoryFactory.php +++ b/src/Service/AdvisoryFactory.php @@ -12,6 +12,7 @@ use Composer\Semver\Semver; use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; +use Symfony\Component\Validator\Validator\ValidatorInterface; class AdvisoryFactory { @@ -19,11 +20,16 @@ public function __construct( private readonly EntityManagerInterface $entityManager, private readonly AdvisoryRepository $advisoryRepository, private readonly LoggerInterface $logger, + private readonly ValidatorInterface $validator, ) { } - public function setAdvisories(Installation $installation, object $audit): void + /** + * @return list why advisories of the audit were skipped, for the detection result + */ + public function setAdvisories(Installation $installation, object $audit): array { + $skipped = []; if (isset($audit->advisories) && is_object($audit->advisories)) { foreach ($audit->advisories as $package => $advisories) { foreach ($advisories as $advisory) { @@ -36,15 +42,13 @@ public function setAdvisories(Installation $installation, object $audit): void 'package' => $advisory->packageName, 'installation' => (string) $installation, ]); + $skipped[] = sprintf('Skipped advisory %s for %s, which the installation does not list', $advisory->advisoryId, $advisory->packageName); continue; } $entity = $this->advisoryRepository->findOneBy(['advisoryId' => $advisory->advisoryId]); if (null === $entity) { $entity = new Advisory(); - $this->entityManager->persist($entity); - - $entity->addPackageVersion($packageVersion); if (isset($advisory->advisoryId)) { $entity->setAdvisoryId($advisory->advisoryId); @@ -55,9 +59,6 @@ public function setAdvisories(Installation $installation, object $audit): void if (isset($advisory->title)) { $entity->setTitle($advisory->title); } - if (isset($advisory->title)) { - $entity->setTitle($advisory->title); - } if (isset($advisory->cve)) { $entity->setCve($advisory->cve); } @@ -75,12 +76,44 @@ public function setAdvisories(Installation $installation, object $audit): void if (!empty($advisory->sources)) { $entity->setSources($advisory->sources); } + + // Persisted with the rest of the result, so one that does not fit would fail them all. + $reason = $this->unstorableReason($entity); + if (null !== $reason) { + $skipped[] = $reason; + continue; + } + + $this->entityManager->persist($entity); + $entity->addPackageVersion($packageVersion); } $this->setAdvisoryForAffectedVersions($packageVersion->getPackage(), $entity); } } } + + return $skipped; + } + + /** + * Why the advisory does not fit the database, or null when it does. Only + * the advisory id and CVE have a fixed size, so only values that cannot be + * real ids are refused. Logged as an error: prod writes nothing below it. + */ + public function unstorableReason(Advisory $advisory): ?string + { + $violations = $this->validator->validate($advisory); + if (0 === count($violations)) { + return null; + } + + $this->logger->error('Skipped advisory {advisory}, which does not fit the database: {violations}', [ + 'advisory' => $advisory->getAdvisoryId(), + 'violations' => (string) $violations, + ]); + + return sprintf('Skipped advisory %s, which does not fit the database: %s', $advisory->getAdvisoryId(), trim((string) $violations)); } private function getInstalledPackageVersion(Installation $installation, string $vendorPackage): ?PackageVersion diff --git a/src/Service/Drupal/ReleaseChecker.php b/src/Service/Drupal/ReleaseChecker.php index 8df04a19..14239352 100644 --- a/src/Service/Drupal/ReleaseChecker.php +++ b/src/Service/Drupal/ReleaseChecker.php @@ -64,7 +64,9 @@ public function check(Package $package): void $constraint = DrupalVersion::constraintToComposer($securityAdvisory->affectedVersions); if ($this->matchesAny($insecureVersions, $constraint)) { $advisory = $this->upsert($package, $securityAdvisory, $constraint); - $this->advisoryFactory->setAdvisoryForAffectedVersions($package, $advisory); + if (null !== $advisory) { + $this->advisoryFactory->setAdvisoryForAffectedVersions($package, $advisory); + } } } } @@ -90,14 +92,13 @@ private function matchesAny(array $versions, string $constraint): bool return false; } - private function upsert(Package $package, SecurityAdvisory $securityAdvisory, string $constraint): Advisory + /** + * Null when the advisory does not fit the database; flushing it would fail the whole package. + */ + private function upsert(Package $package, SecurityAdvisory $securityAdvisory, string $constraint): ?Advisory { - $advisory = $this->advisoryRepository->findOneBy(['advisoryId' => $securityAdvisory->advisoryId]); - if (null === $advisory) { - $advisory = new Advisory()->setAdvisoryId($securityAdvisory->advisoryId); - $this->entityManager->persist($advisory); - $package->addAdvisory($advisory); - } + $existing = $this->advisoryRepository->findOneBy(['advisoryId' => $securityAdvisory->advisoryId]); + $advisory = $existing ?? new Advisory()->setAdvisoryId($securityAdvisory->advisoryId); $advisory ->setAffectedVersions($constraint) @@ -109,6 +110,20 @@ private function upsert(Package $package, SecurityAdvisory $securityAdvisory, st $advisory->setCve($securityAdvisory->cve); } + if (null !== $this->advisoryFactory->unstorableReason($advisory)) { + if (null !== $existing) { + // Drop the changes so the stored advisory is flushed unchanged. + $this->entityManager->refresh($existing); + } + + return null; + } + + if (null === $existing) { + $this->entityManager->persist($advisory); + $package->addAdvisory($advisory); + } + return $advisory; } } diff --git a/tests/Handler/DockerImageHandlerTest.php b/tests/Handler/DockerImageHandlerTest.php index f39fd5c7..735aacc9 100644 --- a/tests/Handler/DockerImageHandlerTest.php +++ b/tests/Handler/DockerImageHandlerTest.php @@ -4,11 +4,14 @@ namespace App\Tests\Handler; +use App\Entity\Advisory; use App\Entity\DetectionResult; use App\Entity\Installation; use App\Entity\PackageVersion; use App\Entity\Server; use App\Handler\DockerImageHandler; +use App\Message\ProcessDetectionResult; +use App\MessageHandler\ProcessDetectionResultHandler; use App\Types\DetectionType; use Doctrine\ORM\EntityManagerInterface; use Hautelook\AliceBundle\PhpUnit\RefreshDatabaseTrait; @@ -43,12 +46,54 @@ public function testPackagesWinOverModules(): void $this->assertSame(['6.2.9'], $this->installedVersions()); } + /** + * An advisory that cannot be stored must not fail the others, and the + * result must say so instead of only logging it. Reprocessing clears it. + */ + public function testSkippedAdvisoriesMarkTheDetectionResult(): void + { + $advisory = static fn (string $id, string $cve): array => [ + 'advisoryId' => $id, 'packageName' => 'acme/audit_probe', 'affectedVersions' => '<2.0.0', 'title' => 'Probe', + 'cve' => $cve, 'link' => 'https://example.com/'.$id, 'reportedAt' => '2026-11-04T00:00:00+00:00', 'sources' => [], + ]; + $detectionResult = $this->persist([ + 'domain' => 'audit-probe.example.com', + 'containers' => [[ + 'name' => 'phpfpm', + 'image' => 'itkdev/php8.4-fpm', + 'packages' => ['installed' => [['name' => 'acme/audit_probe', 'description' => 'Probe', 'version' => '1.0.0']]], + 'audit' => ['advisories' => ['acme/audit_probe' => [ + $advisory('PKSA-unfit-probe', 'CVE-'.str_repeat('9', 50)), + $advisory('PKSA-fit-probe', 'CVE-2026-1'), + ]]], + 'drupal' => 'unknown', + 'symfony' => 'unknown', + ]], + ]); + $process = self::getContainer()->get(ProcessDetectionResultHandler::class); + + $process(new ProcessDetectionResult($detectionResult->getId())); + + $stored = $this->entityManager->getRepository(DetectionResult::class)->find($detectionResult->getId()); + $this->assertCount(1, $stored->getProcessingErrors()); + $this->assertStringContainsString('PKSA-unfit-probe', $stored->getProcessingErrors()[0]); + $this->assertNotNull($this->entityManager->getRepository(Advisory::class)->findOneBy(['advisoryId' => 'PKSA-fit-probe'])); + $this->assertNull($this->entityManager->getRepository(Advisory::class)->findOneBy(['advisoryId' => 'PKSA-unfit-probe'])); + + // Reprocessing once the audit no longer has the unfit advisory clears the mark. + $stored->setData(str_replace('PKSA-unfit-probe', 'PKSA-fit-probe', $stored->getData())); + $stored->setData((string) preg_replace('/"cve":"CVE-9+"/', '"cve":"CVE-2026-1"', $stored->getData())); + $this->entityManager->flush(); + $process(new ProcessDetectionResult($detectionResult->getId())); + + $this->assertSame([], $this->entityManager->getRepository(DetectionResult::class)->find($detectionResult->getId())->getProcessingErrors()); + } + /** * @param array|string $packages */ private function handle(array|string $packages, string $moduleVersion): void { - $server = $this->entityManager->getRepository(Server::class)->findOneBy([]); $data = [ 'domain' => 'fallback-probe.example.com', 'containers' => [[ @@ -61,18 +106,28 @@ private function handle(array|string $packages, string $moduleVersion): void ]], ]; + $detectionResult = $this->persist($data); + + self::getContainer()->get(DockerImageHandler::class)->handleResult($detectionResult); + $this->entityManager->flush(); + } + + /** + * @param array $data + */ + private function persist(array $data): DetectionResult + { $detectionResult = new DetectionResult(); $detectionResult->setType(DetectionType::DOCKER); $detectionResult->setRootDir(self::ROOT_DIR); - $detectionResult->setServer($server); + $detectionResult->setServer($this->entityManager->getRepository(Server::class)->findOneBy([])); $detectionResult->setData(json_encode($data, JSON_THROW_ON_ERROR)); $detectionResult->generateHash(); $detectionResult->setLastContact(); $this->entityManager->persist($detectionResult); $this->entityManager->flush(); - self::getContainer()->get(DockerImageHandler::class)->handleResult($detectionResult); - $this->entityManager->flush(); + return $detectionResult; } /** diff --git a/tests/Service/AdvisoryFactoryTest.php b/tests/Service/AdvisoryFactoryTest.php index d2ba1801..24aae807 100644 --- a/tests/Service/AdvisoryFactoryTest.php +++ b/tests/Service/AdvisoryFactoryTest.php @@ -13,6 +13,7 @@ use Hautelook\AliceBundle\PhpUnit\RefreshDatabaseTrait; use Psr\Log\LoggerInterface; use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase; +use Symfony\Component\Validator\Validator\ValidatorInterface; /** * Runs the real factory against the test database. @@ -40,9 +41,9 @@ public function testSkipsAdvisoriesForUnlistedPackages(): void $logger->expects(self::once())->method('warning')->with(self::anything(), self::callback( static fn (array $context): bool => 'acme/unlisted_probe' === $context['package'] && 'PKSA-unlisted-probe' === $context['advisory'] && isset($context['installation']) )); - $factory = new AdvisoryFactory($entityManager, self::getContainer()->get(AdvisoryRepository::class), $logger); + $factory = new AdvisoryFactory($entityManager, self::getContainer()->get(AdvisoryRepository::class), $logger, self::getContainer()->get(ValidatorInterface::class)); - $factory->setAdvisories($installation, (object) ['advisories' => (object) [ + $skipped = $factory->setAdvisories($installation, (object) ['advisories' => (object) [ 'acme/unlisted_probe' => [$this->advisory('PKSA-unlisted-probe', 'acme/unlisted_probe')], 'acme/listed_probe' => [$this->advisory('PKSA-listed-probe', 'acme/listed_probe')], ]]); @@ -50,9 +51,77 @@ public function testSkipsAdvisoriesForUnlistedPackages(): void $advisories = $entityManager->getRepository(Advisory::class); $this->assertNull($advisories->findOneBy(['advisoryId' => 'PKSA-unlisted-probe'])); + $this->assertCount(1, $skipped); + $this->assertStringContainsString('PKSA-unlisted-probe', $skipped[0]); $this->assertCount(1, $advisories->findOneBy(['advisoryId' => 'PKSA-listed-probe'])->getPackageVersions()); } + /** + * Symfony lists every affected minor back to 2.0; PKSA-b2gg-vk1g-6d5q's + * constraint is 543 characters and failed the flush of the whole result. + * Free-text fields are stored in full rather than skipped. + */ + public function testStoresLongAffectedVersions(): void + { + self::bootKernel(); + $entityManager = self::getContainer()->get(EntityManagerInterface::class); + $installation = $entityManager->getRepository(Installation::class)->findOneBy([]); + self::getContainer()->get(PackageVersionFactory::class)->setPackageVersions($installation, [ + (object) ['name' => 'acme/long_probe', 'description' => 'Probe', 'version' => '1.0.0'], + ]); + $affectedVersions = implode('|', array_map(static fn (int $minor): string => sprintf('>=1.%d.0,<1.%d.0', $minor, $minor + 1), range(0, 79))); + $this->assertGreaterThan(1000, strlen($affectedVersions)); + + $advisory = $this->advisory('PKSA-long-probe', 'acme/long_probe'); + $advisory->affectedVersions = $affectedVersions; + $advisory->title = str_repeat('Long title ', 50); + $advisory->link = 'https://example.com/'.str_repeat('a', 500); + $factory = new AdvisoryFactory($entityManager, self::getContainer()->get(AdvisoryRepository::class), $this->createStub(LoggerInterface::class), self::getContainer()->get(ValidatorInterface::class)); + $factory->setAdvisories($installation, (object) ['advisories' => (object) ['acme/long_probe' => [$advisory]]]); + $entityManager->flush(); + $entityManager->clear(); + + $stored = $entityManager->getRepository(Advisory::class)->findOneBy(['advisoryId' => 'PKSA-long-probe']); + $this->assertSame($affectedVersions, $stored->getAffectedVersions()); + $this->assertSame($advisory->title, $stored->getTitle()); + $this->assertSame($advisory->link, $stored->getLink()); + $this->assertCount(1, $stored->getPackageVersions()); + } + + /** + * The advisories of a result are flushed together, so one whose id or CVE + * cannot fit must be skipped instead of failing the others, and logged as + * an error, the lowest level prod writes. + */ + public function testSkipsAdvisoriesThatDoNotFitAndStoresTheRest(): void + { + self::bootKernel(); + $entityManager = self::getContainer()->get(EntityManagerInterface::class); + $installation = $entityManager->getRepository(Installation::class)->findOneBy([]); + self::getContainer()->get(PackageVersionFactory::class)->setPackageVersions($installation, [ + (object) ['name' => 'acme/fit_probe', 'description' => 'Probe', 'version' => '1.0.0'], + ]); + + $tooLong = $this->advisory('PKSA-too-long-probe', 'acme/fit_probe'); + $tooLong->cve = 'CVE-'.str_repeat('9', 50); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects(self::once())->method('error')->with(self::anything(), self::callback( + static fn (array $context): bool => 'PKSA-too-long-probe' === $context['advisory'] && str_contains($context['violations'], 'cve') + )); + $factory = new AdvisoryFactory($entityManager, self::getContainer()->get(AdvisoryRepository::class), $logger, self::getContainer()->get(ValidatorInterface::class)); + + $skipped = $factory->setAdvisories($installation, (object) ['advisories' => (object) [ + 'acme/fit_probe' => [$tooLong, $this->advisory('PKSA-fits-probe', 'acme/fit_probe')], + ]]); + $entityManager->flush(); + + $advisories = $entityManager->getRepository(Advisory::class); + $this->assertNull($advisories->findOneBy(['advisoryId' => 'PKSA-too-long-probe'])); + $this->assertCount(1, $skipped); + $this->assertStringContainsString('PKSA-too-long-probe', $skipped[0]); + $this->assertCount(1, $advisories->findOneBy(['advisoryId' => 'PKSA-fits-probe'])->getPackageVersions()); + } + private function advisory(string $advisoryId, string $packageName): object { return (object) [