[BUGFIX] Quarantine the domain the request was blocked on (#307)

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 — `github.com` versus
`raw.githubusercontent.com`.

That is not cosmetic: approving an entry allowlists exactly the recorded
domain and replays every entry sharing it. So an admin approving a
GitHub repository creates a `KnownRepositoryDomain` for a domain the
fetch check never consults, while the replayed events are checked
against the real host and land in quarantine again. The approval
silently does nothing.

This records the host the check actually rejected. `updateLastHit()` had
the same mismatch and could never find the row it meant to touch.

**Needs a migration:** the feature shipped in 7.2.0, so existing rows
carry the wrong host and approving one of them reproduces the bug. The
migration recomputes the domain from the push event each row already
stores.

`t3g:test` (162 tests), `t3g:phpstan` and `t3g:cgl` pass. Found while
working on #305, independent of it.

<details>
<summary>Details — migration behaviour, the replay fix, tests, merge
notes</summary>

### Migration

The domain is recomputed from `serialized_push_event`, which contains
`urlToComposerFile`. Rows whose payload cannot be decoded are skipped
rather than failing the migration; rows already correct (Bitbucket
Cloud, most GitLab setups) are left alone. The checksum hashes only the
serialized push event and does not include the domain, so deduplication
is unaffected. It is irreversible — the old value came from a different
url and cannot be reconstructed.

I ran it against a SQLite database seeded with a GitHub row, a Bitbucket
row and a row with an undecodable payload: only the GitHub row changed,
from `github.com` to `raw.githubusercontent.com`.

### Replay loop made robust

Approving a domain replays every entry it holds. Until now a single
entry that cannot be rendered — an irrelevant branch name is the likely
case — threw out of the loop, gave the admin a 500, aborted the
remaining entries and left the queue half-processed. This was invisible
before, because the loop never got that far. Such an entry is now
skipped and the admin is told how many were dropped. Both approval paths
are covered.

### Tests

`RenderDocumentationServiceTest` pins that the host handed to
`quarantine()` and to `updateLastHit()` is the composer host, using a
GitHub-shaped push event where the two differ. I verified it
discriminates by reverting each production line separately — the test
fails each time. The earlier version of this PR only asserted a setter
passthrough, which stayed green when the real bug was restored; that gap
is what this test closes.

### Merge notes

#306 touches `RenderDocumentationService` in the same area. All three
related PRs merge onto `develop` cleanly in any order — verified by
performing the merges, with an identical resulting tree and a green
combined suite.

</details>

Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
Co-authored-by: Andreas Kienast <andreas.kienast@typo3.com>

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

src/Controller/AdminInterface/Docs/QuarantinedDocumentationsController.php

@@ -71,20 +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();
                 try {
                     $this->renderDocumentationService->requestDocumentationRendering($pushEvent, DocumentationRenderingTrigger::WEB);
                 } catch (DocumentationRenderingRequestDeclinedException) {
-                    // Exception is thrown if the request documentation rendering does not comply with requirements
-                    // Intended fall-thru
+                    // 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');
         }

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

@@ -55,12 +55,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.');
+    }
+}