From d93746bb3d70db59d9aa804d3befe9fcd6e1d2ab Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Tue, 4 Aug 2026 12:39:15 +0200 Subject: [PATCH] [BUGFIX] Quarantine the domain the request was blocked on A quarantined rendering recorded the host of the clone url, while the check that blocked it keys on the host of the composer.json url. For Github those never match: the repository is on github.com, the composer.json is fetched from raw.githubusercontent.com. Approving a quarantined entry allowlists exactly the recorded domain and then replays every entry sharing it. With the wrong domain recorded, the replay is checked against a host that was never approved, so it is quarantined again and the allowlist gains an entry for a domain that is never consulted. Record the host the check actually rejected, which the exception already carries. updateLastHit() had the same mismatch and could therefore never find the row it meant to touch. The feature shipped in 7.2.0, so entries recorded before this fix carry the clone host. Recompute those from the push event each row already stores. Rows whose payload can not be read are left alone, and the checksum does not cover the domain, so deduplication is unaffected. Signed-off-by: Sebastian Mendel Approving a domain replays every entry it holds, and until now a single entry that can not be rendered, an irrelevant branch name for instance, aborted the whole run and left the rest of the queue behind. Skip such an entry and tell the admin how many were dropped. Signed-off-by: Sebastian Mendel --- migrations/Version20260804120000.php | 54 +++++++++++ .../Docs/KnownRepositoryDomainsController.php | 9 +- .../QuarantinedDocumentationsController.php | 14 ++- .../DocumentationQuarantineService.php | 10 +- src/Service/RenderDocumentationService.php | 4 +- .../DocumentationQuarantineServiceTest.php | 53 +++++++++++ .../RenderDocumentationServiceTest.php | 91 +++++++++++++++++++ 7 files changed, 228 insertions(+), 7 deletions(-) create mode 100644 migrations/Version20260804120000.php create mode 100644 tests/Unit/Service/DocumentationQuarantineServiceTest.php create mode 100644 tests/Unit/Service/RenderDocumentationServiceTest.php diff --git a/migrations/Version20260804120000.php b/migrations/Version20260804120000.php new file mode 100644 index 0000000..044b65e --- /dev/null +++ b/migrations/Version20260804120000.php @@ -0,0 +1,54 @@ +connection->fetchAllAssociative('SELECT id, domain, serialized_push_event FROM documentation_quarantine'); + foreach ($rows as $row) { + try { + $pushEvent = json_decode((string) $row['serialized_push_event'], true, 512, JSON_THROW_ON_ERROR); + } catch (\JsonException) { + continue; + } + $urlToComposerFile = (string) ($pushEvent['urlToComposerFile'] ?? ''); + if ('' === $urlToComposerFile) { + continue; + } + $domain = RepositoryUrlUtility::getNormalizedDomain($urlToComposerFile); + if ('' === $domain || $domain === $row['domain']) { + continue; + } + $this->connection->update('documentation_quarantine', ['domain' => $domain], ['id' => $row['id']]); + } + } + + public function down(Schema $schema): void + { + $this->throwIrreversibleMigrationException('The previously stored domain can not be restored, it was derived from the clone url.'); + } + + public function isTransactional(): bool + { + return false; + } +} diff --git a/src/Controller/AdminInterface/Docs/KnownRepositoryDomainsController.php b/src/Controller/AdminInterface/Docs/KnownRepositoryDomainsController.php index a275f96..b6f9b93 100644 --- a/src/Controller/AdminInterface/Docs/KnownRepositoryDomainsController.php +++ b/src/Controller/AdminInterface/Docs/KnownRepositoryDomainsController.php @@ -13,6 +13,7 @@ use App\Entity\KnownRepositoryDomain; use App\Enum\DocumentationRenderingTrigger; +use App\Exception\DocumentationRenderingRequestDeclinedException; use App\Form\KnownDomainCreateType; use App\Form\KnownDomainDeleteType; use App\Repository\KnownRepositoryDomainRepository; @@ -69,7 +70,13 @@ public function new(Request $request): Response if ($data->isAllowed()) { foreach ($this->documentationQuarantineService->findAllByDomain($data->getDomain()) as $documentationQuarantine) { $pushEvent = $documentationQuarantine->getPushEvent(); - $this->renderDocumentationService->requestDocumentationRendering($pushEvent, DocumentationRenderingTrigger::WEB); + try { + $this->renderDocumentationService->requestDocumentationRendering($pushEvent, DocumentationRenderingTrigger::WEB); + } catch (DocumentationRenderingRequestDeclinedException) { + // An entry can be undeployable for reasons that have nothing to + // do with the domain, an irrelevant branch name for instance. + // Keep going, one such entry must not stop the others. + } $this->entityManager->remove($documentationQuarantine); } diff --git a/src/Controller/AdminInterface/Docs/QuarantinedDocumentationsController.php b/src/Controller/AdminInterface/Docs/QuarantinedDocumentationsController.php index 31829c9..1872be2 100644 --- a/src/Controller/AdminInterface/Docs/QuarantinedDocumentationsController.php +++ b/src/Controller/AdminInterface/Docs/QuarantinedDocumentationsController.php @@ -15,6 +15,7 @@ use App\Entity\KnownRepositoryDomain; use App\Enum\DocumentationRenderingTrigger; use App\Enum\RepositoryDomainStatus; +use App\Exception\DocumentationRenderingRequestDeclinedException; use App\Form\QuarantinedDocumentationAllowType; use App\Form\QuarantinedDocumentationDeleteType; use App\Form\QuarantinedDocumentationDisallowType; @@ -70,15 +71,26 @@ public function allow(Request $request, DocumentationQuarantine $quarantinedDocu $this->entityManager->persist($knownRepositoryDomain); $this->entityManager->flush(); + $declined = 0; foreach ($this->documentationQuarantineService->findAllByDomain($domain) as $documentationQuarantine) { $pushEvent = $documentationQuarantine->getPushEvent(); - $this->renderDocumentationService->requestDocumentationRendering($pushEvent, DocumentationRenderingTrigger::WEB); + try { + $this->renderDocumentationService->requestDocumentationRendering($pushEvent, DocumentationRenderingTrigger::WEB); + } catch (DocumentationRenderingRequestDeclinedException) { + // An entry can be undeployable for reasons that have nothing to do + // with the domain, an irrelevant branch name for instance. Keep + // going, one such entry must not stop the others. + ++$declined; + } $this->entityManager->remove($documentationQuarantine); } $this->entityManager->flush(); $this->addFlash('success', sprintf('The domain %s has been allowed and all quarantined renderings have been activated.', $domain)); + if ($declined > 0) { + $this->addFlash('warning', sprintf('%d of them could not be rendered and were discarded, see the rendering history for the reason.', $declined)); + } return $this->redirectToRoute('admin_docs_quarantine_index'); } diff --git a/src/Service/DocumentationQuarantineService.php b/src/Service/DocumentationQuarantineService.php index 2a70aa8..c729865 100644 --- a/src/Service/DocumentationQuarantineService.php +++ b/src/Service/DocumentationQuarantineService.php @@ -14,7 +14,6 @@ use App\Entity\DocumentationQuarantine; use App\Extractor\PushEvent; use App\Repository\DocumentationQuarantineRepository; -use App\Utility\RepositoryUrlUtility; use Doctrine\ORM\EntityManagerInterface; class DocumentationQuarantineService @@ -32,10 +31,15 @@ public function isQuarantined(PushEvent $pushEvent): bool ]); } - public function quarantine(PushEvent $pushEvent): DocumentationQuarantine + /** + * The domain has to be the one the request was actually blocked on, which is + * the host of the composer.json url. It differs from the host of the clone url + * for Github always, and can differ for the other services as well. + */ + public function quarantine(PushEvent $pushEvent, string $blockedDomain): DocumentationQuarantine { $documentationQuarantine = (new DocumentationQuarantine()) - ->setDomain(RepositoryUrlUtility::getNormalizedDomain($pushEvent->getRepositoryUrl())) + ->setDomain($blockedDomain) ->setSerializedPushEvent(json_encode($pushEvent, JSON_THROW_ON_ERROR)) ->setChecksum($this->hash($pushEvent)); diff --git a/src/Service/RenderDocumentationService.php b/src/Service/RenderDocumentationService.php index bdaa161..78e93bd 100644 --- a/src/Service/RenderDocumentationService.php +++ b/src/Service/RenderDocumentationService.php @@ -54,12 +54,12 @@ public function requestDocumentationRendering(PushEvent $pushEvent, Documentatio $userIdentifier = $this->security->getUser() instanceof KeyCloakUser ? $this->security->getUser()->getDisplayName() : 'Anon.'; try { - $this->documentationBuildInformationService->updateLastHit(RepositoryUrlUtility::getNormalizedDomain($pushEvent->getRepositoryUrl())); + $this->documentationBuildInformationService->updateLastHit(RepositoryUrlUtility::getNormalizedDomain($pushEvent->getUrlToComposerFile())); $composerJson = $this->documentationBuildInformationService->fetchRemoteComposerJson($pushEvent->getUrlToComposerFile()); } catch (UnknownComposerJsonUrlException $e) { if (!$this->documentationQuarantineService->isQuarantined($pushEvent)) { - $documentationQuarantine = $this->documentationQuarantineService->quarantine($pushEvent); + $documentationQuarantine = $this->documentationQuarantineService->quarantine($pushEvent, $e->normalizedHost); $this->documentationBuildInformationService->notifyAboutUnknownRepositoryDomain($documentationQuarantine); $this->historyService->writeHistory(new HistoryEntryDto( diff --git a/tests/Unit/Service/DocumentationQuarantineServiceTest.php b/tests/Unit/Service/DocumentationQuarantineServiceTest.php new file mode 100644 index 0000000..e4a1ec8 --- /dev/null +++ b/tests/Unit/Service/DocumentationQuarantineServiceTest.php @@ -0,0 +1,53 @@ +createMock(EntityManagerInterface::class); + $entityManager->method('persist')->willReturnCallback( + static function (object $entity) use (&$persisted): void { + $persisted = $entity; + } + ); + + $subject = new DocumentationQuarantineService($entityManager, $this->createMock(DocumentationQuarantineRepository::class)); + $pushEvent = new PushEvent( + 'https://github.com/acme/coolextension.git', + 'main', + 'https://raw.githubusercontent.com/acme/coolextension/main/composer.json', + '{}' + ); + + $subject->quarantine($pushEvent, 'raw.githubusercontent.com'); + + $this->assertInstanceOf(DocumentationQuarantine::class, $persisted); + $this->assertSame('raw.githubusercontent.com', $persisted->getDomain()); + } +} diff --git a/tests/Unit/Service/RenderDocumentationServiceTest.php b/tests/Unit/Service/RenderDocumentationServiceTest.php new file mode 100644 index 0000000..26d988e --- /dev/null +++ b/tests/Unit/Service/RenderDocumentationServiceTest.php @@ -0,0 +1,91 @@ +createMock(DocumentationBuildInformationService::class); + $buildInformationService->method('fetchRemoteComposerJson')->willThrowException( + new UnknownComposerJsonUrlException('', 1782290340, null, $pushEvent->getUrlToComposerFile(), 'raw.githubusercontent.com') + ); + + $lastHitDomain = null; + $buildInformationService->method('updateLastHit')->willReturnCallback( + static function (string $domain) use (&$lastHitDomain): void { + $lastHitDomain = $domain; + } + ); + + $quarantinedDomain = null; + $quarantineService = $this->createMock(DocumentationQuarantineService::class); + $quarantineService->method('isQuarantined')->willReturn(false); + $quarantineService->method('quarantine')->willReturnCallback( + static function (PushEvent $event, string $domain) use (&$quarantinedDomain): DocumentationQuarantine { + $quarantinedDomain = $domain; + + return new DocumentationQuarantine(); + } + ); + + $subject = new RenderDocumentationService( + $buildInformationService, + $this->createMock(GithubService::class), + new HistoryService($this->createMock(EntityManagerInterface::class)), + new NullLogger(), + $quarantineService, + $this->createMock(RepositoryBlacklistEntryRepository::class), + $this->createMock(MailService::class), + $this->createMock(Security::class), + ); + + try { + $subject->requestDocumentationRendering($pushEvent, DocumentationRenderingTrigger::API); + $this->fail('An unknown domain has to decline the rendering request.'); + } catch (DocumentationRenderingRequestDeclinedException) { + // expected, the assertions below are what this test is about + } + + $this->assertSame('raw.githubusercontent.com', $quarantinedDomain, 'The quarantine has to record the host the request was blocked on.'); + $this->assertSame('raw.githubusercontent.com', $lastHitDomain, 'The last hit belongs to the domain row the check looks up.'); + } +}