[BUGFIX] Require the composer.json url to point at a composer.json - #308
Merged
andreaskienast merged 1 commit intoAug 22, 2026
Merged
Conversation
CybotTM
force-pushed
the
fix/pin-composer-json-path
branch
from
August 4, 2026 14:55
a65f682 to
ab2f8af
Compare
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 <github@sebastianmendel.de>
CybotTM
force-pushed
the
fix/pin-composer-json-path
branch
from
August 4, 2026 15:57
ab2f8af to
f808830
Compare
andreaskienast
approved these changes
Aug 22, 2026
andreaskienast
pushed a commit
that referenced
this pull request
Aug 22, 2026
`assertUrlToComposerFileIsSafe()` validates the composer.json url built from an unauthenticated webhook payload — but only once, on the initial url. The client then follows up to five redirects, none of which is checked again, so an open redirect on an allowed domain is enough to make intercept fetch from anywhere. This re-runs the check on every hop. No legitimate fetch is affected: GitHub raw, GitLab `/raw/` and Forgejo `/raw/branch/` all answer directly with zero redirects. **Second change, in the same PR on purpose:** the check can now fail mid-request, and one of its three exceptions (`InvalidComposerJsonUrlException`) was caught nowhere — that would have turned a redirect to a disallowed scheme into an HTTP 500 instead of the intended 422. Splitting the two would mean merging a state where the first change makes things worse. `t3g:test` (164 tests), `t3g:phpstan` and `t3g:cgl` pass. Found while working on #305, independent of it. <details> <summary>Details — evidence, the new history status, merge notes</summary> ### Evidence `config/services.yaml:53-56` registers `guzzle.client.general` as a bare `GuzzleHttp\Client`; its resolved config is `allow_redirects: {max: 5, protocols: [http, https], strict: false, referer: false}`. I confirmed the behaviour with that same construction against a local redirect server: a 302 to another host was followed and its body returned. I also checked the guard cannot be evaded: `on_redirect` fires for 301, 302, 303, 307 and 308; relative and protocol-relative `Location` headers are resolved and checked (`//evil.example/x` arrives as `https://evil.example/x`); and the exception thrown inside the callback is not a `GuzzleException`, so the existing `catch (GuzzleException)` in `fetchRemoteComposerJson()` does not swallow it into a plain "not found". Whether legitimate urls rely on redirects, checked live: `raw.githubusercontent.com` 200/0 redirects, GitLab `/raw/` 200/0, Forgejo `/raw/branch/` 200/0. Only Forgejo's deprecated short form redirects, and intercept does not build it. ### New history status The new catch writes `DocsRenderingHistoryStatus::INVALID_COMPOSER_JSON_URL` rather than reusing `UNKNOWN_REPOSITORY_DOMAIN`, which would have labelled an unusable url as a domain problem in the rendering history. ### Tests `DocumentationBuildInformationServiceTest` covers a redirect leaving the allowlist (rejected), one staying on it (followed), a plain response and a non-200. I verified they discriminate by removing the `on_redirect` guard and confirming the first test fails. ### Merge notes #307 touches `RenderDocumentationService` in the same area and #308 adds the identical catch block and status. All three merge onto `develop` cleanly in any order — I verified that by performing the merges; the combined tree is identical regardless of order and its unit suite is green. </details> Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The composer.json url is assembled by string substitution from webhook payload fields, and the allowlist guarding it compares the host only. A
#or a?inside one of those fields therefore pushes the intended…/composer.jsonsuffix out of the path, and what actually gets fetched is an arbitrary endpoint on an allowed host.All four services produce a url whose path ends in
/composer.json, so requiring that suffix rejects those shapes while leaving every real url untouched.What this does not fix: the allowlist still stores a bare hostname, so a different port on an allowed host is still reachable — this only bounds what can be retrieved there to files named
composer.json. Making it exact depends on what the existingKnownRepositoryDomainrows look like in production, which I cannot see.t3g:test(169 tests),t3g:phpstanandt3g:cglpass. Found while working on #305, independent of it.Details — evidence, bypass attempts, one risk to check, merge notes
Evidence
Built through the real
GitRepositoryServicewithproject.web_urlas the attacker-controlled field:https://allowed.example/internal/admin#/internal/adminhttps://allowed.example/api/v4/user?a=/api/v4/userhttps://allowed.example:9200/_cluster/health#/_cluster/healthon port 9200In each case the allowlist saw only
allowed.exampleand passed. With this change all three are rejected.Bypass attempts
I tried to get past the new check with percent-encoded slashes and dots (
%2Fcomposer.json,composer%2Ejson), uppercase, a semicolon parameter, a trailing traversal after the suffix, and double slashes. All rejected. What still passes is a traversal that itself ends in/composer.json— consistent with the stated bound above.One risk worth checking
publicComposerJsonUrlis editable in the manual deployment wizard. If any existing row points at a file not namedcomposer.json, it would now be rejected. A quick look at that column would confirm whether that is the case.Exception handling
The new rejection uses
InvalidComposerJsonUrlException, which no caller handled — on its own that would answer a public request with a 500. This PR therefore also handles it, with its own history statusINVALID_COMPOSER_JSON_URLrather than reusing the one for an unknown domain. That block is byte-identical to the one in #306, so the two merge in either order.Merge notes
All three related PRs merge onto
developcleanly in any order — verified by performing the merges, with an identical resulting tree, the catch block and the status each present exactly once, and a green combined suite.