Gate the R2 mirror URL by the requesting client's own trust - #212
Merged
Conversation
Every FOSSBilling install currently deployed (0.8.6 and older) rejects download.fossbilling.org outright with 'The download URL for this release was not specified as a trusted one. Update canceled for security reasons.' - Update::$allowedDownloadPrefixes only started trusting that host in 0.8.7 (FOSSBilling/FOSSBilling#4255), but /versions/v1/latest has been unconditionally preferring the R2 mirror over the GitHub asset for every requester since that release shipped, breaking auto-update for the entire pre-0.8.7 install base. Gate download_url/digest per-request instead of baking a single choice into the shared release cache: FOSSBilling's own HTTP client already sends 'User-Agent: FOSSBilling/{version}' on this exact request (added in 0.8.4 - src/di.php in the main repo), so no client-side change is needed. A client reporting >=0.8.7 gets the R2 mirror; anything older, or unparseable, or missing the header entirely (<=0.8.3 predates it), falls back to the GitHub URL every historical version already trusts. The shared release cache (also written by the stats service's fetch-refresh path) now stores both download_url (GitHub) and mirror_download_url/mirror_digest (R2, nullable) rather than resolving to one; resolveReleaseForClient() picks between them per response and strips the internal mirror_* fields from what's actually returned. Added Vary: User-Agent on the affected routes so this can't be cached across clients with different trust. npm run typecheck/lint/test: all clean, 527 tests pass.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
api | 879fe90 | Commit Preview URL Branch Preview URL |
Sep 01 2026, 10:20 AM |
Narrowing directly on the destructured mirrorDownloadUrl (rather than a separately-computed useMirror boolean derived from the pre-destructure property) lets TypeScript's own control-flow analysis prove non-null inside the branch, so the '!' assertion isn't needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
parseCachedReleases() normalized a missing digest to null but not the new mirror_download_url/mirror_digest fields. A cache entry written by the pre-fix code (i.e. what's live in production right now) lacks those keys entirely, so they read back as undefined - and resolveReleaseForClient()'s `!== null` check treats undefined as "has a mirror", swapping in an undefined download_url that JSON then drops from the response. A trusting (>=0.8.7) client hitting stale cache would get a response with no download_url at all, for up to 24h post-deploy. Reproduced with a seeded legacy-shaped cache entry before fixing. npm run typecheck/lint/test: all clean, 528 tests pass.
Every existing R2-mirror test makes exactly one request against a freshly emptied cache (beforeEach deletes gh-fossbilling-releases), so each resolves its own download_url from a fresh GitHub+R2 fetch. None of them exercised the actual point of resolveReleaseForClient(): the release cache is shared across every requester, and each response must be resolved against that specific request's own User-Agent, not baked into the cache from whichever client happened to warm it. A version that wrongly cached a single resolved URL (using the first requester's trust) would have passed the full suite as it stood. Added two tests that warm the shared cache with one client and assert a second client with different trust, served from that same cache (no second GitHub fetch - asserted via the ghRequest mock's call count), gets its own correctly-resolved URL/digest: an 0.8.7 client getting the mirror after an 0.8.6 client warmed the cache, and vice versa. npm run typecheck/lint/test: all clean, 530 tests pass.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Backfilling a missing mirror_download_url/mirror_digest to null (the previous fix) only patches the two new keys - it does nothing to download_url itself. Between #207 shipping (R2 preferred unconditionally, no mirror_* fields yet) and this gating fix, download_url for a mirrored release WAS the R2 mirror URL, with no separate field anywhere preserving the GitHub URL. Backfilling can't repair that: there's nothing to fall back to in the entry, so every client - trusting or not - would keep reading download_url straight off the poisoned cache. Given the 24h cache TTL and that #207 has been live well over 24h, this is what's actually sitting in production right now. parseCachedReleases() now treats any release missing both mirror fields as untrustworthy and invalidates the whole cache (returns null), the same fallback already used for corrupt JSON - forcing getReleases() to do a fresh GitHub+R2 fetch that rebuilds every entry with its GitHub and mirror URLs kept separate again. Reproduced the poisoned-cache case with a seeded entry (download_url = the R2 URL, no mirror_* keys) and confirmed - by temporarily reverting just the source fix - that the regression test fails without it (ghRequest never called; poisoned entry served as-is to every client). Updated test/services/versions/v1/errors.test.ts's pre-existing 'should serve cached data when available' test: its fixture predates mirror fields entirely (unrelated to this fix - it was only ever testing digest-null backfill), so it now correctly triggers invalidation too. Added mirror_download_url/mirror_digest: null to keep it isolated to the digest-backfill path it's meant to cover. npm run typecheck/lint/test: all clean, 531 tests pass.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The invalidation guard checked mirror_download_url === undefined AND mirror_digest === undefined, so a one-sided pair (one field present, the other missing) slipped through untouched. getReleases() always writes both fields together, so a partial pair can only mean a malformed entry - but resolveReleaseForClient() doesn't know that: for a trusting client it pairs whichever mirror field IS present with the other one's undefined value, and JSON drops that undefined key from the response entirely. A present mirror_download_url with a missing mirror_digest serves a client a download_url with no digest; the reverse loses download_url instead - the exact failure mode the previous fix closed for the fully-legacy case, still open here for the partial one. Changed the guard from && to ||. Reproduced first (seeded a cache entry with mirror_download_url present and mirror_digest missing; a trusting client's response came back with digest missing entirely, served straight from cache with no rebuild) before fixing. npm run typecheck/lint/test: all clean, 532 tests pass.
Contributor
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Auto-approved: Fixes a security-relevant bug where the R2 mirror URL was sent to clients that reject it, by gating it on the requesting client's version and falling back to GitHub for older clients. Adds per-request resolution, Vary: User-Agent, and thorough tests.
Re-trigger cubic
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
1 participant
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.
Gate
download_url/digestper-request instead of baking a single choice into the shared release cache. FOSSBilling's own HTTP client already sendsUser-Agent: FOSSBilling/{version}on this exact request (added in 0.8.4), so no client-side change is needed:>=0.8.7→ R2 mirror URL (the IPv6 fix works as intended)User-Agentat all (<=0.8.3 predates the header) → GitHub URL, which every historical version already trustsThis is deliberately keyed off the requesting client's own version, not which release is "latest" - so it stays correct indefinitely (e.g. a straggler still on 0.8.6 who skips straight to a future 0.8.8+ will still correctly get the GitHub URL until their own client reports 0.8.7+), rather than only patching this one version boundary.
The shared release cache (also written by the stats service's fetch-refresh path) now stores both
download_url(GitHub) andmirror_download_url/mirror_digest(R2, nullable) rather than resolving to one at cache-build time;resolveReleaseForClient()picks between them per response and strips the internalmirror_*fields from what's actually returned to clients. AddedVary: User-Agenton the affected routes so this can't be cached across clients with different trust.