From f808830ab91b544cb350e81965802bd0f61fe4ee Mon Sep 17 00:00:00 2001 From: Sebastian Mendel Date: Tue, 4 Aug 2026 12:47:02 +0200 Subject: [PATCH] [BUGFIX] Require the composer.json url to point at a composer.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The url is assembled by string substitution from webhook payload fields, and the allowlist that guards it compares the host only. A '#' or a '?' inside one of those fields therefore pushes the intended '…/composer.json' suffix into a fragment or a query string and leaves an arbitrary endpoint on an allowed host, which is then fetched. Every url format this application builds ends its path with '/composer.json', for all four services, so require that. Both manipulations lose the suffix and are rejected. This does not make the allowlist exact, a different port on an allowed host still passes, but it bounds what can be reached there to files named composer.json. The new rejection uses InvalidComposerJsonUrlException, which no caller handled, so it would have answered a public request with a 500. Handle it like its two siblings. That block, and the history status it uses, are identical to the ones in the pull request that re-checks the url after a redirect, so the two merge in either order. Signed-off-by: Sebastian Mendel --- src/Enum/DocsRenderingHistoryStatus.php | 3 + .../DocumentationBuildInformationService.php | 8 ++ src/Service/RenderDocumentationService.php | 15 ++++ .../Unit/Service/ComposerJsonUrlShapeTest.php | 87 +++++++++++++++++++ 4 files changed, 113 insertions(+) create mode 100644 tests/Unit/Service/ComposerJsonUrlShapeTest.php diff --git a/src/Enum/DocsRenderingHistoryStatus.php b/src/Enum/DocsRenderingHistoryStatus.php index 5a33ebb..edd9746 100644 --- a/src/Enum/DocsRenderingHistoryStatus.php +++ b/src/Enum/DocsRenderingHistoryStatus.php @@ -18,6 +18,7 @@ final class DocsRenderingHistoryStatus public const NO_COMPOSER_JSON = 'noComposerJson'; public const INVALID_COMPOSER_JSON = 'invalidComposerJson'; public const UNKNOWN_REPOSITORY_DOMAIN = 'unknownRepositoryDomain'; + public const INVALID_COMPOSER_JSON_URL = 'invalidComposerJsonUrl'; public const PACKAGE_REGISTERED_WITH_DIFFERENT_REPOSITORY = 'packageRegisteredWithDifferentRepository'; public const NO_RELEVANT_BRANCH_OR_TAG = 'noRelevantBranchOrTag'; public const MISSING_VALUE_IN_COMPOSER_JSON = 'missingValueInComposerJson'; @@ -35,6 +36,7 @@ final class DocsRenderingHistoryStatus self::NO_COMPOSER_JSON, self::INVALID_COMPOSER_JSON, self::UNKNOWN_REPOSITORY_DOMAIN, + self::INVALID_COMPOSER_JSON_URL, self::PACKAGE_REGISTERED_WITH_DIFFERENT_REPOSITORY, self::NO_RELEVANT_BRANCH_OR_TAG, self::MISSING_VALUE_IN_COMPOSER_JSON, @@ -55,6 +57,7 @@ final class DocsRenderingHistoryStatus self::NO_COMPOSER_JSON => 'No composer.json found.', self::INVALID_COMPOSER_JSON => 'Invalid composer.json.', self::UNKNOWN_REPOSITORY_DOMAIN => 'Unknown repository domain.', + self::INVALID_COMPOSER_JSON_URL => 'The composer.json url can not be used.', self::PACKAGE_REGISTERED_WITH_DIFFERENT_REPOSITORY => 'Package registered with different repository.', self::NO_RELEVANT_BRANCH_OR_TAG => 'No relevant branch or tag found.', self::MISSING_VALUE_IN_COMPOSER_JSON => 'Missing value in composer.json.', diff --git a/src/Service/DocumentationBuildInformationService.php b/src/Service/DocumentationBuildInformationService.php index ce3ea8c..8e285a0 100644 --- a/src/Service/DocumentationBuildInformationService.php +++ b/src/Service/DocumentationBuildInformationService.php @@ -324,6 +324,14 @@ private function assertUrlToComposerFileIsSafe(string $url): void throw new InvalidComposerJsonUrlException('URL to composer.json contains disallowed scheme', 1781613532, null, $url); } + // The url is assembled from payload fields, so a '#' or a '?' inside one of + // them can push the intended '…/composer.json' suffix out of the path and + // leave an arbitrary endpoint on the same host. Every format this + // application builds ends in that suffix, so require it. + if (!str_ends_with($uri->getPath(), '/composer.json')) { + throw new InvalidComposerJsonUrlException('URL to composer.json does not point to a composer.json', 1785816000, null, $url); + } + $normalizedHost = RepositoryUrlUtility::getNormalizedDomain($uri); $allowedRepositoryDomain = $this->knownRepositoryDomainsRepository->findOneBy([ 'domain' => $normalizedHost, diff --git a/src/Service/RenderDocumentationService.php b/src/Service/RenderDocumentationService.php index bdaa161..5b9594b 100644 --- a/src/Service/RenderDocumentationService.php +++ b/src/Service/RenderDocumentationService.php @@ -26,6 +26,7 @@ use App\Exception\DocsPackageDoNotCareBranch; use App\Exception\DocsPackageRegisteredWithDifferentRepositoryException; use App\Exception\DocumentationRenderingRequestDeclinedException; +use App\Exception\InvalidComposerJsonUrlException; use App\Exception\UnknownComposerJsonUrlException; use App\Extractor\DeploymentInformation; use App\Extractor\PushEvent; @@ -90,6 +91,20 @@ public function requestDocumentationRendering(PushEvent $pushEvent, Documentatio )); throw new DocumentationRenderingRequestDeclinedException(sprintf('composer.json\'s host domain %s is disallowed for rendering request', $e->normalizedHost), 1782294348, $e); + } catch (InvalidComposerJsonUrlException $e) { + $this->historyService->writeHistory(new HistoryEntryDto( + type: HistoryEntryType::DOCS_RENDERING, + status: DocsRenderingHistoryStatus::INVALID_COMPOSER_JSON_URL, + triggeredBy: $trigger->toHistoryEntryTrigger(), + data: [ + 'repository' => $pushEvent->getRepositoryUrl(), + 'composerFile' => $pushEvent->getUrlToComposerFile(), + 'payload' => $pushEvent->getPayload(), + 'user' => $userIdentifier, + ] + )); + + throw new DocumentationRenderingRequestDeclinedException(sprintf('composer.json url %s can not be used for a rendering request', $e->composerJsonUrl), 1785810600, $e); } $composerAsObject = $this->documentationBuildInformationService->getComposerJsonObject($composerJson); diff --git a/tests/Unit/Service/ComposerJsonUrlShapeTest.php b/tests/Unit/Service/ComposerJsonUrlShapeTest.php new file mode 100644 index 0000000..c854999 --- /dev/null +++ b/tests/Unit/Service/ComposerJsonUrlShapeTest.php @@ -0,0 +1,87 @@ + ['https://allowed.example/acme/ext/raw/main/composer.json']; + yield 'bitbucket server' => ['https://allowed.example/projects/EXT/repos/ext/raw/composer.json?at=refs%2Fheads%2Fmain']; + yield 'gitlab' => ['https://allowed.example/acme/ext/raw/main/composer.json']; + yield 'github' => ['https://allowed.example/acme/ext/main/composer.json']; + yield 'forgejo' => ['https://allowed.example/acme/ext/raw/branch/main/composer.json']; + } + + #[DataProvider('urlsBuiltForTheSupportedServicesDataProvider')] + public function testUrlsTheServicesActuallyProduceArePassed(string $url): void + { + $composerJson = $this->buildSubject()->fetchRemoteComposerJson($url); + + $this->assertSame('acme/ext', $composerJson['name']); + } + + public static function manipulatedUrlsDataProvider(): \Iterator + { + // A '#' turns the expected '/…/composer.json' suffix into a fragment, + // which is dropped before the request is sent + yield 'fragment cuts off the expected path' => ['https://allowed.example/internal/admin#/raw/branch/main/composer.json']; + // A '?' in the base url pushes the expected suffix into the query string + yield 'query swallows the expected path' => ['https://allowed.example/api/v4/user?a=/raw/branch/main/composer.json']; + yield 'path traversal out of the repository' => ['https://allowed.example/acme/ext/raw/branch/../../../../etc/passwd']; + yield 'no composer.json at all' => ['https://allowed.example/acme/ext/raw/branch/main/']; + } + + #[DataProvider('manipulatedUrlsDataProvider')] + public function testUrlsNotPointingAtAComposerJsonAreRejected(string $url): void + { + $this->expectException(InvalidComposerJsonUrlException::class); + + $this->buildSubject()->fetchRemoteComposerJson($url); + } + + private function buildSubject(): DocumentationBuildInformationService + { + $knownDomain = (new KnownRepositoryDomain())->setDomain('allowed.example')->setStatus(RepositoryDomainStatus::ALLOWED); + $knownRepositoryDomainRepository = $this->createMock(KnownRepositoryDomainRepository::class); + $knownRepositoryDomainRepository->method('findOneBy')->willReturn($knownDomain); + + 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([new Response(200, [], '{"name": "acme/ext"}')]))]), + $this->createMock(SlackService::class), + $this->createMock(MailService::class), + ); + } +}