[BUGFIX] Quarantine the domain the request was blocked on - #307
Merged
Conversation
CybotTM
force-pushed
the
fix/quarantine-domain
branch
from
August 4, 2026 15:00
f42bfd7 to
6c7914e
Compare
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: the repository is on github.com, the composer.json is fetched from raw.githubusercontent.com. Approving a quarantined entry allowlists exactly the recorded domain and then replays every entry sharing it. With the wrong domain recorded, the replay is checked against a host that was never approved, so it is quarantined again and the allowlist gains an entry for a domain that is never consulted. Record the host the check actually rejected, which the exception already carries. updateLastHit() had the same mismatch and could therefore never find the row it meant to touch. The feature shipped in 7.2.0, so entries recorded before this fix carry the clone host. Recompute those from the push event each row already stores. Rows whose payload can not be read are left alone, and the checksum does not cover the domain, so deduplication is unaffected. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de> Approving a domain replays every entry it holds, and until now a single entry that can not be rendered, an irrelevant branch name for instance, aborted the whole run and left the rest of the queue behind. Skip such an entry and tell the admin how many were dropped. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
CybotTM
force-pushed
the
fix/quarantine-domain
branch
from
August 4, 2026 15:57
6c7914e to
d93746b
Compare
Member
|
Sorry, I pushed a commit that fixes one bug your PR already covered. Can you please rebase your PR and give me a ping? Thanks in advance! |
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>
andreaskienast
approved these changes
Aug 22, 2026
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.
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.comversusraw.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
KnownRepositoryDomainfor 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:phpstanandt3g:cglpass. Found while working on #305, independent of it.Details — migration behaviour, the replay fix, tests, merge notes
Migration
The domain is recomputed from
serialized_push_event, which containsurlToComposerFile. 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.comtoraw.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
RenderDocumentationServiceTestpins that the host handed toquarantine()and toupdateLastHit()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
RenderDocumentationServicein the same area. All three related PRs merge ontodevelopcleanly in any order — verified by performing the merges, with an identical resulting tree and a green combined suite.