diff --git a/README.md b/README.md index af3071f9..108fac0d 100644 --- a/README.md +++ b/README.md @@ -84,16 +84,16 @@ original deployment it grew out of). The project home is | Scope | Why | |-------|-----| - | Account · Cloudflare Pages · Edit | create/deploy the Pages project | - | Account · D1 · Edit | create + migrate the database | - | Account · Workers R2 Storage · Edit | create the image bucket | - | Account · Turnstile · Edit | **only if** attaching a custom domain — provisions the admin-login bot check | - | Zone · Zone · Read | **only if** attaching a custom domain — lets connect-domains resolve the zone | - | Zone · DNS · Edit | **only if** attaching a custom domain (writes the apex record) | - | Zone · WAF · Edit | **only if** attaching a custom domain — adds a WAF rate limit on the public API endpoints (the download beacon and the oEmbed provider) | - | Zone · Zone Settings · Edit | *optional* — lets setup enable image resizing for you | - - Without **DNS · Edit**, registering the Pages apex domain succeeds but the DNS + | Account → Cloudflare Pages: Edit | create/deploy the Pages project | + | Account → D1: Edit | create + migrate the database | + | Account → Workers R2 Storage: Edit | create the image bucket | + | Account → Turnstile: Edit | **only if** attaching a custom domain — provisions the admin-login bot check | + | Zone → Zone: Read | **only if** attaching a custom domain — resolves the zone for setup's DNS preflight and for connect-domains | + | Zone → DNS: Edit | **only if** attaching a custom domain (writes the apex record) | + | Zone → WAF: Edit | **only if** attaching a custom domain — adds a WAF rate limit on the public API endpoints (the download beacon and the oEmbed provider) | + | Zone → Zone Settings: Edit | *optional* — lets setup enable image resizing for you | + + Without **Zone → DNS: Edit**, registering the Pages apex domain succeeds but the DNS write fails, leaving the domain stuck `pending` with a confusing 522. 4. **Finish in the first-run wizard.** Open `/admin/setup`, enter your `SETUP_TOKEN`, and set your admin password + site name, owner/persona name, @@ -147,7 +147,7 @@ npm run connect-domains -- --check yourdomain.com # read-only doctor: which s `connect-domains` attaches the CDN host (`cdn.yourdomain.com`) to the images bucket and the site domain to the Pages project. **Images 404 until the CDN -host is attached.** With *Zone · Zone Settings · Edit* on the token it also +host is attached.** With *Zone → Zone Settings: Edit* on the token it also enables Image Transformations. It adds no other DNS records, and it's safe to re-run. Its token scopes are all in the **API token scopes** table in step 3. If your site lives on a subdomain (like @@ -170,7 +170,7 @@ them where they can, but finishing either may need dashboard or DNS access: origin"** so it can pull from the R2 CDN host. Free tier: **5,000 transformations/month**. Until it's on, gallery thumbnails serve the full-size original (slow) or fail. The deploy token can't enable this unless it carries - *Zone · Zone Settings · Edit*. + *Zone → Zone Settings: Edit*. ### Local development diff --git a/UPDATING.md b/UPDATING.md index 2096e1e4..482cd00f 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -94,8 +94,9 @@ Run this once per fork, from a clone: CLOUDFLARE_API_TOKEN= npm run apply-download-ratelimit -- ``` -`` is your site domain (e.g. `akito.dog`). The token needs one permission, -**Zone · WAF · Edit**, on a token whose Zone Resources include that domain; it is +`` is your site domain (e.g. `akito.dog`). The token needs two permissions, +**Zone → Zone: Read** to resolve the zone and **Zone → WAF: Edit** to write the +rule, on a token whose Zone Resources include that domain; it is read from the environment and never printed. The command is idempotent — the first run reports `updated`, any re-run reports `exists` — so it is safe to repeat if you're unsure whether it already ran. diff --git a/scripts/apply-download-ratelimit.test.ts b/scripts/apply-download-ratelimit.test.ts new file mode 100644 index 00000000..a696e3eb --- /dev/null +++ b/scripts/apply-download-ratelimit.test.ts @@ -0,0 +1,47 @@ +import { describe, it, expect } from 'vitest'; +import type { CfApiResult } from './setup-lib.ts'; +import { applyDownloadRateLimit } from './waf-lib.ts'; +import { failureLines, TOKEN_RECIPE } from './apply-download-ratelimit.ts'; + +const SECRET = 'cf-secret-token-value-should-never-leak'; +const ZONE = 'zone123'; +const zonePath = 'GET /zones?name=akito.dog'; +const entryPath = `GET /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`; +const zoneOk: CfApiResult = { ok: true, status: 200, result: [{ id: ZONE }] }; + +/** A cfApi stub answering from a path+method map; anything else is a 500. */ +const fakeApi = + (routes: Record) => + async (_token: string, path: string, init: { method?: string } = {}): Promise => + routes[`${init.method ?? 'GET'} ${path}`] ?? routes[path] ?? { ok: false, status: 500 }; + +// The recipe is a fix for token scopes and nothing else. Printing it for a 500 +// sends the operator to re-mint a token that was never the problem; withholding +// it on a 403 leaves them with no fix at all. Both directions are pinned against +// REAL results from the lib, not hand-built ones. +describe('failureLines — the token-recipe gate', () => { + it('prints the token recipe when the API refused the call', async () => { + const res = await applyDownloadRateLimit( + SECRET, + 'akito.dog', + fakeApi({ [zonePath]: zoneOk, [entryPath]: { ok: false, status: 403 } }) + ); + const text = failureLines(res).join('\n'); + expect(text).toContain(res.detail); + expect(text).toContain(TOKEN_RECIPE); + expect(text).not.toContain('may not be a token-permission problem'); + }); + + it('gives plain retry guidance, and no recipe, for a transient failure', async () => { + const res = await applyDownloadRateLimit( + SECRET, + 'akito.dog', + fakeApi({ [zonePath]: zoneOk, [entryPath]: { ok: false, status: 500 } }) + ); + const text = failureLines(res).join('\n'); + expect(text).toContain(res.detail); + expect(text).toContain('This may not be a token-permission problem.'); + expect(text).not.toContain(TOKEN_RECIPE); + expect(text).toContain('npm run apply-download-ratelimit -- '); + }); +}); diff --git a/scripts/apply-download-ratelimit.ts b/scripts/apply-download-ratelimit.ts index 6f7239d9..96282e1d 100644 --- a/scripts/apply-download-ratelimit.ts +++ b/scripts/apply-download-ratelimit.ts @@ -15,19 +15,42 @@ * is a no-op. Exit 0 on created/updated/exists, 1 on error. * * Token scope required: Zone → WAF: Edit, on a token whose Zone Resources - * include the fork's domain. (Read-only Zone·Read is enough to resolve the + * include the fork's domain. (Read-only Zone → Zone: Read is enough to resolve the * zone, but writing the rule needs WAF: Edit.) */ import { env, argv, exit } from 'node:process'; -import { applyDownloadRateLimit } from './waf-lib.ts'; +import { fileURLToPath } from 'node:url'; +import { applyDownloadRateLimit, isPermissionError, type RateLimitResult } from './waf-lib.ts'; -const TOKEN_RECIPE = +export const TOKEN_RECIPE = 'Set CLOUDFLARE_API_TOKEN to a Cloudflare API token (dash → My Profile → API Tokens →\n' + 'Create Token → Custom token) with:\n' + - ' • Zone · WAF · Edit\n' + + ' • Zone → Zone: Read\n' + + ' • Zone → WAF: Edit\n' + ' and a Zone Resource that includes the fork domain, then re-run:\n' + ' CLOUDFLARE_API_TOKEN= npm run apply-download-ratelimit -- '; +/** + * What the runner prints when the rule wasn't applied. The recipe fixes token + * scopes, so it goes out only when the call actually hit a refusal — the result + * records that; the wording is never consulted. Any other failure gets plain + * retry guidance, since re-minting a token would not have helped. + * + * Pure and exported so the gate itself is testable: main() drives live + * Cloudflare state, and inverting this branch used to pass the whole suite. + */ +export function failureLines(res: RateLimitResult): string[] { + const lines = [`✖ ${res.detail}\n`]; + if (isPermissionError(res)) { + lines.push(TOKEN_RECIPE); + } else { + lines.push('This may not be a token-permission problem. Re-run once the'); + lines.push('reason above is resolved:'); + lines.push(' CLOUDFLARE_API_TOKEN= npm run apply-download-ratelimit -- '); + } + return lines; +} + async function main(): Promise { console.log('— Sona apply-download-ratelimit —\n'); @@ -57,16 +80,18 @@ async function main(): Promise { console.log(`✔ ${res.detail}`); return 0; default: - console.error(`✖ ${res.detail}\n`); - console.error(TOKEN_RECIPE); + for (const line of failureLines(res)) console.error(line); return 1; } } -main() - .then((code) => exit(code)) - .catch((err) => { - // Never surface the token; print only the error class/message. - console.error('✖ Unexpected error:', err instanceof Error ? err.message : String(err)); - exit(1); - }); +// Only run when invoked directly, so the unit tests can import the helpers. +if (argv[1] && fileURLToPath(import.meta.url) === argv[1]) { + main() + .then((code) => exit(code)) + .catch((err) => { + // Never surface the token; print only the error class/message. + console.error('✖ Unexpected error:', err instanceof Error ? err.message : String(err)); + exit(1); + }); +} diff --git a/scripts/connect-domains-lib.test.ts b/scripts/connect-domains-lib.test.ts index 61906480..65b0c370 100644 --- a/scripts/connect-domains-lib.test.ts +++ b/scripts/connect-domains-lib.test.ts @@ -10,6 +10,7 @@ import { cdnDomainState, bucketDomainTlsIssued, pagesDomainAttached, + pagesDomainState, classifyCdnProbe, planConnect, siteUrlMismatch, @@ -17,7 +18,8 @@ import { firstFailingRung, renderLadder, type CdnDomainState, - type CdnProbe + type CdnProbe, + type LadderInputs } from './connect-domains-lib.ts'; describe('cdnHost', () => { @@ -202,9 +204,36 @@ describe('resolveZone', () => { }); }); +describe('pagesDomainAttached / findBucketDomain — malformed entries', () => { + // A junk entry must simply fail to match, never throw: the callers turn a + // non-match into 'absent' or 'unknown', and a TypeError would escape both. + it('does not throw on non-object entries', () => { + expect(() => pagesDomainAttached([null, 'x', 42, { name: 'taro.surf' }], 'taro.surf')).not.toThrow(); + expect(pagesDomainAttached([null, 'x', 42, { name: 'taro.surf' }], 'taro.surf')).toBe(true); + expect(pagesDomainAttached([null, 'x', 42], 'taro.surf')).toBe(false); + expect(() => findBucketDomain({ domains: [null, 'x', 42] }, 'cdn.taro.surf')).not.toThrow(); + expect(findBucketDomain({ domains: [null, 'x', 42] }, 'cdn.taro.surf')).toBeUndefined(); + }); +}); + describe('cdnDomainState', () => { const ok = (domains: unknown[]) => ({ ok: true, status: 200, result: { domains } }); + // The twin of pagesDomainState's rule: findBucketDomain coerces either accepted + // shape to [], so an ok response carrying neither would read as 'absent' and + // green-light the attach this read exists to gate. + it("reports a malformed body as unknown, never as 'absent'", () => { + for (const result of [undefined, null, {}, { domains: 'nope' }, 'nope', 42]) { + expect( + cdnDomainState({ ok: true, status: 200, result }, 'cdn.taro.surf'), + `result=${JSON.stringify(result) ?? 'undefined'}` + ).toBe('unknown'); + } + // Both real shapes still read as a list, not as unreadable. + expect(cdnDomainState({ ok: true, status: 200, result: [] }, 'cdn.taro.surf')).toBe('absent'); + expect(cdnDomainState(ok([]), 'cdn.taro.surf')).toBe('absent'); + }); + it('is attached for an enabled custom domain', () => { expect(cdnDomainState(ok([{ domain: 'cdn.taro.surf', enabled: true }]), 'cdn.taro.surf')).toBe( 'attached' @@ -289,7 +318,7 @@ describe('planConnect', () => { }; it('plans BOTH mutations with the correct paths and bodies when nothing is present', () => { - const plan = planConnect({ ...base, cdnPresent: false, pagesAttached: false }); + const plan = planConnect({ ...base, cdnPresent: false, pagesPresent: false }); expect(plan).toHaveLength(2); expect(plan[0]).toMatchObject({ method: 'POST', @@ -304,13 +333,21 @@ describe('planConnect', () => { }); it('skips the CDN create when the domain is already present (idempotent)', () => { - const plan = planConnect({ ...base, cdnPresent: true, pagesAttached: false }); + const plan = planConnect({ ...base, cdnPresent: true, pagesPresent: false }); expect(plan).toHaveLength(1); expect(plan[0].path).toContain('/pages/projects/'); }); it('plans nothing when both records are already present', () => { - expect(planConnect({ ...base, cdnPresent: true, pagesAttached: true })).toEqual([]); + expect(planConnect({ ...base, cdnPresent: true, pagesPresent: true })).toEqual([]); + }); + + // Each mutation carries the permission IT needs, so a 401/403 on the attach can + // name that one scope instead of the whole token recipe or nothing at all. + it('gives each mutation its own token scope', () => { + const plan = planConnect({ ...base, cdnPresent: false, pagesPresent: false }); + expect(plan[0].scopeHint).toBe('Account → Workers R2 Storage: Edit'); + expect(plan[1].scopeHint).toBe('Account → Cloudflare Pages: Edit'); }); it('does NOT plan a create for a present-but-disabled CDN domain', () => { @@ -319,7 +356,7 @@ describe('planConnect', () => { 'cdn.taro.surf' ); expect(state).toBe('disabled'); - const plan = planConnect({ ...base, cdnPresent: state !== 'absent', pagesAttached: true }); + const plan = planConnect({ ...base, cdnPresent: state !== 'absent', pagesPresent: true }); expect(plan).toEqual([]); // disabled ⇒ present ⇒ no re-create POST }); }); @@ -463,8 +500,119 @@ describe('buildLadder + firstFailingRung', () => { expect(off.find((r) => r.id === 'cdn-loads')?.status).toBe('pass'); expect(firstFailingRung(off)).toBeUndefined(); - const unknown = buildLadder({ ...healthy, imageTransforms: null }); - expect(unknown.find((r) => r.id === 'image-transforms')?.status).toBe('warn'); + const unknown = buildLadder({ + ...healthy, + imageTransforms: null, + imageTransformsStatus: 403 + }); + const unknownRung = unknown.find((r) => r.id === 'image-transforms'); + expect(unknownRung?.status).toBe('warn'); + // The remedy names the missing scope in the arrow form the other + // operator-facing strings use (the notation drift guard's positive arm). + expect(unknownRung?.action).toContain('Zone → Zone Settings: Read'); + }); + + // The scope is the reason for exactly one class of failure. Naming it for a + // 5xx or an unreachable API sends the operator to re-mint a token that was + // never the problem, so the can't-verify rungs read their cause off the + // response they actually got. + const transformsAction = (over: Partial) => + buildLadder({ ...healthy, imageTransforms: null, ...over }).find( + (r) => r.id === 'image-transforms' + )!.action!; + + it('blames the Zone Settings scope for a 403 only, never for a 5xx or an unreachable API', () => { + expect(transformsAction({ imageTransformsStatus: 401 })).toContain('token needs Zone → Zone Settings: Read'); + + const server = transformsAction({ + imageTransformsStatus: 500, + imageTransformsErrors: [{ code: 10000, message: 'Internal error' }] + }); + expect(server).not.toContain('Zone → Zone Settings: Read'); + expect(server).toContain('(HTTP 500)'); + expect(server).toContain('the API said 10000: Internal error'); + + const offline = transformsAction({ imageTransformsStatus: 0 }); + expect(offline).not.toContain('Zone → Zone Settings: Read'); + expect(offline).not.toContain('HTTP 0'); + expect(offline).toContain('the Cloudflare API did not respond'); + }); + + it('says nothing about a cause when no read status was recorded', () => { + const action = transformsAction({}); + expect(action).not.toContain('Zone → Zone Settings: Read'); + expect(action).toContain("Couldn't verify Image Transformations;"); + }); + + const cdnAction = (over: Partial) => + buildLadder({ ...healthy, cdnState: 'unknown', tlsIssued: null, ...over }).find( + (r) => r.id === 'cdn-attached' + )!.action!; + + it('reports the R2 read failure by status too, not always as a missing scope', () => { + expect(cdnAction({ cdnRead: { ok: false, status: 403 } })).toContain( + 'token needs Account → Workers R2 Storage: Read' + ); + expect(cdnAction({ cdnRead: { ok: false, status: 503 } })).not.toContain( + 'Account → Workers R2 Storage: Read' + ); + expect(cdnAction({ cdnRead: { ok: false, status: 0 } })).toContain( + 'the Cloudflare API did not respond' + ); + }); + + // cfApi sets ok only when the API reported success, so an ok response we + // couldn't read is a partial body. Feeding its 200 to the failure tail said + // "the API reported failure with no reason given" — the opposite of what + // happened, about the one case the operator can least explain. + it('says an ok-but-unreadable read carried no domain list, never that the API failed', () => { + const action = cdnAction({ cdnRead: { ok: true, status: 200 } }); + expect(action).toContain('(HTTP 200); the response carried no domain list'); + expect(action).not.toContain('the API reported failure'); + expect(action).not.toContain('Workers R2 Storage: Read'); + }); +}); + +describe('pagesDomainState', () => { + const attached = [{ name: 'taro.surf' }]; + + it('distinguishes attached from absent on a successful read', () => { + expect(pagesDomainState({ ok: true, status: 200, result: attached }, 'taro.surf')).toBe('attached'); + expect(pagesDomainState({ ok: true, status: 200, result: [] }, 'taro.surf')).toBe('absent'); + }); + + // A 200 carrying something that isn't a list tells us nothing about the + // domains, so it has to read the same as a failed request. Coercing it to an + // empty list would say 'absent' and green-light the attach it should gate. + it("reports a malformed body as unknown, never as 'absent'", () => { + for (const result of [undefined, null, {}, 'nope', 42]) { + expect( + pagesDomainState({ ok: true, status: 200, result }, 'taro.surf'), + `result=${JSON.stringify(result) ?? 'undefined'}` + ).toBe('unknown'); + } + }); + + it("reports a failed read as unknown, never as 'absent' (auth-vs-absent)", () => { + expect(pagesDomainState({ ok: false, status: 403 }, 'taro.surf')).toBe('unknown'); + expect(pagesDomainState({ ok: false, status: 500 }, 'taro.surf')).toBe('unknown'); + expect(pagesDomainState({ ok: false, status: 0 }, 'taro.surf')).toBe('unknown'); + }); + + // The whole point of the unknown state: the read that failed is the one that + // would have told us whether the attach is needed, so we must not attach. + it('plans no Pages attach when the read failed', () => { + const state = pagesDomainState({ ok: false, status: 500 }, 'taro.surf'); + const plan = planConnect({ + accountId: 'acct1', + bucket: 'taro-surf-images', + project: 'taro-surf', + host: 'taro.surf', + zoneId: 'z1', + cdnPresent: true, + pagesPresent: state !== 'absent' + }); + expect(plan).toEqual([]); }); }); diff --git a/scripts/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index 613ed16e..bb7b9ae4 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -6,8 +6,12 @@ * The Cloudflare REST caller (`cfApi`) and the bare-host / image-resizing * helpers live in setup-lib.ts and are reused verbatim — this file adds only * new, self-contained functions so it rebases cleanly against branches that - * also touch setup-lib.ts. + * also touch setup-lib.ts. It imports that module's `CfApiResult` shape (so the + * shapes this file consumes can't drift from what `cfApi` actually returns) plus + * the two failure-reporting helpers, so a can't-verify rung explains itself with + * the same per-status honesty the CLI's own messages use. */ +import { failureDetail, isRecord, statusLabel, type CfApiResult } from './setup-lib.ts'; /** `cdn.` — the CDN subdomain we attach to the images R2 bucket. */ export function cdnHost(host: string): string { @@ -73,11 +77,13 @@ export function classifyZone(result: unknown): ZoneStatus { */ export async function resolveZone( candidates: string[], - lookup: (name: string) => Promise<{ ok: boolean; status: number; result?: unknown }> + lookup: (name: string) => Promise ): Promise<{ zone: ZoneStatus; zoneName: string | null; errorStatus: number | null; + /** The failed lookup's parsed `errors` body, for the caller's error detail. */ + errors: unknown; /** The candidate whose lookup failed — error messages must name IT, not the host. */ failedName: string | null; }> { @@ -88,12 +94,20 @@ export async function resolveZone( zone: { exists: false, active: false }, zoneName: null, errorStatus: res.status, + errors: res.errors, failedName: name }; const zone = classifyZone(res.result); - if (zone.exists) return { zone, zoneName: name, errorStatus: null, failedName: null }; + if (zone.exists) + return { zone, zoneName: name, errorStatus: null, errors: undefined, failedName: null }; } - return { zone: { exists: false, active: false }, zoneName: null, errorStatus: null, failedName: null }; + return { + zone: { exists: false, active: false }, + zoneName: null, + errorStatus: null, + errors: undefined, + failedName: null + }; } /** @@ -173,7 +187,9 @@ interface BucketDomain { export function findBucketDomain(result: unknown, name: string): BucketDomain | undefined { const list = ((result as { domains?: BucketDomain[] } | undefined)?.domains ?? (Array.isArray(result) ? (result as BucketDomain[]) : [])) as BucketDomain[]; - return list.find((d) => d.domain === name); + // A list entry that isn't an object would throw on the property read, turning a + // malformed body into a crash instead of the 'unknown' its caller reports. + return list.find((d) => isRecord(d) && d.domain === name); } /** @@ -181,7 +197,7 @@ export function findBucketDomain(result: unknown, name: string): BucketDomain | * treat differently: `attached` (enabled — nothing to do), `disabled` (present * but turned off — enable it in the dashboard, do NOT re-create), `absent` (not * there — safe to create), and `unknown` (the GET failed, e.g. the token lacks - * Account · Workers R2 Storage · Read — we must NOT report this as "not attached"). + * Account → Workers R2 Storage: Read — we must NOT report this as "not attached"). */ export type CdnDomainState = 'attached' | 'disabled' | 'absent' | 'unknown'; @@ -190,6 +206,12 @@ export function cdnDomainState( name: string ): CdnDomainState { if (!res.ok) return 'unknown'; + // Same rule as pagesDomainState: an ok response carrying no domain list is + // unreadable, not empty. findBucketDomain coerces either shape to [], so + // without this a malformed body reads as 'absent' and green-lights the attach + // this read exists to gate. + const r = res.result as { domains?: unknown } | undefined; + if (!Array.isArray(res.result) && !Array.isArray(r?.domains)) return 'unknown'; const d = findBucketDomain(res.result, name); if (!d) return 'absent'; return d.enabled === false ? 'disabled' : 'attached'; @@ -206,7 +228,29 @@ export function bucketDomainTlsIssued(result: unknown, name: string): boolean { */ export function pagesDomainAttached(result: unknown, host: string): boolean { const list = (Array.isArray(result) ? result : []) as { name?: string }[]; - return list.some((d) => d.name === host); + // Same guard as findBucketDomain: a non-object entry would throw on the + // property read rather than simply failing to match. + return list.some((d) => isRecord(d) && d.name === host); +} + +/** + * The Pages project's state for `host`, with the same auth-vs-absent distinction + * the bucket read gets: `unknown` when the GET itself failed. Reading an empty + * `result` off a failed response would say "not attached" and send us straight + * into an attach the read was supposed to tell us whether we needed. + */ +export type PagesDomainState = 'attached' | 'absent' | 'unknown'; + +export function pagesDomainState( + res: { ok: boolean; status: number; result?: unknown }, + host: string +): PagesDomainState { + if (!res.ok) return 'unknown'; + // An ok response whose body isn't a list is unreadable, not empty. Letting it + // coerce to [] would report 'absent' and walk into the attach this read exists + // to gate, which is the same mistake as reading a failed response. + if (!Array.isArray(res.result)) return 'unknown'; + return pagesDomainAttached(res.result, host) ? 'attached' : 'absent'; } /** @@ -232,7 +276,8 @@ export interface ConnectPlanInput { zoneId: string; /** The CDN custom domain already exists (attached OR disabled OR unverifiable) — don't create it. */ cdnPresent: boolean; - pagesAttached: boolean; + /** The Pages domain already exists (attached OR unverifiable) — don't create it. */ + pagesPresent: boolean; } export interface PlannedMutation { @@ -240,6 +285,8 @@ export interface PlannedMutation { path: string; body: Record; label: string; + /** The token permission THIS call needs — printed only when it fails with 401/403. */ + scopeHint: string; } /** @@ -249,6 +296,11 @@ export interface PlannedMutation { * present, so a re-run after a partial success issues just the missing call * (idempotent) — and a present-but-disabled CDN domain is left for the operator * to re-enable rather than re-created. Adds nothing else to the zone. + * + * Both `present` flags mean "don't create", not "confirmed attached": when the + * read that would have told us failed, the caller passes true and we plan + * nothing, because the one thing we must not do is mutate on a state we never + * managed to read. */ export function planConnect(i: ConnectPlanInput): PlannedMutation[] { const out: PlannedMutation[] = []; @@ -258,14 +310,16 @@ export function planConnect(i: ConnectPlanInput): PlannedMutation[] { method: 'POST', path: `/accounts/${i.accountId}/r2/buckets/${i.bucket}/domains/custom`, body: { domain: cdn, zoneId: i.zoneId, enabled: true, minTLS: '1.2' }, - label: `attach ${cdn} to the ${i.bucket} bucket` + label: `attach ${cdn} to the ${i.bucket} bucket`, + scopeHint: 'Account → Workers R2 Storage: Edit' }); - if (!i.pagesAttached) + if (!i.pagesPresent) out.push({ method: 'POST', path: `/accounts/${i.accountId}/pages/projects/${i.project}/domains`, body: { name: i.host }, - label: `attach ${i.host} to the ${i.project} Pages project` + label: `attach ${i.host} to the ${i.project} Pages project`, + scopeHint: 'Account → Cloudflare Pages: Edit' }); return out; } @@ -305,10 +359,15 @@ export interface LadderInputs { zoneActive: boolean; /** attached = healthy, absent = not there, disabled = present-but-off, unknown = R2 read failed. */ cdnState: CdnDomainState; + /** The R2 read itself, so an `unknown` rung can say WHY; absent when it never ran. */ + cdnRead?: { ok: boolean; status: number; errors?: unknown }; /** true = cert issued, false = still provisioning, null = couldn't verify (R2 read failed / not attached). */ tlsIssued: boolean | null; - /** true = on, false = off, null = couldn't verify (token lacks Zone Settings·Read). */ + /** true = on, false = off, null = couldn't verify (the settings read failed). */ imageTransforms: boolean | null; + /** The Image Transformations read's HTTP status + errors body, for the same reason. */ + imageTransformsStatus?: number; + imageTransformsErrors?: unknown; cdnLoad: CdnProbe; /** The RESOLVED zone's name (the parent zone for a subdomain host); null/absent when no zone matched. */ zoneName?: string | null; @@ -316,6 +375,32 @@ export interface LadderInputs { candidates?: string[]; } +/** + * The reason a read couldn't be verified, in the CLI's own per-status form: the + * scope hint only on 401/403, the did-not-respond line on a thrown fetch, the + * API's words when it gave any. Empty when the caller recorded no status — we + * would rather say nothing than name a cause we never observed. + */ +function readFailureTail(status: number | undefined, errors: unknown, scopeHint: string): string { + return status === undefined ? '' : failureDetail({ status, errors }, scopeHint); +} + +/** + * The same reason for a domain read that returned nothing usable — but told + * apart from a failed one. cfApi reports `ok` only when the API said success, so + * an ok response we couldn't read is a partial body: saying "the API reported + * failure" there would state the opposite of what happened. Empty when the read + * never ran, for the same reason readFailureTail is. + */ +export function domainReadFailure( + res: { ok: boolean; status: number; errors?: unknown } | undefined, + scopeHint: string +): string { + if (!res) return ''; + if (res.ok) return `${statusLabel(res.status)}; the response carried no domain list`; + return failureDetail(res, scopeHint); +} + /** * Builds the six-rung health ladder in order. It's a strict chain: the first * hard `fail` blocks every rung below it (marked `skip`), because a deeper check @@ -361,7 +446,10 @@ export function buildLadder(i: LadderInputs): Rung[] { i.cdnState === 'attached' ? 'pass' : i.cdnState === 'absent' ? 'fail' : 'warn'; const cdnAction = i.cdnState === 'unknown' - ? `Couldn't verify — the token lacks Account · Workers R2 Storage · Read (or a transient API error). Check the bucket's Custom Domains in the dashboard.` + ? `Couldn't verify${domainReadFailure( + i.cdnRead, + 'Account → Workers R2 Storage: Read' + )}. Check the bucket's Custom Domains in the dashboard.` : i.cdnState === 'disabled' ? `${cdn} is attached but DISABLED — re-enable it in dashboard → R2 → your images bucket → Settings → Custom Domains.` : `Run \`npm run connect-domains\` (no --check) to attach ${cdn} to the images bucket.`; @@ -380,7 +468,11 @@ export function buildLadder(i: LadderInputs): Rung[] { `Image Transformations are enabled on the ${zoneName} zone`, i.imageTransforms === true ? 'pass' : 'warn', i.imageTransforms === null - ? `Couldn't verify Image Transformations (token lacks Zone Settings·Read); check dashboard → ${zoneName} → Images → Transformations.` + ? `Couldn't verify Image Transformations${readFailureTail( + i.imageTransformsStatus, + i.imageTransformsErrors, + 'Zone → Zone Settings: Read' + )}; check dashboard → ${zoneName} → Images → Transformations.` : `Enable it: dashboard → ${zoneName} → Images → Transformations → "Enable for zone". Until on, thumbnails serve the full-size original or 404.` ); diff --git a/scripts/connect-domains.test.ts b/scripts/connect-domains.test.ts index a3c77f75..0956444a 100644 --- a/scripts/connect-domains.test.ts +++ b/scripts/connect-domains.test.ts @@ -129,6 +129,60 @@ describe('runDoctor', () => { expect(text).not.toContain('Add the root domain'); }); + // The reads never ran, so there is no status to report. A rung that filled in + // "(HTTP 0); the Cloudflare API did not respond" here would be describing a + // call that was never made — and a mutant doing exactly that survived. + it('says couldn\'t-verify with NO status and no scope when the zone is unusable', async () => { + const out: string[] = []; + const spy = vi.spyOn(console, 'log').mockImplementation((...x) => { + out.push(x.join(' ')); + }); + const { api, calls } = recordingApi(); + await runDoctor(args({ zone: { exists: true, active: false, id: 'z1' } }), deps(api)); + spy.mockRestore(); + const text = out.join('\n'); + expect(calls).toEqual([]); + expect(text).toContain("Couldn't verify. Check the bucket's Custom Domains"); + expect(text).toContain("Couldn't verify Image Transformations;"); + expect(text).not.toMatch(/HTTP \d/); + expect(text).not.toContain('the Cloudflare API did not respond'); + expect(text).not.toContain('Workers R2 Storage: Read'); + expect(text).not.toContain('Zone → Zone Settings: Read'); + expect(text).not.toContain('carried no domain list'); + }); + + // One 'unknown' outcome, several causes. The doctor threads the read's own + // status and errors into the rung, so a 5xx or an unreachable API never reads + // as "your token is missing a scope". + it('gives the Image Transformations rung a different reason per failure status', async () => { + const render = async (ir: CfApiResult) => { + const out: string[] = []; + const spy = vi.spyOn(console, 'log').mockImplementation((...x) => { + out.push(x.join(' ')); + }); + const { api } = recordingApi({ image_resizing: ir }); + await runDoctor(args(), deps(api)); + spy.mockRestore(); + return out.join('\n'); + }; + + const denied = await render({ ok: false, status: 403 }); + expect(denied).toContain('token needs Zone → Zone Settings: Read'); + + const server = await render({ + ok: false, + status: 500, + errors: [{ code: 10000, message: 'Internal error' }] + }); + expect(server).not.toContain('Zone → Zone Settings: Read'); + expect(server).toContain('(HTTP 500)'); + expect(server).toContain('the API said 10000: Internal error'); + + const offline = await render({ ok: false, status: 0 }); + expect(offline).not.toContain('Zone → Zone Settings: Read'); + expect(offline).toContain('the Cloudflare API did not respond'); + }); + it('always returns 0 (diagnostic), even when a rung hard-fails', async () => { const spy = vi.spyOn(console, 'log').mockImplementation(() => {}); // Domain unreachable → cdn-loads fails, but the command still exits 0. @@ -159,6 +213,14 @@ describe('connect-domains.ts ↔ candidate-walk source contract', () => { expect(src).toMatch(/resolveZone\(\s*candidates/); }); + it('attributes the API reason on the zone-lookup error line', () => { + // Three sites print '; the API said …' (this one, setup.ts's warn, and + // cfFailureTail); pin this inline copy so the wording cannot drift back + // to a bare parenthetical. + expect(src).toContain('; the API said ${apiWhy}'); + expect(src).not.toContain('(${apiWhy})'); + }); + it('passes the candidates to zoneGuidance', () => { expect(src).toMatch(/zoneGuidance\(\s*zone,\s*host,\s*candidates,\s*zoneName\s*\)/); }); @@ -176,4 +238,69 @@ describe('connect-domains.ts ↔ candidate-walk source contract', () => { it('keeps the whole-zone disclosure on the transforms bullet', () => { expect(src).toMatch(/affects the whole zone, not just \$\{host\}/); }); + + it('reads the API reason whatever the status was', () => { + // Gating the summary on a 2xx dropped it for 400/404/500 — the statuses whose + // reason the operator most needs, since a 2xx-with-success:false is the only + // one they could have guessed at. + expect(src).toContain('const apiWhy = cfErrorSummary(errors);'); + }); +}); + +// Every failed call in the mutating path reports through failureDetail, so the +// reason tracks the status: the scope only on 401/403, the API's own words when +// it gave any, and the network line for a thrown fetch. The previous copy +// recommended the R2 read scope for every failure — including a 5xx and an +// unreachable API — and printed a bare "(HTTP 0)" carrying nothing. +describe('connect-domains.ts ↔ failure-reporting source contract', () => { + const src = readFileSync( + join(dirname(fileURLToPath(import.meta.url)), 'connect-domains.ts'), + 'utf8' + ); + + // The two domain reads go through domainReadFailure, which also tells an ok + // response carrying no list apart from a failed one; everything else composes + // statusLabel + cfFailureTail via failureDetail. + it('reports both domain reads through domainReadFailure', () => { + expect(src).toMatch(/domainReadFailure\(\s*r2Res,\s*'Account → Workers R2 Storage: Read'/); + expect(src).toMatch(/domainReadFailure\(\s*pagesRes,\s*'Account → Cloudflare Pages: Read'/); + }); + + it("reports a failed attach with that mutation's own scope", () => { + expect(src).toContain('failureDetail(res, m.scopeHint)'); + }); + + it('reports a failed Image Transformations enable the same way', () => { + expect(src).toMatch(/failureDetail\(\s*patched,\s*'Zone → Zone Settings: Edit'/); + }); + + it('reads the Image Transformations reason off the response, not off a fixed scope', () => { + expect(src).toMatch(/failureDetail\(\s*irGet,\s*'Zone → Zone Settings: Read'/); + }); +}); + +// A read that failed is the one that would have told us whether the mutation is +// needed, so it can't license the mutation. main() self-executes and isn't +// importable, so pin the wiring at the source level; the behavior itself is +// covered in connect-domains-lib.test.ts (pagesDomainState + planConnect). +describe('connect-domains.ts ↔ never-mutate-on-an-unread-state contract', () => { + const src = readFileSync( + join(dirname(fileURLToPath(import.meta.url)), 'connect-domains.ts'), + 'utf8' + ); + + it('derives the Pages attach from pagesDomainState, which reports a failed read as unknown', () => { + expect(src).toMatch(/pagesDomainState\(\s*pagesRes,\s*host\s*\)/); + expect(src).toContain("pagesPresent: pagesState !== 'absent'"); + }); + + it('skips the attach and says so when either read came back unknown', () => { + expect(src).toMatch(/pagesState === 'unknown'/); + expect(src).toMatch(/skipping the \$\{host\} attach/); + expect(src).toMatch(/skipping the \$\{cdn\} attach/); + }); + + it('never claims "Already connected" off a read that failed', () => { + expect(src).toMatch(/Nothing changed — the \$\{unread\} attachment couldn't be read/); + }); }); diff --git a/scripts/connect-domains.ts b/scripts/connect-domains.ts index f84359d6..a8a16686 100644 --- a/scripts/connect-domains.ts +++ b/scripts/connect-domains.ts @@ -14,7 +14,7 @@ * the resolved zone (for a subdomain host that's the parent zone serving it); * each is idempotent and beyond those nothing else in the zone is touched. The * API token comes from - * CLOUDFLARE_API_TOKEN (Zone·Read + DNS·Edit, plus Zone Settings·Edit to enable + * CLOUDFLARE_API_TOKEN (Zone → Zone: Read + Zone → DNS: Edit, plus Zone → Zone Settings: Edit to enable * Image Transformations for you); it is read from the env, never stored or * printed. * @@ -32,6 +32,8 @@ import { stdin, stdout, env, argv, exit } from 'node:process'; import { fileURLToPath } from 'node:url'; import { cfApi, + cfErrorSummary, + failureDetail, hostFromDomain, zoneNameCandidates, imageResizingOutcome, @@ -46,8 +48,9 @@ import { zoneConsentLabel, cdnDomainState, bucketDomainTlsIssued, - pagesDomainAttached, + pagesDomainState, classifyCdnProbe, + domainReadFailure, planConnect, siteUrlMismatch, buildLadder, @@ -58,11 +61,11 @@ import { const TOKEN_RECIPE = 'Create a Cloudflare API token (dash → My Profile → API Tokens → Create Token → Custom token) with:\n' + - ' • Zone · Zone · Read\n' + - ' • Zone · DNS · Edit\n' + - ' • Account · Workers R2 Storage · Edit (to attach the bucket custom domain)\n' + - ' • Account · Cloudflare Pages · Edit (to attach the site domain)\n' + - ' • Zone · Zone Settings · Edit (optional; lets it enable Image Transformations)\n' + + ' • Zone → Zone: Read\n' + + ' • Zone → DNS: Edit\n' + + ' • Account → Workers R2 Storage: Edit (to attach the bucket custom domain)\n' + + ' • Account → Cloudflare Pages: Edit (to attach the site domain)\n' + + ' • Zone → Zone Settings: Edit (optional; lets it enable Image Transformations)\n' + 'Then export CLOUDFLARE_API_TOKEN and CLOUDFLARE_ACCOUNT_ID and re-run.'; /** Best-effort read of the deployed `siteUrl` site-setting (forward-compat with SONA-24). */ @@ -148,8 +151,9 @@ async function main(): Promise { // a hard token error; any other failed lookup aborts too (a transient error // must not silently pick the parent zone or read as "no zone"). const candidates = zoneNameCandidates(host); - const { zone, zoneName, errorStatus, failedName } = await resolveZone(candidates, (name) => - cfApi(cfToken, `/zones?name=${encodeURIComponent(name)}`) + const { zone, zoneName, errorStatus, errors, failedName } = await resolveZone( + candidates, + (name) => cfApi(cfToken, `/zones?name=${encodeURIComponent(name)}`) ); if (errorStatus !== null) { // Name the candidate whose lookup failed — for a subdomain host that can @@ -163,8 +167,13 @@ async function main(): Promise { `✖ Could not reach the Cloudflare API while looking up the zone for ${lookupName} — check your network and re-run.` ); } else { + // The API answered and the lookup still failed — repeat its own reason + // whenever it gave one. A 400/404/500 body carries that reason as often + // as a 2xx-with-success:false does, and gating the summary on 2xx threw + // it away for exactly the statuses an operator can least explain. + const apiWhy = cfErrorSummary(errors); console.error( - `✖ Cloudflare API error (HTTP ${errorStatus}) while looking up the zone for ${lookupName} — wait a moment and re-run.` + `✖ Cloudflare API error (HTTP ${errorStatus}${apiWhy ? `; the API said ${apiWhy}` : ''}) while looking up the zone for ${lookupName} — wait a moment and re-run.` ); } return 1; @@ -185,15 +194,32 @@ async function main(): Promise { const r2Res = await cfApi(cfToken, `/accounts/${cfAccount}/r2/buckets/${bucket}/domains/custom`); const pagesRes = await cfApi(cfToken, `/accounts/${cfAccount}/pages/projects/${project}/domains`); const cdnState = cdnDomainState(r2Res, cdn); - const pagesAttached = pagesDomainAttached(pagesRes.result, host); + const pagesState = pagesDomainState(pagesRes, host); if (cdnState === 'disabled') console.warn( `⚠ ${cdn} is already on the bucket but DISABLED — re-enable it in dashboard → R2 → ${bucket} → Custom Domains (not re-creating it).` ); if (cdnState === 'unknown') + // The reason comes from the response, not from a guess: naming the R2 read + // scope for a 500 or an unreachable API sends the operator to re-mint a + // token that was never the problem. console.warn( - `⚠ Couldn't read the bucket's custom domains (token may lack Account · Workers R2 Storage · Read) — skipping the ${cdn} attach.` + `⚠ Couldn't read the bucket's custom domains${domainReadFailure( + r2Res, + 'Account → Workers R2 Storage: Read' + )} — skipping the ${cdn} attach.` + ); + + if (pagesState === 'unknown') + // Same discipline as the R2 path: the read that failed is the one that + // would have told us whether the attach is needed, so we don't attach. + // Posting it anyway would be mutating on state we never managed to read. + console.warn( + `⚠ Couldn't read the ${project} Pages project's domains${domainReadFailure( + pagesRes, + 'Account → Cloudflare Pages: Read' + )} — skipping the ${host} attach.` ); const plan = planConnect({ @@ -203,7 +229,7 @@ async function main(): Promise { host, zoneId: zone.id ?? '', cdnPresent: cdnState !== 'absent', - pagesAttached + pagesPresent: pagesState !== 'absent' }); // Image Transformations current state (read-only), so the preview can honestly @@ -213,11 +239,19 @@ async function main(): Promise { const willEnableTransforms = transformsCurrent === false; if (plan.length === 0 && !willEnableTransforms) { - console.log(`✔ Already connected: ${cdn} → ${bucket} bucket and ${host} → ${project} Pages.`); + // "Already connected" is a claim about state we read. When either read + // failed, an empty plan means we declined to act, not that the domains + // are in place — say which one it was. + const unread = cdnState === 'unknown' ? cdn : pagesState === 'unknown' ? host : null; + console.log( + unread + ? `ℹ Nothing changed — the ${unread} attachment couldn't be read, so nothing was created.` + : `✔ Already connected: ${cdn} → ${bucket} bucket and ${host} → ${project} Pages.` + ); console.log( transformsCurrent === true ? ' Image Transformations: on.' - : " Image Transformations: couldn't verify (token lacks Zone Settings·Read)." + : ` Image Transformations: couldn't verify${failureDetail(irGet, 'Zone → Zone Settings: Read')}.` ); console.log(` Re-check anytime: npm run connect-domains -- --check ${host}`); return 0; @@ -258,7 +292,14 @@ async function main(): Promise { for (const m of plan) { const res = await cfApi(cfToken, m.path, { method: m.method, body: m.body }); if (res.ok) console.log(`✔ ${m.label}`); - else console.warn(`⚠ Could not ${m.label} (HTTP ${res.status}) ${JSON.stringify(res.errors ?? '')}`); + else + // failureDetail keeps the reason honest per status: the call's own scope + // on 401/403, the API's sanitized words when it gave any, and the + // did-not-respond line for a thrown fetch (where a bare "HTTP 0" said + // nothing at all). + console.warn( + `⚠ Could not ${m.label}${failureDetail(res, m.scopeHint)}` + ); } if (willEnableTransforms) { @@ -269,13 +310,13 @@ async function main(): Promise { if (patched.ok) console.log(`✔ Image Transformations enabled on ${zoneLabel}.`); else console.warn( - `⚠ Could not enable Image Transformations (HTTP ${patched.status}) — enable it in the dashboard.` + `⚠ Could not enable Image Transformations${failureDetail(patched, 'Zone → Zone Settings: Edit')} — enable it in the dashboard.` ); } else if (transformsCurrent === true) { console.log('✔ Image Transformations already on.'); } else { console.log( - "⚠ Image Transformations: couldn't verify (token lacks Zone Settings·Read) — enable it in the dashboard." + `⚠ Image Transformations: couldn't verify${failureDetail(irGet, 'Zone → Zone Settings: Read')} — enable it in the dashboard.` ); } @@ -335,13 +376,19 @@ export async function runDoctor(a: DoctorArgs, deps: DoctorDeps = defaultDoctorD let tlsIssued: boolean | null = null; let transforms: boolean | null = null; let cdnLoad: CdnProbe = 'unreachable'; + // Left undefined when the zone is unusable and the reads never ran — a rung + // then says "couldn't verify" with no cause, rather than inventing one. + let r2Read: CfApiResult | undefined; + let irRead: CfApiResult | undefined; if (zoneUsable(a.zone)) { const r2Res = await deps.api(a.cfToken, `/accounts/${a.cfAccount}/r2/buckets/${a.bucket}/domains/custom`); + r2Read = r2Res; cdnState = cdnDomainState(r2Res, a.cdn); tlsIssued = cdnState === 'attached' ? bucketDomainTlsIssued(r2Res.result, a.cdn) : null; const ir = await deps.api(a.cfToken, `/zones/${a.zone.id}/settings/image_resizing`); + irRead = ir; transforms = imageResizingOutcome(ir, false); // Independent HTTPS probe (not via the API) — informative even when the token @@ -356,8 +403,11 @@ export async function runDoctor(a: DoctorArgs, deps: DoctorDeps = defaultDoctorD zoneName: a.zoneName, candidates: a.candidates, cdnState, + cdnRead: r2Read, tlsIssued, imageTransforms: transforms, + imageTransformsStatus: irRead?.status, + imageTransformsErrors: irRead?.errors, cdnLoad }); for (const line of renderLadder(ladder)) console.log(line); diff --git a/scripts/setup-lib.test.ts b/scripts/setup-lib.test.ts index 2d7154e9..b474d8d9 100644 --- a/scripts/setup-lib.test.ts +++ b/scripts/setup-lib.test.ts @@ -8,6 +8,7 @@ import { buildSeedSql, sanitizeProjectName, isR2NotEnabled, + bucketCreateSucceeded, ensureUrlScheme, ghSecretEligibility, parseDatabaseId, @@ -21,10 +22,21 @@ import { imageResizingIsOn, ciWiringEntries, cfApi, + cfErrorSummary, securitySummaryLines, + zoneLookupWarnLines, + storageSummaryLines, + telegramSummaryLine, + resendSecretWarnLines, + setupTokenLines, + provisioningNoteLine, pagesPatchConfirmsSitekey, - cdnAttachmentLines + cdnAttachmentLines, + type CfApiResult, + type SecuritySummaryInput } from './setup-lib.ts'; +import { applyDownloadRateLimit, SCOPE_HINT as WAF_SCOPE_HINT } from './waf-lib.ts'; +import { provisionTurnstileWidget, SCOPE_HINT as TURNSTILE_SCOPE_HINT } from './turnstile-lib.ts'; describe('buildMigrationSql', () => { it('creates schema_migrations and records each file after its body, in order', () => { @@ -169,6 +181,28 @@ describe('isR2NotEnabled', () => { }); }); +describe('bucketCreateSucceeded', () => { + it('is true for a clean create', () => { + expect(bucketCreateSucceeded('Created bucket sona-images', true)).toBe(true); + }); + + // The re-run case: wrangler exits non-zero, but the bucket is there, so setup + // must not report the R2 backend as broken. + it('treats an already-exists failure as the bucket being in place', () => { + expect(bucketCreateSucceeded('A bucket with that name already exists', false)).toBe(true); + expect(bucketCreateSucceeded('Failed [code: 10004]', false)).toBe(true); + }); + + // The failure that prompted this: a token without Account → Workers R2 + // Storage: Edit carries none of isR2NotEnabled's markers, so sniffing the text + // read it as success and setup bound a bucket that does not exist. + it('is false for a permission failure and for R2 not being enabled', () => { + expect(bucketCreateSucceeded('Authentication error [code: 10000]', false)).toBe(false); + expect(bucketCreateSucceeded('Failed [code: 10042]: R2 not enabled', false)).toBe(false); + expect(bucketCreateSucceeded('', false)).toBe(false); + }); +}); + describe('ensureUrlScheme', () => { it('prepends https:// to a bare host (the cdn. default)', () => { expect(ensureUrlScheme('cdn.taro.surf')).toBe('https://cdn.taro.surf'); @@ -611,47 +645,129 @@ describe('ciWiringEntries ↔ workflow YAML contract', () => { }); describe('securitySummaryLines', () => { - const turnstileWarning = ' • Admin-login bot check: NOT set (token lacks Account · Turnstile · Edit).'; + const turnstileWarning = ' • Admin-login bot check: NOT set.'; + + const sum = (over: Partial) => + securitySummaryLines({ + host: 'taro.surf', + downloadRateLimit: null, + downloadRateLimitDetail: null, + turnstileStatus: null, + turnstileDetail: null, + turnstileWired: false, + ...over + }); + + /** + * waf-lib's REAL error detail for the given stubbed API outcomes. The summary + * tests below must exercise wording waf-lib actually emits — a hand-typed + * fixture once asserted against phrasing waf-lib never produced, making the + * "does not blame token scope" test vacuously green. + */ + async function realRateLimitDetail(routes: Record): Promise { + const res = await applyDownloadRateLimit( + 'test-token', + 'taro.surf', + async (_token, path, init: { method?: string } = {}) => + routes[`${init.method ?? 'GET'} ${path}`] ?? { ok: false, status: 500 } + ); + expect(res.status).toBe('error'); + return res.detail; + } it('prints the Turnstile warning for EVERY rate-limit outcome (regression: a missing brace once nested it inside the applied branch)', () => { for (const rl of [null, 'exists', 'error', 'created', 'updated'] as const) { - const lines = securitySummaryLines('taro.surf', rl, null, 'error', true); + const lines = sum({ downloadRateLimit: rl, turnstileStatus: 'error', turnstileWired: true }); expect(lines, `downloadRateLimit=${rl}`).toContain(turnstileWarning); } }); it('reports an applied rate limit and a created Turnstile widget together', () => { - const lines = securitySummaryLines('taro.surf', 'created', null, 'created', true); + const lines = sum({ downloadRateLimit: 'created', turnstileStatus: 'created', turnstileWired: true }); expect(lines.join('\n')).toContain('Public-endpoint rate limit: applied to the taro.surf zone'); - expect(lines.join('\n')).toContain('Admin-login bot check: Turnstile created for taro.surf'); + expect(lines.join('\n')).toContain('Admin-login bot check: Turnstile created for taro.surf.'); }); - it('repeats waf-lib’s failure reason and the retry command for a rate-limit error', () => { - const detail = 'token has no access to zone taro.surf: add Zone · WAF · Edit'; - const text = securitySummaryLines('taro.surf', 'error', detail, null, false).join('\n'); - expect(text).toContain(detail); + it('repeats waf-lib’s failure reason and the retry command for a rate-limit error', async () => { + // The real no-zone-access detail: the zone query succeeds but returns []. + const detail = await realRateLimitDetail({ + 'GET /zones?name=taro.surf': { ok: true, status: 200, result: [] } + }); + expect(detail).toContain('no taro.surf zone was found on this Cloudflare account'); + const text = sum({ downloadRateLimit: 'error', downloadRateLimitDetail: detail }).join('\n'); + // The reason line carries the real detail and ends in a period. + expect(text).toContain(`Reason: ${detail}.`); + // The connective line between the reason and the retry command. + expect(text).toContain('When that is fixed, run:'); expect(text).toContain('npm run apply-download-ratelimit -- taro.surf'); }); - it('does not blame token scope for a non-permission rate-limit failure', () => { - const detail = 'failed to write the rate-limit rule to taro.surf (HTTP 500)'; - const text = securitySummaryLines('taro.surf', 'error', detail, null, false).join('\n'); + it('does not blame token scope for a non-permission rate-limit failure', async () => { + // The real write-failure detail: zone resolves, no ruleset yet, PUT 500s. + const detail = await realRateLimitDetail({ + 'GET /zones?name=taro.surf': { ok: true, status: 200, result: [{ id: 'z1' }] }, + 'GET /zones/z1/rulesets/phases/http_ratelimit/entrypoint': { ok: false, status: 404 }, + 'PUT /zones/z1/rulesets/phases/http_ratelimit/entrypoint': { ok: false, status: 500 } + }); + // Pin the branch, not just the status: the stub 500s any unmatched route, + // so path drift would silently reroute this to the zone query. + expect(detail).toContain('failed to write'); + expect(detail).toContain('HTTP 500'); + const text = sum({ downloadRateLimit: 'error', downloadRateLimitDetail: detail }).join('\n'); expect(text).toContain(detail); - expect(text).not.toContain('token lacks Zone · WAF · Edit'); + expect(text).not.toContain('token needs'); expect(text).toContain('npm run apply-download-ratelimit -- taro.surf'); }); it('falls back to a generic failure line when no detail survived', () => { - const text = securitySummaryLines('taro.surf', 'error', null, null, false).join('\n'); - expect(text).toContain('NOT set (provisioning failed)'); - expect(text).not.toContain('token lacks'); + const text = sum({ downloadRateLimit: 'error' }).join('\n'); + expect(text).toContain('Public-endpoint rate limit: NOT set.'); + expect(text).toContain('Reason: none reported.'); + expect(text).toContain('When that is fixed, run:'); + expect(text).not.toContain('token needs'); + }); + + it('repeats turnstile-lib’s real failure reason for a Turnstile error', async () => { + // The real no-scope detail: the widget list 403s. + const res = await provisionTurnstileWidget( + 'test-token', + 'acct1', + 'taro.surf', + async () => ({ ok: false, status: 403 }) + ); + expect(res.status).toBe('error'); + expect(res.detail).toContain('token needs'); + const text = sum({ turnstileStatus: 'error', turnstileDetail: res.detail }).join('\n'); + expect(text).toContain('Admin-login bot check: NOT set.'); + expect(text).toContain(`Reason: ${res.detail}.`); + expect(text).toContain('When that is fixed, re-run setup to protect /admin/login.'); + }); + + it('does not blame token scope for a non-permission Turnstile failure', async () => { + // The real transient detail: the widget list 500s. + const res = await provisionTurnstileWidget( + 'test-token', + 'acct1', + 'taro.surf', + async () => ({ ok: false, status: 500 }) + ); + expect(res.status).toBe('error'); + const text = sum({ turnstileStatus: 'error', turnstileDetail: res.detail }).join('\n'); + expect(text).toContain(`Reason: ${res.detail}.`); + expect(text).not.toContain('token needs'); + }); + + it('falls back to a generic Turnstile failure line when no detail survived', () => { + const text = sum({ turnstileStatus: 'error' }).join('\n'); + expect(text).toContain('Admin-login bot check: NOT set.'); + expect(text).toContain('Reason: none reported.'); }); it('reports NO bot check when the widget provisioned but the wiring failed', () => { // The login check fails open without the sitekey var + secret, so a // provisioned widget with failed wiring must never read as enforced. for (const status of ['created', 'exists'] as const) { - const text = securitySummaryLines('taro.surf', 'exists', null, status, false).join('\n'); + const text = sum({ downloadRateLimit: 'exists', turnstileStatus: status }).join('\n'); expect(text).toContain('NOT confirm the TURNSTILE_SITEKEY'); expect(text).toContain('/admin/login has NO bot check'); // Honest on re-runs: the claim is scoped to this run / first runs. @@ -662,28 +778,210 @@ describe('securitySummaryLines', () => { }); it('never prints the enforced claim unless the wiring landed', () => { - const wired = securitySummaryLines('taro.surf', null, null, 'created', true).join('\n'); + const wired = sum({ turnstileStatus: 'created', turnstileWired: true }).join('\n'); expect(wired).toContain('enforced once deployed'); - const unwired = securitySummaryLines('taro.surf', null, null, 'created', false).join('\n'); + const unwired = sum({ turnstileStatus: 'created' }).join('\n'); expect(unwired).not.toContain('enforced once deployed'); }); it('names the RESOLVED parent zone in the applied line for a subdomain host', () => { - const text = securitySummaryLines( - 'sona.taro.surf', - 'created', - null, - null, - false, - 'taro.surf' - ).join('\n'); + const text = sum({ + host: 'sona.taro.surf', + downloadRateLimit: 'created', + zoneName: 'taro.surf' + }).join('\n'); expect(text).toContain('applied to the taro.surf zone'); expect(text).not.toContain('applied to the sona.taro.surf zone'); }); it('stays silent about pre-existing rate limits and unattempted Turnstile', () => { - expect(securitySummaryLines('taro.surf', 'exists', null, null, false)).toEqual([]); - expect(securitySummaryLines('taro.surf', null, null, null, false)).toEqual([]); + expect(sum({ downloadRateLimit: 'exists' })).toEqual([]); + expect(sum({})).toEqual([]); + }); +}); + +describe('zoneLookupWarnLines', () => { + const warn = (status: number, errors?: unknown) => zoneLookupWarnLines('taro.surf', status, errors).join('\n'); + + it('says the API did not respond on a thrown fetch, never "HTTP 0"', () => { + const text = warn(0); + expect(text).toContain('the Cloudflare API did not respond'); + expect(text).not.toContain('HTTP 0'); + expect(text).not.toContain('Zone → Zone: Read'); + }); + + it('names Zone → Zone: Read only on a 401/403', () => { + for (const status of [401, 403]) { + const text = warn(status, [{ code: 9109, message: 'Unauthorized' }]); + expect(text).toContain(`(HTTP ${status})`); + expect(text).toContain('token needs Zone → Zone: Read'); + expect(text).toContain('the API said 9109: Unauthorized'); + } + }); + + it('repeats the API reason on a 2xx whose body said success:false', () => { + const text = warn(200, [{ code: 1001, message: 'nope' }]); + expect(text).toContain('the API reported failure (1001: nope)'); + expect(text).not.toContain('Zone → Zone: Read'); + }); + + it('repeats the API reason on a non-2xx that carried one, without guessing a scope', () => { + // The bug this replaced computed the reason only for a 2xx, so a 400/404/500 + // that carried one dropped it — exactly the statuses an operator can least + // explain on their own. + for (const status of [400, 404, 500]) { + const text = warn(status, [{ code: 1002, message: 'boom' }]); + expect(text).toContain(`(HTTP ${status})`); + expect(text).toContain('the API said 1002: boom'); + expect(text).not.toContain('Zone → Zone: Read'); + } + }); + + it('prints just the status when the failure carried no reason', () => { + const text = warn(500); + expect(text).toContain('(HTTP 500)'); + expect(text).not.toContain('the API said'); + }); + + it('names the failed candidate and keeps the skip + retry wording in every arm', () => { + for (const status of [0, 403, 200, 500]) { + const text = warn(status, [{ code: 1, message: 'x' }]); + expect(text).toContain('Zone lookup failed for taro.surf'); + expect(text).toContain('— skipping the DNS / image-transform preflight.'); + expect(text).toContain('Re-run setup to retry the preflight.'); + } + }); +}); + +describe('storageSummaryLines', () => { + const base = { bucket: 'taro-images', project: 'taro' }; + + it('reports R2 as set up, and NOT READY when R2 is not enabled', () => { + expect( + storageSummaryLines({ + ...base, + provider: 'r2', + r2Missing: false, + bucketReady: true, + uploadThingTokenSet: false + }) + ).toEqual(['Storage backend: Cloudflare R2 (set up).']); + const missing = storageSummaryLines({ + ...base, + provider: 'r2', + r2Missing: true, + bucketReady: false, + uploadThingTokenSet: false + }).join('\n'); + expect(missing).toContain('NOT READY (R2 is not enabled on this account)'); + expect(missing).toContain('npx wrangler r2 bucket create taro-images'); + }); + + // "R2 is not enabled" is one of several ways the create fails. A token without + // Account → Workers R2 Storage: Edit carries none of that error's markers, and + // claiming "(set up)" there binds a bucket that does not exist. + it('reports R2 as NOT READY when the bucket create failed for any other reason', () => { + const failed = storageSummaryLines({ + ...base, + provider: 'r2', + r2Missing: false, + bucketReady: false, + uploadThingTokenSet: false + }).join('\n'); + expect(failed).toContain('NOT READY (the taro-images bucket was not created)'); + expect(failed).toContain('npx wrangler r2 bucket create taro-images'); + expect(failed).not.toContain('(set up)'); + }); + + it('only calls UploadThing set up when the token secret actually landed', () => { + expect( + storageSummaryLines({ + ...base, + provider: 'uploadthing', + r2Missing: false, + bucketReady: false, + uploadThingTokenSet: true + }) + ).toEqual(['Storage backend: UploadThing (set up).']); + const unset = storageSummaryLines({ + ...base, + provider: 'uploadthing', + r2Missing: false, + bucketReady: false, + uploadThingTokenSet: false + }).join('\n'); + expect(unset).toContain('NOT READY (the UPLOADTHING_TOKEN secret is not set)'); + expect(unset).toContain('--project-name taro'); + }); +}); + +describe('resendSecretWarnLines', () => { + it('stays silent when every supplied Resend secret landed', () => { + expect(resendSecretWarnLines([], 'taro-surf')).toEqual([]); + }); + + // A failed put here surfaces months later as a dead password-reset link, so the + // names and the command to fix them have to be said at setup time. + it('names each failed secret and the command that sets it', () => { + const text = resendSecretWarnLines(['RESEND_API_KEY', 'RESEND_FROM'], 'taro-surf').join('\n'); + expect(text).toContain('RESEND_API_KEY, RESEND_FROM'); + expect(text).toContain('Password-reset email stays off'); + expect(text).toContain('npx wrangler pages secret put RESEND_API_KEY --project-name taro-surf'); + expect(text).toContain('npx wrangler pages secret put RESEND_FROM --project-name taro-surf'); + }); +}); + +describe('setup.ts ↔ Resend secret contract', () => { + // main() isn't importable, so pin at source level that both puts are checked + // rather than fired and forgotten, the way they were before. + const src = readFileSync(join(dirname(fileURLToPath(import.meta.url)), 'setup.ts'), 'utf8'); + + it('records a failed Resend put instead of discarding the result', () => { + expect(src).toMatch(/!putSecret\('RESEND_API_KEY'/); + expect(src).toMatch(/!putSecret\('RESEND_FROM'/); + expect(src).toMatch(/resendSecretWarnLines\(resendFailed/); + }); +}); + +describe('telegramSummaryLine', () => { + it('claims the bot token is set only when the put succeeded', () => { + expect(telegramSummaryLine(false, false)).toContain('not configured'); + expect(telegramSummaryLine(true, true)).toContain('enabled (bot token set)'); + const failed = telegramSummaryLine(true, false); + expect(failed).toContain('did NOT get set'); + expect(failed).not.toContain('(bot token set)'); + }); +}); + +describe('setupTokenLines', () => { + const input = { setupToken: 'abc123', project: 'taro' }; + + it('hands over the token when the secret landed', () => { + const text = setupTokenLines({ ...input, setupTokenSet: true }).join('\n'); + expect(text).toContain('SETUP_TOKEN = abc123'); + expect(text).toContain('enter it in the wizard'); + expect(text).not.toContain('did NOT get set'); + }); + + it('says the wizard will reject it, and how to set it, when the put failed', () => { + const text = setupTokenLines({ ...input, setupTokenSet: false }).join('\n'); + expect(text).toContain('did NOT get set'); + expect(text).toContain('npx wrangler pages secret put SETUP_TOKEN --project-name taro'); + // Still print the value — it is the token the operator will need after fixing. + expect(text).toContain('SETUP_TOKEN = abc123'); + }); +}); + +describe('provisioningNoteLine', () => { + it('asserts each half only when its write landed', () => { + expect(provisioningNoteLine(true, true)).toBe( + ' (CRON_SECRET set for the cron jobs; storageProvider seeded.)' + ); + expect(provisioningNoteLine(false, true)).toContain('CRON_SECRET NOT set'); + expect(provisioningNoteLine(true, false)).toContain('storageProvider NOT seeded'); + const both = provisioningNoteLine(false, false); + expect(both).toContain('CRON_SECRET NOT set'); + expect(both).toContain('storageProvider NOT seeded'); }); }); @@ -695,22 +993,318 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { // helper exists to prevent. const src = readFileSync(join(dirname(fileURLToPath(import.meta.url)), 'setup.ts'), 'utf8'); + // The whole securitySummaryLines({...}) call, so property assertions below + // can't accidentally match unrelated code elsewhere in setup.ts. + const summaryCall = src.match(/securitySummaryLines\(\{[\s\S]*?\}\)/)?.[0] ?? ''; + it('passes pagesConfigOk && turnstileSecretSet, not a literal', () => { - expect(src).toMatch(/securitySummaryLines\(/); - expect(src).toMatch(/pagesConfigOk && turnstileSecretSet/); + expect(summaryCall).toMatch(/turnstileWired:\s*pagesConfigOk && turnstileSecretSet/); }); it('passes the resolved zone name so subdomain summaries name the real zone', () => { - expect(src).toMatch(/pagesConfigOk && turnstileSecretSet,\s*\n?\s*resolvedZoneName/); + expect(summaryCall).toMatch(/zoneName:\s*resolvedZoneName/); + }); + + it('assigns each detail from the provisioning result, not a literal', () => { + expect(src).toMatch(/downloadRateLimitDetail\s*=\s*rateLimit\.detail/); + expect(src).toMatch(/turnstileDetail\s*=\s*ts\.detail/); + }); + + // Shorthand properties are the variables themselves — a hardcoded + // `host: 'x'` or `turnstileStatus: null` (the SONA-189 bug shape) breaks + // the trailing-comma match. + it.each(['host', 'downloadRateLimit', 'downloadRateLimitDetail', 'turnstileStatus', 'turnstileDetail'])( + '%s is passed as call-site shorthand', + (prop) => { + expect(summaryCall).toMatch(new RegExp(`\\b${prop},`)); + } + ); + + it('both zone-lookup warn sites repeat the API reason', () => { + // main()s are not importable, so pin the wiring at source level: the + // resolveZone consumers must thread the errors body into a summarizer. + // setup's arms are unit-tested directly via zoneLookupWarnLines below. + expect(src).toContain('zoneLookupWarnLines('); + expect(src).toContain('zoneLookupErrors'); + const connectSrc = readFileSync( + join(dirname(fileURLToPath(import.meta.url)), 'connect-domains.ts'), + 'utf8' + ); + expect(connectSrc).toContain('cfErrorSummary(errors)'); + }); + + it('never stringifies a raw cfApi errors body into the console', () => { + // The Pages-binding and connect-domains warns once printed + // JSON.stringify(res.errors), which can echo account/project identifiers — + // both must go through failureDetail, which reads only the allowlisted + // code+message pairs (and lets the status decide whether a scope is named). + const connectSrc = readFileSync( + join(dirname(fileURLToPath(import.meta.url)), 'connect-domains.ts'), + 'utf8' + ); + for (const source of [src, connectSrc]) { + expect(source).not.toMatch(/JSON\.stringify\(res\.errors/); + } + expect(src).toContain('failureDetail(res, PAGES_SCOPE_HINT)'); + }); + + it('stops rather than wire an empty D1 database_id', () => { + // A failed `wrangler d1 create` plus an empty paste used to flow on and + // print ✔ lines for bindings pointing at no database. + expect(src).toMatch(/if \(!dbId\) \{[\s\S]*?process\.exitCode = 1;/); + }); + + it('reports secret puts from their results, not from what it tried to write', () => { + expect(src).toMatch(/const setupTokenSet = putSecret\('SETUP_TOKEN'/); + expect(src).toMatch(/const cronSecretSet = putSecret\('CRON_SECRET'/); + expect(src).toContain('telegramSummaryLine(Boolean(telegramBotToken), telegramTokenSet)'); + // The seed's own result, from the execute's exit — not a literal true. + expect(src).toContain('provisioningNoteLine(cronSecretSet, seedOk)'); + }); + + // Shorthand or nothing: `uploadThingTokenSet: true` satisfies a bare + // toContain of the name, and that literal is exactly the over-claim these + // helpers exist to prevent. + it('passes the storage summary the real results as shorthand', () => { + const storageCall = src.match(/storageSummaryLines\(\{[\s\S]*?\}\)/)?.[0] ?? ''; + expect(storageCall).not.toBe(''); + for (const prop of ['bucketReady', 'uploadThingTokenSet', 'r2Missing']) { + expect(storageCall).toMatch(new RegExp(`\\b${prop},`)); + } + }); + + // The R2 "(set up)" claim: derived from the create's own outcome, never from + // the absence of the not-enabled error text. + it('derives bucketReady from the bucket create outcome', () => { + expect(src).toMatch(/const bucketReady = bucketCreateSucceeded\(r2Out, r2CreateOk\)/); + }); + + // Same shape as the securitySummaryLines pin: hardcoding setupTokenSet: true + // here prints the token as if the wizard would take it, and survived the suite. + it('passes the setup-token block the real put result as shorthand', () => { + const tokenCall = src.match(/setupTokenLines\(\{[\s\S]*?\}\)/)?.[0] ?? ''; + expect(tokenCall).not.toBe(''); + expect(tokenCall).toMatch(/\bsetupTokenSet[,}]/); + }); + + it('no middot scope names remain anywhere an operator sees one', () => { + // The arrow form (Account → Turnstile: Edit) is the ruling; this guard fails + // on any middot (·) reintroduced in the CLIs or the operator docs. Comments + // are in scope too now that they use the arrow form, so the whole file is + // scanned — connect-domains-lib's skip glyph is the one allowance. + const here = dirname(fileURLToPath(import.meta.url)); + + // The resolved constants themselves, not just their use sites. + expect(WAF_SCOPE_HINT).not.toContain('·'); + expect(TURNSTILE_SCOPE_HINT).not.toContain('·'); + + for (const file of [ + 'setup.ts', + 'setup-lib.ts', + 'connect-domains.ts', + 'connect-domains-lib.ts', + 'apply-download-ratelimit.ts', + 'waf-lib.ts', + 'turnstile-lib.ts' + ]) { + const source = readFileSync(join(here, file), 'utf8').replaceAll("skip: '·'", ''); + expect(source, `${file}`).not.toContain('·'); + } + + // Positive canaries so the scan can't pass on a file that lost its recipe. + expect(readFileSync(join(here, 'setup.ts'), 'utf8')).toContain('Zone → Zone: Read'); + expect(readFileSync(join(here, 'apply-download-ratelimit.ts'), 'utf8')).toContain( + 'Zone → WAF: Edit' + ); + + // README and UPDATING.md: every line outside code fences. + for (const doc of ['README.md', 'UPDATING.md']) { + const text = readFileSync(join(here, '..', doc), 'utf8'); + let inFence = false; + const prose = text + .split('\n') + .filter((l) => { + if (/^\s*```/.test(l)) { + inFence = !inFence; + return false; + } + return !inFence; + }) + .join('\n'); + // Positive canaries so an empty or mis-parsed doc can't pass vacuously. + if (doc === 'README.md') expect(prose).toContain('| Scope | Why |'); + if (doc === 'UPDATING.md') expect(prose).toContain('Zone → WAF: Edit'); + expect(prose, `${doc} prose`).not.toContain('·'); + } + }); + + it('the token recipe names Turnstile’s scope via the shared constant', () => { + expect(src).toMatch(/\$\{TURNSTILE_SCOPE_HINT\}/); }); it('assigns pagesConfigOk from the Pages PATCH result, read-back confirmed', () => { - expect(src).toMatch(/pagesConfigOk =\s*\n?\s*res\.ok/); expect(src).toMatch(/pagesPatchConfirmsSitekey\(res\.result, turnstileSitekey\)/); }); +}); - it('putSecret reports failure instead of swallowing it', () => { - expect(src).toMatch(/catch\s*\{\s*\n?\s*return false/); +describe('cfErrorSummary', () => { + it('prints each error as code: message, joined', () => { + expect( + cfErrorSummary([ + { code: 8000000, message: 'An unknown error occurred' }, + { code: 10000, message: 'Authentication error' } + ]) + ).toBe('8000000: An unknown error occurred; 10000: Authentication error'); + }); + + it('redacts everything but code + message (no stringified bodies)', () => { + const summary = cfErrorSummary([ + { code: 10000, message: 'Authentication error', detail: { account_id: 'acct-id-must-not-leak' } } + ]); + expect(summary).toBe('10000: Authentication error'); + expect(summary).not.toContain('acct-id-must-not-leak'); + }); + + it('handles messages without a numeric code', () => { + expect(cfErrorSummary([{ message: 'plain message' }])).toBe('plain message'); + }); + + it('yields empty for undefined, non-arrays, and junk entries', () => { + expect(cfErrorSummary(undefined)).toBe(''); + expect(cfErrorSummary('a string body')).toBe(''); + expect(cfErrorSummary({ message: 'not an array' })).toBe(''); + expect(cfErrorSummary([null, 'junk', {}])).toBe(''); + }); + + it('drops code-only and empty-message entries (no dangling "10000: ")', () => { + expect(cfErrorSummary([{ code: 10000 }])).toBe(''); + expect(cfErrorSummary([{ code: 10000, message: '' }])).toBe(''); + expect(cfErrorSummary([{ code: 10000 }, { code: 7003, message: 'kept' }])).toBe('7003: kept'); + }); + + it('collapses whitespace so a multi-line message stays one printable line', () => { + expect(cfErrorSummary([{ code: 7003, message: 'line one\n\t line two' }])).toBe( + '7003: line one line two' + ); + }); + + it('caps an over-long message so pasteable output stays readable', () => { + const long = 'x'.repeat(500); + const summary = cfErrorSummary([{ code: 7003, message: long }]); + expect(summary.length).toBeLessThan(230); + expect(summary).toContain('7003: '); + expect(summary.endsWith('…')).toBe(true); + }); + + it('scrubs path-shaped object ids out of the message (code 7003 echoes them)', () => { + const zoneId = 'a'.repeat(32); + const summary = cfErrorSummary([ + { + code: 7003, + message: `Could not route to /zones/${zoneId}/rulesets/phases/http_ratelimit/entrypoint, perhaps your object identifier is invalid?` + } + ]); + expect(summary).not.toContain(zoneId); + expect(summary).toContain('/zones//rulesets'); + expect(summary).toContain('7003: Could not route'); + }); + + it('scrubs ANY 32-hex path segment: uppercase, nested, and boundary-punctuated', () => { + const upper = 'ABCDEF0123456789ABCDEF0123456789'; + const zid = 'b'.repeat(32); + const rid = 'c'.repeat(32); + const ruleId = 'd'.repeat(32); + const summary = cfErrorSummary([ + { code: 1, message: `bad account /accounts/${upper}.` }, + { code: 2, message: `no rule at /zones/${zid}/rulesets/${rid}/rules/${ruleId}, sorry` } + ]); + for (const id of [upper, zid, rid, ruleId]) expect(summary).not.toContain(id); + // Ids straddling '.', ',', '/', and end-of-string all scrub. + expect(summary).toContain('1: bad account /accounts/.'); + expect(summary).toContain('2: no rule at /zones//rulesets//rules/, sorry'); + }); + + it('scrubs ids that are not path segments: assigned, quoted, parenthesized', () => { + const acct = 'e'.repeat(32); + const zid = 'f'.repeat(32); + const rid = '0'.repeat(32); + const summary = cfErrorSummary([ + { code: 1, message: `account_id=${acct} is not authorized` }, + { code: 2, message: `zone "${zid}" not found` }, + { code: 3, message: `ruleset (${rid}) is missing` } + ]); + for (const id of [acct, zid, rid]) expect(summary).not.toContain(id); + expect(summary).toContain('1: account_id= is not authorized'); + expect(summary).toContain('2: zone "" not found'); + expect(summary).toContain('3: ruleset () is missing'); + }); + + // A zero-width joiner inside an id used to become a space, splitting the run + // into two halves that no longer matched the 32-hex pattern — so the id + // printed. Deleting format chars keeps the run intact and the scrub working. + it('scrubs an id even when a zero-width char sits inside it', () => { + const zid = 'a'.repeat(32); + const split = `${zid.slice(0, 10)}\u200b${zid.slice(10)}`; + const summary = cfErrorSummary([{ code: 1, message: `zone ${split} not found` }]); + expect(summary).toBe('1: zone not found'); + }); + + // \b treats '_' as a word char, so an id butted against an underscore never + // hit a word boundary and slipped through unscrubbed. + it('scrubs an id sitting next to an underscore', () => { + const zid = 'b'.repeat(32); + const summary = cfErrorSummary([{ code: 1, message: `key zone_${zid}_v2 rejected` }]); + expect(summary).not.toContain(zid); + expect(summary).toBe('1: key zone__v2 rejected'); + }); + + it('leaves shorter hex runs alone (only a full 32-hex id is an object id)', () => { + const summary = cfErrorSummary([ + { code: 1, message: 'checksum abcdef01 and prefix abcdef0123456789 are fine' } + ]); + expect(summary).toBe('1: checksum abcdef01 and prefix abcdef0123456789 are fine'); + }); + + it('strips control/format chars (ANSI escapes) before printing', () => { + const summary = cfErrorSummary([{ code: 7003, message: '\u001b[31mred\u001b[0m\u200b alert' }]); + expect(summary).not.toContain('\u001b'); + expect(summary).not.toContain('\u200b'); + expect(summary).toContain('alert'); + }); + + it('caps by code point so an emoji at the boundary is never split', () => { + // 199 chars + two emoji = 201 code points: over the 200 cap by one. + const summary = cfErrorSummary([{ code: 7003, message: 'x'.repeat(199) + '🎉🎉' }]); + expect(summary.endsWith('…')).toBe(true); + expect(summary).not.toContain('�'); + // The kept 200 points end with the first emoji, whole. + expect(Array.from(summary).length).toBe(Array.from('7003: ').length + 201); + expect(summary).toContain('🎉'); + }); + + it('caps the JOINED summary so many errors cannot yield a multi-KB line', () => { + const many = Array.from({ length: 10 }, (_, i) => ({ code: 1000 + i, message: 'y'.repeat(50) })); + const summary = cfErrorSummary(many); + expect(Array.from(summary).length).toBeLessThanOrEqual(301); + expect(summary.endsWith('…')).toBe(true); + expect(summary).toContain('1000: '); + }); + + it('the joined cap also counts code points (emoji at the join boundary stays whole)', () => { + // Two entries land the join at exactly 299 points (each under the + // per-message cap), so the third entry's emoji straddle the 300 + // boundary: the cap must drop or keep each one whole. + const summary = cfErrorSummary([ + { code: 1000, message: 'y'.repeat(141) }, // line: 147 points + { code: 1001, message: 'y'.repeat(142) }, // +2 +148 = 297, +2 = 299 + { message: '🎉🎉🎉' } // 300..302 — over the cap mid-emoji-run + ]); + // The kept 300th point is the first emoji, whole — a UTF-16 slice would + // cut it into a lone surrogate and fail both of these. + expect(summary.endsWith('🎉…')).toBe(true); + expect(summary.isWellFormed()).toBe(true); + expect(summary).not.toContain('\uFFFD'); + expect(Array.from(summary).length).toBe(301); }); }); diff --git a/scripts/setup-lib.ts b/scripts/setup-lib.ts index b72bd3bc..6d27b49a 100644 --- a/scripts/setup-lib.ts +++ b/scripts/setup-lib.ts @@ -96,6 +96,19 @@ export function isR2NotEnabled(output: string): boolean { return /\b10042\b/.test(output) || /enable R2/i.test(output) || /must (be|first be) enabled/i.test(output); } +/** + * True when a `wrangler r2 bucket create` run left the bucket in place: a clean + * exit, or a failure that only means the bucket was already there (error 10004 — + * the re-run case, which exits non-zero but is a success for our purposes). + * + * Every other failure means NO bucket: R2 not enabled, or — the case that + * prompted this — a token missing Account → Workers R2 Storage: Edit, which + * carries none of isR2NotEnabled's markers and so used to read as success. + */ +export function bucketCreateSucceeded(output: string, commandOk: boolean): boolean { + return commandOk || /\b10004\b/.test(output) || /already exists/i.test(output); +} + /** * Ensures a URL carries an http(s) scheme, mirroring `sanitizeUrl` in * src/lib/server/validate.ts: a value with no `http://`/`https://` prefix is @@ -252,7 +265,7 @@ export function dnsProbeBlocksSetup(probe: { ok: boolean; status: number }): boo * Classifies the Image Transformations preflight outcome from the zone-setting * GET and (when it was off) the enabling PATCH's success: `true` = on (already * on, or PATCHed on), `false` = still off (PATCH failed), `null` = unknown (the - * GET failed, e.g. the token lacks Zone Settings·Read). `patchOk` is ignored + * GET failed, e.g. the token lacks Zone → Zone Settings: Read). `patchOk` is ignored * unless the GET succeeded and reported the setting as off. */ export function imageResizingOutcome( @@ -315,6 +328,130 @@ export async function cfApi( } } +/** + * True for a non-null object — the guard every API-body walk needs before it + * reads a property, since `typeof null === 'object'`. Shared so the four copies + * that guarded Cloudflare list entries can't drift apart. + */ +export function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +// Cap by code point, not UTF-16 unit, so an emoji at the boundary is dropped +// whole instead of split into a lone surrogate. +const capPoints = (value: string, max: number): string => { + const points = Array.from(value); + return points.length > max ? `${points.slice(0, max).join('')}…` : value; +}; + +/** + * One printable line naming a cfApi failure. Only the code + message fields + * are read (never the raw `errors` value JSON.stringified into + * operator-pasteable output), and any standalone 32-hex run in the message — + * case-insensitive, whatever delimits it: a path segment, `account_id=…`, + * quoted, or parenthesized — is scrubbed to ``: Cloudflare's own text can + * echo object ids (code 7003 quotes the request path, zone id included). A + * longer hex run is left alone, since it isn't an object id. The message + * content is otherwise printed verbatim. Anything + * that isn't the documented { code, message } array shape yields '' (the + * caller prints just the HTTP status). + */ +export function cfErrorSummary(errors: unknown): string { + if (!Array.isArray(errors)) return ''; + const joined = errors + .filter(isRecord) + .map((e) => { + // No message, no line — a bare code would print a dangling '10000: '. + // Strip control/format chars (ANSI escapes, zero-widths), collapse + // whitespace, and cap each message: bodies can be multi-line or + // arbitrarily long, and this lands in operator-pasteable output. + // Format chars are DELETED, not spaced: a zero-width inside an id would + // otherwise split it into two halves that no longer match the 32-hex run, + // and the id would print. Control chars still become a space — those do + // separate words. The id boundaries are alphanumeric lookarounds rather + // than \b, so an id butted against an underscore still scrubs (\b treats + // _ as a word char, so `zone_` slipped through). + const raw = + typeof e.message === 'string' + ? e.message + .replace(/\p{Cf}/gu, '') + .replace(/\p{Cc}/gu, ' ') + .replace(/\s+/g, ' ') + .trim() + .replace(/(?') + : ''; + if (!raw) return ''; + const message = capPoints(raw, 200); + return typeof e.code === 'number' ? `${e.code}: ${message}` : message; + }) + .filter(Boolean) + .join('; '); + // Cap the whole summary too, so many errors can't yield a multi-KB line. + return capPoints(joined, 300); +} + +/** + * Failure tail for a failed cfApi step, appended to an error detail so it + * carries an honest reason instead of a bare status: + * - status 0 → cfApi's thrown-fetch marker; the API was never reached. + * - 2xx → the body said success:false (cfApi maps that to ok=false), + * so repeat the API's own code+message summary. + * - 401/403 → the caller's scope hint, then '; the API said …' when + * the body gave a reason (the attribution keeps our advice + * and the API's own words separable). + * - anything else (500 etc.) → '; the API said …' when the body gave a + * reason, else bare; never a scope hint — that misdirects. + * Shared by waf-lib and turnstile-lib, which each bind their own scope hint — + * the two private copies drifted apart once already. + */ +export function cfFailureTail(status: number, errors: unknown, scopeHint: string): string { + if (status === 0) return '; the Cloudflare API did not respond'; + const why = cfErrorSummary(errors); + if (status >= 200 && status < 300) { + return `; the API reported failure${why ? ` (${why})` : ' with no reason given'}`; + } + const hint = status === 401 || status === 403 ? `; token needs ${scopeHint}` : ''; + return `${hint}${why ? `; the API said ${why}` : ''}`; +} + +/** + * The "(HTTP )" fragment of an error detail — empty for status 0, where + * cfFailureTail already says the API did not respond and a bare "(HTTP 0)" + * is noise. + */ +export function statusLabel(status: number): string { + return status === 0 ? '' : ` (HTTP ${status})`; +} + +/** + * The whole "why it failed" suffix for a failed cfApi call: the status label + * followed by the failure tail. The two are always printed together, so this is + * the single form every caller appends to its own sentence. + */ +export function failureDetail(res: { status: number; errors?: unknown }, scopeHint: string): string { + return `${statusLabel(res.status)}${cfFailureTail(res.status, res.errors, scopeHint)}`; +} + +/** The scope a zone lookup needs — named only when the status proves it (401/403). */ +export const ZONE_READ_SCOPE_HINT = 'Zone → Zone: Read'; + +/** + * The warn setup prints when the custom-domain zone lookup fails, as lines. + * cfFailureTail decides what the reason is per status, so a 400/404/500 that + * carried a message repeats it (the inline version gated that on 2xx and threw + * it away for exactly the statuses an operator can least explain), a thrown + * fetch says the API did not respond instead of "HTTP 0", and Zone → Zone: Read + * is named only on a 401/403 rather than guessed at for every failure. + * `name` is the candidate whose lookup failed — for a subdomain host that can be + * the parent zone, and pointing at the host would mislead. + */ +export function zoneLookupWarnLines(name: string, status: number, errors: unknown): string[] { + return [ + `\n⚠ Zone lookup failed for ${name}${failureDetail({ status, errors }, ZONE_READ_SCOPE_HINT)} — skipping the DNS / image-transform preflight.`, + ' Re-run setup to retry the preflight.' + ]; +} + export interface GhEligibilityInput { /** `gh` binary is on PATH. */ ghInstalled: boolean; @@ -401,34 +538,46 @@ export function ciWiringEntries(input: CiWiringInput): CiEntry[] { * can pin that, and the wording, without running the CLI. * * Status contracts: null = not attempted (no domain / no zone / no token); - * 'error' = provisioning failed — `downloadRateLimitDetail` carries waf-lib's - * reason (missing scope, absent zone, HTTP failure), which the summary repeats - * instead of assuming a cause; 'exists' rate limits are old news and stay - * silent. + * 'error' = provisioning failed — `downloadRateLimitDetail` / `turnstileDetail` + * carry the provisioning lib's reason (missing scope, HTTP failure), which the + * summary repeats instead of assuming a cause; 'exists' rate limits are old + * news and stay silent. * * `turnstileWired` says whether BOTH halves of the wiring actually landed (the * Pages PATCH carrying TURNSTILE_SITEKEY and the TURNSTILE_SECRET put). The * login check fails open when either is missing, so a provisioned widget with * failed wiring must read as NOT protected — never as enforced. */ -export function securitySummaryLines( - host: string, - downloadRateLimit: RateLimitStatus | null, - downloadRateLimitDetail: string | null, - turnstileStatus: TurnstileStatus | null, - turnstileWired: boolean, +export interface SecuritySummaryInput { + host: string; + downloadRateLimit: RateLimitStatus | null; + downloadRateLimitDetail: string | null; + turnstileStatus: TurnstileStatus | null; + // turnstile-lib's failure reason when turnstileStatus is 'error'. + turnstileDetail: string | null; + turnstileWired: boolean; // The RESOLVED zone's name when it differs from the host (subdomain forks): // the rate-limit rule is zone-wide, so the applied line must name the zone // it actually landed on. The retry command keeps the host — the applier // resolves the zone itself. - zoneName?: string | null -): string[] { + zoneName?: string | null; +} + +export function securitySummaryLines(input: SecuritySummaryInput): string[] { + const { + host, + downloadRateLimit, + downloadRateLimitDetail, + turnstileStatus, + turnstileDetail, + turnstileWired, + zoneName + } = input; const lines: string[] = []; if (downloadRateLimit === 'error') { - lines.push( - ` • Public-endpoint rate limit: NOT set (${downloadRateLimitDetail ?? 'provisioning failed'}).` - ); - lines.push(' Fix that, then run:'); + lines.push(' • Public-endpoint rate limit: NOT set.'); + lines.push(` Reason: ${downloadRateLimitDetail ?? 'none reported'}.`); + lines.push(' When that is fixed, run:'); lines.push(` CLOUDFLARE_API_TOKEN= npm run apply-download-ratelimit -- ${host}`); } else if (downloadRateLimit && downloadRateLimit !== 'exists') { lines.push( @@ -436,8 +585,9 @@ export function securitySummaryLines( ); } if (turnstileStatus === 'error') { - lines.push(' • Admin-login bot check: NOT set (token lacks Account · Turnstile · Edit).'); - lines.push(' Add that permission to the token and re-run setup to protect /admin/login.'); + lines.push(' • Admin-login bot check: NOT set.'); + lines.push(` Reason: ${turnstileDetail ?? 'none reported'}.`); + lines.push(' When that is fixed, re-run setup to protect /admin/login.'); } else if (turnstileStatus && !turnstileWired) { // Worded as an unverified-THIS-RUN claim: on a re-run, a previous run may // have wired the project already, so "no bot check" would be false there — @@ -449,12 +599,122 @@ export function securitySummaryLines( lines.push(' first run that means /admin/login has NO bot check (it fails open without both) —'); lines.push(' re-run setup, or set the var + secret on the Pages project yourself.'); } else if (turnstileStatus) { - lines.push(` • Admin-login bot check: Turnstile ${turnstileStatus} for ${host}`); + lines.push(` • Admin-login bot check: Turnstile ${turnstileStatus} for ${host}.`); lines.push(' (TURNSTILE_SITEKEY var + TURNSTILE_SECRET secret set; enforced once deployed).'); } return lines; } +export interface StorageSummaryInput { + /** Active image store: 'r2' | 'uploadthing'. */ + provider: string; + /** The R2 bucket create said R2 isn't enabled on the account. */ + r2Missing: boolean; + /** The bucket create reported the bucket in place (created, or already there). */ + bucketReady: boolean; + /** The UPLOADTHING_TOKEN secret put succeeded (false when no token was given). */ + uploadThingTokenSet: boolean; + bucket: string; + project: string; +} + +/** + * The end-of-run "Storage backend:" lines. Both backends report NOT READY on the + * state we actually established: R2 when the bucket create didn't put a bucket + * there, UploadThing when the UPLOADTHING_TOKEN put didn't land (a skipped or + * failed put used to still print "(set up)", which is the same over-claim + * turnstileWired exists to prevent — the operator deploys and every upload fails). + * + * "R2 is not enabled" is only ONE way the create fails. A token without Account → + * Workers R2 Storage: Edit fails with none of that error's markers, so the claim + * keys off the create's own outcome and the not-enabled text only picks the + * wording — otherwise setup says "(set up)" about a bucket that doesn't exist. + */ +export function storageSummaryLines(input: StorageSummaryInput): string[] { + const { provider, r2Missing, bucketReady, uploadThingTokenSet, bucket, project } = input; + if (provider === 'r2') { + if (bucketReady) return ['Storage backend: Cloudflare R2 (set up).']; + if (r2Missing) + return [ + 'Storage backend: Cloudflare R2 — NOT READY (R2 is not enabled on this account).', + ` Create the bucket, then re-run setup: npx wrangler r2 bucket create ${bucket}` + ]; + return [ + `Storage backend: Cloudflare R2 — NOT READY (the ${bucket} bucket was not created).`, + ` Create it, then re-run setup: npx wrangler r2 bucket create ${bucket}` + ]; + } + if (uploadThingTokenSet) return ['Storage backend: UploadThing (set up).']; + return [ + 'Storage backend: UploadThing — NOT READY (the UPLOADTHING_TOKEN secret is not set).', + ` Set it, then re-deploy: npx wrangler pages secret put UPLOADTHING_TOKEN --project-name ${project}` + ]; +} + +/** + * Names the Resend secrets whose put failed. These are optional, so silence is + * right when the operator supplied none — but a value they DID supply that + * failed to land must be said out loud: password-reset email reads these at + * runtime, so the failure would otherwise surface as a dead reset link long + * after setup finished. + */ +export function resendSecretWarnLines(failed: string[], project: string): string[] { + if (failed.length === 0) return []; + return [ + `\n⚠ Resend secrets that did NOT get set: ${failed.join(', ')}.`, + ' Password-reset email stays off until they are:', + ...failed.map((name) => ` npx wrangler pages secret put ${name} --project-name ${project}`) + ]; +} + +/** + * The end-of-run "Telegram sticker import:" line. `enabled (bot token set)` is a + * claim about a secret put, so it needs the put's result — a failed put leaves + * Telegram import hidden, and saying "enabled" would send the operator hunting in + * the app for a feature that never turned on. + */ +export function telegramSummaryLine(tokenProvided: boolean, tokenSet: boolean): string { + if (!tokenProvided) return 'Telegram sticker import: not configured.'; + return tokenSet + ? 'Telegram sticker import: enabled (bot token set).' + : 'Telegram sticker import: enabled, but the bot token did NOT get set — import stays hidden.'; +} + +/** + * The end-of-run block that hands the operator their one-time SETUP_TOKEN. The + * wizard authenticates against the SETUP_TOKEN secret on the Pages project, so + * when that put failed the printed value is not yet a working token — say that + * and give the command, rather than printing it as if the wizard would take it. + */ +export function setupTokenLines(input: { setupToken: string; setupTokenSet: boolean; project: string }): string[] { + const { setupToken, setupTokenSet, project } = input; + if (setupTokenSet) { + return ['\n Your one-time setup token (enter it in the wizard):\n', ` SETUP_TOKEN = ${setupToken}`]; + } + return [ + '\n ⚠ The SETUP_TOKEN secret did NOT get set on the Pages project, so the wizard will', + ' reject this token until you set it yourself:', + ` npx wrangler pages secret put SETUP_TOKEN --project-name ${project}`, + ` SETUP_TOKEN = ${setupToken}` + ]; +} + +/** + * The closing parenthetical of the summary, which used to assert both halves + * unconditionally. Each is a write that can fail: without CRON_SECRET the + * scheduled syncs can't authenticate, and without the seed the app boots with no + * storage backend — so each is reported from its own result. + */ +export function provisioningNoteLine(cronSecretSet: boolean, seedOk: boolean): string { + const cron = cronSecretSet + ? 'CRON_SECRET set for the cron jobs' + : 'CRON_SECRET NOT set — the cron jobs cannot authenticate'; + const seed = seedOk + ? 'storageProvider seeded' + : 'storageProvider NOT seeded — set it in admin Settings'; + return ` (${cron}; ${seed}.)`; +} + /** * True when a Pages-project PATCH response confirms TURNSTILE_SITEKEY persisted * with the value we sent. The PATCH returns the updated project; a 200 whose diff --git a/scripts/setup.ts b/scripts/setup.ts index 4d420804..ba7b7f06 100644 --- a/scripts/setup.ts +++ b/scripts/setup.ts @@ -27,6 +27,7 @@ import { buildMigrationSql, buildSeedSql, sanitizeProjectName, + bucketCreateSucceeded, isR2NotEnabled, ensureUrlScheme, ghSecretEligibility, @@ -41,13 +42,24 @@ import { imageResizingIsOn, ciWiringEntries, cfApi, + failureDetail, + zoneLookupWarnLines, securitySummaryLines, + storageSummaryLines, + telegramSummaryLine, + resendSecretWarnLines, + setupTokenLines, + provisioningNoteLine, pagesPatchConfirmsSitekey, cdnAttachmentLines } from './setup-lib.ts'; import { resolveZone } from './connect-domains-lib.ts'; import { applyDownloadRateLimit, type RateLimitStatus } from './waf-lib.ts'; -import { provisionTurnstileWidget, type TurnstileStatus } from './turnstile-lib.ts'; +import { + provisionTurnstileWidget, + SCOPE_HINT as TURNSTILE_SCOPE_HINT, + type TurnstileStatus +} from './turnstile-lib.ts'; // Shared with the admin Settings save so the seeded siteUrl passes the same // https-URL validation (validate.ts has no imports, so tsx loads it directly). import { normalizeHttpsUrl } from '../src/lib/server/validate.ts'; @@ -68,6 +80,8 @@ type RunOpts = { allowFail?: boolean; stdin?: 'inherit' | 'ignore'; env?: NodeJS.ProcessEnv; + /** Called on a tolerated failure, so a caller can record that it happened. */ + onFail?: () => void; }; function run(cmd: string, opts: RunOpts = {}): string { console.log(`\n$ ${cmd}`); @@ -81,7 +95,10 @@ function run(cmd: string, opts: RunOpts = {}): string { if (opts.allowFail) { // On a tolerated failure, hand back whatever the command printed so callers // can sniff it (e.g. the R2 "not enabled" error). Inherited stdio isn't - // captured, so this is only non-empty when `capture` was set. + // captured, so this is only non-empty when `capture` was set — which is why + // a caller that must know whether the command SUCCEEDED takes onFail rather + // than reading the text. + opts.onFail?.(); const e = err as { stdout?: string | Buffer; stderr?: string | Buffer }; return `${e.stdout ?? ''}${e.stderr ?? ''}`; } @@ -121,18 +138,23 @@ function ghSet(kind: 'secret' | 'variable', name: string, value: string, repo: s const token = (bytes = 32) => randomBytes(bytes).toString('hex'); +// The scope the Pages-project PATCH needs, named in a failure only when the +// status (401/403) proves that is the reason. +const PAGES_SCOPE_HINT = 'Account → Cloudflare Pages: Edit'; + // The friend-facing API-token recipe, printed whenever a scope preflight fails so // the operator knows exactly what to (re)create. Kept in one place so the CLI and // the message stay in sync with README's "API token" section. const TOKEN_RECIPE = 'Create a Cloudflare API token (dash → My Profile → API Tokens → Create Token → Custom token) with:\n' + - ' • Account · Cloudflare Pages · Edit\n' + - ' • Account · D1 · Edit\n' + - ' • Account · Workers R2 Storage · Edit\n' + - ' • Account · Turnstile · Edit (only with a custom domain; adds the admin-login bot check)\n' + - ' • Zone · DNS · Edit (only if you are attaching a custom domain)\n' + - ' • Zone · WAF · Edit (only with a custom domain; adds the public rate limit)\n' + - ' • Zone · Zone Settings · Edit (optional; lets setup enable image resizing for you)'; + ` • ${PAGES_SCOPE_HINT}\n` + + ' • Account → D1: Edit\n' + + ' • Account → Workers R2 Storage: Edit\n' + + ` • ${TURNSTILE_SCOPE_HINT} (only with a custom domain; adds the admin-login bot check)\n` + + ' • Zone → Zone: Read (only with a custom domain; resolves the zone)\n' + + ' • Zone → DNS: Edit (only with a custom domain; needed later to attach the apex record)\n' + + ' • Zone → WAF: Edit (only with a custom domain; adds the public rate limit)\n' + + ' • Zone → Zone Settings: Edit (optional; lets setup enable image resizing for you)'; async function main() { console.log('— Sona setup —\n'); @@ -302,7 +324,7 @@ async function main() { // 0. Custom-domain preflight (only when a domain was given). Checks (does not // guarantee) DNS access for the zone — the Pages apex CNAME needs - // Zone·DNS·Edit, and without it the domain sticks `pending` with a confusing + // Zone → DNS: Edit, and without it the domain sticks `pending` with a confusing // 522 — and, while we have the zone, checks/enables Image Transformations // (thumbnails/OG images are built via /cdn-cgi/image, which is off by default // and per-zone). Runs before provisioning so a missing DNS scope surfaces @@ -322,8 +344,14 @@ async function main() { // domain — a *.pages.dev-only fork isn't provisioned one. Its sitekey (public) // is set as a Pages var below and its secret as a Pages secret; the login page // enforces the challenge only when BOTH are present. null = not attempted - // (no domain / no token); 'error' = token lacked Account · Turnstile · Edit. + // (no domain / no token); 'error' = provisioning failed — turnstileDetail + // carries the actual reason (scope on 401/403, otherwise the HTTP status + // or a partial body). let turnstileStatus: TurnstileStatus | null = null; + // The human-readable reason behind a Turnstile 'error' — mirrors + // downloadRateLimitDetail so the summary repeats turnstile-lib's real + // failure instead of assuming a missing scope. + let turnstileDetail: string | null = null; let turnstileSitekey = ''; let turnstileSecret = ''; // Whether the Pages-project PATCH (which carries TURNSTILE_SITEKEY) landed — @@ -344,6 +372,7 @@ async function main() { zone: preflightZone, zoneName: preflightZoneName, errorStatus: zoneLookupError, + errors: zoneLookupErrors, failedName: zoneLookupFailedName } = await resolveZone(zoneNameCandidates(host), (name) => cfApi(cfToken, `/zones?name=${encodeURIComponent(name)}`) @@ -351,18 +380,15 @@ async function main() { resolvedZoneName = preflightZoneName; const zoneId = preflightZone.id; if (zoneLookupError !== null) { - // status 0 = fetch threw (no network); a 2xx here means the API said - // success:false — neither reads sensibly as a bare "HTTP ". - const why = - zoneLookupError === 0 - ? 'could not reach the Cloudflare API' - : `Cloudflare API error, HTTP ${zoneLookupError}`; - console.warn( - `\n⚠ Zone lookup failed for ${zoneLookupFailedName ?? host} (${why}) — skipping the DNS / image-transform preflight.` - ); - console.warn( - ' A 401/403 means the token lacks Zone · Zone · Read; otherwise re-run setup to retry.' - ); + // The reason (and whether a scope is named at all) is decided by the + // status, in the shared helper — see zoneLookupWarnLines. + for (const line of zoneLookupWarnLines( + zoneLookupFailedName ?? host, + zoneLookupError, + zoneLookupErrors + )) { + console.warn(line); + } } else if (!zoneId) { console.warn( `\n⚠ No Cloudflare zone found for ${host} — skipping the DNS / image-transform preflight.` @@ -376,7 +402,7 @@ async function main() { const dnsProbe = await cfApi(cfToken, `/zones/${zoneId}/dns_records?per_page=1`); if (dnsProbeBlocksSetup(dnsProbe)) { console.warn( - `\n⚠ Could not verify DNS access for ${host} (token lacks Zone · DNS · Read; attaching the apex CNAME later needs Zone · DNS · Edit).` + `\n⚠ Could not verify DNS access for ${host} (token lacks Zone → DNS: Read; attaching the apex CNAME later needs Zone → DNS: Edit).` ); console.warn(' Setup only checks access — it never writes DNS itself. If you plan to attach'); console.warn(' the domain from the Cloudflare dashboard, you can continue.'); @@ -392,7 +418,7 @@ async function main() { } } // Image Transformations. Off by default, per-zone, and NOT grantable by - // the deploy token — enable it if the token carries Zone Settings·Edit, + // the deploy token — enable it if the token carries Zone → Zone Settings: Edit, // else leave imageResizingOn=null (unknown) and warn in Next steps. const ir = await cfApi(cfToken, `/zones/${zoneId}/settings/image_resizing`); let patchOk = false; @@ -407,7 +433,7 @@ async function main() { // WAF rate limit for the anonymously-reachable /api paths (download // beacon + oEmbed provider — one rule, Free-plan cap). Non-fatal: - // a token without Zone · WAF · Edit just yields an 'error' + // a token without Zone → WAF: Edit just yields an 'error' // result we warn about in Next steps — setup keeps going regardless. // Reuse the zone the preflight just resolved — no second candidate walk. const rateLimit = await applyDownloadRateLimit(cfToken, host, cfApi, zoneId); @@ -423,10 +449,11 @@ async function main() { // Turnstile widget for the admin-login bot check. Account- // scoped, so — unlike the DNS / image-resizing checks above — it does NOT // need a resolved zone and runs even when the domain's DNS lives elsewhere. - // Non-fatal: a token without Account · Turnstile · Edit just yields an + // Non-fatal: a token without Account → Turnstile: Edit just yields an // 'error' result we warn about in Next steps — setup keeps going regardless. const ts = await provisionTurnstileWidget(cfToken, cfAccount, host); turnstileStatus = ts.status; + turnstileDetail = ts.detail; turnstileSitekey = ts.sitekey ?? ''; turnstileSecret = ts.secret ?? ''; if (ts.status === 'error') { @@ -439,7 +466,7 @@ async function main() { '\n⚠ A custom domain was given but CLOUDFLARE_API_TOKEN/ACCOUNT_ID are not in the env,' ); console.warn(' so setup cannot preflight DNS access. Attaching the apex domain needs a token'); - console.warn(' with Zone · DNS · Edit (see README → custom domain).'); + console.warn(' with Zone → DNS: Edit (see README → custom domain).'); } } @@ -451,19 +478,47 @@ async function main() { process.stdout.write(d1Out); let dbId = parseDatabaseId(d1Out); if (!dbId) dbId = await ask('Could not auto-detect database_id — paste it from the output above', ''); + // Still nothing: `wrangler d1 create` failed and no id was pasted, so we never + // established that a database exists. Writing wrangler.toml and PATCHing the + // Pages project with an empty database_id would print two ✔ lines for bindings + // that point at nothing, and only blow up later in the migration step. + if (!dbId) { + console.error('\n✖ No D1 database_id — `wrangler d1 create` did not report one and none was pasted.'); + console.error(' Setup stopped rather than wire wrangler.toml and the Pages project to no database.'); + console.error(' Check the wrangler output above, then re-run setup.'); + process.exitCode = 1; + rl.close(); + return; + } // 3. R2 bucket — always create it so the IMAGES binding is valid. Detect the // "R2 not enabled on this account" case (error 10042) rather than swallowing // it as success and later claiming the R2 backend is set up. - const r2Out = run(`npx wrangler r2 bucket create ${bucket}`, { capture: true, allowFail: true }); + let r2CreateOk = true; + const r2Out = run(`npx wrangler r2 bucket create ${bucket}`, { + capture: true, + allowFail: true, + onFail: () => { + r2CreateOk = false; + } + }); process.stdout.write(r2Out); const r2Missing = isR2NotEnabled(r2Out); + // Whether a bucket is actually there, from the create's own outcome — not from + // the absence of one particular error string. An "already exists" failure on a + // re-run counts as ready; a permission failure does not. + const bucketReady = bucketCreateSucceeded(r2Out, r2CreateOk); if (r2Missing) { console.warn('\n⚠ R2 does not appear to be enabled on this Cloudflare account.'); console.warn(' Enable it at dash.cloudflare.com → R2, then re-run setup'); console.warn(` (or run: npx wrangler r2 bucket create ${bucket}).`); if (useR2) console.warn(` Image uploads will NOT work until the bucket "${bucket}" exists.`); + } else if (!bucketReady) { + console.warn(`\n⚠ Could not create the R2 bucket "${bucket}" — see the wrangler output above.`); + console.warn(' A token without Account → Workers R2 Storage: Edit fails exactly here.'); + console.warn(` Create it, then re-run setup: npx wrangler r2 bucket create ${bucket}`); + if (useR2) console.warn(' Image uploads will NOT work until that bucket exists.'); } // 4. Render wrangler.toml from the template. @@ -486,7 +541,9 @@ async function main() { if (cfToken && cfAccount) { const payload = buildPagesConfigPayload({ dbId, - bucket: r2Missing ? '' : bucket, + // Bind R2 only when a bucket is actually there — binding a name the create + // never made ships a broken IMAGES binding on the first CI deploy. + bucket: bucketReady ? bucket : '', // TURNSTILE_SITEKEY is public (rendered into the login page), so it rides // as a plain Pages var alongside FURTRACK_MODE. Its secret is set separately // as a Pages secret below. Absent when Turnstile wasn't provisioned. @@ -504,12 +561,18 @@ async function main() { pagesConfigOk = res.ok && (!turnstileSitekey || pagesPatchConfirmsSitekey(res.result, turnstileSitekey)); if (res.ok) { + // Name only what was sent: the R2 binding is omitted when no bucket exists. console.log( - '✔ attached D1/R2 bindings + FURTRACK_MODE to the Pages project (CI deploys get working bindings).' + `✔ attached D1${bucketReady ? '/R2' : ''} bindings + FURTRACK_MODE to the Pages project (CI deploys get working bindings).` ); } else { - console.warn('\n⚠ Could not attach bindings to the Pages project via the API'); - console.warn(` (HTTP ${res.status}) ${JSON.stringify(res.errors ?? '')}`); + // failureDetail keeps the reason honest per status — a thrown fetch says + // the API did not respond instead of "(HTTP 0)", and only a 401/403 names + // the scope. It reads the allowlisted code+message pairs, never the raw + // errors body, which can echo account/project identifiers. + console.warn( + `\n⚠ Could not attach bindings to the Pages project via the API${failureDetail(res, PAGES_SCOPE_HINT)}` + ); console.warn(' Fix: run ONE local deploy with wrangler.toml present so the bindings attach:'); console.warn(' npx wrangler pages deploy .svelte-kit/cloudflare'); console.warn(' Until then, CI (git push) deploys will have no D1/R2 binding.'); @@ -562,10 +625,16 @@ async function main() { // Seed the FurTrack character/tag the fursuit feature queries. primaryCharacter }); - run(`npx wrangler d1 execute ${dbName} --remote --command "${seed}"`, { - allowFail: true, - stdin: 'ignore' - }); + // Tolerated failure, but not an ignored one: the summary says the provider was + // seeded, and a failed execute leaves the app with no storage backend at boot. + let seedOk = true; + try { + run(`npx wrangler d1 execute ${dbName} --remote --command "${seed}"`, { stdin: 'ignore' }); + } catch { + seedOk = false; + console.warn('\n⚠ Could not seed site_settings — the storage provider is not recorded yet.'); + console.warn(' Set it in admin Settings → Storage Provider after the first deploy.'); + } // 7. Generate + set secrets. SETUP_TOKEN gates the first-run wizard. const setupToken = token(); @@ -584,12 +653,19 @@ async function main() { return false; // allowFail } }; - putSecret('SETUP_TOKEN', setupToken); - putSecret('CRON_SECRET', cronSecret); - if (!useR2 && uploadThingToken) putSecret('UPLOADTHING_TOKEN', uploadThingToken); - if (telegramBotToken) putSecret('TELEGRAM_BOT_TOKEN', telegramBotToken); - if (resendApiKey) putSecret('RESEND_API_KEY', resendApiKey); - if (resendFrom) putSecret('RESEND_FROM', resendFrom); + // Keep every put's result: the summary below must report what actually landed, + // not what we tried to write. + const setupTokenSet = putSecret('SETUP_TOKEN', setupToken); + const cronSecretSet = putSecret('CRON_SECRET', cronSecret); + const uploadThingTokenSet = + !useR2 && uploadThingToken ? putSecret('UPLOADTHING_TOKEN', uploadThingToken) : false; + const telegramTokenSet = telegramBotToken ? putSecret('TELEGRAM_BOT_TOKEN', telegramBotToken) : false; + // Optional, but a supplied value whose put failed can't stay silent — see + // resendSecretWarnLines: the failure would otherwise show up as a dead reset + // link months later. + const resendFailed: string[] = []; + if (resendApiKey && !putSecret('RESEND_API_KEY', resendApiKey)) resendFailed.push('RESEND_API_KEY'); + if (resendFrom && !putSecret('RESEND_FROM', resendFrom)) resendFailed.push('RESEND_FROM'); // Turnstile secret for the admin-login siteverify. Server-only, so // it's a Pages secret (never a plain var); the public sitekey was set above. // The login check fails open without it, so remember whether the put landed. @@ -658,16 +734,21 @@ async function main() { rl.close(); console.log('\n──────────────────────────────────────────────'); - if (useR2 && r2Missing) { - console.log('Storage backend: Cloudflare R2 — NOT READY (R2 is not enabled on this account).'); - console.log(` Create the bucket, then re-run setup: npx wrangler r2 bucket create ${bucket}`); - } else { - console.log(`Storage backend: ${provider === 'r2' ? 'Cloudflare R2' : 'UploadThing'} (set up).`); + for (const line of storageSummaryLines({ + provider, + r2Missing, + bucketReady, + uploadThingTokenSet, + bucket, + project + })) { + console.log(line); } console.log( `Fursuit photos: ${furtrackMode === 'off' ? 'disabled' : `enabled (${furtrackMode})`}${primaryCharacter ? ` — character "${primaryCharacter}"` : ''}.` ); - console.log(`Telegram sticker import: ${telegramBotToken ? 'enabled (bot token set)' : 'not configured'}.`); + console.log(telegramSummaryLine(Boolean(telegramBotToken), telegramTokenSet)); + for (const line of resendSecretWarnLines(resendFailed, project)) console.warn(line); console.log('Migrations applied and recorded in schema_migrations (first CI deploy is a no-op).'); console.log('\nNext steps:\n'); console.log(' 1. Deploy: git push (or `npx wrangler pages deploy .svelte-kit/cloudflare`)'); @@ -701,26 +782,28 @@ async function main() { console.log(' Until on, gallery thumbnails serve the full-size original (slow) or 404.'); } // Zone security: rate limit + admin-login Turnstile. - for (const line of securitySummaryLines( + for (const line of securitySummaryLines({ host, downloadRateLimit, downloadRateLimitDetail, turnstileStatus, - pagesConfigOk && turnstileSecretSet, - resolvedZoneName - )) { + turnstileDetail, + turnstileWired: pagesConfigOk && turnstileSecretSet, + zoneName: resolvedZoneName + })) { console.log(line); } } - console.log('\n Your one-time setup token (enter it in the wizard):\n'); - console.log(` SETUP_TOKEN = ${setupToken}`); + for (const line of setupTokenLines({ setupToken, setupTokenSet, project })) { + console.log(line); + } if (ciSecretsSet) { console.log('\n CI deploy secrets/variables are set — pushing to main will deploy.'); } else { console.log('\n Before deploying via GitHub, set the CI secrets/variables (see the note above).'); } console.log(' Verify bindings any time with: npx wrangler pages project list / npx wrangler d1 list'); - console.log(' (CRON_SECRET set for the cron jobs; storageProvider seeded.)'); + console.log(provisioningNoteLine(cronSecretSet, seedOk)); console.log('──────────────────────────────────────────────\n'); } diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index c41f9249..2483e62f 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -38,9 +38,21 @@ function fakeApi(routes: Record) { return { api, calls }; } -const listPath = `GET /accounts/${ACCT}/challenges/widgets?per_page=50`; +const PER_PAGE = 50; +const listPage = (page: number) => + `GET /accounts/${ACCT}/challenges/widgets?page=${page}&per_page=${PER_PAGE}&order=created_on&direction=asc`; +const listPath = listPage(1); +/** A full page of widgets belonging to nobody we care about. */ +const fullPageOfStrangers = (tag: string) => + Array.from({ length: PER_PAGE }, (_, i) => ({ + name: `stranger-${tag}-${i}`, + sitekey: `stranger-key-${tag}-${i}`, + domains: ['someone-else.example'] + })); const createPath = `POST /accounts/${ACCT}/challenges/widgets`; const getPath = `GET /accounts/${ACCT}/challenges/widgets/${SITEKEY}`; +// This fork's own widget, exactly as the list returns it. +const ourWidget = { name: WIDGET_NAME, sitekey: SITEKEY, domains: ['akito.dog'] }; describe('buildCreateBody', () => { it('encodes the stable name, the domain, and managed mode', () => { @@ -94,7 +106,7 @@ describe('provisionTurnstileWidget — reuses when present (idempotent)', () => [listPath]: { ok: true, status: 200, - result: [{ name: WIDGET_NAME, sitekey: SITEKEY, domains: ['akito.dog'] }] + result: [ourWidget] }, [getPath]: { ok: true, status: 200, result: { sitekey: SITEKEY, secret: WIDGET_SECRET } } }); @@ -108,6 +120,29 @@ describe('provisionTurnstileWidget — reuses when present (idempotent)', () => expect(get?.method).toBe('GET'); }); + // The sitekey comes back from the API, so it is encoded like any other + // untrusted path segment — one carrying a '/' would otherwise rewrite the URL. + it('encodes the sitekey it puts in the single-widget GET path', async () => { + const odd = 'key/../evil'; + const { api, calls } = fakeApi({ + [listPath]: { + ok: true, + status: 200, + result: [{ name: WIDGET_NAME, sitekey: odd, domains: ['akito.dog'] }] + }, + [`GET /accounts/${ACCT}/challenges/widgets/key%2F..%2Fevil`]: { + ok: true, + status: 200, + result: { sitekey: odd, secret: WIDGET_SECRET } + } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('exists'); + expect(calls.some((c) => c.path === `/accounts/${ACCT}/challenges/widgets/key%2F..%2Fevil`)).toBe( + true + ); + }); + // One Cloudflare account can hold several forks, and every fork's widget carries // the same stable name — so the host, not the name alone, is what identifies ours. // Reusing a sibling fork's widget would hand this fork a sitekey scoped to the @@ -138,10 +173,7 @@ describe('provisionTurnstileWidget — reuses when present (idempotent)', () => [listPath]: { ok: true, status: 200, - result: [ - { name: WIDGET_NAME, sitekey: 'sparky-key', domains: ['sparky.ink'] }, - { name: WIDGET_NAME, sitekey: SITEKEY, domains: ['akito.dog'] } - ] + result: [{ name: WIDGET_NAME, sitekey: 'sparky-key', domains: ['sparky.ink'] }, ourWidget] }, [getPath]: { ok: true, status: 200, result: { sitekey: SITEKEY, secret: WIDGET_SECRET } } }); @@ -165,12 +197,32 @@ describe('provisionTurnstileWidget — reuses when present (idempotent)', () => expect(res.sitekey).toBe(SITEKEY); }); + // An entry carrying our name AND our host is ours, whatever shape its sitekey + // came back in. Treating it as "not ours" would walk on and create a second + // widget for the same name+host, which is the duplicate the walk exists to avoid. + it('errors (no create) when our own entry carries no usable sitekey', async () => { + for (const sitekey of [undefined, '', 42]) { + const { api, calls } = fakeApi({ + [listPath]: { + ok: true, + status: 200, + result: [{ name: WIDGET_NAME, sitekey, domains: ['akito.dog'] }] + }, + [createPath]: { ok: true, status: 200, result: { sitekey: SITEKEY, secret: WIDGET_SECRET } } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status, `sitekey=${String(sitekey)}`).toBe('error'); + expect(res.detail).toContain('no usable sitekey'); + expect(calls.some((c) => c.method === 'POST')).toBe(false); + } + }); + it('errors (no mutation) when the existing widget’s secret cannot be read', async () => { const { api, calls } = fakeApi({ [listPath]: { ok: true, status: 200, - result: [{ name: WIDGET_NAME, sitekey: SITEKEY, domains: ['akito.dog'] }] + result: [ourWidget] }, // GET succeeds but returns no secret (e.g. a partial/blank body). [getPath]: { ok: true, status: 200, result: { sitekey: SITEKEY } } @@ -179,8 +231,156 @@ describe('provisionTurnstileWidget — reuses when present (idempotent)', () => expect(res.status).toBe('error'); expect(res.secret).toBeUndefined(); expect(res.detail).toContain('could not read its secret'); + // The GET was a 200 — a blank body is not a token-scope FAILURE, but the + // one actionable lead is still the token's read-vs-edit scope. + expect(res.detail).not.toContain('token needs'); + expect(res.detail).toContain('the widget came back without one'); + expect(res.detail).toContain('check that the token has'); + expect(calls.some((c) => c.method === 'POST')).toBe(false); + }); +}); + +// An account can hold more widgets than one page returns. Reading only the first +// page would miss ours and the re-run would mint a duplicate, rewiring Pages to a +// fresh sitekey/secret, so the list has to be walked page by page. +describe('provisionTurnstileWidget — walks past the first list page', () => { + it('finds and reuses our widget when it sits on page 2', async () => { + const { api, calls } = fakeApi({ + [listPage(1)]: { ok: true, status: 200, result: fullPageOfStrangers('p1') }, + [listPage(2)]: { ok: true, status: 200, result: [ourWidget] }, + [getPath]: { ok: true, status: 200, result: { sitekey: SITEKEY, secret: WIDGET_SECRET } } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('exists'); + expect(res.sitekey).toBe(SITEKEY); + expect(res.secret).toBe(WIDGET_SECRET); + // The whole point: no duplicate widget. + expect(calls.some((c) => c.method === 'POST')).toBe(false); + // Stopped at the match rather than reading on. + expect(calls.some((c) => c.path.includes('page=3'))).toBe(false); + // Offsets only mean the same thing page to page under an explicit sort. + expect(calls[0].path).toContain('order=created_on&direction=asc'); + }); + + it('creates exactly once after two full pages hold no widget of ours', async () => { + const { api, calls } = fakeApi({ + [listPage(1)]: { ok: true, status: 200, result: fullPageOfStrangers('p1') }, + [listPage(2)]: { ok: true, status: 200, result: fullPageOfStrangers('p2') }, + [listPage(3)]: { ok: true, status: 200, result: [] }, + [createPath]: { ok: true, status: 200, result: { sitekey: SITEKEY, secret: WIDGET_SECRET } } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('created'); + expect(res.sitekey).toBe(SITEKEY); + expect(calls.filter((c) => c.method === 'POST')).toHaveLength(1); + // The empty third page ended the walk. + expect(calls.filter((c) => c.method === 'GET')).toHaveLength(3); + }); + + // A body that isn't a list reads as a zero-length page, which looks exactly like + // the last page of the walk — so the old `?? []` ended the walk and created a + // second widget for a fork that already had one. + it('an ok page whose body is not a list errors instead of creating a duplicate', async () => { + const { api, calls } = fakeApi({ + [listPage(1)]: { ok: true, status: 200, result: { widgets: [ourWidget] } } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('could not list Turnstile widgets'); + expect(res.detail).toContain('HTTP 200'); + expect(res.detail).toContain('carried no widget list'); + // The reason is the body's shape, not the token — no scope misdirection. + expect(res.detail).not.toContain('token needs'); + expect(calls.some((c) => c.method === 'POST')).toBe(false); + }); + + it('a missing result on a later page errors rather than ending the walk quietly', async () => { + const { api, calls } = fakeApi({ + [listPage(1)]: { ok: true, status: 200, result: fullPageOfStrangers('p1') }, + [listPage(2)]: { ok: true, status: 200 } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('carried no widget list'); + expect(calls.some((c) => c.method === 'POST')).toBe(false); + }); + + it('a failure on page 2 reports it the same way a first-page failure does', async () => { + const { api, calls } = fakeApi({ + [listPage(1)]: { ok: true, status: 200, result: fullPageOfStrangers('p1') }, + [listPage(2)]: { ok: false, status: 403 } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('could not list Turnstile widgets'); + expect(res.detail).toContain('token needs'); + expect(res.detail).toContain('Turnstile: Edit'); + // A failed page must never fall through to a create. + expect(calls.some((c) => c.method === 'POST')).toBe(false); + }); + + // An API that ignored `page` would hand back the same full page forever. The fake + // here is a handler rather than a route map so it can do exactly that: every list + // request answers with a full page of strangers, whatever page was asked for. + it('stops after MAX_PAGES when every page comes back full, without creating', async () => { + const calls: Call[] = []; + const api = async ( + token: string, + path: string, + init: { method?: string; body?: unknown } = {} + ): Promise => { + const method = init.method ?? 'GET'; + calls.push({ token, path, method, body: init.body }); + // Trip fast if the page bound is ever removed: without this, an unbounded + // walk only dies by exhausting the heap minutes later, taking the whole + // file's results with it and reading as CI flake instead of a lost bound. + if (calls.filter((c) => c.method === 'GET').length > 25) { + throw new Error('walk exceeded MAX_PAGES — the page bound is gone'); + } + if (method === 'POST') { + return { ok: true, status: 200, result: { sitekey: SITEKEY, secret: WIDGET_SECRET } }; + } + return { ok: true, status: 200, result: fullPageOfStrangers('endless') }; + }; + + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + + // Terminated at the bound instead of hanging. + expect(calls.filter((c) => c.method === 'GET')).toHaveLength(20); + // Ending on a full page is not the end of the list: ours could sit on page 21. + // Creating here would mint a duplicate for the same name+host, after which + // every later run matches whichever one comes back first — so the walk stops + // and says what it could not rule out. + expect(res.status).toBe('error'); + expect(res.detail).toContain('first 20 pages'); + expect(res.detail).toContain('did not end there'); + expect(calls.some((c) => c.method === 'POST')).toBe(false); + }); + + it('a page holding a non-object entry errors instead of throwing', async () => { + const { api, calls } = fakeApi({ + [listPage(1)]: { ok: true, status: 200, result: [null] } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('carried no widget list'); expect(calls.some((c) => c.method === 'POST')).toBe(false); }); + + it('an entry whose domains is not a list never matches', async () => { + const { api, calls } = fakeApi({ + [listPage(1)]: { + ok: true, + status: 200, + result: [{ ...ourWidget, domains: 'akito.dog' }] + }, + [createPath]: { ok: true, status: 200, result: { sitekey: SITEKEY, secret: WIDGET_SECRET } } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + // A short page ended the walk, so absence is proven and the create is right. + expect(res.status).toBe('created'); + expect(calls.filter((c) => c.method === 'POST')).toHaveLength(1); + }); }); describe('provisionTurnstileWidget — clear errors, no mutation', () => { @@ -190,13 +390,53 @@ describe('provisionTurnstileWidget — clear errors, no mutation', () => { }); const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); expect(res.status).toBe('error'); + expect(res.detail).toContain('token needs'); expect(res.detail).toContain('Turnstile: Edit'); // Only the list GET happened — never proceeded to create. expect(calls).toHaveLength(1); expect(calls[0].method).toBe('GET'); }); - it('create call fails → scoped error, sitekey/secret absent', async () => { + it('a 401 list names the scope too — both permission statuses hint', async () => { + const { api } = fakeApi({ + [listPath]: { ok: false, status: 401 } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('HTTP 401'); + expect(res.detail).toContain('token needs'); + expect(res.detail).toContain('Turnstile: Edit'); + }); + + it('a 403 secret read names the scope, and no create fires', async () => { + const { api, calls } = fakeApi({ + [listPath]: { + ok: true, + status: 200, + result: [ourWidget] + }, + [getPath]: { ok: false, status: 403 } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('could not read its secret'); + expect(res.detail).toContain('token needs'); + expect(calls.some((c) => c.method === 'POST')).toBe(false); + }); + + // New contract: a 500 stays bare only when the body carried no usable + // errors (this fixture has none); with errors the reason is appended. + it('a transient list failure (500, no errors body) reports the status without blaming token scope', async () => { + const { api } = fakeApi({ + [listPath]: { ok: false, status: 500 } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('HTTP 500'); + expect(res.detail).not.toContain('token needs'); + }); + + it('create call fails with 403 → scoped error, sitekey/secret absent', async () => { const { api } = fakeApi({ [listPath]: { ok: true, status: 200, result: [] }, [createPath]: { ok: false, status: 403 } @@ -204,17 +444,98 @@ describe('provisionTurnstileWidget — clear errors, no mutation', () => { const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); expect(res.status).toBe('error'); expect(res.detail).toContain('failed to create'); + expect(res.detail).toContain('token needs'); expect(res.sitekey).toBeUndefined(); expect(res.secret).toBeUndefined(); }); - it('create returns ok but a body with no sitekey/secret → error', async () => { + it('create returns ok but a body with no sitekey/secret → error, no scope blame', async () => { const { api } = fakeApi({ [listPath]: { ok: true, status: 200, result: [] }, [createPath]: { ok: true, status: 200, result: {} } }); const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); expect(res.status).toBe('error'); + // The ok create just proved the token's scope, so no scope advice — and + // no claim about whether the widget exists, because we can't know. + expect(res.detail).not.toContain('token needs'); + expect(res.detail).not.toContain('confirm the token'); + expect(res.detail).toContain( + 'the response carried no sitekey/secret, so the widget may exist but setup could not read its keys' + ); + }); + + // cfApi maps a 2xx whose body says success:false to ok=false — a bare + // '(HTTP 200)' would be nonsense, so the detail carries the error summary. + it('a 200 list whose body says success:false repeats the API’s own reason', async () => { + const { api } = fakeApi({ + [listPath]: { + ok: false, + status: 200, + errors: [{ code: 1001, message: 'account is on hold' }] + } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('HTTP 200'); + expect(res.detail).toContain('the API reported failure (1001: account is on hold)'); + expect(res.detail).not.toContain('token needs'); + }); + + it('a 200 secret read whose body says success:false repeats the reason too', async () => { + const { api } = fakeApi({ + [listPath]: { ok: true, status: 200, result: [ourWidget] }, + [getPath]: { ok: false, status: 200, errors: [{ code: 1002, message: 'widget is locked' }] } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('could not read its secret'); + expect(res.detail).toContain('the API reported failure (1002: widget is locked)'); + expect(res.detail).not.toContain('token needs'); + }); + + it('a 200 create whose body says success:false repeats the reason too', async () => { + const { api } = fakeApi({ + [listPath]: { ok: true, status: 200, result: [] }, + [createPath]: { ok: false, status: 200, errors: [{ code: 1003, message: 'quota exceeded' }] } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('failed to create'); + expect(res.detail).toContain('the API reported failure (1003: quota exceeded)'); + expect(res.detail).not.toContain('token needs'); + }); + + it('a 201 whose body says success:false is treated the same as a 200', async () => { + const { api } = fakeApi({ + [listPath]: { ok: false, status: 201, errors: [{ code: 1004, message: 'odd but possible' }] } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('the API reported failure (1004: odd but possible)'); + expect(res.detail).not.toContain('token needs'); + }); + + it('a thrown fetch (status 0) says the API was never reached', async () => { + const { api } = fakeApi({ + [listPath]: { ok: false, status: 0 } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('the Cloudflare API did not respond'); + // The tail already says it; a bare '(HTTP 0)' would be noise. + expect(res.detail).not.toContain('HTTP 0'); + expect(res.detail).not.toContain('token needs'); + expect(res.detail).not.toContain('reported failure'); + }); + + it('a success:false body with no errors says so instead of trailing nothing', async () => { + const { api } = fakeApi({ + [listPath]: { ok: false, status: 200 } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('the API reported failure with no reason given'); }); it('empty domain → error before any network call', async () => { @@ -252,7 +573,7 @@ describe('provisionTurnstileWidget — never leaks the token or the widget secre [listPath]: { ok: true, status: 200, - result: [{ name: WIDGET_NAME, sitekey: SITEKEY, domains: ['akito.dog'] }] + result: [ourWidget] }, [getPath]: { ok: true, status: 200, result: { sitekey: SITEKEY, secret: WIDGET_SECRET } } } @@ -265,4 +586,84 @@ describe('provisionTurnstileWidget — never leaks the token or the widget secre expect(res.secret).toBe(WIDGET_SECRET); } }); + + it('no error detail carries a non-allowlisted error field, the secret, or the sitekey', async () => { + // The allowlisted code+message IS repeated in error details now (that is + // the honest contract); everything else in the body must never be. + const ALLOWLISTED_MESSAGE = 'cf-error-message-expected-in-detail'; + const NON_ALLOWLISTED_MARKER = 'cf-error-extra-field-must-not-leak'; + const apiErrors = [ + { code: 10000, message: ALLOWLISTED_MESSAGE, detail_url: NON_ALLOWLISTED_MARKER } + ]; + // `carriesReason` = the failing route returned an errors body, so the + // detail must repeat its code+message. + const scenarios: { routes: Record; carriesReason?: boolean }[] = [ + // list: scope failure / transient failure + { routes: { [listPath]: { ok: false, status: 403, errors: apiErrors } }, carriesReason: true }, + { routes: { [listPath]: { ok: false, status: 500, errors: apiErrors } }, carriesReason: true }, + // secret read: scope failure / 200 with a blank body + { + routes: { + [listPath]: { ok: true, status: 200, result: [ourWidget] }, + [getPath]: { ok: false, status: 403, errors: apiErrors } + }, + carriesReason: true + }, + // Partial-body branches ignore `errors` entirely — the bodies here make + // the absent-message assertion below load-bearing. + { + routes: { + [listPath]: { ok: true, status: 200, result: [ourWidget] }, + [getPath]: { ok: true, status: 200, result: { sitekey: SITEKEY }, errors: apiErrors } + } + }, + // create: scope failure / 200 with a partial body + { + routes: { + [listPath]: { ok: true, status: 200, result: [] }, + [createPath]: { ok: false, status: 403, errors: apiErrors } + }, + carriesReason: true + }, + { + routes: { + [listPath]: { ok: true, status: 200, result: [] }, + [createPath]: { ok: true, status: 200, result: {}, errors: apiErrors } + } + } + ]; + for (const { routes, carriesReason } of scenarios) { + const { api } = fakeApi(routes); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).not.toContain(NON_ALLOWLISTED_MARKER); + expect(res.detail).not.toContain(WIDGET_SECRET); + expect(res.detail).not.toContain(SITEKEY); + if (carriesReason) expect(res.detail).toContain(`10000: ${ALLOWLISTED_MESSAGE}`); + else expect(res.detail).not.toContain(ALLOWLISTED_MESSAGE); + } + + // 200/success:false: the reported-failure arm repeats the allowlisted + // code+message too, and only the allowlisted fields may appear. + const { api } = fakeApi({ + [listPath]: { + ok: false, + status: 200, + errors: [ + { + code: 9109, + message: ALLOWLISTED_MESSAGE, + detail_url: NON_ALLOWLISTED_MARKER, + account_id: NON_ALLOWLISTED_MARKER + } + ] + } + }); + const res = await provisionTurnstileWidget(TOKEN, ACCT, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain(`9109: ${ALLOWLISTED_MESSAGE}`); + expect(res.detail).not.toContain(NON_ALLOWLISTED_MARKER); + expect(res.detail).not.toContain(WIDGET_SECRET); + expect(res.detail).not.toContain(SITEKEY); + }); }); diff --git a/scripts/turnstile-lib.ts b/scripts/turnstile-lib.ts index 4cd66443..3a6451f2 100644 --- a/scripts/turnstile-lib.ts +++ b/scripts/turnstile-lib.ts @@ -19,7 +19,14 @@ * can be issued for any domain, including one whose DNS lives elsewhere — so the * caller does not gate this on zone resolution, only on having a custom domain. */ -import { cfApi, hostFromDomain } from './setup-lib.ts'; +import { + cfApi, + cfFailureTail, + failureDetail, + hostFromDomain, + statusLabel, + type CfApiResult +} from './setup-lib.ts'; /** * Stable widget name we match on so re-runs find-and-reuse our widget (idempotent) @@ -43,8 +50,25 @@ export const WIDGET_NAME = 'sona-admin-login'; */ export const WIDGET_MODE = 'managed'; -/** The token permission a fork operator must add, quoted verbatim in errors. */ -const SCOPE_HINT = 'Account → Turnstile: Edit'; +/** The token permission a fork operator must add — exported so setup's token + * recipe names the same scope this module's errors do. */ +export const SCOPE_HINT = 'Account → Turnstile: Edit'; + +/** Widgets asked for per list page; also the "was that page full?" test. */ +const PER_PAGE = 50; + +/** + * Stop after this many list pages. Termination normally comes from a short page, + * but an API that ignored `page` would hand back the same full page forever, so + * the loop needs a floor it cannot fall through. 20 pages covers far more widgets + * than any account we provision into holds. + */ +const MAX_PAGES = 20; + +/** setup-lib's shared cfFailureTail, bound to this lib's scope hint. */ +function failureTail(res: CfApiResult): string { + return cfFailureTail(res.status, res.errors, SCOPE_HINT); +} /** A Turnstile widget as returned by the challenges/widgets API. */ interface Widget { @@ -75,9 +99,11 @@ export interface TurnstileResult { * Idempotently provision the admin-login Turnstile widget for `domain`'s host. * * Sequence (all via `cfApi`, Bearer `cfToken`): - * 1. GET /accounts//challenges/widgets → list the account's widgets. A - * non-ok response (401/403 = no Turnstile scope, or a transient error) → a - * clear error naming the missing scope. No mutation. + * 1. GET /accounts//challenges/widgets → list the account's widgets, a + * page at a time until ours turns up or a page comes back short. A non-ok + * response on any page → a clear error whose detail carries the actual reason + * (the scope hint on 401/403, otherwise just the HTTP status), and so does an + * ok page whose body isn't a list at all. No mutation. * 2. Match our widget by its stable `name` (WIDGET_NAME) AND `domains` containing * this fork's host — see WIDGET_NAME on why the host half is required: * - found → GET .../widgets/ to read its secret authoritatively @@ -90,11 +116,11 @@ export interface TurnstileResult { * `api` is injectable (defaults to the real `cfApi`) so tests exercise every branch * without network. Never logs the token; the widget secret appears in no `detail`. * - * Note on matching: the list is read with a generous page size, not paginated. A - * fresh fork's account has at most a handful of widgets, so a single page finds - * ours; the cost of the rare miss is a duplicate widget, never a crash. A miss is - * the only acceptable failure direction here — see WIDGET_NAME on why matching must - * never reuse a widget issued for a different host. + * Note on matching: the list is paginated because a first-page-only read on an + * account holding more than PER_PAGE widgets can miss ours, and a miss is not free + * — the re-run mints a duplicate widget and rewires Pages to its sitekey/secret. + * Missing is still the only acceptable failure direction, though: see WIDGET_NAME + * on why matching must never reuse a widget issued for a different host. */ export async function provisionTurnstileWidget( cfToken: string, @@ -105,28 +131,86 @@ export async function provisionTurnstileWidget( const host = hostFromDomain(domain); if (!host) return { status: 'error', detail: 'no domain given' }; - // 1. List existing widgets; reconcile against ours by stable name. - const listRes = await api(cfToken, `/accounts/${accountId}/challenges/widgets?per_page=50`); - if (!listRes.ok) { - return { - status: 'error', - detail: `could not list Turnstile widgets (HTTP ${listRes.status}); token needs ${SCOPE_HINT}` - }; + // 1. List existing widgets a page at a time; reconcile against ours by stable + // name. Stop at the first match or at a short page (the last one); MAX_PAGES + // full pages means the list never ended, which is an error rather than a miss. + // The explicit sort pins the ordering for the life of the walk: created_on never + // changes, while the API's default order can be recomputed mid-walk and shuffle + // widgets across page boundaries. A concurrent delete can still shift a widget + // onto an already-read page — that miss just mints a duplicate, per above. + let mine: Widget | undefined; + for (let page = 1; page <= MAX_PAGES; page++) { + const listRes = await api( + cfToken, + `/accounts/${accountId}/challenges/widgets?page=${page}&per_page=${PER_PAGE}&order=created_on&direction=asc` + ); + if (!listRes.ok) { + return { + status: 'error', + detail: `could not list Turnstile widgets${failureDetail(listRes, SCOPE_HINT)}` + }; + } + // An ok page whose result isn't a list of widget objects is a partial body. + // Reading it as an empty page would end the walk and mint a duplicate + // widget — the exact failure the paging exists to prevent — so stop and say + // so instead. Entries are checked before the cast: a `[null]` page would + // otherwise throw a TypeError out of the match below. + if ( + !Array.isArray(listRes.result) || + listRes.result.some((w) => typeof w !== 'object' || w === null) + ) { + return { + status: 'error', + detail: `could not list Turnstile widgets${statusLabel(listRes.status)}; the response carried no widget list, so setup stopped rather than risk creating a second widget` + }; + } + const widgets = listRes.result as Widget[]; + // Match on name + host first, THEN insist on a usable sitekey. Folding the + // sitekey test into the match would read a sitekey-less entry as "not ours" + // and walk on to create — but an entry carrying our name and our host IS + // ours, so its absence is disproven and creating would mint a second widget. + const candidate = widgets.find( + (w) => w.name === WIDGET_NAME && Array.isArray(w.domains) && w.domains.includes(host) + ); + if (candidate && (typeof candidate.sitekey !== 'string' || !candidate.sitekey)) { + return { + status: 'error', + detail: `found the ${WIDGET_NAME} Turnstile widget for ${host} but it carried no usable sitekey, so setup stopped rather than risk creating a second widget` + }; + } + mine = candidate; + if (mine || widgets.length < PER_PAGE) break; + // Every page came back full, so this walk never reached the end of the list + // and ours could still sit on page MAX_PAGES + 1. Absence is unproven, and + // creating on an unproven absence mints a second widget for the same + // name+host — after which runs match whichever one the API returns first. + if (page === MAX_PAGES) { + return { + status: 'error', + detail: `could not find the ${WIDGET_NAME} Turnstile widget for ${host} in the first ${MAX_PAGES} pages of the widget list${statusLabel(listRes.status)}; the list did not end there, so setup stopped rather than risk creating a second widget` + }; + } } - const widgets = (listRes.result as Widget[] | undefined) ?? []; - const mine = widgets.find( - (w) => w.name === WIDGET_NAME && w.sitekey && w.domains?.includes(host) - ); // 2a. Reuse: fetch the single widget so we read its secret from the authoritative // GET (matching `wrangler turnstile widget get`, which returns the secret). if (mine?.sitekey) { - const getRes = await api(cfToken, `/accounts/${accountId}/challenges/widgets/${mine.sitekey}`); + // The sitekey comes back from the API, so it is encoded like any other + // untrusted path segment rather than pasted into the URL. + const getRes = await api( + cfToken, + `/accounts/${accountId}/challenges/widgets/${encodeURIComponent(mine.sitekey)}` + ); const secret = (getRes.result as Widget | undefined)?.secret; if (!getRes.ok || !secret) { + // An ok response with no secret is a partial body — the one actionable + // lead is that a read-scoped token gets the widget without its secret. + const why = getRes.ok + ? `; the widget came back without one, so check that the token has ${SCOPE_HINT}` + : failureTail(getRes); return { status: 'error', - detail: `found the ${WIDGET_NAME} widget for ${host} but could not read its secret (HTTP ${getRes.status}); token needs ${SCOPE_HINT}` + detail: `found the ${WIDGET_NAME} widget for ${host} but could not read its secret${statusLabel(getRes.status)}${why}` }; } return { @@ -144,9 +228,15 @@ export async function provisionTurnstileWidget( }); const created = createRes.result as Widget | undefined; if (!createRes.ok || !created?.sitekey || !created?.secret) { + // An ok create with no sitekey/secret is a partial body — the widget most + // likely WAS created, so claim only what we know; the summary's re-run + // line carries the remedy (step 1's name+host match finds and reuses it). + const why = createRes.ok + ? '; the response carried no sitekey/secret, so the widget may exist but setup could not read its keys' + : failureTail(createRes); return { status: 'error', - detail: `failed to create the ${WIDGET_NAME} Turnstile widget for ${host} (HTTP ${createRes.status}); token needs ${SCOPE_HINT}` + detail: `failed to create the ${WIDGET_NAME} Turnstile widget for ${host}${statusLabel(createRes.status)}${why}` }; } return { diff --git a/scripts/typecheck.test.ts b/scripts/typecheck.test.ts index ce014ae1..d3dd0ff4 100644 --- a/scripts/typecheck.test.ts +++ b/scripts/typecheck.test.ts @@ -31,8 +31,10 @@ const compilerOptions: ts.CompilerOptions = { module: ts.ModuleKind.ESNext, moduleResolution: ts.ModuleResolutionKind.Bundler, target: ts.ScriptTarget.ESNext, + // No explicit typeRoots: TypeScript's default walks up parent directories + // for node_modules/@types, so this also works in a git worktree that has no + // node_modules of its own (module resolution already walks up the same way). types: ['node'], - typeRoots: [join(rootDir, 'node_modules', '@types')], paths: { '$app/environment': [join(rootDir, 'vitest-stubs', 'app-environment.ts')], '$lib/*': [join(rootDir, 'src', 'lib', '*')] @@ -47,6 +49,7 @@ describe('scripts/ typecheck', () => { it('every scripts/*.ts file typechecks cleanly', () => { const program = ts.createProgram(entryFiles, compilerOptions); const diagnostics = [ + ...program.getOptionsDiagnostics(), ...program.getSyntacticDiagnostics(), ...program.getSemanticDiagnostics() ]; diff --git a/scripts/waf-lib.test.ts b/scripts/waf-lib.test.ts index 55bcc6c0..221dd53d 100644 --- a/scripts/waf-lib.test.ts +++ b/scripts/waf-lib.test.ts @@ -3,6 +3,8 @@ import type { CfApiResult } from './setup-lib.ts'; import { applyDownloadRateLimit, buildRule, + isPermissionError, + SCOPE_HINT as WAF_SCOPE_HINT, RULE_REF, RULE_DESCRIPTION, RULE_EXPRESSION, @@ -41,6 +43,7 @@ const ZONE = 'zone123'; const RULESET = 'ruleset456'; const zonePath = 'GET /zones?name=akito.dog'; const entryPath = `GET /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`; +const putEntryPath = `PUT /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`; const zoneOk: CfApiResult = { ok: true, status: 200, result: [{ id: ZONE }] }; describe('buildRule', () => { @@ -79,7 +82,7 @@ describe('applyDownloadRateLimit — zone resolves, rule created', () => { [zonePath]: zoneOk, // No http_ratelimit ruleset on the zone yet. [entryPath]: { ok: false, status: 404 }, - [`PUT /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`]: { ok: true, status: 200 } + [putEntryPath]: { ok: true, status: 200 } }); const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); expect(res.status).toBe('created'); @@ -237,14 +240,17 @@ describe('applyDownloadRateLimit — subdomain host resolves via the registrable }); describe('applyDownloadRateLimit — clear errors, no mutation', () => { - it('token has no access to the zone (empty result) → error naming WAF scope, no ruleset touched', async () => { + // An empty zone list is as often a typo or the wrong account as it is a + // permission, so it must not read as a refusal — isPermissionError decides + // whether the runner prints the whole token recipe. + it('an empty zone list says the zone was not found, without claiming a refusal', async () => { const { api, calls } = fakeApi({ [zonePath]: { ok: true, status: 200, result: [] } }); const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); expect(res.status).toBe('error'); - expect(res.detail).toContain('no access to zone akito.dog'); - expect(res.detail).toContain('WAF: Edit'); + expect(res.detail).toContain('no akito.dog zone was found on this Cloudflare account'); + expect(isPermissionError(res)).toBe(false); // Never proceeded to the ruleset endpoint. expect(calls).toHaveLength(1); }); @@ -267,6 +273,8 @@ describe('applyDownloadRateLimit — clear errors, no mutation', () => { const res = await applyDownloadRateLimit(SECRET, 'sub.example.com', api); expect(res.status).toBe('error'); expect(res.detail).toContain('HTTP 500'); + // A 500 is not a permission problem — no scope advice on this branch. + expect(res.detail).not.toContain('token needs'); // The walk stopped at the failing candidate instead of trying example.com. expect(calls.map((c) => c.path)).toEqual(['/zones?name=sub.example.com']); }); @@ -281,6 +289,12 @@ describe('applyDownloadRateLimit — clear errors, no mutation', () => { // Pin the abort semantics too: without this, a walk that stops aborting on // failed lookups still passed this test (found by mutation). expect(res.detail).toContain('HTTP 403'); + // A 403 IS a permission failure — the scope hint must survive here even + // though non-permission statuses dropped it. The scope named is the one + // THIS call needs: the zone lookup is a Zone: Read, not a WAF: Edit. + expect(res.detail).toContain('token needs Zone → Zone: Read'); + expect(res.detail).not.toContain('WAF: Edit'); + expect(isPermissionError(res)).toBe(true); expect(calls).toHaveLength(1); }); @@ -296,6 +310,124 @@ describe('applyDownloadRateLimit — clear errors, no mutation', () => { expect(calls.every((c) => c.method === 'GET')).toBe(true); }); + it('a 401 entrypoint read names the scope too — both permission statuses hint', async () => { + const { api } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 401 } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('HTTP 401'); + expect(res.detail).toContain('token needs'); + expect(res.detail).toContain('WAF: Edit'); + }); + + // New contract: a 500 stays bare only when the body carried no usable + // errors (this fixture has none); with errors the reason is appended. + it('a transient entrypoint failure (500, no errors body) reports the status without blaming token scope', async () => { + const { api, calls } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 500 } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('HTTP 500'); + expect(res.detail).not.toContain('token needs'); + expect(calls.every((c) => c.method === 'GET')).toBe(true); + }); + + it('a thrown fetch (status 0) says the API was never reached', async () => { + const { api } = fakeApi({ + [zonePath]: { ok: false, status: 0 } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('the Cloudflare API did not respond'); + // The tail already says it; a bare '(HTTP 0)' would be noise. + expect(res.detail).not.toContain('HTTP 0'); + expect(res.detail).not.toContain('token needs'); + }); + + it('a 200 zone query whose body says success:false carries the API’s reason', async () => { + // resolveZone threads the failed lookup's errors body through, so this + // arm prints the reason like the ruleset-read and write branches do. + const { api } = fakeApi({ + [zonePath]: { ok: false, status: 200, errors: [{ code: 2003, message: 'zones listing disabled' }] } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('could not query zones'); + expect(res.detail).toContain('the API reported failure (2003: zones listing disabled)'); + expect(res.detail).not.toContain('token needs'); + }); + + it('a 200 entrypoint read whose body says success:false repeats the API’s reason', async () => { + // cfApi maps a 2xx with success:false to ok=false — a bare '(HTTP 200)' + // would be nonsense, so the detail carries the error summary. + const { api } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 200, errors: [{ code: 2001, message: 'zone is on hold' }] } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('HTTP 200'); + expect(res.detail).toContain('the API reported failure (2001: zone is on hold)'); + expect(res.detail).not.toContain('token needs'); + }); + + // Ids come back from the API, so they are encoded like any other untrusted + // path segment — an id carrying a '/' would otherwise rewrite the URL. + it('encodes the ruleset and rule ids it puts in the write path', async () => { + const odd = 'rs/../evil'; + const { api, calls } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { + ok: true, + status: 200, + result: { id: odd, rules: [{ id: 'r/1', ref: RULE_REF }] } + }, + [`PATCH /zones/${ZONE}/rulesets/rs%2F..%2Fevil/rules/r%2F1`]: { ok: true, status: 200 } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('updated'); + const patch = calls.find((c) => c.method === 'PATCH')!; + expect(patch.path).toBe(`/zones/${ZONE}/rulesets/rs%2F..%2Fevil/rules/r%2F1`); + }); + + // WAF Read and WAF Edit are separate permission groups, so a read-only token + // gets through both GETs and is refused exactly on the write. That is the case + // the recipe gate exists for, and nothing above it can prove it. + it('a 403 on the write itself is a permission error, even though both reads passed', async () => { + const { api, calls } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 404 }, + [putEntryPath]: { ok: false, status: 403 } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('failed to write'); + expect(res.detail).toContain('token needs Zone → WAF: Edit'); + expect(isPermissionError(res)).toBe(true); + expect(calls.filter((c) => c.method === 'PUT')).toHaveLength(1); + }); + + // An ok body whose `rules` isn't an array threw `existing.find is not a + // function` — a crash after setup had already written D1, R2 and Pages. It + // stops like turnstile-lib's partial-body arms: an error, and no write. + it('an ok entrypoint whose rules is not an array stops without mutating', async () => { + for (const rules of [{}, 'nope', 42, null]) { + const { api, calls } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: true, status: 200, result: { id: RULESET, rules } } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status, `rules=${JSON.stringify(rules)}`).toBe('error'); + expect(res.detail).toContain('the response carried no rule list'); + expect(isPermissionError(res)).toBe(false); + expect(calls.every((c) => c.method === 'GET')).toBe(true); + } + }); + it('empty domain → error before any network call', async () => { const { api, calls } = fakeApi({}); const res = await applyDownloadRateLimit(SECRET, ' ', api); @@ -305,39 +437,159 @@ describe('applyDownloadRateLimit — clear errors, no mutation', () => { }); describe('applyDownloadRateLimit — never leaks the token', () => { - it('the secret appears in no returned detail across every branch', async () => { - const scenarios: Record[] = [ + it('no returned detail carries a non-allowlisted error field, the secret, or the zone id, across every branch', async () => { + // The allowlisted code+message IS repeated in error details now (that is + // the honest contract); everything else in the body must never be. + const ALLOWLISTED_MESSAGE = 'cf-error-message-expected-in-detail'; + const NON_ALLOWLISTED_MARKER = 'cf-error-extra-field-must-not-leak'; + const apiErrors = [ + { code: 10000, message: ALLOWLISTED_MESSAGE, detail_url: NON_ALLOWLISTED_MARKER } + ]; + // Each scenario pins the branch it exercises via the expected status — + // otherwise route drift would silently collapse them all into the error + // branch and the sweep would stop covering the success details. + // `carriesReason` = the failing route returned an errors body, so the + // detail must repeat its code+message. + const scenarios: { + routes: Record; + status: string; + carriesReason?: boolean; + }[] = [ // error: no zone access - { [zonePath]: { ok: true, status: 200, result: [] } }, + { routes: { [zonePath]: { ok: true, status: 200, result: [] } }, status: 'error' }, // error: zones query failed - { [zonePath]: { ok: false, status: 403 } }, + { + routes: { [zonePath]: { ok: false, status: 403, errors: apiErrors } }, + status: 'error', + carriesReason: true + }, // error: entrypoint scope failure - { [zonePath]: zoneOk, [entryPath]: { ok: false, status: 403 } }, + { + routes: { [zonePath]: zoneOk, [entryPath]: { ok: false, status: 403, errors: apiErrors } }, + status: 'error', + carriesReason: true + }, // error: write failed { - [zonePath]: zoneOk, - [entryPath]: { ok: false, status: 404 }, - [`PUT /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`]: { ok: false, status: 500 } + routes: { + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 404 }, + [putEntryPath]: { + ok: false, + status: 500, + errors: apiErrors + } + }, + status: 'error', + carriesReason: true }, - // success: created + // success: created (no ruleset yet, PUT creates the entrypoint) { - [zonePath]: zoneOk, - [entryPath]: { ok: false, status: 404 }, - [`PUT /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`]: { ok: true, status: 200 } + routes: { + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 404 }, + [putEntryPath]: { ok: true, status: 200 } + }, + status: 'created' + }, + // success: created (ruleset exists, POST appends our rule) + { + routes: { + [zonePath]: zoneOk, + [entryPath]: { ok: true, status: 200, result: { id: RULESET, rules: [] } }, + [`POST /zones/${ZONE}/rulesets/${RULESET}/rules`]: { ok: true, status: 200 } + }, + status: 'created' + }, + // success: updated (our rule present with stale params, PATCH in place) + { + routes: { + [zonePath]: zoneOk, + [entryPath]: { + ok: true, + status: 200, + result: { + id: RULESET, + rules: [{ id: 'mine', ref: RULE_REF, action: 'block', enabled: true, expression: 'stale' }] + } + }, + [`PATCH /zones/${ZONE}/rulesets/${RULESET}/rules/mine`]: { ok: true, status: 200 } + }, + status: 'updated' + }, + // success: exists (our rule already identical — no-op) + { + routes: { + [zonePath]: zoneOk, + [entryPath]: { + ok: true, + status: 200, + result: { + id: RULESET, + rules: [ + { + id: 'mine', + ref: RULE_REF, + action: 'block', + enabled: true, + expression: RULE_EXPRESSION, + ratelimit: { ...RULE_RATELIMIT } + } + ] + } + } + }, + status: 'exists' } ]; - for (const routes of scenarios) { + for (const { routes, status, carriesReason } of scenarios) { const { api } = fakeApi(routes); const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe(status); expect(res.detail).not.toContain(SECRET); + expect(res.detail).not.toContain(ZONE); + expect(res.detail).not.toContain(NON_ALLOWLISTED_MARKER); + if (carriesReason) expect(res.detail).toContain(`10000: ${ALLOWLISTED_MESSAGE}`); + else expect(res.detail).not.toContain(ALLOWLISTED_MESSAGE); } + // The knownZoneId entry point skips the zone walk — its details must be + // just as clean (the caller-provided zone id is the leak candidate here). + const { api } = fakeApi({ + [entryPath]: { ok: false, status: 500, errors: apiErrors } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api, ZONE); + expect(res.status).toBe('error'); + expect(res.detail).not.toContain(SECRET); + expect(res.detail).not.toContain(ZONE); + expect(res.detail).not.toContain(NON_ALLOWLISTED_MARKER); + expect(res.detail).toContain(`10000: ${ALLOWLISTED_MESSAGE}`); + + // 200/success:false: the reported-failure arm repeats the allowlisted + // code+message too, and only the allowlisted fields may appear. + const split = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 404 }, + [putEntryPath]: { + ok: false, + status: 200, + errors: [ + { code: 9110, message: ALLOWLISTED_MESSAGE, detail_url: NON_ALLOWLISTED_MARKER } + ] + } + }); + const splitRes = await applyDownloadRateLimit(SECRET, 'akito.dog', split.api); + expect(splitRes.status).toBe('error'); + expect(splitRes.detail).toContain(`9110: ${ALLOWLISTED_MESSAGE}`); + expect(splitRes.detail).not.toContain(NON_ALLOWLISTED_MARKER); + expect(splitRes.detail).not.toContain(SECRET); + expect(splitRes.detail).not.toContain(ZONE); }); it('passes the token through to cfApi as the first arg (used as Bearer, not in path/body)', async () => { const { api, calls } = fakeApi({ [zonePath]: zoneOk, [entryPath]: { ok: false, status: 404 }, - [`PUT /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`]: { ok: true, status: 200 } + [putEntryPath]: { ok: true, status: 200 } }); await applyDownloadRateLimit(SECRET, 'akito.dog', api); // Token is the first arg on every call; never embedded in a path or body. @@ -350,14 +602,120 @@ describe('applyDownloadRateLimit — never leaks the token', () => { }); describe('applyDownloadRateLimit — write failure', () => { - it('surfaces a scoped error when the create PUT fails', async () => { + it('reports the write failure with its status, without blaming token scope', async () => { + // The token already read the ruleset by this point, so a failed write is + // rarely a scope problem — the old "token needs …" suffix here sent + // operators to fix scopes for what was really an HTTP 500. const { api } = fakeApi({ [zonePath]: zoneOk, [entryPath]: { ok: false, status: 404 }, - [`PUT /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`]: { ok: false, status: 500 } + [putEntryPath]: { ok: false, status: 500 } }); const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); expect(res.status).toBe('error'); expect(res.detail).toContain('failed to write'); + expect(res.detail).toContain('HTTP 500'); + expect(res.detail).not.toContain('token needs'); + }); + + it('a 403 with an errors body appends the attributed API reason after the scope hint', async () => { + // Our advice first, the API's own words attributed after — separable, not + // contradictory, and neither is dropped. + const { api } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 403, errors: [{ code: 10000, message: 'Authentication error' }] } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toMatch(/token needs .*; the API said 10000: Authentication error/); + }); + + it('a 403 write DOES name the scope: WAF Read and WAF Edit are separate groups', async () => { + // A token minted with WAF Read passes the zone query and the entrypoint + // read, then 403s on the first mutation — the most likely real-world + // failure, so it must keep the guidance. + const { api } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 404 }, + [putEntryPath]: { ok: false, status: 403 } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('failed to write'); + expect(res.detail).toContain('HTTP 403'); + expect(res.detail).toContain('token needs'); + expect(res.detail).toContain('WAF: Edit'); + }); + + it('a 200 write whose body says success:false repeats the API’s reason', async () => { + const { api } = fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 404 }, + [putEntryPath]: { + ok: false, + status: 200, + errors: [{ code: 2002, message: 'ruleset limit reached' }] + } + }); + const res = await applyDownloadRateLimit(SECRET, 'akito.dog', api); + expect(res.status).toBe('error'); + expect(res.detail).toContain('failed to write'); + expect(res.detail).toContain('the API reported failure (2002: ruleset limit reached)'); + expect(res.detail).not.toContain('token needs'); + }); +}); + +describe('isPermissionError — the standalone runner’s recipe gate', () => { + // apply-download-ratelimit.ts prints its token recipe only when this returns + // true, so pin it against REAL branch output, both directions. + it('is true for real permission details and false for real transient ones', async () => { + const denied = await applyDownloadRateLimit( + SECRET, + 'akito.dog', + fakeApi({ [zonePath]: zoneOk, [entryPath]: { ok: false, status: 403 } }).api + ); + expect(isPermissionError(denied)).toBe(true); + + // A zone lookup refused outright IS a permission failure, even though the + // scope it names is the zone one rather than WAF. + const zoneDenied = await applyDownloadRateLimit( + SECRET, + 'akito.dog', + fakeApi({ [zonePath]: { ok: false, status: 403 } }).api + ); + expect(isPermissionError(zoneDenied)).toBe(true); + + const transient = await applyDownloadRateLimit( + SECRET, + 'akito.dog', + fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 404 }, + [putEntryPath]: { ok: false, status: 500 } + }).api + ); + expect(isPermissionError(transient)).toBe(false); + }); + + // The gate used to search the formatted detail for the scope hint. Since the + // API's own message is echoed into that same text, a 500 whose body quoted the + // permission read as a refusal and sent the operator to re-mint a working + // token. The result now records what happened, so the wording cannot lie. + it('is false for a 500 whose API message quotes the scope hint verbatim', async () => { + const res = await applyDownloadRateLimit( + SECRET, + 'akito.dog', + fakeApi({ + [zonePath]: zoneOk, + [entryPath]: { ok: false, status: 404 }, + [putEntryPath]: { + ok: false, + status: 500, + errors: [{ code: 1000, message: `internal error; check Zone → WAF: Edit (plus a Zone resource covering the domain)` }] + } + }).api + ); + expect(res.detail).toContain(WAF_SCOPE_HINT); + expect(isPermissionError(res)).toBe(false); }); }); diff --git a/scripts/waf-lib.ts b/scripts/waf-lib.ts index 9a060b55..71bc429e 100644 --- a/scripts/waf-lib.ts +++ b/scripts/waf-lib.ts @@ -12,7 +12,16 @@ * The Cloudflare token is passed in, used only as a Bearer header by `cfApi`, and * never logged or returned in any result. */ -import { cfApi, hostFromDomain, zoneNameCandidates, type CfApiResult } from './setup-lib.ts'; +import { + cfApi, + failureDetail, + hostFromDomain, + isRecord, + statusLabel, + zoneNameCandidates, + ZONE_READ_SCOPE_HINT, + type CfApiResult +} from './setup-lib.ts'; import { resolveZone } from './connect-domains-lib.ts'; /** @@ -123,18 +132,41 @@ export interface RateLimitResult { status: RateLimitStatus; /** Human-readable, secret-free summary safe to print. */ detail: string; + /** + * True when the API refused this call for a token permission (401/403, or a + * zone the token cannot see). Carried as a fact rather than re-derived by + * reading the formatted `detail`, so the standalone runner decides whether to + * print the token recipe from what happened, not from wording that a + * Cloudflare message could coincidentally echo. + */ + permissionDenied?: boolean; } -/** The token permission a fork operator must add, quoted verbatim in errors. */ -const SCOPE_HINT = 'Zone → WAF: Edit (plus a Zone resource covering the domain)'; +/** Whether an HTTP status means the token was refused, as opposed to any other failure. */ +const deniedByScope = (status: number): boolean => status === 401 || status === 403; + +/** The token permission a fork operator must add, quoted verbatim in errors — + * exported so the notation drift guard can check the resolved string. */ +export const SCOPE_HINT = 'Zone → WAF: Edit (plus a Zone resource covering the domain)'; + +/** + * True when the run failed because the token was refused. Reads the fact the + * call recorded rather than searching the formatted `detail` for the scope + * hint: the API's own message is echoed into that text, so a Cloudflare + * message quoting the permission would otherwise be read as a refusal. + */ +export function isPermissionError(res: Pick): boolean { + return res.permissionDenied === true; +} /** * Idempotently apply the public-endpoint rate-limit rule to `domain`'s zone. * * Sequence (all via `cfApi`, Bearer `cfToken`): * 1. GET /zones?name= → resolve the zone id (host derived from - * the domain input; scheme/path stripped). No zone in the account, or the - * token can't see it, → a clear error naming the missing scope. No mutation. + * the domain input; scheme/path stripped). A failed lookup names Zone → + * Zone: Read, the scope THIS call needs; an empty list says the zone wasn't + * found, which is as much a wrong domain as a permission. No mutation. * 2. GET /zones//rulesets/phases/http_ratelimit/entrypoint → the zone's * rate-limit ruleset. 404 = no ruleset yet (fine — we create it). 401/403 or * other non-ok = token lacks WAF scope → clear error, no mutation. @@ -164,74 +196,107 @@ export async function applyDownloadRateLimit( // caller already resolved the zone (the setup CLI's preflight just did). let zoneId = knownZoneId; if (!zoneId) { - const { zone, errorStatus, failedName } = await resolveZone(zoneNameCandidates(host), (name) => - api(cfToken, `/zones?name=${encodeURIComponent(name)}`) + const { zone, errorStatus, errors, failedName } = await resolveZone( + zoneNameCandidates(host), + (name) => api(cfToken, `/zones?name=${encodeURIComponent(name)}`) ); if (errorStatus !== null) { + // The zone lookup is a Zone → Zone: Read call, not a WAF one — naming the + // WAF scope here sends the operator to fix a permission this step never + // needed. return { status: 'error', - detail: `could not query zones for ${failedName ?? host} (HTTP ${errorStatus}); token needs ${SCOPE_HINT}` + detail: `could not query zones for ${failedName ?? host}${failureDetail({ status: errorStatus, errors }, ZONE_READ_SCOPE_HINT)}`, + permissionDenied: deniedByScope(errorStatus) }; } zoneId = zone.id; } if (!zoneId) { + // An empty zone list is two different stories the API can't tell apart: the + // domain isn't on this account (a typo, or the wrong account), or the token + // can't see it. permissionDenied stays UNSET so the runner doesn't print the + // whole token recipe for what is most often a wrong domain. return { status: 'error', - detail: `token has no access to zone ${host}: add ${SCOPE_HINT}` + detail: `no ${host} zone was found on this Cloudflare account — check the domain, the account, and that the token's Zone Resources include it` }; } // 2. Read the zone's http_ratelimit entrypoint ruleset. - const entry = await api(cfToken, `/zones/${zoneId}/rulesets/phases/http_ratelimit/entrypoint`); + const entry = await api(cfToken, `/zones/${encodeURIComponent(zoneId)}/rulesets/phases/http_ratelimit/entrypoint`); let rulesetId: string | undefined; let existing: ExistingRule[] = []; if (entry.ok) { - const r = entry.result as { id?: string; rules?: ExistingRule[] } | undefined; + const r = entry.result as { id?: string; rules?: unknown } | undefined; + // An ok body whose `rules` is present but not an array is a partial body, not + // an empty ruleset: reading it as [] would append a second copy of our rule, + // and calling .find on it threw outright — after setup had already written D1, + // R2 and Pages. Stop the way turnstile-lib stops, without mutating. + if (r?.rules !== undefined && !Array.isArray(r.rules)) { + return { + status: 'error', + detail: `could not read the rate-limit ruleset for ${host}${statusLabel(entry.status)}; the response carried no rule list, so the rule was not written` + }; + } rulesetId = r?.id; - existing = r?.rules ?? []; + existing = (r?.rules ?? []) as ExistingRule[]; } else if (entry.status !== 404) { // 401/403 (no WAF scope) or a transient error — do NOT mutate. return { status: 'error', - detail: `could not read the rate-limit ruleset for ${host} (HTTP ${entry.status}); token needs ${SCOPE_HINT}` + detail: `could not read the rate-limit ruleset for ${host}${failureDetail(entry, SCOPE_HINT)}`, + permissionDenied: deniedByScope(entry.status) }; } // 3. Reconcile against any rule we already own. Match on our stable ref ONLY — // not the human description — so we never PATCH an operator's own rule that // merely happens to share the label. - const mine = existing.find((r) => r.ref === RULE_REF); + // The entry guard matters as much as the ref match: a non-object in the zone's + // rules would throw on the property read, turning someone else's odd ruleset + // into a crash instead of a rule we simply don't own. + const mine = existing.find((r) => isRecord(r) && r.ref === RULE_REF); if (mine && ruleMatches(mine)) { return { status: 'exists', detail: `rate-limit rule already present on ${host} — no change` }; } + // The ruleset and rule ids come back from the API, so they are encoded like any + // other untrusted path segment rather than pasted into the URL. const write: CfApiResult = await (async () => { if (mine && rulesetId && mine.id) { // Param bump: update just our rule in place. - return api(cfToken, `/zones/${zoneId}/rulesets/${rulesetId}/rules/${mine.id}`, { - method: 'PATCH', - body: buildRule() - }); + return api( + cfToken, + `/zones/${encodeURIComponent(zoneId)}/rulesets/${encodeURIComponent(rulesetId)}/rules/${encodeURIComponent(mine.id)}`, + { + method: 'PATCH', + body: buildRule() + } + ); } if (rulesetId) { // Ruleset exists, our rule is absent: append only our rule. - return api(cfToken, `/zones/${zoneId}/rulesets/${rulesetId}/rules`, { + return api(cfToken, `/zones/${encodeURIComponent(zoneId)}/rulesets/${encodeURIComponent(rulesetId)}/rules`, { method: 'POST', body: buildRule() }); } // No http_ratelimit ruleset yet: create the entrypoint with our rule. - return api(cfToken, `/zones/${zoneId}/rulesets/phases/http_ratelimit/entrypoint`, { + return api(cfToken, `/zones/${encodeURIComponent(zoneId)}/rulesets/phases/http_ratelimit/entrypoint`, { method: 'PUT', body: { rules: [buildRule()] } }); })(); if (!write.ok) { + // A 401/403 here is real even after the reads passed: WAF Read and WAF + // Edit are separate permission groups, so a Read-only token 403s exactly + // on this write. Any other status gets no scope advice. return { status: 'error', - detail: `failed to write the rate-limit rule to ${host} (HTTP ${write.status}); token needs ${SCOPE_HINT}` + detail: `failed to write the rate-limit rule to ${host}${failureDetail(write, SCOPE_HINT)}`, + permissionDenied: deniedByScope(write.status) }; } return mine