fix(setup): honest, identifier-safe error reporting across the setup CLIs

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
 

UPDATING.md

@@ -94,8 +94,9 @@ Run this once per fork, from a clone:
 CLOUDFLARE_API_TOKEN=<token> npm run apply-download-ratelimit -- <domain>
 ```
 
-`<domain>` 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
+`<domain>` 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.

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<string, CfApiResult>) =>
+	async (_token: string, path: string, init: { method?: string } = {}): Promise<CfApiResult> =>
+		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 -- <domain>');
+	});
+});

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=<token> npm run apply-download-ratelimit -- <domain>';
 
+/**
+ * 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=<token> npm run apply-download-ratelimit -- <domain>');
+	}
+	return lines;
+}
+
 async function main(): Promise<number> {
 	console.log('— Sona apply-download-ratelimit —\n');
 
@@ -57,16 +80,18 @@ async function main(): Promise<number> {
 			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);
+		});
+}

scripts/connect-domains-lib.test.ts

@@ -10,14 +10,16 @@ import {
 	cdnDomainState,
 	bucketDomainTlsIssued,
 	pagesDomainAttached,
+	pagesDomainState,
 	classifyCdnProbe,
 	planConnect,
 	siteUrlMismatch,
 	buildLadder,
 	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<LadderInputs>) =>
+		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<LadderInputs>) =>
+		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([]);
 	});
 });