From 64638d162a727e55000d911ea9848a1a50489a40 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Sun, 4 Oct 2026 23:17:40 +0100 Subject: [PATCH 1/6] Tighten preview GitHub budget controls Reduce preview GitHub rate-limit burst capacity to 12 per client (while keeping global burst 60), with bucket capacities enforced per key and legacy balances clamped. Add request-level subrequest budgeting (max 8 calls) and normalized client identity hashing (IPv4, IPv6 /64, IPv4-mapped IPv6, invalid/missing fallback) to prevent one cold lookup or caller from exhausting shared budget. Update previews v1 docs and expand tests for new limits, identity sharing, concurrent reserve behavior, and durable-object bucket isolation/reset in route tests. --- .../cloudflare/preview-github-budget.ts | 20 ++- src/services/previews/v1/README.md | 15 +- src/services/previews/v1/github/request.ts | 49 +++++- test/services/previews/v1/budget.test.ts | 141 ++++++++++++++++-- test/services/previews/v1/commit.test.ts | 7 + test/services/previews/v1/main.test.ts | 13 ++ test/services/previews/v1/pr.test.ts | 7 + 7 files changed, 226 insertions(+), 26 deletions(-) diff --git a/src/lib/adapters/cloudflare/preview-github-budget.ts b/src/lib/adapters/cloudflare/preview-github-budget.ts index 8558600..c5f6e1e 100644 --- a/src/lib/adapters/cloudflare/preview-github-budget.ts +++ b/src/lib/adapters/cloudflare/preview-github-budget.ts @@ -1,8 +1,8 @@ import { DurableObject } from "cloudflare:workers"; // One singleton for all preview traffic, independent of URL, client and colo. -// Token buckets permit 60 calls in a burst, then 1000/hour globally and -// 120/hour per client. Every upstream call (not just each lookup) costs one. +// Token buckets permit 60 calls globally and 12 per client in a burst, +// then refill at 1000/hour globally and 120/hour per client. Every upstream call (not just each lookup) costs one. export class PreviewGitHubBudget extends DurableObject { constructor(ctx: DurableObjectState, env: CloudflareBindings) { super(ctx, env); @@ -14,15 +14,16 @@ export class PreviewGitHubBudget extends DurableObject { reserve(client: string): boolean { const now = Date.now(); return this.ctx.storage.transactionSync(() => { - // A client bucket is full after 30 minutes; discard only older state. + // A client bucket is full after six minutes; retaining thirty minutes + // of idle state avoids granting an early reset. this.ctx.storage.sql.exec( "DELETE FROM buckets WHERE key != 'global' AND updated_at < ?", now - 1800000 ); const buckets = [ - { key: "global", rate: 1000 / 3600000 }, - { key: `client:${client}`, rate: 120 / 3600000 } - ].map(({ key, rate }) => { + { key: "global", capacity: 60, rate: 1000 / 3600000 }, + { key: `client:${client}`, capacity: 12, rate: 120 / 3600000 } + ].map(({ key, capacity, rate }) => { const row = this.ctx.storage.sql .exec<{ tokens: number; updated_at: number }>( "SELECT tokens, updated_at FROM buckets WHERE key = ?", @@ -30,8 +31,11 @@ export class PreviewGitHubBudget extends DurableObject { ) .toArray()[0]; const tokens = row - ? Math.min(60, row.tokens + Math.max(0, now - row.updated_at) * rate) - : 60; + ? Math.min( + capacity, + row.tokens + Math.max(0, now - row.updated_at) * rate + ) + : capacity; return { key, tokens }; }); if (buckets.some(({ tokens }) => tokens < 1)) return false; diff --git a/src/services/previews/v1/README.md b/src/services/previews/v1/README.md index 6fc767c..f17995e 100644 --- a/src/services/previews/v1/README.md +++ b/src/services/previews/v1/README.md @@ -98,9 +98,18 @@ Every previews GitHub request reserves a token from the singleton fallback pages, PR head lookups, live download redirects and main enrichment. Metadata cache hits and R2 downloads spend no tokens. All locations and lookup keys share a token bucket with a 60-call burst and a refill of 1000 calls/hour. -Each client IP shares a second bucket with a 60-call burst and 120 calls/hour -refill; only a SHA-256 digest of Cloudflare's connecting IP is stored. Missing -addresses share one allowance. Idle client buckets expire after 30 minutes. +Each client shares a second bucket with a 12-call burst and 120 calls/hour +refill, so one client cannot spend the whole global burst. IPv4 addresses use +one allowance per address; IPv6 addresses share an allowance per /64, with +IPv4-mapped IPv6 addresses using their IPv4 allowance. Only a SHA-256 digest of +this normalized identity from Cloudflare's connecting IP is stored. Missing or +invalid addresses share one allowance. Idle client buckets expire after 30 minutes. + +Each incoming request also has an eight-call ceiling shared across PR head +resolution, artifact fallback, main enrichment and live download resolution. +Exhausting this ceiling returns unavailable rather than a false not-found result. +A match beyond the ceiling requires a cheaper lookup (such as a full commit SHA) +or a later cached result; repeating a cold lookup does not resume its scan. Abbreviated SHAs and fork-PR resolution remain supported within these budgets. Exhaustion or budget-service failure stops before another GitHub request and diff --git a/src/services/previews/v1/github/request.ts b/src/services/previews/v1/github/request.ts index bd5f860..e204335 100644 --- a/src/services/previews/v1/github/request.ts +++ b/src/services/previews/v1/github/request.ts @@ -7,15 +7,62 @@ export interface PreviewGitHub { reserve: () => Promise; } +// Includes PR resolution, artifact fallback and the live download redirect. +// Eight calls preserve ordinary fork-PR lookups and the page-six fallback. +const MAX_REQUEST_SUBREQUESTS = 8; + +function clientBudgetIdentity(address: string | undefined): string { + if (!address) return "unknown"; + if (!address.includes(":")) { + const octets = address.split("."); + if ( + octets.length !== 4 || + octets.some( + (part) => !/^(0|[1-9]\d{0,2})$/.test(part) || Number(part) > 255 + ) + ) + return "unknown"; + return `ipv4:${octets.join(".")}`; + } + // URL's IPv6 parser handles compression and embedded IPv4 consistently. + // Reject URL syntax (including ports and zone IDs) before parsing a literal. + if (!/^[0-9a-f:.]+$/i.test(address)) return "unknown"; + try { + const host = new URL(`http://[${address}]/`).hostname.slice(1, -1); + const [left, right] = host.split("::"); + const start = left ? left.split(":") : []; + const end = right ? right.split(":") : []; + const parts = + right === undefined + ? start + : [...start, ...Array(8 - start.length - end.length).fill("0"), ...end]; + const words = parts.map((part) => parseInt(part, 16)); + // IPv4-mapped addresses share the native IPv4 allowance. + if (words.slice(0, 5).every((word) => word === 0) && words[5] === 0xffff) { + return `ipv4:${words[6] >> 8}.${words[6] & 255}.${words[7] >> 8}.${words[7] & 255}`; + } + return `ipv6:${words + .slice(0, 4) + .map((word) => word.toString(16)) + .join(":")}/64`; + } catch { + return "unknown"; + } +} + export function previewGitHub( c: Context<{ Bindings: CloudflareBindings }> ): PreviewGitHub { // CF supplies this header at the edge; never trust X-Forwarded-For. // Missing addresses share a conservative allowance. - const client = c.req.header("CF-Connecting-IP") ?? "unknown"; + const client = clientBudgetIdentity(c.req.header("CF-Connecting-IP")); + let remaining = MAX_REQUEST_SUBREQUESTS; return { token: c.env.GITHUB_TOKEN, reserve: async () => { + if (remaining === 0) return false; + // Consume before awaiting so concurrent calls cannot exceed the ceiling. + remaining--; const digest = await crypto.subtle.digest( "SHA-256", new TextEncoder().encode(client) diff --git a/test/services/previews/v1/budget.test.ts b/test/services/previews/v1/budget.test.ts index e5afed1..d991386 100644 --- a/test/services/previews/v1/budget.test.ts +++ b/test/services/previews/v1/budget.test.ts @@ -6,7 +6,11 @@ import { waitOnExecutionContext } from "cloudflare:test"; import app from "../../../../src/app"; -import { previewRequest } from "../../../../src/services/previews/v1/github/request"; +import { Hono } from "hono"; +import { + previewGitHub, + previewRequest +} from "../../../../src/services/previews/v1/github/request"; vi.mock("@octokit/request", async () => (await import("../../../mocks/octokit")).octokitRequestMock() @@ -46,10 +50,9 @@ describe("preview GitHub budget", () => { }); it("enforces the client bucket even when global capacity remains", async () => { - await runInDurableObject(budget(), (instance, state) => { - for (let i = 0; i < 60; i++) + await runInDurableObject(budget(), (instance) => { + for (let i = 0; i < 12; i++) expect(instance.reserve("one-client")).toBe(true); - state.storage.sql.exec("DELETE FROM buckets WHERE key = 'global'"); expect(instance.reserve("one-client")).toBe(false); expect(instance.reserve("other-client")).toBe(true); }); @@ -80,7 +83,7 @@ describe("preview GitHub budget", () => { }); }); - it("bounds distinct short SHA misses including every fallback page", async () => { + it("caps each cold scan and leaves capacity for another client", async () => { vi.mocked(request).mockResolvedValue({ data: { artifacts: Array.from({ length: 100 }, () => ({ @@ -89,17 +92,127 @@ describe("preview GitHub budget", () => { })) } } as never); - expect((await get("/previews/v1/commit/aaaaaaa")).status).toBe(404); - expect(request).toHaveBeenCalledTimes(51); - expect((await get("/previews/v1/commit/bbbbbbb", "192.0.2.2")).status).toBe( - 503 - ); - expect(request).toHaveBeenCalledTimes(60); + expect((await get("/previews/v1/commit/aaaaaaa")).status).toBe(503); + expect(request).toHaveBeenCalledTimes(8); + expect(await env.CACHE_KV.get("preview:commit:aaaaaaa")).toBeNull(); + expect((await get("/previews/v1/commit/bbbbbbb")).status).toBe(503); + expect(request).toHaveBeenCalledTimes(12); expect(await env.CACHE_KV.get("preview:commit:bbbbbbb")).toBeNull(); - expect((await get("/previews/v1/commit/ccccccc", "192.0.2.3")).status).toBe( - 503 + vi.mocked(request).mockResolvedValue({ data: { artifacts: [] } } as never); + expect((await get("/previews/v1/commit/ccccccc", "192.0.2.2")).status).toBe( + 404 ); - expect(request).toHaveBeenCalledTimes(60); + expect(request).toHaveBeenCalledTimes(14); + }); + + it("bounds full-SHA run scans without negative caching", async () => { + const sha = "d".repeat(40); + vi.mocked(request).mockImplementation( + async (route) => + ({ + data: + route === "GET /repos/{owner}/{repo}/actions/runs" + ? { + workflow_runs: Array.from({ length: 100 }, (_, id) => ({ + id, + head_sha: sha + })) + } + : { artifacts: [] } + }) as never + ); + expect((await get(`/previews/v1/commit/${sha}`)).status).toBe(503); + expect(request).toHaveBeenCalledTimes(8); + expect(await env.CACHE_KV.get(`preview:commit:${sha}`)).toBeNull(); + }); + + it("shares the ceiling across PR head, artifact scan and redirect", async () => { + const sha = "e".repeat(40); + let listings = 0; + vi.mocked(request).mockImplementation(async (route) => { + if (route === "GET /repos/{owner}/{repo}/pulls/{pull_number}") + return { data: { head: { sha } } } as never; + if (route === "GET /repos/{owner}/{repo}/actions/runs") + return { + data: { + workflow_runs: Array.from({ length: 5 }, (_, id) => ({ + id, + head_sha: sha + })) + } + } as never; + if ( + route === "GET /repos/{owner}/{repo}/actions/runs/{run_id}/artifacts" && + ++listings === 5 + ) + return { + data: { + artifacts: [ + { + id: 123, + name: "FOSSBilling-preview-build.zip", + expired: false, + size_in_bytes: 10 + } + ] + } + } as never; + return { data: { artifacts: [] } } as never; + }); + expect((await get("/previews/v1/pr/9001/download")).status).toBe(503); + expect(request).toHaveBeenCalledTimes(8); + expect(request).not.toHaveBeenCalledWith( + "GET /repos/{owner}/{repo}/actions/artifacts/{artifact_id}/{archive_format}", + expect.anything() + ); + }); + + it("clamps old persisted client balances to the new capacity", async () => { + await runInDurableObject(budget(), (instance, state) => { + state.storage.sql.exec( + "INSERT INTO buckets VALUES ('client:legacy', 60, ?)", + Date.now() + ); + for (let i = 0; i < 12; i++) + expect(instance.reserve("legacy")).toBe(true); + expect(instance.reserve("legacy")).toBe(false); + expect(instance.reserve("new-client")).toBe(true); + }); + }); + + it.each([ + ["2001:db8:1234:5678::1", "2001:0DB8:1234:5678:abcd:0:0:2"], + ["192.0.2.1", "::ffff:192.0.2.1"], + ["::ffff:c000:201", "0:0:0:0:0:ffff:c000:201"], + ["invalid", "also-invalid"] + ])("shares a client allowance for %s and %s", async (first, second) => { + vi.mocked(request).mockRejectedValue( + Object.assign(new Error("Not found"), { status: 404 }) + ); + for (let i = 1; i <= 12; i++) + expect((await get(`/previews/v1/pr/${i}`, first)).status).toBe(404); + expect((await get("/previews/v1/pr/13", second)).status).toBe(503); + expect(request).toHaveBeenCalledTimes(12); + expect( + (await get("/previews/v1/pr/14", "2001:db8:1234:5679::1")).status + ).toBe(404); + }); + + it("caps concurrent subrequests before awaiting reservations", async () => { + const probe = new Hono<{ Bindings: CloudflareBindings }>(); + probe.get("/", async (c) => { + const github = previewGitHub(c); + const allowed = await Promise.all( + Array.from({ length: 20 }, () => github.reserve()) + ); + return c.json(allowed.filter(Boolean).length); + }); + const response = await probe.request( + "/", + { headers: { "CF-Connecting-IP": "192.0.2.1" } }, + env + ); + expect(await response.json()).toBe(8); }); it("bounds distinct PR keys before contacting GitHub", async () => { diff --git a/test/services/previews/v1/commit.test.ts b/test/services/previews/v1/commit.test.ts index 0eafd83..1892c0b 100644 --- a/test/services/previews/v1/commit.test.ts +++ b/test/services/previews/v1/commit.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createExecutionContext, + runInDurableObject, waitOnExecutionContext } from "cloudflare:test"; import { env } from "cloudflare:workers"; @@ -52,6 +53,12 @@ let restoreConsole: (() => void) | null = null; describe("Previews API v1 - GET /previews/v1/commit/:sha", () => { beforeEach(async () => { restoreConsole = suppressConsole(); + await runInDurableObject( + env.PREVIEW_GITHUB_BUDGET.getByName("previews"), + (_instance, state) => { + state.storage.sql.exec("DELETE FROM buckets"); + } + ); await env.CACHE_KV.delete(`preview:commit:${SHA.toLowerCase()}`); vi.clearAllMocks(); }); diff --git a/test/services/previews/v1/main.test.ts b/test/services/previews/v1/main.test.ts index 7e3b4d7..c6af5cc 100644 --- a/test/services/previews/v1/main.test.ts +++ b/test/services/previews/v1/main.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createExecutionContext, + runInDurableObject, waitOnExecutionContext } from "cloudflare:test"; import { env } from "cloudflare:workers"; @@ -44,6 +45,12 @@ let restoreConsole: (() => void) | null = null; describe("Previews API v1 - GET /previews/v1/main", () => { beforeEach(async () => { restoreConsole = suppressConsole(); + await runInDurableObject( + env.PREVIEW_GITHUB_BUDGET.getByName("previews"), + (_instance, state) => { + state.storage.sql.exec("DELETE FROM buckets"); + } + ); await env.CACHE_KV.delete("preview:main"); await env.DOWNLOAD_BUCKET.delete(MAIN_PREVIEW_KEY); vi.clearAllMocks(); @@ -222,6 +229,12 @@ describe("Previews API v1 - GET /previews/v1/main", () => { describe("Previews API v1 - GET /previews/v1/main/download", () => { beforeEach(async () => { restoreConsole = suppressConsole(); + await runInDurableObject( + env.PREVIEW_GITHUB_BUDGET.getByName("previews"), + (_instance, state) => { + state.storage.sql.exec("DELETE FROM buckets"); + } + ); await env.CACHE_KV.delete("preview:main"); await env.DOWNLOAD_BUCKET.delete(MAIN_PREVIEW_KEY); vi.clearAllMocks(); diff --git a/test/services/previews/v1/pr.test.ts b/test/services/previews/v1/pr.test.ts index d7e616f..c077cda 100644 --- a/test/services/previews/v1/pr.test.ts +++ b/test/services/previews/v1/pr.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createExecutionContext, + runInDurableObject, waitOnExecutionContext } from "cloudflare:test"; import { env } from "cloudflare:workers"; @@ -53,6 +54,12 @@ let restoreConsole: (() => void) | null = null; describe("Previews API v1 - GET /previews/v1/pr/:number", () => { beforeEach(async () => { restoreConsole = suppressConsole(); + await runInDurableObject( + env.PREVIEW_GITHUB_BUDGET.getByName("previews"), + (_instance, state) => { + state.storage.sql.exec("DELETE FROM buckets"); + } + ); await env.CACHE_KV.delete(`preview:pr:${PR_NUMBER}`); // The PR route now shares the commit-keyed cache entry for its head // SHA (see resolvePrPreview), so a negative/positive entry left by an From 919c2c071af0ce5fddfb330efdc68d20ae758101 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Mon, 5 Oct 2026 00:22:59 +0100 Subject: [PATCH 2/6] Reject ambiguous preview prefixes and bypass prefix cache --- src/services/previews/v1/README.md | 12 +- src/services/previews/v1/github/artifacts.ts | 77 ++++----- src/services/previews/v1/routes/commit.ts | 43 +++-- src/services/previews/v1/routes/errors.ts | 5 +- src/services/previews/v1/routes/pr.ts | 2 + src/services/previews/v1/schemas/previews.ts | 2 +- test/services/previews/v1/budget.test.ts | 2 +- test/services/previews/v1/commit.test.ts | 160 ++++++++++++++++++- test/services/previews/v1/main.test.ts | 9 +- 9 files changed, 248 insertions(+), 64 deletions(-) diff --git a/src/services/previews/v1/README.md b/src/services/previews/v1/README.md index f17995e..d4ac909 100644 --- a/src/services/previews/v1/README.md +++ b/src/services/previews/v1/README.md @@ -33,8 +33,12 @@ Endpoints are not listed here. The service publishes its own contract: - `GET /main` and `GET /pr/{number}` are **pointers** - they always resolve to whatever is current. -- `GET /commit/{sha}` is a **fixed point** - one commit, one build, +- `GET /commit/{sha}` with a full SHA is a **fixed point** - one commit, permanently addressable (until GitHub's artifact retention expires it). + Abbreviated SHAs (7+ hex characters) are accepted only when a complete + artifact scan finds one distinct live preview commit. Multiple commits + return HTTP 409 (`AMBIGUOUS_COMMIT`); use the full SHA to disambiguate. + An incomplete scan returns 503, never a selected artifact or a 404. - `pr/{number}`'s handler resolves the PR to its head SHA (`GET /pulls/{number}`) and delegates to the same resolver `commit/{sha}` uses - one GitHub-facing code path, not two. @@ -71,7 +75,7 @@ Endpoints are not listed here. The service publishes its own contract: metadata route already cached) and `GET /main` (`preview:main`) use the 60s default, matching how often a moving pointer can realistically change. - `GET /commit/{sha}` (`preview:commit:{sha}`, likewise shared with + Full-SHA `GET /commit/{sha}` (`preview:commit:{sha}`, likewise shared with `/commit/{sha}/download`) uses 3600s instead - a commit's build never changes once it exists, so there's no correctness reason to re-check it every minute. That 3600s is capped at the artifact's own remaining @@ -82,6 +86,10 @@ Endpoints are not listed here. The service publishes its own contract: minimum TTL, so those requests (and any more before the artifact expires or a request refreshes it) are just served live instead of cached - a short burst of extra GitHub calls right at the end, never stale data. +- Abbreviated commit lookups bypass KV reads and writes, including legacy + prefix entries: a newly built colliding commit must be detected on the + next lookup. Concurrent prefix lookups share the in-flight scan, but its + result is never retained. Full-SHA cache behavior is unchanged. - `GET /pr/{number}/download` and `GET /commit/{sha}/download` always resolve GitHub's signed redirect URL live, never cached - it expires in about a minute, and Cloudflare KV's 60s minimum TTL leaves no safe margin diff --git a/src/services/previews/v1/github/artifacts.ts b/src/services/previews/v1/github/artifacts.ts index 0dafbb9..38a10a1 100644 --- a/src/services/previews/v1/github/artifacts.ts +++ b/src/services/previews/v1/github/artifacts.ts @@ -87,38 +87,47 @@ async function listArtifacts( return result.data.artifacts as RawArtifact[]; } -// A circuit breaker, not a correctness bound. This fallback is only used -// for short (prefix) SHAs these days - the runs-API path below handles -// full SHAs - but a short-SHA caller still deserves termination -// guarantees. Every GitHub Actions artifact expires after 14 days -// regardless of type, so a repo's total artifact count is inherently -// finite even for very active repos; this exists only to guarantee -// termination if the API ever doesn't behave as expected (e.g. never -// returns a short page), not because 5,000 artifacts is a realistic -// amount to actually page through. +// Short identifiers require a complete scan: a second matching commit may +// have a different artifact name (fork PRs use the merge SHA), or appear on +// a later page. Never turn an incomplete scan into a match or a cached miss. const MAX_FALLBACK_PAGES = 50; -// The short-SHA fallback can't filter server-side by name, so a repo with -// more than one page of live preview artifacts would silently miss a -// genuine match sitting on page 2+ with a single unpaginated call. Pages -// through until GitHub returns a page short of per_page - the real "no -// more results" signal - or a match is found, whichever happens first. async function findInFallbackPages( github: PreviewGitHub, shaLower: string ): Promise { + let match: ArtifactMatch | null = null; for (let page = 1; page <= MAX_FALLBACK_PAGES; page++) { const artifacts = await listArtifacts(github, undefined, page); - const match = matchArtifact( - artifacts.filter((artifact) => - artifact.name?.startsWith(ARTIFACT_NAME_PREFIX) - ), - shaLower - ); - if (match) return match; - if (artifacts.length < 100) break; // last page + for (const artifact of artifacts) { + if (!artifact.name?.startsWith(ARTIFACT_NAME_PREFIX)) continue; + const candidate = matchArtifact([artifact], shaLower); + if (!candidate) continue; + if ( + match && + candidate.headSha.toLowerCase() !== match.headSha.toLowerCase() + ) { + throw new GitHubError( + "Commit prefix matches multiple preview commits; use a full SHA", + 409, + "AMBIGUOUS_COMMIT" + ); + } + if ( + !match || + (candidate.artifact.created_at ?? "") > + (match.artifact.created_at ?? "") + ) { + match = candidate; + } + } + if (artifacts.length < 100) return match; } - return null; + throw new GitHubError( + "Commit prefix scan exhausted its page budget before establishing uniqueness", + 503, + "preview_scan_incomplete" + ); } // Circuit-breaker budget on run-artifact listings, not a correctness @@ -264,11 +273,8 @@ function toPreviewArtifact(match: ArtifactMatch): PreviewArtifact { // so preview-build-pr names its artifact after a SHA this service never // asks about - only the run's own head_sha metadata (populated by GitHub // independently of what the job saw as $GITHUB_SHA) still says which -// commit it actually is. Which fallback runs depends on the SHA: -// full-length SHAs go through the runs API (findArtifactByRunHeadSha, -// server-side head_sha filter - a handful of calls regardless of repo -// size); short prefixes stay on the page scan (findInFallbackPages), -// since the runs API can't be trusted to match on a partial SHA. +// commit it actually is. Full-length SHAs use the runs API fallback; +// short prefixes always scan all artifact pages to establish uniqueness. export async function findPreviewArtifactByCommitSha( github: PreviewGitHub, sha: string @@ -276,14 +282,13 @@ export async function findPreviewArtifactByCommitSha( const shaLower = sha.toLowerCase(); const url = `https://api.github.com/repos/${REPO_OWNER}/${REPO_NAME}/actions/artifacts`; try { - const exact = await listArtifacts(github, artifactNameForSha(shaLower)); - let match = matchArtifact(exact, shaLower); - - if (!match) { - match = - shaLower.length === 40 - ? await findArtifactByRunHeadSha(github, shaLower) - : await findInFallbackPages(github, shaLower); + let match: ArtifactMatch | null; + if (shaLower.length === 40) { + const exact = await listArtifacts(github, artifactNameForSha(shaLower)); + match = matchArtifact(exact, shaLower); + if (!match) match = await findArtifactByRunHeadSha(github, shaLower); + } else { + match = await findInFallbackPages(github, shaLower); } if (!match) { diff --git a/src/services/previews/v1/routes/commit.ts b/src/services/previews/v1/routes/commit.ts index c5ac98c..f549b07 100644 --- a/src/services/previews/v1/routes/commit.ts +++ b/src/services/previews/v1/routes/commit.ts @@ -7,6 +7,7 @@ import { errorResponse } from "../schemas/previews"; import { resolveArtifactPreview } from "../resolve"; +import { singleFlight } from "../../../../lib/cache"; import { cachedLookup } from "../cache"; import { respondWithDownloadRedirect, respondWithLookup } from "./respond"; import { PreviewsV1App } from "./app"; @@ -70,6 +71,7 @@ export function registerCommitRoutes(app: PreviewsV1App): void { }, description: "The preview build for that commit" }, + 409: errorResponse("Commit prefix matches multiple preview commits"), 404: errorResponse("No preview artifact exists for that commit"), 422: errorResponse("sha param failed validation"), 429: errorResponse("GitHub API rate limit exceeded"), @@ -82,13 +84,20 @@ export function registerCommitRoutes(app: PreviewsV1App): void { const { sha } = c.req.valid("param"); const github = previewGitHub(c); - const result = await cachedLookup( - c.env.CACHE_KV, - cacheKeyForSha(sha), - () => resolveArtifactPreview(github, sha, null), - ttlForArtifact, - (p) => c.executionCtx.waitUntil(p) - ); + // A prefix can become ambiguous as new builds arrive. Do not read old + // prefix cache entries or persist a uniqueness decision for later use. + const result = + sha.length < 40 + ? await singleFlight(`previews:prefix:${sha.toLowerCase()}`, () => + resolveArtifactPreview(github, sha, null) + ) + : await cachedLookup( + c.env.CACHE_KV, + cacheKeyForSha(sha), + () => resolveArtifactPreview(github, sha, null), + ttlForArtifact, + (p) => c.executionCtx.waitUntil(p) + ); return respondWithLookup( c, @@ -105,6 +114,7 @@ export function registerCommitRoutes(app: PreviewsV1App): void { request: { params: CommitShaParamSchema }, responses: { 302: { description: "Redirect to GitHub's live artifact download URL" }, + 409: errorResponse("Commit prefix matches multiple preview commits"), 404: errorResponse("No preview artifact exists for that commit"), 422: errorResponse("sha param failed validation"), 429: errorResponse("GitHub API rate limit exceeded"), @@ -121,13 +131,18 @@ export function registerCommitRoutes(app: PreviewsV1App): void { // download - only the signed URL itself (resolved inside // respondWithDownloadRedirect, on its own short-lived cache) has to be // re-checked often, since that's the part that actually expires. - const artifact = await cachedLookup( - c.env.CACHE_KV, - cacheKeyForSha(sha), - () => resolveArtifactPreview(github, sha, null), - ttlForArtifact, - (p) => c.executionCtx.waitUntil(p) - ); + const artifact = + sha.length < 40 + ? await singleFlight(`previews:prefix:${sha.toLowerCase()}`, () => + resolveArtifactPreview(github, sha, null) + ) + : await cachedLookup( + c.env.CACHE_KV, + cacheKeyForSha(sha), + () => resolveArtifactPreview(github, sha, null), + ttlForArtifact, + (p) => c.executionCtx.waitUntil(p) + ); return respondWithDownloadRedirect( c, github, diff --git a/src/services/previews/v1/routes/errors.ts b/src/services/previews/v1/routes/errors.ts index 9e9c5a0..ea559c4 100644 --- a/src/services/previews/v1/routes/errors.ts +++ b/src/services/previews/v1/routes/errors.ts @@ -2,7 +2,10 @@ import { GitHubError, RateLimitError } from "../../../../lib/github-errors"; // A GitHub outage/rate-limit is a 503 (retry later); anything else // unexpected from classifyGitHubError is a 500. -export function statusFromGithubError(error: GitHubError): 429 | 503 | 500 { +export function statusFromGithubError( + error: GitHubError +): 409 | 429 | 503 | 500 { + if (error.errorCode === "AMBIGUOUS_COMMIT") return 409; if (error instanceof RateLimitError) return 429; if (error.httpStatus !== undefined && error.httpStatus >= 500) return 503; return 500; diff --git a/src/services/previews/v1/routes/pr.ts b/src/services/previews/v1/routes/pr.ts index 218f90f..f203a8c 100644 --- a/src/services/previews/v1/routes/pr.ts +++ b/src/services/previews/v1/routes/pr.ts @@ -59,6 +59,7 @@ export function registerPrRoutes(app: PreviewsV1App): void { }, description: "The current preview build for that pull request" }, + 409: errorResponse("Commit prefix matches multiple preview commits"), 404: errorResponse("No such pull request, or it has no preview build"), 422: errorResponse("number param failed validation"), 429: errorResponse("GitHub API rate limit exceeded"), @@ -93,6 +94,7 @@ export function registerPrRoutes(app: PreviewsV1App): void { request: { params: PrNumberParamSchema }, responses: { 302: { description: "Redirect to GitHub's live artifact download URL" }, + 409: errorResponse("Commit prefix matches multiple preview commits"), 404: errorResponse("No such pull request, or it has no preview build"), 422: errorResponse("number param failed validation"), 429: errorResponse("GitHub API rate limit exceeded"), diff --git a/src/services/previews/v1/schemas/previews.ts b/src/services/previews/v1/schemas/previews.ts index 2a16d33..48859e9 100644 --- a/src/services/previews/v1/schemas/previews.ts +++ b/src/services/previews/v1/schemas/previews.ts @@ -39,7 +39,7 @@ export const PrNumberParamSchema = z.object({ // Full or abbreviated (7+ char) hex commit SHA - GitHub accepts either as a // git ref, and workflow_run.head_sha in the artifacts API is always the full -// 40-char form, so a short SHA here is matched as a prefix by the resolver. +// 40-char form. The resolver rejects prefixes matching multiple commits. export const CommitShaParamSchema = z.object({ sha: z .string() diff --git a/test/services/previews/v1/budget.test.ts b/test/services/previews/v1/budget.test.ts index d991386..48d0cfb 100644 --- a/test/services/previews/v1/budget.test.ts +++ b/test/services/previews/v1/budget.test.ts @@ -102,7 +102,7 @@ describe("preview GitHub budget", () => { expect((await get("/previews/v1/commit/ccccccc", "192.0.2.2")).status).toBe( 404 ); - expect(request).toHaveBeenCalledTimes(14); + expect(request).toHaveBeenCalledTimes(13); }); it("bounds full-SHA run scans without negative caching", async () => { diff --git a/test/services/previews/v1/commit.test.ts b/test/services/previews/v1/commit.test.ts index 1892c0b..36e32fe 100644 --- a/test/services/previews/v1/commit.test.ts +++ b/test/services/previews/v1/commit.test.ts @@ -31,6 +31,7 @@ const SAMPLE_ARTIFACTS = { artifacts: [ { id: 555, + name: `FOSSBilling-preview-${SHA.slice(0, 7)}.zip`, size_in_bytes: 12345, created_at: "2026-08-13T10:00:00Z", expires_at: FAR_FUTURE_EXPIRES_AT, @@ -60,6 +61,7 @@ describe("Previews API v1 - GET /previews/v1/commit/:sha", () => { } ); await env.CACHE_KV.delete(`preview:commit:${SHA.toLowerCase()}`); + await env.CACHE_KV.delete(`preview:commit:${SHA.slice(0, 7)}`); vi.clearAllMocks(); }); @@ -125,6 +127,157 @@ describe("Previews API v1 - GET /previews/v1/commit/:sha", () => { expect(body.result.commit_sha).toBe(SHA); }); + it.each(["", "/download"])( + "rejects colliding prefixes on %s even with a legacy cached selection", + async (suffix) => { + const prefix = SHA.slice(0, 7); + await env.CACHE_KV.put( + `preview:commit:${prefix}`, + JSON.stringify({ + artifact_id: 666, + commit_sha: `${prefix}${"f".repeat(33)}` + }) + ); + const collision = { + ...SAMPLE_ARTIFACTS.artifacts[0], + id: 666, + created_at: "2026-08-14T10:00:00Z", + workflow_run: { id: 1000, head_sha: `${prefix}${"f".repeat(33)}` } + }; + (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation( + async () => ({ + data: { artifacts: [...SAMPLE_ARTIFACTS.artifacts, collision] } + }) + ); + const putSpy = vi.spyOn(env.CACHE_KV, "put"); + const res = await get( + `/previews/v1/commit/${prefix.toUpperCase()}${suffix}` + ); + expect(res.status).toBe(409); + expect(await res.json()).toMatchObject({ + error: { code: "AMBIGUOUS_COMMIT" } + }); + expect(res.headers.get("location")).toBeNull(); + expect(putSpy).not.toHaveBeenCalled(); + expect(ghRequest).toHaveBeenCalledTimes(1); + putSpy.mockRestore(); + } + ); + + it("detects a fork-named collision on a later page", async () => { + const prefix = SHA.slice(0, 7); + const firstPage = Array.from({ length: 100 }, (_, i) => ({ + ...SAMPLE_ARTIFACTS.artifacts[0], + id: 555 + i + })); + (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation( + async (_route: string, params?: { name?: string; page?: number }) => ({ + data: { + artifacts: + params?.page === 2 + ? [ + { + ...SAMPLE_ARTIFACTS.artifacts[0], + name: "FOSSBilling-preview-deadbee.zip", + workflow_run: { + id: 1000, + head_sha: `${prefix}${"f".repeat(33)}` + } + } + ] + : firstPage + } + }) + ); + const res = await get(`/previews/v1/commit/${prefix}/download`); + expect(res.status).toBe(409); + expect(ghRequest).toHaveBeenCalledTimes(2); + expect(await env.CACHE_KV.get(`preview:commit:${prefix}`)).toBeNull(); + }); + + it("does not retain prefix uniqueness after a new colliding build arrives", async () => { + const prefix = SHA.slice(0, 7); + const mock = vi.mocked(ghRequest) as MockGitHubRequest; + mock.mockImplementation(async () => ({ data: SAMPLE_ARTIFACTS })); + expect((await get(`/previews/v1/commit/${prefix}`)).status).toBe(200); + expect(await env.CACHE_KV.get(`preview:commit:${prefix}`)).toBeNull(); + mock.mockImplementation(async () => ({ + data: { + artifacts: [ + ...SAMPLE_ARTIFACTS.artifacts, + { + ...SAMPLE_ARTIFACTS.artifacts[0], + workflow_run: { id: 1000, head_sha: `${prefix}${"f".repeat(33)}` } + } + ] + } + })); + expect((await get(`/previews/v1/commit/${prefix}`)).status).toBe(409); + }); + + it("accepts repeated builds of one commit but ignores expired and non-preview collisions", async () => { + (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation( + async () => ({ + data: { + artifacts: [ + ...SAMPLE_ARTIFACTS.artifacts, + { + ...SAMPLE_ARTIFACTS.artifacts[0], + id: 777, + created_at: "2026-08-14T10:00:00Z", + workflow_run: { id: 1000, head_sha: SHA.toUpperCase() } + }, + { + ...SAMPLE_ARTIFACTS.artifacts[0], + expired: true, + workflow_run: { + id: 1001, + head_sha: `${SHA.slice(0, 7)}${"f".repeat(33)}` + } + }, + { + ...SAMPLE_ARTIFACTS.artifacts[0], + name: "preview-build", + workflow_run: { + id: 1002, + head_sha: `${SHA.slice(0, 7)}${"e".repeat(33)}` + } + } + ] + } + }) + ); + const res = await get(`/previews/v1/commit/${SHA.slice(0, 7)}`); + expect(res.status).toBe(200); + expect(await res.json()).toMatchObject({ result: { artifact_id: 777 } }); + }); + + it("matches a full SHA exactly despite a newer prefix collision", async () => { + (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation( + async () => ({ + data: { + artifacts: [ + ...SAMPLE_ARTIFACTS.artifacts, + { + ...SAMPLE_ARTIFACTS.artifacts[0], + id: 666, + created_at: "2026-08-14T10:00:00Z", + workflow_run: { + id: 1000, + head_sha: `${SHA.slice(0, 7)}${"f".repeat(33)}` + } + } + ] + } + }) + ); + const res = await get(`/previews/v1/commit/${SHA}`); + expect(res.status).toBe(200); + expect(await res.json()).toMatchObject({ + result: { artifact_id: 555, commit_sha: SHA } + }); + }); + it("ignores expired artifacts", async () => { (vi.mocked(ghRequest) as MockGitHubRequest).mockImplementation( async () => ({ @@ -377,7 +530,7 @@ describe("Previews API v1 - GET /previews/v1/commit/:sha", () => { expect.objectContaining({ head_sha: SHA }) ); }); - it("pages through the short-SHA fallback scan past the old 5-page cap, then stops as soon as it finds a match", async () => { + it("pages through the short-SHA fallback scan past the old 5-page cap, then establishes uniqueness on the last page", async () => { // Regression check: an earlier version of the page-scan fallback // stopped after 5 pages (500 artifacts) as a hard cutoff, which would // have reported this commit not_found even though its artifact @@ -437,10 +590,9 @@ describe("Previews API v1 - GET /previews/v1/commit/:sha", () => { expect(res.status).toBe(200); const body = (await res.json()) as { result: { artifact_id: number } }; expect(body.result.artifact_id).toBe(9000); - // Exact-name miss + 6 fallback pages - stops on page 6 rather than - // continuing to page 7. + // Short lookups scan all pages without the name-filtered fast path. expect(fallbackCalls).toBe(6); - expect(ghRequest).toHaveBeenCalledTimes(7); + expect(ghRequest).toHaveBeenCalledTimes(6); }); it("falls back to the default TTL ceiling when expires_at can't be parsed", async () => { diff --git a/test/services/previews/v1/main.test.ts b/test/services/previews/v1/main.test.ts index c6af5cc..0ad538d 100644 --- a/test/services/previews/v1/main.test.ts +++ b/test/services/previews/v1/main.test.ts @@ -23,6 +23,7 @@ const SAMPLE_ARTIFACTS = { artifacts: [ { id: 555, + name: `FOSSBilling-preview-${COMMIT_SHA.slice(0, 7)}.zip`, size_in_bytes: 12345, created_at: "2026-08-13T10:00:00Z", expires_at: "2026-08-27T10:00:00Z", @@ -206,11 +207,9 @@ describe("Previews API v1 - GET /previews/v1/main", () => { result: { commit_sha: string }; }; expect(secondBody.result.commit_sha).toBe("111"); - // 2, not 1: findPreviewArtifactByCommitSha's exact-name query misses - // (no artifact was mocked), so it falls back to a second, broader - // query before giving up - both happen on the first /main request - // only, since the second is served entirely from cache. - expect(ghRequest).toHaveBeenCalledTimes(2); + // Prefix enrichment scans the artifact list once. The second main + // response is served entirely from its R2-backed response cache. + expect(ghRequest).toHaveBeenCalledTimes(1); }); it("falls back to R2 instead of erroring on a corrupt cache entry", async () => { From 69cea11429d89708e920354a5dc964b64efba1c0 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Mon, 5 Oct 2026 00:36:31 +0100 Subject: [PATCH 3/6] Bound mirror-trust cache variants Normalize mirror-trust User-Agent classes before public cache lookup so trusted and untrusted clients share a single cached entry per semantic variant. This keeps /versions responses consistent while preserving the User-Agent Vary contract and conditional request behavior. The change also adds regression tests covering root, latest, and version endpoints across both cache classes. --- src/services/versions/v1/README.md | 2 +- src/services/versions/v1/index.ts | 60 ++++++--- test/services/versions/v1/index.test.ts | 168 +++++++++++++++++------- 3 files changed, 165 insertions(+), 65 deletions(-) diff --git a/src/services/versions/v1/README.md b/src/services/versions/v1/README.md index a9f51fc..ee5b071 100644 --- a/src/services/versions/v1/README.md +++ b/src/services/versions/v1/README.md @@ -4,7 +4,7 @@ Provides release metadata from the FOSSBilling GitHub repo. Responses are cached in `CACHE_KV` for 24 hours. -Successful GET responses are also edge-cached (Cache API, per-PoP) for 5 minutes, so repeat requests don't wake the Worker at all; the window deliberately bounds how long a post-`/update` refresh takes to become visible everywhere. Unused Authorization headers do not bypass this public cache. Non-200s bypass it, and 4xx/5xx responses are stamped `Cache-Control: no-store` so a transient failure can't be pinned client-side. `/`, `/latest`, and `/{version}` bodies vary by mirror trust (derived from `User-Agent`), which they declare with `Vary: User-Agent`; the edge cache folds that header into its key. +Successful GET responses are also edge-cached (Cache API, per-PoP) for 5 minutes, so repeat requests don't wake the Worker at all; the window deliberately bounds how long a post-`/update` refresh takes to become visible everywhere. Unused Authorization headers do not bypass this public cache. Non-200s bypass it, and 4xx/5xx responses are stamped `Cache-Control: no-store` so a transient failure can't be pinned client-side. `/`, `/latest`, and `/{version}` bodies vary by mirror trust (derived from `User-Agent`), which they declare with `Vary: User-Agent`; the internal edge cache canonicalizes that header into two mirror-trust classes before lookup and storage, so different clients in the same class reuse one entry. ## Authentication diff --git a/src/services/versions/v1/index.ts b/src/services/versions/v1/index.ts index a93c4e3..e3f78e6 100644 --- a/src/services/versions/v1/index.ts +++ b/src/services/versions/v1/index.ts @@ -1,5 +1,5 @@ import { bearerAuth } from "hono/bearer-auth"; -import { Hono, type Context, type Handler } from "hono"; +import { Hono, type Context, type Handler, type MiddlewareHandler } from "hono"; import { cors } from "hono/cors"; import { etag } from "hono/etag"; import { prettyJSON } from "hono/pretty-json"; @@ -111,27 +111,53 @@ interface CachedRouteOptions { // Cache-key overrides forwarded to hono's cache middleware; default is // the normalized URL. keyGenerator?: (c: Context) => string; - // Response headers the body varies on. Declaring them lets hono fold the - // request's header value into the cache key instead of refusing to store - // the response at all. - vary?: string[]; + varyByMirrorTrust?: boolean; } function registerCachedRoute

( path: P, handler: Handler, - { keyGenerator = publicCacheKey, vary }: CachedRouteOptions = {} + { + keyGenerator = publicCacheKey, + varyByMirrorTrust = false + }: CachedRouteOptions = {} ) { + const responseCache = publicResponseCache({ + cacheName: VERSIONS_CACHE_NAME, + cacheControl: RELEASES_CACHE_CONTROL, + keyGenerator, + ...(varyByMirrorTrust ? { vary: ["User-Agent"] } : {}) + }); + const boundedCache: MiddlewareHandler = async (c, next) => { + if (!varyByMirrorTrust) return responseCache(c, next); + + // Hono folds raw Vary header values into its key. Give it only the two + // semantic classes, while handlers and downstream caches retain the + // original User-Agent and the public Vary: User-Agent contract. + const original = c.req.raw; + const headers = new Headers(original.headers); + headers.set( + "User-Agent", + clientTrustsMirror(headers.get("User-Agent")) + ? `FOSSBilling/${MIRROR_TRUST_MIN_VERSION}` + : "" + ); + c.req.raw = new Request(original, { headers }); + try { + return await responseCache(c, async () => { + c.req.raw = original; + await next(); + }); + } finally { + c.req.raw = original; + } + }; + return versionsV1.get( path, // Honor conditional requests on cache hits; the inner etag stamps entries. etag(), - publicResponseCache({ - cacheName: VERSIONS_CACHE_NAME, - cacheControl: RELEASES_CACHE_CONTROL, - keyGenerator, - ...(vary ? { vary } : {}) - }), + boundedCache, stampCacheHeaders, etag(), prettyJSON(), @@ -270,8 +296,7 @@ registerCachedRoute( c.header("Vary", "*"); } else { // The body varies by mirror trust (derived from the UA); declaring it - // also lets hono's cache middleware key entries per UA instead of - // refusing to store the response. + // keeps downstream caches from mixing the two response classes. c.header("Vary", "User-Agent"); } @@ -285,7 +310,7 @@ registerCachedRoute( return c.json(buildSuccessResponse(resolvedReleases, result.source)); }, - { vary: ["User-Agent"] } + { varyByMirrorTrust: true } ); versionsV1.get( @@ -455,8 +480,7 @@ registerCachedRoute( } // The body varies by mirror trust (derived from the UA); declaring it - // also lets hono's cache middleware key entries per UA instead of - // refusing to store the response. + // keeps downstream caches from mixing the two response classes. c.header("Vary", "User-Agent"); const userAgent = c.req.header("User-Agent"); @@ -486,7 +510,7 @@ registerCachedRoute( message: `FOSSBilling version ${version} does not appear to exist.` }); }, - { vary: ["User-Agent"] } + { varyByMirrorTrust: true } ); export default versionsV1; diff --git a/test/services/versions/v1/index.test.ts b/test/services/versions/v1/index.test.ts index 09b3ba9..ef6af08 100644 --- a/test/services/versions/v1/index.test.ts +++ b/test/services/versions/v1/index.test.ts @@ -1347,57 +1347,133 @@ describe("Versions API v1", () => { } ); - it("keys mirror-trust variants separately on the same URL", async () => { - setupGitHubApiMock( - vi.mocked(ghRequest) as MockGitHubRequest, - vi.mocked(graphql) as unknown as MockGitHubGraphQL, - [...mockGitHubReleases, mockMirroredRelease], - mockComposerJson - ); - await env.DOWNLOAD_BUCKET.put( - "releases/0.8.0/FOSSBilling-0.8.0.zip", - "mirrored archive contents", - { - customMetadata: { - digest: - "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000", - version: "0.8.0" + it.each([ + ["", false], + ["", true], + ["/latest", false], + ["/latest", true], + ["/0.8.0", false], + ["/0.8.0", true] + ] as const)( + "bounds mirror-trust cache variants for %s (mirror first: %s)", + async (path, mirrorFirst) => { + setupGitHubApiMock( + vi.mocked(ghRequest) as MockGitHubRequest, + vi.mocked(graphql) as unknown as MockGitHubGraphQL, + [...mockGitHubReleases, mockMirroredRelease], + mockComposerJson + ); + await env.DOWNLOAD_BUCKET.put( + "releases/0.8.0/FOSSBilling-0.8.0.zip", + "mirrored archive contents", + { + customMetadata: { + digest: + "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000", + version: "0.8.0" + } } - } - ); - - const requestAs = async (userAgent: string) => { - const ctx = createExecutionContext(); - const response = await app.request( - "/versions/v1/latest", - { headers: { "User-Agent": userAgent } }, - env, - ctx ); - await waitOnExecutionContext(ctx); - const data: ApiResponse = await response.json(); - return data.result?.download_url; - }; - const trusting = "FOSSBilling/0.8.7"; - const distrusting = "curl/8.0.0"; + const requestAs = async ( + userAgent?: string, + ifNoneMatch?: string + ) => { + const ctx = createExecutionContext(); + const headers = new Headers({ + authorization: "Bearer unused-public-credential" + }); + if (userAgent !== undefined) headers.set("User-Agent", userAgent); + if (ifNoneMatch) headers.set("If-None-Match", ifNoneMatch); + const response = await app.request( + `/versions/v1${path}`, + { headers }, + env, + ctx + ); + await waitOnExecutionContext(ctx); + return response; + }; - // Two variants populate two distinct cache entries for one URL. - expect(await requestAs(distrusting)).toBe( - "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.8.0/FOSSBilling.zip" - ); - expect(await requestAs(trusting)).toBe( - "https://download.fossbilling.org/releases/0.8.0/FOSSBilling-0.8.0.zip" - ); + const warm = async (trusted: boolean) => { + const response = await requestAs( + trusted ? "FOSSBilling/0.8.7" : "FOSSBilling/0.8.6" + ); + expect(response.status).toBe(200); + const body = await response.text(); + const data = JSON.parse(body); + const release = path ? data.result : data.result["0.8.0"]; + expect(release.download_url).toBe( + trusted + ? "https://download.fossbilling.org/releases/0.8.0/FOSSBilling-0.8.0.zip" + : "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.8.0/FOSSBilling.zip" + ); + expect(release.digest).toBe( + trusted + ? "sha256:deadbeefcafe0000000000000000000000000000000000000000000000000000" + : mockMirroredRelease.assets[0].digest + ); + expect(release).not.toHaveProperty("mirror_download_url"); + expect(release).not.toHaveProperty("mirror_digest"); + const etag = response.headers.get("ETag"); + expect(etag).toBeTruthy(); + return { body, etag: etag! }; + }; + const first = await warm(mirrorFirst); + const second = await warm(!mirrorFirst); + const mirror = mirrorFirst ? first : second; + const github = mirrorFirst ? second : first; + expect(mirror.etag).not.toBe(github.etag); - // Repeats hit the cached variants without cross-contamination. - expect(await requestAs(distrusting)).toBe( - "https://github.com/FOSSBilling/FOSSBilling/releases/download/0.8.0/FOSSBilling.zip" - ); - expect(await requestAs(trusting)).toBe( - "https://download.fossbilling.org/releases/0.8.0/FOSSBilling-0.8.0.zip" - ); - }); + const get = vi + .spyOn(env.CACHE_KV, "get") + .mockRejectedValue(new Error("backend must not be read")); + try { + for (const [trusted, userAgents] of [ + [ + false, + [ + undefined, + "", + "curl/unique-a", + "attacker/unique-b", + "FOSSBilling/0.8.4", + "FOSSBilling/0.8.7-rc.1", + "FOSSBilling/invalid", + "FOSSBilling/0.8.7 extra" + ] + ], + [ + true, + [ + "FOSSBilling/0.8.8", + "FOSSBilling/1.0.0", + "FOSSBilling/v0.8.7", + "FOSSBilling/0.8.7+unique-a" + ] + ] + ] as const) { + const expected = trusted ? mirror : github; + const other = trusted ? github : mirror; + for (const userAgent of userAgents) { + const response = await requestAs(userAgent, other.etag); + expect(response.status).toBe(200); + expect(response.headers.get("Vary")?.toLowerCase()).toBe( + "user-agent" + ); + expect(response.headers.get("ETag")).toBe(expected.etag); + await expect(response.text()).resolves.toBe(expected.body); + + const conditional = await requestAs(userAgent, expected.etag); + expect(conditional.status).toBe(304); + } + } + expect(get).not.toHaveBeenCalled(); + } finally { + get.mockRestore(); + } + } + ); it("does not edge-cache error responses and stamps no-store", async () => { vi.mocked(ghRequest).mockRejectedValue(new Error("GitHub down")); From aac1c0e7d59e520b800f7ec7a4c9e01d2c0f2bb6 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 6 Oct 2026 19:40:05 +0100 Subject: [PATCH 4/6] Enforce moderator checks at commit time Tighten extensions v2 moderation and ownership writes so approvals, rejections, and delisting now require the actor to still be an active moderator at commit time, not just when the request starts. This adds a shared `moderatorActorError` path, updates SQL guards and route status mapping to return `FORBIDDEN` vs `ACCOUNT_INACTIVE` correctly, and adds regression coverage for moderators being demoted or deactivated mid-request. --- .../extensions/v2/db/developer-claims.ts | 17 +- .../extensions/v2/db/developer-profiles.ts | 23 +-- src/services/extensions/v2/db/errors.ts | 16 ++ src/services/extensions/v2/db/extensions.ts | 11 +- src/services/extensions/v2/db/revisions.ts | 17 +- .../extensions/v2/routes/moderation.ts | 11 +- .../extensions/v2/routes/ownership.ts | 7 +- .../v2/moderation-revocation.test.ts | 168 ++++++++++++++++++ 8 files changed, 227 insertions(+), 43 deletions(-) create mode 100644 test/services/extensions/v2/moderation-revocation.test.ts diff --git a/src/services/extensions/v2/db/developer-claims.ts b/src/services/extensions/v2/db/developer-claims.ts index 234a75f..2f80eb6 100644 --- a/src/services/extensions/v2/db/developer-claims.ts +++ b/src/services/extensions/v2/db/developer-claims.ts @@ -5,6 +5,7 @@ import { encodeCursor as encode, decodeCursor as decode } from "./cursor"; import { developerClaims, developers, users } from "./schema"; import { databaseError, + moderatorActorError, isDeveloperOwnerConflict, isOwnershipEpochRollback } from "./errors"; @@ -430,8 +431,12 @@ export class DeveloperClaimsDatabase { } private async explainClaimApprovalNoOp( - claim: DeveloperClaim + claim: DeveloperClaim, + reviewerId: string ): Promise> { + const actorError = await moderatorActorError(this.db, reviewerId); + if (actorError) return { data: null, error: actorError }; + const latestClaim = await this.getClaimById(claim.id); if (latestClaim.error || latestClaim.data?.status !== "pending") { return { @@ -524,7 +529,7 @@ export class DeveloperClaimsDatabase { ) AND EXISTS ( SELECT 1 FROM users - WHERE users.id = ? AND users.deleted_at IS NULL + WHERE users.id = ? AND users.deleted_at IS NULL AND users.is_moderator = 1 )`, params: [reviewerId, claimId, reviewerId] }); @@ -575,7 +580,7 @@ export class DeveloperClaimsDatabase { } catch (error) { if (isOwnershipEpochRollback(error)) { return this.claimApprovalOutcome( - await this.explainClaimApprovalNoOp(claim), + await this.explainClaimApprovalNoOp(claim, reviewerId), claim ); } @@ -596,7 +601,7 @@ export class DeveloperClaimsDatabase { // Diagnose only after the guarded transaction. These reads improve the // response without participating in (or weakening) its race safety. return this.claimApprovalOutcome( - await this.explainClaimApprovalNoOp(claim), + await this.explainClaimApprovalNoOp(claim, reviewerId), claim ); } @@ -644,6 +649,7 @@ export class DeveloperClaimsDatabase { sql`EXISTS ( SELECT 1 FROM ${users} WHERE ${users.id} = ${reviewerId} AND ${users.deletedAt} IS NULL + AND ${users.isModerator} = 1 )` ) ); @@ -652,6 +658,9 @@ export class DeveloperClaimsDatabase { } if (!result.meta?.changes) { + const actorError = await moderatorActorError(this.db, reviewerId); + if (actorError) return { data: null, error: actorError }; + return { data: null, error: { diff --git a/src/services/extensions/v2/db/developer-profiles.ts b/src/services/extensions/v2/db/developer-profiles.ts index 280d926..81bb0a1 100644 --- a/src/services/extensions/v2/db/developer-profiles.ts +++ b/src/services/extensions/v2/db/developer-profiles.ts @@ -9,7 +9,7 @@ import { extensions, users } from "./schema"; -import { databaseError } from "./errors"; +import { databaseError, moderatorActorError } from "./errors"; import { toD1Statement } from "./batch"; import { optionalBool } from "./columns"; import { @@ -833,6 +833,7 @@ export class DeveloperProfilesDatabase { sql`EXISTS ( SELECT 1 FROM ${users} WHERE ${users.id} = ${reviewerId} AND ${users.deletedAt} IS NULL + AND ${users.isModerator} = 1 )` ) ); @@ -841,24 +842,8 @@ export class DeveloperProfilesDatabase { } if (!result.meta?.changes) { - let reviewer: { deletedAt: string | null } | undefined; - try { - [reviewer] = await this.db - .select({ deletedAt: users.deletedAt }) - .from(users) - .where(eq(users.id, reviewerId)); - } catch (error) { - return databaseError("approve", error); - } - if (!reviewer || reviewer.deletedAt !== null) { - return { - data: null, - error: { - code: "ACCOUNT_INACTIVE", - message: "Active account required" - } - }; - } + const actorError = await moderatorActorError(this.db, reviewerId); + if (actorError) return { data: null, error: actorError }; const existing = await this.getById(id); if (existing.error) { diff --git a/src/services/extensions/v2/db/errors.ts b/src/services/extensions/v2/db/errors.ts index e415576..ee76c70 100644 --- a/src/services/extensions/v2/db/errors.ts +++ b/src/services/extensions/v2/db/errors.ts @@ -86,3 +86,19 @@ export async function inactiveActorError( ? null : { message: "Active account required", code: "ACCOUNT_INACTIVE" }; } + +// Diagnose a failed commit-time moderator guard before workflow conflicts. +// Activity takes precedence so deactivation retains ACCOUNT_INACTIVE. +export async function moderatorActorError( + db: ExtensionsDb, + userId: string +): Promise { + const { data, error } = await new UsersDatabase(db).moderatorAccess(userId); + if (error) return error; + if (!data?.active) { + return { message: "Active account required", code: "ACCOUNT_INACTIVE" }; + } + return data.moderator + ? null + : { message: "Moderator access required", code: "FORBIDDEN" }; +} diff --git a/src/services/extensions/v2/db/extensions.ts b/src/services/extensions/v2/db/extensions.ts index a6d13fa..ecbc5fc 100644 --- a/src/services/extensions/v2/db/extensions.ts +++ b/src/services/extensions/v2/db/extensions.ts @@ -5,7 +5,11 @@ import { ExtensionsDb } from "../../../../lib/db"; import { sortReleasesDescending } from "../../../../lib/releases"; import { parseJSON } from "../../../../lib/json"; import { extensions, extensionRevisions, developers, users } from "./schema"; -import { databaseError, inactiveActorError } from "./errors"; +import { + databaseError, + inactiveActorError, + moderatorActorError +} from "./errors"; import { UsersDatabase } from "./users"; import { toD1Statement } from "./batch"; import { encodeCursor as encode, decodeCursor as decode } from "./cursor"; @@ -706,6 +710,7 @@ export class ExtensionsDatabase { sql`EXISTS ( SELECT 1 FROM ${users} WHERE ${users.id} = ${moderatorId} AND ${users.deletedAt} IS NULL + AND ${users.isModerator} = 1 )` ) ); @@ -726,8 +731,8 @@ export class ExtensionsDatabase { id: string, moderatorId: string ): Promise> { - const inactive = await inactiveActorError(this.db, moderatorId); - if (inactive) return { data: null, error: inactive }; + const actorError = await moderatorActorError(this.db, moderatorId); + if (actorError) return { data: null, error: actorError }; let existing: { publishedAt: string | null; delistedAt: string | null } | undefined; diff --git a/src/services/extensions/v2/db/revisions.ts b/src/services/extensions/v2/db/revisions.ts index 1256056..33a3fd2 100644 --- a/src/services/extensions/v2/db/revisions.ts +++ b/src/services/extensions/v2/db/revisions.ts @@ -2,7 +2,11 @@ import { and, asc, desc, eq, gt, lt, or, sql, SQL } from "drizzle-orm"; import { DatabaseError, DatabaseResult } from "../../../../lib/interfaces"; import { ExtensionsDb } from "../../../../lib/db"; import { extensionRevisions, developers, extensions, users } from "./schema"; -import { databaseError, inactiveActorError } from "./errors"; +import { + databaseError, + inactiveActorError, + moderatorActorError +} from "./errors"; import { toD1Statement } from "./batch"; import { encodeCursor as encode, decodeCursor as decode } from "./cursor"; import { MAX_PENDING_REVISIONS_PER_USER, parseContent } from "./extensions"; @@ -351,8 +355,8 @@ export class ExtensionRevisionsDatabase { id: string, reviewerId: string ): Promise> { - const inactive = await inactiveActorError(this.db, reviewerId); - if (inactive) return { data: null, error: inactive }; + const actorError = await moderatorActorError(this.db, reviewerId); + if (actorError) return { data: null, error: actorError }; const existing = await this.getById(extensionId, id); if (existing.error || !existing.data) { @@ -393,6 +397,7 @@ export class ExtensionRevisionsDatabase { sql`EXISTS ( SELECT 1 FROM ${users} WHERE ${users.id} = ${reviewerId} AND ${users.deletedAt} IS NULL + AND ${users.isModerator} = 1 )` ) ); @@ -475,7 +480,7 @@ export class ExtensionRevisionsDatabase { ) AND EXISTS ( SELECT 1 FROM users u - WHERE u.id = ? AND u.deleted_at IS NULL + WHERE u.id = ? AND u.deleted_at IS NULL AND u.is_moderator = 1 )`, params: [ reviewerId, @@ -521,10 +526,10 @@ export class ExtensionRevisionsDatabase { } if (!results[0]?.meta?.changes) { - const inactive = await inactiveActorError(this.db, reviewerId); + const actorError = await moderatorActorError(this.db, reviewerId); return { data: null, - error: inactive ?? { + error: actorError ?? { message: "Revision is not pending, or ownership changed since it was proposed", code: "CONFLICT" diff --git a/src/services/extensions/v2/routes/moderation.ts b/src/services/extensions/v2/routes/moderation.ts index a84f1be..878cfe8 100644 --- a/src/services/extensions/v2/routes/moderation.ts +++ b/src/services/extensions/v2/routes/moderation.ts @@ -3,11 +3,7 @@ import { getExtensionsDb } from "../../../../lib/db"; import { getPlatform } from "../../../../lib/middleware"; import { getAuth } from "../../../../lib/auth"; import { createRoute, z } from "@hono/zod-openapi"; -import { - errorBody, - statusFromErrorCode, - statusFromWriteErrorCode -} from "./errors"; +import { errorBody, statusFromWriteErrorCode } from "./errors"; import { ActiveAccountRequiredResponse, CursorPaginationQuerySchema, @@ -494,10 +490,7 @@ export function registerModerationRoutes(app: ExtensionsV2App): void { auth.userId ); if (error || !data) { - const status = - error?.code === "ACCOUNT_INACTIVE" - ? 403 - : statusFromErrorCode(error?.code); + const status = statusFromWriteErrorCode(error?.code); return c.json(errorBody(error, "Unable to approve developer"), status); } revalidateCatalogue(c); diff --git a/src/services/extensions/v2/routes/ownership.ts b/src/services/extensions/v2/routes/ownership.ts index 8f703c5..dc6814d 100644 --- a/src/services/extensions/v2/routes/ownership.ts +++ b/src/services/extensions/v2/routes/ownership.ts @@ -325,7 +325,7 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { if (error || !data) { return c.json( errorBody(error, "Unable to approve claim"), - statusFromErrorCode(error?.code) + statusFromWriteErrorCode(error?.code) ); } revalidateCatalogue(c); @@ -399,7 +399,10 @@ export function registerOwnershipRoutes(app: ExtensionsV2App): void { const db = new DeveloperClaimsDatabase(extDb); const { data, error } = await db.rejectClaim(id, auth.userId, review_note); if (error || !data) { - const status = statusFromErrorCode(error?.code, false); + const status = + error?.code === "FORBIDDEN" || error?.code === "ACCOUNT_INACTIVE" + ? 403 + : statusFromErrorCode(error?.code, false); return c.json(errorBody(error, "Unable to reject claim"), status); } revalidateCatalogue(c); diff --git a/test/services/extensions/v2/moderation-revocation.test.ts b/test/services/extensions/v2/moderation-revocation.test.ts new file mode 100644 index 0000000..afba67b --- /dev/null +++ b/test/services/extensions/v2/moderation-revocation.test.ts @@ -0,0 +1,168 @@ +import { describe, it, expect, vi } from "vitest"; +import { env } from "cloudflare:workers"; +import { wrapD1WithHook } from "./db-interceptor"; +import { + setupExtensionsV2Tests, + db, + authHeaders, + post, + sampleContent +} from "./harness"; +import { + insertUser, + insertDeveloper, + insertExtension, + insertRevision, + insertDeveloperClaim, + getDeveloper, + getExtension, + getRevision, + getDeveloperClaim +} from "./db-fixtures"; + +vi.mock("@octokit/request", async () => + (await import("../../../mocks/octokit")).octokitRequestMock() +); +setupExtensionsV2Tests(); + +const actions = [ + "revision approve", + "revision reject", + "developer approve", + "claim approve", + "claim reject", + "delist" +] as const; + +async function snapshot() { + return { + developer: await getDeveloper(db, "review-developer"), + extension: await getExtension(db, "review-extension"), + revision: await getRevision(db, "review-revision"), + claim: await getDeveloperClaim(db, "review-claim"), + competitor: await getDeveloperClaim(db, "competing-claim") + }; +} + +describe("commit-time moderator authority", () => { + for (const action of actions) { + it.each(["demoted", "inactive", "moderator"] as const)( + `${action} checks a %s actor at the write`, + async (actorState) => { + await insertUser(db, { id: "reviewer", is_moderator: 1 }); + const claimAction = action.startsWith("claim"); + await insertDeveloper(db, { + id: "review-developer", + type: "individual", + name: "Developer", + owner_user_id: claimAction ? null : "owner" + }); + await insertExtension(db, { + id: "review-extension", + developer_id: "review-developer" + }); + await insertRevision(db, { + id: "review-revision", + extension_id: "review-extension", + developer_id: "review-developer", + submitted_by: "owner", + content: JSON.stringify(sampleContent({ name: "Reviewed content" })) + }); + await insertDeveloperClaim(db, { + id: "review-claim", + developer_id: "review-developer", + claimant_id: "claimant" + }); + await insertDeveloperClaim(db, { + id: "competing-claim", + developer_id: "review-developer", + claimant_id: "competitor" + }); + const headers = await authHeaders("reviewer"); + const before = await snapshot(); + const table = action.startsWith("revision") + ? "extension_revisions" + : claimAction + ? "developer_claims" + : action === "delist" + ? "extensions" + : "developers"; + let reachedWrite = false; + env.DB_EXTENSIONS = wrapD1WithHook(db, async (sql) => { + if ( + !reachedWrite && + new RegExp(`^\\s*update\\s+"?${table}"?\\s`, "i").test(sql) + ) { + reachedWrite = true; + if (actorState === "demoted") { + await db + .prepare( + "UPDATE users SET is_moderator = 0 WHERE id = 'reviewer'" + ) + .run(); + } else if (actorState === "inactive") { + // Both fields change: inactive takes precedence over demotion. + await db + .prepare( + "UPDATE users SET is_moderator = 0, deleted_at = CURRENT_TIMESTAMP WHERE id = 'reviewer'" + ) + .run(); + } + } + }); + const path = action.startsWith("revision") + ? `/extensions/v2/extensions/review-extension/revisions/review-revision/${action.split(" ")[1]}` + : claimAction + ? `/extensions/v2/developers/claims/review-claim/${action.split(" ")[1]}` + : action === "delist" + ? "/extensions/v2/extensions/REVIEW-extension/delist" + : "/extensions/v2/developers/review-developer/approve"; + const res = await post(path, headers, { + ...(action.endsWith("reject") + ? { review_note: "Review decision" } + : {}), + ...(action === "delist" ? { reason: "Upstream removed" } : {}), + ...(action === "developer approve" ? { expected_revision: 1 } : {}) + }); + expect(reachedWrite).toBe(true); + if (actorState !== "moderator") { + expect(res.status).toBe(403); + expect(await res.json()).toMatchObject({ + error: { + code: actorState === "demoted" ? "FORBIDDEN" : "ACCOUNT_INACTIVE" + } + }); + // Includes published content, ownership/epochs, and competing claims. + expect(await snapshot()).toEqual(before); + } else { + expect(res.status).toBe(200); + const after = await snapshot(); + if (action === "revision approve") { + expect(after.revision?.status).toBe("approved"); + expect(after.extension?.published_revision_id).toBe( + "review-revision" + ); + expect(after.extension?.name).toBe("Reviewed content"); + } else if (action === "revision reject") { + expect(after.revision?.status).toBe("rejected"); + expect(after.extension).toEqual(before.extension); + } else if (action === "developer approve") { + expect(after.developer?.approved_revision).toBe(1); + expect(after.developer?.approved_by).toBe("reviewer"); + } else if (action === "claim approve") { + expect(after.claim?.status).toBe("approved"); + expect(after.developer?.owner_user_id).toBe("claimant"); + expect(after.competitor?.status).toBe("rejected"); + } else if (action === "claim reject") { + expect(after.claim?.status).toBe("rejected"); + expect(after.developer).toEqual(before.developer); + expect(after.competitor).toEqual(before.competitor); + } else { + expect(after.extension?.delisted_at).not.toBeNull(); + expect(after.extension?.delist_reason).toBe("Upstream removed"); + } + } + } + ); + } +}); From 28d31144bd0525193f4a84f07a3dd7de630d91d0 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 6 Oct 2026 19:47:07 +0100 Subject: [PATCH 5/6] Fix extension id casing in revocation test --- test/services/extensions/v2/moderation-revocation.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/services/extensions/v2/moderation-revocation.test.ts b/test/services/extensions/v2/moderation-revocation.test.ts index afba67b..a7c1e88 100644 --- a/test/services/extensions/v2/moderation-revocation.test.ts +++ b/test/services/extensions/v2/moderation-revocation.test.ts @@ -115,7 +115,7 @@ describe("commit-time moderator authority", () => { : claimAction ? `/extensions/v2/developers/claims/review-claim/${action.split(" ")[1]}` : action === "delist" - ? "/extensions/v2/extensions/REVIEW-extension/delist" + ? "/extensions/v2/extensions/review-extension/delist" : "/extensions/v2/developers/review-developer/approve"; const res = await post(path, headers, { ...(action.endsWith("reject") From 678f71acef84888d75f4d010efa0410d47cf35c5 Mon Sep 17 00:00:00 2001 From: Adam Daley Date: Tue, 6 Oct 2026 20:13:35 +0100 Subject: [PATCH 6/6] Address review findings on preview error contract --- src/services/previews/v1/README.md | 3 +- src/services/previews/v1/routes/errors.ts | 10 +++- src/services/previews/v1/routes/pr.ts | 7 +-- src/services/previews/v1/routes/respond.ts | 70 +++++++++++++++++++--- 4 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/services/previews/v1/README.md b/src/services/previews/v1/README.md index d4ac909..8b37e47 100644 --- a/src/services/previews/v1/README.md +++ b/src/services/previews/v1/README.md @@ -38,7 +38,8 @@ Endpoints are not listed here. The service publishes its own contract: Abbreviated SHAs (7+ hex characters) are accepted only when a complete artifact scan finds one distinct live preview commit. Multiple commits return HTTP 409 (`AMBIGUOUS_COMMIT`); use the full SHA to disambiguate. - An incomplete scan returns 503, never a selected artifact or a 404. + Budget or page-cap exhaustion returns 503; GitHub rate limits return 429. + An incomplete scan never returns a selected artifact or a 404. - `pr/{number}`'s handler resolves the PR to its head SHA (`GET /pulls/{number}`) and delegates to the same resolver `commit/{sha}` uses - one GitHub-facing code path, not two. diff --git a/src/services/previews/v1/routes/errors.ts b/src/services/previews/v1/routes/errors.ts index ea559c4..dc17511 100644 --- a/src/services/previews/v1/routes/errors.ts +++ b/src/services/previews/v1/routes/errors.ts @@ -4,8 +4,16 @@ import { GitHubError, RateLimitError } from "../../../../lib/github-errors"; // unexpected from classifyGitHubError is a 500. export function statusFromGithubError( error: GitHubError +): 409 | 429 | 503 | 500; +export function statusFromGithubError( + error: GitHubError, + includeAmbiguous: false +): 429 | 503 | 500; +export function statusFromGithubError( + error: GitHubError, + includeAmbiguous = true ): 409 | 429 | 503 | 500 { - if (error.errorCode === "AMBIGUOUS_COMMIT") return 409; + if (includeAmbiguous && error.errorCode === "AMBIGUOUS_COMMIT") return 409; if (error instanceof RateLimitError) return 429; if (error.httpStatus !== undefined && error.httpStatus >= 500) return 503; return 500; diff --git a/src/services/previews/v1/routes/pr.ts b/src/services/previews/v1/routes/pr.ts index f203a8c..f105d7d 100644 --- a/src/services/previews/v1/routes/pr.ts +++ b/src/services/previews/v1/routes/pr.ts @@ -59,7 +59,6 @@ export function registerPrRoutes(app: PreviewsV1App): void { }, description: "The current preview build for that pull request" }, - 409: errorResponse("Commit prefix matches multiple preview commits"), 404: errorResponse("No such pull request, or it has no preview build"), 422: errorResponse("number param failed validation"), 429: errorResponse("GitHub API rate limit exceeded"), @@ -83,7 +82,7 @@ export function registerPrRoutes(app: PreviewsV1App): void { (p) => c.executionCtx.waitUntil(p) ); - return respondWithLookup(c, result, notFoundMessage(number)); + return respondWithLookup(c, result, notFoundMessage(number), false); }); const prDownloadRoute = createRoute({ @@ -94,7 +93,6 @@ export function registerPrRoutes(app: PreviewsV1App): void { request: { params: PrNumberParamSchema }, responses: { 302: { description: "Redirect to GitHub's live artifact download URL" }, - 409: errorResponse("Commit prefix matches multiple preview commits"), 404: errorResponse("No such pull request, or it has no preview build"), 422: errorResponse("number param failed validation"), 429: errorResponse("GitHub API rate limit exceeded"), @@ -125,7 +123,8 @@ export function registerPrRoutes(app: PreviewsV1App): void { c, github, artifact, - notFoundMessage(number) + notFoundMessage(number), + false ); }); } diff --git a/src/services/previews/v1/routes/respond.ts b/src/services/previews/v1/routes/respond.ts index 49426a1..73eb321 100644 --- a/src/services/previews/v1/routes/respond.ts +++ b/src/services/previews/v1/routes/respond.ts @@ -1,15 +1,49 @@ import { PreviewGitHub } from "../github/request"; -import { Context } from "hono"; +import { Context, TypedResponse } from "hono"; import { getArtifactDownloadUrl } from "../github/artifacts"; import { PreviewLookupResult } from "../resolve"; import { githubErrorBody, notFoundBody, statusFromGithubError } from "./errors"; +type LookupData = Extract["data"]; +type LookupOk = Response & TypedResponse<{ result: LookupData }, 200, "json">; +type LookupMissing = Response & + TypedResponse<{ error: { message: string; code: string } }, 404, "json">; +type LookupError = Response & + TypedResponse< + { error: { message: string; code: string } }, + 409 | 429 | 500 | 503, + "json" + >; +type LookupErrorWithoutAmbiguity = Response & + TypedResponse< + { error: { message: string; code: string } }, + 429 | 500 | 503, + "json" + >; +type LookupRedirect = Response & TypedResponse; + // Shared by /commit/{sha} and /pr/{number}: both resolve to a -// PreviewLookupResult and only differ in their not-found message. +// PreviewLookupResult and only differ in their not-found message. PR routes +// resolve from GitHub's full head SHA, which never takes the prefix-scan +// path that produces AMBIGUOUS_COMMIT, so they pass includeAmbiguous=false +// to keep the unreachable 409 out of their OpenAPI contract. +export function respondWithLookup( + c: Context, + result: PreviewLookupResult, + notFoundMessage: string, + includeAmbiguous?: true +): LookupOk | LookupMissing | LookupError; export function respondWithLookup( c: Context, result: PreviewLookupResult, - notFoundMessage: string + notFoundMessage: string, + includeAmbiguous: false +): LookupOk | LookupMissing | LookupErrorWithoutAmbiguity; +export function respondWithLookup( + c: Context, + result: PreviewLookupResult, + notFoundMessage: string, + includeAmbiguous = true ) { if (result.status === "found") { return c.json({ result: result.data }, 200); @@ -17,9 +51,12 @@ export function respondWithLookup( if (result.status === "not_found") { return c.json(notFoundBody(notFoundMessage), 404); } + const status = includeAmbiguous + ? statusFromGithubError(result.error) + : statusFromGithubError(result.error, false); return c.json( githubErrorBody(result.error, "Failed to look up the preview artifact"), - statusFromGithubError(result.error) + status ); } @@ -33,15 +70,34 @@ export async function respondWithDownloadRedirect( c: Context, github: PreviewGitHub, artifact: PreviewLookupResult, - notFoundMessage: string + notFoundMessage: string, + includeAmbiguous?: true +): Promise; +export async function respondWithDownloadRedirect( + c: Context, + github: PreviewGitHub, + artifact: PreviewLookupResult, + notFoundMessage: string, + includeAmbiguous: false +): Promise; +export async function respondWithDownloadRedirect( + c: Context, + github: PreviewGitHub, + artifact: PreviewLookupResult, + notFoundMessage: string, + includeAmbiguous = true ) { + const statusFor = (error: Parameters[0]) => + includeAmbiguous + ? statusFromGithubError(error) + : statusFromGithubError(error, false); if (artifact.status === "not_found") { return c.json(notFoundBody(notFoundMessage), 404); } if (artifact.status === "unavailable") { return c.json( githubErrorBody(artifact.error, "Failed to look up the preview artifact"), - statusFromGithubError(artifact.error) + statusFor(artifact.error) ); } @@ -58,7 +114,7 @@ export async function respondWithDownloadRedirect( redirect.error, "Failed to resolve the artifact download URL" ), - statusFromGithubError(redirect.error) + statusFor(redirect.error) ); }