From 68c912747b499ec42f6ceee34a1eafbf1a500ae1 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 1 Sep 2026 10:43:11 +0100 Subject: [PATCH 1/6] Gate the R2 mirror URL by the requesting client's own trust 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. --- src/services/versions/v1/index.ts | 96 ++++++++++++++++++--- src/services/versions/v1/interfaces.ts | 18 ++++ test/services/stats/v1/index.test.ts | 19 +++-- test/services/versions/v1/index.test.ts | 108 +++++++++++++++++++++++- 4 files changed, 220 insertions(+), 21 deletions(-) diff --git a/src/services/versions/v1/index.ts b/src/services/versions/v1/index.ts index 59e0db8..ed6bb48 100644 --- a/src/services/versions/v1/index.ts +++ b/src/services/versions/v1/index.ts @@ -12,7 +12,7 @@ import { gte as semverGte, valid as semverValid } from "semver"; -import { Releases, ReleaseDetails } from "./interfaces"; +import { Releases, ReleaseDetails, ResolvedReleaseDetails } from "./interfaces"; import { getReleaseR2Object } from "./r2"; import { getPlatform } from "../../../lib/middleware"; import { ICache } from "../../../lib/interfaces"; @@ -124,6 +124,56 @@ function buildSuccessResponse( }; } +// FOSSBilling's own Update.php sends this on every request already (src/di.php, +// added in 0.8.4), so no client change is needed to gate the R2 mirror by version. +// Anything we can't confidently read a trusted version out of - no header at all +// (<=0.8.3, which predates this header), or a version we don't recognize - falls +// through to `false`, matching the fully-backward-compatible default in +// resolveReleaseForClient() below. +const USER_AGENT_VERSION_PATTERN = /^FOSSBilling\/(.+)$/; + +// First version whose Update::$allowedDownloadPrefixes trusts +// download.fossbilling.org. Older clients reject that URL outright with +// "Update canceled for security reasons" - see FOSSBilling/FOSSBilling +// incident following #4255/#2479. +const MIRROR_TRUST_MIN_VERSION = "0.8.7"; + +function clientTrustsMirror(userAgent: string | undefined | null): boolean { + if (!userAgent) return false; + + const match = userAgent.match(USER_AGENT_VERSION_PATTERN); + if (!match) return false; + + const version = match[1]; + if (!semverValid(version)) return false; + + return semverGte(version, MIRROR_TRUST_MIN_VERSION); +} + +// Resolves a release's dual (GitHub + optional R2 mirror) download info down to +// the single URL/digest pair this specific requester should be sent, and drops +// the internal-only mirror_* fields from the public response shape. +function resolveReleaseForClient( + release: ReleaseDetails, + userAgent: string | undefined | null +): ResolvedReleaseDetails { + const useMirror = + release.mirror_download_url !== null && clientTrustsMirror(userAgent); + + const { + mirror_download_url: mirrorDownloadUrl, + mirror_digest: mirrorDigest, + ...resolved + } = release; + + if (useMirror) { + resolved.download_url = mirrorDownloadUrl!; + resolved.digest = mirrorDigest; + } + + return resolved; +} + interface ReleaseAsset { name: string; browser_download_url: string; @@ -152,9 +202,19 @@ registerCachedRoute("/", async (c) => { if (hasNoReleases(releases)) { c.header("Vary", "*"); + } else { + c.header("Vary", "User-Agent"); } - return c.json(buildSuccessResponse(releases, result.source)); + const userAgent = c.req.header("User-Agent"); + const resolvedReleases = Object.fromEntries( + Object.entries(releases).map(([tag, release]) => [ + tag, + resolveReleaseForClient(release, userAgent) + ]) + ); + + return c.json(buildSuccessResponse(resolvedReleases, result.source)); }); versionsV1.get( @@ -310,17 +370,26 @@ registerCachedRoute("/:version", async (c) => { }); } + c.header("Vary", "User-Agent"); + const userAgent = c.req.header("User-Agent"); + if (version === "latest") { const sortedKeys = Object.keys(releases).sort(semverCompare); const lastKey = sortedKeys.at(-1); + const resolved = lastKey + ? resolveReleaseForClient(releases[lastKey], userAgent) + : null; - return c.json( - buildSuccessResponse(lastKey ? releases[lastKey] : null, result.source) - ); + return c.json(buildSuccessResponse(resolved, result.source)); } if (version in releases) { - return c.json(buildSuccessResponse(releases[version], result.source)); + return c.json( + buildSuccessResponse( + resolveReleaseForClient(releases[version], userAgent), + result.source + ) + ); } c.status(404); @@ -473,9 +542,11 @@ export async function getReleases( } } - // Prefer the R2 mirror over the GitHub asset - github.com has no AAAA - // record, so IPv6-only hosts can't reach it. Fall back to GitHub for - // releases that predate mirroring or if the R2 lookup fails. + // Record both the GitHub asset and the R2 mirror (if this release has + // one) rather than resolving to a single download_url here - which of + // the two a given client should actually be sent depends on whether + // *that client's own version* trusts download.fossbilling.org, and is + // decided per-request in resolveReleaseForClient(). const releaseEntries: [string, ReleaseDetails][] = await Promise.all( releasesToProcess.map( async ({ tag, release, zipAsset, cachedPhpVersion }) => { @@ -499,13 +570,14 @@ export async function getReleases( version: release.name || tag, released_on: release.published_at ?? "", minimum_php_version: phpVersion, - download_url: - r2Object?.downloadUrl ?? zipAsset.browser_download_url, + download_url: zipAsset.browser_download_url, + mirror_download_url: r2Object?.downloadUrl ?? null, size_bytes: zipAsset.size, is_prerelease: Boolean(release.prerelease), github_release_id: release.id ?? 0, changelog: release.body || "", - digest: r2Object?.digest ?? zipAsset.digest ?? null + digest: zipAsset.digest ?? null, + mirror_digest: r2Object?.digest ?? null }; return [tag, releaseDetails]; } diff --git a/src/services/versions/v1/interfaces.ts b/src/services/versions/v1/interfaces.ts index e1e11a4..2bffd34 100644 --- a/src/services/versions/v1/interfaces.ts +++ b/src/services/versions/v1/interfaces.ts @@ -2,7 +2,14 @@ export type ReleaseDetails = { version: string; released_on: string; minimum_php_version: string; + // Always the GitHub asset URL - every FOSSBilling version ever released + // trusts this, so it's the safe default for a client we can't identify. download_url: string; + // The R2 mirror URL, if this release has one. Only versions >= + // MIRROR_TRUST_MIN_VERSION have download.fossbilling.org in their signed + // download-URL allowlist (Update::$allowedDownloadPrefixes) and will + // accept it - see resolveReleaseForClient(). + mirror_download_url: string | null; size_bytes: number; is_prerelease: boolean; github_release_id: number; @@ -10,8 +17,19 @@ export type ReleaseDetails = { // SHA-256 digest of the release zip (`sha256:`), as computed by // GitHub; null for older assets predating GitHub's digest support. digest: string | null; + // Digest for mirror_download_url. Set alongside it from the R2 object's + // own custom metadata - see getReleaseR2Object(). + mirror_digest: string | null; }; +// The public response shape returned to FOSSBilling clients - download_url/digest +// already resolved to whichever source (GitHub or the R2 mirror) that specific +// client trusts. See resolveReleaseForClient(). +export type ResolvedReleaseDetails = Omit< + ReleaseDetails, + "mirror_download_url" | "mirror_digest" +>; + export type Releases = { [version: string]: ReleaseDetails; }; diff --git a/test/services/stats/v1/index.test.ts b/test/services/stats/v1/index.test.ts index d2e5dfa..2350f98 100644 --- a/test/services/stats/v1/index.test.ts +++ b/test/services/stats/v1/index.test.ts @@ -262,10 +262,16 @@ describe("Stats API v1", () => { describe("Shared release cache", () => { // getReleases writes gh-fossbilling-releases - the same cache key the // versions service reads - so a stats-triggered fresh fetch must still - // resolve R2 download_url/digest. Otherwise stats would overwrite that - // cache with GitHub-only URLs for up to 24h, silently undoing the R2 - // preference for IPv6-only hosts. See FOSSBilling/FOSSBilling#2479. - it("resolves R2 download_url/digest when it triggers the shared release fetch", async () => { + // resolve the R2 mirror. Otherwise stats would overwrite that cache with + // GitHub-only entries for up to 24h, silently undoing the R2 mirror for + // clients that trust it. See FOSSBilling/FOSSBilling#2479. + // + // The cache stores both download_url (GitHub - always trusted) and + // mirror_download_url (R2, when mirrored); which one a given FOSSBilling + // install is actually sent is resolved per-request in the versions + // service based on that client's own reported version, not baked into + // this shared cache - see resolveReleaseForClient() there. + it("resolves both the GitHub and R2 mirror URLs when it triggers the shared release fetch", async () => { await env.DOWNLOAD_BUCKET.put( "releases/0.6.0/FOSSBilling-0.6.0.zip", "mirrored archive contents", @@ -292,9 +298,12 @@ describe("Stats API v1", () => { const releases = JSON.parse(cached!); expect(releases["0.6.0"].download_url).toBe( + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip" + ); + expect(releases["0.6.0"].mirror_download_url).toBe( "https://download.fossbilling.org/releases/0.6.0/FOSSBilling-0.6.0.zip" ); - expect(releases["0.6.0"].digest).toBe( + expect(releases["0.6.0"].mirror_digest).toBe( "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000" ); }); diff --git a/test/services/versions/v1/index.test.ts b/test/services/versions/v1/index.test.ts index 98a34f1..04d25d4 100644 --- a/test/services/versions/v1/index.test.ts +++ b/test/services/versions/v1/index.test.ts @@ -221,8 +221,13 @@ describe("Versions API v1", () => { describe("R2 release mirror", () => { // github.com has no AAAA record, so IPv6-only hosts must download - // from the R2 mirror instead - see FOSSBilling/FOSSBilling#2479. - it("prefers the R2 mirror's download_url and digest when mirrored", async () => { + // from the R2 mirror instead - see FOSSBilling/FOSSBilling#2479. But + // only clients whose own version trusts download.fossbilling.org + // (Update::$allowedDownloadPrefixes, added alongside the mirror itself) + // should ever be sent that URL - anything older rejects it outright + // with "Update canceled for security reasons" (the incident this + // describe block guards against). + async function mirrorRelease060() { await env.DOWNLOAD_BUCKET.put( "releases/0.6.0/FOSSBilling-0.6.0.zip", "mirrored archive contents", @@ -234,12 +239,22 @@ describe("Versions API v1", () => { } } ); + } + + it("prefers the R2 mirror's download_url and digest for a client that trusts it", async () => { + await mirrorRelease060(); const ctx = createExecutionContext(); - const response = await app.request("/versions/v1/latest", {}, env, ctx); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.7" } }, + env, + ctx + ); await waitOnExecutionContext(ctx); expect(response.status).toBe(200); + expect(response.headers.get("Vary")).toContain("User-Agent"); const data: ApiResponse = await response.json(); if (!data.result) { throw new Error("Expected latest release data"); @@ -254,7 +269,12 @@ describe("Versions API v1", () => { it("falls back to the GitHub asset when a release hasn't been mirrored to R2", async () => { const ctx = createExecutionContext(); - const response = await app.request("/versions/v1/latest", {}, env, ctx); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.7" } }, + env, + ctx + ); await waitOnExecutionContext(ctx); expect(response.status).toBe(200); @@ -269,6 +289,86 @@ describe("Versions API v1", () => { "sha256:9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08" ); }); + + it("falls back to the GitHub asset for a client older than the mirror-trust cutoff, even when mirrored", async () => { + await mirrorRelease060(); + + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.6" } }, + env, + ctx + ); + await waitOnExecutionContext(ctx); + + const data: ApiResponse = await response.json(); + if (!data.result) { + throw new Error("Expected latest release data"); + } + expect(data.result.download_url).toBe( + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip" + ); + expect(data.result.digest).toBe( + "sha256:9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08" + ); + }); + + it("falls back to the GitHub asset for a client sending no User-Agent, even when mirrored", async () => { + // Versions <=0.8.3 predate FOSSBilling's own User-Agent header entirely + // (added in 0.8.4) and send none of their own. + await mirrorRelease060(); + + const ctx = createExecutionContext(); + const response = await app.request("/versions/v1/latest", {}, env, ctx); + await waitOnExecutionContext(ctx); + + const data: ApiResponse = await response.json(); + if (!data.result) { + throw new Error("Expected latest release data"); + } + expect(data.result.download_url).toBe( + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip" + ); + }); + + it("falls back to the GitHub asset for an unparseable User-Agent, even when mirrored", async () => { + await mirrorRelease060(); + + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "curl/8.0.0" } }, + env, + ctx + ); + await waitOnExecutionContext(ctx); + + const data: ApiResponse = await response.json(); + if (!data.result) { + throw new Error("Expected latest release data"); + } + expect(data.result.download_url).toBe( + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip" + ); + }); + + it("never exposes the internal mirror_download_url/mirror_digest fields in the response", async () => { + await mirrorRelease060(); + + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.7" } }, + env, + ctx + ); + await waitOnExecutionContext(ctx); + + const data: ApiResponse = await response.json(); + expect(data.result).not.toHaveProperty("mirror_download_url"); + expect(data.result).not.toHaveProperty("mirror_digest"); + }); }); describe("GET /:version", () => { From 0f47eb97c3dda9e1c8c98d090fa755e7bffe84ca Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 1 Sep 2026 10:47:16 +0100 Subject: [PATCH 2/6] Remove unnecessary non-null assertion in resolveReleaseForClient 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. --- src/services/versions/v1/index.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/services/versions/v1/index.ts b/src/services/versions/v1/index.ts index ed6bb48..cc813fb 100644 --- a/src/services/versions/v1/index.ts +++ b/src/services/versions/v1/index.ts @@ -157,17 +157,14 @@ function resolveReleaseForClient( release: ReleaseDetails, userAgent: string | undefined | null ): ResolvedReleaseDetails { - const useMirror = - release.mirror_download_url !== null && clientTrustsMirror(userAgent); - const { mirror_download_url: mirrorDownloadUrl, mirror_digest: mirrorDigest, ...resolved } = release; - if (useMirror) { - resolved.download_url = mirrorDownloadUrl!; + if (mirrorDownloadUrl !== null && clientTrustsMirror(userAgent)) { + resolved.download_url = mirrorDownloadUrl; resolved.digest = mirrorDigest; } From 2a4529c2c25798d30aa6cc812e2eac3d987a0edd Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 1 Sep 2026 10:52:52 +0100 Subject: [PATCH 3/6] Backfill mirror_download_url/mirror_digest on legacy cache entries 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. --- src/services/versions/v1/index.ts | 15 ++++++-- test/services/versions/v1/index.test.ts | 48 +++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/services/versions/v1/index.ts b/src/services/versions/v1/index.ts index cc813fb..be614ef 100644 --- a/src/services/versions/v1/index.ts +++ b/src/services/versions/v1/index.ts @@ -675,12 +675,23 @@ function parseCachedReleases( try { const parsedCache = JSON.parse(cachedReleases); if (parsedCache && typeof parsedCache === "object") { - // Cache entries written before the `digest` field existed lack the - // key entirely; normalize them to the documented `null` fallback. + // Cache entries written before the `digest`/`mirror_download_url`/ + // `mirror_digest` fields existed lack those keys entirely; normalize + // them to the documented `null` fallback. Otherwise a legacy entry's + // `mirror_download_url` reads back as `undefined`, which + // resolveReleaseForClient()'s `!== null` check treats as "has a + // mirror" - swapping in an `undefined` download_url that JSON then + // drops from the response entirely. for (const release of Object.values(parsedCache as Releases)) { if (release.digest === undefined) { release.digest = null; } + if (release.mirror_download_url === undefined) { + release.mirror_download_url = null; + } + if (release.mirror_digest === undefined) { + release.mirror_digest = null; + } } return parsedCache as Releases; } diff --git a/test/services/versions/v1/index.test.ts b/test/services/versions/v1/index.test.ts index 04d25d4..8734cc3 100644 --- a/test/services/versions/v1/index.test.ts +++ b/test/services/versions/v1/index.test.ts @@ -369,6 +369,54 @@ describe("Versions API v1", () => { expect(data.result).not.toHaveProperty("mirror_download_url"); expect(data.result).not.toHaveProperty("mirror_digest"); }); + + // Regression test: a cache entry in this shape - only `download_url`/ + // `digest`, no `mirror_download_url`/`mirror_digest` keys at all - is + // exactly what the pre-fix code (and therefore the current production + // cache) writes. parseCachedReleases() must backfill the missing keys + // to `null`, or a trusting client's resolveReleaseForClient() treats the + // `undefined` mirror_download_url as present and returns a response + // with download_url missing entirely (JSON drops `undefined` values). + it("backfills missing mirror_download_url/mirror_digest on a legacy-shaped cache entry", async () => { + const legacyCachedReleases = { + "0.6.0": { + version: "0.6.0", + released_on: "2023-04-01T00:00:00Z", + minimum_php_version: "8.1", + download_url: + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip", + size_bytes: 15485760, + is_prerelease: false, + github_release_id: 987654321, + changelog: "## 0.6.0", + digest: + "sha256:9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08" + } + }; + await env.CACHE_KV.put( + "gh-fossbilling-releases", + JSON.stringify(legacyCachedReleases) + ); + + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.7" } }, + env, + ctx + ); + await waitOnExecutionContext(ctx); + + const data: ApiResponse = await response.json(); + if (!data.result) { + throw new Error("Expected latest release data"); + } + expect(data.result.download_url).toBe( + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip" + ); + expect(data.result).not.toHaveProperty("mirror_download_url"); + expect(data.result).not.toHaveProperty("mirror_digest"); + }); }); describe("GET /:version", () => { From 097aeffce59635345eb6e3262a71927ca79b8849 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 1 Sep 2026 10:56:50 +0100 Subject: [PATCH 4/6] Add shared-cache cross-client test coverage for R2 mirror gating 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. --- test/services/versions/v1/index.test.ts | 95 +++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/test/services/versions/v1/index.test.ts b/test/services/versions/v1/index.test.ts index 8734cc3..50db998 100644 --- a/test/services/versions/v1/index.test.ts +++ b/test/services/versions/v1/index.test.ts @@ -370,6 +370,101 @@ describe("Versions API v1", () => { expect(data.result).not.toHaveProperty("mirror_digest"); }); + // These are the actual point of the fix: the release cache is shared + // across every requester, and each response must still be resolved + // against *that request's own* User-Agent - not whichever client + // happened to trigger the fetch that populated the cache. Every test + // above only ever makes one request, so a version that wrongly baked + // the URL choice into the stored cache (using the first requester's + // trust) would still pass them all. + it("serves the R2 mirror to a trusting client even when an older client warmed the shared cache", async () => { + await mirrorRelease060(); + + const ctx1 = createExecutionContext(); + const warmingResponse = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.6" } }, + env, + ctx1 + ); + await waitOnExecutionContext(ctx1); + const warmingData: ApiResponse = + await warmingResponse.json(); + if (!warmingData.result) { + throw new Error("Expected latest release data"); + } + expect(warmingData.result.download_url).toBe( + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip" + ); + expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(1); + + const ctx2 = createExecutionContext(); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.7" } }, + env, + ctx2 + ); + await waitOnExecutionContext(ctx2); + + // Still just the one GitHub fetch from warming the cache above - + // this request was served from that shared cache, not a fresh fetch. + expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(1); + const data: ApiResponse = await response.json(); + if (!data.result) { + throw new Error("Expected latest release data"); + } + expect(data.result.download_url).toBe( + "https://download.fossbilling.org/releases/0.6.0/FOSSBilling-0.6.0.zip" + ); + expect(data.result.digest).toBe( + "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000" + ); + }); + + it("falls back to the GitHub asset for an older client even when a trusting client warmed the shared cache", async () => { + await mirrorRelease060(); + + const ctx1 = createExecutionContext(); + const warmingResponse = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.7" } }, + env, + ctx1 + ); + await waitOnExecutionContext(ctx1); + const warmingData: ApiResponse = + await warmingResponse.json(); + if (!warmingData.result) { + throw new Error("Expected latest release data"); + } + expect(warmingData.result.download_url).toBe( + "https://download.fossbilling.org/releases/0.6.0/FOSSBilling-0.6.0.zip" + ); + expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(1); + + const ctx2 = createExecutionContext(); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.6" } }, + env, + ctx2 + ); + await waitOnExecutionContext(ctx2); + + expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(1); + const data: ApiResponse = await response.json(); + if (!data.result) { + throw new Error("Expected latest release data"); + } + expect(data.result.download_url).toBe( + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip" + ); + expect(data.result.digest).toBe( + "sha256:9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08" + ); + }); + // Regression test: a cache entry in this shape - only `download_url`/ // `digest`, no `mirror_download_url`/`mirror_digest` keys at all - is // exactly what the pre-fix code (and therefore the current production From c716524273e6b69681b0f72b8c091c42f1adb575 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 1 Sep 2026 11:13:46 +0100 Subject: [PATCH 5/6] Invalidate legacy cache entries instead of backfilling mirror fields 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. --- src/services/versions/v1/index.ts | 36 ++++++--- test/services/versions/v1/errors.test.ts | 7 +- test/services/versions/v1/index.test.ts | 94 ++++++++++++++++++++++-- 3 files changed, 116 insertions(+), 21 deletions(-) diff --git a/src/services/versions/v1/index.ts b/src/services/versions/v1/index.ts index be614ef..cd80e51 100644 --- a/src/services/versions/v1/index.ts +++ b/src/services/versions/v1/index.ts @@ -675,22 +675,34 @@ function parseCachedReleases( try { const parsedCache = JSON.parse(cachedReleases); if (parsedCache && typeof parsedCache === "object") { - // Cache entries written before the `digest`/`mirror_download_url`/ - // `mirror_digest` fields existed lack those keys entirely; normalize - // them to the documented `null` fallback. Otherwise a legacy entry's - // `mirror_download_url` reads back as `undefined`, which - // resolveReleaseForClient()'s `!== null` check treats as "has a - // mirror" - swapping in an `undefined` download_url that JSON then - // drops from the response entirely. + // Cache entries written before the `digest` field existed lack the + // key entirely; normalize them to the documented `null` fallback. for (const release of Object.values(parsedCache as Releases)) { if (release.digest === undefined) { release.digest = null; } - if (release.mirror_download_url === undefined) { - release.mirror_download_url = null; - } - if (release.mirror_digest === undefined) { - release.mirror_digest = null; + + // A release missing *both* mirror fields predates them entirely - + // which includes the window when download_url itself was written + // as the (unconditionally preferred) R2 mirror URL, with no + // separate field preserving the GitHub URL every older client + // trusts. There's no `null` to backfill that repairs it: the + // GitHub URL isn't recoverable from this cache entry at all, and + // serving download_url as-is risks handing every client - not + // just old ones - the untrusted mirror URL. Invalidate the whole + // cache so getReleases() falls through to a fresh fetch, which + // rebuilds every entry with its GitHub and mirror URLs kept + // separate again. + if ( + release.mirror_download_url === undefined && + release.mirror_digest === undefined + ) { + logWarn( + "versions", + "Cache entry predates mirror fields; invalidating cache to rebuild with separated URLs", + { cacheKey: RELEASE_CACHE_KEY, version: release.version } + ); + return null; } } return parsedCache as Releases; diff --git a/test/services/versions/v1/errors.test.ts b/test/services/versions/v1/errors.test.ts index f09278e..da8ecd2 100644 --- a/test/services/versions/v1/errors.test.ts +++ b/test/services/versions/v1/errors.test.ts @@ -249,7 +249,12 @@ describe("Versions API v1 - Error Handling", () => { size_bytes: 1000, is_prerelease: false, github_release_id: 1, - changelog: "Release notes" + changelog: "Release notes", + // Present (as null) so this entry isn't treated as predating the + // mirror fields entirely - see the "R2 release mirror" describe + // block in index.test.ts for that invalidation behavior. + mirror_download_url: null, + mirror_digest: null } }) ); diff --git a/test/services/versions/v1/index.test.ts b/test/services/versions/v1/index.test.ts index 50db998..37f78b9 100644 --- a/test/services/versions/v1/index.test.ts +++ b/test/services/versions/v1/index.test.ts @@ -465,14 +465,13 @@ describe("Versions API v1", () => { ); }); - // Regression test: a cache entry in this shape - only `download_url`/ - // `digest`, no `mirror_download_url`/`mirror_digest` keys at all - is - // exactly what the pre-fix code (and therefore the current production - // cache) writes. parseCachedReleases() must backfill the missing keys - // to `null`, or a trusting client's resolveReleaseForClient() treats the - // `undefined` mirror_download_url as present and returns a response - // with download_url missing entirely (JSON drops `undefined` values). - it("backfills missing mirror_download_url/mirror_digest on a legacy-shaped cache entry", async () => { + // Regression test: a cache entry missing both mirror_* keys entirely - + // whether its download_url happens to be the GitHub URL (this case) or + // the R2 mirror URL (next case) - predates those fields and can't be + // trusted as-is. parseCachedReleases() must invalidate it and force a + // fresh fetch, not serve it (backfilling the missing keys to `null` + // isn't enough - see the next test for why). + it("invalidates a legacy-shaped cache entry and rebuilds it via a fresh fetch", async () => { const legacyCachedReleases = { "0.6.0": { version: "0.6.0", @@ -502,6 +501,9 @@ describe("Versions API v1", () => { ); await waitOnExecutionContext(ctx); + // A live fetch happened rather than the legacy entry being served + // (backfilled or otherwise) straight from cache. + expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(1); const data: ApiResponse = await response.json(); if (!data.result) { throw new Error("Expected latest release data"); @@ -512,6 +514,82 @@ describe("Versions API v1", () => { expect(data.result).not.toHaveProperty("mirror_download_url"); expect(data.result).not.toHaveProperty("mirror_digest"); }); + + // The dangerous case: between #207 (R2 preferred unconditionally, no + // mirror_* fields yet) and this gating fix shipping, a mirrored + // release's cached download_url was the R2 mirror URL itself, with no + // separate field preserving the GitHub URL. Backfilling the missing + // mirror_* keys to `null` can't repair that - there's nothing to fall + // back to in the entry itself - and would leave every client, old and + // new, reading download_url straight off this poisoned entry. Given the + // 24h cache TTL and that #207 shipped over 24h before this fix, this is + // what's actually sitting in the production cache right now. + it("invalidates a legacy cache entry poisoned with the R2 URL as download_url, for clients of any trust", async () => { + await mirrorRelease060(); + const poisonedCachedReleases = { + "0.6.0": { + version: "0.6.0", + released_on: "2023-04-01T00:00:00Z", + minimum_php_version: "8.1", + download_url: + "https://download.fossbilling.org/releases/0.6.0/FOSSBilling-0.6.0.zip", + size_bytes: 15485760, + is_prerelease: false, + github_release_id: 987654321, + changelog: "## 0.6.0", + digest: + "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000" + } + }; + await env.CACHE_KV.put( + "gh-fossbilling-releases", + JSON.stringify(poisonedCachedReleases) + ); + + const oldClientCtx = createExecutionContext(); + const oldClientResponse = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.6" } }, + env, + oldClientCtx + ); + await waitOnExecutionContext(oldClientCtx); + + expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(1); + const oldClientData: ApiResponse = + await oldClientResponse.json(); + if (!oldClientData.result) { + throw new Error("Expected latest release data"); + } + expect(oldClientData.result.download_url).toBe( + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip" + ); + + // The rebuilt cache is now correctly shaped, so a trusting client + // right after gets the mirror - from that same (now healthy) cache, + // with no second live fetch. + const newClientCtx = createExecutionContext(); + const newClientResponse = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.7" } }, + env, + newClientCtx + ); + await waitOnExecutionContext(newClientCtx); + + expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(1); + const newClientData: ApiResponse = + await newClientResponse.json(); + if (!newClientData.result) { + throw new Error("Expected latest release data"); + } + expect(newClientData.result.download_url).toBe( + "https://download.fossbilling.org/releases/0.6.0/FOSSBilling-0.6.0.zip" + ); + expect(newClientData.result.digest).toBe( + "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000" + ); + }); }); describe("GET /:version", () => { From 879fe90b76bca3256df3a065a1a3e79baa45c853 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 1 Sep 2026 11:20:14 +0100 Subject: [PATCH 6/6] Invalidate cache entries with only one mirror field present 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. --- src/services/versions/v1/index.ts | 16 ++++--- test/services/versions/v1/index.test.ts | 56 +++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/src/services/versions/v1/index.ts b/src/services/versions/v1/index.ts index cd80e51..2948dbc 100644 --- a/src/services/versions/v1/index.ts +++ b/src/services/versions/v1/index.ts @@ -682,19 +682,25 @@ function parseCachedReleases( release.digest = null; } - // A release missing *both* mirror fields predates them entirely - + // A release missing *either* mirror field predates them (both are + // always written together - see getReleases() below - so a partial + // pair means this entry is malformed or from before they existed), // which includes the window when download_url itself was written // as the (unconditionally preferred) R2 mirror URL, with no // separate field preserving the GitHub URL every older client // trusts. There's no `null` to backfill that repairs it: the // GitHub URL isn't recoverable from this cache entry at all, and // serving download_url as-is risks handing every client - not - // just old ones - the untrusted mirror URL. Invalidate the whole - // cache so getReleases() falls through to a fresh fetch, which - // rebuilds every entry with its GitHub and mirror URLs kept + // just old ones - the untrusted mirror URL. A one-sided pair is + // just as unsafe: resolveReleaseForClient() would pair a real + // mirror_download_url with an undefined mirror_digest (or vice + // versa) for a trusting client, and JSON drops that undefined key + // from the response entirely - incomplete update metadata. Invalidate + // the whole cache so getReleases() falls through to a fresh fetch, + // which rebuilds every entry with its GitHub and mirror URLs kept // separate again. if ( - release.mirror_download_url === undefined && + release.mirror_download_url === undefined || release.mirror_digest === undefined ) { logWarn( diff --git a/test/services/versions/v1/index.test.ts b/test/services/versions/v1/index.test.ts index 37f78b9..712173b 100644 --- a/test/services/versions/v1/index.test.ts +++ b/test/services/versions/v1/index.test.ts @@ -590,6 +590,62 @@ describe("Versions API v1", () => { "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000" ); }); + + // A one-sided pair (one mirror field present, the other missing) is just + // as unsafe as both being missing: resolveReleaseForClient() would pair + // a real mirror_download_url with an undefined mirror_digest for a + // trusting client, and JSON drops that undefined key from the response + // entirely - incomplete update metadata (missing digest here; a present + // mirror_digest with a missing mirror_download_url loses download_url + // instead, the same failure mode as the fully-legacy case above). + it("invalidates a cache entry with only one of the two mirror fields present", async () => { + await mirrorRelease060(); + const partialCachedReleases = { + "0.6.0": { + version: "0.6.0", + released_on: "2023-04-01T00:00:00Z", + minimum_php_version: "8.1", + download_url: + "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.6.0/FOSSBilling.zip", + size_bytes: 15485760, + is_prerelease: false, + github_release_id: 987654321, + changelog: "## 0.6.0", + digest: + "sha256:9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08", + mirror_download_url: + "https://download.fossbilling.org/releases/0.6.0/FOSSBilling-0.6.0.zip" + // mirror_digest is missing entirely + } + }; + await env.CACHE_KV.put( + "gh-fossbilling-releases", + JSON.stringify(partialCachedReleases) + ); + + const ctx = createExecutionContext(); + const response = await app.request( + "/versions/v1/latest", + { headers: { "User-Agent": "FOSSBilling/0.8.7" } }, + env, + ctx + ); + await waitOnExecutionContext(ctx); + + // A live fetch happened rather than the malformed entry being served + // straight from cache. + expect(vi.mocked(ghRequest)).toHaveBeenCalledTimes(1); + const data: ApiResponse = await response.json(); + if (!data.result) { + throw new Error("Expected latest release data"); + } + expect(data.result.download_url).toBe( + "https://download.fossbilling.org/releases/0.6.0/FOSSBilling-0.6.0.zip" + ); + expect(data.result.digest).toBe( + "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000" + ); + }); }); describe("GET /:version", () => {