[BUGFIX] Re-check the composer.json url after every redirect - #306
Merged
andreaskienast merged 1 commit intoAug 22, 2026
Merged
Conversation
CybotTM
force-pushed
the
fix/harden-composer-json-fetch
branch
from
August 4, 2026 15:57
fcfd5b8 to
09c465a
Compare
andreaskienast
pushed a commit
that referenced
this pull request
Aug 22, 2026
) 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.json` suffix 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 existing `KnownRepositoryDomain` rows look like in production, which I cannot see. `t3g:test` (169 tests), `t3g:phpstan` and `t3g:cgl` pass. Found while working on #305, independent of it. <details> <summary>Details — evidence, bypass attempts, one risk to check, merge notes</summary> ### Evidence Built through the real `GitRepositoryService` with `project.web_url` as the attacker-controlled field: | payload value | path actually requested | |---|---| | `https://allowed.example/internal/admin#` | `/internal/admin` | | `https://allowed.example/api/v4/user?a=` | `/api/v4/user` | | `https://allowed.example:9200/_cluster/health#` | `/_cluster/health` on port 9200 | In each case the allowlist saw only `allowed.example` and 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 `publicComposerJsonUrl` is editable in the manual deployment wizard. If any existing row points at a file not named `composer.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 status `INVALID_COMPOSER_JSON_URL` rather 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 `develop` cleanly 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. </details> Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
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 <github@sebastianmendel.de> 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 <github@sebastianmendel.de>
andreaskienast
force-pushed
the
fix/harden-composer-json-fetch
branch
from
August 22, 2026 17:12
09c465a to
79c57f3
Compare
andreaskienast
approved these changes
Aug 22, 2026
andreaskienast
added a commit
that referenced
this pull request
Aug 22, 2026
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>
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.
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:phpstanandt3g:cglpass. Found while working on #305, independent of it.Details — evidence, the new history status, merge notes
Evidence
config/services.yaml:53-56registersguzzle.client.generalas a bareGuzzleHttp\Client; its resolved config isallow_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_redirectfires for 301, 302, 303, 307 and 308; relative and protocol-relativeLocationheaders are resolved and checked (//evil.example/xarrives ashttps://evil.example/x); and the exception thrown inside the callback is not aGuzzleException, so the existingcatch (GuzzleException)infetchRemoteComposerJson()does not swallow it into a plain "not found".Whether legitimate urls rely on redirects, checked live:
raw.githubusercontent.com200/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_URLrather than reusingUNKNOWN_REPOSITORY_DOMAIN, which would have labelled an unusable url as a domain problem in the rendering history.Tests
DocumentationBuildInformationServiceTestcovers a redirect leaving the allowlist (rejected), one staying on it (followed), a plain response and a non-200. I verified they discriminate by removing theon_redirectguard and confirming the first test fails.Merge notes
#307 touches
RenderDocumentationServicein the same area and #308 adds the identical catch block and status. All three merge ontodevelopcleanly in any order — I verified that by performing the merges; the combined tree is identical regardless of order and its unit suite is green.