diff --git a/apps/api/src/storage-verify.test.ts b/apps/api/src/storage-verify.test.ts index 160e6a77..dc907d20 100644 --- a/apps/api/src/storage-verify.test.ts +++ b/apps/api/src/storage-verify.test.ts @@ -385,6 +385,60 @@ describe("verifyStorageConfig — recommended public-URL probe", () => { expect(fetchImpl).toHaveBeenCalledOnce(); }); + it("warns (embed-cache) when the domain serves cacheable headers, without gating ok", async () => { + const client = new FakeStorageClient(); + const fetchImpl = vi.fn(async () => { + const [key] = [...client.store.keys()]; + return new Response(client.store.get(key)!, { + status: 200, + headers: { "cache-control": "public, max-age=14400" }, + }); + }); + const result = await run( + { ...VALID, publicBaseUrl: "https://media.example.com" }, + client, + fetchImpl as unknown as typeof fetch, + ); + expect(result.ok).toBe(true); + const embedCache = result.checks.find((c) => c.id === "embed-cache")!; + expect(embedCache.ok).toBe(false); + expect(embedCache.required).toBe(false); + expect(embedCache.hint).toMatch(/optional but recommended/); + expect(embedCache.hint).toMatch(/Transform Rule/); + }); + + it("passes embed-cache when Cache-Control carries no-store/no-cache", async () => { + const client = new FakeStorageClient(); + const fetchImpl = vi.fn(async () => { + const [key] = [...client.store.keys()]; + return new Response(client.store.get(key)!, { + status: 200, + headers: { "cache-control": "max-age=0, no-cache, no-store, must-revalidate" }, + }); + }); + const result = await run( + { ...VALID, publicBaseUrl: "https://media.example.com" }, + client, + fetchImpl as unknown as typeof fetch, + ); + const embedCache = result.checks.find((c) => c.id === "embed-cache")!; + expect(embedCache.ok).toBe(true); + expect(embedCache.hint).toBeUndefined(); + }); + + it("omits embed-cache entirely when the domain couldn't be reached", async () => { + const client = new FakeStorageClient(); + const fetchImpl = vi.fn(async () => { + throw new Error("network error: ENOTFOUND"); + }); + const result = await run( + { ...VALID, publicBaseUrl: "https://media.example.com" }, + client, + fetchImpl as unknown as typeof fetch, + ); + expect(result.checks.find((c) => c.id === "embed-cache")).toBeUndefined(); + }); + it("reports a thrown fetch (DNS/timeout/subrequest failure) as 'couldn't verify from here', not 'domain is broken'", async () => { const client = new FakeStorageClient(); const fetchImpl = vi.fn(async () => { diff --git a/apps/api/src/storage-verify.ts b/apps/api/src/storage-verify.ts index 8681f53e..9dc239cd 100644 --- a/apps/api/src/storage-verify.ts +++ b/apps/api/src/storage-verify.ts @@ -43,7 +43,7 @@ export interface StorageVerifyCandidate { } export interface StorageVerifyCheck { - /** Stable identifier — `"shape" | "auth" | "round-trip" | "not-empty" | "public-url"`. */ + /** Stable identifier — `"shape" | "auth" | "round-trip" | "not-empty" | "public-url" | "embed-cache"`. */ id: string; ok: boolean; /** Required checks gate `StorageVerifyResult.ok`; recommended ones only ever warn. */ @@ -228,31 +228,41 @@ async function checkPublicUrl( probeKey: string, probeBytes: Uint8Array, fetchImpl: typeof fetch, -): Promise { +): Promise<{ check: StorageVerifyCheck; cacheControl: string | null | undefined }> { const url = `${publicBaseUrl.replace(/\/$/, "")}/${probeKey}`; try { const res = await fetchImpl(url, { redirect: "error", signal: AbortSignal.timeout(PUBLIC_URL_PROBE_TIMEOUT_MS), }); + // Read the header before consuming the body: the embed-cache check + // (below) only makes sense when the domain actually answered, so the + // reading rides along with whichever public-url outcome this is. + const cacheControl = res.headers.get("cache-control"); if (!res.ok) { return { - id: "public-url", - ok: false, - required: false, - hint: `fetching the probe object via publicBaseUrl returned HTTP ${res.status} — the domain may be connected to a different bucket, or public access isn't enabled yet`, + check: { + id: "public-url", + ok: false, + required: false, + hint: `fetching the probe object via publicBaseUrl returned HTTP ${res.status} — the domain may be connected to a different bucket, or public access isn't enabled yet`, + }, + cacheControl: undefined, }; } const body = new Uint8Array(await res.arrayBuffer()); if (!bytesEqual(body, probeBytes)) { return { - id: "public-url", - ok: false, - required: false, - hint: "the bytes served from publicBaseUrl didn't match what was just written — this can mean a cached/stale response from an edge in front of the domain rather than a wiring problem; try again in a minute", + check: { + id: "public-url", + ok: false, + required: false, + hint: "the bytes served from publicBaseUrl didn't match what was just written — this can mean a cached/stale response from an edge in front of the domain rather than a wiring problem; try again in a minute", + }, + cacheControl, }; } - return { id: "public-url", ok: true, required: false }; + return { check: { id: "public-url", ok: true, required: false }, cacheControl }; } catch { // A thrown fetch means this probe — run from inside the API worker — // couldn't reach the domain; it does NOT mean the domain is broken. A @@ -261,14 +271,39 @@ async function checkPublicUrl( // #783). Say "we couldn't verify it from here", not "your domain is // broken", and point at the one check that actually settles it. return { - id: "public-url", - ok: false, - required: false, - hint: "we couldn't verify publicBaseUrl from here — this can happen even when the domain is working fine (a same-account custom domain isn't always reachable as a server-side request). Open a known object's URL in a browser to check for yourself; if that loads, the domain is fine.", + check: { + id: "public-url", + ok: false, + required: false, + hint: "we couldn't verify publicBaseUrl from here — this can happen even when the domain is working fine (a same-account custom domain isn't always reachable as a server-side request). Open a known object's URL in a browser to check for yourself; if that loads, the domain is fine.", + }, + cacheControl: undefined, }; } } +/** + * Recommended, non-blocking check (issue #592): whether the custom domain + * serves the badge-style no-cache headers GitHub's Camo proxy needs to + * revalidate an image after an in-place overwrite — the same Transform Rule + * `embed.uploads.sh` carries on hosted storage. Only emitted when the + * public-URL probe actually got a response; `null` (header absent) counts as + * missing. `no-store` or `no-cache` in Cache-Control is what makes Camo + * refetch reliably; a plain short max-age is not enough (issue #152). + */ +function checkEmbedCache(cacheControl: string | null): StorageVerifyCheck { + const value = (cacheControl ?? "").toLowerCase(); + const ok = value.includes("no-store") || value.includes("no-cache"); + return { + id: "embed-cache", + ok, + required: false, + hint: ok + ? undefined + : 'optional but recommended: add a Cloudflare Transform Rule on this domain setting Cache-Control to "max-age=0, no-cache, no-store, must-revalidate" so GitHub embeds refresh when a file is overwritten in place — see the setup guide (/docs/byo-bucket)', + }; +} + /** Warning shown when no `publicBaseUrl` is configured — signed-only mode is allowed but degraded (issue #783 follow-up comment). */ const NO_PUBLIC_URL_HINT = "no public base URL set — files will only be reachable through signed links that expire after an hour, and embeds/galleries won't work. Add one any time; it applies retroactively to files already uploaded (URLs are derived on request, nothing is stored per file)."; @@ -339,6 +374,7 @@ export async function verifyStorageConfig( let roundTripOk = false; let roundTripHint: string | undefined; let publicUrlCheck: StorageVerifyCheck | undefined; + let publicUrlCacheControl: string | null | undefined; try { await client.upload(probeKey, probeBytes, { contentType: "application/octet-stream" }); const downloaded = await client.download(probeKey); @@ -348,12 +384,9 @@ export async function verifyStorageConfig( roundTripHint = "wrote and read back a probe object but the bytes didn't match — check for another writer racing this bucket"; } else if (candidate.publicBaseUrl) { - publicUrlCheck = await checkPublicUrl( - candidate.publicBaseUrl, - probeKey, - probeBytes, - fetchImpl, - ); + const probed = await checkPublicUrl(candidate.publicBaseUrl, probeKey, probeBytes, fetchImpl); + publicUrlCheck = probed.check; + publicUrlCacheControl = probed.cacheControl; } } catch (err) { roundTripHint = @@ -396,6 +429,12 @@ export async function verifyStorageConfig( hint: "skipped — the write/read round-trip failed before the public URL could be verified", }, ); + // Cache-rule reading for GitHub embeds (issue #592) — only when the + // domain answered, so the check never piles "couldn't check" noise on + // top of an unreachable-domain public-url failure. + if (publicUrlCacheControl !== undefined) { + checks.push(checkEmbedCache(publicUrlCacheControl)); + } } else { // Signed-only mode is allowed, but it's a degraded state, not a neutral // default — flag it the same warning-level way a failed recommended diff --git a/apps/web/src/pages/account/workspaces/[name]/settings/storage.astro b/apps/web/src/pages/account/workspaces/[name]/settings/storage.astro index 6b73fb44..e778ce1b 100644 --- a/apps/web/src/pages/account/workspaces/[name]/settings/storage.astro +++ b/apps/web/src/pages/account/workspaces/[name]/settings/storage.astro @@ -282,6 +282,12 @@ if (!workspace) return Astro.redirect("/account/workspaces"); class="text-foreground">Custom Domains. That domain is the public base URL below. +
  • + Optional but recommended: a Transform Rule on that domain setting Cache-Control to max-age=0, no-cache, no-store, must-revalidate keeps GitHub embeds fresh + after overwrites. +

    • Full walkthrough in the setup guide. @@ -540,6 +546,7 @@ if (!workspace) return Astro.redirect("/account/workspaces"); "round-trip": "We couldn't write a test file to this bucket", "not-empty": "This bucket already has files in it", "public-url": "The public base URL didn't serve our test file", + "embed-cache": "GitHub embeds: add a cache rule to this domain", }; let formMode: "connect" | "rotate" = "connect"; @@ -875,7 +882,11 @@ if (!workspace) return Astro.redirect("/account/workspaces"); const hint = check.hint ? `${escapeHtml(check.hint)}` : ""; - return `

  • ${escapeHtml(label)}${hint}
    • `; + // `embed-cache` is an optional-but-recommended tip (#592), not a + // blocker — render it in body color so it doesn't read as an + // error standing between the user and saving. + const tone = check.id === "embed-cache" ? "text-foreground" : "text-destructive"; + return `
  • ${escapeHtml(label)}${hint}
    • `; }) .join(""); } @@ -921,8 +932,13 @@ if (!workspace) return Astro.redirect("/account/workspaces"); } // Any failing check keeps you on the form with the fix in reach — // including `public-url`, which the API treats as recommended but - // this form requires. - if (verify.result.checks.some((check) => !check.ok)) { + // this form requires. The one exception is `embed-cache` (#592): + // optional but recommended, so it never blocks the save — the tip + // is surfaced after the lane card appears instead. + const embedCacheWarn = verify.result.checks.find( + (check) => check.id === "embed-cache" && !check.ok, + ); + if (verify.result.checks.some((check) => !check.ok && check.id !== "embed-cache")) { storageSaveBtn.disabled = false; renderFailures(verify.result); revealAdoptRowIfNotEmpty(verify.result); @@ -940,6 +956,16 @@ if (!workspace) return Astro.redirect("/account/workspaces"); // lane card appearing IS the confirmation — no "Saved." status // line lingering as page furniture (#788). applyStatus(saved.status); + // Saved fine, but the domain serves cacheable headers: leave the + // optional-but-recommended cache-rule tip on whichever lane card + // is now showing (#592). Plain text, no error state — the save + // succeeded. + if (embedCacheWarn) { + const tip = + "Optional but recommended: add a cache rule to this domain so GitHub embeds refresh after overwrites — see the setup guide."; + storageSavedActionStatus.textContent = tip; + storageActionStatus.textContent = tip; + } return; } if (saved.kind === "invalid") { diff --git a/apps/web/src/pages/docs/byo-bucket.astro b/apps/web/src/pages/docs/byo-bucket.astro index d4e23ce2..8429a3a4 100644 --- a/apps/web/src/pages/docs/byo-bucket.astro +++ b/apps/web/src/pages/docs/byo-bucket.astro @@ -33,7 +33,7 @@ const TOC = [

    A workspace admin connects from the workspace settings page — one form, filled from three - things you set up on the Cloudflare dashboard first. + things you set up on the Cloudflare dashboard first (plus one optional cache rule).

    1. Create an R2 bucket

    @@ -62,6 +62,21 @@ const TOC = [ https://media.example.com) into the form as the public base URL. The settings page requires one — it's what makes links stable and embeddable. See the serving matrix below.

    +

    4. Optional but recommended: add a cache rule for GitHub embeds

    +

    + On hosted storage, images embedded in GitHub comments refresh when a file is overwritten in + place, because a dedicated embed host serves badge-style no-cache headers that GitHub's Camo + proxy revalidates. You can give your own domain the same behavior with one rule on your zone: + Rules → Transform Rules → + Modify Response Header, matching your domain's hostname, setting + Cache-Control to max-age=0, no-cache, no-store, must-revalidate. + Verification checks for it and reminds you if it's missing — without it everything still + works, but a GitHub embed can keep showing the old bytes after an in-place overwrite. The + trade-off: these headers turn off edge caching for the whole domain, which is negligible for + screenshot workflows (R2 egress is free) but worth weighing if the same bucket serves + high-traffic assets — in that case, connect a second custom domain to the bucket and scope the + rule to just that host. +

    Then Verify & save: one click checks the settings, signs in to the bucket, round-trips a test object, and fetches it through your public URL. Anything that fails @@ -135,10 +150,11 @@ const TOC = [