From 79c57f3fd2bb5f65343b35cb12977f39410f81de Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Tue, 4 Aug 2026 11:39:42 +0200 Subject: [PATCH] [BUGFIX] Re-check the composer.json url after every redirect assertUrlToComposerFileIsSafe() ran once, on the url built from the webhook payload, and the client then followed up to five redirects without checking any of them. An open redirect on an allowed domain was therefore enough to make intercept fetch from anywhere. Check every hop with the same method. None of the url forms the providers actually serve rely on a redirect, verified against Github raw, Gitlab and Forgejo, so this costs nothing in practice. The check can now also fail mid-request with an InvalidComposerJsonUrlException, which no caller handled, so a redirect to a disallowed scheme would have answered a public request with a 500 and a stack trace. Handle it like its two siblings, which turns it into the intended 422 with a history entry. Signed-off-by: Sebastian Mendel The history entry gets its own status rather than reusing the one for an unknown domain, which would have labelled an unusable url as a domain problem. Signed-off-by: Sebastian Mendel --- .../DocumentationBuildInformationService.php | 18 ++- ...cumentationBuildInformationServiceTest.php | 115 ++++++++++++++++++ 2 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 tests/Unit/Service/DocumentationBuildInformationServiceTest.php diff --git a/src/Service/DocumentationBuildInformationService.php b/src/Service/DocumentationBuildInformationService.php index 8e285a0..66389cc 100644 --- a/src/Service/DocumentationBuildInformationService.php +++ b/src/Service/DocumentationBuildInformationService.php @@ -33,6 +33,9 @@ use GuzzleHttp\ClientInterface; use GuzzleHttp\Exception\GuzzleException; use GuzzleHttp\Psr7\Uri; +use Psr\Http\Message\RequestInterface; +use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\UriInterface; use Symfony\Component\Filesystem\Filesystem; /** @@ -75,7 +78,20 @@ public function fetchRemoteComposerJson(string $path): array $this->assertUrlToComposerFileIsSafe($path); try { - $response = $this->generalClient->request('GET', $path); + // The url is only known to be safe until the first redirect, so every + // hop has to pass the same check. Without this an open redirect on an + // allowed domain would be enough to reach an arbitrary target. + $response = $this->generalClient->request('GET', $path, [ + 'allow_redirects' => [ + 'max' => 5, + 'protocols' => ['http', 'https'], + 'strict' => false, + 'referer' => false, + 'on_redirect' => function (RequestInterface $request, ResponseInterface $response, UriInterface $uri): void { + $this->assertUrlToComposerFileIsSafe((string) $uri); + }, + ], + ]); } catch (GuzzleException $e) { throw new ComposerJsonNotFoundException($e->getMessage(), $e->getCode()); } diff --git a/tests/Unit/Service/DocumentationBuildInformationServiceTest.php b/tests/Unit/Service/DocumentationBuildInformationServiceTest.php new file mode 100644 index 0000000..f45606e --- /dev/null +++ b/tests/Unit/Service/DocumentationBuildInformationServiceTest.php @@ -0,0 +1,115 @@ +buildSubject( + allowedDomain: 'allowed.example', + responses: [ + new Response(302, ['Location' => 'https://evil.example/composer.json']), + new Response(200, [], '{"name": "should/never-be-reached"}'), + ] + ); + + $this->expectException(UnknownComposerJsonUrlException::class); + + $subject->fetchRemoteComposerJson('https://allowed.example/acme/ext/raw/branch/main/composer.json'); + } + + public function testRedirectStayingOnAnAllowedDomainIsFollowed(): void + { + $subject = $this->buildSubject( + allowedDomain: 'allowed.example', + responses: [ + new Response(302, ['Location' => 'https://allowed.example/elsewhere/composer.json']), + new Response(200, [], '{"name": "acme/ext"}'), + ] + ); + + $composerJson = $subject->fetchRemoteComposerJson('https://allowed.example/acme/ext/raw/branch/main/composer.json'); + + $this->assertSame('acme/ext', $composerJson['name']); + } + + public function testResponseWithoutARedirectIsReturned(): void + { + $subject = $this->buildSubject( + allowedDomain: 'allowed.example', + responses: [new Response(200, [], '{"name": "acme/ext"}')] + ); + + $composerJson = $subject->fetchRemoteComposerJson('https://allowed.example/acme/ext/raw/branch/main/composer.json'); + + $this->assertSame('acme/ext', $composerJson['name']); + } + + public function testNonSuccessfulResponseIsReportedAsNotFound(): void + { + $subject = $this->buildSubject( + allowedDomain: 'allowed.example', + responses: [new Response(404)] + ); + + $this->expectException(ComposerJsonNotFoundException::class); + + $subject->fetchRemoteComposerJson('https://allowed.example/acme/ext/raw/branch/main/composer.json'); + } + + /** + * @param Response[] $responses + */ + private function buildSubject(string $allowedDomain, array $responses): DocumentationBuildInformationService + { + $knownDomain = (new KnownRepositoryDomain())->setDomain($allowedDomain)->setStatus(RepositoryDomainStatus::ALLOWED); + + $knownRepositoryDomainRepository = $this->createMock(KnownRepositoryDomainRepository::class); + $knownRepositoryDomainRepository->method('findOneBy')->willReturnCallback( + static fn (array $criteria): ?KnownRepositoryDomain => ($criteria['domain'] ?? null) === $allowedDomain ? $knownDomain : null + ); + + return new DocumentationBuildInformationService( + '/tmp', + 'sub', + $this->createMock(DocumentationJarRepository::class), + $knownRepositoryDomainRepository, + $this->createMock(EntityManagerInterface::class), + $this->createMock(Filesystem::class), + new Client(['handler' => HandlerStack::create(new MockHandler($responses))]), + $this->createMock(SlackService::class), + $this->createMock(MailService::class), + ); + } +}