[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 <github@sebastianmendel.de>

Author: CybotTM

migrations/Version20260804120000.php

@@ -0,0 +1,54 @@
+<?php
+
+declare(strict_types=1);
+
+namespace DoctrineMigrations;
+
+use App\Utility\RepositoryUrlUtility;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Quarantined entries recorded the host of the clone url, while the check that
+ * blocked them keys on the host of the composer.json url. Recompute the domain
+ * of the existing rows from the push event they carry, so approving them
+ * allowlists the host that is actually consulted.
+ */
+final class Version20260804120000 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return 'Set the quarantined domain to the host of the composer.json url';
+    }
+
+    public function up(Schema $schema): void
+    {
+        $rows = $this->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;
+    }
+}

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));
 

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(

tests/Unit/Service/DocumentationQuarantineServiceTest.php

@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the package t3g/intercept.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace App\Tests\Unit\Service;
+
+use App\Entity\DocumentationQuarantine;
+use App\Extractor\PushEvent;
+use App\Repository\DocumentationQuarantineRepository;
+use App\Service\DocumentationQuarantineService;
+use Doctrine\ORM\EntityManagerInterface;
+use PHPUnit\Framework\TestCase;
+
+class DocumentationQuarantineServiceTest extends TestCase
+{
+    /**
+     * Approving a quarantined entry allowlists the domain it recorded, and that
+     * decision is only correct if the recorded domain is the one the request was
+     * blocked on. For Github those two never match: the clone url is on
+     * github.com while the composer.json is fetched from
+     * raw.githubusercontent.com.
+     */
+    public function testQuarantineRecordsTheDomainTheRequestWasBlockedOn(): void
+    {
+        $persisted = null;
+        $entityManager = $this->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());
+    }
+}

tests/Unit/Service/RenderDocumentationServiceTest.php

@@ -0,0 +1,91 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the package t3g/intercept.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace App\Tests\Unit\Service;
+
+use App\Entity\DocumentationQuarantine;
+use App\Enum\DocumentationRenderingTrigger;
+use App\Exception\DocumentationRenderingRequestDeclinedException;
+use App\Exception\UnknownComposerJsonUrlException;
+use App\Extractor\PushEvent;
+use App\Repository\RepositoryBlacklistEntryRepository;
+use App\Service\DocumentationBuildInformationService;
+use App\Service\DocumentationQuarantineService;
+use App\Service\GithubService;
+use App\Service\HistoryService;
+use App\Service\MailService;
+use App\Service\RenderDocumentationService;
+use Doctrine\ORM\EntityManagerInterface;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Symfony\Bundle\SecurityBundle\Security;
+
+class RenderDocumentationServiceTest extends TestCase
+{
+    /**
+     * The quarantined domain is what an admin later allowlists, so it has to be
+     * the host the request was blocked on. That is the host of the composer.json
+     * url, which for Github is never the host of the clone url.
+     */
+    public function testTheBlockedDomainIsHandedToTheQuarantine(): void
+    {
+        $pushEvent = new PushEvent(
+            'https://github.com/acme/coolextension.git',
+            'main',
+            'https://raw.githubusercontent.com/acme/coolextension/main/composer.json',
+            '{}'
+        );
+
+        $buildInformationService = $this->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.');
+    }
+}