[FEATURE] Support Forgejo and Gitea push webhooks for docs rendering - #305
Merged
andreaskienast merged 1 commit intoAug 22, 2026
Merged
Conversation
CybotTM
marked this pull request as draft
August 3, 2026 20:13
CybotTM
force-pushed
the
feat/forgejo-webhook-support
branch
6 times, most recently
from
August 4, 2026 07:46
4dedc1f to
3a40193
Compare
CybotTM
marked this pull request as ready for review
August 4, 2026 08:34
This was referenced Aug 4, 2026
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>
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>
Forgejo and Gitea send X-Gitea-Event and X-GitHub-Event compatibility
headers along with their own, so their pushes were handled as Github
pushes and the composer.json was looked up on raw.githubusercontent.com.
For a self hosted repository that resolves to an unrelated Github
repository sharing the owner and name, or to nothing at all.
Detect their push events before the Github check, the order matters
because of those compatibility headers, and resolve the composer.json
against the repository html url from the payload, so instances on any
domain work. The url format is {baseUrl}/raw/{branch|tag}/{version}/,
verified against codeberg.org and a self hosted instance.
Their push payloads follow the Github structure, so the payload handling
is shared. Two things differ and stay on the Forgejo path:
Deleting a tag arrives as a regular push whose 'after' is the all zero
object id, 40 or 64 characters wide depending on the object format,
because the payload has no 'deleted' property. Deleting a branch sends
no push event at all.
A ref that is neither a branch nor a tag, and a payload without the
repository html url, cannot produce a usable raw url and are rejected.
Two further points affect all providers sharing this code path:
A payload that is valid json but not an object is now rejected as an
unsupported request. The shared handling is typed, so without this the
request would raise a TypeError and answer 500 where it answered 422.
Senders cap the commit list, Forgejo at 15 and Gitea at 5 by default,
while 'total_commits' keeps the real number. A documentation change in
one of the dropped commits would never be rendered and could not be
recovered, so a truncated list no longer counts as 'nothing to render'.
Github payloads carry no 'total_commits' and are unaffected.
Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
andreaskienast
force-pushed
the
feat/forgejo-webhook-support
branch
from
August 22, 2026 17:17
3a40193 to
e74ae57
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
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.
Fixes #304
Forgejo and Gitea send
X-GitHub-Eventas a compatibility header alongside their own, so their pushes were handled as Github pushes and the composer.json was looked up onraw.githubusercontent.com. For the repository in the issue that is a 404 wheregit.codemacher.de/…/raw/branch/main/composer.jsonreturns 200.This routes their push events before the Github check and resolves the composer.json against
repository.html_url, so instances on any domain work.Two things touch the shared Github path, please look at these: a body that is valid json but not an object now returns 422 instead of raising a
TypeError(500), and a truncated commit list now renders instead of being dropped as "no documentation changes". Both are explained below.t3g:test(192 tests),t3g:phpstan,t3g:cgl,npm ciandnpm run buildpass. The functional suite needs the privatet3g/lib-test-helperand could not be run here.Documentation follow-up, held as a draft until this is released: TYPO3-Documentation/TYPO3CMS-Guide-HowToDocument#526
Details — verification against a real instance, the two behaviour changes, known limitations, and how to reproduce
Verified against a real Forgejo instance
Not only against the source: I ran Forgejo 12.0.4 locally, pointed a webhook at a recorder and replayed the captured deliveries through the actual
WebHookService.Documentation/changeX-Forgejo-Event,X-GitHub-Event,X-Gitea-Event,X-Gogs-Eventallpushref: refs/tags/…,commits: []after= 40 zeros, nodeletedpropertydeleteeventcommits: 15,total_commits: 20Replaying those captures against
developshows what users get today: every url points atraw.githubusercontent.com, the tag deletion is accepted as a render trigger, and the 20-commit push is dropped. With this branch the urls point at the instance and resolve (/raw/branch/main/composer.jsonand/raw/tag/2.0.0/composer.jsonboth 200). The fixtures here were checked field by field against those real payloads — that is what added the missinghead_commitand correctedcompare_urlon the deleted-tag fixture.To reproduce, no Forgejo account needed:
Then create a repository, point a
pushwebhook at any request bin, and push. I deliberately did not add this as an automated test: the project has no container in its test path at all — the functional suite runs against file-based SQLite and the CI workflow has noservices:section — so adding one for a single provider would be infrastructure out of proportion to the change.The two changes on the shared Github path
Non-object json body. The shared payload handling is typed, so without a guard a body like
123raises aTypeErrorand answers 500 where it used to answer 422. It is now rejected as an unsupported request. Ondevelopthis wasInvalidWebHookPayloadException/HOOK_INVALID_PAYLOAD; it is nowUnsupportedWebHookRequestException/UNSUPPORTED_HOOK, same http status. Say the word if you would rather keep the old classification.Truncated commit list. Senders cap the list — Forgejo at 15, Gitea at 5 — while
total_commitskeeps the real number, so a documentation change in a dropped commit would never render and could not be recovered. A truncated list therefore no longer counts as "nothing to render". Github payloads carry nototal_commitsand are unaffected.I verified every other existing path is byte-identical to
developby running a payload matrix (Github branch/tag/rst/form-encoded/ping/broken-json/malformed-commits, GitLab, both Bitbucket variants) against both revisions and diffing.Known limitations, so they are not mistaken for oversights
Deleting a branch on Forgejo sends no push event, only a
deleteevent, which falls through toUNSUPPORTED_HOOK(422) rather than theBRANCH_DELETED(404) a Github deletion produces.RepositoryUrlUtilitystill matches only the three cloud hosts, soRenderOtherBranchesFromNewDocsCommandcannot resolve a composer.json for a Forgejo repository. The webhook path is unaffected.SERVICE_NAMESis deliberately not extended: it only feeds the manual wizard's dropdown, and that path resolves urls viaRepositoryUrlUtility— a Forgejo entry there would build a Bitbucket-shaped url.Merging this does not by itself make the reporter's instance work:
assertUrlToComposerFileIsSafe()still requires an admin to approve the domain.